This is an archive of the discontinued LLVM Phabricator instance.

Fix spurious errors that would be emitted when DW_TAG_subprogram DIEs had mutliple ranges in DW_AT_ranges.
ClosedPublic

Authored by clayborg on Aug 10 2023, 6:22 PM.

Details

Summary

llvm-gsymutil would emit errors about address ranges for DW_TAG_inlined_subroutine DIEs whose address range didn't exist in the parent inline information. When a DW_TAG_subprogram DIE has more than one address range with a DW_AT_ranges attribute, we emit multiple FunctionInfo objets, one for each range of a function. When we parsed the inline information, it might have inline contribution that appear in any of the function's ranges, and if we were parsing the first range of a function, all inline entries that appeared in other valid ranges of the functions would end up emitting error messages. This patch fixes this by always passing down the full list of ranges, even if they aren't being used in the parse of the information. This eliminates reporting of errors when we shouldn't have been emitting error messages. Added a test to track this and ensure this doesn't regress.

Diff Detail

Event Timeline

clayborg created this revision.Aug 10 2023, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 6:22 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
clayborg requested review of this revision.Aug 10 2023, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 6:22 PM
clayborg updated this revision to Diff 549243.Aug 10 2023, 10:00 PM

Update to not warn about inline functions not being in the parent address ranges if:

  • the concrete function has more than one range and there are no inline ranges in the current range
  • all top level inline functions have empty address ranges (which mean they were elided)
kusmour accepted this revision.Aug 16 2023, 11:03 AM
This revision is now accepted and ready to land.Aug 16 2023, 11:03 AM
kusmour added inline comments.Aug 16 2023, 11:26 AM
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
238–242

Should we remove the curly brackets for one-line if branching?

clayborg added inline comments.Aug 17 2023, 12:13 PM
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
238–242

If the else statement has curly brackets, I believe we need them on the "if" too, even if it is a single line.

ayermolo accepted this revision.Aug 17 2023, 1:51 PM
This revision was landed with ongoing or failed builds.Aug 17 2023, 2:10 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
212

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?)

clayborg added inline comments.Sep 22 2023, 5:02 PM
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
212

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.

dblaikie added inline comments.Sep 25 2023, 4:25 PM
llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
212

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)