ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
33.77k stars 2.47k forks source link

Zig cc generated wrong executable code for switch statement when (-Ofast) is used #19369

Open MahmoudFayed opened 6 months ago

MahmoudFayed commented 6 months ago

Zig Version

0.12.0

Steps to Reproduce and Observed Behavior

All of the steps and how to reproduce the bug is explained in detail here: https://github.com/ring-lang/ring/blob/master/marketing/articles/ZigCCisNotReadyYet.md

The bug is discovered while using Zig cc to build the Ring programming language

Expected Behavior

As in this article: https://github.com/ring-lang/ring/blob/master/marketing/articles/ZigCCisNotReadyYet.md

silversquirl commented 6 months ago

It would help if you could provide a small reproduction. Also, do you see the same behaviour with clang? zig cc is a wrapper for clang with a few extra features, so I would be surprised if it introduces miscompilations that clang doesn't have

Rexicon226 commented 6 months ago

Also, do you see the same behaviour with clang?

All other compilers including Clang doesn't have this bug and produce valid Ring executable
silversquirl commented 6 months ago

@MahmoudFayed I can reproduce this with Clang on Ubuntu 22.04, using your Clang build script. I suspect your code is triggering undefined behaviour, causing garbage optimizations in Clang. Regardless, this is not an issue in Zig.

MahmoudFayed commented 6 months ago

@silversquirl I am use Clang and Zig on Windows Clang (Windows) doesn't have the problem Zig (Windows) have it I think this requires attention at Zig project level

Tested building Ring on macOS using Clang ---> The problem doesn't exist there too. Will test it now on Ubuntu (based on your feedback) to see the results.

silversquirl commented 6 months ago

Your clang build script for windows doesn't provide any optimization options. If you provide -O2 or -Ofast, I expect you will see the same result as zig cc

MahmoudFayed commented 6 months ago

I added (-Ofast) in this commit: https://github.com/ring-lang/ring/commit/90ad2071925f8cb563d99b3e44adc5cf858a8e6d

The problem doesn't exist when using (Clang) in windows while (-Ofast) is active

ehaas commented 6 months ago

I can confirm that the bug happens on MacOS using Apple clang version 15.0.0 (clang-1500.3.9.4) if -O2 is used, and does not occur if -O0 is used. This is either a clang optimizer bug or undefined behavior in your C code; it is not a zig cc bug.

MahmoudFayed commented 6 months ago

@ehaas We already use -o2 on macOS and we don't have this problem: https://github.com/ring-lang/ring/blob/master/language/build/buildclang.sh

ehaas commented 6 months ago

Here is a screen recording of myself compiling it with Apple system clang and reproducing the bug.

https://github.com/ziglang/zig/assets/4575/7de29a2e-0e0c-4158-9105-62ff7136765e

The warning about unsequenced modifications to nNum2 may be relevant.

MahmoudFayed commented 6 months ago

The warning is related to a function implementation called Random() Doesn't have anything to do with the reported bug.

The bug doesn't appears using my old Clang version: Apple clang version 12.

I can reproduce it now using Clang (Windows) - A modern version of Clang.

We can say now it's not a Zig bug ---> Looks like a Clang bug.

Thanks

nektro commented 6 months ago

if it reproduces on the latest clang as well, we generally still track it here if it's affecting zig

MahmoudFayed commented 6 months ago

@nektro I am trying to find a way (workaround) to still use Zig/Clang and (Ofast) flag and avoid this issue

MahmoudFayed commented 6 months ago

Hello

I applied a workaround to avoid the problem so I can use Zig cc Or Clang & keep the -Ofast flag - The workaround is applied in this commit: https://github.com/ring-lang/ring/commit/f9abf2376e61f0eea37091095f9c2de4eebbbd1e Thanks to everyone who checked this issue. Keep up the GREAT WORK