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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D64281#1600464 <https://reviews.llvm.org/D64281#1600464>, @abrachet wrote:

> Safer way for getSectionRef to get pointer to header for updated sections by explicitly getting the address of header instead of assuming it starts at byte 0 (probably this is guaranteed by the standard though?).


It //might// be guaranteed by the standard with the current state of the class, I don't know, but that will quickly go to pot as soon as you change the structure, inherit from something etc. I certainly wouldn't rely on it.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:21
+
+template <typename T> static inline DataRefImpl toDataRef(T Ptr) {
+  DataRefImpl Ref;
----------------
abrachet wrote:
> jhenderson wrote:
> > In nearly every case I can see, toDataRef is called with a uintptr_t, so do you need the template? What case doesn't work in that case?
> The `MutableRange<MutableELFSection<ELFT>>::getNew` method returns a pointer to a `MutableELFSection<ELFT>` which is used for getName and getContents which need a pointer to the new section.
I might be being blind, but I don't see any reference to getName or getContents in this patch that have anything to do with this function? I assume you meant getSectionName and getSectionContents...

How about you do the cast to uintptr_t outside this function? That would seem like the logical choice instead of creating a template with a dubious cast in it.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:39
+
+  void updateData(ArrayRef<uint8_t> Ref) {
+    Data = OwningArrayRef<uint8_t>(Ref);
----------------
setData would be a more appropriate name.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:17
+// taking an Iterable::value_type.
+template <typename T> class MutableRange {
+public:
----------------
This is the only class in this file, so the file name seems a bit off?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:21
+    bool New : 1;
+    uintptr_t Ptr : sizeof(void *) * 8 - 1;
+
----------------
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).


================
Comment at: llvm/include/llvm/Object/MutableObject.h:43-45
+  MutableRange(Iter Begin, Iter End,
+               function_ref<uintptr_t(typename Iter::value_type)> Extract) {
+    std::transform(Begin, End, std::back_inserter(Mappings),
----------------
Have you run clang-format here?


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

https://reviews.llvm.org/D64281