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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:26
+
+    MappingType(bool IsNew, uintptr_t Ptr) : Ptr(Ptr), IsNew(IsNew) {}
+  };
----------------
nit: it's simpler if all the params (Ptr/IsNew) are consistently in the same order (for class members, constructor arg order, and member initializers)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:59-60
+
+  const T *getNew(uint64_t Index) const { return &NewValues[Index]; }
+  T *getNew(uint64_t Index) { return &NewValues[Index]; }
+
----------------
These should have an assertion (in debug mode) that `Index` is in range


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:66
+    if (Mapping.IsNew)
+      return &NewValues[reinterpret_cast<uintptr_t>(Mapping.Ptr)];
+    NewValues.emplace_back(Mapping.Ptr, Arguments...);
----------------
`Mapping.Ptr` is already a `uintptr_t`, is the `reinterpret_cast` necessary?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:89
+        Data(OwningArrayRef<uint8_t>(Header.sh_size)) {
+    ::memcpy(Data.data(), ObjFile->base() + Header.sh_offset, Header.sh_size);
+  }
----------------
`std::memcpy`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:110-111
+    MappingType Mapping = Sections[Sec.p];
+    return Mapping.IsNew ? toDataRef(reinterpret_cast<uintptr_t>(
+                             &Sections.getNew(Mapping.Ptr)->Header))
+                       : toDataRef(Mapping.Ptr);
----------------
Is `toDataRef(reinterpret_cast<uintptr_t>(...))` redundant? (Converting from pointer -> uintptr_t -> pointer)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:134
+      : ELFObjectFile<ELFT>(std::move(B)),
+        Sections(B.section_begin(), B.section_end(),
+                 [&](SectionRef Ref) { return Ref.getRawDataRefImpl().p; }) {}
----------------
I think this is a potentially-invalid reference, as `B` is `std::move`-d for the previous member. It looks like this works because the move constructor for `ELFObjectFile` is implemented as a copy constructor, although that is unusual (a typical implementation would swap() members):

```
template <class ELFT>
ELFObjectFile<ELFT>::ELFObjectFile(ELFObjectFile<ELFT> &&Other)
    : ELFObjectFile(Other.Data, Other.EF, Other.DotDynSymSec,
                    Other.DotSymtabSec, Other.ShndxTable) {}
```

I'm curious if you can test this theory by "fixing" the move constructor to be something like:
```
template <class ELFT>
ELFObjectFile<ELFT>::ELFObjectFile(ELFObjectFile<ELFT> &&Other) {
  using std::swap;
  swap(Data, Other.Data);
  swap(EF, Other.EF);
  swap(DotDynSymSec, Other.DotDynSymSec);
  swap(DotSymtabSec, Other.DotSymtabSec);
  swap(ShndxTable, Other.ShndxTable);
}
```
(no need to check it in though)

I think you can just use `section_begin()/section_end()` directly here, and it will correctly call the methods from the parent class (the vtable doesn't update until later)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:135
+        Sections(B.section_begin(), B.section_end(),
+                 [&](SectionRef Ref) { return Ref.getRawDataRefImpl().p; }) {}
+
----------------
What's being captured by `[&]` here?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:47
+    StringRef Name;
+    Iter->getName(Name);
+    return Name.data();
----------------
This should check the error code too


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:48
+    Iter->getName(Name);
+    return Name.data();
+  };
----------------
Can you avoid the copy here, and just return `StringRef`?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:108
+
+  ArrayRef<uint8_t> ZeroData{'0', '0', '0', '0'};
+
----------------
nit: `ZeroData` is kind of misleading since this is not `0`, but rather an ASCII `'0'`, i.e. this is {48, 48, 48, 48}.

At any rate, it doesn't matter that it's "zero", any random data works. Testing against something non-zero is probably healthier for tests too. Maybe just rename it to something else and use different bytes (e.g. to test byte ordering)


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:168-169
+  auto TestSections = [](SectionRef ObjFile, SectionRef MutObj) {
+#define EXPECT_EQ_ITER(getMethod)                                              \
+  EXPECT_EQ(ObjFile.getMethod(), MutObj.getMethod())
+    EXPECT_EQ_ITER(getAddress);
----------------
This macro seems small enough that it should just be typed out


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:190-191
+  // section headers outside of the file's mapping.
+  const auto End = MutableObject.section_end();
+  for (auto Iter = MutableObject.section_begin(); Iter != End; ++Iter)
+    MutableObject.getMutableSection(Iter);
----------------
You can put both `Iter` and `End` in the for header as described here: http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

```
for (auto Iter = MutableObject.section_begin(), End = MutableObject.section_end();
     Iter != End; ++Iter)
```


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

https://reviews.llvm.org/D64281