[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 12 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
jhenderson wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > Use `explicit` here. Should this be an r-value reference? What do you think?
> > Sounds fine to me. In the unit tests I move the ELFObjectFile but then continue to use it because as @rupprecht pointed out previously the move constructor is just a copy constructor. Is it safe to do this or should I explicitly copy?
> > Is it safe to do this or should I explicitly copy?
> 
> This doesn't sound safe to me - you need to copy or just simply not use the object after it's been moved. a) I wouldn't be surprised if this is technically undefined behaviour and the compiler makes assumptions about the state of the instance after the move. b) It's fragile to future changes.
> 
I did an explicit copy /I think/ in the unit test now. I have an `ELFObjectFile *` and then deference it and assign to an `ObjectFile &`. Does this make a copy or does the compiler omit it and just assign the address?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:168
+template <typename ELFT>
+void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
+  ++Sec.p;
----------------
jhenderson wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > What is this function for?
> > It advances the `section_iterator`. `MutableELFObject` uses a different iterator to that of `ELFObjectFile` which I belive would have a `moveSectionNext` that did something like `Sec.p += sizeof(Elf_Shdr)` as those just point directly to the section header.
> At the moment, it doesn't look like it's used, so it shouldn't exist. Re-add it when it is needed. However, perhaps a safer thing to do is define your own iterator class, as @jakehehrlich said that does this internally.
This was referring to `moveSectionNext()` before.

It does get used, it is an overload of the function that `content_iterator::operator ++()` calls. It must be overriden because `ELFObjectFile::moveSectionNext()` iterates over sections not with their index but their mapped address. This uses index into `MutableTable`.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:117
+
+  const Elf_Ehdr &getHeader() const {
+    return *reinterpret_cast<const Elf_Ehdr *>(this->base());
----------------
jhenderson wrote:
> Does this need to exist? If so, it needs unit-testing.
Yes it is used and thus tested implicitly. How would I go about testing the private interface? I could make it protected and then inherit from it in the unit test, but that doesn't sound great. FWIW this is almost identical to the `getHeader` method in `ELFFile`. https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Object/ELF.h#L108


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:121
+
+  const Elf_Shdr *getSection(DataRefImpl Sec) const override {
+    return &Sections[Sec.p];
----------------
jhenderson wrote:
> Does this need to exist? If so, it needs unit-testing.
Yes. It is used for every public method on `SectionRef` that gets tested in the unit test. Is this enough or should I test it directly as well?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:43
+  // constructor on ObjectFile is a copy constructor.
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+  const ObjectFile &ObjFile = *ErrOrObj->get();
----------------
jhenderson wrote:
> Isn't the solution here to just make a second local variable that's a pointer to an ELFObjectFile constructed from MutableObject?
> 
> `ELFObjectFile *ElfObject = &MutableObject;`
I don't think so. Then both will be `MutableELFObject`'s and the same virtual methods will be called for each, I believe. I think testing the methods on an `ELFObjectFile` and `MutableELFObject` test that overloads for `MutableELFObject` behave the same as the methods in `ELFObjectFile` when the sections haven't been changed.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:78
+       Iter != End; ++Iter)
+    SafeDestroy = static_cast<bool>(MutableObject.getMutableSection(Iter));
+
----------------
jhenderson wrote:
> IIRC, this will still abort if there is an error, since the error itself hasn't been handled. The correct thing to do would be to a) check there's no Error, and then b) call consumeError().
Thanks!


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:114
+
+  ptrdiff_t NumSections =
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
----------------
jhenderson wrote:
> You need to assert somewhere that NumSections is as expected.
I have `ASSERT_EQ(NumSections, 7);` it ended up being 7 sections because of additional sections that `yaml2obj` added. Is this what you meant?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:192
+  // Check that getSize properly uses the header's sh_size value.
+  EXPECT_EQ(FirstSec->getSize(), 2UL);
+
----------------
jhenderson wrote:
> I'm pretty sure you don't need the UL suffix here. Same below.
I don't actually need the L it turns out. Without making it unsigned I get this warning `comparison of integers of different signs` on clang.


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

https://reviews.llvm.org/D64281