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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

(taking this out-of-line due to length)

> The problem is that the original and new entries are of completely different types.

What bit is this referring to? The MutableRange is for wrapping a container (vector) of Ts, and the new values are stored as Ts.

I don't want to need to know whether I'm dealing with a new or original member at any point outside the class itself. Indeed, if I can hide the MappingType completely from the public interface, that's even better. If I'm looking up an item, I want to have an index/key into the range, which transparently works on fetching the new or old member depending on where it is in the range. That will require some plumbing to know which container (the underlying one or the "new" one) is referring to.

What about the following?

  template <typename T>
  class MutableRange {
    ArrayRef<T> Originals;
    std::map<size_t Optional<T>> Mapping;
    using value_type = Mapping::value_type;
  
  public:
    MutableRange(ArrayRef<T> Container) : Originals(Container) {
      for (size_t I = 0, Size = Container.size(); I != Size; ++I)
        Mapping.insert(std::make_pair(I, None));
    }
  
    void push_back(T t) {
      Mapping.insert(value_type(Mapping.size(), T));
    }
  
    const T& operator[](size_t Index) const {
      assert(Mapping.count(Index) != 0);
      if (Mapping[Index] == None)
        return Originals[Index];
      return Mapping[Index];
    }
  
    T& getMutable(size_t Index) {
      assert(Mapping.count(Index) != 0);
      if (Mapping[Index] == None)
        Mapping[Index] = Originals[Index];
      return Mapping[Index];
    }
  
    void remove(T t) {
      size_t Removed = 0;
      for (size_t I = 0, Removed = 0, Size = Mapping.size(); I != Size; ++I) {
        const T& Value = Mapping[I] ? *Mapping[I] : Originals[I];
        if (Value == t) {
          Mapping.erase(I);
          ++Removed;
          continue;
        }
        if (Removed == 0)
          continue;
        Mapping[I-Removed] = Mapping[I];
      }
    }
  };

I think this satisfies the requirements of the container: it provides access to the underlying container member, until it gets modified, even if members before it have been removed. It also provides a way to get a modifiable version. You could even use a different key instead of size_t e.g. a pointer, by keeping a record of the mapping between pointer and index, though that is a little trickier. If you use a pointer, you could replace the Optional with a std::unique_ptr. In turn, that might be enough to avoid co-opting the DataRefImpl behaviour so that the base class behaviour will just work (because the pointer will point to a real section, just the real section is stored in this map).

Does this work?



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:801
   const Elf_Shdr *EShdr = getSection(Sec);
-  return !isBerkeleyText(Sec) && EShdr->sh_type != ELF::SHT_NOBITS &&
-         EShdr->sh_flags & ELF::SHF_ALLOC;
+  return !ELFObjectFile<ELFT>::isBerkeleyText(Sec) &&
+          EShdr->sh_type != ELF::SHT_NOBITS &&
----------------
abrachet wrote:
> jhenderson wrote:
> > Are you sure you want to be calling the base-class method though? Isn't that going to go wrong if Sec is a modified or added section?
> When these functions are called, they get called with a DataRef pointing to the Elf_Shdr whether they are new or not. I was segfaulting here before because in `ELFObjectFile` the DataRef's point to the Elf_Shdr but in `MutableELFObject` they are indexes into the `MutableRange`. I couldn't find a way for the DataRefs to be interpreted the same way between the two classes. This isn't ideal though.
Oh, I realised I was mistaken as to which class we are in. I really don't like this change here at all. The base class shouldn't need to know that it has to call the base class version of the method. It should just work. Effectively the design here now means the base class has to know that there's a subclass that might change the meaning of some of its functions under-the-hood, and guard against that.

The solution might be to make getSection a virtual method and override it in your sub-class. What do you think of that?


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

https://reviews.llvm.org/D64281