[compiler-rt] Fix a check that controls adding builtins to CMAKE_REQUIRED_LIBRARIES

[barannikov88]

Summary

The condition was always false.

[barannikov88]

compiler-rt/cmake/config-ix.cmake

238

This check is also broken, but I'm not sure how to fix it.
There is no arch variable in this scope, so ${arch} expands to nothing and
NOT MATCHES "avr|msp430" evaluates to false because there is no variable named NOT.

[barannikov88]

compiler-rt/cmake/config-ix.cmake

238

I'd drop this if if there are no objections.
Targets that are interested in the value of the variable check it inside test_targets below.

[phosek]

compiler-rt/cmake/config-ix.cmake

44

I'm fine with this change as a cleanup, but this should behave identically. The change description says "The condition was always false." Where did you observe this?

238

I'd consider removing this entire check since I'm not sure whether it provides any value, but I'd do it in a separate change.

[barannikov88]

compiler-rt/cmake/config-ix.cmake

44

I found it when porting compiler-rt for an out-of-tree target. It was probably unnoticed because it only affects checks like check_symbol_exists.

This should demonstrate the issue:

cmake_minimum_required(VERSION 3.20)

project(test)

execute_process(
  COMMAND which time
  OUTPUT_VARIABLE time
)
string(STRIP "${time}" time)
file(TO_CMAKE_PATH "${time}" time)

message(STATUS "time=${time}")

if (${time})
  message(STATUS "${time} is true")
else()
  message(STATUS "${time} is false")
endif()

-- time=/usr/bin/time
-- /usr/bin/time is false

[barannikov88]

Kindly ping

[barannikov88]

compiler-rt/cmake/config-ix.cmake

44

if (${VAR}) and if (VAR) behave identical for boolean values but not for string values.
This is because ON, True etc. are recognized by CMake, everything else is considered a variable name.
In this example, "/usr/bin/time" is a valid variable name, but there is no such variable, so it evaluates to false.

[barannikov88]

ping

[barannikov88]

ping?

[phosek]

compiler-rt/cmake/config-ix.cmake

43–44

I think we should remove this condition altogether since PR51389 has already been addressed.

[barannikov88]

compiler-rt/cmake/config-ix.cmake

43–44

Thanks for the heads-up, I'll see what I can do

[barannikov88]

compiler-rt/cmake/config-ix.cmake

43–44

I think we should remove this condition altogether since PR51389 has already been addressed.

It is still open though https://github.com/llvm/llvm-project/issues/50731
Could you point me to the commit that addressed the issue? At quick glance the issue is still there.

[phosek]

compiler-rt/cmake/config-ix.cmake

43–44

It was addressed in https://reviews.llvm.org/D88458.

[barannikov88]

compiler-rt/cmake/config-ix.cmake

43–44

Thanks!

compiler-rt/cmake/config-ix.cmake

 
 check_c_compiler_flag(-nodefaultlibs C_SUPPORTS_NODEFAULTLIBS_FLAG)
 if (C_SUPPORTS_NODEFAULTLIBS_FLAG)
   set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nodefaultlibs")
   if (COMPILER_RT_HAS_LIBC)
     list(APPEND CMAKE_REQUIRED_LIBRARIES c)
   endif ()
   if (COMPILER_RT_USE_BUILTINS_LIBRARY)
     # TODO: remote this check once we address PR51389.
-    if (${COMPILER_RT_BUILTINS_LIBRARY})
+    if (COMPILER_RT_BUILTINS_LIBRARY)
       list(APPEND CMAKE_REQUIRED_LIBRARIES "${COMPILER_RT_BUILTINS_LIBRARY}")
     endif()
   elseif (COMPILER_RT_HAS_GCC_S_LIB)
     list(APPEND CMAKE_REQUIRED_LIBRARIES gcc_s)
   elseif (COMPILER_RT_HAS_GCC_LIB)
     list(APPEND CMAKE_REQUIRED_LIBRARIES gcc)
   endif ()
   if (MINGW)

 set(SIMPLE_SOURCE ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/simple.cc)
 file(WRITE ${SIMPLE_SOURCE} "#include <stdlib.h>\n#include <stdio.h>\nint main(void) { printf(\"hello, world\"); }\n")
 
 # Detect whether the current target platform is 32-bit or 64-bit, and setup
 # the correct commandline flags needed to attempt to target 32-bit and 64-bit.
 # AVR and MSP430 are omitted since they have 16-bit pointers.
 if (NOT CMAKE_SIZEOF_VOID_P EQUAL 4 AND
     NOT CMAKE_SIZEOF_VOID_P EQUAL 8 AND
     NOT ${arch} MATCHES "avr|msp430")
   message(FATAL_ERROR "Please use architecture with 4 or 8 byte pointers.")
 endif()
 
 test_targets()
 
 # Returns a list of architecture specific target cflags in @out_var list.
 function(get_target_flags_for_arch arch out_var)
   list(FIND COMPILER_RT_SUPPORTED_ARCH ${arch} ARCH_INDEX)