This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Don't try to unmap unaligned memory
ClosedPublic

Authored by ro on Nov 17 2020, 4:35 AM.

Details

Summary

Enabling sanitizer_common tests on Solaris (D91606) and SPARC (D91608) uncovered a sparcv9 failure

SanitizerCommon-Unit :: ./Sanitizer-sparcv9-Test/CompactRingBuffer.int64

like this:

[ RUN      ] CompactRingBuffer.int64
==24576==ERROR: SanitizerTool failed to deallocate 0x2000 (8192) bytes at address 0xffffffff7f59b000
==24576==Sanitizer CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp:61 (("unable to unmap" && 0)) != (0) (0, 0)

The problemis that the original allocation via MmapAlignedOrDieOnFatalError is for 4 kB, but the Solaris/sparcv9 pagesize is 8 kB. So the initial allocation is for 12 kB, rounded to a multiple of the pagesize. Afterwards, the unneeded rest is unmapped again, but this fails since the address is not pagesize-aligned.

This patch avoids this by only unmapping correctly aligned memory.

The patch still FAILs, but for a different reason, and this part of the issue is generic.

Diff Detail

Event Timeline

ro created this revision.Nov 17 2020, 4:35 AM
Herald added subscribers: Restricted Project, fedor.sergeev, jyknight. · View Herald TranscriptNov 17 2020, 4:35 AM
ro requested review of this revision.Nov 17 2020, 4:35 AM
vitalybuka added inline comments.Nov 17 2020, 4:35 PM
compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
95

map_res is just from MMap, it must be aligned?

99

probably better to align end up to the pagesize

vitalybuka requested changes to this revision.Feb 17 2021, 1:54 PM
This revision now requires changes to proceed.Feb 17 2021, 1:54 PM
ro added a comment.Feb 4 2022, 3:57 AM

Sorry for dropping the ball on this one: the revised patch is trivial. I've now tested it (together with D91827 which is necessary to make the test PASS) on sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.

compiler-rt/lib/sanitizer_common/sanitizer_posix.cpp
95

It is indeed: this snippet is unnecessary.

99

Right, patch amended accordingly.

ro updated this revision to Diff 405916.Feb 4 2022, 3:57 AM

Incorporate review comments.

vitalybuka accepted this revision.Feb 8 2022, 12:55 PM
This revision is now accepted and ready to land.Feb 8 2022, 12:55 PM
This revision was automatically updated to reflect the committed changes.