Fix potential integer underflow in rounded up divisions

[EddieBreeg]

A new division_round_up function was added in the Math class, allowing for easy and correct computation of integer divisions when the result needs to be rounded up.
https://github.com/EddieBreeg/godot/blob/8349f16f316b90f705da1219f52828a0af889907/core/math/math_funcs.h#L190-L201

• Bugsquad edit, fixes: Integer underflows throughout the code when trying to round up #80358

[AThousandShips]

To link it use just "fixes", also don't write "bugsquad", that means it was added by one of the members of that team

[AThousandShips]

Please squash your commits into one, see here

[EddieBreeg]

Done

[miv391]

How about adding comments about the parameter limits/requirements? For example the first function requires that the sum of num and den must be between min_int and max_int (otherwise there in an overflow).

[EddieBreeg]

Would there be any benefit to marking the function as constexpr?

[AThousandShips]

No I'd say, none of the contexts where it is used are constexpr compatible, all uses are runtime, and since it's always inlined it wouldn't change anything anyway

[EddieBreeg]

Well right now they're not used in a constexpr context but do we know that's not going to change? And adding it wouldn't change a thing if it has to be evaluated at runtime anyway

[EddieBreeg]

Ok what the hell, I didn't close this

[AThousandShips]

But if you're doing things at compile time then you should probably not be depending on this right?

constexpr should only be added if you know it'll help:

Any feature not listed below is allowed. Using features like constexpr variables and nullptr is encouraged when possible. Still, try to keep your use of modern C++ features conservative. Their use needs to serve a real purpose, such as improving code readability or performance.

[AThousandShips]

You closed this, you probably clicked the wrong button when commenting

[EddieBreeg]

You closed this, you probably clicked the wrong button when commenting

Well that's great, thanks github...

But if you're doing things at compile time then you should probably not be depending on this right?

In theory no, but then why does anyone use constexpr right? Arguably, for code readability so that it's still obvious what you're doing. I agree it's unlikely it will actually get used, I just think it doesn't hurt

[miv391]

As this PR is being made because previous implementation bugged when num was 0, I would suggest making unit tests to prove that now this works.

[AThousandShips]

Except it's inlined so it will have no effect what so ever, and it doesn't help readability either, and no other function in this header has constexpr, for the same reason

[EddieBreeg]

As this PR is being made because previous implementation bugged when num was 0, I would suggest making unit tests to prove that now this works.

Will do

[lawnjelly]

Regarding overflow, we often just don't handle it (for instance even the simplest expression x + y can overflow), and leave it to client code to check if this seems a possibility. That would probably be fine.

But if you wanted some cheap checks, if you look at error_macros.h in 3.x, we have DEV_CHECK_ONCE(), which I never added to master but can be used for some very basic error detection in DEV builds.

#ifdef DEV_ENABLED
#define DEV_CHECK_ONCE(m_cond)                                                   \
	if (unlikely(!(m_cond))) {                                                   \
		ERR_PRINT_ONCE("DEV_CHECK_ONCE failed  \"" _STR(m_cond) "\" is false."); \
	} else                                                                       \
		((void)0)
#else
#define DEV_CHECK_ONCE(m_cond)
#endif

So you could have e.g. DEV_CHECK_ONCE(((int64_t) pnum + (int64_t) pden) < INT32_MAX); type checks for overflow.

This will print a one time error (so as not to spam) if overflow is detected, but is compiled out in release builds so is free.

[EddieBreeg]

So you'd suggest not adding comments and adding the cheap tests instead?

[AThousandShips]

To ensure the correct behaviour of these I'd say test cases should be added to tests/core/math/test_math_funcs.h

[EddieBreeg]

As I said, I will. In fact I have, the changes just aren't pushed yet.

[AThousandShips]

Wanted to make sure as there was talk of different tests going on

[AThousandShips]

Potential case for this:

• Executing MultiMesh.set_buffer function crashes Godot #84672

[akien-mga]

Looks good to me from a cursory look. Sorry for the delay reviewing.

[YuriSizov]

@EddieBreeg Will you be available to address the feedback? This PR also needs a rebase.

[akien-mga]

I rebased the PR myself, solving the conflicts, adding the change I suggested, and handling a few other occurrences which I found with this regex:

rg "\- 1\) /.*\+ 1" -g'!thirdparty'

There might be more when they're multiline of with either operand being a local variable, but this should be a good start.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[reduz]

This PR was merged without even being tested. I am not happy about it.

[reduz]

This should not have been merged, the shader does not even compile.
Was this PR even tested?

[akien-mga]

My mistake, I added this yesterday and didn't notice it was touching a .glsl file.

[akien-mga]

Fixed by 7abaac6.

[akien-mga]

This PR was merged without even being tested. I am not happy about it.

I tested, but didn't notice the error. My test project was likely not using SDFGI.
It does show a shader compilation error when running Godot from terminal, but none in the editor itself, so I missed it when doing a quick check after finalizing the PR.

Anyway, this was my bad, I added this change yesterday after rebasing and noticing a few new cases of (x + 1) / y - 1 that should be refactored. I've undone the .glsl change in 7abaac6.