[Sema] Disable c++17 aligned new and delete operators if not implemented in the deployment target's c++ standard library

[ahatanak]

Summary

This patch stops declaring implicitly the new and delete operators with alignment if the deployment target is earlier than the version in which support for them was introduced . This fixes linker errors which we see when the compiler emits a new or delete operator with alignment but the c++ library it links against is old and doesn't implement those operators.

I also sent a patch that annotates the declarations in libc++ today:

https://reviews.llvm.org/D34556

rdar://problem/32664169

[rsmith]

Unlike with sized deallocation (which also requires a new stdlib entry point), it was intentional that this be enabled by default in C++17 mode. Implicit calls are only generated to the aligned forms of operator new and operator delete in cases where the program would otherwise be broken (due to requiring more alignment than the allocator would provide). A link error seems strictly preferable to silent misalignment at runtime. Plus, you have no way to tell whether the user is providing their own aligned allocation / deallocation functions, so disabling this by fiat for certain targets breaks legitimate use cases -- we need to at least respect the command-line -faligned-allocation flag and only use this target-based detection as a default / fallback.

What is the motivation for this change?

include/clang/Basic/TargetInfo.h

780

This is the wrong name for a TargetInfo member. The TargetInfo gets to say whether or not the target is known to support aligned allocation, but it's none of the target's business whether that support is ultimately enabled or disabled. Something like supportsAlignedAllocation would make more sense.

lib/Sema/SemaExprCXX.cpp

2583–2584

The driver should be making this determination, not Sema. If -cc1 is invoked with aligned allocation enabled, we should not be overriding it here.

[ahatanak]

Remove -fno-aligned-allocation from the RUN lines. The test cases should check that new or delete operators with alignment are disabled without providing -fno-aligned-allocation.

[ahatanak]

The motivation for this change was to silence linker errors I saw when compiling and linking programs in which operator new was called to allocate over-aligned types (similar to the code in cxx1z-aligned-allocation.cpp). Linkage failed because clang would emit the calls to the operator with alignment which were not implemented in the library.

I see now that it is probably not a good idea to disable this in the compiler for the two reasons you brought up.

Is there any cases where we can turn the linker error into a compile time error?

[rsmith]

In D34574#789539, @ahatanak wrote:

Is there any cases where we can turn the linker error into a compile time error?

We could possibly implicitly put an availability attribute on the implicit declarations or something equivalent, to say "hey, we think your target doesn't support this" (but it'd still need to be a decision made by the driver and overridable by the user if they know better).

[ahatanak]

I suppose it's possible to define a new driver option that can be used to tell clang to annotate the implicit declarations with an availability attribute, which will enable the link time error to be caught at compile time. Alternatively, it's also possible to put the availability attributes by default and use the driver option to remove the attributes, but this will cause clang to error out on a valid code when the users have provided their own implementations of the aligned operators.

[ahatanak]

Duncan and I had a discussion on this.

We are thinking about adding a warning that tells users that aligned allocation /deallocation operators are being called but they are not defined in the library. If the users haven't defined their own aligned allocation / deallocation operators, they will get a link error if the deployment target is too old, but the warning will tell them what the root cause of the error is. If they have defined their own operators, they will get a false positive warning, but it's still possible to turn it off by passing -faligned-allocation. Annotating the implicit declarations with availability will cause compile time errors, so we can't do so if we just want to issue a warning.

Richard, what do you think?

[dexonsmith]

In D34574#791124, @ahatanak wrote:

Duncan and I had a discussion on this.

We are thinking about adding a warning that tells users that aligned allocation /deallocation operators are being called but they are not defined in the library.

Likely, something like -Werror=aligned-allocation-availability (by default).

If the users haven't defined their own aligned allocation / deallocation operators, they will get a link error if the deployment target is too old, but the warning will tell them what the root cause of the error is. If they have defined their own operators, they will get a false positive warning, but it's still possible to turn it off by passing -faligned-allocation.

Or by passing -Wno-aligned-allocation-availability.

Annotating the implicit declarations with availability will cause compile time errors, so we can't do so if we just want to issue a warning.

Richard, what do you think?

I also wonder: should we add a warning for such code in pre-C++17?

[rsmith]

So the driver would pass a flag to the frontend to indicate "this target probably doesn't have aligned allocation support", and Sema would produce a warning if it selects an aligned allocation function when that flag is specified? That makes sense to me.

Should we also remove the recently-added availability attributes in libc++ too? If I'm using a recent libc++ and providing my own aligned allocation functions, we shouldn't reject attempts to call those aligned allocation functions.

[rsmith]

In D34574#791157, @dexonsmith wrote:

Likely, something like -Werror=aligned-allocation-availability (by default).

An error by default seems OK if accompanied with a "use -faligned-allocation if you supply your own aligned allocation functions" hint.

I also wonder: should we add a warning for such code in pre-C++17?

-Wover-aligned catches this for pre-C++17. We should probably enable it by default, though :)

