clayborg added a comment.
I really like the idea of coming up with a low PC that is the first valid .text
address. This will filter out many of the zeroed out dead stripped DWARF and
will make a cheap way for us to check addresses when parsing debug info and
line tables. Checking for -1 and -2 of course is great to do as well.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1044
for (const llvm::DWARFDebugLine::Sequence &seq : line_table->Sequences) {
+ if (!list.ContainsFileAddressRange(seq.LowPC, seq.HighPC - seq.LowPC))
+ continue;
----------------
labath wrote:
> MaskRay wrote:
> > clayborg wrote:
> > > labath wrote:
> > > > clayborg wrote:
> > > > > dblaikie wrote:
> > > > > > Could you specifically look for/propagate tombstones here, to
> > > > > > reduce the risk of large functions overlapping with the valid
> > > > > > address range, even when they're tombstoned to zero? (because
> > > > > > zero+large function size could still end up overlapping with valid
> > > > > > code)
> > > > > >
> > > > > > To do that well, I guess it'd have to be implemented at a
> > > > > > lower-layer, inside the line table state machine - essentially
> > > > > > dropping all lines derived from a "set address" operation that
> > > > > > specifies a tombstone.
> > > > > Just checking if the section lists contains an address doesn't help
> > > > > weed out addresses that were tombstoned to zero since our PT_LOAD[0]
> > > > > will almost always contain zero for shared libraries. It might be
> > > > > nice to make a list of addresses that come from sections with read +
> > > > > execute permissions and store that in SymbolFileDWARF one time at
> > > > > startup. Then these searches will be faster as we are looking in less
> > > > > ranges, and most likely will not contain address zero. This code will
> > > > > catch the -1 and -2 tombstones, but most linkers I have run into use
> > > > > zero and the tombstone.
> > > > >
> > > > > If our algorithm only checks sections with no subsections and then
> > > > > makes a list of file addresses for and section ranges for those, we
> > > > > should have a great list. The entire PT_LOAD[0] will usually be
> > > > > mapped read + execute, so we can't just check top level sections for
> > > > > ELF. Mach-o also has this issue __TEXT in mac is also mapped read +
> > > > > execute and usually contains zero for shared libraries, but since the
> > > > > sections must come after the mach-o header, the sections within the
> > > > > __TEXT segment have correct permissions and would work, just like
> > > > > they would for ELF.
> > > > You're right -- this would not handle shared libraries with base zero.
> > > >
> > > > I am slightly uneasy about requiring executable permissions for all
> > > > line tables. While it does not seem terribly useful to have line tables
> > > > for non-executable code, if someone does have a line table for it for
> > > > whatever reason (maybe he wants to make it executable at runtime?) it
> > > > would be a shame not to display it. Also the choice of using section
> > > > rather than segment permissions feels slightly arbitrary (although I
> > > > could make a case for it), as it's the segment permissions which will
> > > > actually define the runtime memory permissions.
> > > >
> > > > Since this is really about filtering out (near) zero addresses, how
> > > > about we make that explicit? Find the lowest executable (section)
> > > > address and reject anything below that? Additionally, I'd also reject
> > > > all addresses which are completely outside of the module range, as
> > > > those not going to get used for anything, and they are generating bogus
> > > > line-table dumps.
> > > >
> > > > What do you think?
> > > >
> > > > David: The -1 tombstones are already sort of handled in llvm (and in
> > > > lldb since D83957). They are "handled" in the sense that the all
> > > > sequences with and end PC lower than the start PC are rejected (and
> > > > line sequences starting with (unsigned)-1 will definitely wrap). This
> > > > is trying to do something about the zero tombstones.
> > > > Since this is really about filtering out (near) zero addresses, how
> > > > about we make that explicit? Find the lowest executable (section)
> > > > address and reject anything below that? Additionally, I'd also reject
> > > > all addresses which are completely outside of the module range, as
> > > > those not going to get used for anything, and they are generating bogus
> > > > line-table dumps.
> > > >
> > > > What do you think?
> > >
> > > That will work for me. My main goal is to get anything that should have
> > > been dead stripped out from appearing in results for line lookups or
> > > function lookups. The quicker we can short circuit these cases the better
> > > for performance. We can also use this when we try to lookup functions and
> > > don't return any matches for functions whose start address falls below
> > > this value.
> > >
> > >
> > There is a concern about ContainsFileAddressRange's performance.
> >
> > FindSectionContainingFileAddress iterates all sections and can be slow when
> > the number of sections is large.
> >
> > > if someone does have a line table for it for whatever reason (maybe he
> > > wants to make it executable at runtime?) it would be a shame not to
> > > display it.
> >
> > +1
> >
> > Instead of a [lowpc,highpc) range check, I wonder whether we should just
> > filter out lowpc. We don't seem to benefit much from a range check. A data
> > point is that gdb filters with the CU address range and only checks lowpc
> > (https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=gdb/dwarf2/read.c;h=405b5fb3348c94aad10e3bb40f393137ddb0759c;hp=4622d14a05c6819b482c9c97c14a14755876aa72;hb=a8caed5d7faa639a1e6769eba551d15d8ddd9510;hpb=33d1369f183f1c276e3f0f52b5573fb2f5843b1c
> > )
> > If the CU's lowpc is 0, it will allow a line sequence at address 0,
> > otherwise disallow it. There is some bare metal usage with zero address.
> > There is a concern about ContainsFileAddressRange's performance.
> > FindSectionContainingFileAddress iterates all sections and can be slow when
> > the number of sections is large.
> Yeah, that function isn't very optimized, but OTOH, it is used in pretty much
> every address translation in lldb (Address class internally represents things
> as section+offset), and it hasn't been a problem so far. (can't say we run
> many benchmarks, but we do run _some_). My thinking was that if it ever
> becomes a problem, it could be solved at the source so that all users benefit.
>
> > If the CU's lowpc is 0, it will allow a line sequence at address 0,
> > otherwise disallow it.
> Hmm.... are you sure that code actually works? Because using DW_AT_low_pc=0
> in combination with DW_AT_ranges is a common practice for describing code
> from multiple sections (which is pretty much all code in c++). It relies on
> the fact that when DW_AT_ranges is used, the only purpose of DW_AT_low_pc is
> to set a base address for range and location lists. An absolute base of zero
> allows those entries to have relocations applied to them. I'm not sure this
> practice is still useful/needed for dwarf 5, but clang does it nonetheless.
> ```
> $ clang -gdwarf-5 -x c -o - -c -g - <<<"void f(){} void g(){}"
> -ffunction-sections | llvm-dwarfdump - | grep -C 1 -e DW_AT_ranges
> DW_AT_low_pc (0x0000000000000000)
> DW_AT_ranges (indexed (0x0) rangelist = 0x00000010
> [0x0000000000000000, 0x0000000000000006)
> ```
> If the CU's lowpc is 0, it will allow a line sequence at address 0, otherwise
> disallow it.
I agree with Pavel here, DW_AT_low_pc is often specified as zero just for
DW_AT_ranges. The whole relying on the low PC of a compile unit as a base
address creates a lot of ways for the debug info to be messed up with LTO and
dead stripping.
I really like the idea of coming up with a minimum PC that makes sense and
filter everything else out below that.
> if someone does have a line table for it for whatever reason (maybe he wants
> to make it executable at runtime?) it would be a shame not to display it.
I don't agree here. In reality how many times will this happen? Almost zero.
And every day in anything that isn't macOS (since both DWARF in .o files and
dSYM files _do_ link debug info correctly), we deal with linkers that only can
concatenate and relocate and we often have many functions mapped to zero due to
dead stripping. So I think we will be doing our users a disservice by not
fixing this huge flaw in debug info that is generated by many linkers and is
_very_ prevalent in all flavors of linux. I would be fine adding a setting for
the DWARF plug-in if we really care about this issue, but I would rather wait
until we see a real case of this issue before we try and proactively fix this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84402/new/
https://reviews.llvm.org/D84402
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
