[scudo] Fix implicitly narrow casting (NFC)

[Chia-hungDuan]

Summary

Explicitly cast int literal to u16 to resolve the warning of narrow
casting.

[dyung]

Oddly, this change does not seem to fix the error:

/home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:154:41: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion]
  154 |       P->MaxCount = static_cast<u16>(2) * TransferBatch::getMaxCached(Size);
      |                     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Chia-hungDuan]

Fix arithmetic type conversion

[Chia-hungDuan]

In D135945#3857851, @dyung wrote:

Oddly, this change does not seem to fix the error:

/home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:154:41: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion]
  154 |       P->MaxCount = static_cast<u16>(2) * TransferBatch::getMaxCached(Size);
      |                     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It's the arithmetic type conversion. The u16 may be promoted to int. I added the cast for both places. @dyung Can you help on verifying if this works?

[dyung]

In D135945#3859138, @Chia-hungDuan wrote:

In D135945#3857851, @dyung wrote:

Oddly, this change does not seem to fix the error:

/home/dyung/src/upstream/llvm_clean_git/compiler-rt/lib/scudo/standalone/local_cache.h:154:41: error: conversion from ‘int’ to ‘scudo::u16’ {aka ‘short unsigned int’} may change value [-Werror=conversion]
  154 |       P->MaxCount = static_cast<u16>(2) * TransferBatch::getMaxCached(Size);
      |                     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It's the arithmetic type conversion. The u16 may be promoted to int. I added the cast for both places. @dyung Can you help on verifying if this works?

Sure, testing it now.

[dyung]

I can confirm that this seems to fix the problem for these two locations. Can you also fix the other two instances of this error that I mentioned in https://reviews.llvm.org/D133145#3857847?

[Chia-hungDuan]

In D135945#3859203, @dyung wrote:

I can confirm that this seems to fix the problem for these two locations. Can you also fix the other two instances of this error that I mentioned in https://reviews.llvm.org/D133145#3857847?

Sure. Sorry I missed those two. Let me do it!

[Chia-hungDuan]

Fix more missing casts

[Chia-hungDuan]

@dyung, hope this will fix all the casting issues. Let me know if there's any issue left. Sorry for bring the problem.

[dyung]

I can successfully build now on my machine with these changes, so LGTM, and thanks for fixing this!

[kamaub]

Please hold off on commit anymore patches until the test breaks identified by my colleague in https://reviews.llvm.org/D134226#3858268 are resolved.
Several scudo test are intermittently failing on multiple powerpc bots https://lab.llvm.org/buildbot/#/builders?tags=%2Bppc
I am able to reproduce the failure on a ppc machines with a stress test:

 ~/llvm/mono/bot_reprod $ cat ~/stress-scudo.sh
RC=0
while [[ $RC == 0 ]]; do
  ninja check-runtimes >> stress-scudo.log
  RC=$?
done

which fails on the 1st, 2nd or 3rd loops (hence the flaky behaviour on the bots)
and I was able to confirm that by applying these reverts the flaky behaviour of ./ScudoUnitTest-powerpc64le-Test/163/220 and ./ScudoUnitTest-powerpc64le-Test/ScudoPrimaryTestMemoryGroup_TestConfig3/MemoryGroup disappear

~/llvm/mono/llvm-project $ git revert fd7c7ad4fe0138314b922ea0db1691d5a679cc75
[main 77190d04d010] Revert "[scudo] Fix implicitly narrow casting (NFC)"
 3 files changed, 5 insertions(+), 10 deletions(-)
~/llvm/mono/llvm-project $ git revert 9c26f51f5e178ac0fda98419e3a61d205d3b58b1
[main d982b881645e] Revert "[scudo] Support partial page releasing"
 4 files changed, 19 insertions(+), 91 deletions(-)
~/llvm/mono/llvm-project $ git revert cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa
[main 556f28478a6c] Revert "[scudo] Manage free blocks in BatchGroup."
 8 files changed, 67 insertions(+), 635 deletions(-)

The flaky tests still fail after reverting 9c26f51f5e178ac0fda98419e3a61d205d3b58b1 so cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa is likely the culprit but I will have to revert all three patches since they are dependent on each other in a chain i.e I must revert fd7c7ad4fe0138314b922ea0db1691d5a679cc75 in order to revert 9c26f51f5e178ac0fda98419e3a61d205d3b58b1 in order to revert cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa

