[PATCH] D61214: [scudo][standalone] Add the memory reclaiming mechanism

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/scudo/standalone/release.h:20
+  ReleaseRecorder(uptr BaseAddress = 0, MapPlatformData *Data = nullptr)
+      : BaseAddress(BaseAddress), Data(Data) {}
+
----------------
Do you need default args here?


================
Comment at: lib/scudo/standalone/release.h:37
+  uptr ReleasedBytes = 0;
+  uptr BaseAddress;
+  MapPlatformData *Data;
----------------
Initialize everything for consistency?


================
Comment at: lib/scudo/standalone/release.h:59
+    CounterSizeBitsLog = getLog2(CounterSizeBits);
+    CounterMask = ~0UL >> (MaxCounterBits - CounterSizeBits);
+
----------------
with MSVC 64bit sizeof(~0UL) < sizeof(uptr)
~((uptr)0)

or just
CounterMask = 0;
CounterMask =  ~CounterMask >> (MaxCounterBits - CounterSizeBits);


================
Comment at: lib/scudo/standalone/release.h:66
+
+    BufferSize = (roundUpTo(N, 1UL << PackingRatioLog) >> PackingRatioLog) *
+                 sizeof(*Buffer);
----------------
same for 1UL
Not sure why we use uptr for most members
u64 and ULL should work



================
Comment at: lib/scudo/standalone/release.h:159
+  uptr FullPagesBlockCountMax;
+  bool SameBlockCountPerPage;
+  if (BlockSize <= PageSize && PageSize % BlockSize == 0) {
----------------
I think the following way it's more readable and easier to see that UNREACHABLE is not possible
```
if (BlockSize <= PageSize) {
  if (PageSize % BlockSize == 0) {
    // Same number of chunks per page, no cross overs.
    FullPagesBlockCountMax = PageSize / BlockSize;
    SameBlockCountPerPage = true;
  } else if (BlockSize % (PageSize % BlockSize) == 0) {
    // Some chunks are crossing page boundaries, which means that the page
    // contains one or two partial chunks, but all pages contain the same
    // number of chunks.
    FullPagesBlockCountMax = PageSize / BlockSize + 1;
    SameBlockCountPerPage = true;
  } else {
    FullPagesBlockCountMax = PageSize / BlockSize + 2;
    SameBlockCountPerPage = false;
  }
} else {
  if (BlockSize % PageSize == 0) {
    // One chunk covers multiple pages, no cross overs.
    FullPagesBlockCountMax = 1;
    SameBlockCountPerPage = true;
  } else {
    // One chunk covers multiple pages, Some chunks are crossing page
    // boundaries. Some pages contain one chunk, some contain two.
    FullPagesBlockCountMax = 2;
    SameBlockCountPerPage = false;
  }
}
```


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61214