OpenTTD / nml

NewGRF Meta Language
GNU General Public License v2.0
44 stars 36 forks source link

Suspected integer underflow when switch returns a negative number #196

Open 2TallTyler opened 3 years ago

2TallTyler commented 3 years ago

I am creating a house set which uses the construction_check callback to allow or deny construction based on a land value I calculate from various tile attributes. If I use a negative modifier, say a switch which checks if the tile is desert and returns return: -2;, the house is allowed to build even if I require a value of >=10000. As -2 is obviously not >= 10000, I believe this is an integer underflow caused by returning a negative value.

I would be happy to build a simple GRF to demonstrate this, if requested.

The expected behavior, assuming a switch to a signed integer isn't feasible, would be for NMLC to return an error if the user attempts to return a negative number.

LordAro commented 3 years ago

A minimal reproducer would be beneficial (for regression testing, if nothing else)

Eddi-z commented 3 years ago

i'm pretty sure the game handles all callback values as unsigned

Eddi-z commented 3 years ago

i checked, and the NFO specs (https://newgrf-specs.tt-wiki.net/wiki/VarAction2Advanced#operator) do provide both signed and unsigned comparison operators, so what would be necessary here would be a way in NML to specify which one is intended

glx22 commented 3 years ago

Yeah we need a test file, to at least check NFO output, and to test a fixed nmlc (if there's a bug).

BTW after checking nml source, it seems binary and ternary ops use signed comparison, so the issue is at another level.

2TallTyler commented 3 years ago

Test grf: nml_underflow.zip

glx22 commented 3 years ago

Ok I think I see the issue when looking at NFO

7E FE 20 \dxFFFFFFFF    // TileIsDesert
\2cmp 1A 20 \dx00000064

only 15bits of the return value are checked, and I think sign bit is dropped too. You can try switch (FEAT_HOUSES, SELF, ConstructionCheck, [TileIsDesert(), last_computed_result > 100] ) {return;} but maybe nml should handle that.

I think you can also confirm it's the issue by checking for 32766 (0x7FFE) without using last_computed_result

2TallTyler commented 3 years ago

Sorry for the late reply and thanks for the ping on the livestream. I forgot about this one.

I can confirm that switch (FEAT_HOUSES, SELF, ConstructionCheck, [TileIsDesert(), last_computed_result > 100] ) {return;} does not underflow. Here is the modified GRF and source.

underflow_fixed.zip

andythenorth commented 3 years ago

15 bit callback results strikes again? :)