Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Illegal instruction in code generated from C with default optimisation. #17065

Closed
rdebath opened this issue Sep 3, 2023 · 10 comments
Closed

Comments

@rdebath
Copy link

rdebath commented Sep 3, 2023

Zig Version

0.11.0

Steps to Reproduce and Observed Behavior

Given this C code.

#include <stdlib.h>
#include <stdio.h>
#include <limits.h>

int
main()
{
    int x, flg = sizeof(int) * CHAR_BIT +2;
    for(x=1;x+1>x && --flg;x=(x|(x<<1)));
    x = sizeof(int) * CHAR_BIT +3 - flg;

    if (x >= (int)(sizeof(int) * CHAR_BIT))
        printf("WARNING: Signed integers have too many bits.\n");
    else if (x != sizeof(int) * CHAR_BIT -1)
        printf("WARNING: Signed integers have %d magnitude bits.\n", x);
    else
        printf("Two's complement integers\n");
}

Run like this:

$ zig cc -Wall -Wextra prog.c
$ ./a.out
Illegal instruction
$ 

If you turn on optimisation it works, if you turn on -fwrapv there is no SIGILL.

Expected Behavior

I would expect somewhat consistent behaviour.
It's likely you'll do this, as the optimiser is configured to assume integers are unbounded when it's turned on.
(This is what happens with -O2)

$ zig cc -Wall -Wextra prog.c
$ ./a.out
WARNING: Signed integers have too many bits.
$ 

It should probably do this if you're actually following POSIX. (Or if the optimiser is actually turned off)

$ zig cc -Wall -Wextra prog.c
$ ./a.out
Two's complement integers
$ 
@rdebath rdebath added the bug Observed behavior contradicts documented or intended behavior label Sep 3, 2023
@squeek502
Copy link
Collaborator

squeek502 commented Sep 3, 2023

You're hitting the undefined behavior sanitizer that Zig enables by default. You can disable it by passing -fno-sanitize=undefined (or, better yet, you can fix the UB).

Note: the relevant issue for more feedback when UB is hit is #5163

@ifreund ifreund closed this as completed Sep 3, 2023
@ifreund ifreund removed the bug Observed behavior contradicts documented or intended behavior label Sep 3, 2023
@ifreund ifreund closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2023
@rdebath
Copy link
Author

rdebath commented Sep 4, 2023

Okay, then please stop wasting my time making ubisan look like a compiler bug. Lets change the expected behaviour to something like current GCC/Clang (the below) and not something that throws a tantrum when you don't optimise.

$ clang x.c -fsanitize=undefined -Wextra -Wall && ./a.out
x.c:9:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior x.c:9:14 in
Two's complement integers
$ gcc  x.c -fsanitize=undefined -Wextra -Wall && ./a.out
x.c:9:35: runtime error: left shift of 2147483647 by 1 places cannot be represented in type 'int'
WARNING: Signed integers have too many bits.
$ clang x.c -fsanitize=undefined -Wextra -Wall -O3 && ./a.out
x.c:9:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior x.c:9:14 in
Two's complement integers
$ gcc  x.c -fsanitize=undefined -Wextra -Wall -O3 && ./a.out
x.c:9:35: runtime error: left shift of 2147483647 by 1 places cannot be represented in type 'int'
WARNING: Signed integers have too many bits.
$

@squeek502
Copy link
Collaborator

That is the plan, see #5163

@andrewrk
Copy link
Member

andrewrk commented Sep 4, 2023

@rdebath maybe you should use a different programming language if you can't write correct C code.

@ikskuh
Copy link
Contributor

ikskuh commented Sep 4, 2023

Okay, then please stop wasting my time making ubisan look like a compiler bug. Lets change the expected behaviour to something like current GCC/Clang (the below) and not something that throws a tantrum when you don't optimise.

It's 100% legal to do that according to the C standard, so what Zig does is fine and definitly better than what gcc/clang do by default. Zig is not breaking code here, code was already broken, the defect is just made visible.
GCC had times where it would literally just invoke games installed on the system when UB was invoked, so be happy that this isn't the case here

@rdebath
Copy link
Author

rdebath commented Sep 4, 2023

I assume that was the April first release, because they're not usually so stupid as to destroy their "quality of implementation" reputation.

The simple fact is that for integers the intent of the standard was to allow different physical representations of integers along with their defined by the CPU method of implementation. This was reduced to "sign and magnitude", "ones complement" and "twos complement" in later versions of the standard. AIUI it'll be reduced to just "twos complement" in the next version as nobody uses anything else. Now, this might seem like there's no need for something like ubsan (on integers) after this, but that's not true because some of us like -ftrapv. So even with just twos complement the standard wants to allow -ftrapv or -fwrapv. This leaves the compiler writers a (smaller and smaller) gap that they can claim their, basically, random deletion of code that looks like it might trip -ftrapv is valid. At no point was this laxity in the standard designed to allow lazy "optimisation", but users want bad (and complicated) code to be optimised as hard as possible, so that's what they get.

Personally, I'd much prefer -ftrapv on GCC if it actually worked; if you put the above code into GCC with optimisation turned on it fails to hit the SIGABORT. Clang is better, it seems to work most of the time, but it's a good thing it doesn't turn it on by default because it uses a illegal instruction trap (like zig-cc) rather than an abort (SIGABRT) or an "overflow" (SIGFPE) trap and so gives a really stupid error at runtime.

Luckily, both sets of compiler writers seem to understand that sometimes you really do want the defined-by-the-cpu operation of integers (even if it disables some optimisations) so -fwrapv is pretty much bug free. (By which I mean I've never found one)

So, in summary, for signed integers, the UB is supposed to be as defined by the CPU so the compiler writer can use the easiest/best implementation of signed integers on a particular CPU. The essential claim is that it's easier to not skip the optimisation even if a signed integer might overflow might invalidate it. This it technically a valid rationalisation of the reason the UB exists in the first place even if it is (probably) not the original intent. (Butt monkeys are not a valid rationalisation!)

One last thing, they didn't give a list of implementations because at least some of them were mathematicians and they knew there are number representations that have never been used in computer hardware ... but they could be.

PS: @squeek502 Sorry for the noise.

@jimkring
Copy link

jimkring commented Mar 8, 2024

I was shown by @nektro that there's some helpful, relevant info in the zig FAQ here -> why-do-i-get-illegal-instruction-when-using-with-zig-cc-to-build-c-code

@rdebath
Copy link
Author

rdebath commented Mar 10, 2024

The problem is nobody asks that question because the answer is already obvious.
"If a compiler generates an illegal instruction it's a broken compiler."
(Or occasionally it's an i686 compile attempting to run on an i386, ie: a broken compile.)

The more correct thing to do on an x86, as I mentioned before, is an into trap, just like divide by zero does. This gets converted into a SIGFPE and gives an error that actually says your program is wrong (even if it says "floating point" rather than "numeric").

PS: I guess "Why is zig's C compiler broken" isn't a question you want to answer though 😈

@Beyley
Copy link

Beyley commented Mar 10, 2024

PS: I guess "Why is zig's C compiler broken" isn't a question you want to answer though 😈

This is behaviour coming from LLVM/clang, not Zig, if you want to change how UBSAN works upstream in clang, i highly encourage you to try

@rdebath
Copy link
Author

rdebath commented Mar 13, 2024

@Beyley Actually, no. Clang's UBsan has already been changed to give verbose messages and Zig is planned see #5163
and #17065 (comment) above. Also back-porting doesn't need to be requested because it was never on by default.

And just to be clear, squeek502 answered this issue in full right at the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants