If the DW_AT_addr_base is not set, dsymutil should not copy any addrx attributes into the cloned DIE, which will result in no .debug_addr section being emitted.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I am not sure how to test this, how do I modify the compile unit in my test to not have a DW_AT_addr_base in it?
Comment Actions
The test could be written using yaml2obj tool. I attached an example of such test. It contains two compile units one is good and another one is missing DW_AT_addr_base(the test passes on current llvm and should be adapted for D154638).
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 1372 | No need to check for DW_AT_addr_base specifically here. Just checking for std::nullopt should be enough: if (!Addr) {
Linker.reportWarning("Cann't read address attribute value.", ObjFile);
return 0;
} | |
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 1372 | That is incorrect, if we just check for std::nullopt, we will fail llvm/test/tools/dsymutil/X86/verify.test This is because, that test adds broken dwarf where a duplicate DW_AT_language has the form DW_FORM_addr, and if we don't add that form to the Die, then the verify pass doesn't catch the error. | |
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 1372 | This is exactly wanted behavior - dsymutil removed broken attribute and so verifier does not report the error. We need to modify /tools/dsymutil/X86/verify.test so that it uses another kind of error. f.e. it might add DW_AT_language that has DW_FORM_ref_addr form or something like this. | |
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 1372 | Hi @avl I respectfully disagree, in this specific case dsymutil removed a broken attribute even though we tried to verify input dwarf, I thought the whole point of the -verify pass was to let us know if any input dwarf was invalid. Which we will not know in this case. Any thoughts @JDevlieghere @aprantl ? | |
| 1372 | I am happy to modify verify.test if that is the consensus, but I am just giving my 2 cents on the matter at hand currently. | |
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 1372 | Is attribute really removed while input DWARF verified? I think that we verify input DWARF before modifications done by dsymutil. | |
Comment Actions
The purpose of the verifier in dsymutil is twofold:
- Catching invalid input DWARF.
- Making sure we don't turn valid input DWARF into invalid output DWARF.
Testing the former is easy, but without any known bugs in dsymutil, testing the latter is tricky. So instead of crafting something that trips up dsymutil, the test for the verifier feeds invalid DWARF into dsymutil, relies on the fact that dsymutil doesn't know how to fix it, and as a result emits invalid DWARF, which the verifier then catches. If I understand correctly, the existing verify test is relying on something dsymutil wasn't able to fix up, but after @rastogishubham's patch, dsymutil is now smart enough to fix it, so the test fails. I think the solution here is to pick a different way to make the input DWARF invalid that dsymutil isn't able to fix.
Comment Actions
Awesome! Thanks @JDevlieghere and @avl I will change verify.test to make sure there is something in there dsymutil cannot fixup.
Comment Actions
Hi @avl Thanks for all the reviews, if you could give me the okay for this patch, I can push the whole stack. Thank you!
Comment Actions
LGTM assuming inline comment is addressed.
| llvm/lib/DWARFLinker/DWARFLinker.cpp | ||
|---|---|---|
| 2589 | this check is redundant. emitDebugAddrSection already checks for AddrPool.Addrs.empty(). it can be removed. | |
No need to check for DW_AT_addr_base specifically here. Just checking for std::nullopt should be enough:
if (!Addr) { Linker.reportWarning("Cann't read address attribute value.", ObjFile); return 0; }