[PATCH] D82838: Parse section ranges when verifying DWARF so we can exclude addresses that should have been stripped from DWARF.

Wed Jul 8 12:24:30 PDT 2020

dblaikie added a comment.

In D82838#2135038 <https://reviews.llvm.org/D82838#2135038>, @clayborg wrote:

> In D82838#2134725 <https://reviews.llvm.org/D82838#2134725>, @dblaikie wrote:
>
> > I think maybe this is sort of orthogonal to 46453... maybe not, but kind of.
> >
> > Seems like we should filter out known-tombstoned ranges (the only ones we can know for sure are the new -1/-2 tombstones - all the others have ambiguities). Then we should maybe flag maybe-tombstones with a little "eh, maybe?". Then we should warn for anything left that's even partially outside the .text range (this patch), then we should warn for overlaps/etc on the remaining ones?
>
>
> So for this patch, anything that isn't in text becomes a warning and not an error? Or do we want to add an option to "llvm-dwarfdump --verify" to enforce the text ranges as feature that is disabled by default? --ignore-invalid-text-ranges?

I think my goal was to suggest implementing filter known-tombstones first (now we have a good/known tombstone) so that "is not in .text" doesn't unduly warn on correctly tombstoned ranges/addresses (honestly bfd's tombstoning should be fairly good - since it creates empty ranges at least in debug_ranges that don't use base address selection entries - . Then we could maybe warn or error to varying degrees on the things in the middle (not certainly tombstoned, not entirely in .text... )

Sorry,  didn't mean to muddy the waters with "warning V error" discussion or need to add more flags, etc - folks who implemented/have more ownership over "verify" should chime in on this, but for myself - yeah, I think I'm coming around to "let's just ignore anything that's even partially outside .text for now" & eventually maybe someone implements the specific tombstone support - and then we warn/error/something on "it's not tombstone, but it's outside .text" which would be a separate issue & a problem, even if it's non-overlapping. Then only the "is in .text" bits would be tested for overlapping.

>> But as @jhenderson said, maybe those first ones come later & we use the .text range to determine which things to look at for overlap first, then add new verifier checks for "things outside .text that aren't clearly tombstoned" knowing that some of those are expected limitations of (at least gold's) previous tombstoning strategies.
>> 
>> (I'd sort of like to avoid actually looking at the object's executable sections - but I can't really fault the strategy & even if we added all the other verifier checks/warnings/etc, it'd still be super reasonable to warn about ranges that are otherwise totally valid, but extend beyond/are entirely outside the actual executable .text)
> 
> Since zero is so prevalent, it is nice to get that noise out of the error checking since it creates so many false errors at the moment. It makes the --verify option less useful and way too noisy if we don't do something. We can also just not do the .text ranges for object files since they typically have relocations on each address. We already avoid looking at ranges in many cases for .o files.

Yup.

In theory all this stuff should be supported for object files too.

================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h:365

+static inline bool operator<(const DWARFVerifier::SortedRanges &LHS,
+                             const DWARFVerifier::SortedRanges &RHS) {
----------------
(non-member static shouldn't be used in headers - I fixed the op< to not do this)

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:743
+      TextRanges.emplace_back(DWARFAddressRange(
+          StartAddr, StartAddr + Size, SectionedAddress::UndefSection));
+    }
----------------
Not sure why this uses UndefSection - be nice to support this in object files and track distinct .text sections. Might be that pre-building a table doesn't suit that strategy - not sure.

(& maybe even non-.text sections, in case someone decides to use __attribute__((section(".my_section"))) for their functions, for instance)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82838