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.

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

Suspected integer underflow when switch returns a negative number #196

Open
2TallTyler opened this issue Mar 24, 2021 · 8 comments
Open

Suspected integer underflow when switch returns a negative number #196

2TallTyler opened this issue Mar 24, 2021 · 8 comments

Comments

@2TallTyler
Copy link
Member

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
Copy link
Member

LordAro commented Mar 24, 2021 •
edited

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

@Eddi-z

This comment has been minimized.

Sign in to view
@Eddi-z

This comment has been minimized.

Sign in to view
@glx22
Copy link
Contributor

glx22 commented Mar 24, 2021 •
edited

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
Copy link
Member Author

Test grf: nml_underflow.zip

@glx22
Copy link
Contributor

glx22 commented Mar 24, 2021 •
edited

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
Copy link
Member Author

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
Copy link
Contributor

15 bit callback results strikes again? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@andythenorth @glx22 @LordAro @Eddi-z @2TallTyler