nml icon indicating copy to clipboard operation
nml copied to clipboard

Suspected integer underflow when switch returns a negative number

Open 2TallTyler opened this issue 3 years ago • 8 comments

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.

2TallTyler avatar Mar 24 '21 12:03 2TallTyler

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

LordAro avatar Mar 24 '21 12:03 LordAro

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

Eddi-z avatar Mar 24 '21 12:03 Eddi-z

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

Eddi-z avatar Mar 24 '21 13:03 Eddi-z

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.

glx22 avatar Mar 24 '21 17:03 glx22

Test grf: nml_underflow.zip

2TallTyler avatar Mar 24 '21 17:03 2TallTyler

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

glx22 avatar Mar 24 '21 17:03 glx22

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

2TallTyler avatar Apr 14 '21 12:04 2TallTyler

15 bit callback results strikes again? :)

andythenorth avatar Aug 16 '21 07:08 andythenorth