[PATCH] D36313: [llvm-dwarfdump] - Print section name and index when dumping .debug_info ranges

[David Blaikie via llvm-commits]

[Adrian - would love to hear your thoughts on some of the format/layout of
this info in the dump output (more comments related to that inline/below)]

On Mon, Aug 7, 2017 at 7:20 AM George Rimar via Phabricator <
reviews at reviews.llvm.org> wrote:

> grimar added inline comments.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1044
>  public:
>    DWARFObjInMemory(const StringMap<std::unique_ptr<MemoryBuffer>>
> &Sections,
>                     uint8_t AddrSize, bool IsLittleEndian)
> ----------------
> dblaikie wrote:
> > If/how do section indexes work in this codepath? Are they provided &
> printed but without names (because the section itself cannot be looked up)?
> Is there a way to support that? (probably not necessary, but consistency is
> nice where it's possible)
> Currently I think only 2 tools use `DIContext::dump()` call:
> `llvm-dwarfdump` and `llvm-objdump`,
> both are creating `DWARFObjInMemory` using another codepatch, where
> `ObjectFile` is provided.
> If it is not, then `getSectionName()` (introduced in this patch) would
> just return empty section name
> for any index, because it checks `Obj` member and takes section index from
> it.
>
> But that is not so simple to implement because currently we take
> `SectionIndex` from relocations and
> this constructor assumes to take relocated data. That mean we have no
> section index to take from.
>
> Fortunately tools do not need it. I would leave this place as is.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1194-1200
> +    StringRef Ret;
> +    if (Obj) {
> +      auto SecI = Obj->section_begin();
> +      std::advance(SecI, Index);
> +      (*SecI).getName(Ret);
> +    }
> +    return Ret;
> ----------------
> dblaikie wrote:
> > Maybe invert this for an early return:
> >
> >   if (!Obj)
> >     return "";
> >   return std::next(Obj->sections(), Index)->getName(Ret);
> Done.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:64
> +
> +    std::string NameIndex = "N/A";
> +    if (Range.SectionIndex != -1ULL)
> ----------------
> aprantl wrote:
> > dblaikie wrote:
> > > Rather than creating a temporary string, maybe move this to after the
> format line, and print it directly to the stream with a format call as well?
> > I think I would prefer an empty string instead of "N/A".
> Done.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:64-67
> +    std::string NameIndex = "N/A";
> +    if (Range.SectionIndex != -1ULL)
> +      NameIndex = Obj.getSectionName(Range.SectionIndex).str() + "(" +
> +                  std::to_string(Range.SectionIndex) + ")";
> ----------------
> grimar wrote:
> > aprantl wrote:
> > > dblaikie wrote:
> > > > Rather than creating a temporary string, maybe move this to after
> the format line, and print it directly to the stream with a format call as
> well?
> > > I think I would prefer an empty string instead of "N/A".
> > Done.
> Done.
>
>
> ================
> Comment at: test/DebugInfo/X86/dwarfdump-ranges-unrelocated.s:7-8
> +# CHECK: DW_AT_ranges [DW_FORM_sec_offset] (0x00000000
> +# CHECK-NEXT:  [0x0000000000000000 - 0x0000000000000001) .text.foo1(3)
> +# CHECK-NEXT:  [0x0000000000000000 - 0x0000000000000002) .text.foo2(4))
> +
> ----------------
> dblaikie wrote:
> > It'd be nice to columnize the section number  - but maybe that's too
> much work? (scanning through to find the longest section name, etc)
> >
> > Also, similar amount of work: Do you think it'd be worth it/good to omit
> the section number if the section names are not ambiguous in this list? (or
> perhaps necessary to only do this if the name is not ambiguous in the whole
> file (so you on't look at the ranges for two functions and see them
> describing what appears to be the same section))
> >
> > & what about omitting the name or putting it somewhere else (like at the
> start on a separate line) if every entry is in the same section? (which
> will be the case for all ranges except the compile_unit ranges, most likely)
> >
> > Maybe that's all overkill - wouldn't mind getting Adrian's take on this,
> as he was driving a bunch of the dwarfdump improvements like this inline
> range dumping.
> I would not scan over all sections of file, because it looks too much.
> Scanning over all sections can be slow.
>

Too much/slow? I'd be surprised if it was very expensive to do a single
loop over the sections (~hundreds of sections in an object file? Fewer in a
linked executable), maybe lazily (when the first range is rendered) - it'd
only happen once for the whole dump run (& add a lookup in the resulting
map for each range entry dumping - which, yeah, that's something, but
still). dwarfdump's an interactive/user-focussed tool, it probably takes
longer to print things to the terminal than most of this computation. (&
longer still for the user to read it, etc)


> I would not omit something dependent on third conditions too, because it
> makes logic of output unclear for
> people that are not familar with tool. ("Why it prints section index for
> "foo" and does not for "bar" ? BUUUG !")
>

Not sure it'd be particulary bug-like if the section numbers were only
shown when the section names were ambiguous. Given two examples it would
seem fairly clear to me, I think, that the numbers were added to
disambiguate two sections with the same name.


> Also I think often I am looking for section number in
> readelf/objdump/other tools,


Really? That surprises me - why are the section numbers of interest to you?
For myself I'm usually interested in the section name.


> so I would not omit
> it to be able to find it as fast as possible without manual eye-mapping
> name to index by myself.
>
> Basing on above I think I like "omitting the name or putting it somewhere
> else (like at the start on a separate line) if every entry is in the same
> section" as it avoids printing the same name+index multiple times.
>

I think that's a good thing to do regardless - and I'm leaning towards
omitting the name/index entirely in the case where everything in the list
is in the same section. (because local ranges within a function don't seem
important to mention which section they're from). But I don't think I'd
mind too much if the section/index was included...

(I'm mostly thinking about scope ranges in a function - not sure it's worth
burning a line of output on every range list for every scope in a function
when they're all in the same section anyway)


> It also removes need to columnize anything probably and thus scan through
> to find longest index/section name, what should leave code simple.
>
> What do you think about new version of output ? (I am wondering if we
> still want to columnize the section index, I am not sure).
>

I'd skip printing it entirely if it's empty. 'Section: ""' doesn't seem
helpful.

I think printing it on the same line without the "Section: " prefix, as it
was in your first version, is probably better when it is per-line.

I guess maybe you're optimizing this for the case where some set of ranges
share the same section (so it prints a "Section: "x"" header and then all
the ranges in that section? (or all the ranges in that section that are
contiguous until another section change?)? Though I don't see a test case
for that (multiple sections, some ranges next to each other in the same
section).

But I don't think that's a scenario we realy need to optimize for - it's
going to be pretty rare that a list contains both multiple sections and
distinct ranges in the same section. (function/scope ranges will contain
ranegs all from the same section and CU ranges will /mostly/ contain all
ranges from distinct sections - except in cases of nodebug functions
putting holes in the range)


>
>
> ================
> Comment at: test/DebugInfo/dwarfdump-ranges.test:7-8
>  CHECK:  DW_AT_ranges [DW_FORM_data4]      (0x00000000
> -CHECK-NEXT:          [0x000000000000062c - 0x0000000000000637)
> -CHECK-NEXT:          [0x0000000000000637 - 0x000000000000063d))
> +CHECK-NEXT:          [0x000000000000062c - 0x0000000000000637) N/A
> +CHECK-NEXT:          [0x0000000000000637 - 0x000000000000063d) N/A)
>
> ----------------
> dblaikie wrote:
> > Why does this print N/A in this test case?
> dwarfdump-test4.elf-x86-64 is not an object but DSO. It does not have
> .rel[a] sections
> (targeted to debug sections I mean), and so all `RelocAddrMaps` are empty
> and we can not
> take `SectionIndex` from relocation.
>
>
> https://reviews.llvm.org/D36313
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170807/926a79fd/attachment.html>