Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RemoveDIs][NFC] Fix rotten green C API test #92362

Merged
merged 4 commits into from
May 16, 2024
Merged

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented May 16, 2024

llvm_test_dibuilder(/*NewDebugInfoMode=*/true) isn't currently executed in
return llvm_test_dibuilder(false) && llvm_test_dibuilder(true);
because llvm_test_dibuilder returns 0 for success.

Split the llvm-c-test flag --test-dibuilder into two, one for the old and
one for the new debug informat. Add another lit test for the new mode.

Now that the test actually runs, it crashes using the new format with
llvm/lib/IR/LLVMContextImpl.cpp:53:llvm::LLVMContextImpl::~LLVMContextImpl(): Assertion 'TrailingDbgRecords.empty() && "DbgRecords in blocks not cleaned"' failed. Aborted.

Insert terminators into the blocks so that we don't leave the debug records
trailing, unattached to any instructions, which fixes that.

@OCHyams OCHyams assigned jryans and SLTozer and unassigned jryans and SLTozer May 16, 2024
@OCHyams OCHyams requested review from jryans and SLTozer May 16, 2024 08:29
Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment: in some places you use mode but in others you use format.
I would suggest to have consistent terms and options, based on the format term:

new debuginfo format.
--test-dibuilder-debuginfo-new-format
--test-dibuilder-debuginfo-old-format
debug_info_new_format.ll

@@ -215,10 +215,26 @@ int llvm_test_dibuilder(bool NewDebugInfoFormat) {

LLVMDIBuilderFinalize(DIB);

// In the new debug mode, debug records get attached to instructions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code depending on the given NewDebugInfoFormat?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it'd be better to minimise the difference between the tests. Adding the instructions into the old-format test doesn't hurt it IMO. In a future patch I will possibly be adding some NewDebugInfoFormat-specific behaviour, but I think minimising the difference is still worthwhile. What do you think?

Copy link
Member

@jryans jryans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me! 😄

Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@OCHyams OCHyams merged commit 9112073 into llvm:main May 16, 2024
3 of 4 checks passed
@OCHyams
Copy link
Contributor Author

OCHyams commented May 16, 2024

Thanks both

qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Jun 5, 2024
…085055ecb

Local branch amd-gfx 3ae0850 Merged main:ba2e4fe4e7f79e49fcac54ea20f5b899dc687cfc into amd-gfx:c19a0f2d1e97
Remote branch main 9112073 [RemoveDIs][NFC] Fix rotten green C API test (llvm#92362)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants