This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer][test] Don't hard-code page size in CompactRingBuffer.int64
AbandonedPublic

Authored by tangyouling on Nov 4 2022, 2:17 AM.

Details

Reviewers
kcc
eugenis
SixWeining
xen0n
xry111
MaskRay
XiaodongLoong
wangleiat
Group Reviewers
Restricted Project
Summary

The munmap syscall expects a page size aligned address, but the
CompactRingBuffer.int64 case expects a hard coded page size of 4KiB.
This breaks on e.g. LoongArch which defaults to a 16KiB page size:

$ ./build/runtimes/runtimes-bins/compiler-rt/lib/sanitizer_common/tests/Sanitizer-loongarch64-Test --gtest_filter=CompactRingBuffer.int64
Note: Google Test filter = CompactRingBuffer.int64
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from CompactRingBuffer
[ RUN      ] CompactRingBuffer.int64
==1395551==ERROR: SanitizerTool failed to deallocate 0xfffffffffffff000 (-4096) bytes at address 0x7ffff2a30000
SanitizerTool: CHECK failed: sanitizer_posix.cpp:63 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0) (tid=1395551)

Diff Detail

Event Timeline

tangyouling created this revision.Nov 4 2022, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 2:17 AM
tangyouling requested review of this revision.Nov 4 2022, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 2:17 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
xen0n retitled this revision from [sanitizer][test] Fix CompactRingBuffer.int64 munmap error on LoongArch to [sanitizer][test] Don't hard-code page size in CompactRingBuffer.int64.Nov 4 2022, 3:10 AM
xen0n edited the summary of this revision. (Show Details)
xen0n accepted this revision.Nov 4 2022, 3:16 AM
xen0n added a reviewer: Restricted Project.

LGTM but let's wait for the code owners.

Also I've reworded the patch summary (again) so it's not overly verbose and unnecessarily mentioning LoongArch (it's appropriate to refer to some test output, but the change itself isn't related to LoongArch at all).

Please also pay attention to the patch summary diff (you can do so by clicking the "Show Details" link alongside the edit action in the timeline) so you don't make the same/similar grammatical mistakes over and over again. I'm seriously thinking about composing a checklist of common Chinglish mistakes these days for lightening my proofreading workload...

Please also pay attention to the patch summary diff (you can do so by clicking the "Show Details" link alongside the edit action in the timeline) so you don't make the same/similar grammatical mistakes over and over again. I'm seriously thinking about composing a checklist of common Chinglish mistakes these days for lightening my proofreading workload...

I'm sorry for the heavy proofreading workload caused by my grammatical mistakes, thanks again!

eugenis requested changes to this revision.Nov 7 2022, 9:44 AM

As explained in D117635, kPageSizeBits is part of the compiler contract and must be a compile-time constant.

This revision now requires changes to proceed.Nov 7 2022, 9:44 AM
tangyouling abandoned this revision.Nov 7 2022, 5:02 PM