[PATCH] D37082: [sanitizer] Re-introduce kUseSeparateSizeClassForBatch for the 32-bit Primary

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:103
   static uptr ClassIdToSize(uptr class_id) {
-    return SizeClassMap::Size(class_id);
+    return (class_id == SizeClassMap::kBatchClassID) ?
+        kBatchSize : SizeClassMap::Size(class_id);
----------------
cryptoad wrote:
> alekseyshl wrote:
> > cryptoad wrote:
> > > alekseyshl wrote:
> > > > Shouldn't it be also guarded by kUseSeparateSizeClassForBatch flag?
> > > In my opinion, even when not using a separate size class for batches, this should return `kBatchSize` for consistency.
> > Maybe yes, it does not matter that much. Now I am more concerned with ClassID(ClassIdToSize(kBatchClassID)) != kBatchClassID, although I cannot point out where exactly it might be a problem.
> It does show in the `SizeClassMap::Print`:
> ```
> c53 => s: 512 diff: +0 00% l 0 cached: 16 8192; id 20
> ```
> 53 is the `kBatchClassID`,  and the `ClassID(ClassIdToSize(kBatchClassID))` is 20.
> I didn't think much of it, it could be solved with a special case in `ClassID`.
How exactly can it be solved there? It does not know what these 8KB of memory are going to be used for, for the transfer batch or is it a regular user allocation. Anyway, it's more of a hypothetical problem at this point.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_size_class_map.h:206
   static void Validate() {
-    for (uptr c = 1; c < kNumClasses; c++) {
+    for (uptr c = 1; c <= kLargestClassID; c++) {
       // Printf("Validate: c%zd\n", c);
----------------
cryptoad wrote:
> cryptoad wrote:
> > alekseyshl wrote:
> > > Why this changed? Everywhere else iterating classes we use kNumClasses.
> > The issue is that `kBatchClassID` doesn't follow the same logic as the other class: its size is not larger than the one of the previous class, and it is meant to allocate only a single size. Meaning the test checking that size minus 1 still belongs to that class wouldn't pass.
> > In that sense, the validation code doesn't make sense for that particular class.
> Revisiting this: I can loop on `kNumClasses` and make a special case for `kBatchClassID`. Validate is only a test/debug functionality anyway.
That would be a better way to handle it, please do.


https://reviews.llvm.org/D37082