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

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 14 09:36:34 PST 2018

sbc100 added a comment.

In terms of placement new, the implementation can only every assume sizeof(T) storage.  Its documented that users of placement new are supposed to allocate sizeof(T) bytes and pass it in: http://en.cppreference.com/w/cpp/language/new.

I think added the extra assert seems reasonable.  I'm not familiar with object layout rules.  I think that can happen as a followup and we can add it to all three implementations.

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:
> sbc100 wrote:
> > sbc100 wrote:
> > > 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.
> > FWIW, it looks it should be easy to protect against (1) and (2) by adding:
> > 
> > ```
> >   static_assert(std::is_trivially_destructible<T>(),                             
> >                 "Symbol types must be trivially destructible");  
> > ```
> > 
> > The spec on this explicitly says `Storage occupied by trivially destructible objects may be reused without calling the destructor.`.
> > 
> > That still leaves the concerns you raise in (3) of course.
> "have always worked this way" - I think it's actually recent, looks like the change dates to 31 Oct 2017.
> Agreed that it's OK to change Wasm to match.
I guess so.   I was thinking that they have always done similar tricks involving in-place replacement of the symbols with sub-types of symbol.   But I don't know if the old behaviour was more or less risky.

  rLLD LLVM Linker


More information about the llvm-commits mailing list