Page MenuHomePhabricator

[lsan] Share platform allocator settings between ASan and LSan
Needs ReviewPublic

Authored by mcgrathr on Wed, Sep 16, 2:14 PM.

Details

Summary

This moves the platform-specific parameter logic from asan into
lsan_common.h to lsan can share it.

Diff Detail

Event Timeline

mcgrathr created this revision.Wed, Sep 16, 2:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 16, 2:14 PM
Herald added subscribers: Restricted Project, fedor.sergeev. · View Herald Transcript
mcgrathr requested review of this revision.Wed, Sep 16, 2:14 PM

can you please clang format this?

can you please clang format this?

I had avoided it because I was moving existing code verbatim. But I'll do it now.

vitalybuka added inline comments.Wed, Sep 16, 6:28 PM
compiler-rt/lib/sanitizer_common/sanitizer_allocator.h
73 ↗(On Diff #292339)

sanitizer allocator is used by msan, scudo, hwasan and upcoming D87120
but this constants make sense only for lsan/asan, so it does not look like a right place for them

However asan can #include lsan
right now it's only #include "lsan/lsan_common.h"
so maybe move there or create another header?

can you please clang format this?

I had avoided it because I was moving existing code verbatim. But I'll do it now.

I think verbatim is no worth of fighting clang-format and clang-tidy

mcgrathr updated this revision to Diff 293614.Tue, Sep 22, 6:36 PM

Moved to lsan_common.h.

mcgrathr updated this revision to Diff 293615.Tue, Sep 22, 6:36 PM
mcgrathr edited the summary of this revision. (Show Details)

update

I redid this to put it in the __lsan namespace in lsan_common.h, which is definitely simpler all around.

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_allocator.h
73 ↗(On Diff #292339)

Good thought. I changed it like that, and it seems cleaner now.

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

Thank you,
Filipe

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

Please don’t do this. You’re adding a dependency between asan and lsan, and
making it impossible to support platforms without lsan.

It’s a layering violation. I guess you can hack it if you add a *very
minimal * “allocator for asan and/or lsan” in sanitizer common. That
wouldn’t be ideal (a header shared between only two Sanitizers, but at
least it would be in the right place.

The source dependency already exists throughout the asan code, which unconditionally includes lsan headers.
There is no change to the fact that systems without lsan support work fine. It's already the case that #if CAN_SANITIZE_LEAKS is checked throughout the asan code to control whether it assumes lsan support.
This change does not any new interdependencies. The only reliance on lsan is that it's in the compiler-rt source tree, as before.

Please don’t do this. You’re adding a dependency between asan and lsan, and

This dependency already exists in asan_allocator.cpp

Also I think that duplication of these constants is not rely a problems and it's fine as-is without the patch.

The purpose of the change is to fix the lsan allocator configuration, which is practically unusable for aarch64.
It was clear that nobody had really maintained the lsan configurations and that the asan configurations were better-maintained and the proven settings for each platform. So everyone agreed that sharing the same settings between asan and lsan was the right thing to do. It seems very error-prone for future maintenance if this is done by copying the code. That's what happened before, and then someone changed the asan configuration settings to actually work well on more platforms and neglected to update the lsan configurations. We know that will happen again next time there is a need to change the asan configurations, so it seems unwise to merely update the copy rather than share the code, especially when it's so trivial to do so and there are no new interdependencies actually introduced.

it's LGTM as-is

@filcab CAN_SANITIZE_LEAKS is the one which controls if asan uses lsan and it's already in this header. So it's already can be considered layering violation and we not making it worse. Do we miss something?

Yeah. I looked at it again and looks ok. I missed that we were already
including the file. As long as we can support lsan-less asan, we’re cool.

Thank you,
Filipe

Since everyone approves can someone mark it approved?