-
Notifications
You must be signed in to change notification settings - Fork 725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eager symbol tables #1537
Eager symbol tables #1537
Conversation
My TODO here is to add more tests. I will probably add the a.o test above; if there's anything particular I should look at, LMK. |
I don't think |
I don't think Regarding |
Ah, you are correct. clang produced objects look the same.
No I don't it matters from wasm-ld perspective. Symbol names can be whatever you want them to be. But if you want to link with C/C++ you won't want wabt adding My understanding is that the |
Instead of adding entries to the symbol table as they are referenced, when writing relocatable binaries, we're going to make symbols for all functions. This allows exported functions and globals to be written with the proper exported / no_strip flags, so that resulting files will link with wasm-ld.
602f105
to
d7e70c3
Compare
Ah interesting, I didn't know that wasm-ld used the name from the symbol table; I thought it used the name from the exports. I guess I was just linking from wat to C and not the other way. Will fix! |
In the previous commit, I wrongly assumed that the globally visible name for linking was taken from the exports section, whereas actually it's from the symbol table. Therefore this patch changes to strip off the dollar, and also to make all named bindings globally visible. The exported-to-the-host binding is mostly unrelated to the visible-to-other-compilation-units binding. Unnamed definitions aren't added to the symbol table.
I rebased then pushed a followup; see f118694 for the changes. Now the test file from the original PR message looks as it did before, plus an additional entry for the export as expected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. Have you tried actually linking the wabt output with C/C++ code using wasm-ld?
src/binary-writer.cc
Outdated
|
||
std::set<string_view> seen_names_; | ||
|
||
Result Intern(const string_view& name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just call this Add
? or AddName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, apologies for the name ;) I come from the lisp world, which is what this operation is called there... But I can update it to something less jargon-ful.
Yeah, you can see a little test here: https://github.com/Igalia/ref-cpp/blob/master/milestones/m2/Makefile. The |
PRs WebAssembly#1527 and WebAssembly#1539 needed their test expectations updated after WebAssembly#1537 was merged; this patch does that. It also renames a test for consistency.
Based on #1535.
Instead of adding entries to the symbol table as they are referenced, when writing relocatable binaries, we're going to make symbols for all functions. This allows exported functions and globals to be written with the proper exported / no_strip flags, so that resulting files will link with wasm-ld.
The current state is that I've only updated the tests in the repo. However the real difference can be seen if we make a simple wasm file from LLVM:
The resulting file looks like:
If I then use
wat2wasm --relocatable
on that file, and Iwasm-objdump -dx
it, I used to get:Whereas now I get:
I.e. there's now an entry for the exported
$a
definition. This allowswasm-ld
to do the right thing.Note that the name added to the symbol table is
$a
. This causes wabt to use that name when printing$a
elsewhere in the disassembly; hence the diffs to existing tests (but only when printing relocatable binaries). Not sure what the desired UI is for that. I think it's just an internal name in the module; the external names are defined already in the imports/exports.