[scudo][NFC] Suppress warnings for missing-noreturn, conditional-uninitialized, zero-length-array

[ddcc]

@vitalybuka ping

[vitalybuka]

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

46

can you avoid pragmas with NORETURN?

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

247–271

248

Please don't mix different warning types in a single patch?

397

Can you avoid this by extractioing Quarantine with template specialization

class <size_t Size>
class Quarantine {
  doStuff1() {blocks...}
  doStuff2() {blocks...}
private
  CachedBlock blocks[Size];
};

template<> class Quarantine<0> {
  doStuff1() {}
  doStuff2() {}
}

[ddcc]

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

46

Sorry, I don't understand the question. The UNREACHABLE macro calls the die() function that is labeled with NORETURN, which causes the compiler to warn about functions that don't return but that aren't labeled NORETURN. I can remove NORETURN from that function, but that would also require changing its' callers as well, such as dieOnMapUnmapError.

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

247–271

The issue is on HeaderPos; for some reason the compiler thinks it may be uninitialized. I can just initialize that to zero instead.

397

I'll split this off into another patch.

[ddcc]

Initialize HeaderPos, drop Quarantine changes

[vitalybuka]

zero-length-array in description is not needed

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

46

I am asking about:
NORETURN inline uptr archMemoryTagGranuleSize()

If this does not suppress the warning then just
return 0;

Which compiler do you use?
Can you paste warning content with context?

[ddcc]

Add noreturn attribute

[ddcc]

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

46

Oh sorry I misunderstood. Yeah adding noreturn avoids most of the warnings, except for the one on setRandomTag. This is because that warning only gets emitted while compiling memtag_test.cpp, which does not include combined.h, which has the only non-fatal uses of setRandomTag. But it's not correct to declare it noreturn in general, because the other uses are not fatal.

include/internal/scudo/memtag.h:305:62: error: function 'setRandomTag' could be declared with attribute 'noreturn'
      [-Werror,-Wmissing-noreturn]
                         uptr *TaggedBegin, uptr *TaggedEnd) {
                                                             ^

include/internal/scudo/memtag.h:308:1: error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
}

[vitalybuka]

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

46

I don't like pragma there but NORETURN setRandomTag is incorrect.

[vitalybuka]

BTW, I suspect your fixes are going to regress with the time.
Would you like to update scudo_standalone cmake files so buildbots check these warnings?

[ddcc]

In D121853#3414960, @vitalybuka wrote:

BTW, I suspect your fixes are going to regress with the time.
Would you like to update scudo_standalone cmake files so buildbots check these warnings?

Sure, I'll handle that in a follow-up. I'm not familiar with how the current build; are scudo/CMakeLists.txt and scudo/standalone/CMakeLists.txt separate projects? Are there instructions somewhere for just building standalone scudo instead of compiler-rt + scudo?

[vitalybuka]

In D121853#3415113, @ddcc wrote:

In D121853#3414960, @vitalybuka wrote:

BTW, I suspect your fixes are going to regress with the time.
Would you like to update scudo_standalone cmake files so buildbots check these warnings?

Sure, I'll handle that in a follow-up. I'm not familiar with how the current build; are scudo/CMakeLists.txt and scudo/standalone/CMakeLists.txt separate projects? Are there instructions somewhere for just building standalone scudo instead of compiler-rt + scudo?

standalone is replacement of scudo which uses no stuff from compiler-rt/lib/sanitizer_common/
scudo (not scudo/standalone) should be deleted and @hctim has some pending patch, so ignore that one.

bots should do: ninja check-scudo_standalone
e.g. here https://lab.llvm.org/buildbot/#/builders/37/builds/11999

[abrachet]

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

313

These need to be wrapped in #if __clang__. This doesn't build with gcc + -Werror=unknown-pragmas

[mcgrathr]

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

313

For warning switches that GCC also has, you should instead be using #pragma GCC diagnostic ....
Clang's feature is the same as GCC's, and Clang also supports the GCC syntax (which predates the Clang-specific pragma AFAIK). -Wmissing-noreturn is a warning switch both compilers have and that works the same in both.

[ddcc]

Reverting for now, looks like there's some other build failures.

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

 inline uptr untagPointer(uptr Ptr) { return Ptr & ((1ULL << 56) - 1); }
 
 inline uint8_t extractTag(uptr Ptr) { return (Ptr >> 56) & 0xf; }
 
 #else
 
 inline constexpr bool archSupportsMemoryTagging() { return false; }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wmissing-noreturn"
 inline uptr archMemoryTagGranuleSize() {
   UNREACHABLE("memory tagging not supported");
 }
 
 inline uptr untagPointer(uptr Ptr) {
   (void)Ptr;
   UNREACHABLE("memory tagging not supported");
 }
 
 inline uint8_t extractTag(uptr Ptr) {
   (void)Ptr;
   UNREACHABLE("memory tagging not supported");
 }
+#pragma clang diagnostic pop
 
 #endif
 
 #if __clang_major__ >= 12 && defined(__aarch64__)
 
 #if SCUDO_LINUX
 
 inline bool systemSupportsMemoryTagging() {
prctl(PR_SET_TAGGED_ADDR_CTRL,
         PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC | (0xfffe << PR_MTE_TAG_SHIFT),
         0, 0, 0);
 }
 
 #else // !SCUDO_LINUX
 
 inline bool systemSupportsMemoryTagging() { return false; }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wmissing-noreturn"
 inline bool systemDetectsMemoryTagFaultsTestOnly() {
   UNREACHABLE("memory tagging not supported");
 }
 
 inline void enableSystemMemoryTaggingTestOnly() {
   UNREACHABLE("memory tagging not supported");
 }