[dexonsmith]

In D34574#791158, @rsmith wrote:

Should we also remove the recently-added availability attributes in libc++ too? If I'm using a recent libc++ and providing my own aligned allocation functions, we shouldn't reject attempts to call those aligned allocation functions.

Ugh; maybe. There's -D_LIBCPP_DISABLE_AVAILABILITY already, but that's too big a hammer (disables all of the markup).

[ahatanak]

The updated patch produces diagnostics if an aligned allocation function is selected that is not implemented in the c++ standard library of the deployment target (except when the function is defined by the user and its definition is available).

A couple of points I'd like to address about the approach I took:

• The only way to silence the warning is to use -Wno-aligned-allocation-unavailable, as is mentioned in the note. I initially considered using -faligned-allocation for that purpose, but I realized it would silence errors/warnings when using "-std=c++14 -faligned-allocation", which is not desirable unless the selected aligned functions are defined by the user.

• isReplaceableGlobalAllocationFunction takes a pointer to a bool, but it's also possible to have it return a pair of bools.

[ahatanak]

I'll revert the changes I made to libc++ that annotates the allocation functions with availability later.

[rsmith]

Thanks, this looks good to me. Now if we could only figure out when aligned allocation is unavailable on other platforms this easily :)

include/clang/Basic/DiagnosticSemaKinds.td

6413 ↗