[Chia-hungDuan]

In D135945#3859545, @kamaub wrote:

Please hold off on commit anymore patches until the test breaks identified by my colleague in https://reviews.llvm.org/D134226#3858268 are resolved.
Several scudo test are intermittently failing on multiple powerpc bots https://lab.llvm.org/buildbot/#/builders?tags=%2Bppc
I am able to reproduce the failure on a ppc machines with a stress test:

 ~/llvm/mono/bot_reprod $ cat ~/stress-scudo.sh
RC=0
while [[ $RC == 0 ]]; do
  ninja check-runtimes >> stress-scudo.log
  RC=$?
done

which fails on the 1st, 2nd or 3rd loops (hence the flaky behaviour on the bots)
and I was able to confirm that by applying these reverts the flaky behaviour of ./ScudoUnitTest-powerpc64le-Test/163/220 and ./ScudoUnitTest-powerpc64le-Test/ScudoPrimaryTestMemoryGroup_TestConfig3/MemoryGroup disappear

~/llvm/mono/llvm-project $ git revert fd7c7ad4fe0138314b922ea0db1691d5a679cc75
[main 77190d04d010] Revert "[scudo] Fix implicitly narrow casting (NFC)"
 3 files changed, 5 insertions(+), 10 deletions(-)
~/llvm/mono/llvm-project $ git revert 9c26f51f5e178ac0fda98419e3a61d205d3b58b1
[main d982b881645e] Revert "[scudo] Support partial page releasing"
 4 files changed, 19 insertions(+), 91 deletions(-)
~/llvm/mono/llvm-project $ git revert cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa
[main 556f28478a6c] Revert "[scudo] Manage free blocks in BatchGroup."
 8 files changed, 67 insertions(+), 635 deletions(-)

The flaky tests still fail after reverting 9c26f51f5e178ac0fda98419e3a61d205d3b58b1 so cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa is likely the culprit but I will have to revert all three patches since they are dependent on each other in a chain i.e I must revert fd7c7ad4fe0138314b922ea0db1691d5a679cc75 in order to revert 9c26f51f5e178ac0fda98419e3a61d205d3b58b1 in order to revert cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa

I can create the revert 9c26f51f5e178ac0fda98419e3a61d205d3b58b1 and cf9d7f55d3bec7640fa8b2f8ec1d9c1268233caa. May I have 2~3 hours more to try to fix that before we revert them?

[Chia-hungDuan]

@kamaub, I think there may be some crashes happened. Could you share the backtrace? Or where can I get that information from buildbot?

[kamaub]

Sorry, I didn’t see your message in time and I reverted. Please let me know if we can help by providing access to a machine where you can test any potential fixes so that you can re-commit.

[kamaub]

https://lab.llvm.org/buildbot/#/builders/230/builds/4037/steps/7/logs/stdio Here is a example log, I will try to get a backtrace for you.

[Chia-hungDuan]

In D135945#3859568, @kamaub wrote:

Sorry, I didn’t see your message in time and I reverted. Please let me know if we can help by providing access to a machine where you can test any potential fixes so that you can re-commit.

It's ok. I'll prepare another CL for fixing the narrow casting issue in https://reviews.llvm.org/D133145. Yes, can you help me with accessing a PPC machine?

[Chia-hungDuan]

