This is an archive of the discontinued LLVM Phabricator instance.

Introduce `AddressSpaceView` template parameter to `SizeClassAllocator32`, `FlatByteMap`, and `TwoLevelByteMap`.
ClosedPublic

Authored by delcypher on Nov 26 2018, 8:56 AM.

Details

Summary

This is a follow up patch to r346956 for the SizeClassAllocator32
allocator.

This patch makes AddressSpaceView a template parameter both to the
ByteMap implementations, AP32 implementations and is used in
SizeClassAllocator32. The actual changes to ByteMap implementations
and SizeClassAllocator32 are very simple. However the patch is large
because it requires changing all the AP32 definitions, and users of
those definitions.

Note that the reason for not making LocalAddressSpaceView the default
template parameter for the various templates is because that would
require all users to add empty braces (i.e. PrimaryAllocator<> instead
of PrimaryAllocator to instantiate). To avoid this we create a
template version with a T suffix (e.g. PrimaryAllocatorT) and then
create an alias that has the previous type name (e.g.
PrimaryAllocator) that instantiates with LocalAddressSpaceView (e.g.
PrimaryAllocatorT<LocalAddressSpaceView>).

In order to check that template is instantiated in the correct a way a
static_assert(...) has been added that checks that the
AddressSpaceView type used by Params::ByteMap::AddressSpaceView matches
the Params::AddressSpaceView. This uses the new sanitizer_type_traits.h
header.

rdar://problem/45284065

Event Timeline

delcypher created this revision.Nov 26 2018, 8:56 AM
Herald added subscribers: Restricted Project, llvm-commits, mgorny. · View Herald TranscriptNov 26 2018, 8:56 AM

The patch is quite big. I've added a couple suggestions how to reduce it to make it easier to review.

lib/sanitizer_common/sanitizer_allocator_bytemap.h
18

Is this necessary? FlatByteMap isn't doing any dereferences besides reading it's own fields.

81

Avoid reformatting code we don't need to change.

lib/sanitizer_common/sanitizer_allocator_internal.h
50

Can we unify the naming convention of adding "T" and "Ty" suffix to types? Do we need both?

63

Avoid reformatting code we don't need to change.

lib/sanitizer_common/sanitizer_allocator_primary32.h
243–247

This looks like an optimization. Can we remove it from the patch?

