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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht added a comment.
This revision is now accepted and ready to land.

I don't see anything major left, but please wait for James to take another look



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:337-350
+template <typename ELFT>
+DataRefImpl MutableELFObject<ELFT>::toDataRef(uintptr_t Ptr) {
+  DataRefImpl Ref;
+  Ref.p = Ptr;
+  return Ref;
+}
+
----------------
I don't think these methods have anything to do with MutableELFObject, so they should moved `Object/SymbolicFile.h` so others can use it. (A FIXME comment is fine for 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");
----------------
abrachet wrote:
> 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.
I suppose it depends on how the writer/finalizer for `MutableElfObject` looks like -- a naive implementation that just iterates over sections and writes out the contents will lose the renamed values, because those will be privately owned by `MutableELFSymbol`, and not reflected in the (non-mutated) string table.


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

https://reviews.llvm.org/D65367