interp: rewrite entire package #1430

aykevl · 2020-10-07T21:02:15Z

For a full explanation, see interp/README.md. In short, this rewrite is a redesign of the partial evaluator which improves it over the previous partial evaluator. The main functional difference is that when interpreting a function, the interpretation can be rolled back when an unsupported instruction is encountered (for example, an actual unknown instruction or a branch on a value that's only known at runtime). This also means that it is no longer necessary to scan functions to see whether they can be interpreted: instead, this package now just tries to interpret it and reverts when it can't go further.

This new design has several benefits:

Most errors coming from the interp package are avoided, as it can simply skip the code it can't handle. This has long been an issue.
The memory model has been improved, which means some packages now pass all tests that previously didn't pass them.
Because of a better design, it is in fact a bit faster than the previous version.

This means the following packages now pass tests with tinygo test:

hash/adler32: previously it would hang in an infinite loop
math/cmplx: previously it resulted in errors

This PR depends on #1412, which should be merged first. Also, it requires a change to the go-llvm package.

deadprogram · 2020-10-17T20:21:02Z

#1412 has been merged. Not sure what change is needed to go-llvm or if that has already been made.

aykevl · 2020-10-17T20:32:58Z

No, a different change is needed. I'll get back to this, it really helps to make TinyGo a more correct Go compiler.

aykevl · 2020-10-27T23:37:33Z

Looks like this almost works, but needs some work to support some Go versions.

aykevl · 2020-10-29T12:17:10Z

Note that this should probably be merged after the next release, to give it some time to find possible bugs.

deadprogram · 2020-10-29T12:31:27Z

Indeed 😺

aykevl · 2020-11-04T22:47:02Z

Updated the PR, I think it is ready now. This newer version fixes an issue with the append builtin and (in a separate commit) fixes a bug with an assertion in the coroutine lowering pass (#1409 and #733).

ofauchon · 2020-11-13T22:33:38Z

With tinygo (dev branch), I could not compile my application:

traceback:
/usr/lib/go/src/bytes/bytes.go:
  br i1 %14, label %for.body, label %for.done, !dbg !2460
/home/olivier/go/src/github.com/jacobsa/crypto/cmac/subkey.go:29:27:
  %9 = call { i8*, i32, i32 } @bytes.Repeat(i8* %6, i32 %7, i32 %8, i32 16, i8* undef, i8* undef), !dbg !2457
github.com/jacobsa/crypto/cmac/<init>:
  call void @"github.com/jacobsa/crypto/cmac.init#1"(i8* undef, i8* undef), !dbg !2452

Then I applied the 3 commits

git cherry-pick b3794a9ea8af6b7c71112a15ad54470db060c66c
git cherry-pick 54ce74a784e05ac6c7d009aa840c802cf66fa1b6
git cherry-pick 0b3b692d533630e306bc815ac3387073bd954f8c
go install

This fixed the previous error !

Thanks

Olivier

Previously, EmitPointerPack would generate an out-of-bounds read from an alloca. This commit fixes that by creating an alloca of the appropriate size instead of using the size of the to-be-packed data (which might be smaller than a pointer). I discovered this error while working on a rewrite of the interp package, which checks for out-of-bounds reads and writes. There I discovered this issue when the image package was compiled.

Because the parentHandle parameter wasn't always set to the right value, the coroutine lowering pass would sometimes panic with "trying to make exported function async" even though there was no exported function involved. Therefore, it should unconditionally be set to avoid this. The parent function doesn't always have the parentHandle function parameter set because it can only be set after defining a function, not when it is only declared.

niaow

Looks good, one minor comment

interp/interp.go

niaow · 2020-11-28T23:47:48Z

interp/memory.go

+			}
+			mv.objects[objectIndex] = obj
+		}
+		// TODO: also mark objects referenced by this object.


Is this going to break anything now?

In my testing, no. However it is technically unsound and should probably be fixed.

So, does this require additional work now, or should we merge and deal with in future effort?

Yes, I think I shouldn't be lazy and fix this (even if it doesn't seem to affect the behavior right now).

interp/memory.go

