Suspected integer underflow when switch returns a negative number

Question

Suspected integer underflow when switch returns a negative number

2TallTyler opened this issue 4 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.

Answer 1 · 2021-03-24T12:34:00.000Z

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

Answer 2 · 2021-03-24T12:53:33.000Z

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

Answer 3 · 2021-03-24T13:00:55.000Z

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

Answer 4 · 2021-03-24T17:05:55.000Z

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.

Answer 5 · 2021-03-24T17:31:23.000Z

Test grf: nml_underflow.zip

Answer 6 · 2021-03-24T17:45:30.000Z

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

Answer 7 · 2021-04-14T12:45:43.000Z

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

Answer 8 · 2021-08-16T07:15:51.000Z

15 bit callback results strikes again? :)