[PATCH] D54904: Introduce `AddressSpaceView` template parameter to `SizeClassAllocator32`, `FlatByteMap`, and `TwoLevelByteMap`.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 4 inline comments as done.
delcypher added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_bytemap.h:18
 // Maps integers in rage [0, kSize) to u8 values.
-template<u64 kSize>
+template <u64 kSize, typename AddressSpaceViewTy = LocalAddressSpaceView>
 class FlatByteMap {
----------------
kubamracek wrote:
> Is this necessary? FlatByteMap isn't doing any dereferences besides reading it's own fields.
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.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_bytemap.h:80
+    if (!map2)
+      return 0;
+    auto value_ptr = AddressSpaceView::Load(&map2[idx % kSize2]);
----------------
kubamracek wrote:
> Avoid reformatting code we don't need to change.
I could fix this but please note this is just clang-format doing its thing. Surely we want to clang-format all our patches?


================
Comment at: lib/sanitizer_common/sanitizer_allocator_internal.h:50
+template <typename AddressSpaceView>
+using PrimaryInternalAllocatorT = SizeClassAllocator32<AP32<AddressSpaceView>>;
+using PrimaryInternalAllocator =
----------------
kubamracek wrote:
> Can we unify the naming convention of adding "T" and "Ty" suffix to types? Do we need both?
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?


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:243-246
+    for (uptr region = 0; region < kNumPossibleRegions; region++) {
+      uptr class_id = possible_regions[region];
+      if (class_id) {
+        uptr chunk_size = ClassIdToSize(class_id);
----------------
kubamracek wrote:
> This looks like an optimization. Can we remove it from the patch?
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.


================
Comment at: lib/sanitizer_common/sanitizer_type_traits.h:36-40
+template <typename T, typename U>
+struct is_same : public false_type {};
+
+template <typename T>
+struct is_same<T, T> : public true_type {};
----------------
kubamracek wrote:
> This (sanitizer_type_traits.h, the test file and the static_assert) can be moved to a separate patch.
Sure. I'll try to do this.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54904/new/

https://reviews.llvm.org/D54904