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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
