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] Do not load into new debug info format from bitcode by default #86268

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Mar 22, 2024

This is NFC right now, as the global default behaviour is also "do not load into the new debug info format by default", but we want to change that soon.

Additionally unconditionally convert from the new debug info format into if we've loaded into it (e.g., if the bitcode file loaded was already in the new format).

The latter change is needed because verify-uselistorder doesn't yet understand DbgRecords (it doesn't know how to map them).

The former change is needed because if we load from an old debug format bitcode file but load directly into the new format and then convert back to the old mode after, the use-lists of the debug intrinsic functions (the functions' global value uses) change.

…fault

This is NFC right now, as the default behaviour is "do not load into the
new debug info format by default", but we want to change that soon.

Additionally unconditionally convert from the new debug info format into if
we've loaded into it (e.g., if the bitcode file loaded was already in the new
format).

The latter change is needed because verify-uselistorder doesn't yet understand
DbgRecords (it doesn't know how to map them).

The former change is needed because if we load from an old debug format bitcode
file but load directly into the new format _and then convert back to the old
mode after_, the use-lists of the debug intrinsic functions (the functions'
global value uses) change.
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

Why the extra 'r' first in DbgInforFormat? Presumably the intention behind this so that we can widen the scope of RemoveDIs while keeping it turned off for verify-uselistorder?

@OCHyams
Copy link
Contributor Author

OCHyams commented Mar 22, 2024

Why the extra 'r' first in DbgInforFormat?.

Oops, a typo in the patch that added the flag. I'll fix that separately.

Presumably the intention behind this so that we can widen the scope of RemoveDIs while keeping it turned off for verify-uselistorder?

Yep, I'm going to upload more patches that enable the bitcode loading by default in specific tools (e.g. #86271). The intention is that once those stick for a while we can make it the global default.

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.

Looks reasonable overall following discussion in the PR, thanks!

@@ -68,6 +68,7 @@ static cl::opt<unsigned>
cl::desc("Number of times to shuffle and verify use-lists"),
cl::init(1), cl::cat(Cat));

extern cl::opt<cl::boolOrDefault> LoadBitcodeIntoNewDbgInforFormat;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Probably better to keep a blank line after this

@OCHyams OCHyams merged commit 2ef6120 into llvm:main Mar 25, 2024
3 of 4 checks passed
@OCHyams OCHyams deleted the ddd-bitcode-verify-uselistorder-skip branch March 25, 2024 09:33
@mstorsjo
Copy link
Member

This is causing failed builds here:

ld.lld: error: undefined symbol: LoadBitcodeIntoNewDbgInforFormat
>>> referenced by verify-uselistorder.cpp
>>>               tools/verify-uselistorder/CMakeFiles/verify-uselistorder.dir/verify-uselistorder.cpp.o:(main)
>>> did you mean: LoadBitcodeIntoNewDbgInfoFormat
>>> defined in: lib/libLLVMBitReader.a(BitcodeReader.cpp.o)
collect2: error: ld returned 1 exit status

I pushed a fix in 336bdf1.

@OCHyams
Copy link
Contributor Author

OCHyams commented Mar 25, 2024

Thanks very much for sorting that out - I renamed the variable (to fix the typo), but didn't remember to update this PR before pushing.

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.

None yet

4 participants