+#pragma clang diagnostic pop
 
 #endif // SCUDO_LINUX
 
 class ScopedDisableMemoryTagChecks {
   uptr PrevTCO;
 
 public:
   ScopedDisableMemoryTagChecks() {
__asm__ __volatile__(
       : "+r"(TaggedPtr)
       :
       : "memory");
   return TaggedPtr;
 }
 
 #else
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wmissing-noreturn"
 inline bool systemSupportsMemoryTagging() {
   UNREACHABLE("memory tagging not supported");
 }
 
 inline bool systemDetectsMemoryTagFaultsTestOnly() {
   UNREACHABLE("memory tagging not supported");
 }
 
inline void storeTag(uptr Ptr) {
   (void)Ptr;
   UNREACHABLE("memory tagging not supported");
 }
 
 inline uptr loadTag(uptr Ptr) {
   (void)Ptr;
   UNREACHABLE("memory tagging not supported");
 }
+#pragma clang diagnostic pop
 
 #endif
 
 inline void setRandomTag(void *Ptr, uptr Size, uptr ExcludeMask,
                          uptr *TaggedBegin, uptr *TaggedEnd) {
   *TaggedBegin = selectRandomTag(reinterpret_cast<uptr>(Ptr), ExcludeMask);
   *TaggedEnd = storeTags(*TaggedBegin, *TaggedBegin + Size);
 }
 
 inline void *untagPointer(void *Ptr) {
   return reinterpret_cast<void *>(untagPointer(reinterpret_cast<uptr>(Ptr)));
 }

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

public:
   }
 
   bool retrieve(Options Options, uptr Size, uptr Alignment,
                 LargeBlock::Header **H, bool *Zeroed) {
     const uptr PageSize = getPageSizeCached();
     const u32 MaxCount = atomic_load_relaxed(&MaxEntriesCount);
     bool Found = false;
     CachedBlock Entry;
-    uptr HeaderPos;
+    uptr HeaderPos = 0;
     {
       ScopedLock L(Mutex);
       if (EntriesCount == 0)
         return false;
       for (u32 I = 0; I < MaxCount; I++) {
         const uptr CommitBase = Entries[I].CommitBase;
         if (!CommitBase)
           continue;
         const uptr CommitSize = Entries[I].CommitSize;
         const uptr AllocPos =
             roundDownTo(CommitBase + CommitSize - Size, Alignment);
         HeaderPos =
             AllocPos - Chunk::getHeaderSize() - LargeBlock::getHeaderSize();
         if (HeaderPos > CommitBase + CommitSize)
           continue;
         if (HeaderPos < CommitBase ||
             AllocPos > CommitBase + PageSize * MaxUnusedCachePages)
           continue;
         Found = true;
         Entry = Entries[I];
         Entries[I].CommitBase = 0;
         break;
       }
     }
     if (Found) {
       *H = reinterpret_cast<LargeBlock::Header *>(
           LargeBlock::addHeaderTag<Config>(HeaderPos));
       *Zeroed = Entry.Time == 0;
       if (useMemoryTagging<Config>(Options))
         setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0, &Entry.Data);
       uptr NewBlockBegin = reinterpret_cast<uptr>(*H + 1);
       if (useMemoryTagging<Config>(Options)) {
         if (*Zeroed)
           storeTags(LargeBlock::addHeaderTag<Config>(Entry.CommitBase),
                     NewBlockBegin);
         else if (Entry.BlockBegin < NewBlockBegin)
           storeTags(Entry.BlockBegin, NewBlockBegin);
         else
           storeTags(untagPointer(NewBlockBegin),
                     untagPointer(Entry.BlockBegin));
       }
       (*H)->CommitBase = Entry.CommitBase;
       (*H)->CommitSize = Entry.CommitSize;
       (*H)->MapBase = Entry.MapBase;
       (*H)->MapSize = Entry.MapSize;
       (*H)->Data = Entry.Data;
       EntriesCount--;
     }
     return Found;
   }
 
   bool canCache(uptr Size) {
     return atomic_load_relaxed(&MaxEntriesCount) != 0U &&
            Size <= atomic_load_relaxed(&MaxEntrySize);
   }
 
   bool setOption(Option O, sptr Value) {
private:
   u32 EntriesCount = 0;
   u32 QuarantinePos = 0;
   atomic_u32 MaxEntriesCount = {};
   atomic_uptr MaxEntrySize = {};
   u64 OldestTime = 0;
   u32 IsFullEvents = 0;
   atomic_s32 ReleaseToOsIntervalMs = {};
 
   CachedBlock Entries[Config::SecondaryCacheEntriesArraySize] = {};
   CachedBlock Quarantine[Config::SecondaryCacheQuarantineSize] = {};
 };
 
 template <typename Config> class MapAllocator {
 public:
   void init(GlobalStats *S, s32 ReleaseToOsInterval = -1) {
     DCHECK_EQ(AllocatedBytes, 0U);
     DCHECK_EQ(FreedBytes, 0U);