This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Introduce the combined allocator
ClosedPublic

Authored by cryptoad on Jun 12 2019, 3:29 PM.

Details

Summary

The Combined allocator hold together all the other components, and
provides a memory allocator interface based on various template
parameters. This will be in turn used by "wrappers" that will provide
the standard C and C++ memory allocation functions, but can be
used as is as well.

This doesn't depart significantly from the current Scudo implementation
except for a few details:

  • Quarantine batches are now protected by a header a well;
  • an Allocator instance has its own TSD registry, as opposed to a static one for everybody;
  • a function to iterate over busy chunks has been added, for Android purposes;

This also adds the associated tests, and a few default configurations
for several platforms, that will likely be further tuned later on.

Diff Detail

Repository
rL LLVM

Event Timeline

cryptoad created this revision.Jun 12 2019, 3:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 12 2019, 3:29 PM
Herald added subscribers: Restricted Project, jfb, delcypher and 2 others. · View Herald Transcript
cryptoad updated this revision to Diff 204566.Jun 13 2019, 9:44 AM

Enabled the Quarantine for some tests, which wasn't used before.

cryptoad updated this revision to Diff 204570.Jun 13 2019, 10:00 AM

Tune some tests & Quarantine parameters.

morehouse accepted this revision.Jun 14 2019, 1:18 PM
morehouse added inline comments.
lib/scudo/standalone/allocator.h
1 ↗(On Diff #204570)

Why call this file allocator.h? allocator_config.h seems more accurate.

lib/scudo/standalone/combined.h
114 ↗(On Diff #204570)

Is this reinterpret_cast required?

lib/scudo/standalone/tests/combined_test.cc
73 ↗(On Diff #204570)

Is this test flaky at all?

133 ↗(On Diff #204570)

What about FuchsiaConfig?

This revision is now accepted and ready to land.Jun 14 2019, 1:18 PM
cryptoad marked 5 inline comments as done.Jun 14 2019, 3:33 PM
cryptoad added inline comments.
lib/scudo/standalone/allocator.h
1 ↗(On Diff #204570)

Renamed to allocator_config.h and changed header and #if guards accordingly. Updated the CMake file.

lib/scudo/standalone/combined.h
114 ↗(On Diff #204570)

So indeed, I tried on a few platforms and it appears not to be.
At this point I am a bit wary about extra warning inducing compiler flags, and I am actually not certain if one would trip on that.
I am open to removing it if that feels safe "enough". WDYT?

lib/scudo/standalone/tests/combined_test.cc
73 ↗(On Diff #204570)

Currently no, the number of iterations & size chosen are large enough with a small quarantine to ensure that recycling will happen and that we will end up reusing the chunk. This is mostly experimentally determined. This could change in the future if more randomization is introduced or concepts of "never released blocks" or things like that are introduced.

133 ↗(On Diff #204570)

Added FuchsiaConfig for 64-bit tests. Replaced Config with DefaultConfig to cover them all.

cryptoad updated this revision to Diff 204866.Jun 14 2019, 3:37 PM
cryptoad marked 2 inline comments as done.

Addressing review comments:

  • renaming allocator.h to allocator_config.h
  • adding FuchsiaConfig to the battery of tests (64-bit only)
morehouse added inline comments.Jun 14 2019, 3:45 PM
lib/scudo/standalone/combined.h
114 ↗(On Diff #204570)

I think it's pretty common to not worry about casting pointers to void*. For example, Scudo already does this on every memset(this, ...).

cryptoad updated this revision to Diff 205078.Jun 17 2019, 8:18 AM
cryptoad marked 3 inline comments as done.

Removing a reinterpret_cast<void *>.

This revision was automatically updated to reflect the committed changes.