This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][libcxxabi] CMAKE_REQUIRED_FLAGS is a string, not a list
ClosedPublic

Authored by nehaljwani on Sep 6 2021, 8:17 PM.

Details

Reviewers
compnerd
ldionne
Group Reviewers
Restricted Project
Restricted Project
Commits
rG1613ab8a4a3e: [libcxx][libcxxabi] CMAKE_REQUIRED_FLAGS is a string, not a list
Summary

When libcxx or libcxxabi is built with -DLLVM_USE_SANITIZER=MemoryWithOrigins
and -DLIBCXX[ABI]_USE_COMPILER_RT=ON, all of the LIBCXX[ABI]_SUPPORTS_*_FLAG
checks fail, since the value of CMAKE_REQUIRED_FLAGS is not set correctly.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=51774

Diff Detail

Event Timeline

nehaljwani created this revision.Sep 6 2021, 8:17 PM
nehaljwani requested review of this revision.Sep 6 2021, 8:17 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 6 2021, 8:17 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
nehaljwani updated this revision to Diff 370987.Sep 6 2021, 8:20 PM

Added link to BZ in commit message

compnerd requested changes to this revision.Sep 6 2021, 8:34 PM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
libcxx/cmake/config-ix.cmake
53

This is changing the behavior. CMAKE_REQUIRED_LIBRARIES was being set here previously and that is correct. Unlike CMAKE_REQUIRED_FLAGS, that is a ; delimited list.

libcxxabi/cmake/config-ix.cmake
43

Similar.

This revision now requires changes to proceed.Sep 6 2021, 8:34 PM
nehaljwani updated this revision to Diff 371049.Sep 7 2021, 6:01 AM

Remove changes that touch CMAKE_REQUIRED_LIBRARIES

nehaljwani marked 2 inline comments as done.Sep 7 2021, 6:02 AM

Can you add a more verbose description? The current commit message is a bit short.
It seems buildkite has a lot of failures. Looking at the used pipelines I miss a lot of them. Is your patch against a recent version of main?

ldionne accepted this revision.Sep 7 2021, 9:38 AM
ldionne added a subscriber: ldionne.

LGTM if CI passes.

compnerd accepted this revision.Sep 7 2021, 2:44 PM
This revision is now accepted and ready to land.Sep 7 2021, 2:44 PM
nehaljwani updated this revision to Diff 371352.EditedSep 8 2021, 8:45 AM

Added more information in the commit message, rebased over latest origin/main

Added more information in the commit message, rebased over latest origin/main

Thanks Buildkite now looks good. I don't see a new description in this patch. Did you only update it in your commit message?

nehaljwani edited the summary of this revision. (Show Details)Sep 8 2021, 3:25 PM

I've updated the summary of this differential as well. Is that what you were referring to, @Mordante ?

I've updated the summary of this differential as well. Is that what you were referring to, @Mordante ?

Yes thanks. Especially nice to know this fixes a bug.

Do you need somebody to commit his for you? If so please provide "your name" <your@email.address> so somebody can commit on your behalf.

I don't have any commit rights. Here is the info you need:

$ git show | grep Author:
Author: Nehal J Wani <nehaljw.kkd1@gmail.com>