FIX? Internal Compiler Error on SH-4 #4445
Conversation
reneme
force-pushed
the
fix/ice_on_sh4
branch
from
68c5730 to
12501bf
Compare
|
Thanks for looking into this. The valgrind finding is interesting... |
This fixes the Internal Compiler Error described in GH randombit#4444 and makes the code more readable to humans as a side-effect. Note that bitvector::equals() is implemented as (lhs ^ rhs).none() and is meant to be constant time as well.
reneme
force-pushed
the
fix/ice_on_sh4
branch
from
12501bf to
9e069c8
Compare
|
This isn't a complete fix as of now. There are more ICEs within the test-code of the bitvector class that I didn't find workarounds for so far. @randombit In your experience, could this indicate some undefined behavior looming within bitvector or would you rather guess that the code generation for the 30-year old processor might be buggy? After all the bitvector is pretty dense template code, so I wouldn't rule out either.
I forgot to unpoison the comparison result (previously this was done implicitly in the invocation of |
This also works around an Internal Compiler Error on SH-4.
reneme
force-pushed
the
fix/ice_on_sh4
branch
from
9e069c8 to
e9bbe85
Compare
The ASM-based approach caused ICE in GCC 13.2, see GH randombit#4444. Co-Authored-By: Fabian Albert <[email protected]>
reneme
force-pushed
the
fix/ice_on_sh4
branch
from
e9bbe85 to
d8749ec
Compare
|
Turns out that the value_barrier tripped up the compiler. If I understand the ICE correctly, something goes wrong in the code generation when allocating registers somehow. We added a quick-and-dirty fallback to a volatile-read for targets without inline assembly and explicitly disabled SuperH as well. This fixes the compilation, but this certainly needs discussion. |
|
IME a GCC ICE is just that, a compiler bug. I’ve seen what GCC calls ICE-on-invalid (ie, it crashes when fed code that is not conforming/normally compilable) but I’ve never seen ICE where it had any relation to undefined behavior in the code that triggered it. Especially on the niche arch (these days that means anything except x86, POWER, ARM, and maybe RISCV) a lot of these compiler targets only have 1-2 maintainers so it’s not so surprising. It would be nice if we could reduce this to something reasonable (~couple hundred lines) at which point it’s reportable. Usually given a testcase GCC devs will fix ICEs. |
| return x; | ||
| #else | ||
| return volatile_read(x); |
There was a problem hiding this comment.
This introduces it for MSVC (and others) as well which maybe is the “right” thing to do but I could see this causing a massive performance regression and AFAIK MSVC is not known to ever perform Clang-style optimizations that undo constant time code.
Likewise it introduces the volatile reads for the sanitizer build which doesn’t seem necessary; in general MemSan isn’t safe for production builds anyway.
I think we need A Couple More Macros (tm) here
BOTAN_COMPILER_USE_CT_BARRIER- possibly set from the compiler info file? For GCC, Clang, XCode, maybe others.BOTAN_COMPILER_CT_BARRIER_USE_ASM_BLOCKBOTAN_COMPILER_CT_BARRIER_USE_VOLATILE
Maybe even toggleable at configure time? (none, asm, volatile) OK at this point I’m convinced this is worthwhile but I can take it over the holiday weekend …
There was a problem hiding this comment.
For the record: #4447
|
ok to close? |
Closes #4444