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

Thu Aug 8 06:43:06 PDT 2019

jhenderson added inline comments.

================
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:
> > 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`.
Sorry, I missed that `moveSectionNext()` is an override.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+
+template <typename ELFT> struct MutableELFSection {
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
----------------
Comment?

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:43
+
+template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
+  /// This class is used for a 'copy on write' effect with tables in an ELF
----------------
How about a comment for MutableELFObject?

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:86
+
+    /// Get the OrigType at index Index regardless if it is an OrigType or
+    /// NewType. In the case that Mappings[Index].Type == New, call NewTypes
----------------
"regardless if it" sounds off to me. How about "regardless of whether it"

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:140
+  /// Many getSection* methods in ELFObjectFile use getSection to get the
+  /// the header associated with that section. This overrie returns a valid
+  /// section header wether the section has been modified or not.
----------------
overrie -> override

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:141
+  /// the header associated with that section. This overrie returns a valid
+  /// section header wether the section has been modified or not.
+  const Elf_Shdr *getSection(DataRefImpl Sec) const override {
----------------
wether -> whether

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:117
+
+  const Elf_Ehdr &getHeader() const {
+    return *reinterpret_cast<const Elf_Ehdr *>(this->base());
----------------
abrachet wrote:
> 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
Sorry, missed that getHeader was private.

Being able to modify the header contents is a required thing however, so you might want to consider how this could be done (possibly a different change though).

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:121
+
+  const Elf_Shdr *getSection(DataRefImpl Sec) const override {
+    return &Sections[Sec.p];
----------------
abrachet wrote:
> 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?
I have no idea which method this was referring to any more...

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:114
+
+  ptrdiff_t NumSections =
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
----------------
abrachet wrote:
> 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?
Yes, that's right.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:192
+  // Check that getSize properly uses the header's sh_size value.
+  EXPECT_EQ(FirstSec->getSize(), 2UL);
+
----------------
abrachet wrote:
> 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.
Sounds like clang isn't doing a good enough job here - it's a spurious warning, because 2 is a positive integer literal so can clearly be handled properly in this comparison.

Check elsewhere, but I thought it was more common in the code base to use lower-case for literal suffixes when they're needed.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:54
+  ASSERT_THAT_EXPECTED(NewErrOrObj, Succeeded());
+  ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(NewErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
----------------
Rather than reusing the same variable, create a new variable here. Then don't bother with the ObjFile variable above. It doesn't give us anything.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:88
+       Iter != End; ++Iter) {
+    auto Expect = MutableObject.getMutableSection(Iter);
+    if (!Expect)
----------------
Don't use `auto` here.

================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:141
+  auto MutSectionOrErr = MutableObject.getMutableSection(Iter);
+  EXPECT_EQ(std::distance(MutableObject.section_begin(), Iter), 2);
+  ASSERT_THAT_EXPECTED(MutSectionOrErr, Succeeded());
----------------
EXPECT_EQ -> ASSERT_EQ to avoid dereferencing an invalid iterator later on.

I think I either misread the code last time, as I don't know why I wanted this check here. However, you do show that the std::distance from section_begin to section_end is still correct after you called getMutableSection, but not until after you've done the iteration, meaning that your iterator could be invalid part way through your calls to compareSectionName. Move the later std::distance check up to here instead.

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

https://reviews.llvm.org/D64281