This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Drop -Werror in compiler flag checks
Changes PlannedPublic

Authored by barannikov88 on Mar 26 2023, 9:28 AM.

Details

Summary

-Werror is redundant because CMake not only checks the
exit status, but also parses the output of try_compile.
-Wformat-security is a special case, because specifying it
without -Wformat causes gcc to emit a warning in a format
not recognized by CMake, which makes the check pass.

Diff Detail

Event Timeline

barannikov88 created this revision.Mar 26 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 9:28 AM
Herald added subscribers: Enna1, dberris. · View Herald Transcript
barannikov88 requested review of this revision.Mar 26 2023, 9:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 9:28 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
barannikov88 edited the summary of this revision. (Show Details)

Update commit message

I'm not sure this is the right thing to do. The regression was introduced in https://github.com/llvm/llvm-project/commit/f051c1ded40970169cda84b0966331ae7ad424ed

The fix seems correct to me.

compiler-rt/cmake/config-ix.cmake
96

Does this have to be fixed too?

Fix another occurance

barannikov88 marked an inline comment as done.Mar 28 2023, 1:19 PM
This revision is now accepted and ready to land.Mar 28 2023, 8:59 PM
This revision was landed with ongoing or failed builds.Mar 28 2023, 9:22 PM
This revision was automatically updated to reflect the committed changes.

I don't understand the motivation behind rGf051c1ded40970169cda84b0966331ae7ad424ed, why is it needed? check_cxx_compiler_flag checks if the flag is accepted, whether the flag itself causes Clang to emit error or warning shouldn't have any impact on the result of the check.

compiler-rt/cmake/config-ix.cmake
96

This shouldn't need -Werror at all.

ahatanak added a subscriber: wlei.Mar 29 2023, 6:28 AM

I added -Werror because the changes I made in https://reviews.llvm.org/D131714 broke someone's internal build (see @wlei's comment). Apparently, gcc emits a warning if you try to use -Wformat-security without passing -Wformat too.

-Werror is probably only needed for -Wformat-security, but I think passing it in other places doesn't do any harm.

Hey, looks like this breaks check-sanitizer on all the bots. Easily reproducible on a vanilla build, just do check-sanitizer on something that builds compiler-rt.

https://lab.llvm.org/buildbot/#/builders/85/builds/15278

[4/90] Generating Sanitizer-x86_64-Test-Nolibc
FAILED: projects/compiler-rt/lib/sanitizer_common/tests/Sanitizer-x86_64-Test-Nolibc /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build0/projects/compiler-rt/lib/sanitizer_common/tests/Sanitizer-x86_64-Test-Nolibc 
cd /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build0/projects/compiler-rt/lib/sanitizer_common/tests && /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build0/./bin/clang++ sanitizer_nolibc_test_main.x86_64.o -Wl,-whole-archive libRTSanitizerCommon.test.nolibc.x86_64.a -Wl,-no-whole-archive -o /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build0/projects/compiler-rt/lib/sanitizer_common/tests/./Sanitizer-x86_64-Test-Nolibc -static -nostdlib -m64 -m64
ld: error: undefined symbol: memset
>>> referenced by sanitizer_allocator_primary32.h:356 (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h:356)
>>>               sanitizer_allocator.cpp.o:(__sanitizer::SizeClassAllocator32<__sanitizer::AP32>::PopulateFreeList(__sanitizer::AllocatorStats*, __sanitizer::SizeClassAllocator32LocalCache<__sanitizer::SizeClassAllocator32<__sanitizer::AP32> >*, __sanitizer::SizeClassAllocator32<__sanitizer::AP32>::SizeClassInfo*, unsigned long)) in archive libRTSanitizerCommon.test.nolibc.x86_64.a
>>> referenced by sanitizer_deadlock_detector.h:244 (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h:244)
>>>               sanitizer_deadlock_detector1.cpp.o:(__sanitizer::DeadlockDetector<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >::addEdges(__sanitizer::DeadlockDetectorTLS<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >*, unsigned long, unsigned int, int)) in archive libRTSanitizerCommon.test.nolibc.x86_64.a
>>> referenced by sanitizer_printf.cpp:308 (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_printf.cpp:308)
>>>               sanitizer_printf.cpp.o:(__sanitizer::SharedPrintfCode(bool, char const*, __va_list_tag*)) in archive libRTSanitizerCommon.test.nolibc.x86_64.a
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
[5/90] Linking CXX static library lib/libllvm_gtest.a
ninja: build stopped: subcommand failed.

