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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:21
+    bool New : 1;
+    uintptr_t Ptr : sizeof(void *) * 8 - 1;
+
----------------
abrachet wrote:
> jhenderson wrote:
> > Looking at this, this seems incredibly suspicious. If this is supposed to hold a pointer, what's stopping that pointer using the highest bit for memory? Don't worry so much about conserving size here, and also put the two members the other way around (i.e. Ptr before New).
> FWIW, I think the MMU's on X86 and AArch64 both only support pointer sizes of 48 bits or similar, in either case less than 64, so pointers cannot be this large. I'm not sure this would be safe for 32 bit architectures though, but we have [[ https://llvm.org/doxygen/classllvm_1_1PointerIntPair.html | PointerIntPair ]] in ADT, so this pattern is used in LLVM.
> 
> I would also add that moving the bool out effectively doubles the structs size because presumably the compiler will word align it, but then reading those members is faster now.
> 
> In either case it is crazy to worry about something like this in this stage! So I have done away with the bit field.
Note that PointerIntPair uses the *low* bits of the pointer to store data, which is a generally safe (depending on your reading of the c++ standard) thing to do for aligned values. 64-bit pointers are indeed smaller, but for instance there's an arm64 pointer authentication extension, which uses those extra bits to store pointer "signatures". And yeah, there are no free bits on 32-bit pointers. :)


================
Comment at: llvm/include/llvm/Object/MutableObject.h:46
+                     uintptr_t Extracted = Extract(Value);
+                     assert(Extracted <= (UINTPTR_MAX >> 1) &&
+                            "returned type too large");
----------------
I guess this assert isn't needed now that you store the full uintptr_t?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:166-170
+  auto MutObjIter = MutableObject.section_begin();
+  for (const auto &ObjFileIter : ObjFile.sections()) {
+    TestIters(ObjFileIter, MutObjIter);
+    ++MutObjIter;
+  }
----------------
using `llvm::zip(ObjFile.sections(), MutableObject.sections())` is a bit cleaner way to handle parallel iteration (but don't forget to assert that the range sizes are the same). 

With something like `EXPECT_THAT(MutableObject.sections(), testing::ContainerEq(ObjFile.sections())` this check would be a one-liner, but it would require a bit more plumbing to make sure the elements are comparable, so it may not be worth it if this is just a one-off thing...


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

https://reviews.llvm.org/D64281