[PATCH] D66070: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 6]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

How about some testing for changing program header properties, e.g. size or address?



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:213
 
-  using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Ehdr = typename ELFT::Ehdr;
----------------
Any particular reason you've moved this line?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:243
     size_t size() const {
-      return llvm::count_if(Mappings, [](MappingType &Mapping) {
+      return llvm::count_if(Mappings, [](const MappingType &Mapping) {
         return Mapping.Type != MappingType::Removed;
----------------
This should be in one of the earlier changes probably, if it can be.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:251
   void moveSymbolNext(DataRefImpl &Sym) const override;
-  const Elf_Sym *getSymbol(DataRefImpl Sym) const override;
   Expected<StringRef> getSymbolName(DataRefImpl Sym) const override;
----------------
Did you mean to remove this?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:331
+
+    bool operator==(const SegmentRef &Other) const {
+      return std::tie(Object, Index) == std::tie(Other.Object, Other.Index);
----------------
What is the point of this and the less-than operator?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:332
+    bool operator==(const SegmentRef &Other) const {
+      return std::tie(Object, Index) == std::tie(Other.Object, Other.Index);
+    }
----------------
Just do:


```
return Object == Other.Object && Index == Other.Index;
```


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:334
+    }
+    bool operator<(const SegmentRef &Other) const {
+      assert(Object == Other.Object && "Not from the same object");
----------------
Blank line before this. Also, is it clang-formatted?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:345
+
+    MutableELFSegment<ELFT> &makeMutable() {}
+
----------------
What's the point of this function?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:347-353
+    bool hasSection(SectionRef Sec) const {
+      return Object->segmentHasSection(Index, Sec.getRawDataRefImpl().p);
+    }
+
+    void addSection(SectionRef Sec) {
+      return Object->segmentAddSection(Index, Sec.getRawDataRefImpl().p);
+    }
----------------
I'm not convinced this patch should attempt to add sections to segments. The reason for this is because they're not an intrinsic part of segments, but rather an artefact of the layout of an ELF object. Relatedly, this doesn't handle segments within other segments, so feels incomplete. I'd defer it to a patch that handles laying out an ELF.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:367
 
-  /// Removes a symbol.
-  void removeSymbol(symbol_iterator Sym) { removeSymbol(*Sym); }
----------------
Did you mean to remove this? Looks like a bad rebase to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:375
 
-  /// Removes a section.
-  void removeSection(section_iterator Sec) { removeSection(*Sec); }
----------------
Did you mean to remove this? Looks like a bad rebase to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:413-419
-const Elf_Sym_Impl<ELFT> *
-MutableELFObject<ELFT>::getSymbol(DataRefImpl Sym) const {
-  const MutableSymbolTable &Table = getWhichTable(Sym);
-  if (const MutableELFSymbol<ELFT> *SymOrNull = Table.getConstIfNew(Sym.d.b))
-    return &SymOrNull->Header;
-  return ELFObjectFile<ELFT>::getSymbol(Sym);
-}
----------------
Did you mean to remove this? Looks like a bad rebase to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:444
 
+  Expected<MutableELFSegment<ELFT> &> getMutableSegment(SegmentRef Seg) {
+    return makeMutableSegment(Seg.getIndex());
----------------
Comment?

Also Seg -> Segment


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:439
   EXPECT_EQ(*NameOrErr, "first");
-  MutableObject.removeSymbol(FirstSym);
+  MutableObject.removeSymbol(*FirstSym);
 
----------------
Seems unrelated?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:481
 
-  MutableObject.removeSection(Iter);
+  MutableObject.removeSection(*Iter);
 
----------------
Seems unrelated?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:500
 
-  MutableObject.removeSection(Iter);
+  MutableObject.removeSection(*Iter);
 
----------------
Seems unrelated?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:605
+ProgramHeaders:
+  - Type:       PT_LOAD
+    Flags:      [ PF_X, PF_R ]
----------------
I'd have at least 2 different segments here and test that both can be read in.


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

https://reviews.llvm.org/D66070