[PATCH] D100826: [Debug-Info][NFC] add -gstrict-dwarf support in backend

Wed Apr 21 20:37:00 PDT 2021

dblaikie added a comment.

In D100826#2707145 <https://reviews.llvm.org/D100826#2707145>, @shchenz wrote:

> In D100826#2702135 <https://reviews.llvm.org/D100826#2702135>, @dblaikie wrote:
>
>> 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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D100440> D100629 <https://reviews.llvm.org/D100629> D100628 <https://reviews.llvm.org/D100628> should be abandoned. And let the tags related patch D100630 <https://reviews.llvm.org/D100630> be unchanged. What do you think? @dblaikie @probinson

Sounds alright.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100826/new/

https://reviews.llvm.org/D100826