Skip to content

Renamed multiple definitions of exported function names #162

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

Closed
wants to merge 1 commit into from
Closed

Renamed multiple definitions of exported function names #162

wants to merge 1 commit into from

Conversation

jcbeyler
Copy link

@jcbeyler jcbeyler commented Nov 5, 2015

No description provided.

@sunfishcode
Copy link
Member

I realize that I've neglected #141 as of late, but I still think that as long as we have a multiple-modules-per-file setup, we should introduce new tests which specifically test that one can have multiple modules with functions with the same name when we fix the accidental occurrences of this.

@jcbeyler
Copy link
Author

jcbeyler commented Nov 5, 2015

Are you talking about multiple functions of the same name but not exported? Or are you talking about multiple functions that are then mangled because they come from different modules?

I' m open to doing whatever you like and helping out but I'd like to get this (or a form of) merged because it blocks my testing of the two test files :)

@sunfishcode
Copy link
Member

Ah. Sorry for being slow in #141; I had somehow thought that you were going to implement per-module name mangling or something and that it wasn't urgent. I've now updated and merged #141.

That said, consider yourself encouraged to make your implementation pass the new testcase too :-).

@jcbeyler
Copy link
Author

jcbeyler commented Nov 5, 2015

I see #141 so I was able to do: jcbeyler/wasm-to-llvm-prototype@53392c3

But two things that are weird: names.wast seems not to have made in the mirror testsuite repo or is it me?

Second, it seems that I still have the issue for the exports.wast file that I fixed here too.

(Thanks for doing this :))

@jcbeyler
Copy link
Author

jcbeyler commented Nov 5, 2015

Though to be fair: because of your names.wast; I have to mangle the names a bit more (otherwise LLVM won't be happy). I'll probably end up mangling the names and adding the support to handle the exports.wast case; so we can probably go and do more interesting things :)

@sunfishcode
Copy link
Member

The testsuite repo update script missed the new file. This is fixed now.

@lukewagner
Copy link
Member

With #141 fixed, can we close this PR? It seems like the changes in this PR to exports.wast shouldn't be needed since the reused names are in separate modules.

@jcbeyler
Copy link
Author

jcbeyler commented Nov 6, 2015

Yes I should be fixing it on my side with my name mangling, if I do have issues, I might come back but we can close this now

@lukewagner lukewagner closed this Nov 6, 2015
@jcbeyler
Copy link
Author

Just FYI: done now here: jcbeyler/wasm-to-llvm-prototype@94f1fc6

Supporting names.wast and exports.wast :); next on my list is forward.wast!

ngzhian pushed a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
Clarify that the natural alignment is always the size of the loaded
data.

Resolves WebAssembly#230 and resolves and WebAssembly#162.
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Mar 2, 2023
This changes the encoding of the `attribute` field, which is preserved
for future use, from `varuint32` to `uint8`. `uint8` is simpler to
encode/decode and this attribute is not likely to need `varuint32` range
even in future.

Suggested in
WebAssembly/exception-handling#161 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants