clang-repl value unit test fails on 32 bit Arm #94994
Comments
|
Compiling debug builds natively is a nightmare these days due to RAM constraints, so I cross compiled it using the instructions I wrote in #94741 (comment). When run under qemu we get different numbers: Then I debugged it using qemu's GDB stub and gdb-multiarch: What's interesting is that the value seen in the failure: Turns up in the varargs as the first argument: Which suggests that the code in If I go up the callstack and track the value of what would be the first vararg, Which is branching to the address of According to the ABI and compiler generated code, the value of the int would be in I checked this on AArch64 and saw that We get the same set of function calls: And in And we see that it gets set before the call: Do we see anything like that in Arm's version? I need to look more to confirm but though there are loads they look to be close to the PC, and they don't write to |
The original bug is legitimate UB, but this is in the clang constant expression interpreter, not clang-repl. #94994 covers investigation of this specific bug in clang-repl.
|
Adding @lhames and @weliveindetail for more wisdom about the JIT infrastructure. |
EugeneZelenko
added
clang-tools-extra
test-suite
and removed
clang
|
FWIW, I am able to use printf fine on Arm, and that's variadic: I also thought maybe there is some circumstance where |
|
Perhaps it is the whole typecasting business we do but you say that it is already wrong there. In that case perhaps it is the way we take the ownership of the execution result around here ? |
|
Another place to look is here where we fetch from an union-like struct and maybe the casts went wrong but I do not see how… |
|
I cannot look into it more closely right now, but is there a difference between Arm and Thumb code? If so, it might be a relocation issue. |
|
I tried adding No combination of those fixed the issue. Assuming that's how I was supposed to try it. I did see |
Yes, that's what I wanted to confirm. If it happens in both ISA modes, Arm and Thumb, then it's likely not an issue with the AArch32 backend in JITLink. They use different relocation types, indirection stubs, etc. and probably they aren't both broken the same way. |
|
If this issue is still open in 2 weeks, I can take a look :) |
The original bug is legitimate UB, but this is in the clang constant expression interpreter, not clang-repl. llvm#94994 covers investigation of this specific bug in clang-repl.
|
#95911 may have the same cause as this. |
|
The issue reproduces with both, GCC and Clang. So it's likely not a toolchain issue. I didn't find anything suspicious in the jitlink debug dumps and the repro hits for different sub arches (armv6kz and armv7) and both modes, arm and thumb. So it's likely not a relocation issue or anything else in jitlink. I checked the patch again that apparently introduced the bug. It keeps passing the value of the I didn't yet investigate further, but this looks really suspicious: |
|
Ok, short summary from what I found:
I wonder if that actually is the expected behavior and x86_64 only works by accident, because we happen to be lucky with va_args order? @DavidSpickett Maybe you know ways to align va_args behavior on ARM or in general? Here is a draft that fixes the issue by consuming the unused half where necessary: #96900 I could formalize that approach and we submit that, if there are no better solutions? What do you think? |
Assuming that this is called as: Is it not undefined behaviour to then in Would it make more sense to have: Then just access the correct member of the union, which should work as normal. Or does this conflict with the final intent of #89811 ? Is the use of var args crucial to this effort? I assume it is doing something, because we only have one var arg right now but presumably the reason to use them is you might have many in future. The other option would be to cast all instances of Storage to the type stored within it, before passing it to __clang_Interpreter_SetValueNoAlloc, but that seems like repeating work that __clang_Interpreter_SetValueNoAlloc itself does. Or you can have var args, but each var arg is always of type Storage and you access the union that way. Depends what's important here, that the var arg has the concrete type, or that you can have any number of them, or both. If it's just the ability to have any number of them, maybe the fix here is very simple. @vgvassilev can you comment on that? And if that's a confusing ramble to you, just lay out the goals of using var args from your point of view. |
|
FYI: I got to the bottom of the issue and post a new PR in a bit 🤞 |
|
I checked IR dumps from clang-repl today and found that integral types are always extended to 64-bit no matter which system we run on. The runtime interface bindings are generated on to fly in |
|
I took a few days off and was worried about that issue, I come back and it's solved. @weliveindetail and @DavidSpickett, you both rock! |
The intent of this interface is to provide some conversion between interpreted and compiled values. The trick with the va_args is that I can push practically a |
Maybe it's worth noting that (compared to the |
|
Bots are all green. Thanks @weliveindetail, excellent work! |
When generating runtime interface bindings, extend integral types to the native register size rather than 64-bit per se Fixes llvm#94994
When generating runtime interface bindings, extend integral types to the native register size rather than 64-bit per se Fixes llvm#94994
Since #89811 landed one of the unit tests has been failing on Arm 32 bit:
It does not fail on AArch64. It does fail under qemu emulation too, but with different results. Which points to some UB in the program.
The text was updated successfully, but these errors were encountered: