[sanitizer_common][test] Fix CompactRingBuffer.int64 munmap error

[XiaodongLoong]

Summary

The munmap syscall expects a page size aligned address, but hard
coded 4k page size is used in CompactRingBuffer.int64 test, then
the munmap syscall get a address (alignment-16k-addr+4k) that is
not page size aligned on 16k page size system.

Co-authored-by: Weining Lu <[e-mail]>

Depends on D123910

[XiaodongLoong]

update for sanitizer_ring_buffer_test.cpp:82 EXPECT_EQ check and delete redundant variable page_size

[XiaodongLoong]

delete Change-Id

[benshi001]

compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h

91

GetPageSizeCached() is called multiple times, can we

1. keep the orginal class level variable static constexpr int kPageSizeBits,
2. initialize it with Log2(GetPageSizeCached())
3. replace other calls of GetPageSizeCached() with (1 << kPageSizeBits) ?

[benshi001]

I expect others can take a look.

[eugenis]

See https://github.com/google/sanitizers/issues/1446 - I'd rather remove the requirement that buffer size is a multiple of page size.

[XiaodongLoong]

In D117635#3277529, @eugenis wrote:

See https://github.com/google/sanitizers/issues/1446 - I'd rather remove the requirement that buffer size is a multiple of page size.

Hi, thank you for your attention to this issue.
This patch is a bugfix for the current implementation since version 8.0. I think what you mentioned is another implementation that can save memory, but there are technical problems as you said:

There is also an efficiency issue in HwasanThreadList::DontNeedThread - if
the buffer is smaller than a page, it will never be able to release
anything. That's a bit harder to fix, and I'm not sure how significant it
actually is.

compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h

91

Thanks for your comments. Your idea is very good to reduce duplicate code. I try to change the code as you said, but I met a compilation error as following:

compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h:88:24: error: constexpr variable 'kPageSizeBits' must be
      initialized by a constant expression
  static constexpr int kPageSizeBits = Log2(GetPageSizeCached());

I also attempt to implement your idea in other way, but all failed with compilation or test error.
(1) static const unsigned kPageSizeBits = Log2(GetPageSizeCached());

compilation error logs:
sanitizer_ring_buffer.h:88:41: error: in-class initializer for static data member is
      not a constant expression
  static const unsigned kPageSizeBits = Log2(GetPageSizeCached());

(2) static unsigned kPageSizeBits = Log2(GetPageSizeCached());

compilation error logs:
 error: non-const static data member must be initialized
      out of line
  static unsigned kPageSizeBits = Log2(GetPageSizeCached());

(3) unsigned kPageSizeBits = Log2(GetPageSizeCached());

test error logs:
==30453==Sanitizer CHECK failed: compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h:99 ((sizeof(CompactRingBuffer<T>))) == ((sizeof(void *))) (16, 8)

[XiaodongLoong]

A gentle ping.

[eugenis]

This patch is a bugfix for the current implementation since version 8.0. I think what you mentioned is another implementation that can save memory, but there are technical problems as you said:

The problem is that kPageSizeBits is part of a contract between the runtime library and the compiler instrumentation. See HWAddressSanitizer::emitPrologue. There is no way to read the runtime page size in compiler instrumentation. This has to be a compile-time constant.

[eugenis]

(sorry for not catching this earlier, and thank you for trying to fix this!)

[XiaodongLoong]

In D117635#3305351, @eugenis wrote:

The problem is that kPageSizeBits is part of a contract between the runtime library and the compiler instrumentation. See HWAddressSanitizer::emitPrologue. There is no way to read the runtime page size in compiler instrumentation. This has to be a compile-time constant.

If I'm not mistaken, HWAddressSanitizer is only for the AArch64. Could we keep 4K page size on AArch64 and use variable page size for other CPU architectures?

[eugenis]

In D117635#3306956, @XiaodongLoong wrote:

In D117635#3305351, @eugenis wrote:

The problem is that kPageSizeBits is part of a contract between the runtime library and the compiler instrumentation. See HWAddressSanitizer::emitPrologue. There is no way to read the runtime page size in compiler instrumentation. This has to be a compile-time constant.

If I'm not mistaken, HWAddressSanitizer is only for the AArch64. Could we keep 4K page size on AArch64 and use variable page size for other CPU architectures?

It's not, hwasan supports x86_64, too, and potentially any future arch with some kind of top-N-bit-ignore feature.

Also, CompactRingBuffer is only used in hwasan, unless you have an out-of-tree user.

[XiaodongLoong]

I have found a new solution. Please take a look. Thanks!

[XiaodongLoong]

rebase code

compiler-rt/lib/sanitizer_common/sanitizer_ring_buffer.h

 // A ring buffer with externally provided storage that encodes its state in 8
 // bytes. Has significant constraints on size and alignment of storage.
 // See a comment in hwasan/hwasan_thread_list.h for the motivation behind this.
 #if SANITIZER_WORDSIZE == 64
 template <class T>
 class CompactRingBuffer {
   // Top byte of long_ stores the buffer size in pages.
   // Lower bytes store the address of the next buffer element.
-  static constexpr int kPageSizeBits = 12;
   static constexpr int kSizeShift = 56;
   static constexpr uptr kNextMask = (1ULL << kSizeShift) - 1;
 
-  uptr GetStorageSize() const { return (long_ >> kSizeShift) << kPageSizeBits; }
+  uptr GetStorageSize() const {
+    unsigned kPageSizeBits = Log2(GetPageSizeCached());
+    return (long_ >> kSizeShift) << kPageSizeBits;
+  }
 
   void Init(void *storage, uptr size) {
+    unsigned kPageSizeBits = Log2(GetPageSizeCached());
     CHECK_EQ(sizeof(CompactRingBuffer<T>), sizeof(void *));
     CHECK(IsPowerOfTwo(size));
     CHECK_GE(size, 1 << kPageSizeBits);
     CHECK_LE(size, 128 << kPageSizeBits);
-    CHECK_EQ(size % 4096, 0);
+    CHECK_EQ(size % GetPageSizeCached(), 0);
     CHECK_EQ(size % sizeof(T), 0);
     CHECK_EQ((uptr)storage % (size * 2), 0);
     long_ = (uptr)storage | ((size >> kPageSizeBits) << kSizeShift);
   }
 
   void SetNext(const T *next) {
     long_ = (long_ & ~kNextMask) | (uptr)next;
   }

compiler-rt/lib/sanitizer_common/tests/sanitizer_ring_buffer_test.cpp

 //===-- sanitizer_vector_test.cpp -----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of *Sanitizer runtime.
 //
 //===----------------------------------------------------------------------===//
 #include "sanitizer_common/sanitizer_ring_buffer.h"
+
 #include "gtest/gtest.h"
+#include "sanitizer_common/sanitizer_common.h"
 
 namespace __sanitizer {
 
 struct LargeStruct {
   int64_t v;
   int64_t extra[3];
 
   explicit LargeStruct(int64_t v) : v(v) {}
CompactRingBuffer<T> *AllocCompactRingBuffer(size_t count) {
   size_t sz = sizeof(T) * count;
   EXPECT_EQ(0ULL, sz % 4096);
   void *p = MmapAlignedOrDieOnFatalError(sz, sz * 2, "CompactRingBuffer");
   return new CompactRingBuffer<T>(p, sz);
 }
 
 TEST(CompactRingBuffer, int64) {
   const size_t page_sizes[] = {1, 2, 4, 128};
+  size_t page_size = GetPageSizeCached();
 
   for (size_t pages : page_sizes) {
-    size_t count = 4096 * pages / sizeof(int64_t);
+    size_t count = page_size * pages / sizeof(int64_t);
     auto R = AllocCompactRingBuffer<int64_t>(count);
     int64_t top = count * 3 + 13;
     for (int64_t i = 0; i < top; ++i) R->push(i);
     for (int64_t i = 0; i < (int64_t)count; ++i)
       EXPECT_EQ(top - i - 1, (*R)[i]);
   }
 }
 #endif
 }  // namespace __sanitizer