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

Wed Aug 7 08:54:02 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:70
+
+  NewType *getIfNew(uint64_t Index) {
+    assert(Index < Mappings.size() && "Out of bounds");
----------------
abrachet wrote:
> jhenderson wrote:
> > What is this function useful for?
> > 
> > Returning an Optional would be a nicer interface than returning a nullptr, IMO.
> This was referencing `getIfNew` previously.
> 
> It's for methods which could not use just the OrigType to do their job properly. For example, `getSectionName` returns the `Name` member when the section has been modified because there is no way to describe the change to `ELFObjectFile` through its `sh_name` field.
> 
> `Optional` does not work with references so far as I can tell. Is this correct? A copy would be too costly for `MutableELFSections` which have an `OwningArrayRef` and `std::string`.
You're right about `Optional`, sorry for the noise.

`getSectionName` currently calls `getConstIfNew`, so this function doesn't need to exist, right?

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
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.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:168
+template <typename ELFT>
+void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
+  ++Sec.p;
----------------
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.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:117
+
+  const Elf_Ehdr &getHeader() const {
+    return *reinterpret_cast<const Elf_Ehdr *>(this->base());
----------------
Does this need to exist? If so, it needs unit-testing.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:121
+
+  const Elf_Shdr *getSection(DataRefImpl Sec) const override {
+    return &Sections[Sec.p];
----------------
Does this need to exist? If so, it needs unit-testing.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:21-22
+
+// Test that when no modifications have been made SectionRef's methods are
+// the same between ELFObjectFile and MutableELFObject.
+TEST(MutableELFObject, NoChange) {
----------------
I'd rephrase the second half of this sentence as "methods work the same in both ELFObjectFile and MutableELFObject".

================
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();
----------------
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;`

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:46
+
+  ptrdiff_t ObjFileSecs =
+      std::distance(ELFObjFile->section_begin(), ELFObjFile->section_end());
----------------
ObjFileSecCount?

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:48
+      std::distance(ELFObjFile->section_begin(), ELFObjFile->section_end());
+  ptrdiff_t MutObjSecs =
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
----------------
MutObjSecCount?

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:50
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
+  EXPECT_EQ(ObjFileSecs, MutObjSecs);
+
----------------
This should be ASSERT_EQ, since the rest of the test assumes there's the same number.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:78
+       Iter != End; ++Iter)
+    SafeDestroy = static_cast<bool>(MutableObject.getMutableSection(Iter));
+
----------------
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().

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:114
+
+  ptrdiff_t NumSections =
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
----------------
You need to assert somewhere that NumSections is as expected.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:125
+  Iter = MutableObject.section_begin();
+  std::advance(Iter, 2);
+  auto MutSectionOrErr = MutableObject.getMutableSection(Iter);
----------------
You need to show the iterator is still valid after this, by checking std::distance.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:192
+  // Check that getSize properly uses the header's sh_size value.
+  EXPECT_EQ(FirstSec->getSize(), 2UL);
+
----------------
I'm pretty sure you don't need the UL suffix here. Same below.

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

https://reviews.llvm.org/D64281