Page MenuHomePhabricator

[Debug-Info][NFC] add -gstrict-dwarf support in backend
ClosedPublic

Authored by shchenz on Apr 20 2021, 1:32 AM.

Details

Summary

we add -gstrict-dwarf support in FE in D100809, this patch is to add the same support in BE, it contains:
1: supporting passing the strict dwarf flags to TargetOptions;
2: adding a debug option -strict-dwarf to backend components, like llc.

Diff Detail

Event Timeline

shchenz created this revision.Apr 20 2021, 1:32 AM
shchenz requested review of this revision.Apr 20 2021, 1:32 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 20 2021, 1:32 AM
probinson accepted this revision.Apr 20 2021, 10:04 AM

You have a clang-format warning, aside from that LGTM.

This revision is now accepted and ready to land.Apr 20 2021, 10:04 AM

Do you know if you're going to be using LTO with DBX? If so, to respect this flag, it would need to be added to LLVM IR module metadata (like the Dwarf Version and Debug Info Version fields) rather than as an MCOption. (we're pretty wishy-washy about what things we decide have to be carried through LTO (& thus go in the IR metadata) and what things we're OK saying "eh, this'll be set on MCOptions or in a -mllvm flag and you'll have to pass the flag to your link step if you want this functionality in an LTO build") - so there's no hard line here)

If you stick with the MCOption, it'll need to be plumbed into llc as a flag there, and should be tested (might be that the whole functionality of the llc flag, MCOption to lower it to, and its use in LLVM's debug info emission should go together in one patch - I'm not sure)

A general architectural issue too: I think the other patches that are using this functionality for specific features should be abandoned if the following alternative is possible: Could the code for adding attributes be modified to check the version of the attribute and omit it if the in-use version doesn't support the attribute (& strict DWARF is enabled)? (is it only attributes you're dealing with, or also tags? I would guess for tags the architectural changes might be a bit more difficult, but I think would still be worth considering such a general solution)

Do you know if you're going to be using LTO with DBX? If so, to respect this flag, it would need to be added to LLVM IR module metadata (like the Dwarf Version and Debug Info Version fields) rather than as an MCOption. (we're pretty wishy-washy about what things we decide have to be carried through LTO (& thus go in the IR metadata) and what things we're OK saying "eh, this'll be set on MCOptions or in a -mllvm flag and you'll have to pass the flag to your link step if you want this functionality in an LTO build") - so there's no hard line here)

If you stick with the MCOption, it'll need to be plumbed into llc as a flag there, and should be tested (might be that the whole functionality of the llc flag, MCOption to lower it to, and its use in LLVM's debug info emission should go together in one patch - I'm not sure)

A general architectural issue too: I think the other patches that are using this functionality for specific features should be abandoned if the following alternative is possible: Could the code for adding attributes be modified to check the version of the attribute and omit it if the in-use version doesn't support the attribute (& strict DWARF is enabled)? (is it only attributes you're dealing with, or also tags? I would guess for tags the architectural changes might be a bit more difficult, but I think would still be worth considering such a general solution)

Hi David @dblaikie , thanks very much for the kind reminder about the LTO part. Yes, we need to consider the LTO part on AIX for debugging. But we can not fix this issue upstream as we did much downstream change for LTO on AIX, so I have to fix this downstream. And I will pass flag(-strict-dwarf=true) to llc, so we don't need to do any other changes in llc after this patch is committed.

And for the general architectural issue: yes, agreed. For now, most issues related to the strict dwarf are for attributes, this should be easy to control under a centralized interface, for example in addValue function of DIEValueList class.
But we also found 1 issue related to tags, as you saw in D100630, unlike attributes, we should normally not ignore the TAGs, especially when there is a replacement tag in the lower version DWARF.
We also found 1 issue related to location expression, (DW_OP_stack_value is generated at -gdwarf-3), I am still working on how to fix this.
I am thinking just adding/modifying a centralized interface for attributes for now, then patches D100440 D100629 D100628 should be abandoned. And let the tags related patch D100630 be unchanged. What do you think? @dblaikie @probinson

Do you know if you're going to be using LTO with DBX? If so, to respect this flag, it would need to be added to LLVM IR module metadata (like the Dwarf Version and Debug Info Version fields) rather than as an MCOption. (we're pretty wishy-washy about what things we decide have to be carried through LTO (& thus go in the IR metadata) and what things we're OK saying "eh, this'll be set on MCOptions or in a -mllvm flag and you'll have to pass the flag to your link step if you want this functionality in an LTO build") - so there's no hard line here)

If you stick with the MCOption, it'll need to be plumbed into llc as a flag there, and should be tested (might be that the whole functionality of the llc flag, MCOption to lower it to, and its use in LLVM's debug info emission should go together in one patch - I'm not sure)

A general architectural issue too: I think the other patches that are using this functionality for specific features should be abandoned if the following alternative is possible: Could the code for adding attributes be modified to check the version of the attribute and omit it if the in-use version doesn't support the attribute (& strict DWARF is enabled)? (is it only attributes you're dealing with, or also tags? I would guess for tags the architectural changes might be a bit more difficult, but I think would still be worth considering such a general solution)

Hi David @dblaikie , thanks very much for the kind reminder about the LTO part. Yes, we need to consider the LTO part on AIX for debugging. But we can not fix this issue upstream as we did much downstream change for LTO on AIX, so I have to fix this downstream. And I will pass flag(-strict-dwarf=true) to llc, so we don't need to do any other changes in llc after this patch is committed.

Fair enough - please add a test for the -strict-dwarf flag. (If you want to split this patch from the one that adds any strict-dwarf functionality, then maybe the llc -strict-dwarf test should include something that's expected to have different behavior with CHECKs showing the incorrect behavior and a FIXME explaining what should happen - then in a follow-up patch that behavior would be implemented, the CHECKs updated and the FIXME removed)

And for the general architectural issue: yes, agreed. For now, most issues related to the strict dwarf are for attributes, this should be easy to control under a centralized interface, for example in addValue function of DIEValueList class.
But we also found 1 issue related to tags, as you saw in D100630, unlike attributes, we should normally not ignore the TAGs, especially when there is a replacement tag in the lower version DWARF.
We also found 1 issue related to location expression, (DW_OP_stack_value is generated at -gdwarf-3), I am still working on how to fix this.
I am thinking just adding/modifying a centralized interface for attributes for now, then patches D100440 D100629 D100628 should be abandoned. And let the tags related patch D100630 be unchanged. What do you think? @dblaikie @probinson

Sounds alright.

shchenz updated this revision to Diff 339470.Apr 21 2021, 10:04 PM

1: add NFC testcases to use -gstrict-dwarf option

dblaikie accepted this revision.Apr 22 2021, 8:18 AM

Sounds good - probably only need one test file, rather than two - can put a bunch of strict-dwarf related testing in that one file (& honestly, maybe don't even need to test every DWARF attribute gets the right handling if the handling is generic/applies across all the attributes)

shchenz updated this revision to Diff 339840.Apr 22 2021, 7:13 PM

1: put test cases in single file

This revision was automatically updated to reflect the committed changes.