[compiler-rt] Fix false positive detection of a target in compile-only mode

[skosukhin]

Summary

When compiler-rt is configured as a runtime, the configure-time target detection for builtins is done in compile-only mode, which is basically a test of whether the newly-built clang can compile a simple program with an additional flag (-m32 and -m64 in my case). The problem is that on my Debian system clang can compile int foo(int x, int y) { return x + y; } with -m32 but fails to include limits.h (or any other target-specific header) for the i386 target:

$ /path/to/build/./bin/clang --target=x86_64-unknown-linux-gnu -DVISIBILITY_HIDDEN  -O3 -DNDEBUG  -m32 -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -MD -MT CMakeFiles/clang_rt.builtins-i386.dir/absvdi2.c.o -MF CMakeFiles/clang_rt.builtins-i386.dir/absvdi2.c.o.d -o CMakeFiles/clang_rt.builtins-i386.dir/absvdi2.c.o -c /path/to/src/compiler-rt/lib/builtins/absvdi2.c
In file included from /path/to/src/compiler-rt/lib/builtins/absvdi2.c:13:
In file included from /path/to/src/compiler-rt/lib/builtins/int_lib.h:93:
In file included from /path/to/build/lib/clang/15.0.0/include/limits.h:21:
In file included from /usr/include/limits.h:25:
/usr/include/features.h:364:12: fatal error: 'sys/cdefs.h' file not found
#  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
1 error generated.

This is an attempt to make the target detection more robust: extend the test program with #include <limits.h>. A small problem here is that we might need to include either sys/limits.h or machine/limits.h instead. So, the first attempt to solve this is to simply iterate over possible system header names and accept the target if any of the headers is available.

[compnerd]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

131–133

This seems incorrect. limits.h is the standard header and should be available. This seems more related to -m32 requiring the 32-bit compatibility headers on Linux. The other headers are not the same header in different locations.

[skosukhin]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

131–133

limits.h is available (/usr/include/limits.h) but it needs sys/cdefs.h, which is expected in /usr/include with the -m32 argument. However, it is not there. It's in /usr/include/x86_64-linux-gnu, which is not on the list of header search paths in this case (as it should be, I guess). There is Debian package libc6-dev-i386, which is not installed on my machine. Among other things, it creates symlink /usr/include/sys/cdefs.h that points to /usr/include/x86_64-linux-gnu/sys/cdefs.h. So, sys/cdefs.h is supposed to be the same for i386 as for x86_64. But even if I try something very strange - compile with -isystem/usr/include/x86_64-linux-gnu - I get an error that gnu/stubs-32.h is missing. So, there is no way to #include <limits.h> in my configuration.

I understand that sys/limits.h and machine/limits.h are not the same but this implies that limits.h might not be needed in some situations, therefore it is wrong to regect the target if the header is not available in those cases. At first, I was going to extend the test program with the same #if conditions as in the code but decided that it would be less maintainable than simply checking for the "alternatives".

I admit that my solution is at least awkward but, as far as I understand it, CMake is supposed to figure out that the project cannot really be built for i386 due to missing headers and disable the target. Probably, checking for limits.h is not the best choice, so I would appreciate a piece of advice on how to do it better.

[skosukhin]

It turns out that the problem is related to D91334. Before the change, cmake ran an additional linking step in check_compile_definition->check_symbol_exists->try_compile, which failed as it should:

/usr/bin/ld: cannot find crt1.o: No such file or directory
/usr/bin/ld: cannot find crti.o: No such file or directory
/usr/bin/ld: cannot find crtbegin.o: No such file or directory
/usr/bin/ld: cannot find -lgcc
/usr/bin/ld: cannot find -lgcc_s
/usr/bin/ld: cannot find -lc
/usr/bin/ld: cannot find -lgcc
/usr/bin/ld: cannot find -lgcc_s
/usr/bin/ld: cannot find crtend.o: No such file or directory
/usr/bin/ld: cannot find crtn.o: No such file or directory

This was the reason for cmake to discard i386. After the aforementioned modification, the linking step is skipped and i386 is accepted.

An additional question now is whether try_compile_only is needed at all. At least, in test_target_arch.

I will try to ping people from the discussion in D91334 to ask them whether they have any ideas on how to proceed with this (sorry if that's somehow against any rules of the community).

[mstorsjo]

If the issue is that compiling with -m32 generally seems to work, but fails in practice due to some header missing, perhaps the cmake test should be extended to try compiling a file that includes that header?

[mstorsjo]

In D127975#3592242, @mstorsjo wrote:

If the issue is that compiling with -m32 generally seems to work, but fails in practice due to some header missing, perhaps the cmake test should be extended to try compiling a file that includes that header?

Sorry, I didn’t look at the patch itself, only the discussion - I see that the patch does exactly that. Thus, it looks quite reasonable to me!

[mstorsjo]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

131–133

Yes - limits.h is a standard header and can be expected to found IF you have a full complete working toolchain - but builtins are built at a point where you don't have a full complete working toolchain yet. Additionally when it tries to build both i386 and x86_64, even if you might not have complete working headers for both.

But I agree that we probably shouldn't be probing for nonstandard headers like sys/limits.h and machine/limits.h (which aren't available on all systems, even if their toolchains are complete). Is it enough to fix the issue to just check whether including limits.h alone succeeds?

[skosukhin]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

131–133

In my case, checking just for limits.h is enough. The question is whether it will break this case (please, note, however, that fp_lib.h includes limits.h unconditionally).

[mstorsjo]

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

131–133

Nah, I think it's fine - if that part ever becomes a problem, we can extend the cmake check (but then it'd need to be within similar BSD conditions in cmake too) - as right now, those headers aren't available and aren't needed for other platforms. So modifying this patch to just one try_compile_only, which includes <limits.h> unconditionally, sounds good to me.

[skosukhin]

This introduces checking for limits.h only.

[mstorsjo]

Thanks, this looks reasonable to me!

Do you need someone to push the patch for you? In that case, can you provide your preferred git author line, i.e. Real Name <email@address>?

[skosukhin]

@mstorsjo, thanks! I can't push the patch myself, I guess, so, yes, it would be great if somebody did it for me. The author line is Sergey Kosukhin <[e-mail]>.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

macro(test_target_arch arch def)
   foreach(arg ${ARGN})
     set(argstring "${argstring} ${arg}")
   endforeach()
   check_compile_definition("${def}" "${argstring}" HAS_${arch}_DEF)
   if(NOT DEFINED CAN_TARGET_${arch})
     if(NOT HAS_${arch}_DEF)
       set(CAN_TARGET_${arch} FALSE)
     elseif(TEST_COMPILE_ONLY)
-      try_compile_only(CAN_TARGET_${arch} FLAGS ${TARGET_${arch}_CFLAGS})
+      try_compile_only(CAN_TARGET_${arch}
+                       SOURCE "#include <limits.h>\nint foo(int x, int y) { return x + y; }\n"
+                       FLAGS ${TARGET_${arch}_CFLAGS})
     else()
       set(FLAG_NO_EXCEPTIONS "")
       if(COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG)
         set(FLAG_NO_EXCEPTIONS " -fno-exceptions ")
       endif()
       set(SAVED_CMAKE_EXE_LINKER_FLAGS ${CMAKE_EXE_LINKER_FLAGS})
       set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${argstring}")
       try_compile(CAN_TARGET_${arch} ${CMAKE_BINARY_DIR} ${SIMPLE_SOURCE}