[PATCH] D157669: Fix spurious errors that would be emitted when DW_TAG_subprogram DIEs had mutliple ranges in DW_AT_ranges.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp:212
+  for (const DWARFAddressRange &DwarfRange : DwarfRanges) {
+    if (DwarfRange.LowPC < DwarfRange.HighPC)
+      Ranges.insert({DwarfRange.LowPC, DwarfRange.HighPC});
----------------
clayborg wrote:
> dblaikie wrote:
> > We've got a utility for checking for tombstones, perhaps that should be used here? (I guess this check is essentially for detecting tombstoned ranges?)
> The normal path as clang uses zero as the tombstone value, so that will mean the LowPC will always be zero, but it could be -1, which would omit it here, which is ok. We are just making sure we don't put invalid ranges, including empties into the returned ranges. Any tombstone values are actually removed by checking if they exist in the TextRanges that the GsymCreator contains, so if an address is tombstoned to either zero or to -1, those ranges won't fall into the actual address ranges of the sections with execute permissions. Checking if functions exist inside one of the TextRanges is the main way we remove invalid address ranges in the GSYM conversion process.
> Checking if functions exist inside one of the TextRanges is the main way we remove invalid address ranges in the GSYM conversion process.

Not sure I follow (I don't understand a lot about GSYM's implementation) - but it'd be good to check for tombstones only in one place, rather than multiple. Could this checking here subsume/replace the checking done in TextRanges, or the other way around? (could the checking done in TextRanges be made more robust/modified in some way so as to not need this checking here)

(& the tombstoning checks in libDebugInfoDWARF only check for the -1 "official" tombstone, since 0 might be a valid address on some architectures so can't be used as a clear proof of tombstoning... so these checks are necessarily a bit non-portable, etc - all the more reason to make them in as few places as possible to make it clearer rather than having them somewhat quietly showing up in different places)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157669