Page MenuHomePhabricator

[Sanitizer][RISCV][AArch64][Android] Adjust allocator tests
ClosedPublic

Authored by luismarques on Feb 22 2021, 3:19 PM.

Details

Summary

On 64-bit systems with small VMAs (e.g. 39-bit) we can't use SizeClassAllocator64 parameterized with size class maps containing a large number of classes, as that will make the allocator region size too small (< 2^32). Several tests were already disabled for Android because of this.

This patch provides the correct allocator configuration for RISC-V (riscv64), generalizes the gating condition for tests that can't be enabled for small VMA systems, and tweaks the tests that can be made compatible with those systems to enable them.

I think the previous gating on Android should instead be AArch64+Android, so the patch reflects that, but feedback regarding the correct target gating is welcome.

I believe that the tests remaining gated can't be tweaked, as they specifically test allocators with incompatible size class maps (for small VMAs), so I deleted kostyak's comment to reflect that. Please let me know if I missed something there.

Diff Detail

Event Timeline

luismarques created this revision.Feb 22 2021, 3:19 PM
luismarques requested review of this revision.Feb 22 2021, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 3:19 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
luismarques edited the summary of this revision. (Show Details)Feb 22 2021, 3:23 PM
vitalybuka added inline comments.Feb 26 2021, 3:58 PM
compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp
1127

ALLOCATOR64_SMALL_SPACE ? 15 : 21

luismarques added inline comments.Feb 26 2021, 4:51 PM
compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp
1127

That would currently work but I don't think it's correct in general, so I think it would be misleading.
The class id here depends on the actual allocator size, which for other targets could be different from 0x2000000000ULL even if ALLOCATOR64_SMALL_SPACE == 1.
Your comment might indicate that my patch here is a bit misleading, though. I was originally going to go with a more verbose if (size X) Class2 = ... else if (size Y) Class2 = ... else abort, but it seemed verbose and it clashed with the const. Maybe I should reconsider that now.

vitalybuka added inline comments.Feb 26 2021, 5:26 PM
compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp
45

this name does not match how tests define kAllocatorSpace
If we rename ALLOCATOR64_SMALL_SPACE -> ALLOCATOR64_SMALL_SIZE it will be more consistent

1127

It's a test, and I would rather have logic as straightforward as possible.
I don't like that tests depends on some magical constant which can be changed.
It would be nice if select size for the platform, not calculate it indirectly.

ALLOCATOR64_SMALL_SPACE is good enough.
But renamed ALLOCATOR64_SMALL_SIZE will make it consistent.

Address review feedback.

luismarques marked 3 inline comments as done.Mar 8 2021, 7:37 AM
vitalybuka accepted this revision.Mar 10 2021, 10:35 PM
This revision is now accepted and ready to land.Mar 10 2021, 10:35 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 4:03 AM
This revision was automatically updated to reflect the committed changes.

Fix silly mistake in the last update.

luismarques reopened this revision.Mar 17 2021, 6:48 AM

Before relanding, I need to check/fix the SizeClassAllocator64PopulateFreeListOOM test for Android x86, as the respective buildbot failed in the first landing attempt.

This revision is now accepted and ready to land.Mar 17 2021, 6:48 AM

Rebased.
Tested with the sanitizer-x86_64-linux-android and sanitizer-windows buildbots, so this time there shouldn't be buildbot failures when committing.

This revision was landed with ongoing or failed builds.Mar 30 2021, 1:20 PM
This revision was automatically updated to reflect the committed changes.