Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[scudo][standalone] Use CheckAtomic to decide to link to libatomic
Needs RevisionPublic

Authored by benwolsieffer on Dec 10 2022, 7:12 PM.



Standalone scudo uses the atomic operation builtin functions, which require
linking to libatomic on some platforms. Currently, this is done in an ad-hoc
manner. MIPS platforms always link to libatomic, and the tests are always linked
to it as well. libatomic is required on base ARMv6 (but not ARMv6K), but it is
currently not linked, causing the build to fail.

This patch replaces this ad-hoc logic with the CheckAtomic CMake module already
used in other parts of LLVM. The CheckAtomic module checks whether std::atomic
requires libatomic, which is not strictly the same as checking the atomic
builtins, but should have the same results as far as I know. If this is
problematic, a custom version of CheckAtomic could be used to specifically test
the builtins.

Diff Detail

Event Timeline

benwolsieffer created this revision.Dec 10 2022, 7:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2022, 7:12 PM
benwolsieffer requested review of this revision.Dec 10 2022, 7:12 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 10 2022, 7:12 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Note that I have not tested this exact version of the patch. My testing so far has been done with a backported version with LLVM 14.

I would appreciate feedback about whether compiler-rt should have its own version of CheckAtomic.cmake (that specifically checks for builtins) or whether using the LLVM one is good enough.

vitalybuka requested changes to this revision.Aug 27 2023, 10:16 PM

Is this still relevant?

This revision now requires changes to proceed.Aug 27 2023, 10:16 PM

Rebased on main

Yes, this is still needed along with and to build compiler-rt on plain ARMv6 (not ARMv6k).

Update diff with arcanist to provide full context. I was previously having issues with arcanist.

vitalybuka accepted this revision.Sep 22 2023, 6:01 PM
This revision is now accepted and ready to land.Sep 22 2023, 6:01 PM
vitalybuka requested changes to this revision.Sep 22 2023, 6:03 PM
This revision now requires changes to proceed.Sep 22 2023, 6:03 PM

ninja -C ../out/t
-- Performing Test HAVE_CXX_ATOMICS_WITH_LIB - Failed
CMake Error at llvm-project/llvm/cmake/modules/CheckAtomic.cmake:57 (message):
  Host compiler must support std::atomic!
Call Stack (most recent call first):
  llvm-project/compiler-rt/lib/scudo/standalone/CMakeLists.txt:3 (include)

cat out/t/runtimes/runtimes-bins/CMakeFiles/CMakeError.log

out/t/./bin/clang++ --target=x86_64-unknown-linux-gnu -DHAVE_CXX_ATOMICS_WITH_LIB  -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter  --start-no-unused-arguments --unwindlib=none --end-no-unused-arguments -nostdlib++ -nostdinc++ -nodefaultlibs -std=c++11 -std=c++17 -MD -MT CMakeFiles/cmTC_6228f.dir/src.cxx.o -MF CMakeFiles/cmTC_6228f.dir/src.cxx.o.d -o CMakeFiles/cmTC_6228f.dir/src.cxx.o -c out/t/runtimes/runtimes-bins/CMakeFiles/CMakeScratch/TryCompile-1c0GZ3/src.cxx
out/t/runtimes/runtimes-bins/CMakeFiles/CMakeScratch/TryCompile-1c0GZ3/src.cxx:2:10: fatal error: 'atomic' file not found
    2 | #include <atomic>
      |          ^~~~~~~~
1 error generated.