[cmake] Strip quotes in compiler-rt/lib/crt and explicitly throw error if check executable fails

[zhizhouy]

Summary

Similar change to CMakeLists as r372312.

After r372209, compiler command line may include argument with quotes:

-fprofile-instr-use="/foo/bar.profdata"

And it causes a hidden failure with execute_process later: Could not read profile "/foo/bar.profdata": No such file or directory.

In this particular case, the check for .init_array will fail silently and creates a PGO-ed binary with bad .init_array section in compiler-rt.

Bug details can be found in bug/45022

[hans]

Can you provide steps to reproduce what's broken here?

[zhizhouy]

Created a llvm bug on how to reproduce and referring it in summary: https://bugs.llvm.org/show_bug.cgi?id=45022

[phosek]

Wouldn't this break when compile command contains a quoted path with spaces?

[zhizhouy]

Wouldn't this break when compile command contains a quoted path with spaces?

As mentioned in the comment of r372312,
space should not be a new issue here since in line 50 we are going to replace all spaces with ";".

string(REPLACE " " ";" test_compile_command "${test_compile_command}")

[xur]

Thanks for working on this. Here are a few comments:
(1) I'm not sure why PGO options are passed to compiler-rt. It's probably harmless for -fprofile-use or -fprofile-instr-use as we only use the profile. My concern is we might instrument profdata runtime routine. This can create recursion that leads to run time issue.

(2) As I mentioned in (1), passing an invalid profile does not suppose to cause error in section. Silently ignoring the option should be a noop. The error seems to be incheck_cxx_section_exists() logic. In this sense, this patch does not seem to be complete to me: For example, if we pass an invalid profile path (after stripping the "), will it still fail?

[zhizhouy]

(1) I'm not sure why PGO options are passed to compiler-rt. It's probably harmless for -fprofile-use or -fprofile-instr-use as we only use the profile. My concern is we might instrument profdata runtime routine. This can create recursion that leads to run time issue.

I created a new patch to not pass PGO related options to compiler-rt: D75499

(2) As I mentioned in (1), passing an invalid profile does not suppose to cause error in section. Silently ignoring the option should be a noop. The error seems to be incheck_cxx_section_exists() logic. In this sense, this patch does not seem to be complete to me: For example, if we pass an invalid profile path (after stripping the "), will it still fail?

Seems that if the profile path is not correct, it will be filtered before getting passed into compiler-rt, so your example should not happen. But it is not good to fail silently, so I added a check for it.

[xur]

LGTM

compiler-rt/lib/crt/CMakeLists.txt

elseif(substitution STREQUAL "<FLAGS>")
       string(REPLACE "<FLAGS>" "${CMAKE_C_FLAGS} ${extra_flags}"
              test_compile_command ${test_compile_command})
     else()
       string(REPLACE "${substitution}" "" test_compile_command
              ${test_compile_command})
     endif()
   endforeach()
 
+  # Strip quotes from the compile command, as the compiler is not expecting
+  # quoted arguments (potential quotes added from D62063).
+  string(REPLACE "\"" "" test_compile_command "${test_compile_command}")
+
   string(REPLACE " " ";" test_compile_command "${test_compile_command}")
 
   execute_process(
     COMMAND ${test_compile_command}
     RESULT_VARIABLE TEST_RESULT
     OUTPUT_VARIABLE TEST_OUTPUT
     ERROR_VARIABLE TEST_ERROR
   )
 
+  # Explicitly throw a fatal error message if test_compile_command fails.
+  if(TEST_RESULT)
+    message(FATAL_ERROR "${TEST_ERROR}")
+    return()
+  endif()
+
   execute_process(
     COMMAND ${CMAKE_OBJDUMP} -h "${TARGET_NAME}/CheckSectionExists.o"
     RESULT_VARIABLE CHECK_RESULT
     OUTPUT_VARIABLE CHECK_OUTPUT
     ERROR_VARIABLE CHECK_ERROR
   )
   string(FIND "${CHECK_OUTPUT}" "${section}" SECTION_FOUND)