This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][DWARF] Fix TU Index handling for DWARF4/5
ClosedPublic

Authored by ayermolo on May 20 2022, 12:56 PM.

Details

Summary

When we generate split dwarf with -fdebug-types-section we will have
.debug_types.dwo sections. These go into TU Index when we run llvm-dwp. BOLT was
not handling DWP input correctly with this section.

Added support for handling DWP with TU Index as an input and output for DWARF4.
Added support for handling DWP with TU Index as an input for DWARF5

Diff Detail

Event Timeline

ayermolo created this revision.May 20 2022, 12:56 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: hoy, modimo, wenlei and 2 others. · View Herald Transcript
ayermolo requested review of this revision.May 20 2022, 12:56 PM
Herald added a project: Restricted Project. · View Herald Transcript
ayermolo updated this revision to Diff 431467.May 23 2022, 1:16 PM

missed llvm-dwp

Added support for handling it when DWP output or DWP is output for DWARF4. For DWARF5 it just handles it as an input for now.

Could you please rephrase?

bolt/include/bolt/Rewrite/DWARFRewriter.h
31

Or DebugTypesSignaturesPerCUMap.

bolt/lib/Rewrite/DWARFRewriter.cpp
738

If you are validating the input here, then it's better to issue an error as llvm_unreachable() has no effect in no-assert builds.

1261
1263

Could you add detailed comments? E.g., it's unclear which parameters are input and which are output.

1291

Is reinterpret_cast<> necessary here? Does StringRef(DWOTUSection) not work? Do you actually need to return StringRef from the function, as it's always a reference to DWOTUSection which is a parameter to the function?

1405

Should this be an error instead?

1511
1622

Likewise, should this be an error?

ayermolo added inline comments.May 23 2022, 5:10 PM
bolt/include/bolt/Rewrite/DWARFRewriter.h
31

oh yeah, that's better. thx

bolt/lib/Rewrite/DWARFRewriter.cpp
738

Ah right forgot about that. K, will change to llvm::errs() << "BOLT-ERROR: " ?

1291

This avoids a bug where a null byte is interpreted as the end of string.
I thought returning StringRef would be cleaner then putting this logic on the callee side.

1405

llvm::errs() << "BOLT-ERROR: "
exit(1);
?

1622

Because we are validating input? Way I thought about it, is that it's not a common path, and more of a sanity check so assert would be more appropriate.

maksfb added inline comments.May 23 2022, 5:24 PM
bolt/lib/Rewrite/DWARFRewriter.cpp
738

What's the desired action? Do you want to terminate the processing? Alternatively, you can return an error and handle it from a caller.

1291

Sorry, I don't follow. Where is null-byte bug manifesting itself? Both, std::string and StringRef allow null characters, right?

1622

If you are validating an internal assumption, then it's fine to leave the assert() as is.

ayermolo added inline comments.May 23 2022, 5:34 PM
bolt/lib/Rewrite/DWARFRewriter.cpp
738

yes, I think we should terminate process.

1291

I ran into this issue in updateDebugData. Don't quite remember all the details, but when doing conversion from std::string to StringRef using StringRef(std::string) semantics if there were null bytes in std::string it will think that's the end of the string, so StringRef will get the length wrong. Thus StringRef will only capture part of the string.

1622

Yep, exactly.

zr33 added a subscriber: zr33.May 23 2022, 8:55 PM
ayermolo edited the summary of this revision. (Show Details)May 24 2022, 1:03 PM
ayermolo marked 11 inline comments as done.May 24 2022, 2:09 PM
ayermolo updated this revision to Diff 431791.May 24 2022, 2:13 PM
ayermolo edited the summary of this revision. (Show Details)

Addressing comments.

ayermolo updated this revision to Diff 431793.May 24 2022, 2:23 PM

formating

ayermolo updated this revision to Diff 432133.May 25 2022, 2:39 PM

refactored extractDWOTUFromDWP

maksfb added inline comments.May 26 2022, 11:31 AM
bolt/lib/Rewrite/DWARFRewriter.cpp
738
1266

Should this be const?

1316
1334–1336
1405
ayermolo marked 4 inline comments as done.May 31 2022, 11:09 AM
ayermolo marked an inline comment as done.May 31 2022, 11:13 AM
ayermolo added inline comments.
bolt/lib/Rewrite/DWARFRewriter.cpp
1316

If this is const compiler errors out on
for (uint64_t Val : TypeSignaturesPerCU[DWOId])

ayermolo updated this revision to Diff 433148.May 31 2022, 11:14 AM

addressing comments

ayermolo retitled this revision from [BOLT][DWARF] Fix TU Index handling for DWARF4/5. to [BOLT][DWARF] Fix TU Index handling for DWARF4/5.May 31 2022, 5:24 PM
ayermolo updated this revision to Diff 433248.May 31 2022, 5:46 PM

changed type unordered map to const.

ayermolo updated this revision to Diff 433268.May 31 2022, 6:28 PM

update with arc diff git merge-base HEAD origin --update D126087

ayermolo updated this revision to Diff 433583.Jun 1 2022, 3:57 PM

rebase ontop of Fix LIT tests on Windows VS2019

maksfb accepted this revision.Jun 1 2022, 5:17 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Jun 1 2022, 5:17 PM
This revision was automatically updated to reflect the committed changes.