llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

clang crashes due to bool redeclaration

Open ivanrodriguez3753 opened this issue 2 years ago • 12 comments

/* Visual Studio < 2013 does not have stdbool.h so here it is a replacement: */
#if defined __STDC__ && defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
/* have a C99 compiler */
typedef _Bool bool;
#else
/* do not have a C99 compiler */
typedef unsigned char bool;
#endif

When compiling with OpenMP for an AMDGPU, it seems to automatically include stdbool.h.

> cat test.c
/* Visual Studio < 2013 does not have stdbool.h so here it is a replacement: */
#if defined __STDC__ && defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
/* have a C99 compiler */
typedef _Bool bool;
#else
/* do not have a C99 compiler */
typedef unsigned char bool;
> clang -c -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx908 test.c 
test.c:4:15: error: cannot combine with previous 'bool' declaration specifier
typedef _Bool bool;
              ^
lib/clang/16.0.0/include/stdbool.h:20:14: note: expanded from macro 'bool'
#define bool _Bool
             ^
../test.c:4:1: warning: typedef requires a name [-Wmissing-declarations]
typedef _Bool bool;
^~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.

There is no error when compiling for the CPU (leave off all openmp flags). I also noticed that amdclang does not run into this problem (using amdclang from rocm/5.1.0). Is there special handling going on in amdclang that is not going on upstream? amdclang shows the desired behavior. That is, amdclang runs into the problem if you modify the testcase to include stdbool.h, that is, prepend #include stdbool.h to the testcase.

ivanrodriguez3753 avatar Aug 10 '22 19:08 ivanrodriguez3753

Tagging @arsenm for visibility and additional contacts (if appropriate)

ivanrodriguez3753 avatar Aug 10 '22 19:08 ivanrodriguez3753

Also tested with rocm/5.2.1, same behavior for amdclang (lack of error).

ivanrodriguez3753 avatar Aug 10 '22 19:08 ivanrodriguez3753

@llvm/issue-subscribers-clang-driver

llvmbot avatar Aug 10 '22 21:08 llvmbot

@llvm/issue-subscribers-openmp

llvmbot avatar Aug 10 '22 21:08 llvmbot

Is it possible for you to verify that the patch in https://reviews.llvm.org/D131639 solves your issue?

jhuber6 avatar Aug 11 '22 02:08 jhuber6

It gets past the original error but, like @shiltian mentioned on your review, bool is used by another file:

In file included from <built-in>:1:
In file included from /ptmp/rodriiva/hpc-pe-cce-cce/build/dev/install/cce-clang/x86_64/lib/clang/15.0.0/include/openmp_wrappers/__clang_openmp_device_functions.h:51:
/ptmp/rodriiva/hpc-pe-cce-cce/build/dev/install/cce-clang/x86_64/lib/clang/15.0.0/include/__clang_hip_libdevice_declares.h:315:63: error: unknown type name 'bool'
                                                     float c, bool s);
                                                              ^
1 error generated.

ivanrodriguez3753 avatar Aug 11 '22 15:08 ivanrodriguez3753

Yeah that's the only usage I know of, was wondering if it work work regardless. One solution is to just use _Bool directly.

jhuber6 avatar Aug 11 '22 15:08 jhuber6

Sure. Do you want to update your patch or should I push one?

ivanrodriguez3753 avatar Aug 11 '22 15:08 ivanrodriguez3753

I'll update it real quick, let me know if it works because I didn't have any errors on my end.

jhuber6 avatar Aug 11 '22 15:08 jhuber6

I also got the provided test case to compile, but I ran it on our in-house test suites and a lot of stuff broke. I still haven't gotten to the bottom of it, but it seems it's causing kernels to be invalid when attempting to load them via HIP. I haven't ran any upstream test suites yet, however.

ivanrodriguez3753 avatar Aug 12 '22 15:08 ivanrodriguez3753

If this were a simple workaround to make someone happy I'd be fine, but since this seems to be a rather annoying work-around only required for broken user code I'm tempted to close this as wontfix.

jhuber6 avatar Aug 12 '22 15:08 jhuber6

Turns out my build environment was erroneous. The patch worked cleanly and didn't break anything on this end.

ivanrodriguez3753 avatar Aug 15 '22 17:08 ivanrodriguez3753

As discussed in D131639, we will not work around this. There is no guarantee that the system or toolchain will not include headers like <stdbool.h> so we shouldn't be expected to work around these cases. The user should instead use the provided __bool_true_false_are_defined macro to prevent potential collisions with <stdbool.h>.

jhuber6 avatar Aug 29 '22 16:08 jhuber6