Page MenuHomePhabricator

[WebAssembly] Error on relocations against undefined data symbols.
ClosedPublic

Authored by sbc100 on Apr 18 2019, 11:44 AM.

Details

Summary

We can't meaningfully resolve certain types of relocations against
undefined data symbols. Previously when --allow-undefined was used
we were treating such relocation much like weak data symbols and
simply inserting zeros. This change turns such use cases in to an
error.

This means that --allow-undefined is no longer effective for data
symbols.

Fixes https://bugs.llvm.org/show_bug.cgi?id=40364

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

sbc100 created this revision.Apr 18 2019, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 11:44 AM
sbc100 updated this revision to Diff 195811.Apr 18 2019, 1:52 PM

fix debug info tests

sbc100 updated this revision to Diff 195812.Apr 18 2019, 1:54 PM

remove debugging

As mentioned on the issue, I'd prefer if we got imports for such globals, but if that's not possible then definitely an error instead of just silently emitting a 0 is better, lgtm.

As mentioned on the issue, I'd prefer if we got imports for such globals, but if that's not possible then definitely an error instead of just silently emitting a 0 is better, lgtm.

We can get imports I think, but it will have to be a codegen option I think, the linker can't convert them. So I see it as a separate compiler issue.

ruiu added a comment.Apr 18 2019, 6:36 PM

Although I understand that reporting an error is sometimes useful, are you sure that that's the behavior that users want? I wonder if we would end up adding --really-allow-undefined in the future to override this new behavior.

lld/wasm/InputFiles.cpp
144 ↗(On Diff #195812)

Can you add a comment for this code block?

Although I understand that reporting an error is sometimes useful, are you sure that that's the behavior that users want? I wonder if we would end up adding --really-allow-undefined in the future to override this new behavior.

The problem is that this would actually mean "treat all data symbols as weak undefined with no way to provide them". Which I guess somebody might want but its hard to imagine. the current users of --allow-undefined kind of expect to be able to provide the undefined symbols at runtime.

In reality what we will probably end up doing is linker relaxation to allow "i32.load <foo>" to re-written to "global.get <foo>", although I'm not sure when we will end up going there.

ruiu accepted this revision.Apr 22 2019, 1:31 AM

LGTM

This revision is now accepted and ready to land.Apr 22 2019, 1:31 AM
This revision was automatically updated to reflect the committed changes.