This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix local references to weak aliases
ClosedPublic

Authored by sbc100 on Dec 20 2017, 5:25 PM.

Details

Summary

When weak aliases are used with in same translation
unit we need to be able to directly reference to alias
and not just the thing it is aliases. We do this by
defining both a wasm import and a wasm export in this
case that result in a single Symbol. This change is
a partial revert of rL314245. A corresponding lld
change address the previous issues we had with this.

See: https://github.com/WebAssembly/tool-conventions/issues/34

LLD change: https://reviews.llvm.org/D41473

Event Timeline

sbc100 created this revision.Dec 20 2017, 5:25 PM
sbc100 added a reviewer: ncw.Dec 20 2017, 5:25 PM
sbc100 edited the summary of this revision. (Show Details)Dec 20 2017, 5:38 PM
sbc100 updated this revision to Diff 127811.Dec 20 2017, 5:49 PM
sbc100 edited the summary of this revision. (Show Details)
  • update test
This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2017, 6:31 PM
This revision was automatically updated to reflect the committed changes.
ncw added a comment.Dec 21 2017, 3:18 AM

Ouch, hacky hacky!

I can see why you're tempted to do a quick fix to "get it working", so that it's not broken over Christmas. I don't mind this - as the Wasm-LLD maintainer you can commit what you want. On the other hand, no-one's using it in production yet and if it's broken until we get round to the "proper" fix in January maybe that's OK.

To me, taking half a step back like this feels like a more scary change than pressing on and fixing the index used by relocations. Plus, this doesn't fix table relocations, it's only a half fix, so it doesn't even put the code back into a "working" state.

My first step for fixing the table relocations would be to revert this and carry on from where we were before. If that's your intention too, I guess I don't need to review the change, if the plan is for it to be reverted soon.

Thanks for the prompt response on this issue anyway! I was expecting it sit and wait over Christmas, not to cause a panic before we leave on holiday.

https://reviews.llvm.org/D41473

In D41472#961910, @ncw wrote:

Ouch, hacky hacky!

I can see why you're tempted to do a quick fix to "get it working", so that it's not broken over Christmas. I don't mind this - as the Wasm-LLD maintainer you can commit what you want. On the other hand, no-one's using it in production yet and if it's broken until we get round to the "proper" fix in January maybe that's OK.

To me, taking half a step back like this feels like a more scary change than pressing on and fixing the index used by relocations. Plus, this doesn't fix table relocations, it's only a half fix, so it doesn't even put the code back into a "working" state.

My first step for fixing the table relocations would be to revert this and carry on from where we were before. If that's your intention too, I guess I don't need to review the change, if the plan is for it to be reverted soon.

Thanks for the prompt response on this issue anyway! I was expecting it sit and wait over Christmas, not to cause a panic before we leave on holiday.

I agree that we are pushing the limits here of modeling the symbol table via imports and exports, so it can seem a little hacky. In the long run I agree that we may need to model the symbol table more explicitly, but I think that is a large scale change, that will take some planning. This kind of incremental fix isn't meant to block the way for more sweeping one. I actually think that modeling a weak alias as both an import and an export is quite a neat tick :) The import can then be resolved by its own export, in the absence of any strong symbol. In a sense the object both exports a version of this symbol and also depends on an external version.

Hopefully this fix will also help with the table relocations issue you found since it gives the alias and the non-alias different function indexes.

I'm sorry I landed this unilaterally. I very much appreciate the work you've been doing on wasm+lld.

ncw added a comment.Dec 21 2017, 3:57 PM

I agree that we are pushing the limits here of modeling the symbol table via imports and exports, so it can seem a little hacky. In the long run I agree that we may need to model the symbol table more explicitly, but I think that is a large scale change, that will take some planning. This kind of incremental fix isn't meant to block the way for more sweeping one. I actually think that modeling a weak alias as both an import and an export is quite a neat tick :) The import can then be resolved by its own export, in the absence of any strong symbol. In a sense the object both exports a version of this symbol and also depends on an external version.

I'm not so sure about "neat". Here's how it is in my head: in the frontend (LLVM IR) the fundamental unit of linkage is the "symbol", pretty-much identical to the C concept. In the LLD linker, the "symbol" is also the fundamental unit for all function/global relocations. When the frontend wants one function to call another, that's always communicated to the linker via the "symbol" abstraction, which handles weak/strong and all sort of other link-time redirections.

So we really want to be sure that the LLD symbols map one-to-one to the frontend symbols. The "clean" way to do that is to have the frontend IR symbols map one-to-one to "something" in the object file, and then back out again into LLD. Then we have a fairly clean mental model that we can reason about - MCSymbols are encoded into the object file, then reassembled one-to-one into lld::wasm::Symbols in LLD.

That's why I liked not having symbols duplicated in the imports/exports. I get worried when I see logic one place for duplicating things, and then de-duplicating elsewhere, because it needs to be kept in-sync, and it's harder to understand what's going on.

On the other hand, it works well mentally to take those MCSymbols, split them into two non-overlapping sets, the imports/exports, and then reassemble them back again in LLD. I do hope that we can move back to that model, for the sake of simplicity.

Hopefully this fix will also help with the table relocations issue you found since it gives the alias and the non-alias different function indexes.

Hmm, you're right, I believe that the fix works. But it's harder to see, since it doesn't fall out the modelling exactly.

We have two different concepts to model:

  • Weak/strong. A "weak" symbol could be resolved by the linker to point to another translation unit. A strong symbol will always refer in LLD to the function body that the frontend knew about, or cause a linker error otherwise. If I understand correctly, the idea of putting some symbols in both imports and exports is to model the properties of weak symbols? The idea that the symbol is provided by this translation unit, but also could be brought in from another file. Only weak symbols will have the imports/exports duplication.
  • Aliases. In the case of strong aliases (I'm guessing here...) the Wasm file will still have two exports for the same symbol, but no import(?) We certainly have to emit the two exports, since the file is genuinely providing two different symbols for other files to call. And it's not a weak alias, so it won't generate an additional import in your code(?)

Strong/weak wasn't really a problem before, that side of things actually worked. It was only aliases where the model didn't match the implementation.

The change to go back to duplicating imports/exports doesn't really fix the aliases problem, it just changes the weak/strong implementation (which wasn't the "root cause" of the issue). And by some clever coding you've altered the weak-symbol implementation to now handle weak-aliases. Cunning! But it's a case of the "tail wagging the dog" when a problem with aliases causes the weak-symbol modelling to be changed to accommodate it.

In the case of strong aliases, we still have a mental mismatch between the modelling and the implementation. We still have two symbols, and the single function entry in the "Wasm index space of functions". It's only by luck that we get away with it. Having said that, I can't think of a bug that would result. But it's awkward, and it's bound to break something eventually.


Whew! Long comment, sorry. I don't mean to argue, I just like to give my reasons if I'm suggesting a particular idea!

I think we're agreed. The change you've made here is OK and should work, but a follow-up fix would be nice. I could have a go at that in January, but feel free if you want to look at it sooner.