Page MenuHomePhabricator

Simplify DWARFCompileUnit / DWARFTypeUnit constructors (NFC).
AbandonedPublic

Authored by samsonov on Oct 7 2014, 6:09 PM.

Details

Reviewers
dblaikie
friss
Summary

Now that DWARFUnit constructor takes a reference to full DWARFContext,
we can choose between DWO- and non-DWO sections inside the constructor,
instead of passing a bunch of StringRefs around.

Diff Detail

Event Timeline

samsonov updated this revision to Diff 14543.Oct 7 2014, 6:09 PM
samsonov retitled this revision from to Simplify DWARFCompileUnit / DWARFTypeUnit constructors (NFC)..
samsonov updated this object.
samsonov edited the test plan for this revision. (Show Details)
samsonov added reviewers: friss, dblaikie.
samsonov added a subscriber: Unknown Object (MLST).
friss accepted this revision.Oct 8 2014, 8:09 AM
friss edited edge metadata.

This looks like a nice improvement to me. Some comments inline just for discussion, nothing to change.

Thanks!

lib/DebugInfo/DWARFUnit.cpp
29–37

We talked at one point of using inheritance to distinguish the DWO Units from the others, do you already have a plan for that? I ask because in my branch I tried various approaches to avoid all these conditionals by having the DWO/standard section selection done using a virtual method, but I couldn't come up with something that I found nicer than this (at which level of the type hierarchy do you introduce the DWO attribute for example?).

lib/DebugInfo/DWARFUnit.h
82–83

It's a pity that we can't get rid of the bool argument that is also a Context derivative, but it's already much nicer like this.

This revision is now accepted and ready to land.Oct 8 2014, 8:09 AM
dblaikie edited edge metadata.Oct 8 2014, 9:39 AM

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

friss added a comment.Oct 8 2014, 9:56 AM
In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

In D5659#8, @friss wrote:
In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

Not concerned about performance implications - mostly just that the static choice seemed to be made where the information needed for the choice existed - and to plumb that information down a few layers, unnecessarily deferring the choice (it's not like we already had a separate DWO/O abstraction - we actually had to push the choice down with the boolean flag) doesn't seem great.

If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

friss added a comment.Oct 8 2014, 10:41 AM
In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice. Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy). I have the feeling that this commit gets us one step closer to that.

But I can see your point though, won't argue if you reject the change.

In D5659#10, @friss wrote:
In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice.

Though DWARFUnitSection is currently dwo/o-agnostic. Unless we were to make it type-specific (which we could do, making the current DWARFUnitSection a CRTP base, perhaps... - but now we've got to pivot in two directions & end up with 4 classes. {DWO,O}x{TypeUnit,CompileUnit}... which seems awkward, though possible. (note that the debug info emission code (lib/CodeGen/AsmPrinter) currently only pivots, in the inheritance hierarchy, by type/compile unit, not by .o/.dwo - though similarly, it's not a perfect fit and there are certainly times I'd like the other axis rather than a lot of "isSplitDwarf" checks)

Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy).

I'm not quite sure what you're describing when you say "a unit type attribute" - could you sketch out the ideal end state you have in mind (in terms of inheritance, reuse, where decisions get made, etc)?

I have the feeling that this commit gets us one step closer to that.

But I can see your point though, won't argue if you reject the change.

In D5659#9, @dblaikie wrote:
In D5659#8, @friss wrote:
In D5659#7, @dblaikie wrote:

I'm not really sure I agree that this is an improvement.

I have a bit of an aversion to making something a dynamic choice (passing a bool down & then doing a bunch of conditionals) compared to a static choice (at the call site, we know which sections to retrieve).

I always disliked the huge list of acronym parameters that got passed to these constructor methods, and I stylistically prefer the proposed patch. But as I commented, the bunch of conditionals isn't that nice either and if someone can come up with a nice solution that combines the best of both worlds then I'm all for it.

Are you concerned about the performance implications of making this choice dynamic or just objecting that something that can be static should be static?

Not concerned about performance implications - mostly just that the static choice seemed to be made where the information needed for the choice existed - and to plumb that information down a few layers, unnecessarily deferring the choice (it's not like we already had a separate DWO/O abstraction - we actually had to push the choice down with the boolean flag) doesn't seem great.

Fair enough. I agree that it would be cleaner to first separate .dwo units from regular ones, and then proceed with this change, so that we don't have to pull conditionals into constructor code.

If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

lib/DebugInfo/DWARFUnit.cpp
29–37

I haven't actually tried to code it yet, so probably you're in a better position. I thought of introducing a DWARFDWOContext entity, that would contain .dwo counterparts of regular sections. DWARFDWOContext is contained inside a regular DWARFContext (to represent the case when .dwo sections are not yet moved away from the executable). On the other hand, we should be able to construct DWARFDWOContext directly from an object file (with .dwo extension) - this would be used in DWARFUnit::parseDWO() method, that locates and loads .dwo file for a given compilation unit.

We can make DWARFDWOContext implement the same set of methods as DWARFContext, e.g. DWARFDWOContext::getDebugAbbrev() will return .debug_abbrev.dwo section. Instead of calling DWARFContext.getDebugAbbrevDWO() one may call DWARFContext.getDWOContext().getDebugAbbrev().

friss added a comment.Oct 8 2014, 11:14 AM
In D5659#11, @dblaikie wrote:
In D5659#10, @friss wrote:
In D5659#9, @dblaikie wrote

:> If the problem is just the number of arguments - we can wrap that in a struct and achieve the same terseness without pushing the choice down into layers that are otherwise agnostic of that detail.

It's not only about the number of arguments, I like the fact that the Unit choses the sections it needs. I find it more logical from an OO POV than having a helper in DWARFUnitSection make the choice.

Though DWARFUnitSection is currently dwo/o-agnostic. Unless we were to make it type-specific (which we could do, making the current DWARFUnitSection a CRTP base, perhaps... - but now we've got to pivot in two directions & end up with 4 classes. {DWO,O}x{TypeUnit,CompileUnit}... which seems awkward, though possible. (note that the debug info emission code (lib/CodeGen/AsmPrinter) currently only pivots, in the inheritance hierarchy, by type/compile unit, not by .o/.dwo - though similarly, it's not a perfect fit and there are certainly times I'd like the other axis rather than a lot of "isSplitDwarf" checks)

Ideally, I'd like to get rid of parseDWO totally and have the DWOness be a Unit type attribute (that was the point of my comment regarding the type hierarchy).

I'm not quite sure what you're describing when you say "a unit type attribute" - could you sketch out the ideal end state you have in mind (in terms of inheritance, reuse, where decisions get made, etc)?

The idea would be to push the decision further down in the Unit type. This would remove the need for the parseDWO helper, the high level code would just call a generic parse method on the UnitSection. I tried to do something like that but I wasn't satisfied by the end result (I introduced some kind of DWO trait template argument to DWARFUnit to avoid duplicating the code, but then the iterator types got ugly and moreover I ended up with 2 virtual calls per call to parse). So I don't have a precise idea of the outcome, but Alexey was the first one coming up with the idea of specializing the Unit types to account for the DWO specificities thus I thought this patch was the first of his grand plan to make that happen :-)

samsonov abandoned this revision.Oct 27 2014, 3:32 PM