Page MenuHomePhabricator

[scudo] Initial standalone skeleton check-in
ClosedPublic

Authored by cryptoad on Jan 29 2019, 12:40 PM.

Details

Summary

This is the initial check-in for the Standalone version of Scudo.

The project is initially going to live in scudo/standalone then will
replace scudo. See http://lists.llvm.org/pipermail/llvm-dev/2019-January/129113.html
for details.

This initial CL is meant to lay out the project structure, of both
code & tests, providing a minimal amount of functionalities, namely
various definitions, some atomic helpers and an intrusive list.
(empty.cc is just here to have a compilation unit, but will go away
in the upcoming CLs).

Initial support is restricted to Linux i386 & x86_64 in make files
and will be extended once things land & work.

We will grow organically from here, adding functionalities in limited
amounts.

Diff Detail

Event Timeline

cryptoad created this revision.Jan 29 2019, 12:40 PM
Herald added subscribers: Restricted Project, jfb, delcypher and 3 others. · View Herald TranscriptJan 29 2019, 12:40 PM

I have got a scudo support patch for NetBSD locally.. but the original tests are havily prepared against GNU malloc. This makes me uncertain whether scudo really works. Are there plans to make the tests more generic?

I have got a scudo support patch for NetBSD locally.. but the original tests are havily prepared against GNU malloc. This makes me uncertain whether scudo really works. Are there plans to make the tests more generic?

That should be the case for the unit-tests. The new way things are done will clearly separate the C/C++ wrappers.
I can add you on the standalone check-ins for you to chime in on the direction we are headed to.
All the development efforts so far has targeted Linux/Android/Fuchsia, and I unfortunately have no BSD knowledge, but we are definitely open to contributions.
Initial check-ins will be Linux specific though since it's my workstation and that's what I am testing things against.

I wouldn't pursue work on your side on the current non-standalone Scudo, as the standalone version will be better (more performant, smaller memory footprint, etc) on all fronts.

I have got a scudo support patch for NetBSD locally.. but the original tests are havily prepared against GNU malloc. This makes me uncertain whether scudo really works. Are there plans to make the tests more generic?

That should be the case for the unit-tests. The new way things are done will clearly separate the C/C++ wrappers.
I can add you on the standalone check-ins for you to chime in on the direction we are headed to.
All the development efforts so far has targeted Linux/Android/Fuchsia, and I unfortunately have no BSD knowledge, but we are definitely open to contributions.
Initial check-ins will be Linux specific though since it's my workstation and that's what I am testing things against.

I wouldn't pursue work on your side on the current non-standalone Scudo, as the standalone version will be better (more performant, smaller memory footprint, etc) on all fronts.

Please keep tests agnostic to system malloc implementation (as much as possible). I will rebase or rework my NetBSD support code and upstream. When I did the port I was unsure whether it even works, but the situation maybe improved since the introduction of Fuchsia.

morehouse added inline comments.Jan 31 2019, 2:08 PM
lib/scudo/standalone/atomic_helpers.h
54

necessarily*

64

Have we lost the memory_order checking that sanitizer_atomic_clang_x86.h does, or is this done in __atomic_load?

99

Can we simplify to return __atomic_exchange(&A->ValDoNotUse, &V, MO) instead?

lib/scudo/standalone/list.h
149

Any reason not to put a private: here?

lib/scudo/standalone/tests/CMakeLists.txt
37

Will this be uncommented eventually?

cryptoad marked 5 inline comments as done.Jan 31 2019, 2:25 PM
cryptoad added inline comments.
lib/scudo/standalone/atomic_helpers.h
64

I looked around (non-exhaustively) and found that it was checked by isValidOrderingForOp in Clang, so I figured it could go away.
Just checked gcc and it has checks in https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L6434.
Let me know if you feel it's reasonable to not have them.

99

As far as I can tell __atomic_exchange has a void return type.
It's worth noting that __atomic_exchange_n returns type but it doesn't seem to be widely used (see libcxx's __c11_atomic_exchange for example).

lib/scudo/standalone/list.h
149

Good question, I think this was the case in the sanitizer_common code and I didn't question it.
I will check if that can actually be private.

lib/scudo/standalone/tests/CMakeLists.txt
37

It might, or not!
I'm not awesome at this CMake/lit thing and I feel it could come back and bit me if I remove it and forgot it was useful at some point.
But I'm Ok removing it.

cryptoad updated this revision to Diff 184611.Jan 31 2019, 2:34 PM
  • s/necessary/necessarily
  • actually make the private members of the list class private
cryptoad updated this revision to Diff 184615.Jan 31 2019, 2:38 PM
  • remove commented RUNTIME ${SCUDO_TEST_RUNTIME} in tests CMake
morehouse accepted this revision.Jan 31 2019, 2:55 PM
morehouse added inline comments.
lib/scudo/standalone/atomic_helpers.h
64

In sanitizer_common, these are DCHECKs so not critical. And since we're just wrapping the builtins, I think we can delegate the checking to them.

99

Hm, right I see that now in the gcc documentation.

I was looking at this list, which might not be the right reference.

lib/scudo/standalone/tests/CMakeLists.txt
37

My guess is that you'll need it once you add some .cpp files. I'm fine either leaving the commented line or not.

This revision is now accepted and ready to land.Jan 31 2019, 2:55 PM
vitalybuka accepted this revision.Jan 31 2019, 11:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 31 2019, 11:30 PM
This revision was automatically updated to reflect the committed changes.

Build with -DLLVM_ENABLE_LIBCXX=ON is broken because:

clang-9: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]

Build with -DLLVM_ENABLE_LIBCXX=ON is broken because:

clang-9: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]

Ack, I'll fix that.

Build with -DLLVM_ENABLE_LIBCXX=ON is broken because:

clang-9: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]

What are your full cmake options? I can't seem to reproduce this with just -DLLVM_ENABLE_LIBCXX=ON

Repro'd the reported error with:

-DLLVM_ENABLE_LIBCXX=ON \
-DLLVM_ENABLE_LIBCXXABI=On \
-DCLANG_DEFAULT_CXX_STDLIB=libc++ \
-DCMAKE_CXX_FLAGS="-stdlib=libc++" \

Fix in D57757.