This is an archive of the discontinued LLVM Phabricator instance.

Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.
ClosedPublic

Authored by hctim on Nov 16 2022, 3:25 PM.

Details

Summary

Leaves the implementation and tests files in-place for right now, but
deletes the ability to build the old sanitizer-common based scudo. This
has been on life-support for a long time, and the newer scudo_standalone
is much better supported and maintained.

Also patches up some GWP-ASan wording, primarily related to the fact
that -fsanitize=scudo now is scudo_standalone, and therefore the way to
reference the GWP-ASan options through the environment variable has
changed.

Future follow-up patches will delete the original scudo, and migrate all
its tests over to be part of the scudo_standalone test suite.

Diff Detail

Event Timeline

hctim created this revision.Nov 16 2022, 3:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 3:25 PM
hctim requested review of this revision.Nov 16 2022, 3:25 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptNov 16 2022, 3:25 PM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits, MaskRay. · View Herald Transcript
vitalybuka accepted this revision.Nov 17 2022, 2:11 PM
vitalybuka added inline comments.
llvm/docs/GwpAsan.rst
174

how this related to the patch?

185

unrelated?

This revision is now accepted and ready to land.Nov 17 2022, 2:11 PM
hctim updated this revision to Diff 476241.Nov 17 2022, 2:22 PM
hctim marked 2 inline comments as done.

Remove unrelated diff from doc.

llvm/docs/GwpAsan.rst
174

unrelated, was a drive-by cleanup. i'll remove it from the diff.

Is the plan to eventually rename scudo_standalone to scudo once the santiizer-based Scudo implementation is deleted?

hctim added a comment.Nov 17 2022, 3:41 PM

Is the plan to eventually rename scudo_standalone to scudo once the santiizer-based Scudo implementation is deleted?

Ideally, yes (as well as make scudo_standalone not be in a subfolder). Practically, no, because of all the folks that depend on it (you@fuchsia, android, llvm-libc).

This revision was landed with ongoing or failed builds.Nov 22 2022, 12:08 PM
This revision was automatically updated to reflect the committed changes.

We're seeing a CMake error after this:

CMake Error at /data/users/smeenai/Source/tcdev/external/llvm-project/compiler-rt/cmake/Modules/AddCompilerRT.cmake:368 (add_library):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:RTGwpAsan.x86_64>

  Objects of target "RTGwpAsan.x86_64" referenced but no such target exists.

What appears to be happening is that COMPILER_RT_SCUDO_STANDALONE_BUILD_SHARED is ON by default, as is COMPILER_RT_BUILD_GWP_ASAN. However, gwp_asan isn't actually built unless it's also present in COMPILER_RT_SANITIZERS_TO_BUILD, so the combination of defaults is broken.

One option would be to add gwp_asan to COMPILER_RT_SANITIZERS_TO_BUILD if COMPILER_RT_BUILD_GWP_ASAN is ON, as in P8299. Another would be to make Scudo consider COMPILER_RT_SANITIZERS_TO_BUILD in addition to COMPILER_RT_HAS_GWP_ASAN before linking the GWP-ASan libraries. What are your thoughts here?

We (Linaro) also have an issue with a bot that uses -DCOMPILER_RT_BUILD_SANITIZERS=OFF.

https://lab.llvm.org/buildbot/#/builders/178/builds/3318

CMake Error at cmake/modules/AddLLVM.cmake:1915 (add_dependencies):
  The dependency target "ScudoUnitTests" of target "check-all" does not
  exist.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:1955 (add_lit_target)
  CMakeLists.txt:1249 (umbrella_lit_testsuite_end)
hans added a subscriber: hans.Nov 23 2022, 4:50 AM

Chromium is broken too

CMake Error at /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/cmake/modules/AddLLVM.cmake:1915 (add_dependencies):
  The dependency target "ScudoUnitTests" of target "check-runtimes" does not
  exist.
Call Stack (most recent call first):
  /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/cmake/modules/AddLLVM.cmake:1955 (add_lit_target)
  CMakeLists.txt:238 (umbrella_lit_testsuite_end)


CMake Error at /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/cmake/modules/AddLLVM.cmake:1915 (add_dependencies):
  The dependency target "ScudoUnitTests" of target "check-scudo_standalone"
  does not exist.
Call Stack (most recent call first):
  /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/cmake/modules/AddLLVM.cmake:1981 (add_lit_target)
  /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/test/scudo/standalone/CMakeLists.txt:15 (add_lit_testsuite)

from https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8796726895518599345/+/u/package_clang/stdout

Thanks for the detailed information folks. I'll probably fix it up and re-submit next week (after the US holidays) given that the blast radius seems to be bigger than I expected.

hctim reopened this revision.Nov 29 2022, 2:24 PM
This revision is now accepted and ready to land.Nov 29 2022, 2:24 PM
hctim updated this revision to Diff 479693.Dec 2 2022, 11:24 AM

Lots of places in scudo_standalone depend on GWP-ASan, and it seems far more reasonable to make COMPILER_RT_HAS_GWP_ASAN mean "is GWP-ASan actually going to be built", rather than checking three variables in all the places.

Give the same treatment to COMPILER_RT_HAS_SCUDO_STANDALONE.

This should fix the reviewer problems that were identified:

  1. scudo_standalone's cmake rules don't get generated when COMPILER_RT_BUILD_SANITIZERS=Off or COMPILER_RT_SANITIZERS_TO_BUILD does not contain scudo_standalone.
  2. check-scudo_standalone's cmake rules don't get generated when COMPILER_RT_INCLUDE_TESTS=Off.
vitalybuka accepted this revision.Dec 2 2022, 11:28 AM
This revision was landed with ongoing or failed builds.Dec 2 2022, 11:30 AM
This revision was automatically updated to reflect the committed changes.
compiler-rt/test/CMakeLists.txt