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

interp: correctly mark functions as modifying memory #4742

Merged
merged 2 commits into from
Feb 21, 2025
Merged

interp: correctly mark functions as modifying memory #4742

merged 2 commits into from
Feb 21, 2025

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Feb 18, 2025

This is a fix for an interp bug I found in #4732.
Making a draft PR since I want to see the impact of this, it could be big. I might need to modify the PR.

@aykevl aykevl mentioned this pull request Feb 18, 2025
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

Size difference with the dev branch:

Binary size difference 
 flash                          ram
 before   after   diff          before   after   diff
  12568   12548    -20  -0.16%    6976    6984      8   0.11% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/touch/resistive/pyportal_touchpaint/main.go
   5772    5768     -4  -0.07%    9522    9522      0   0.00% '-xesppie' is not a recognized feature for this target (ignoring feature)
   8376    8376      0   0.00%    2320    2320      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/at24cx/main.go
  67668   67668      0   0.00%    6360    6360      0   0.00% tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/gps/i2c/main.go
  10380   10380      0   0.00%    4788    4788      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp23017-multiple/main.go
   2841    2841      0   0.00%     558     558      0   0.00% tinygo build -size short -o ./build/test.hex -target=arduino ./examples/servo
   5844    5844      0   0.00%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1331/main.go
   6764    6764      0   0.00%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/st7735/main.go
   6692    6692      0   0.00%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/st7789/main.go
   6488    6488      0   0.00%    2320    2320      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd2in13/main.go
   6152    6152      0   0.00%    2312    2312      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd2in13x/main.go
   6400    6400      0   0.00%    2320    2320      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/waveshare-epd/epd4in2/main.go
   1581    1581      0   0.00%     598     598      0   0.00% tinygo build -size short -o ./build/test.hex -target=arduino   ./examples/ws2812
   1056    1056      0   0.00%     180     180      0   0.00% tinygo build -size short -o ./build/test.hex -target=digispark ./examples/ws2812
  32256   32256      0   0.00%    4780    4780      0   0.00% tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/bme280/main.go
   9600    9604      4   0.04%    4748    4748      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adxl345/main.go
   8160    8164      4   0.05%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bh1750/main.go
   7464    7468      4   0.05%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/blinkm/main.go
  70688   70692      4   0.01%    3660    3656     -4  -0.11% tinygo build -size short -o ./build/test.hex -target=pinetime     ./examples/bma42x/main.go
   8008    8012      4   0.05%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/hcsr04/main.go
  10756   10760      4   0.04%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mag3110/main.go
   8272    8276      4   0.05%    4748    4748      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mma8653/main.go
   8180    8184      4   0.05%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mpu6050/main.go
  10204   10208      4   0.04%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/tm1637/main.go
  15368   15372      4   0.03%    4748    4748      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/vl53l1x/main.go
  13848   13852      4   0.03%    4748    4748      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/vl6180x/main.go
   6984    6988      4   0.06%    4780    4780      0   0.00% tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/ws2812
   8816    8824      8   0.09%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/main.go
  27796   27804      8   0.03%    4780    4780      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp180/main.go
   4620    4628      8   0.17%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/easystepper/main.go
   7224    7232      8   0.11%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/gc9a01/main.go
  68212   68220      8   0.01%    6504    6504      0   0.00% tinygo build -size short -o ./build/test.hex -target=feather-m0 ./examples/gps/uart/main.go
   5832    5840      8   0.14%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hd44780/customchar/main.go
   5784    5792      8   0.14%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hd44780/text/main.go
  10560   10568      8   0.08%    4748    4748      0   0.00% tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/hd44780i2c/main.go
  16152   16160      8   0.05%    2360    2360      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/hub75/main.go
  10776   10784      8   0.07%    4868    4876      8   0.16% tinygo build -size short -o ./build/test.hex -target=xiao ./examples/ili9341/basic
  10864   10872      8   0.07%    4868    4868      0   0.00% tinygo build -size short -o ./build/test.hex -target=xiao ./examples/ili9341/scroll
  11764   11772      8   0.07%    4780    4780      0   0.00% tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/lis3dh/main.go
  26492   26500      8   0.03%    2328    2328      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/lsm303agr/main.go
  12476   12484      8   0.06%    4788    4788      0   0.00% tinygo build -size short -o ./build/test.hex -target=arduino-nano33 ./examples/lsm6ds3/main.go
   9932    9940      8   0.08%    4780    4780      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp23017/main.go
   9916    9924      8   0.08%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp3008/main.go
   6228    6236      8   0.13%    3288    3288      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/pcd8544/setbuffer/main.go
   5256    5264      8   0.15%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/pcd8544/setpixel/main.go
   6696    6704      8   0.12%    2288    2288      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1306/i2c_128x32/main.go
   6144    6152      8   0.13%    2280    2280      0   0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/ssd1306/spi_128x64/main.go
  17536   17544      8   0.05%    4740    4740      0   0.00% tinygo build -size short -o ./build/test.hex -target=circuitplay-express ./examples/thermistor/main.go
  11836   11848     12   0.10%    6580    6580      0   0.00% tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/apds9960/proximity/main.go
  57436   57448     12   0.02%    3692    3696      4   0.11% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/sht4x/main.go
  10700   10712     12   0.11%    4540    4540      0   0.00% tinygo build -size short -o ./build/test.hex -target=circuitplay-bluefruit ./examples/tone
  24696   24708     12   0.05%   13720   13720      0   0.00% tinygo build -size short -o ./build/test.hex -target=feather-nrf52840-sense ./examples/waveshare-epd/epd1in54/main.go
  62732   62744     12   0.02%    5952    5948     -4  -0.07% tinygo build -size short -o ./build/test.hex -target=feather-nrf52840 ./examples/is31fl3731/main.go
  12232   12248     16   0.13%    4812    4812      0   0.00% tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/bmp388/main.go
  14732   14748     16   0.11%    6580    6580      0   0.00% tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/hts221/main.go
  14096   14112     16   0.11%    6580    6580      0   0.00% tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/lps22hb/main.go
  57460   57480     20   0.03%    3684    3688      4   0.11% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/sht3x/main.go
  57424   57444     20   0.03%    3684    3688      4   0.11% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/shtc3/main.go
  10820   10840     20   0.18%    3340    3348      8   0.24% tinygo build -size short -o ./build/test.hex -target=pico ./examples/touch/capacitive
  13560   13584     24   0.18%    6788    6788      0   0.00% tinygo build -size short -o ./build/test.hex -target=pybadge ./examples/amg88xx
  10252   10276     24   0.23%    6916    6924      8   0.12% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/basic
  10276   10300     24   0.23%    6916    6916      0   0.00% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/scroll
  27104   27128     24   0.09%    3632    3640      8   0.22% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/microbitmatrix/main.go
  75448   75472     24   0.03%    7448    7448      0   0.00% tinygo build -size short -o ./build/test.hex -target=p1am-100 ./examples/p1am/main.go
  12236   12260     24   0.20%    3352    3360      8   0.24% tinygo build -size short -o ./build/test.hex -target=pico ./examples/pca9685/main.go
   8188    8212     24   0.29%    6788    6788      0   0.00% tinygo build -size short -o ./build/test.hex -target=pybadge ./examples/shifter/main.go
   9624    9648     24   0.25%    6780    6788      8   0.12% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/touch/resistive/fourwire/main.go
  10524   10552     28   0.27%    3328    3336      8   0.24% tinygo build -size short -o ./build/test.hex -target=feather-rp2040 ./examples/seesaw
  13744   13772     28   0.20%    3400    3408      8   0.24% tinygo build -size short -o ./build/test.hex -target=pico     ./examples/sgp30
  61440   61472     32   0.05%    6180    6188      8   0.13% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adt7410/main.go
   9888    9920     32   0.32%    4752    4760      8   0.17% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/itsybitsy-m0/main.go
  69688   69720     32   0.05%    6368    6376      8   0.13% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/ds3231/main.go
  26032   26068     36   0.14%   16412   16420      8   0.05% tinygo build -size short -o ./build/test.hex -target=pico ./examples/waveshare-epd/epd2in66b/main.go
  63948   63988     40   0.06%    6196    6196      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmi160/main.go
  64008   64048     40   0.06%    6228    6228      0   0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp280/main.go
   8324    8364     40   0.48%    3344    3352      8   0.24% tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/sram/main.go
  22260   22300     40   0.18%    3540    3556     16   0.45% tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/time/main.go
  68560   68600     40   0.06%    6188    6196      8   0.13% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/mcp2515/main.go
  16636   16680     44   0.26%    4172    4180      8   0.19% tinygo build -size short -o ./build/test.hex -target=feather-rp2040 ./examples/adafruit4650
  29588   29644     56   0.19%   38076   38084      8   0.02% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/pyportal_boing
  26936   26992     56   0.21%    5680    5620    -60  -1.06% tinygo build -size short -o ./build/test.hex -target=microbit-v2 ./examples/microbitmatrix/main.go
 263408  263492     84   0.03%   46748   46752      4   0.01% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/ili9341/slideshow
  69232   69980    748   1.08%    6968    6980     12   0.17% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/flash/console/spi
  65756   66724    968   1.47%    9004    9020     16   0.18% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/flash/console/qspi
