This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Align & pad the allocator structures to the cacheline size
ClosedPublic

Authored by cryptoad on Mar 8 2018, 10:26 AM.

Details

Summary

Both SizeClassInfo structures for the 32-bit primary & RegionInfo
structures for the 64-bit primary can be used by different threads, and as such
they should be aligned & padded to the cacheline size to avoid false sharing.
The former was padded but the array was not aligned, the latter was not padded
but we lucked up as the size of the structure was 192 bytes, and aligned by
the properties of mmap.

I plan on adding a couple of fields to the RegionInfo, and some highly
threaded tests pointed out that without proper padding & alignment, performance
was getting a hit - and it is going away with proper padding.

This patch makes sure that we are properly padded & aligned for both. I used
a template to avoid padding if the size is already a multiple of the cacheline
size. There might be a better way to do this, I am open to suggestions.

Event Timeline

cryptoad created this revision.Mar 8 2018, 10:26 AM
Herald added subscribers: Restricted Project, delcypher, kubamracek. · View Herald TranscriptMar 8 2018, 10:26 AM

I think just declaring "struct ALIGNED(kCacheLineSize) ..." is enough. Compiler should take take care of the rest, the structure size will be extended to the multiple of the alignment, the array will be aligned appropriately, right?

I think just declaring "struct ALIGNED(kCacheLineSize) ..." is enough. Compiler should take take care of the rest, the structure size will be extended to the multiple of the alignment, the array will be aligned appropriately, right?

So this would work with C++11's alignas but not with __attribute__((align())). Currently alignas is only used in LibFuzzer & XRay, I am not sure what the stance is about bringing it to sanitizer_common.

I think just declaring "struct ALIGNED(kCacheLineSize) ..." is enough. Compiler should take take care of the rest, the structure size will be extended to the multiple of the alignment, the array will be aligned appropriately, right?

So this would work with C++11's alignas but not with __attribute__((align())). Currently alignas is only used in LibFuzzer & XRay, I am not sure what the stance is about bringing it to sanitizer_common.

Nevermind, this seems to work!

cryptoad updated this revision to Diff 137627.Mar 8 2018, 11:44 AM

Using @alekseyshl's much simpler solution.
Added a couple of DCHECKs for good measure.
Also forgotten in the initial description: I changed 2 CHECKs to DCHECKs
as they are either called from functions that perform the CHECK or from for
loops that bound the value properly.

alekseyshl added inline comments.Mar 8 2018, 12:04 PM
lib/sanitizer_common/sanitizer_allocator_primary32.h
273–274

In other places we do "struct ALIGNED(N) TypeName { ...". Why after the struct body here?

368

The alignment of an array is the alignment of its elements, no need to specify it here.

cryptoad updated this revision to Diff 137633.Mar 8 2018, 12:11 PM
cryptoad marked an inline comment as done.

Addressing 1 comment.

cryptoad updated this revision to Diff 137635.Mar 8 2018, 12:18 PM
cryptoad marked an inline comment as done.

Addressing the other comment.

alekseyshl accepted this revision.Mar 8 2018, 1:08 PM
This revision is now accepted and ready to land.Mar 8 2018, 1:08 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 9 2018, 11:47 AM
thakis added inline comments.
compiler-rt/trunk/lib/sanitizer_common/sanitizer_allocator_primary32.h
269 ↗(On Diff #137758)

This doesn't build on our bots: https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FToTLinux%2F1811%2F%2B%2Frecipes%2Fsteps%2Fgclient_runhooks%2F0%2Fstdout

FAILED: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.i386.dir/sanitizer_allocator.cc.o
/b/c/b/ToTLinux/src/third_party/llvm-build-tools/gcc485precise/bin/g++ -DHAVE_RPC_XDR_H=0 -DHAVE_TIRPC_RPC_XDR_H=0 -I/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/.. -DLLVM_FORCE_HEAD_REVISION -Wall -std=c++11 -Wno-unused-parameter -O3 -DNDEBUG -DLLVM_FORCE_HEAD_REVISION -Wall -std=c++11 -Wno-unused-parameter -m32 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fvisibility=hidden -fno-lto -O3 -g -Wno-variadic-macros -Wno-non-virtual-dtor -fno-rtti -Wframe-larger-than=570 -MMD -MT lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.i386.dir/sanitizer_allocator.cc.o -MF lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.i386.dir/sanitizer_allocator.cc.o.d -o lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.i386.dir/sanitizer_allocator.cc.o -c /b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc
In file included from /b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.h:76:0,

from /b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:15:

/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h: In instantiation of ‘struct sanitizer::SizeClassAllocator32<sanitizer::AP32>::SizeClassInfo’:
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h:274:3: required from ‘class sanitizer::SizeClassAllocator32<sanitizer::AP32>’
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_combined.h:193:20: required from ‘class sanitizer::CombinedAllocator<sanitizer::SizeClassAllocator32<sanitizer::AP32>, sanitizer::SizeClassAllocatorLocalCache<sanitizer::SizeClassAllocator32<sanitizer::AP32> >, sanitizer::LargeMmapAllocator<sanitizer::NoOpMapUnmapCallback, __sanitizer::LargeMmapAllocatorPtrArrayStatic> >’
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:84:76: required from here
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h:269:34: error: requested alignment is not an integer constant

struct ALIGNED(kCacheLineSize) SizeClassInfo {
                               ^

In file included from /b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.h:17:0,

from /b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:15:

/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h: In instantiation of ‘class sanitizer::SizeClassAllocator32<sanitizer::AP32>’:
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_combined.h:193:20: required from ‘class sanitizer::CombinedAllocator<sanitizer::SizeClassAllocator32<sanitizer::AP32>, sanitizer::SizeClassAllocatorLocalCache<sanitizer::SizeClassAllocator32<sanitizer::AP32> >, sanitizer::LargeMmapAllocator<sanitizer::NoOpMapUnmapCallback, __sanitizer::LargeMmapAllocatorPtrArrayStatic> >’
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:84:76: required from here
/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:330:72: error: size of array is negative

typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
                                                                   ^

/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:324:30: note: in expansion of macro ‘IMPL_COMPILER_ASSERT’
#define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, LINE)

^

/b/c/b/ToTLinux/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary32.h:274:3: note: in expansion of macro ‘COMPILER_CHECK’

COMPILER_CHECK(sizeof(SizeClassInfo) % kCacheLineSize == 0);