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.

Sign up for GitHub

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

Jump to bottom

Restrict //go:wasmimport to a limited number of types #3734

Merged
merged 3 commits into from
May 20, 2023
Merged

Restrict //go:wasmimport to a limited number of types #3734

merged 3 commits into from
May 20, 2023

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented May 19, 2023

This is done upstream, and I think we should do the same so that people won't rely on unsupported //go:wasmimport pragmas.

I'd like to get this in before the next release, so that we don't ship a TinyGo release with a //go:wasimport that allows too many parameter types.

@aykevl aykevl added this to the v0.28.0 milestone May 19, 2023
@github-actions
Copy link

Size difference with the dev branch:

Binary size difference 
 before   after   diff
  60684   60684      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adt7410/main.go
   9768    9768      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adxl345/main.go
  13372   13372      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pybadge ./examples/amg88xx
   8896    8896      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/main.go
  11780   11780      0   0.00%  tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/apds9960/proximity/main.go
  10636   10636      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/itsybitsy-m0/main.go
   7920    7920      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/at24cx/main.go
   8484    8484      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bh1750/main.go
   7708    7708      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/blinkm/main.go
  63128   63128      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmi160/main.go
  28064   28064      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp180/main.go
  63196   63196      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp280/main.go
  12432   12432      0   0.00%  tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/bmp388/main.go
   7724    7724      0   0.00%  tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/sram/main.go
  21896   21896      0   0.00%  tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/time/main.go
  68824   68824      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/ds3231/main.go
   4712    4712      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/easystepper/main.go
  24820   24820      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/espat/espconsole/main.go
  25020   25020      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/espat/esphub/main.go
  24828   24828      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/espat/espstation/main.go
  68572   68572      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/flash/console/spi
  64708   64708      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/flash/console/qspi
   7072    7072      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/gc9a01/main.go
  67160   67160      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/gps/i2c/main.go
  67588   67588      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/gps/uart/main.go
   8576    8576      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/hcsr04/main.go
   5688    5688      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hd44780/customchar/main.go
   5652    5652      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hd44780/text/main.go
  10716   10716      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/hd44780i2c/main.go
  14792   14792      0   0.00%  tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/hts221/main.go
  15896   15896      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hub75/main.go
  10212   10212      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/basic
  11020   11020      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/ili9341/basic
  29204   29204      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/pyportal_boing
  10240   10240      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/scroll
  11096   11096      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/ili9341/scroll
 267484  267484      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/slideshow
  12124   12124      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/lis3dh/main.go
  14148   14148      0   0.00%  tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/lps22hb/main.go
  25940   25940      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/lsm303agr/main.go
  12620   12620      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/lsm6ds3/main.go
  11084   11084      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mag3110/main.go
  10272   10272      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp23017/main.go
  10708   10708      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp23017-multiple/main.go
   9996    9996      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp3008/main.go
  66792   66792      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp2515/main.go
  21808   21808      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/microbitmatrix/main.go
  21808   21808      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit-v2 ./examples/microbitmatrix/main.go
   8636    8636      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mma8653/main.go
   8420    8420      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mpu6050/main.go
  74784   74784      0   0.00%  tinygo build -size short -o ./build/test.hex -target=p1am-100 ./examples/p1am/main.go
  12256   12256      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pico ./examples/pca9685/main.go
   6112    6112      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/pcd8544/setbuffer/main.go
   5124    5124      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/pcd8544/setpixel/main.go
   2889    2889      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino ./examples/servo
   8092    8092      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pybadge ./examples/shifter/main.go
  56188   56188      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/sht3x/main.go
  56168   56168      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/shtc3/main.go
   6420    6420      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1306/i2c_128x32/main.go
   5960    5960      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1306/spi_128x64/main.go
   5728    5728      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1331/main.go
   6420    6420      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/st7735/main.go
   6048    6048      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/st7789/main.go
  17368   17368      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/thermistor/main.go
  10284   10284      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-bluefruit ./examples/tone
  10952   10952      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/tm1637/main.go
   9536    9536      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/touch/resistive/fourwire/main.go
  12460   12460      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/touch/resistive/pyportal_touchpaint/main.go
  15852   15852      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/vl53l1x/main.go
  13876   13876      0   0.00%  tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/vl6180x/main.go
   6484    6484      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd2in13/main.go
   6036    6036      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd2in13x/main.go
   6312    6312      0   0.00%  tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd4in2/main.go
 136404  136404      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/ntpclient/main.go
 136396  136396      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/udpstation/main.go
 136624  136624      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/tcpclient/main.go
 136932  136932      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/wifinina/webclient/main.go
   7120    7120      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/ws2812
   5452    5452      0   0.00%  tinygo build -size short -o ./build/test.bin -target=m5stamp-c3          ./examples/ws2812
  61620   61620      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-nrf52840 ./examples/is31fl3731/main.go
   1565    1565      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino   ./examples/ws2812
    896     896      0   0.00%  tinygo build -size short -o ./build/test.hex -target=digispark ./examples/ws2812
  32332   32332      0   0.00%  tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/bme280/main.go
  16816   16816      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/microphone/main.go
  11484   11484      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/buzzer/main.go
  13076   13076      0   0.00%  tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/veml6070/main.go
   7012    7012      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/l293x/simple/main.go
   8968    8968      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/l293x/speed/main.go
   6980    6980      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/l9110x/simple/main.go
   9524    9524      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/l9110x/speed/main.go
   7464    7464      0   0.00%  tinygo build -size short -o ./build/test.hex -target=nucleo-f103rb ./examples/shiftregister/main.go
   8336    8336      0   0.00%  tinygo build -size short -o ./build/test.hex -target=hifive1b ./examples/ssd1351/main.go
  13364   13364      0   0.00%  tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/lis2mdl/main.go
   8516    8516      0   0.00%  tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/max72xx/main.go
  77196   77196      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/dht/main.go
  70668   70668      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/pcf8563/alarm/
   7492    7492      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/pcf8563/clkout/
  70340   70340      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/pcf8563/time/
  70604   70604      0   0.00%  tinygo build -size short -o ./build/test.hex -target=xiao ./examples/pcf8563/timer/
  12152   12152      0   0.00%  tinygo build -size short -o ./build/test.hex -target=pico ./examples/qmi8658c/main.go
   9024    9024      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/ina260/main.go
   9432    9432      0   0.00%  tinygo build -size short -o ./build/test.hex -target=nucleo-l432kc ./examples/aht20/main.go
  72068   72068      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m4 ./examples/sdcard/console/
 156796  156796      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m4 ./examples/sdcard/tinyfs/
  82608   82608      0   0.00%  tinygo build -size short -o ./build/test.hex -target=wioterminal ./examples/rtl8720dn/webclient/
  71840   71840      0   0.00%  tinygo build -size short -o ./build/test.hex -target=wioterminal ./examples/rtl8720dn/webserver/
  98752   98752      0   0.00%  tinygo build -size short -o ./build/test.hex -target=wioterminal ./examples/rtl8720dn/mqttsub/
  59824   59824      0   0.00%  tinygo build -size short -o ./build/test.hex -target=feather-m4 ./examples/i2csoft/adt7410/
   9908    9908      0   0.00%  tinygo build -size short -o ./build/test.elf -target=wioterminal ./examples/axp192/m5stack-core2-blinky/
   9044    9044      0   0.00%  tinygo build -size short -o ./build/test.uf2 -target=pico ./examples/xpt2046/main.go
