Miscompilation of _BitInt passed as variadic argument

[ostannard]

This code is miscompiled at all optimisation levels except for -O0, and for all (or many) targets (I've tried Arm v7M, AArch64 and x86_64):

#include <stdarg.h>
#include <assert.h>

void F8 (int P0, ...) {
  va_list vl;
  va_start(vl, P0);
  signed _BitInt(13) P1;
  P1 = va_arg(vl, signed _BitInt(13));
  assert((int)P1 == -42);
}

int main (void) {
  F8(0, -42);
  return 0;
}

Generated code for v7M:

$ /work/llvm/build/bin/clang --target=arm-none-eabi -march=armv7-m -c test.c -o - -S -O1
...
F8:
        .fnstart
        .pad    #12
        sub     sp, #12
        .save   {r7, lr}
        push    {r7, lr}
        .setfp  r7, sp
        mov     r7, sp
        .pad    #4
        sub     sp, #4
        add.w   r0, r7, #8
        stm     r0!, {r1, r2, r3}
        add.w   r0, r7, #8
        adds    r0, #4
        str     r0, [sp]
        ldrh    r0, [r7, #8]
        movw    r1, #8150
        cmp     r0, r1
        beq     .LBB0_2
        movw    r0, :lower16:.L.str
        movw    r1, :lower16:.L.str.1
        movt    r0, :upper16:.L.str
        movt    r1, :upper16:.L.str.1
        movs    r2, #10
        bl      __aeabi_assert
.LBB0_2:
        add     sp, #4
        pop.w   {r7, lr}
        add     sp, #12
        bx      lr
...
main:
        .fnstart
        .save   {r7, lr}
        push    {r7, lr}
        .setfp  r7, sp
        mov     r7, sp
        mvn     r1, #41
        bl      F8
        movs    r0, #0
        pop     {r7, pc}
...

The draft ABI spec for _BitInt (https://github.com/ARM-software/abi-aa/pull/191/files) says that the non-significant bits are unspecified, but the generated code for F8 compares the 16-bit chunk (loaded with the LDRH instruction) containing the argument and 3 unspecified bits to a constant which has those bits set to zero, which fails because main sign-extended the value into those bits, setting them to 1.

For a non-variadic version of the function, we emit a BFC instruction to clear the unspecified bits before comparing them, which is correct:

#include <stdarg.h>
#include <assert.h>

__attribute((noinline))
void F8 (int P0, signed _BitInt(13) P1) {
  assert((int)P1 == -42);
}

int main (void) {
  F8(0, -42);
  return 0;
}

$ /work/llvm/build/bin/clang --target=arm-none-eabi -march=armv7-m -c test.c -o - -S -O1
...
F8:
        .fnstart
        bfc     r1, #13, #19
        movw    r0, #8150
        cmp     r1, r0
        it      eq
        bxeq    lr
.LBB0_1:
        movw    r0, :lower16:.L.str
        movw    r1, :lower16:.L.str.1
        movt    r0, :upper16:.L.str
        movt    r1, :upper16:.L.str.1
        movs    r2, #7
        b       __aeabi_assert
...

[llvmbot]

@llvm/issue-subscribers-c2x

[jcranmer-intel]

Looking at the IR transformation here (relying on x86-64 ABI), the key difference is that in -O0, the code looks like this:

%3 = load i13, ptr %vaarg.addr, align 2
store i13 %3, ptr %P1, align 2
%4 = load i13, ptr %P1, align 2
%conv = sext i13 %4 to i32
%cmp = icmp eq i32 %conv, -42

(%vaarg.addr is a pointer into the va_list struct)

After a bout of -O1 opts, this becomes:

%4 = load i13, ptr %vaarg.addr, align 2
%cmp = icmp eq i13 %4, -42

If I reintroduce the sext i13 %4 to i32 instruction, and use icmp eq i32 instead of icmp eq i13, the testcase passes.

Per the LangRef, load i13 is UB if it's loading from memory that's not generated by a store i13. This is coming from something filled in by @llvm.va_start so the definition of what's storing it is somewhat unclear, but given that the function call itself to this varargs function uses i32, I'm inclined to believe that the issue here is Clang's lowering of va_arg for _BitInt types is incorrect.

[royjacobson]

This sounds like the same problem as #62032, but in the ARM ABI.

[AaronBallman]

The initial code example:

#include <stdarg.h>
#include <assert.h>

void F8 (int P0, ...) {
  va_list vl;
  va_start(vl, P0);
  signed _BitInt(13) P1;
  P1 = va_arg(vl, signed _BitInt(13));
  assert((int)P1 == -42);
}

int main (void) {
  F8(0, -42);
  return 0;
}

has UB.

7.16.1p2, in part:

If type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases:

_BitInt does not undergo default argument promotions per 6.5.2.2p6 because _BitInt does not get integer promoted (per 6.3.1.1p2). So it boils down to type compatibility, and int and _BitInt are not compatible types per 6.7.2p7.
(There is a bit of a bug in that wording btw, as it implies all _BitInts (with the same sign) are compatible regardless of bit-width, I’ll see about filing an NB comment or an issue to address that.)

Does a slight tweak to the example to remove that UB demonstrates the same behavior?

#include <stdarg.h>
#include <assert.h>

void F8 (int P0, ...) {
  va_list vl;
  va_start(vl, P0);
  signed _BitInt(13) P1;
  P1 = va_arg(vl, signed _BitInt(13));
  assert((int)P1 == -42);
}

int main (void) {
  F8(0, (signed _BitInt(13)-42);
  return 0;
}

[Fznamznon]

Does a slight tweak to the example to remove that UB demonstrates the same behavior?

I think there is still a bug - https://godbolt.org/z/bWKEnr9Ex .

[AaronBallman]

Thanks! For x64 there is, but I was curious if ARM also had the same failure. I'm still trying to wrap my mind around whether this is a clang IR generation issue or an LLVM backend issue.

[Fznamznon]

I have a small suspicion that it might be LLVM/CodeGen issue in missing support for va_start/va_arg with _BitInt argument. Before _BitInt, passing anything different than int or double to va_arg was UB. Now clang doesn't emit errors/warnings for _BitInt argument, but was it ever supported on lower levels?
Also what concerns me is that -O0 and non-variadic version of the function works fine.

[AaronBallman]

I have a small suspicion that it might be LLVM/CodeGen issue in missing support for va_start/va_arg with _BitInt argument. Before _BitInt, passing anything different than int or double to va_arg was UB.

That's correct for arithmetic types, but not in general (you can pass structure types or whatever, if you want to).

Now clang doesn't emit errors/warnings for _BitInt argument, but was it ever supported on lower levels? Also what concerns me is that -O0 and non-variadic version of the function works fine.

Yeah, the behavior at -O0 is why I also think this is an LLVM issue. From @jcranmer-intel's comment:

Per the LangRef, load i13 is UB if it's loading from memory that's not generated by a store i13. This is coming from something filled in by @llvm.va_start so the definition of what's storing it is somewhat unclear, but given that the function call itself to this varargs function uses i32, I'm inclined to believe that the issue here is Clang's lowering of va_arg for _BitInt types is incorrect.

We see that passing an i13 through llvm.va_start also has the same behavior as passing an i32 (which is UB), so I think it may not be Clang's lowering. WDYT?

[Fznamznon]

We see that passing an i13 through llvm.va_start also has the same behavior as passing an i32 (which is UB), so I think it may not be Clang's lowering. WDYT?

If i32 in function arguments of the original example is the only problem, I agree. Although I can't really identify other problems that might be clang's fault.

[Fznamznon]

Note that this seems to be working now https://godbolt.org/z/hW9KMEr5b .