For a full explanation, see interp/README.md. In short, this rewrite is a redesign of the partial evaluator which improves it over the previous partial evaluator. The main functional difference is that when interpreting a function, the interpretation can be rolled back when an unsupported instruction is encountered (for example, an actual unknown instruction or a branch on a value that's only known at runtime). This also means that it is no longer necessary to scan functions to see whether they can be interpreted: instead, this package now just tries to interpret it and reverts when it can't go further. This new design has several benefits: * Most errors coming from the interp package are avoided, as it can simply skip the code it can't handle. This has long been an issue. * The memory model has been improved, which means some packages now pass all tests that previously didn't pass them. * Because of a better design, it is in fact a bit faster than the previous version. This means the following packages now pass tests with `tinygo test`: * hash/adler32: previously it would hang in an infinite loop * math/cmplx: previously it resulted in errors This also means that the math/big package can be imported. It would previously fail with a "interp: branch on a non-constant" error.

aykevl · 2020-12-12T15:56:35Z

Okay, I've written the missing bits (see the force push for a diff). Ready for review again.

deadprogram · 2020-12-22T07:22:47Z

What is the status of this PR? It would appear that all feedback has now been addressed.

aykevl · 2020-12-22T12:49:03Z

Ready from my POV but maybe @niaow wants to comment.

deadprogram · 2020-12-22T14:54:13Z

Here we go!! 🚀 🦝

aykevl · 2020-12-24T01:05:17Z

Unfortunately this appears to have broken at least support for the Circuit Playground Express. Even examples/blinky1 doesn't work anymore. ~~It appears to be related to machine.RingBuffer (the Put and Used methods).~~ I haven't investigated further yet.

deadprogram · 2020-12-24T12:29:53Z

OK that seems odd, because the TinyHCI tests passed on CPE board after the merge.

However I am able right now to verify the problem on CPE myself. Also confirm it is commit 30df912 that causes the problem.

aykevl · 2020-12-24T16:05:57Z

I'll take a look, but probably not before Saturday.

deadprogram · 2020-12-26T14:38:34Z

I wonder if it is specifically related to USB-CDC code somehow?

I notice that now that this merged into dev in subsequent PRs that most of the ARM boards are failing, except for Microbit which along with RISC-V and AVR boards are still passing TinyHCI:
https://github.com/tinygo-org/tinygo/pull/1543/checks?check_run_id=1601942130

aykevl · 2020-12-26T14:45:23Z

Yes, probably. I will investigate soon.

aykevl · 2020-12-26T14:46:11Z

What I've discovered so far is that -opt=1 seems to mask the issue. Not sure why or how yet, don't rely on it.

aykevl · 2020-12-26T23:25:13Z

I have figured out the issue, the fix is in #1547.

This was referenced Oct 18, 2020

error: interp: branch on a non-const-propagated constant expression #437

Closed

unix: support command line arguments #1410

Closed

This was referenced Oct 26, 2020

spi: remove machine.SPI and replace with drivers.SPI interface tinygo-org/drivers#208

Merged

Maps and init function not playing nice: Redux #1307

Closed

aykevl force-pushed the interp-rewrite branch from 6ce02a0 to 539a81b Compare October 27, 2020 22:12

aykevl marked this pull request as ready for review October 27, 2020 22:16

aykevl mentioned this pull request Oct 27, 2020

compiler: implement negate for complex numbers #1464

Merged

sago35 mentioned this pull request Oct 28, 2020

big.NewInt build failed #1449

Closed

aykevl force-pushed the interp-rewrite branch 2 times, most recently from 0749d15 to 87258f6 Compare October 29, 2020 12:02

sago35 mentioned this pull request Nov 2, 2020

atsamd5x: improve USBCDC #1452

Closed

aykevl force-pushed the interp-rewrite branch from 87258f6 to 946be58 Compare November 4, 2020 22:43

aykevl mentioned this pull request Nov 4, 2020

panic: trying to make exported function async #733

Closed

aykevl force-pushed the interp-rewrite branch from 946be58 to 0b3b692 Compare November 4, 2020 23:01

aykevl mentioned this pull request Nov 6, 2020

Error compiling starlark-go to wasm #1486

Closed

aykevl added 2 commits November 22, 2020 23:01

aykevl force-pushed the interp-rewrite branch from 0b3b692 to 87180f6 Compare November 22, 2020 22:02

deadprogram requested a review from niaow November 28, 2020 17:41

niaow requested changes Nov 28, 2020

View reviewed changes

sykesm mentioned this pull request Dec 2, 2020

Vendored packages used by the standard library cannot be resolved #1518

Closed

aykevl force-pushed the interp-rewrite branch from 87180f6 to b331e3e Compare December 12, 2020 15:53

niaow self-requested a review December 13, 2020 00:11

niaow approved these changes Dec 22, 2020

View reviewed changes

deadprogram merged commit 30df912 into dev Dec 22, 2020

deadprogram deleted the interp-rewrite branch December 22, 2020 14:54

deadprogram mentioned this pull request Dec 26, 2020

GBA: interrupt handlers are not called #1544

Closed

aykevl mentioned this pull request Dec 26, 2020

interp: fix alignment of untyped globals #1547

Merged

This was referenced Jan 13, 2021

interp: improve support for load/store instructions #987

Closed

interp: unknown GEP #1110

Closed

aykevl mentioned this pull request Mar 5, 2021

map[string]uint64 generates incorrect wasm? #1686

Closed

This was referenced Mar 14, 2021

[BUG] Compile error when value type of a map is byte or int8 #1224

Closed

TinyGo fails to compile a Vecty program #1282

Closed

interp: rewrite entire package #1430

interp: rewrite entire package #1430

Uh oh!

Conversation

aykevl commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deadprogram commented Oct 17, 2020

Uh oh!

aykevl commented Oct 17, 2020

Uh oh!

aykevl commented Oct 27, 2020

Uh oh!

aykevl commented Oct 29, 2020

Uh oh!

deadprogram commented Oct 29, 2020

Uh oh!

aykevl commented Nov 4, 2020

Uh oh!

ofauchon commented Nov 13, 2020

Uh oh!

niaow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niaow Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

deadprogram Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aykevl commented Dec 12, 2020

Uh oh!

deadprogram commented Dec 22, 2020

Uh oh!

aykevl commented Dec 22, 2020

Uh oh!

deadprogram commented Dec 22, 2020

Uh oh!

aykevl commented Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deadprogram commented Dec 24, 2020

Uh oh!

aykevl commented Dec 24, 2020

Uh oh!

deadprogram commented Dec 26, 2020

Uh oh!

aykevl commented Dec 26, 2020

Uh oh!

aykevl commented Dec 26, 2020

Uh oh!

aykevl commented Dec 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aykevl commented Oct 7, 2020 •

edited

Loading

aykevl commented Dec 24, 2020 •

edited

Loading