llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.04k stars 11.58k forks source link

regression for SCCP pass on "Illegal instruction (core dumped) #89145

Open diggerlin opened 5 months ago

diggerlin commented 5 months ago

the following test has "Illegal instruction (core dumped) " which is a regression by https://reviews.llvm.org/D126962

in the following test case. bash> cat a.c

char* foo(char*);
int main()
{
        char k[1];
        if (k[0] == 'a')
          foo(k);
}

bash> clang-19 -O2 --target=powerpc64le-unknown-linux-gnu -o a.ll -emit-llvm -S a.c bash> cat a.ll

; Function Attrs: mustprogress nofree norecurse noreturn nosync nounwind willreturn memory(none) uwtable
define dso_local noundef signext i32 @main() local_unnamed_addr #0 {
entry:
  unreachable
}

bash> clang-19 -O2 -o a.o --target=powerpc64le-unknown-linux-gnu -c a.c bash> objdump -D a.o

a.o: file format elf64-powerpcle

Disassembly of section .text:

0000000000000000 <main>:
        ...

Disassembly of section .comment:

it get an empty .text section.

if I have b.c

bash> cat b.c

void foo(char* s) {
  s[0]='a';
}

$ clang-19 -O2 -o b.o --target=powerpc64le-unknown-linux-gnu -c b.c $ clang-19 -O2 -o test a.o b.o $ ./test Illegal instruction (core dumped)

it has Illegal instruction.

bash> objdump -D test there is following code in the text section.

  0000000000010890 <main>:
        ...                         (all zero from 10890 ~1089c)
   1089c:       08 00 e0 7f     trap

00000000000108a0 <foo>:
   108a0:       61 00 80 38     li      r4,97
   108a4:       00 00 83 98     stb     r4,0(r3)
   108a8:       20 00 80 4e     blr
        ...
   108b8:       08 00 e0 7f     trap
   108bc:       08 00 e0 7f     trap
diggerlin commented 5 months ago

according to 6.2.6.1#5 in C18

The C standard says that char types do not have trap representations.

it should not have "Illegal instruction" here.

hubert-reinterpretcast commented 5 months ago

I am not sure that the backend change is wrong. This seems to be an issue with the front-end IR codegen. Presumably, in C, loads of character types need to have the freeze IR instruction applied.

@AaronBallman, thoughts?

aeubanks commented 5 months ago

the original code reads an uninitialized stack variable?

diggerlin commented 5 months ago

the original code reads an uninitialized stack variable?

yes.

nikic commented 5 months ago

Clang currently considers branching on uninitialized values to be undefined behavior. This doesn't quite match what the C standard says in some cases, but for now this is won't fix.

(There are some major changes to uninitialized value handling on the horizon for C++, as well as changes on the horizon for LLVM's undef handling, and that will likely have impact on how clang interprets uninitialized values.)

hubert-reinterpretcast commented 5 months ago

@AaronBallman, is it agreeable with you to treat this case as erroneous behavior (in line with C++)? Should the C committee be consulted about this?

AaronBallman commented 5 months ago

I think this is undefined behavior per spec despite the array being of character type. Array subscripting is defined as doing (*((E1)+(E2))). The value produced by * would have type char which then gets converted to int (via integer promotions or via picking a composite type for ==), that int has a non-value representation and reading that value is undefined.

hubert-reinterpretcast commented 5 months ago

that int has a non-value representation and reading that value is undefined

@AaronBallman, in the absence of padding bits in the object representation of a type (as is the case for unsigned char, signed char, and plain char), there are no non-value representations of that type. I don't see how a valid char value becomes an int with a non-value representation. Indeed, it is a bit of a category error because non-value representations only exist as object representations (i.e., "in memory" and not in the rvalue space).

AaronBallman commented 5 months ago

that int has a non-value representation and reading that value is undefined

@AaronBallman, in the absence of padding bits in the object representation of a type (as is the case for unsigned char, signed char, and plain char), there are no non-value representations of that type. I don't see how a valid char value becomes an int with a non-value representation. Indeed, it is a bit of a category error because non-value representations only exist as object representations (i.e., "in memory" and not in the rvalue space).

Hmmm, I think in practice you are correct, but I'm not seeing wording that says a conversion to a wider type means it won't then get a non-value representation. But you're right about the category error, so I'm probably off-base with that explanation because it heads into malicious reading territory.

Taking another run at it, I see 6.3.2.1p2 says: If the lvalue designates an object of automatic storage duration that could have been declared with the register storage class (never had its address taken), and that object is uninitialized (not declared with an initializer and no assignment to it has been performed prior to use), the behavior is undefined.

And in this case, the object has automatic storage duration but it cannot be declared with the register storage class without inducing UB (p3 says "If the array object has register storage class, the behavior is undefined." so the array to pointer decay would have caused UB, which I take to mean that precludes its use for p2).

So it's possible this actually isn't UB despite being rather erroneous.

I can ask on the WG14 reflectors if you'd like, but in terms of moving forward with this issue, I agree with @nikic that the changes coming for undef in LLVM mean we probably should not touch this right now.

hubert-reinterpretcast commented 5 months ago

I can ask on the WG14 reflectors if you'd like

That would be great; thanks!

AaronBallman commented 4 months ago

I asked on the reflectors and the answer is... the behavior is not undefined but it is also not particularly clear.

Notionally, array-to-pointer decay is the same as taking the address, but the standard doesn't actually say that explicitly. So the array object does have its address taken, which means the object could not have been declared with the register keyword, which means the behavior is not undefined.