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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> > 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.
>
> So remove `MutableELFObject` and put it in `ELFObjectFile`? This might let `ELFObjectFile` no longer depend on `ELFFile` but I'm not entirely sure on that. It's also worth noting that as @rupprecht has pointed out many methods are now linear not constant so without doing some changes (could even be a boolean to track whether any modifications have been made) it probably shouldn't be used yet.


Yes, I was suggesting that an alternative design might be to simply expand the `ELFObjectFile` interface, but you raise a good point - I forgot about the complexity issue. We certainly wouldn't want to impair the complexity of the existing ELFObjectFile methods, at least unless we started to make modifications. The point about not relying on `ELFFile` isn't really the issue here. In fact, the class could for some situations. The important bit is that it is now ELFObjectFile's responsibility to be able to handle mutations, which means that changes to the `ELFObjectFile` class's code must support that, whereas in the current proposal, changes have to be made to ELFObejctFile's code to support a subclass which it otherwise doesn't care about.

>> You could also mess about with more objects to hide away the dangerous methods by doing this.
> 
> Is this in reference to the earlier point about just modifying `ELFObjectFile`?

Yes. You could add additional objects that wrap the ELFFile class and return either the original header or a mutated one, similar to what you are doing with MutableELFObject already, but the difference is that the wrapper object in this case goes in the base class, and thus it's never possible to access the dodgy method.

>> 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.
> 
> I think it isn't that bad to cast `this` to a base class if we know the methods that will be called in that context will always be safe to call. It is much more controlled this way but still bugs can happen if the base class methods get change. I don't feel too strongly.

`reinterpret_cast` is basically saying "pretend this thing here is actually this thing" which technically works in some situations, but bypasses the whole point of hiding methods via private inheritance. The function might be safe at the moment to call, but it doesn't guarantee it in the future, as the subclass hasn't got tests for that particular case. There may be other subtle correctness issues too.


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

https://reviews.llvm.org/D66063