[PATCH] D43112: [WebAssembly] Use Symbol class heirarchy. NFC.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/Symbols.h:231-238
+T *replaceSymbol(Symbol *S, ArgT &&... Arg) {
+  static_assert(sizeof(T) <= sizeof(SymbolUnion), "Symbol too small");
+  static_assert(alignof(T) <= alignof(SymbolUnion),
+                "SymbolUnion not aligned enough");
+  assert(static_cast<Symbol *>(static_cast<T *>(nullptr)) == nullptr &&
+         "Not a Symbol");
+  return new (S) T(std::forward<ArgT>(Arg)...);
----------------
ncw wrote:
> Yikes, this is scary!
> 
> 1. For one, doing `make<SymbolUnion>` means that the actual symbol's destructor won't be called at the program end. We're relying on the Symbol classes being "simple" with trivial destructors, but what if someone forgets and sticks a std::vector in there as a member...? It's not ideal, quite fragile.
> 2. And similarly, replaceSymbol doesn't deallocate the previous data in the union, in just writes straight over it. The new object will be OK, but the previous one will leak any members. Again not a problem as long as the Symbols all have trivial dtors, but it feels like an accident waiting to happen.
> 3. Is it technically Undefined Behaviour? You seem to be relying on the exact details of the base-to-derived pointer adjustments. First we allocate the union, then reinterpret-cast it to a `Symbol*`, then placement-construct an UndefinedFunction symbol. Then the first bit of UB happens, we assume that the newly-constructed UndefinedFunction, when cast to a Symbol, has the same address. That is, we assume the following:
> 
> `(void*)(BaseClass*)(new (addr) DerivedClass) == (void*)addr`
> 
> Then the second bit of UB happens when we do replaceSymbol and make the same assumption again, but in replaceSymbol we further assume that all derived classes will construct their Symbol base at the same offset within the memory block we're placement-constructing them in (it's just another assumption about object layout).
> 
> The basic assumption is that base classes are constructed before the derived class, and the base class is at offset zero within the memory when placement-constructed at a specific address.
> 
> I can see it's just copying the existing LLD code. What you're trying to achieve is to dynamically change the derived type of an object, so that previously-created pointers to the base class remain valid as pointers to the new object's base class.
> 
> There is a solution I can think of that's "safe". Instead of doing `reinterpret_cast<Symbol*>(make<SymbolUnion>())`, why not instead put the Kind member next to the union, something like this: `struct SymbolHandle { int Kind; SymbolUnion U; }` and then store pointers to the union-with-kind. You can then safely destruct the union using a switch, and safely replace the union too; and finally, you can add automatic casting operators that allow casting a SymbolHandle to an `UndefinedFunction&` etc with an assertion on Kind and a cast on the appropriate member of the union.
I think perhaps you raise some good points, but I see this as more of an lld wide discussion.   The other ports have always working in this way AFAICT, and it was always my intention to have the wasm port do the same thing.

Perhaps @ruiu can add some documentation about this technique, why it is actually safe in the this context, and what the motivating factors are for using it.

In any case I don't think we should block this change in this design discussion.   Lets make the linkers consistent and iterate (together) from there.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43112