This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Make tests work on Fuchsia
ClosedPublic

Authored by cryptoad on Nov 25 2019, 10:29 AM.

Details

Summary

This CL makes unit tests compatible with Fuchsia's zxtest. This
required a few changes here and there, but also unearthed some
incompatibilities that had to be addressed.

A header is introduced to allow to account for the zxtest/gtest
differences, some #if SCUDO_FUCHSIA are used to disable incompatible
code (the 32-bit primary, or the exclusive TSD).

It also brought to my attention that I was using
__scudo_default_options in different tests, which ended up in a
single binary, and I am not sure how that ever worked. So move
this to the main cpp.

Additionally fully disable the secondary freelist on Fuchsia as we do
not track VMOs for secondary allocations, so no release possible.

With some modifications to Scudo's BUILD.gn in Fuchsia:

[==========] 79 tests from 23 test cases ran (10280 ms total).
[  PASSED  ] 79 tests

Event Timeline

cryptoad created this revision.Nov 25 2019, 10:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 25 2019, 10:29 AM
Herald added subscribers: Restricted Project, jfb, srhines. · View Herald Transcript
cryptoad updated this revision to Diff 230930.Nov 25 2019, 10:30 AM

Adding allocator_config.h to the diff.

cryptoad updated this revision to Diff 230932.Nov 25 2019, 10:33 AM

Adding the LLVM header to scudo_unit_test.h.

Harbormaster completed remote builds in B41458: Diff 230930.
Harbormaster completed remote builds in B41460: Diff 230932.
hctim added inline comments.Nov 25 2019, 10:48 AM
compiler-rt/lib/scudo/standalone/secondary.h
126

I worry about adding ifdefs in core implementation files. Is it possibly a better idea to abstract the freelist away into a platform-specific implementation file? If so, you could also make this configurable at runtime...

cryptoad added inline comments.Nov 25 2019, 10:55 AM
compiler-rt/lib/scudo/standalone/secondary.h
126

So MaxFreeListSize is set to 0U in the Fuchsia config, which will make this go away, so the SCUDO_FUCHSIA is not needed.
I can change that to a COMPILER_CHECK.

hctim added inline comments.Nov 25 2019, 10:57 AM
compiler-rt/lib/scudo/standalone/secondary.h
126

SGTM!

cryptoad updated this revision to Diff 230951.Nov 25 2019, 11:28 AM
cryptoad marked 3 inline comments as done.

Remove the SCUDO_FUCHSIA conditionals from the code of the
Secondary and prefer a COMPILER_CHECK instead to ensure that
the freelist is disabled on Fuchsia.

hctim added inline comments.Nov 25 2019, 2:53 PM
compiler-rt/lib/scudo/standalone/secondary.h
51

Can we remove this ternary-guided default? Looks like it's being manually initialised everywhere.

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
161–163

Can we now roll this up?

#if SCUDO_FUCHSIA
  testAllocator<scudo::FuchsiaConfig>();
#else
  testAllocator<scudo::DefaultConfig>();
#endif
217

Same as here

cryptoad marked 3 inline comments as done.Nov 25 2019, 3:06 PM
cryptoad added inline comments.
compiler-rt/lib/scudo/standalone/secondary.h
51

Stuff like ScudoSecondaryTest, SecondaryBasic use an default templated allocator.
I am open to gating those or leaving the ternary, as you think is better.

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
161–163

Ack

217

Ack

hctim added inline comments.Nov 25 2019, 3:27 PM
compiler-rt/lib/scudo/standalone/secondary.h
51

Personally, I am in favour of the gating of the tests rather than the ternary default.

cryptoad updated this revision to Diff 231077.Nov 26 2019, 8:27 AM
cryptoad marked 4 inline comments as done.

Addressing Mitch's review comments, wrapping things better in #if
and removing the ternary in the default secondary template parameter.

hctim accepted this revision.Nov 26 2019, 10:10 AM
This revision is now accepted and ready to land.Nov 26 2019, 10:10 AM
This revision was automatically updated to reflect the committed changes.