This is an archive of the discontinued LLVM Phabricator instance.

Differential D105829

sanitizer_common: Fix build for tests
ClosedPublic

Authored by melver on Jul 12 2021, 11:23 AM.

Details

Reviewers
dvyukov
vitalybuka
hctim
Commits
rG98033fdc50e6: sanitizer_common: Fix build for tests
Summary

It turns out that COMPILER_RT_TEST_COMPILER_CFLAGS is actually a string
that is being appended to and not a list.

Therefore, append the thread-safety flags to the string. Because CMake
separates list elements by ';' when turning into a string, also
substitute ';' with ' '.

Diff Detail

Event Timeline

melver created this revision.Jul 12 2021, 11:23 AM
Herald added a subscriber: mgorny. · View Herald TranscriptJul 12 2021, 11:23 AM
melver requested review of this revision.Jul 12 2021, 11:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 11:23 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
melver mentioned this in D105808: sanitizer_common: fix 32-bit build.Jul 12 2021, 11:24 AM
vitalybuka added a subscriber: vitalybuka.Jul 12 2021, 11:27 AM
vitalybuka added inline comments.
compiler-rt/CMakeLists.txt
370

COMPILER_RT_TEST_COMPILER_CFLAGS may have escaped ;
could you fix ; of THREAD_SAFETY_FLAGS in a separate var before appending?

vitalybuka added inline comments.Jul 12 2021, 11:30 AM
compiler-rt/CMakeLists.txt
370

is n't removing quotes around " ${THREAD_SAFETY_FLAGS}" makes it space separated?

melver updated this revision to Diff 358009.Jul 12 2021, 11:36 AM

Use temporary to build space-separated string of flags.

melver marked 2 inline comments as done.Jul 12 2021, 11:37 AM
melver added inline comments.
compiler-rt/CMakeLists.txt
370

Sadly, no.

In that case, I get:

set_default("target_cflags", "-Werror=thread-safety-Werror=thread-safety-reference-Werror=thread-safety-beta ")
melver added a reviewer: vitalybuka.Jul 12 2021, 11:37 AM
Harbormaster completed remote builds in B113548: Diff 358009.Jul 12 2021, 12:25 PM
hctim accepted this revision.Jul 12 2021, 3:38 PM
hctim added a subscriber: hctim.

LGTM - given timezones I'll submit to fix up the bot. Tested on my machine.

This revision is now accepted and ready to land.Jul 12 2021, 3:38 PM
Closed by commit rG98033fdc50e6: sanitizer_common: Fix build for tests (authored by melver, committed by hctim). · Explain WhyJul 12 2021, 3:50 PM
This revision was automatically updated to reflect the committed changes.
hctim added a commit: rG98033fdc50e6: sanitizer_common: Fix build for tests.
thakis added a subscriber: thakis.Jul 12 2021, 4:08 PM

All compiler-rt tests are still broken on Windows even with this. I'll revert the changes that add this. (https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8841885532543990160/+/u/package_clang/stdout?format=raw)

thakis added a comment.Jul 12 2021, 4:10 PM

Oh wait, I think that run missed this change here by 36 revisions. I'll try again with this.

dvyukov added a comment.Jul 12 2021, 11:56 PM

LGTM

thakis added a comment.Jul 13 2021, 5:48 AM
In D105829#2872592, @thakis wrote:

Oh wait, I think that run missed this change here by 36 revisions. I'll try again with this.

(Seems to work with this. Thanks for the fix!)

Revision Contents

PathSize
compiler-rt/
3 lines

Diff 358009

compiler-rt/CMakeLists.txt

Show First 20 Lines • Show All 360 Lines • ▼ Show 20 Lines
if(CMAKE_CXX_COMPILER_ID MATCHES Clang) if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
list(APPEND THREAD_SAFETY_FLAGS list(APPEND THREAD_SAFETY_FLAGS
"-Werror=thread-safety" "-Werror=thread-safety"
"-Werror=thread-safety-reference" "-Werror=thread-safety-reference"
"-Werror=thread-safety-beta" "-Werror=thread-safety-beta"
) )
list(APPEND SANITIZER_COMMON_CFLAGS ${THREAD_SAFETY_FLAGS}) list(APPEND SANITIZER_COMMON_CFLAGS ${THREAD_SAFETY_FLAGS})
list(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS ${THREAD_SAFETY_FLAGS}) string(REPLACE ";" " " thread_safety_flags_space_sep "${THREAD_SAFETY_FLAGS}")
string(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS " ${thread_safety_flags_space_sep}")
vitalybukaUnsubmitted
Done
ReplyInline Actions

COMPILER_RT_TEST_COMPILER_CFLAGS may have escaped ;
could you fix ; of THREAD_SAFETY_FLAGS in a separate var before appending?

vitalybuka: COMPILER_RT_TEST_COMPILER_CFLAGS may have escaped ; could you fix ; of THREAD_SAFETY_FLAGS in a…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

is n't removing quotes around " ${THREAD_SAFETY_FLAGS}" makes it space separated?

vitalybuka: is n't removing quotes around " ${THREAD_SAFETY_FLAGS}" makes it space separated?
melverAuthorUnsubmitted
Done
ReplyInline Actions

Sadly, no.

In that case, I get:

set_default("target_cflags", "-Werror=thread-safety-Werror=thread-safety-reference-Werror=thread-safety-beta ")
melver: Sadly, no. In that case, I get: ``` set_default("target_cflags", "-Werror=thread-safety…
endif() endif()
# If we're using MSVC, # If we're using MSVC,
# always respect the optimization flags set by CMAKE_BUILD_TYPE instead. # always respect the optimization flags set by CMAKE_BUILD_TYPE instead.
if (NOT MSVC) if (NOT MSVC)
# Build with optimization, unless we're in debug mode. # Build with optimization, unless we're in debug mode.
if(COMPILER_RT_DEBUG) if(COMPILER_RT_DEBUG)
▲ Show 20 Lines • Show All 279 Lines • Show Last 20 Lines