Namespaced Wasm Imports so they don't conflict across modules, or reserved LLVM IR #1661

torch2424 · 2021-02-25T03:35:41Z

closes #1643
closes #1559

Kind of relates to #1647
Doesn't fix, but puts on the right path I think, for relates to #1648

So this PR was opened from a discussion in the tinygo slack: https://gophers.slack.com/archives/CDJD3SUP6/p1614201204223100 . Involving a conflict with wasm imports with the name of write. Which conflicts with the LLVM IR @write, and requires decoupling the LLVM IR symbol, and the wasm-import-name.

I tested this pr with the following code: https://github.com/torch2424/tinygo-compiler-playground (Which also contains the output LLVM IR of this current PR) , and ran the tests: go test -target=wasm and make wasmtest.

Screenshots of tests and things

torch2424 · 2021-02-25T03:36:11Z

Oops, looks like my fork got a little weird, Let me force push to this branch based on my current branch 😄

EDIT: Ah! The actual issue was that I tried to merge into release, and not dev 🙃 Fixing that now!

EDIT2: This is now for real good to go! 😄 🎉

deadprogram · 2021-02-25T07:18:44Z

Hello @torch2424 thanks for this contribution.

Looks like there are some problems with the CI build: https://app.circleci.com/pipelines/github/tinygo-org/tinygo/4409/workflows/09f56573-cf9f-4d30-af51-fc15dfc75753/jobs/24355/parallel-runs/0/steps/0-121?invite=true

It appears that you need to run go fmt on the test output.

Separately, please make sure you rebase off of the most current dev branch to pickup some CI fixes from yesterday/

Thank you!

functions

torch2424 · 2021-02-25T19:16:01Z

Hello @deadprogram Thanks for the tips! 😄

Looks like there are some problems with the CI build: https://app.circleci.com/pipelines/github/tinygo-org/tinygo/4409/workflows/09f56573-cf9f-4d30-af51-fc15dfc75753/jobs/24355/parallel-runs/0/steps/0-121?invite=true

It appears that you need to run go fmt on the test output.

