<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 7, 2017, at 9:56 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">[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)]<br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, Aug 7, 2017 at 7:20 AM George Rimar via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">grimar added inline comments.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1044<br class=""> public:<br class="">   DWARFObjInMemory(const StringMap<std::unique_ptr<MemoryBuffer>> &Sections,<br class="">                   <span class="Apple-converted-space"> </span>uint8_t AddrSize, bool IsLittleEndian)<br class="">----------------<br class="">dblaikie wrote:<br class="">> 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)<br class="">Currently I think only 2 tools use `DIContext::dump()` call: `llvm-dwarfdump` and `llvm-objdump`,<br class="">both are creating `DWARFObjInMemory` using another codepatch, where `ObjectFile` is provided.<br class="">If it is not, then `getSectionName()` (introduced in this patch) would just return empty section name<br class="">for any index, because it checks `Obj` member and takes section index from it.<br class=""><br class="">But that is not so simple to implement because currently we take `SectionIndex` from relocations and<br class="">this constructor assumes to take relocated data. That mean we have no section index to take from.<br class=""><br class="">Fortunately tools do not need it. I would leave this place as is.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1194-1200<br class="">+    StringRef Ret;<br class="">+    if (Obj) {<br class="">+      auto SecI = Obj->section_begin();<br class="">+      std::advance(SecI, Index);<br class="">+      (*SecI).getName(Ret);<br class="">+    }<br class="">+    return Ret;<br class="">----------------<br class="">dblaikie wrote:<br class="">> Maybe invert this for an early return:<br class="">><br class="">>   if (!Obj)<br class="">>     return "";<br class="">>   return std::next(Obj->sections(), Index)->getName(Ret);<br class="">Done.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:64<br class="">+<br class="">+    std::string NameIndex = "N/A";<br class="">+    if (Range.SectionIndex != -1ULL)<br class="">----------------<br class="">aprantl wrote:<br class="">> dblaikie wrote:<br class="">> > 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?<br class="">> I think I would prefer an empty string instead of "N/A".<br class="">Done.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:64-67<br class="">+    std::string NameIndex = "N/A";<br class="">+    if (Range.SectionIndex != -1ULL)<br class="">+      NameIndex = Obj.getSectionName(Range.SectionIndex).str() + "(" +<br class="">+                  std::to_string(Range.SectionIndex) + ")";<br class="">----------------<br class="">grimar wrote:<br class="">> aprantl wrote:<br class="">> > dblaikie wrote:<br class="">> > > 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?<br class="">> > I think I would prefer an empty string instead of "N/A".<br class="">> Done.<br class="">Done.<br class=""><br class=""><br class="">================<br class="">Comment at: test/DebugInfo/X86/dwarfdump-ranges-unrelocated.s:7-8<br class="">+# CHECK: DW_AT_ranges [DW_FORM_sec_offset] (0x00000000<br class="">+# CHECK-NEXT:  [0x0000000000000000 - 0x0000000000000001) .text.foo1(3)<br class="">+# CHECK-NEXT:  [0x0000000000000000 - 0x0000000000000002) .text.foo2(4))<br class="">+<br class="">----------------<br class="">dblaikie wrote:<br class="">> 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)<br class="">><br class="">> 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))<br class="">><br class="">> & 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)<br class="">><br class="">> 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.<br class="">I would not scan over all sections of file, because it looks too much. Scanning over all sections can be slow.<br class=""></blockquote><div class=""><br class="">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)<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">I would not omit something dependent on third conditions too, because it makes logic of output unclear for<br class="">people that are not familar with tool. ("Why it prints section index for "foo" and does not for "bar" ? BUUUG !")<br class=""></blockquote><div class=""><br class="">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.<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">Also I think often I am looking for section number in readelf/objdump/other tools,</blockquote><div class=""><br class="">Really? That surprises me - why are the section numbers of interest to you? For myself I'm usually interested in the section name.<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>A model that might work here is to hide the information by default and provide a flag to turn it back on. For example dwarfdump on macOS hides the DWARF FORM values by default, but provides a -F (and a -v) option to show them if they are interesting.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">so I would not omit<br class="">it to be able to find it as fast as possible without manual eye-mapping name to index by myself.<br class=""><br class="">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.<br class=""></blockquote><div class=""><br class="">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... <br class=""><br class="">(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)<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">It also removes need to columnize anything probably and thus scan through to find longest index/section name, what should leave code simple.<br class=""><br class="">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).<br class=""></blockquote><div class=""><br class="">I'd skip printing it entirely if it's empty. 'Section: ""' doesn't seem helpful.<br class=""></div></div></div></div></blockquote><div><br class=""></div>+1</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><div class=""><br class="">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.<br class=""><br class="">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).<br class=""><br class="">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)<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><br class=""><br class="">================<br class="">Comment at: test/DebugInfo/dwarfdump-ranges.test:7-8<br class=""> CHECK:  DW_AT_ranges [DW_FORM_data4]      (0x00000000<br class="">-CHECK-NEXT:          [0x000000000000062c - 0x0000000000000637)<br class="">-CHECK-NEXT:          [0x0000000000000637 - 0x000000000000063d))<br class="">+CHECK-NEXT:          [0x000000000000062c - 0x0000000000000637) N/A<br class="">+CHECK-NEXT:          [0x0000000000000637 - 0x000000000000063d) N/A)<br class=""><br class="">----------------<br class="">dblaikie wrote:<br class="">> Why does this print N/A in this test case?<br class="">dwarfdump-test4.elf-x86-64 is not an object but DSO. It does not have .rel[a] sections<br class="">(targeted to debug sections I mean), and so all `RelocAddrMaps` are empty and we can not<br class="">take `SectionIndex` from relocation.<br class=""><br class=""><br class=""><a href="https://reviews.llvm.org/D36313" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D36313</a></blockquote></div></div></div></blockquote></div><br class=""></body></html>