[PATCH] D66063: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5]

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 23 04:50:34 PDT 2019


jhenderson added a comment.

In D66063#1642202 <https://reviews.llvm.org/D66063#1642202>, @abrachet wrote:

> I have been thinking about this a lot recently. I do really like that `MutableELFObject` inherits from `ELFObjectFile` I think it reuses the most amount of code possible while still doing what we want. I think it isn't worth it to use `MutableELFObject` as a private member that it would be too verbose. Do you think it is sufficiently less fragile to warrant making the larger change? Better to be sure now when the code is much easier to change with all patches still open :)


I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class. That would render the concerns we have a little less of an issue, since it would more clearly be a bug in the base class if the wrong things were called. You could also mess about with more objects to hide away the dangerous methods by doing this. I don't have a strong feeling about any of this either way. I think with sufficient testing and a large comment, you could probably get away without making the changes we've talked about, but it might be worth it still.

> I did have a question about private inheritance and what is safe to do.  I think the most useful methods for llvm-objcopy are actually in the `SectionRef` `SymbolRef` and `SegmentRef` types so really the only methods I can see that need to be publicly exposed are the iterator returning ones. The problem is (not for `SegmentRef` because I can change that) those keep a pointer to an `ObjectFile`. From what I have tested, the virtual table is still constructed correctly and we can `reinterpret_cast<ObjectFile*>(this)` when creating `Segment/SymbolRef`'s and the proper overrides will be called. Is this safe? If not then I guess there would be no difference in how much boilerplate we would need to add between using inheritance and having an ELFObjectFile as a member.

I'm not massively familiar with how private inheritance works really, but the essential point is that a user of a subclass will never know that it's inheriting from the base class. I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all. One advantage of private inheritance over a private member, I believe, is that you should be able to use the `using` directive to pull base-class functions into the public interface of the sub-class: https://softwareengineering.stackexchange.com/a/244910

> Lastly, how should I go about doing this? Should I switch to private inheritance in this patch or in Part 1 D64281 <https://reviews.llvm.org/D64281>?

As this is a structural change that is not specific to this change (as in it doesn't introduce new functionality itself), it makes more sense to do the changes in part 1, if you're going to do them, but we should be sure it's the right thing to do first.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66063/new/

https://reviews.llvm.org/D66063





More information about the llvm-commits mailing list