Looks like @smeenai was working on a fix-forward in https://reviews.llvm.org/D147164. Those should be either merged and submitted together or landed together. Thanks :)

Looks like @smeenai was working on a fix-forward in https://reviews.llvm.org/D147164. Those should be either merged and submitted together or landed together. Thanks :)

I've noticed the failures, but I couldn't relate it to this change. Thank you for clarifying this.
Please revert the patch. It will take some time before I learn how to properly do it.

smeenai added a comment.EditedMar 29 2023, 9:44 AM

Looks like @smeenai was working on a fix-forward in https://reviews.llvm.org/D147164. Those should be either merged and submitted together or landed together. Thanks :)

I've noticed the failures, but I couldn't relate it to this change. Thank you for clarifying this.
Please revert the patch. It will take some time before I learn how to properly do it.

Feel free to include the changes from D147164 as part of this patch, which should fix the errors.

Feel free to include the changes from D147164 as part of this patch, which should fix the errors.

Unfortunately, I won't have time to do this this week.
I think it makes sense to merge this change into D147164, at least for the -ftree-auto-var-init part.

barannikov88 reopened this revision.Apr 1 2023, 4:26 AM
This revision is now accepted and ready to land.Apr 1 2023, 4:26 AM

Drop changes to -ftrivial-auto-var-init causing build failures

Mention "Reland" in the commit title

This now breaks M68k build https://lab.llvm.org/buildbot#builders/192/builds/1022

cc1: error: -Wformat-security ignored without -Wformat [-Werror=format-security]

It appears there is a typo in the use of COMPILER_RT_HAS_BUILTIN_FORMAT_SECURITY_FLAG:
append_list_if(COMPILER_RT_HAS_BUILTIN_FORMAL_SECURITY_FLAG -Werror=format-security flags)
FORMAL vs FORMAT.

barannikov88 reopened this revision.Apr 1 2023, 8:24 AM

I added -Werror because the changes I made in https://reviews.llvm.org/D131714 broke someone's internal build (see @wlei's comment). Apparently, gcc emits a warning if you try to use -Wformat-security without passing -Wformat too.

-Werror is probably only needed for -Wformat-security, but I think passing it in other places doesn't do any harm.

OK, I'm going to drop these -Werrors.
CMake is smart enough to detect if adding a flag causes a warning and conclude that the flag is unsupported.
It didn't work because of a typo in the variable name.

This revision is now accepted and ready to land.Apr 1 2023, 8:24 AM

Remove unnecessary -Werror.

barannikov88 retitled this revision from [compiler-rt] Quote multiple warning flags in check_cxx_compiler_flag invocation to [compiler-rt] Drop -Werror in compiler flag checks.Apr 1 2023, 8:42 AM
barannikov88 edited the summary of this revision. (Show Details)

Nah, it is a typo in both places...
I don't understand what's going on.

Fix the other typo

phosek added a comment.Apr 1 2023, 5:32 PM

Can you remove -Werror from the other checks in compiler-rt/cmake/config-ix.cmake as well?

Can you remove -Werror from the other checks in compiler-rt/cmake/config-ix.cmake as well?

I can try. Hopefully it will not break more bots.

Remove remaining -Werrors

Can you remove -Werror from the other checks in compiler-rt/cmake/config-ix.cmake as well?

I removed remaining occurrences of -Werror before -W flags and special-cased -Wformat-security.

I had to keep the flag before -m flags because clang may emit "argument unused during compilation" for those,
and this is something that CMake does not treat as a symptom of an unsupported option.

Fixing -ftrivial-auto-var-init=pattern case causes build bot failures; D147164 should address this.

Does it look fine now?

barannikov88 edited the summary of this revision. (Show Details)Apr 2 2023, 12:43 AM
phosek accepted this revision.Apr 2 2023, 10:10 PM

LGTM

barannikov88 edited the summary of this revision. (Show Details)

Rebase

Thanks to @nikic for figuring out the cause of the problem.

The reason the check passes is that it includes -Wall, which implies -Wformat. The actual compilation command doesn't include -Wall, so it fails.

barannikov88 planned changes to this revision.Apr 8 2023, 2:27 PM