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

GDScript: Don't optimize division and modulo on debug #83569

Merged
merged 1 commit into from Oct 26, 2023

Conversation

vnen
Copy link
Member

@vnen vnen commented Oct 18, 2023

Since the validated operators don't have checks for division by zero, use the regular evaluator in debug which has those checks.

Fix #83308

Since the validated operators don't have checks for division by zero,
use the regular evaluator in debug which has those checks.
@vnen vnen added this to the 4.2 milestone Oct 18, 2023
@vnen vnen requested a review from a team as a code owner October 18, 2023 17:14
@bitsawer
Copy link
Member

bitsawer commented Oct 18, 2023

I wonder if we should keep the zero division check in release builds, too. It will probably affect performance somewhat, but a full crash on some platforms for a reasonably common scripting language division error is kind of harsh. Even on platforms where it doesn't crash it is still undefined behavior.

Another similar division issue we might have to guard against is division of large (near INT64_MAX) negative value by -1. Warning, this will currently freeze the editor if this is even pasted into a .gd file, at least on Windows:

print(-9223372036854775808 / -1)

While probably not a common situation, it might be used as denial-of-service attack for example against server code that is not prepared for inputs like these.

@vnen
Copy link
Member Author

vnen commented Oct 19, 2023

@bitsawer this can be discussed but since this PR in particular is a bugfix, I prefer to keep the current behavior.

@bitsawer
Copy link
Member

I agree the other division and modulo issue(s) can be handled in a separate PR if needed.

I'm not that familiar with the GDScript inner workings, so when you mentioned you prefer to "keep the current behavior", does that mean that division by integer zero will still crash the app when run on a release template build, but not in a debug build? Or does this PR fix both cases?

@vnen
Copy link
Member Author

vnen commented Oct 19, 2023

I'm not that familiar with the GDScript inner workings, so when you mentioned you prefer to "keep the current behavior", does that mean that division by integer zero will still crash the app when run on a release template build, but not in a debug build? Or does this PR fix both cases?

Division by zero crashes the release export. It has always been like this, even in 3.x. This PR makes it not crash only on debug.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

Let's evaluate the impact of release-mode checks for this in 4.3.

@akien-mga akien-mga merged commit adcd16c into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@vnen vnen deleted the gdscript-no-opt-division-modulo branch January 2, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Division by zero crashes editor and builds instead of logging error message
3 participants