remove ASM div64_32() implementation #2220
Conversation
src/m_fixed.h
Outdated
| @@ -38,8 +38,8 @@ | |||
| { | |||
| return a / b; | |||
| } | |||
| int32_t lo = a; | |||
| int32_t hi = a >> 32; | |||
| volatile int32_t lo = a; | |||
There was a problem hiding this comment.
How about int32_t lo = (int32_t) a;?
It seems to be another case of UB that will differ in various versions of compilers.
There was a problem hiding this comment.
From Linux kernel:
union {
u64 v64;
u32 v32[2];
} d = { dividend };
u32 upper;
upper = d.v32[1];There was a problem hiding this comment.
Tried this, same crash.
--- a/src/m_fixed.h
+++ b/src/m_fixed.h
@@ -38,8 +38,14 @@
{
return a / b;
}
- int32_t lo = a;
- int32_t hi = a >> 32;
+ union {
+ int64_t v64;
+ int32_t v32[2];
+ } d = {a};
+
+ int32_t lo = d.v32[0];
+ int32_t hi = d.v32[1];
+
asm("idivl %[divisor]" : "+a" (lo), "+d" (hi) : [divisor] "r" (b));
return lo;
}There was a problem hiding this comment.
How about
int32_t lo = (int32_t) a;?
Same crash.
There was a problem hiding this comment.
I think the problem is that a and b are both input and output variables to the asm() call. However, the issue is very compiler specific. Ubuntu-latest uses gcc-11.2 and I cannot reproduce it with gcc-14.2 on my main system with the exact same build flags. Also, it is optimization specific and won't occur with gcc-11.2 in a debug build with -O0.
There was a problem hiding this comment.
Honestly, I don't understand why volatile works, some kind of miscompilation? I think we should just remove the GCC version of div64_32.
There was a problem hiding this comment.
Reminds me of this bug, where we also never really understood what was going on:
There was a problem hiding this comment.
This was a miscompilation with Clang -O3. May be fixed in newer versions of the compiler.
There was a problem hiding this comment.
Yes, it's possible the same thing happens here with gcc-11.
There was a problem hiding this comment.
Anyway, I suggest to remove the GCC version of div64_32. It helps only for old CPUs and is only noticeable with voxels (we have very few FixedDiv in other code).
|
The original reporter tried this and said that it fixed the crash. |
* Prone to miscompilation * Only helps with very old PCs * Really only helps with voxel rendering
fabiangreffrath
changed the title
Fixes #2219