[PATCH] D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2]

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:147
+
+    size_t originalSize() const { return OriginalValues.size(); }
   };
----------------
jhenderson wrote:
> Does this belong to a later patch?
No it is needed. I explain this in an answer to your question [[ https://reviews.llvm.org/D65367#inline-591698 |  here ]] 


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:332
+template <typename ELFT>
+uint32_t MutableELFObject<ELFT>::findSectionOfType(uint64_t ShType) const {
+  for (uint32_t I = 0; I < Sections.originalSize(); ++I)
----------------
jhenderson wrote:
> Why is this looping over only the original sections?
Because `ELFObjectFile::getSymbol()` (and also directly many other symbol related methods) needs the DataRef to have an index to the original section, and this is how to differentiate between dynamic and regular symbols. I will think more about this, but this was the least disruptive way to inherit from `ELFObjectFile` I believe. It's worth noting that this is a private method.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:241
+
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
+  EXPECT_EQ(TestSym->getValue(), 0x1234U);
----------------
jhenderson wrote:
> Looks to me like you need a checking first of all counting the number of  symbols. Also, why do you need to construct a new `symbol_iterator`, rather than using the output of `++MutableObject.symbol_begin()` directly?
`SymbolicFile::symbol_begin()` returns a `basic_symbol_iterator` pointing to `BasicSymbolRef`s whose only meaningful method for testing is `getFlags()`, but `symbol_iterator` points to `SymbolRef`s which has `getValue` and `getName`.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:360
+  MutSymOrErr->Header.st_value = 5;
+  EXPECT_EQ(MutSymOrErr->Header.st_value, 5u);
+
----------------
jhenderson wrote:
> This seems a bit redundant...
> 
> Same goes for the other changes below. You're basically testing that C++ works...
> 
> I think you want to be fetching the symbol again from one or both of the object interfaces (base or sub-class) and showing that the value has been updated.
Oops! They are more meaningful now :)


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:364-368
+  EXPECT_EQ(MutSymOrErr->Name, "new_name");
+  TestSym = symbol_iterator(++MutableObject.symbol_begin());
+  auto NameOrErr = TestSym->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "new_name");
----------------
rupprecht wrote:
> What happens to the string table in this case?
> 
> The names for symbols in the symbol table is backed by the string table, so if you change the name from "test" to "new_name", the string table needs to be rewritten for that, but it doesn't looks like there's a corresponding `MutableTable StringTable` to back that change.
Nothing happens for now. The idea, and this is the [[ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-objcopy/ELF/Object.cpp#L751 | same ]] as `objcopy::elf::Object`, is that it gets updated only when doing final layout.


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

https://reviews.llvm.org/D65367