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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw 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)...);
----------------
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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43112