<div dir="ltr">Thanks for all the context!<br><br>As for the section oriented or data oriented view - a section-oriented view could be maintained and something like STLExtras' concat(R1, R2, ...) could be used to provide an accessor over all the units across all the sections. Though I'm not wedded to that direction.<br><br>I think it's probably a bit awkward to try to keep them in the same container and have offsets to remember which parts of that container have the things from debug_types section and which parts are from debug_info section. So if the dumping is going to continue working in that way (as opposed to, say, changing the dumping entirely to not be section-oriented and have an "all units" section) I'd err on the side of separate containers at least by section name (if not by section contribution) - and a concat-based accessor for API users who want to visit all the units together across all sections.<br><br>As for the section abstraction V section name abstraction. Yeah, no big deal there except some worries about error handling perhaps (the current API being one object per section contribution, not per section name - so each object can fail during parsing & the remaining objects remain unmodified and valid). I'd do the renaming either before or with (2) (because (2) is where it stops being one-per-section, instead one-per-section-name). But no big deal either way - end result is the same.<br><br>A different name for "parse" that indicates that it's additive would be helpful (not so much about "parse" V "parseSection" but about the fact that this parse operation is meant to/able to be run on an already initialized/populated object and will further populate it - whereas all the other parse operations take an empty/default initialized object and populate it - the object goes from an unparsed to a parsed state and is done, no more parsing/population to happen - whereas that won't be the case for this data structure). Maybe just "addSection" or "addAndParseSection" ("parseAndAddSection" - though I still worry having "parse" at the start might make it appear like all the other parse operations here)<br><br>It's not that there are bugs in how intra-section offsets work, it's just that they work/appear differently when dealing with separate sections even though those sections have the same name. Offsets are section relative. Looks like the offsets are maintained within the DIEs/Units so it'll continue to dump fine even if they're all added to the same DWARFUnitSection/thing.<br><br>I still suspect the inheritance was (I implemented the original templated stuff - and looking at it, seems excessive to me now) & will be overkill (in part because I'm sort of vaguely hoping that one day all units will be the same and have a variable header of zero or more DIEs identifiable by hash with the DIEs respective offset - at which point units generalize/look more the same) but don't mind for now & we'll just see how it all shakes out over time.<br><br>Long story short, I think it would be best to:<br><br>* rename 'parse' for DWARFUnitVector<br>* keep units from debug_types separate from units from debug_info<br><br>Other than that *thumbs up* and commit away.<br><br>- Dave<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 26, 2018 at 8:06 AM <<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div class="m_275328929548956092WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Here's a broader precis of my thinking in taking the library in this direction.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Prior to v4, DWARF kept information of different natures in different object-file sections. So, .debug_line had the line tables, .debug_macinfo had the macro
information, .debug_info had the DIEs, and so on. In some cases, information in a single section was subdivided according to compilation unit (.debug_info and .debug_line in particular) but the information in each section was all of the same nature.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">In v4, DWARF added .debug_types, which was different in two ways. First, there might be multiple object-file sections with the same name (the COMDATs), and
second, the nature of the information was much the same as in .debug_info. There was a distinction, however, in that .debug_info contained only compile units and .debug_types contained only type units. There's also the aspect that the LLVM parser is oriented
toward the needs of an object-file dumper, which encouraged thinking about the information in object-file terms, i.e. by section. Hence, type units were supported by using a container of section-oriented classes, each element being able to handle multiple
units.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">In v5, DWARF corrected the mistake of putting type units in a differently named object-file section. So, now we can have one or more .debug_info sections,
each of which can contain one or more compile and/or type units. But of course .debug_types still exists if there is some linked object file containing a mix of v4 and 5 compilations.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">In parallel, there is increased interest in making the LLVM and LLDB parsers converge, and if the decision goes with using the LLVM parser in LLDB, the LLVM
parser needs to better accommodate the needs of a debugger rather than just a dumper. In that case, the important distinction is not the object-file section, but the nature of the information. That is, pasting together all the DIE data from .debug_info and
.debug_types into a single manageable whole is what LLDB wants, because the source section is irrelevant. And in fact, within the past year LLDB's parser was modified to understand .debug_types, and models it as being appended to .debug_info for navigation
purposes.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Of course the LLVM dumper still thinks about things in object-file section terms, so it's worth being able to look at the DIE information on a per-section basis.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I think I have achieved that with the refactoring. The DWARF data that is in the nature of DIEs is managed by one container, making it easier for LLDB to look
at it as a unified whole. The container can provide iterators that distinguish between different section names, allowing the dumper to retain a section-oriented view. The functional patch that enables multiple .debug_info sections for DWARF v5 should be
not a whole lot more than changing the parse methods to iterate over possibly multiple instances of object-file sections with that name.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">If there are bugs in how intra-section offsets are reported and handled, then it's worth knowing; what that means is that we don't have existing tests that
demonstrate the offsets are done correctly. Happy to improve testing and fix any problems found.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">If there is bikeshedding about the names of methods, that's fine too. Renaming DWARFUnitVector::parse[DWO] to parse[DWO]Section is obviously simple and can
be folded into the same patch that renamed the class.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">As for merging DWARFTypeUnit and DWARFCompileUnit into DWARFUnit and eliminating the distinction at the class level, I haven't really looked at that. There
are a couple reasons to put that off. First, Jan Kratochvil has been doing LLDB work in the direction of more subclasses, not fewer, i.e. putting partial and imported units into their own subclasses. I haven't looked closely at his patches. Second, I've
had interest internally in getting partial units to work reasonably so we can put them into COMDATs and trivially deduplicate the debug info for linkonce functions. If a separate subclass would help there, merging others would make less sense.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">So I'm not fundamentally opposed to merging the unit classes, but I think it can be deferred with little problem while we work out the other aspects.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Other questions/comments always welcome.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">--paulr<u></u><u></u></span></p>
<p class="MsoNormal"><a name="m_275328929548956092__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></a></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Wednesday, July 25, 2018 3:36 PM<br>
<b>To:</b> Robinson, Paul<br>
<b>Cc:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm-dev] [DWARF] De-segregating type units and compile units<u></u><u></u></span></p>
</div>
</div></div></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_275328929548956092WordSection1"><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">A few general design discussions that cross multiple patches, so figure it's worth discussing here:<br>
<br>
The DWARFUnitSection is currently (before your changes) representing the contents from a single section - and most/all of the classes in libDebugInfoDWARF use "parse" to initialize their complete state. So it's a bit novel/new/different/worth considering (maybe
renaming the function or the like) what it means to make this class now represent data from multiple sections and to be documentend/intended to be used to aggregate the parsing result from multiple sections (both multiple sections with the same name (debug_types
currently, or in v5 debug_info comdat sections for type units)). One observable part of the matter of which sections come from is the section relative offsets (you'll see the difference in dumping a file with at least 2 types with type units enabled, with
and without split DWARF - without split DWARF comdat sections are used, so each types section offsets are zerod at the start of each type unit because it's a separate section (even though it has the same section name) - whereas in split DWARF, in the .dwo
file the offsets are across the whole, singular, debug_types.dwo section)<br>
<br>
As for virtuality in the unit hierarchy - reckon it's worth it, or maybe just use a union/conditional for the dumping differences (& collapse/remove the type distinction between type units and compile units - just have DWARFUnit as the only unit class)? (since
it's just the type_signature V DWO_id (maybe we could generalize that to "id") and type_offset that's different between them).<u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Tue, Jul 24, 2018 at 10:40 AM via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Hello DWARF fans,<br>
<br>
I've just posted a set of four refactoring patches for DebugInfo/DWARF,<br>
which move in the direction of handling DWARF v4 or v5 type units and<br>
compile units more coherently.<br>
<br>
In DWARF v4, type units and compile units are strictly segregated into<br>
the .debug_types and .debug_info sections, respectively. This division<br>
was pretty ingrained into how DebugInfo/DWARF handled the units.<br>
<br>
In DWARF v5, type units and compile units are all in the .debug_info<br>
section, and the .debug_types section is obsolete. So, we need to have<br>
a container than can handle both kinds of units in a straightforward way.<br>
<br>
The refactoring replaces a pair of collections templated on the unit type<br>
with a single collection whose elements are base-class pointers; thus it<br>
can contain elements that are a mix of unit kinds. Everything that really<br>
mattered was either already virtual or was straightforward to convert to<br>
generic handling. I think I needed only one type-based conditional, on<br>
top of what already existed. The code doesn't *quite* support DWARF v5<br>
type units in the .debug_info section, but the new starting point should<br>
make that straightforward.<br>
<br>
In a mixed v4/v5 executable, we can pretend the .debug_types sections<br>
are all really .debug_info sections, and tack them onto the end of the<br>
vector of units that already handles a mix of compile and type units.<br>
This is also how the LLDB DWARF parser handles this case, so it's at<br>
least consistent in principle (even if the actual code differs).<br>
<br>
The existing distinction between "normal" and split (DWO) units remains;<br>
that has to do with what information exists in which object files, and<br>
is not fundamentally changing in DWARF v5.<br>
<br>
Patch 1: De-templatize DWARFUnitSection<br>
<a href="https://reviews.llvm.org/D49741" target="_blank">https://reviews.llvm.org/D49741</a><br>
Patch 2: The TU collection doesn't need to be a deque<br>
<a href="https://reviews.llvm.org/D49742" target="_blank">https://reviews.llvm.org/D49742</a><br>
Patch 3: Rename DWARFUnitSection to DWARFUnitVector<br>
<a href="https://reviews.llvm.org/D49743" target="_blank">https://reviews.llvm.org/D49743</a><br>
Patch 4: Unify handling of type and compile units<br>
<a href="https://reviews.llvm.org/D49744" target="_blank">https://reviews.llvm.org/D49744</a><br>
<br>
Thanks,<br>
--paulr<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><u></u><u></u></p>
</blockquote>
</div>
</div></div></div></blockquote></div></div>