Skip to content
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

Fix asc compilation #122

Merged
merged 4 commits into from
Sep 27, 2023
Merged

Fix asc compilation #122

merged 4 commits into from
Sep 27, 2023

Conversation

tolauwae
Copy link
Member

@tolauwae tolauwae commented Jul 9, 2023

✨ Changes

  • Use locally installed asc instead of global
  • Do not optimize code
  • Use source map generated by asc

@tolauwae tolauwae requested a review from carllocos July 9, 2023 11:20
@tolauwae tolauwae enabled auto-merge (squash) July 9, 2023 11:20
@carllocos
Copy link
Contributor

As explained here #120 (comment), I push the fix for issue #120 in this PR.

@tolauwae When time allows can you please confirm that this PR also fixes the problem for you? I'll then accept the PR.

@tolauwae
Copy link
Member Author

tolauwae commented Jul 14, 2023

@carllocos it appears to be fixed

@tolauwae
Copy link
Member Author

tolauwae commented Jul 15, 2023

@carllocos No it does not. I don't think the runtime argument is related to the issue.

I have isolated a small piece of code that causes the bug, but it doesn't always fail. Haven't tracked down when it does work:

export function invert(mode: PinVoltage): PinVoltage {
    if (mode === PinVoltage.LOW) {
        return PinVoltage.HIGH;
    } else {
        return PinVoltage.LOW;
    }
}

This is compiled to an implicit if, but it should be typed with (result i32).
So the correct WebAssembly code should be:

 (func $invert (param $mode i32) (result i32)
  (if (result i32)
   (i32.eq
    (local.get $mode)
    (global.get $PinVoltage.LOW)
   )
   (return
    (global.get $PinVoltage.HIGH)
   )
   (return
    (global.get $PinVoltage.LOW)
   )
  )
 )

This piece of code only fails for non-optimized compilation on my machine. asc = 0.27.5

@tolauwae tolauwae changed the title Use locally installed asc instead of global Fix asc compilation Jul 15, 2023
@tolauwae
Copy link
Member Author

Also this was not a problem in the old version we were using (0.17.14). So I have reverted to the main branch for the demo. @carllocos so feel free to continue working on this branch.

@carllocos
Copy link
Contributor

@tolauwae This is a peculiar problem. Not necessarily related to the options passed to the asc compiler. I will open an issue on asc repo. Hopefully, they can help me.

@carllocos
Copy link
Contributor

@tolauwae can you please provide me with the source code of your example here above

I have isolated a small piece of code that causes the bug, but it doesn't always fail.

When it fails is it for the same type error reason as mentioned here #120 (comment)?

@tolauwae
Copy link
Member Author

tolauwae commented Aug 2, 2023

You mean an example that uses it? I can't seem to find it.
But as I remember the problem was with the invert function. So any code that uses it, should not compile.

The compile error was:

type mismatch in implicit if, expected [i32] but got []

The wasm code I posted above is the correct .wat code, but asc leaves out the typing of the if.
Which causes the wat2wasm compilation error.

I do know a newer version of the AssemblyScript compiler has been released since, maybe it is fixed by now.

@tolauwae
Copy link
Member Author

tolauwae commented Aug 4, 2023

@carllocos just had a thought. If we use the source-map npm package to get the source mapping straight from AssemblyScript's own mapping, ie output.wasm.map, we don't need to get the .wast file anymore or compile it with wat2wasm.

This way we can potentially circumvent the problem. Since the binary generated by asc does appear to be fully up to spec.

And to be honest this was already a low priority TODO, since this is a faster, cleaner, and more stable approach.

I have a day off today, so I will have a stab at the refactor on Monday.

@tolauwae tolauwae self-assigned this Aug 4, 2023
@tolauwae tolauwae removed the request for review from carllocos August 4, 2023 11:55
tolauwae and others added 3 commits August 4, 2023 13:55
To compile ASC without any optimisation I removed the `03s` argument
AssemblyScript compiler can generate lines that contain the `@` symbol but have nothing to do with the source mapping (e.g., '000287d: 6e67 2e55 5446 382e 656e 636f 6465 4076  ng.UTF8.encode@v') which causes `extractLineInfo` to fail on those lines.
This commit modifies the regex to only be true for lines that start with `@ {` which ensures that `extractLineInfo` is applied to real source map lines.
@carllocos
Copy link
Contributor

@tolauwae that is an excellent idea! However, I tried to work with the same source mapper some time ago but without success :/. The mappings seemed to be only between AssemblyScript and Javascript. I do not know exactly how to get the mappings between Wasm and AssemblyScript. If you have time maybe you can look at it. Maybe there is something that I missed.

@tolauwae
Copy link
Member Author

tolauwae commented Aug 7, 2023

That would be a problem. I'll look in into it.

@tolauwae
Copy link
Member Author

@carllocos I refactored to use the AS source map. That works on my end, and the compilation problem should be circumvented now.

@tolauwae tolauwae merged commit a4e7ca1 into main Sep 27, 2023
30 checks passed
@tolauwae tolauwae deleted the fix/use-npx branch September 27, 2023 13:40
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.

AS generates invalid .wast AssemblyScript sourcemapping
2 participants