Page MenuHomePhabricator

Factor the Unit section parsing into the DWARFUnitSection class.

Authored by friss on Sep 29 2014, 8:39 AM.

Diff Detail


Event Timeline

friss updated this revision to Diff 14166.Sep 29 2014, 8:39 AM
friss retitled this revision from to Factor the Unit section parsing into the DWARFUnitSection class..
friss added reviewers: dblaikie, samsonov.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
samsonov added inline comments.Sep 29 2014, 11:58 AM
59 ↗(On Diff #14166)

initialize Parsed in this constructor as well.

65 ↗(On Diff #14166)

Can't you fetch all the sections' contents (debug_abbrev, debug_ranges, debug_addr etc.) by using a "Context" argument?

dblaikie added inline comments.Sep 29 2014, 12:06 PM
59 ↗(On Diff #14166)

I /think/ you can just use "SmallVector(std::move(DUS))" here.

68 ↗(On Diff #14166)

(non-actionable gripe: I'm not a huge fan of the extra flag here (not that it's any worse than the previous check - and it's marginally better in some ways)... calling parse repeatedly only to have it no-op because it's already been parsed does feel a bit weird.

What would happen if the users instead had an Optional<DWARFUnitSection> and would check that, then construct a DWARFUnitSection passing in the various sections, etc - so there was no possibility of a non-parsed DWARFUnitSection. They were constructed with the data and parsed during construction only?

(OK, maybe semi-actionable, but could just be fanciful ideas on my part))

friss added inline comments.Sep 29 2014, 12:42 PM
65 ↗(On Diff #14166)

Not trivially, because the sections are different between dwo and normal sections. Would you prefer that the patch implements parse() and parseDWO() that fetch the sections themselves (and have the current parse() become a private parseImpl() method)?

68 ↗(On Diff #14166)

You have already mentioned it and I didn't forget it. I too find that the added flag looks clumsy, and for an NFC change, I could have just left the old check as it was (but I really dislike the previous empty() check).

As you point out, doing it with Optional would mean removing the laziness, or rather moving it into another accessor. This looks orthogonal and big enough to mandate another patch if it is wanted. I can look into it in a followup patch.

dblaikie added inline comments.Sep 29 2014, 12:54 PM
68 ↗(On Diff #14166)

Agreed on all counts.

samsonov added inline comments.Sep 29 2014, 3:15 PM
65 ↗(On Diff #14166)

Yes, I'd prefer that. This would make the caller's code simpler. After that (in the follow-up changes), we can probably go on and sink getDebugAbbrev() / getRangeSection() / isLittleEndian() and calls like that further down to DWARFUnit (not DWARFUnitSection) constructor: after your recent changes DWARFUnit ctor now takes a reference to full DWARFContext.

We might even discriminate between regular and .dwo units by making them different classes, inherited from DWARFUnit. For instance, both regular and dwo units now have a reference to corresponding .dwo unit (DWOHolder stuff), which makes no sense.

friss updated this revision to Diff 14381.Oct 3 2014, 8:30 AM

As per review comments, move the collection of debug sections into helpers rather
than passing all the sections explicitely to the parse methods.

samsonov accepted this revision.Oct 3 2014, 5:04 PM
samsonov edited edge metadata.

I think this is fine for a incremental change (but see a comment below).

39 ↗(On Diff #14381)

Can these methods take const DWARFContext::Section & instead of StringRef/RelocAddrMap ?

This revision is now accepted and ready to land.Oct 3 2014, 5:04 PM
friss added inline comments.Oct 3 2014, 5:11 PM
39 ↗(On Diff #14381)

Nope because you can't forward declare nested structs and including DWARFContext.h in DWARFUnit.h isn't possible (circular dependency). We could unnest the Section struct from the Context object to allow this.

samsonov added inline comments.Oct 3 2014, 5:19 PM
39 ↗(On Diff #14381)

Yes. I think it's fine to commit this change as is, and then hoist Section (essentially, StringRef/RelocAddrMap pair) from DWARFContext and pass it here in a small follow-up change. Thanks!

friss closed this revision.Oct 5 2014, 8:46 PM
friss updated this revision to Diff 14439.

Closed by commit rL219098 (authored by @friss).