3409902 3409902      0   0.00%  sum

@deadprogram
Copy link
Member

@aykevl looks like failing test on Windows related to this change https://github.com/tinygo-org/tinygo/actions/runs/5024866881/jobs/9011086657?pr=3734#step:17:10

@aykevl
compiler: only calculate functionInfo once
d4748bf 
This is a small change that's not really important in itself, but it
avoids duplicate errors in a future commit that adds error messages to
//go:wasmimport.
@aykevl
compiler: add tests for error messages
a242937 
The test is currently empty, but will be used in the next commit.
@aykevl
Copy link
Member Author

aykevl commented May 19, 2023

Should be fixed now.

@deadprogram
Copy link
Member

SInce this implements what was accepted in golang/go#59149 seems correct to me.

Any comments from @codefromthecrypt @achille-roussel @dgryski or anyone else WASM-oriented before we merge this?

achille-roussel
achille-roussel reviewed May 19, 2023
View reviewed changes
compiler/symbol.go Outdated
Comment on lines 369 to 395
func (c *compilerContext) checkWasmImport(f *ssa.Function, pragma string) {
if c.pkg.Path() == "runtime" {
// The runtime is a special case. Allow all kinds of parameters
// (importantly, including pointers).
return
}
if f.Blocks != nil {
// Defined functions cannot be exported.
c.addError(f.Pos(), fmt.Sprintf("can only use //go:wasmimport on declarations"))
return
}
for _, param := range f.Params {
typ := param.Type().Underlying()
// Check whether the type is allowed.
// Only a very limited number of types can be mapped to WebAssembly.
allowedType := false
switch typ := typ.(type) {
case *types.Basic:
switch typ.Kind() {
case types.Int32, types.Uint32, types.Int64, types.Uint64:
allowedType = true
case types.Float32, types.Float64:
allowedType = true
case types.UnsafePointer:
allowedType = true
}
}
if !allowedType {
c.addError(param.Pos(), fmt.Sprintf("%s: unsupported parameter type %s", pragma, param.Type().String()))
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go, we also validate the return values (see https://github.com/golang/go/blob/86c6b4763ed486f20cf018d0810cfd7a1fd91998/src/cmd/compile/internal/ssagen/abi.go#L392-L415)

Note the key difference is we don't allow returning pointers, only integer/floats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I missed return values! I've updated the PR accordingly.

Also thanks for mentioning pointers, because I almost missed those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achille-roussel is who I would ask, so I approve indirectly ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but heh, I should mention that some signatures are using unsafe pointers. There was that chat about nil vs zero recently. So, are we saying that folks should coerce to uintptr(0) in the rare case the signature accepted an unsafe pointer?

tinygo/src/runtime/arch_tinygowasm_malloc.go

Line 43 in 0d56dee

func libc_realloc(oldPtr unsafe.Pointer, size uintptr) unsafe.Pointer {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uintptr isn't supported actually because unlike the rest of the industry, GOARCH=wasm uses 64 bits pointers so we didn't want to risk weird bugs by implicitly truncating values when crossing import boundaries.

I guess the answer to "how do I return a pointer to a region that the host allocated in the linear memory?" is to return a uint32. It's not great but we decided it would be better to push that complexity to the code rather than pretending it can be handled safely by the compiler.

Baby steps :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw this works for me. At first, I found pointer types semantically helpful, but from a mapping perspective disturbing. I didn't realize GOARCH=wasm uses 64 bits pointers either! I kindof assumed it went with wasm 1 and 2 which don't support large memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but heh, I should mention that some signatures are using unsafe pointers.

The example you give is from the runtime, which is special-cased. The restrictions on //go:wasmimport don't apply to the runtime.

uintptr isn't supported actually because unlike the rest of the industry, GOARCH=wasm uses 64 bits pointers so we didn't want to risk weird bugs by implicitly truncating values when crossing import boundaries.

...and TinyGo uses 32-bit uintptr (because I didn't check when first adding support for wasm, I just assumed it would be 32-bits). Not allowing uintptr in //go:wasmimport does in fact avoid potential issues there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification!

@aykevl
compiler: disallow most types in //go:wasmimport
5885fce 
This is for compatibility with upstream Go.
See golang/go#59149 for more context.
@deadprogram
Copy link
Member

Thank you very much for the improvement @aykevl and to @codefromthecrypt and @achille-roussel for the review.

Now merging, thanks again!

@deadprogram deadprogram merged commit b08ff17 into dev May 20, 2023
@deadprogram deadprogram deleted the restrict-wasmimport branch May 20, 2023 09:24
@orsinium orsinium mentioned this pull request Mar 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achille-roussel achille-roussel achille-roussel left review comments

@codefromthecrypt codefromthecrypt codefromthecrypt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.28.0
Development

Successfully merging this pull request may close these issues.
4 participants
@aykevl @deadprogram @codefromthecrypt @achille-roussel