[PATCH] D66063: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've been trying and failing to come up with a good solution to the design. I only see three options:

1. Adding the getHeader function as this patch does and accepting some fragility. In practice, usages within LLVM are probably fine, because we'll have testing, but it might not work as well for those who wish to use the library in their own tools.
2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.
3. Something more radical involving bigger changes in ELFObjectFile to prevent accessing the wrong header within the base class. I don't have anything specific in mind here at the moment.

Does anybody else have any thoughts on this?



================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:352
 
-  symbol_iterator TestSym(++MutableObject.symbol_begin());
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
   Expected<MutableELFSymbol<ELF64LE> &> MutSymOrErr =
----------------
Why has this changed from your previous (correct) version?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:390
 
-  symbol_iterator DynSym(++MutableObject.dynamic_symbol_begin());
+  auto DynSym = symbol_iterator(++MutableObject.dynamic_symbol_begin());
   EXPECT_EQ(DynSym->getValue(), 0x1234U);
----------------
Why has this changed from your previous (correct) version?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:483
   // 4 additional sections, index 0, .symtab, .strtab and .shstrtab are added
-  // by yaml2obj.
+  // yaml2obj.
   EXPECT_EQ(
----------------
Bad rebase?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:616
       std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
-      101);
+      10001);
 }
----------------
Bad rebase?


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

https://reviews.llvm.org/D66063