Given that three changes got reverted. I create another CL(https://reviews.llvm.org/D135985) to address the narrow casting in https://reviews.llvm.org/D133145.
@dyung, sorry I may need your help to verify this on your machine again.

[amyk]

Hi @Chia-hungDuan! I have sent you an email to facilitate the process in gaining access to a PPC machine.

compiler-rt/lib/scudo/standalone/local_cache.h

struct TransferBatch {
     void setFromArray(CompactPtrT *Array, u16 N) {
       DCHECK_LE(N, MaxNumCached);
       Count = N;
       memcpy(Batch, Array, sizeof(Batch[0]) * Count);
     }
     void appendFromArray(CompactPtrT *Array, u16 N) {
       DCHECK_LE(N, MaxNumCached - Count);
       memcpy(Batch + Count, Array, sizeof(Batch[0]) * N);
-      Count += N;
+      // u16 will be promoted to int by arithmetic type conversion.
+      Count = static_cast<u16>(Count + N);
     }
     void clear() { Count = 0; }
     void add(CompactPtrT P) {
       DCHECK_LT(Count, MaxNumCached);
       Batch[Count++] = P;
     }
     void copyToArray(CompactPtrT *Array) const {
       memcpy(Array, Batch, sizeof(Batch[0]) * Count);
ALWAYS_INLINE void initCacheMaybe(PerClass *C) {
     initCache();
     DCHECK_NE(C->MaxCount, 0U);
   }
 
   NOINLINE void initCache() {
     for (uptr I = 0; I < NumClasses; I++) {
       PerClass *P = &PerClassArray[I];
       const uptr Size = SizeClassAllocator::getSizeByClassId(I);
-      P->MaxCount = 2 * TransferBatch::getMaxCached(Size);
+      P->MaxCount = static_cast<u16>(2 * TransferBatch::getMaxCached(Size));
       if (I != BatchClassId) {
         P->ClassSize = Size;
       } else {
         // ClassSize in this struct is only used for malloc/free stats, which
         // should only track user allocations, not internal movements.
         P->ClassSize = 0;
       }
     }
NOINLINE bool refill(PerClass *C, uptr ClassId) {
     B->clear();
     destroyBatch(ClassId, B);
     return true;
   }
 
   NOINLINE void drain(PerClass *C, uptr ClassId) {
     const u16 Count = Min(static_cast<u16>(C->MaxCount / 2), C->Count);
     Allocator->pushBlocks(this, ClassId, &C->Chunks[0], Count);
-    C->Count -= Count;
+    // u16 will be promoted to int by arithmetic type conversion.
+    C->Count = static_cast<u16>(C->Count - Count);
     for (u16 I = 0; I < C->Count; I++)
       C->Chunks[I] = C->Chunks[I + Count];
   }
 };
 
 } // namespace scudo
 
 #endif // SCUDO_LOCAL_CACHE_H_

compiler-rt/lib/scudo/standalone/primary32.h

void pushBlocksImpl(CacheT *C, uptr ClassId, CompactPtrT *Array, u32 Size,
 
     auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
       SinglyLinkedList<TransferBatch> &Batches = BG->Batches;
       TransferBatch *CurBatch = Batches.front();
       DCHECK_NE(CurBatch, nullptr);
 
       for (u32 I = 0; I < Size;) {
         DCHECK_GE(BG->MaxCachedPerBatch, CurBatch->getCount());
-        u16 UnusedSlots = BG->MaxCachedPerBatch - CurBatch->getCount();
+        u16 UnusedSlots =
+            static_cast<u16>(BG->MaxCachedPerBatch - CurBatch->getCount());
         if (UnusedSlots == 0) {
           CurBatch = C->createBatch(ClassId, reinterpret_cast<void *>(
               decompactPtr(ClassId, Array[I])));
           CurBatch->clear();
           Batches.push_front(CurBatch);
           UnusedSlots = BG->MaxCachedPerBatch;
         }
+        // `UnusedSlots` is u16 so the result will be also fit in u16.
         u16 AppendSize = static_cast<u16>(Min<u32>(UnusedSlots, Size - I));
         CurBatch->appendFromArray(&Array[I], AppendSize);
         I += AppendSize;
       }
 
       BG->PushedBlocks += Size;
     };
 

compiler-rt/lib/scudo/standalone/primary64.h

void pushBlocksImpl(CacheT *C, uptr ClassId, CompactPtrT *Array, u32 Size,
 
     auto InsertBlocks = [&](BatchGroup *BG, CompactPtrT *Array, u32 Size) {
       SinglyLinkedList<TransferBatch> &Batches = BG->Batches;
       TransferBatch *CurBatch = Batches.front();
       DCHECK_NE(CurBatch, nullptr);
 
       for (u32 I = 0; I < Size;) {
         DCHECK_GE(BG->MaxCachedPerBatch, CurBatch->getCount());
-        u16 UnusedSlots = BG->MaxCachedPerBatch - CurBatch->getCount();
+        u16 UnusedSlots =
+            static_cast<u16>(BG->MaxCachedPerBatch - CurBatch->getCount());
         if (UnusedSlots == 0) {
           CurBatch = C->createBatch(ClassId, reinterpret_cast<void *>(
               decompactPtr(ClassId, Array[I])));
           CurBatch->clear();
           Batches.push_front(CurBatch);
           UnusedSlots = BG->MaxCachedPerBatch;
         }
         // `UnusedSlots` is u16 so the result will be also fit in u16.