[scudo] Align objects with alignas

[vitalybuka]

Summary

Operator new must align allocations for types with large alignment.

Before c++17 behavior was implementation defined and both clang and gc++
before 11 ignored alignment. Miss-aligned objects mysteriously crashed
tests on Ubuntu 14.

Alternatives are compile with -std=c++17 or -faligned-new, but they were
discarded as less portable.

[vitalybuka]

typo

[pcc]

I'm not sure that we can do this until all of our minimum compiler versions support C++17. E.g. minimum gcc version is 5.1.0 which was released in 2015, so it presumably doesn't have C++17 support.

[hctim]

In D101874#2737687, @pcc wrote:

I'm not sure that we can do this until all of our minimum compiler versions support C++17. E.g. minimum gcc version is 5.1.0 which was released in 2015, so it presumably doesn't have C++17 support.

Looks like gcc 5.1 has support for -std=c++17, but unfortunately no support for -faligned-new (which I thought would be preferable to bumping to cxx17).

Shouldn't we just mark the individual test rather than bumping the whole allocator?

[vitalybuka]

In D101874#2737809, @hctim wrote:

In D101874#2737687, @pcc wrote:

I'm not sure that we can do this until all of our minimum compiler versions support C++17. E.g. minimum gcc version is 5.1.0 which was released in 2015, so it presumably doesn't have C++17 support.

Looks like gcc 5.1 has support for -std=c++17, but unfortunately no support for -faligned-new (which I thought would be preferable to bumping to cxx17).

I would prefer -std=c++17 as more portable.

Shouldn't we just mark the individual test rather than bumping the whole allocator?

Why? No one complained when you asked for entire compiler-rt.

[vitalybuka]

In D101874#2737687, @pcc wrote:

I'm not sure that we can do this until all of our minimum compiler versions support C++17. E.g. minimum gcc version is 5.1.0 which was released in 2015, so it presumably doesn't have C++17 support.

Probably I should be able to convert tests to allocate aligned stuff on the stack.

[vitalybuka]

switch to new overload

[hctim]

In D101874#2737834, @vitalybuka wrote:

Shouldn't we just mark the individual test rather than bumping the whole allocator?

Why? No one complained when you asked for entire compiler-rt.

I was planning on abandoning that patch after your fix in D101514. It's not clear to me whether there's going to be enough benefit to switch crt to c++17, but these two bugs in the last two weeks are definitely fresh in the mind. Maybe let's bring it up in the next team meeting.

This test fix LGTM.

[vitalybuka]

BTW Allocating them on stack is not an option. TestConfig1 on 64bit defines allocator as .5GB object :)

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

 
 #include "allocator_config.h"
 #include "combined.h"
 
 #include <condition_variable>
 #include <memory>
 #include <mutex>
 #include <set>
+#include <stdlib.h>
 #include <thread>
 #include <vector>
 
 static constexpr scudo::Chunk::Origin Origin = scudo::Chunk::Origin::Malloc;
 static constexpr scudo::uptr MinAlignLog = FIRST_32_SECOND_64(3U, 4U);
 
 // Fuchsia complains that the function is not used.
 UNUSED static void disableDebuggerdMaybe() {
template <typename Config> struct TestAllocator : scudo::Allocator<Config> {
   TestAllocator() {
     this->reset();
     this->initThreadMaybe();
     if (scudo::archSupportsMemoryTagging() &&
         !scudo::systemDetectsMemoryTagFaultsTestOnly())
       this->disableMemoryTagging();
   }
   ~TestAllocator() { this->unmapTestOnly(); }
+
+  void *operator new(size_t size) {
+    void *p = nullptr;
+    EXPECT_EQ(0, posix_memalign(&p, alignof(TestAllocator), size));
+    return p;
+  }
+
+  void operator delete(void *ptr) { free(ptr); }
 };
 
 template <class TypeParam> struct ScudoCombinedTest : public Test {
   ScudoCombinedTest() {
     UseQuarantine = std::is_same<TypeParam, scudo::AndroidConfig>::value;
     Allocator = std::make_unique<AllocatorT>();
   }
   ~ScudoCombinedTest() {

compiler-rt/lib/scudo/standalone/tests/primary_test.cpp

 //===-- primary_test.cpp ----------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "tests/scudo_unit_test.h"
 
 #include "primary32.h"
 #include "primary64.h"
 #include "size_class_map.h"
 
 #include <condition_variable>
 #include <mutex>
+#include <stdlib.h>
 #include <thread>
 #include <vector>
 
 // Note that with small enough regions, the SizeClassAllocator64 also works on
 // 32-bit architectures. It's not something we want to encourage, but we still
 // should ensure the tests pass.
 
 struct TestConfig1 {
struct SizeClassAllocator
     : public scudo::SizeClassAllocator64<Config<BaseConfig, SizeClassMapT>> {};
 template <typename SizeClassMapT>
 struct SizeClassAllocator<TestConfig1, SizeClassMapT>
     : public scudo::SizeClassAllocator32<Config<TestConfig1, SizeClassMapT>> {};
 
 template <typename BaseConfig, typename SizeClassMapT>
 struct TestAllocator : public SizeClassAllocator<BaseConfig, SizeClassMapT> {
   ~TestAllocator() { this->unmapTestOnly(); }
+
+  void *operator new(size_t size) {
+    void *p = nullptr;
+    EXPECT_EQ(0, posix_memalign(&p, alignof(TestAllocator), size));
+    return p;
+  }
+
+  void operator delete(void *ptr) { free(ptr); }
 };
 
 template <class BaseConfig> struct ScudoPrimaryTest : public Test {};
 
 #if SCUDO_FUCHSIA
 #define SCUDO_TYPED_TEST_ALL_TYPES(FIXTURE, NAME)                              \
   SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, TestConfig2)                            \
   SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, TestConfig3)