So I tried running go fmt and gofmt -w but it just gives the same error unfortunately :(

Though, I am a bit confused why this would be happening? As I don't know why the test output is incorrectly formatted for the fix that I made? 🤔

Separately, please make sure you rebase off of the most current dev branch to pickup some CI fixes from yesterday/

I made sure to do that! Looking at my branch, I am now rebased on top of: torch2424@e76729c . Which seems to match up with what is at the HEAD of tinygo:dev 😄 🎉

torch2424 · 2021-02-25T19:59:27Z

So I tried running go fmt and gofmt -w but it just gives the same error unfortunately :(

So I did some digging around the makefile, and I just had to run the fmt in the makefile, make fmt. 😄

Things are starting to pass in the CI, so I think this is good to go @deadprogram ! 😄 🎉

aykevl · 2021-02-25T20:02:39Z

compiler/symbol.go

+			wasmImportNameAttr := c.ctx.CreateStringAttribute("wasm-import-name", info.importName)
+			llvmFn.AddFunctionAttr(wasmImportNameAttr)


Perhaps this needs an extra check that info.importName is not unset (an empty string)?

aykevl · 2021-02-25T20:13:27Z

compiler/symbol.go

+				if info.module == "" {
+					info.linkName = info.importName
+				} else {
+					info.linkName = fmt.Sprintf("tinygo_wasm_import_%s_%s", info.module, info.importName)


You are here introducing an order dependency between //go:wasm-module and //export: using //export before //go:wasm-module would change the behavior which is probably not expected.
Also, you're setting info.importName for all systems, not just WebAssembly? I guess LLVM will ignore it but it doesn't seem clean to me.

Instead of this if/else, I would move this if/else right after the for loop (putting parts[1] in a temporary variable). Something like this:

if info.module != "" { // WebAssembly import info.importName = importName } else { info.linkName = importName }

Note, in particular, that this doesn't change the link name. I think that's fine (and perhaps better): a TinyGo function name is unlikely to conflict with a name from another programming language with names such as main.someExportedName or github.com/user/pkg/subpkg.someFunction.

torch2424 · 2021-02-26T03:19:03Z

Thanks for the review @aykevl ! 😄

So I addressed the first comment!

and then for the second suggestion I tried moving it after the for loop, but then I get runtime panics (I added a screenshot), and I couldn't figure out why that was happening. 🤔 But! I was going to start heading out for the day, so I just pushed up where I'm at. But no worries, I'm sure I'll figure out what is going on. 😄

Once everything is good to go I'll add a mention 😄 👍

torch2424 · 2021-02-26T21:04:30Z

@deadprogram @aykevl Alright! So I fixed the bug I had yesterday! 😄 🎉

I saw the CI was passing, but then found an uneeded comment, removed it, and then re-ran make fmt. And my example is still working! 🎉

So I think this is good to go! 😄

deadprogram · 2021-02-27T09:24:28Z

This is some great work @torch2424 thank you for all of the updates to feedback.

Would you prefer that I squash the commits before merging, or would you like to do that yourself?

aykevl

Looks good to me!

aykevl · 2021-02-27T19:52:01Z

compiler/symbol.go

 	if decl, ok := f.Syntax().(*ast.FuncDecl); ok && decl.Doc != nil {
+
+		// Our importName for a wasm module (if we are compiling to wasm), or llvm link name
+		var importName string = ""


Just a nitpick: this can be declared slightly shorter as follows:

importName := ""

or like this:

var importName string

torch2424 · 2021-03-03T01:45:07Z

Ah! My apologies, been handling some family stuff as of lately! 😄

@deadprogram

Would you prefer that I squash the commits before merging, or would you like to do that yourself?

First, thank you for the kind words! 😄 🎉 Please feel free to squash this, I'd very much appreciate it 😄

@aykevl

Thank you for the review. Made the suggested change, this should be good to go! 😄 🎉

deadprogram · 2021-03-03T06:55:20Z

Thank you so very much for all the effort on this contribution. Now squash/merging.

torch2424 · 2021-03-03T22:19:44Z

@deadprogram @aykevl Yay! Thank you very much for the review, and squashing / merging! 😄 🎉

torch2424 changed the base branch from release to dev February 25, 2021 03:39

torch2424 added 7 commits February 25, 2021 10:39

Started logging out some stuff to figure out what's going on in external

6e2e2a0

functions

Started taking a look at the Symbol.go code for the wasi fix

cd0b7ad

Got the function name to correctly change in the symbols

5b03fda

Namespaced all of the wasm import functions

8f89d4d

Fixed issues with no module imports

3d9b708

Fixed the PR so it's actually good to go

37626e6

Ran go fmt, and removed a newline

226b52e

torch2424 force-pushed the torch2424/fix-1559 branch from 7a97d65 to 226b52e Compare February 25, 2021 18:45

Ran the fmt from the makefile

6baf53a

aykevl reviewed Feb 25, 2021

View reviewed changes

Started making the suggested fixes, but got a little stuck

5d1e43a

torch2424 added 3 commits February 26, 2021 12:49

Made requested changes

19eb66f

Ran the formatter

5bf3960

Removed a bad comment

20de259

aykevl approved these changes Feb 27, 2021

View reviewed changes

fgsch mentioned this pull request Mar 1, 2021

Testing non-browser specific WASM / WASI changes #1675

Closed

Made requested changes

11a7520

deadprogram merged commit dcff8b6 into tinygo-org:dev Mar 3, 2021

This was referenced Mar 3, 2021

Import uses the wrong module when function name is "write" #1559

Closed

Cannot import functions with the same name from different modules #1643

Closed

torch2424 deleted the torch2424/fix-1559 branch March 3, 2021 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Namespaced Wasm Imports so they don't conflict across modules, or reserved LLVM IR #1661

Namespaced Wasm Imports so they don't conflict across modules, or reserved LLVM IR #1661

Uh oh!

torch2424 commented Feb 25, 2021 •

edited

Loading

Uh oh!

torch2424 commented Feb 25, 2021 •

edited

Loading

Uh oh!

deadprogram commented Feb 25, 2021

Uh oh!

torch2424 commented Feb 25, 2021

Uh oh!

torch2424 commented Feb 25, 2021 •

edited

Loading

Uh oh!

aykevl Feb 25, 2021

Uh oh!

aykevl Feb 25, 2021 •

edited

Loading

Uh oh!

torch2424 commented Feb 26, 2021

Uh oh!

torch2424 commented Feb 26, 2021

Uh oh!

deadprogram commented Feb 27, 2021

Uh oh!

aykevl left a comment

Uh oh!

aykevl Feb 27, 2021

Uh oh!

torch2424 commented Mar 3, 2021

Uh oh!

deadprogram commented Mar 3, 2021

Uh oh!

torch2424 commented Mar 3, 2021

Uh oh!

Uh oh!

		wasmImportNameAttr := c.ctx.CreateStringAttribute("wasm-import-name", info.importName)
		llvmFn.AddFunctionAttr(wasmImportNameAttr)

Namespaced Wasm Imports so they don't conflict across modules, or reserved LLVM IR #1661

Namespaced Wasm Imports so they don't conflict across modules, or reserved LLVM IR #1661

Uh oh!

Conversation

torch2424 commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots of tests and things

Uh oh!

torch2424 commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deadprogram commented Feb 25, 2021

Uh oh!

torch2424 commented Feb 25, 2021

Uh oh!

torch2424 commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

aykevl Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torch2424 commented Feb 26, 2021

Uh oh!

torch2424 commented Feb 26, 2021

Uh oh!

deadprogram commented Feb 27, 2021

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

aykevl Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

torch2424 commented Mar 3, 2021

Uh oh!

deadprogram commented Mar 3, 2021

Uh oh!

torch2424 commented Mar 3, 2021

Uh oh!

Uh oh!

torch2424 commented Feb 25, 2021 •

edited

Loading

torch2424 commented Feb 25, 2021 •

edited

Loading

torch2424 commented Feb 25, 2021 •

edited

Loading

aykevl Feb 25, 2021 •

edited

Loading