Page MenuHomePhabricator

[compiler-rt] Fix cmake warnings
ClosedPublic

Authored by winksaville on Apr 26 2019, 10:46 AM.

Details

Summary
  • Fix cmake BOOL misspellings
  • Set cmake policy for CMP0075 to NEW

As requested by smeeani I've compared CMAkeCache.txt in master with and
without this patch and the only changes are to the variable types I fixed:

    
$ diff build-b1-master/CMakeCache.txt build-b1-compiler-rt-fix-cmake-warnings/CMakeCache.txt
503c503
< COMPILER_RT_BAREMETAL_BUILD:STRING=OFF
---
> COMPILER_RT_BAREMETAL_BUILD:BOOL=OFF
550c550
< COMPILER_RT_HWASAN_WITH_INTERCEPTORS:STRING=ON
---
> COMPILER_RT_HWASAN_WITH_INTERCEPTORS:BOOL=ON

Diff Detail

Repository
rL LLVM

Event Timeline

winksaville created this revision.Apr 26 2019, 10:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 26 2019, 10:46 AM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 4 others. · View Herald Transcript

I think __hwasan_tls should be defined if !SANITIZER_ANDROID, without the other condition.

I think __hwasan_tls should be defined if !SANITIZER_ANDROID, without the other condition.

Ok, should I do that here or a different change?

Looking at the code it seems we should remove HWASAN_WITH_INTERCEPTORS in hwasan_linux.cpp
and just use SANITIZER_ANDROID, your thoughts?

What have I done wrong to cause the 8-10 FAILED's tests?

Sorry I've missed your comment. I've uploaded a different fix for __hwasan_tls problem:
https://reviews.llvm.org/D61337

I don't know about the test failures. You'll have to dig deeper.

I believe this is ready to merge.

I checked out llvmorg-8.0.0 of llvm/llvm-project:

$ git log -1
commit d2298e74235598f15594fe2c99bbac870a507c59 (HEAD, tag: llvmorg-8.0.0)
Author: Hans Wennborg <hans@hanshq.net>
Date:   Fri Mar 15 08:44:10 2019 +0000

    Re-generate DiagnosticsReference.rst (PR41080)
    
      $ bin/clang-tblgen -gen-diag-docs -I../cfe.src/include
      -I../cfe.src/include/clang/Basic/
      ../cfe.src/include/clang/Basic/Diagnostic.td -o
      ../cfe.src/docs/DiagnosticsReference.rst
    
    llvm-svn: 356240

And ran these commands capturing the output:

$ cmake ../llvm -G Ninja '-DLLVM_ENABLE_PROJECTS=clang;lld;compiler-rt' -DCMAKE_INSTALL_PREFIX=/home/wink/local-llvmorg-8.0.0-check-tsan -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_LINKER=bfd
$ ninja -j11 -v
$ ninja -j11 -v check-tsan

And got these 5 failures:

$ grep 'FAILED ' log-llvmorg-8.0.0-check-tsan.txt 
******************** TEST 'ThreadSanitizer-x86_64 :: Linux/double_race.cc' FAILED ********************
******************** TEST 'ThreadSanitizer-x86_64 :: inlined_memcpy_race2.cc' FAILED ********************
******************** TEST 'ThreadSanitizer-x86_64 :: inlined_memcpy_race.cc' FAILED ********************
******************** TEST 'ThreadSanitizer-x86_64 :: memcmp_race.cc' FAILED ********************
******************** TEST 'ThreadSanitizer-x86_64 :: memcpy_race.cc' FAILED ********************
******************** TEST 'ThreadSanitizer-x86_64 :: memcpy_race.original.cc' FAILED ********************

So I conclude my changes have nothing to do with the failures I'm seeing and
I've created bug 41805

Is there anything more I need to do to get this merged?

This is a straightforward build system fix, so it should be fine.

The CMP0075 setting has some potential to change configure results. Could you do a clean CMake run before and after this patch and verify that the CMakeCache.txt files are the same? If so, this LGTM. (That's only testing for your configuration, so it's not comprehensive and there might be configuration changes on other platforms, but it at least gets us some basic sanity checking.)

Update commit message and rebase on newer master

Verified that the only CMakeCache.txt changes between
with and without this patch are the variable types.

smeenai accepted this revision.May 24 2019, 1:39 PM

LGTM

This revision is now accepted and ready to land.May 24 2019, 1:39 PM
winksaville edited the summary of this revision. (Show Details)May 24 2019, 1:41 PM
winksaville removed a reviewer: daemon.

@smeenai, who/how does this get submitted?

@winksaville I can commit this for you on Monday if no one's gotten to it before.

@winksaville I can commit this for you on Monday if no one's gotten to it before.

Txs

This revision was automatically updated to reflect the committed changes.

@smeenai thanks for submitting!