This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][lsan] Share platform allocator settings between ASan and LSan
AbandonedPublic

Authored by leonardchan on Apr 14 2022, 2:25 PM.

Details

Summary

This attempts to reland D87795 which refactors the asan allocator to use lsan settings.

This also updates LSan on aarch64 to use the 64-bit allocator instead of the 32-bit one. This depends on D123814 landing first. Ideally, this does not affect which allocator is used for other platforms.

Diff Detail

Event Timeline

leonardchan created this revision.Apr 14 2022, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 2:25 PM
leonardchan requested review of this revision.Apr 14 2022, 2:25 PM
Herald added subscribers: Restricted Project, pcwang-thead. · View Herald TranscriptApr 14 2022, 2:25 PM
vitalybuka added inline comments.Apr 18 2022, 9:25 AM
compiler-rt/lib/asan/asan_allocator.h
129–142

i suspect these smaller values has something to do with ability allocate shadow, which is not an issue for lsan

leonardchan added inline comments.Apr 21 2022, 1:20 PM
compiler-rt/lib/asan/asan_allocator.h
129–142

Do you recommend I should remove them?

*ping* any more comments?

*ping* any more comments?

No objections, but some small concerns that it will be annoying to split them again if we need different setups for these sanitizers.

compiler-rt/lib/asan/asan_allocator.h
129–142

we can't just remove them for asan, because there probably was a reason
but now, with shared config, it's behavior change for lsan

compiler-rt/lib/lsan/lsan_common.h
80

SANITIZER_ARM64 and the rest?

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
252–257

this part looks a little bit unrelated, would it be better to have a separate patch?

Can you swticth to stuff like SANITIZER_ARM?

Also maybe it's better to have condition SANITIZER_WORDSIZE != 64 || the rest?

leonardchan added inline comments.Jun 1 2022, 2:11 PM
compiler-rt/lib/asan/asan_allocator.h
129–142

Oh I see, yeah perhaps we can instead just "unify" the values that are already matching in the asan and lsan configs rather than moving everything.

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
252–257

Uploaded https://reviews.llvm.org/D126825 which instead just tells lsan to use SANITIZER_CAN_USE_ALLOCATOR64 (which should just be a no-op). Then I'll have a followup patch that sets this macro to 1 for aarch64.

leonardchan added inline comments.Jun 2 2022, 3:22 PM
compiler-rt/lib/asan/asan_allocator.h
129–142

D126927 should address this.

vitalybuka requested changes to this revision.Dec 7 2022, 1:41 PM

I suspect it's abandoned

This revision now requires changes to proceed.Dec 7 2022, 1:41 PM
leonardchan abandoned this revision.Dec 7 2022, 3:22 PM

I suspect it's abandoned

Yeah D126825 and D126927 I believe address what this changed intended to do.