(On Diff #104579)

How about "this diagnostic" instead of "error/warning"?

include/clang/Driver/CC1Options.td

571 ↗

(On Diff #104579)

-faligned-alloc-unavailable would be more consistent with other flags.

[mclow.lists]

This commit breaks all the libc++ aligned new/delete tests on Mac OS. Was that deliberate?

Failing Tests (8):

libc++ :: std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow_replace.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp
libc++ :: std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow_replace.pass.cpp

[ahatanak]

Does r307218 unbreak the tests?

include/clang/Basic/TargetInfo.h

public:
   virtual const char *getClobbers() const = 0;
 
   /// \brief Returns true if NaN encoding is IEEE 754-2008.
   /// Only MIPS allows a different encoding.
   virtual bool isNan2008() const {
     return true;
   }
 
+  /// Returns true if the target doesn't support c++17's aligned new and
+  /// delete operators. For example, if the deployment target is old and the
+  /// operators are not available in the c++ standard library, this function
+  /// should return true.
+  virtual bool disableAlignedAllocation() const {
+    return false;
+  }
+
   /// \brief Returns the target triple of the primary target.
   const llvm::Triple &getTriple() const {
     return Triple;
   }
 
   const llvm::DataLayout &getDataLayout() const {
     assert(DataLayout && "Uninitialized DataLayout!");
     return *DataLayout;

lib/Basic/Targets.cpp

public:
   }
 
   unsigned getExnObjectAlignment() const override {
     // The alignment of an exception object is 8-bytes for darwin since
     // libc++abi doesn't declare _Unwind_Exception with __attribute__((aligned))
     // and therefore doesn't guarantee 16-byte alignment.
     return  64;
   }
+
+  bool disableAlignedAllocation() const override {
+    unsigned Major, Minor, Micro;
+    const llvm::Triple &T = this->getTriple();
+    T.getOSVersion(Major, Minor, Micro);
+
+    switch (T.getOS()) {
+    case llvm::Triple::Darwin:
+    case llvm::Triple::MacOSX: // Earlier than 10.13.
+      if (Major > 10)
+        return false;
+      if (Major < 10)
+        return true;
+      return Minor < 13;
+    case llvm::Triple::IOS:
+    case llvm::Triple::TvOS: // Earlier than 11.0.
+      return Major < 11;
+    case llvm::Triple::WatchOS: // Earlier than 4.0.
+      return Major < 4;
+    default:
+      llvm_unreachable("unexpected OS");
+    }
+  }
 };
 
 
 // DragonFlyBSD Target
 template<typename Target>
 class DragonFlyBSDTargetInfo : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,

lib/Sema/SemaExprCXX.cpp

void Sema::DeclareGlobalNewDelete() {
   auto DeclareGlobalAllocationFunctions = [&](OverloadedOperatorKind Kind,
                                               QualType Return, QualType Param) {
     llvm::SmallVector<QualType, 3> Params;
     Params.push_back(Param);
 
     // Create up to four variants of the function (sized/aligned).
     bool HasSizedVariant = getLangOpts().SizedDeallocation &&
                            (Kind == OO_Delete || Kind == OO_Array_Delete);
-    bool HasAlignedVariant = getLangOpts().AlignedAllocation;
+    bool HasAlignedVariant = getLangOpts().AlignedAllocation &&
+                             !Context.getTargetInfo().disableAlignedAllocation();
 
     int NumSizeVariants = (HasSizedVariant ? 2 : 1);
     int NumAlignVariants = (HasAlignedVariant ? 2 : 1);
     for (int Sized = 0; Sized < NumSizeVariants; ++Sized) {
       if (Sized)
         Params.push_back(SizeT);
 
       for (int Aligned = 0; Aligned < NumAlignVariants; ++Aligned) {

test/CodeGenCXX/cxx1z-aligned-allocation.cpp

 // Check that delete exprs call aligned (de)allocation functions if
 // -faligned-allocation is passed in both C++11 and C++14.
 // RUN: %clang_cc1 -std=c++11 -fexceptions -fsized-deallocation -faligned-allocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s
 // RUN: %clang_cc1 -std=c++14 -fexceptions -fsized-deallocation -faligned-allocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s
 // RUN: %clang_cc1 -std=c++1z -fexceptions -fsized-deallocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++1z -fexceptions -fsized-deallocation %s -emit-llvm -triple x86_64-apple-darwin10.13 -o - | FileCheck %s
 
 // Check that we don't used aligned (de)allocation without -faligned-allocation or C++1z.
 // RUN: %clang_cc1 -std=c++14 -DUNALIGNED -fexceptions %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
 // RUN: %clang_cc1 -std=c++1z -DUNALIGNED -fexceptions -fno-aligned-allocation %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
+// RUN: %clang_cc1 -std=c++1z -DUNALIGNED -fexceptions -fno-aligned-allocation %s -emit-llvm -triple x86_64-apple-darwin10.12 -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
+// RUN: %clang_cc1 -std=c++1z -DUNALIGNED -fexceptions -fno-aligned-allocation %s -emit-llvm -triple arm64-apple-ios10 -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
+// RUN: %clang_cc1 -std=c++1z -DUNALIGNED -fexceptions -fno-aligned-allocation %s -emit-llvm -triple arm64-apple-tvos10 -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
+// RUN: %clang_cc1 -std=c++1z -DUNALIGNED -fexceptions -fno-aligned-allocation %s -emit-llvm -triple thumbv7k-apple-watchos4 -o - | FileCheck %s --check-prefix=CHECK-UNALIGNED
 
 // CHECK-UNALIGNED-NOT: _Znwm_St11align_val_t
 // CHECK-UNALIGNED-NOT: _Znam_St11align_val_t
 // CHECK-UNALIGNED-NOT: _ZdlPv_St11align_val_t
 // CHECK-UNALIGNED-NOT: _ZdaPv_St11align_val_t
 // CHECK-UNALIGNED-NOT: _ZdlPvm_St11align_val_t
 // CHECK-UNALIGNED-NOT: _ZdaPvm_St11align_val_t