This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Check function signatures at link time
ClosedPublic

Authored by sbc100 on Nov 22 2017, 11:16 AM.

Details

Summary

This change allows checking of function signatures but
does not yes enable it by default. In this mode, linking
two objects that were compiled with a different signatures
for the same function will produce a link error.

New options for enabling and disabling this feaure have been
added: (--check-signatures/--no-check-signatures).

Event Timeline

sbc100 created this revision.Nov 22 2017, 11:16 AM
sbc100 edited the summary of this revision. (Show Details)Nov 22 2017, 11:20 AM
sbc100 added reviewers: dschuff, ruiu.
sbc100 updated this revision to Diff 123991.Nov 22 2017, 11:21 AM
sbc100 edited the summary of this revision. (Show Details)

Remove elf test

sbc100 updated this revision to Diff 123992.Nov 22 2017, 11:25 AM
  • add unreachable
sbc100 updated this revision to Diff 124004.Nov 22 2017, 12:40 PM
  • Fix exports
  • don't export stack pointer
sbc100 updated this revision to Diff 124025.Nov 22 2017, 5:09 PM
  • don't export stack pointer
  • handle lazy symbols
ruiu edited edge metadata.Nov 27 2017, 10:48 AM

What is the motivation to add a new option to control the feature? I mean, is there a reason not to enable this feature by default?

Its something that I/we would like to experiment with. Turning it on my default would be great, but it would currently break many/most C programs because there are common cases where functions are declare and defined with different signatures. Specifically in the musl C library which we use for a lot of our testing the main(), _init() and _fini() functions all have this issue. I'm trying to find a way to enable to this without have these common cases cause failure. In the mean time it is useful to have this feature behind a flag to that people can experiment with it. For example I imagine the rust folks might be want to turn this on as it can catch errors earlier and give better errors than the runtime errors that result.

ruiu added inline comments.Nov 27 2017, 1:02 PM
wasm/Config.h
26โ€“33

Sort

wasm/Driver.cpp
296โ€“297

We usually leave Config members uninitialized if they are overwritten unconditionally (so that we can catch errors using ubsan) and use false or true as the third parameter to hasFlags.

308

Is this related change?

wasm/Options.td
32

In other ports, we use the exact the same name for variables. I'd name this no_check_signatures.

39

Ditto -- check_signatures

wasm/SymbolTable.cpp
80

Please add a function comment.

98

Please add a function comment.

wasm/SymbolTable.h
56

Can you run clang-format-diff on this patch?

58

The definition of this function in a .cpp file doesn't have Mutable parameter. Does it compile?

63

We named this Symtab in ELF and COFF.

wasm/Symbols.h
106

How large is WasmSignature? I wonder if it copying a value is a right approach.

sbc100 updated this revision to Diff 124470.Nov 27 2017, 2:25 PM
sbc100 marked 8 inline comments as done.
  • clang format
  • review feedback
sbc100 marked an inline comment as done.Nov 27 2017, 2:25 PM
sbc100 added inline comments.
wasm/Driver.cpp
308

I had thought it would make sense for the default value to be the one that the variable itself is initialized to, so I was trying to be consistent, but after your above comment it seems that its more desirable to set the defaults here in the code.

wasm/SymbolTable.h
58

Reverted.. this method was never referecnced

63

I found this confusing because there is also global with that name. If you agree I could make a separate change to the others too?

wasm/Symbols.h
106

Its a uint32_t for the return type and a uint32_t for each param type. The reason I ended up doing it this way is for synthetic symbols (mostly main() I guess) which don't have a signature we can point to in an input object.

Ideally this class would be split into separate subclass and stored as union like we do using the SymbolBody in the ELF linker. That way the we could reduce the size of the each symbol to sizeof the largest symbol type. If its OK with you I'll leave that as a later optimization.

ruiu added inline comments.Nov 27 2017, 2:38 PM
wasm/SymbolTable.h
63

Sure. I just wanted to make all the ports as consistent as possible.

wasm/Symbols.h
106

Then does it make sense to make it a pointer to WasmSignature (and represent "no value" as a nullptr)? You are handling pointers to WasmSignature in many places in this patch and copy it only when storing them to this variable, which seems a bit odd to me. If you can handle them as pointers, you can store them as pointers, no?

sbc100 updated this revision to Diff 124482.Nov 27 2017, 3:25 PM
sbc100 marked an inline comment as done.
  • rebase on top of split out changes
sbc100 updated this revision to Diff 124495.Nov 27 2017, 5:11 PM
  • Don't store sigs in symbols
ruiu added inline comments.Nov 27 2017, 5:35 PM
wasm/Driver.cpp
207

Can you add a brief comment about this? I think this is dummy data, and this code is not technically correct, as synthetic undefined symbols should have expected type, right?

wasm/SymbolTable.cpp
127โ€“134

Why is this an exception?

sbc100 updated this revision to Diff 124503.Nov 27 2017, 5:45 PM
sbc100 marked 2 inline comments as done.
  • review feedback
sbc100 added inline comments.Nov 27 2017, 5:45 PM
wasm/SymbolTable.cpp
127โ€“134

This was part of an experiment. Reverted.

ruiu added inline comments.Nov 27 2017, 5:49 PM
wasm/SymbolTable.cpp
127โ€“134

Did you forgot to actually delete the code?

sbc100 updated this revision to Diff 124594.Nov 28 2017, 9:39 AM
  • remove exception for main
ruiu accepted this revision.Nov 28 2017, 12:30 PM

LGTM

wasm/Symbols.h
106

Please run clang-format-diff on this patch.

wasm/Writer.cpp
293

be exported because it's

300โ€“306

If you flip the innermost if condition, you could write this as

if (ExportFuncs)
  for (Symbol *S : Symtab->getSymbols())
    if (S->isFunction() && !S->isWeak() && S->isDefined())
      ++NumExports;
341

Ditto

599โ€“601

These two ifs are the same as one if (WasmFile->memories().size() > 1)

This revision is now accepted and ready to land.Nov 28 2017, 12:30 PM
sbc100 updated this revision to Diff 124658.Nov 28 2017, 3:43 PM
  • rebase
sbc100 updated this revision to Diff 124864.Nov 29 2017, 5:37 PM
  • split out that other part of this change regarding exporting by default

This new version of this change is a strict subset of the one you approved. Sorry for the churn, but I thought it better to go with this smaller change.

sbc100 edited the summary of this revision. (Show Details)Nov 29 2017, 5:39 PM
ruiu added a comment.Nov 29 2017, 5:39 PM

Please go ahead and submit.

This revision was automatically updated to reflect the committed changes.