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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+
+template <typename ELFT> struct MutableELFSection {
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
----------------
jhenderson wrote:
> Comment?
Not really sure what to say, is that enough you think?


================
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
----------------
jhenderson wrote:
> How about a comment for MutableELFObject?
Same as above, not quite sure what to say.


================
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:
> 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.
FWIW I think there is there's a function template instantiation somewhere in these macros and so its doing `unsigned_expr == signed_expr` not `unsigned_expr == 2`.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:54
+  ASSERT_THAT_EXPECTED(NewErrOrObj, Succeeded());
+  ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(NewErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
----------------
jhenderson wrote:
> 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.
I think that it does give something, The `ObjectFile`s are made from the same yaml so I'm testing that the public methods return the same values. Its a whole thing because ObjectFile has no copy constructor. Also we don't want to use the ObjectFile after moving it. I also still think that doing something like this

```
MutableELFObject<ELF64LE> MutableObject(std::move(*MutELFObjFile));
ELFObjectFile<ELF64LE> *ElfObject = &MutableObject;

// test the interface
```

will never produce any differences so it doesn't test that MutableELFObject has the same behavior as ELFObjectFile on an unmodified object file.


================
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());
----------------
jhenderson wrote:
> 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.
Is this what you mean? And then remove the one on 156? Sorry wasn't quite sure.


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

https://reviews.llvm.org/D64281