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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+
+template <typename ELFT> struct MutableELFSection {
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
----------------
abrachet wrote:
> jhenderson wrote:
> > Comment?
> Not really sure what to say, is that enough you think?
Why would you use this class? What is the overall aim here?

It probably wants to say something like it is used by MutableELFObject to provide a mutable version of an ELFObjectFile Section or something to that effect.


================
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
----------------
abrachet wrote:
> jhenderson wrote:
> > How about a comment for MutableELFObject?
> Same as above, not quite sure what to say.
I think this comment needs to explain why this class exists. Essentially it should say why we can't use ELFObjectFile directly for our purposes.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:54
+  ASSERT_THAT_EXPECTED(NewErrOrObj, Succeeded());
+  ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(NewErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
----------------
abrachet wrote:
> 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.
I think you misunderstood. Having `MutELFObjFile` is fine, as is constructing two objects from the YAML. `ObjFile` doesn't need to exist however. You can use `ELFObjFile` directly now that you're not reusing the local variable for the creation of `MutableObject`, so the two variables you operate on are `ELFObjFile` (created directly from YAML) and `MutableObject` (created indirectly from YAML via MutELFObjFile).


================
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());
----------------
abrachet wrote:
> 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.
Sorry, I obviously wasn't clear enough, and I realised something else whilst writing this comment.

At the time of writing, the std::distance at line 127 (checks the section count before mutating), and the one at line 142 (checks it after mutating) are the ones that should exist. The one at line 155 is unnecessary, since you check it at line 142. The one at line 140 is necessary because it shows that the Iter variable is still valid after getting a mutable section.


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

https://reviews.llvm.org/D64281