lib/sanitizer_common/sanitizer_type_traits.h
36–40 ↗(On Diff #175280)

This (sanitizer_type_traits.h, the test file and the static_assert) can be moved to a separate patch.

delcypher marked 4 inline comments as done.Nov 27 2018, 7:41 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_allocator_bytemap.h
18

It is necessary for this implementation. This is because SizeClassAllocator32 contains a static_assert that checks that Params::ByteMap::AddressSpaceView is the same type as the Params::AddressSpaceView.
In order for this to work all ByteMap implementations need to expose the AddressSpaceView type.

This patch currently does this to ensure that the AddressSpaceView type is consistent everywhere.

There is a way to avoid the static_assert by making AP32::ByteMap be a templated type (i.e. not a concrete type like FlatByteMap<..., LocalAddressSpaceView>) and instead be a type that takes a single AddressSpaceView template argument. Then inside SizeClassAllocator32 the allocator would need to fill in the AddressSpaceView argument. I.e. replace

typedef typename Params::ByteMap ByteMap;

with

using ByteMap = Params::ByteMap<Params::AddressSpaceView>;

This still requires that FlatByteMap still take an AddressSpaceView template parameter, even though it isn't really used because all ByteMap implementations need to accept the same template parameters.

81

I could fix this but please note this is just clang-format doing its thing. Surely we want to clang-format all our patches?

lib/sanitizer_common/sanitizer_allocator_internal.h
50

Sure they can be unified. However before I go for a mass rename, do we want a different name? The T suffix was a placeholder for until I came up with a better name. Given that these types always take a single template parameter that is an AddressSpaceView make the suffix should reflect that. For example we could make the suffix be ASVT (Address Space View Type) rather than T. What do you think?

lib/sanitizer_common/sanitizer_allocator_primary32.h
243–247

I can. It is more than just an optimization. It's also there for clarity. Previously it was not obvious that doing possible_regions[region] gives you a class id.

lib/sanitizer_common/sanitizer_type_traits.h
36–40 ↗(On Diff #175280)

Sure. I'll try to do this.

delcypher marked an inline comment as done.Nov 27 2018, 8:10 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_type_traits.h
36–40 ↗(On Diff #175280)

Okay I've split this out into https://reviews.llvm.org/D54951 . I'll update this patch shortly.

delcypher updated this revision to Diff 175487.Nov 27 2018, 8:13 AM

Remove sanitize_type_traits from this patch and put in https://reviews.llvm.org/D54951

delcypher updated this revision to Diff 175491.Nov 27 2018, 8:17 AM

Try again.

delcypher edited the summary of this revision. (Show Details)Nov 27 2018, 8:20 AM

The patch is quite big. I've added a couple suggestions how to reduce it to make it easier to review.

lib/sanitizer_common/sanitizer_allocator.h
25

Also remove.

lib/sanitizer_common/sanitizer_allocator_primary32.h
112–114

Also remove.

kubamracek added a comment.EditedNov 27 2018, 8:32 AM

Please make this patch independent on the other (type_traits).

delcypher updated this revision to Diff 175498.Nov 27 2018, 8:48 AM
delcypher edited the summary of this revision. (Show Details)

Remove some unnecessary formatting changes.

delcypher updated this revision to Diff 175499.Nov 27 2018, 8:49 AM

Replace typedef with using for consistency.

delcypher updated this revision to Diff 175500.Nov 27 2018, 8:53 AM

Remove tweak in ForEachChunk(...).

delcypher marked 3 inline comments as done.Nov 27 2018, 8:54 AM

Please make this patch independent on the other (type_traits).

Is this really necessary? Without the static_assert it really isn't obvious why ByteMap::AddressSpaceView exists.

  • Use ASVT (Address Space View Template) as a suffix on types that takes AddressSpaceView as a single template parameter (apart from AP32 and AP64 to avoid making the name longer).
  • Remove static_assert and related header file. I'll put that in another patch.
delcypher marked 4 inline comments as done.Nov 29 2018, 11:50 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_allocator_internal.h
50

@kubamracek I went with ASVT as the suffix for types that take a single AddressSpaceView template parameter. The only place I didn't make the change is the actually allocator parameter struct type names (e.g. AP32) so that the name is kept short.

kcc added a comment.Dec 6 2018, 6:22 PM

Kostya, Vitaly,
I'd like to hear your opinion.
Primarily, from the maintainability POV: how much does this (and the following) patch reduce our ability to maintain and modify the code.

LGTM
but PrimaryAllocatorASVT is unused here, should we expect patches with PrimaryAllocatorASVT?

LGTM
but PrimaryAllocatorASVT is unused here, should we expect patches with PrimaryAllocatorASVT?

@vitalybuka
Yes there will be patches that will use PrimaryAllocatorASVT. However I should note that we actually only need to support the AddressSpaceView template parameter not being LocalAddressSpaceView for ASan and LSan. For the other sanitizer allocators (e.g. TSan) would could have slightly simpler code that just hard codes using LocalAddressSpaceView rather than making it possible at compile time to change this. I didn't do this because I wanted to make the change uniformly to all the sanitizers. If you'd prefer that I make the changes to all the other sanitizers (i.e. not ASan and LSan) in a different way (i.e. hardcode LocalAddressSpaceView) then please let me know and I'll redo this patch.

vitalybuka added a comment.EditedDec 10 2018, 1:39 PM

And for where ever it's going to be used, would be nice to have at least draft/WIP patch in the stack to see how this is going to be used

delcypher updated this revision to Diff 178018.EditedDec 13 2018, 1:45 AM

Only make AP32 templated for ASan and LSan. For other allocator AP32 types just hardcode LocalAddressSpaceView. Also put type_traits stuff back in.

@cryptoad @vitalybuka
Please take another look. I've updated the patch to use type_traits.h again and I've simplified AP32 for some of the sanitizers.

And for where ever it's going to be used, would be nice to have at least draft/WIP patch in the stack to see how this is going to be used

I've avoided doing this thus far for several reasons:

  • Creating lots of patches further divides your team's limited review time.
  • If changes are requested to patches low in the stack of patches, then this will likely require changes to many patches higher in stack, and possibly even a complete rethink of the approach. This rewriting can burn a lot of time so I prefer to rewrite just one or two patches at a time rather than a large patch set to avoid this.

I think this patch is much closer to something that your team will accept so I'll start creating more patches that tries to follow the style of this patch. I'll put these up for review today.

@kcc @cryptoad @vitalybuka Is this patch okay to land now?

This revision is now accepted and ready to land.Dec 13 2018, 3:48 PM
This revision was automatically updated to reflect the committed changes.