2038270 2041166   2896   0.00%  472394  472522    128   0.00%

@deadprogram
Copy link
Member 

--- FAIL: TestBinarySize (0.00s)
    --- FAIL: TestBinarySize/microbit/examples/serial (40.40s)
        sizes_test.go:67: Unexpected code size when compiling: -target=microbit examples/serial
        sizes_test.go:68:             code rodata   data    bss
        sizes_test.go:69: expected:   2908    388      8   [22](https://github.com/tinygo-org/tinygo/actions/runs/13391197573/job/37399093533?pr=4742#step:15:23)72
        sizes_test.go:70: actual:     4352    404    176   2552
    --- FAIL: TestBinarySize/wioterminal/examples/pininterrupt (41.07s)
        sizes_test.go:67: Unexpected code size when compiling: -target=wioterminal examples/pininterrupt
        sizes_test.go:68:             code rodata   data    bss
        sizes_test.go:69: expected:   7293   1487    116   6912
        sizes_test.go:70: actual:     8691   1501    [28](https://github.com/tinygo-org/tinygo/actions/runs/13391197573/job/37399093533?pr=4742#step:15:29)8   7200
FAIL

Impact was not small on your first attempt, it would seem. 🐘

@aykevl
Copy link
Member Author

aykevl commented Feb 18, 2025

Oops, yeah, I was afraid of this.
I'll need to investigate this a bit deeper.

@aykevl
Copy link
Member Author

aykevl commented Feb 19, 2025

Trying a workaround that should hopefully fix the worst of this. Let's see what happens.

@aykevl
Copy link
Member Author

aykevl commented Feb 19, 2025 •
edited
Loading

So this is fixing a real bug, but also uncovering a bunch of cases where the old behavior was incorrect but resulted in smaller binary sizes. Most importantly (at least for small programs), the runtime.fastrand function was previously just returning 0 during interp since xorshift32State wasn't initialized but interp (incorrectly) continued interpreting it anyway. This bugfix fixes this issue - but also means that callers of fastrand (most notably the hashmap code) now can't be run during interp.

I've added a fix that should fix the worst of this, by initializing the xorshift state to a valid (non-random) value so that fastrand actually returns pseudo-random numbers that can be used by interp while still making fastrand use hardware random numbers at runtime.

@dgryski you wrote some of this, what do you think of this solution?

@aykevl
interp: correctly mark functions as modifying memory
b75e542 
Previously, if a function couldn't be interpreted in the interp pass, it
would just be run at runtime and the parameters would be marked as
potentially being modified. However, this is incorrect: the function
itself can also modify memory. So the function itself also needs to be
marked (recursively).

This fixes a number of regressions while upgrading to Go 1.24.

This results in a significant size regression that is mostly worked
around in the next commit.
@aykevl aykevl force-pushed the interp-fix-function-mark branch from 845385d to cdd3ad6 Compare February 19, 2025 14:53
@deadprogram
Copy link
Member

@aykevl I think we can afford the extra 2 bytes here https://github.com/tinygo-org/tinygo/actions/runs/13415479905/job/37475187142?pr=4742#step:19:16 😺

@aykevl
runtime: manually initialize xorshift state
f65532f 
This ensures:

 1. The xorshift state is initialized during interp.
 2. The xorshift state gets initialized to a real random number on
    hardware that supports it at runtime.

This fixes a big binary size regression from the previous commit. It's
still not perfect: most programs increase binary size by a few bytes.
But it's not nearly as bad as before.
@aykevl aykevl force-pushed the interp-fix-function-mark branch from cdd3ad6 to f65532f Compare February 20, 2025 16:17
@aykevl
Copy link
Member Author

aykevl commented Feb 20, 2025

Oops, yeah I missed that one.

@deadprogram
Copy link
Member

@aykevl looking at the sizediff, it would appear that this PR is a success.

@dgryski
Copy link
Member

dgryski commented Feb 20, 2025

I have the feeling this will fix a number of other interp-related issues on our bug tracker.

@aykevl aykevl marked this pull request as ready for review February 21, 2025 09:00
deadprogram
deadprogram approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@deadprogram deadprogram left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @aykevl

@deadprogram
Copy link
Member

I will merge after the current build finishes.

@deadprogram
Copy link
Member

Now merging, thanks @aykevl

@deadprogram deadprogram merged commit f4fd79f into dev Feb 21, 2025
24 checks passed
@deadprogram deadprogram deleted the interp-fix-function-mark branch February 21, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deadprogram deadprogram deadprogram approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aykevl @deadprogram @dgryski