This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common][test] Disable CombinedAllocator32Compact etc. on Solaris/sparcv9
ClosedPublic

Authored by ro on Nov 17 2020, 5:12 AM.

Details

Summary

As reported in PR 48202, two allocator tests FAIL on Solaris/sparcv9, presumably because Solaris uses the full 64-bit address space and the allocator cannot deal with that:

SanitizerCommon-Unit :: ./Sanitizer-sparcv9-Test/SanitizerCommon.CombinedAllocator32Compact
SanitizerCommon-Unit :: ./Sanitizer-sparcv9-Test/SanitizerCommon.SizeClassAllocator32Iteration

This patch disables the tests.

Tested on sparcv9-sun-solaris2.11.

Diff Detail

Event Timeline

ro created this revision.Nov 17 2020, 5:12 AM
Herald added subscribers: Restricted Project, fedor.sergeev, jyknight. · View Herald TranscriptNov 17 2020, 5:12 AM
ro requested review of this revision.Nov 17 2020, 5:12 AM

Have you tried to change kAddressSpaceSize above?

compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp
948

Better approach is to use:

#if defined(_MSC_VER) && defined(_DLL)
#define MAYBE_StrDupOOBTest DISABLED_StrDupOOBTest
#else
#define MAYBE_StrDupOOBTest StrDupOOBTest
#endif

TEST(AddressSanitizer, MAYBE_StrDupOOBTest) {

this way the test at least compiles on the platform.

But it's going to make the file inconsistent.
So you like you can update the file with MAYBE_ in a separate patch.
But feel free to keep as-is.

ro added a comment.Nov 18 2020, 5:37 AM

Have you tried to change kAddressSpaceSize above?

Not yet: TBH I was quite confused by how target-specific configuration was sprinkled over both the allocator implementation (where I'd expect it) and the allocator test (where I wouldn't) and how the two interact.

I now gave it a try:

  • set SANITIZER_MMAP_RANGE_SIZE for Solaris/sparcv9 to FIRST_32_SECOND_64(1ULL << 32, 0xFFFFFFFFFFFFFFFF)
  • keep SANITIZER_SIGN_EXTENDED_ADDRESSES as 0 for Solaris/sparcv9 (IIUC this is only needed/correct on Linux/sparc64 which only uses a low part of the 64-bit address space albeit with the VA hole in the middle just like Solaris)
  • set kAddressSpaceSize, keeping the rest as in the default case:
+#elif SANITIZER_SOLARIS && defined(__sparcv9)
+static const uptr kAllocatorSpace = 0x700000000000ULL;
+static const uptr kAllocatorSize  = 0x010000000000ULL;  // 1T.
+static const u64 kAddressSpaceSize = 0xFFFFFFFFFFFFFFFF;
+typedef DefaultSizeClassMap SizeClassMap;

While this allowed Sanitizer-sparcv9-test to compile, both affected tests still FAIL, though in a different way:

  [ RUN      ] SanitizerCommon.SizeClassAllocator32Compact
==15161==Sanitizer CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/sanitizer_common/sanitizer_allocator_bytemap.h:69 ((idx)) < ((kSize1 * kSize2)) (1099511627568, 1099511623680)

[ RUN      ] SanitizerCommon.SizeClassAllocator32Iteration
==15162==Sanitizer CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/sanitizer_common/sanitizer_allocator_bytemap.h:69 ((idx)) < ((kSize1 * kSize2)) (1099511627644, 1099511623680)
compiler-rt/lib/sanitizer_common/tests/sanitizer_allocator_test.cpp
948

While I like this approach (I'm effectively doing the same with ASan on SPARC: keep it compiling in compiler-rt for the benefit of gcc where it works, despite being unusable with clang due to a compiler bug/limitation).

However, that's a considerable change and I'm running out of time for LLVM work (GCC just entered stage3 for the GCC 11 release and I'll soon will have to switch attention there), so I'd like to keep my patch as is at least for the moment.

vitalybuka accepted this revision.Nov 18 2020, 11:37 PM

LGTM with some FIXME explaining that it needs investigation in future

This revision is now accepted and ready to land.Nov 18 2020, 11:37 PM
ro updated this revision to Diff 306651.Nov 20 2020, 3:59 AM
ro edited the summary of this revision. (Show Details)
  • Switch disablement to SKIP_ON_* form.
  • Add FIXME comment.
This revision was landed with ongoing or failed builds.Nov 20 2020, 4:02 AM
This revision was automatically updated to reflect the committed changes.