Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

[dblaikie]

Summary

Spinoff from D119051 to experiment with moving properties from TargetCXXABI to TargetInfo.

This is incomplete as it fails a couple of clang tests at the moment:

Clang :: CodeGenCXX/armv7k.cpp
Clang :: Layout/watchos-standard-layout.cpp

Haven't looked into why those are failing, but posting the code to see if this is even in the right ballpark of what was being suggested

[rnk]

clang/include/clang/Basic/TargetInfo.h

953

The virtual method works, but it does seem to have caused a behavior change. I would be fine with switches and conditionals on the LLVM triple, or a TargetInfo field set in the constructor. That's how a lot of these flag-like behaviors are controlled. Whatever makes things easy.

My concern was more that we should put the target's rules for tail padding next to the target's rules for what constitutes POD-ness.

clang/lib/Basic/Targets/OSTargets.h

169

I think it would be equivalent to check T.getArch() == llvm::Triple::AArch64, that's probably what set the TargetCXXABI.

860

WindowsTargetInfo seems shared with mingw which uses GCC rules, so this isn't quite right. You can check getTriple().isWindowsMSVCEnvironment() here to get equivalent behavior.

[dblaikie]

Fix to use CXXABI

[dblaikie]

clang/lib/Basic/Targets/OSTargets.h

169

Hmm, seems one of the tests failed because apparently CXXABI can be chosen separately from target. clang/test/CodeGenCXX/armv7k.cpp for instance runs with -triple=arm64_32-apple-ios -emit-llvm -target-abi darwinpcs

Which seems to have an UnknownOS on the triple, but a WatchOS on the TargetCXXABI... (I don't really understand/haven't looked into how all these things relate to the command line arguments there - none of it mentions WatchOS, but maybe darwinpcs is the name of watchOS?

Updating this function to inspect the CXXABI for WatchOS, AppleARM64, and iOS (which I'd missed from here previously) here makes the tests pass...

Maybe that suggests we should move all this (& the D119051) to TargetCXXABI?

[dblaikie]

clang/lib/Basic/Targets/OSTargets.h

169

I guess if this version is going to defer entirely to the CXXABI then presumably the other implementations of getTailPaddingUseRules should do that too - or all of this is to say maybe this property should stay in the ABI? & we should add the DefaultedSMFArePOD property to TargetCXXABI, instead of TargetInfo?

(all that said, I am somewhat confused by what an "OS" is, if it's not an ABI? What does it mean to use one OS but a different ABI? What features of the OS persist in that case? I can't think of any, but don't know much about these things)

[rnk]

I think in the end, perhaps it is not worth disturbing this code.

clang/lib/Basic/Targets/OSTargets.h

169

I can't speak to any of that, it doesn't make any sense to me to separate the ABI from the OS.

[dblaikie]

(abandoned this, but still kind of curious)

@rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? Though it means these CXX ABIs don't get the benefit of being grouped with the rest of the targetOS - instead they're a bunch of switches, which seems a bit unfortunate, but I guess there's not lots of properties here, so maybe that's OK.

[dblaikie]

In D135326#3851672, @dblaikie wrote:

(abandoned this, but still kind of curious)

@rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? Though it means these CXX ABIs don't get the benefit of being grouped with the rest of the targetOS - instead they're a bunch of switches, which seems a bit unfortunate, but I guess there's not lots of properties here, so maybe that's OK.

I guess the other question: Is this flexibility valuable/worthwhile, or could we fold the CXXABI back into the TargetOS?

[rjmccall]

In D135326#3851678, @dblaikie wrote:

In D135326#3851672, @dblaikie wrote:

(abandoned this, but still kind of curious)

@rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? Though it means these CXX ABIs don't get the benefit of being grouped with the rest of the targetOS - instead they're a bunch of switches, which seems a bit unfortunate, but I guess there's not lots of properties here, so maybe that's OK.

I guess the other question: Is this flexibility valuable/worthwhile, or could we fold the CXXABI back into the TargetOS?

I don't think we really want to support targeting an OS while independently changing the target C++ ABI, no. But I think "OS implies C++ ABI implies ABI properties" is generally the way people define platforms, and I wouldn't want to create a situation where targets have to manually implement all of the right C++ ABI properties just to get default behavior for a specific C++ ABI or else end up weirdly divergent. If you have a different way to achieve that goal besides composition, that's worth considering.

[dblaikie]

In D135326#3853324, @rjmccall wrote:

In D135326#3851678, @dblaikie wrote:

In D135326#3851672, @dblaikie wrote:

(abandoned this, but still kind of curious)

@rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? Though it means these CXX ABIs don't get the benefit of being grouped with the rest of the targetOS - instead they're a bunch of switches, which seems a bit unfortunate, but I guess there's not lots of properties here, so maybe that's OK.

I guess the other question: Is this flexibility valuable/worthwhile, or could we fold the CXXABI back into the TargetOS?

I don't think we really want to support targeting an OS while independently changing the target C++ ABI, no. But I think "OS implies C++ ABI implies ABI properties" is generally the way people define platforms, and I wouldn't want to create a situation where targets have to manually implement all of the right C++ ABI properties just to get default behavior for a specific C++ ABI or else end up weirdly divergent. If you have a different way to achieve that goal besides composition, that's worth considering.

Thanks for the reply!

Ah, I think I see what you mean.

So we could remove the -fc++-abi flag? (well, it's a driver flag... - deprecate it? make it a no-op or an error if you ask for an abi that isn't the one the OS already implies?)

& then maybe ABI could be a mixin the same way TargetOS works? (TargetOS could mixin the C++ ABI, then TargetInfo mixes in TargetOS)

[rjmccall]

I don't remember the history of the -fc++-abi flag at all, so if that's a driver flag, you'll probably need to investigate it more to remove it. Maybe it was added for testing purposes and only made a driver flag accidentally.

[dblaikie]

In D135326#3854141, @rjmccall wrote:

I don't remember the history of the -fc++-abi flag at all, so if that's a driver flag, you'll probably need to investigate it more to remove it. Maybe it was added for testing purposes and only made a driver flag accidentally.

Ah, fair enough. Looks like this got added relatively recently (I mean, still a couple of years ago in 2020) in https://reviews.llvm.org/D85802
Guess we had one ABI flag for this added in rG95a546ee4dbdc997e9898c9839f3851f46a78b5c, removed in rGc9bd88e6811fb622cde644a82eac41c0b02c00ee and then the new one added in the above review.
Ah well, all very confusing.

Thanks!

clang/include/clang/Basic/TargetCXXABI.h

bool canKeyFunctionBeInline() const {
     case Microsoft:
     case GenericMIPS:
     case XL:
       return true;
     }
     llvm_unreachable("bad ABI kind");
   }
 
-  /// When is record layout allowed to allocate objects in the tail
-  /// padding of a base class?
-  ///
-  /// This decision cannot be changed without breaking platform ABI
-  /// compatibility. In ISO C++98, tail padding reuse was only permitted for
-  /// non-POD base classes, but that restriction was removed retroactively by
-  /// DR 43, and tail padding reuse is always permitted in all de facto C++
-  /// language modes. However, many platforms use a variant of the old C++98
-  /// rule for compatibility.
-  enum TailPaddingUseRules {
-    /// The tail-padding of a base class is always theoretically
-    /// available, even if it's POD.
-    AlwaysUseTailPadding,
-
-    /// Only allocate objects in the tail padding of a base class if
-    /// the base class is not POD according to the rules of C++ TR1.
-    UseTailPaddingUnlessPOD03,
-
-    /// Only allocate objects in the tail padding of a base class if
-    /// the base class is not POD according to the rules of C++11.
-    UseTailPaddingUnlessPOD11
-  };
-  TailPaddingUseRules getTailPaddingUseRules() const {
-    switch (getKind()) {
-    // To preserve binary compatibility, the generic Itanium ABI has
-    // permanently locked the definition of POD to the rules of C++ TR1,
-    // and that trickles down to derived ABIs.
-    case GenericItanium:
-    case GenericAArch64:
-    case GenericARM:
-    case iOS:
-    case GenericMIPS:
-    case XL:
-      return UseTailPaddingUnlessPOD03;
-
-    // AppleARM64 and WebAssembly use the C++11 POD rules.  They do not honor
-    // the Itanium exception about classes with over-large bitfields.
-    case AppleARM64:
-    case Fuchsia:
-    case WebAssembly:
-    case WatchOS:
-      return UseTailPaddingUnlessPOD11;
-
-    // MSVC always allocates fields in the tail-padding of a base class
-    // subobject, even if they're POD.
-    case Microsoft:
-      return AlwaysUseTailPadding;
-    }
-    llvm_unreachable("bad ABI kind");
-  }
-
   friend bool operator==(const TargetCXXABI &left, const TargetCXXABI &right) {
     return left.getKind() == right.getKind();
   }
 
   friend bool operator!=(const TargetCXXABI &left, const TargetCXXABI &right) {
     return !(left == right);
   }
 };
 
 }  // end namespace clang
 
 #endif

clang/include/clang/Basic/TargetInfo.h

public:
 
   ///===---- Other target property query methods --------------------------===//
 
   /// Appends the target-specific \#define values for this
   /// target set to the specified buffer.
   virtual void getTargetDefines(const LangOptions &Opts,
                                 MacroBuilder &Builder) const = 0;
 
+  enum TailPaddingUseRules {
+    AlwaysUseTailPadding,
+    UseTailPaddingUnlessPOD03,
+    UseTailPaddingUnlessPOD11
+  };
+  virtual TailPaddingUseRules getTailPaddingUseRules() const {
+    return UseTailPaddingUnlessPOD03;
+  }
 
   /// Return information about target-specific builtins for
   /// the current primary target, and info about which builtins are non-portable
   /// across the current set of primary and secondary targets.
   virtual ArrayRef<Builtin::Info> getTargetBuiltins() const = 0;
 
   /// Returns target-specific min and max values VScale_Range.
   virtual Optional<std::pair<unsigned, unsigned>>

clang/lib/AST/RecordLayoutBuilder.cpp

 DiagnosticBuilder ItaniumRecordLayoutBuilder::Diag(SourceLocation Loc,
                                                    unsigned DiagID) {
   return Context.getDiagnostics().Report(Loc, DiagID);
 }
 
 /// Does the target C++ ABI require us to skip over the tail-padding
 /// of the given class (considering it as a base class) when allocating
 /// objects?
-static bool mustSkipTailPadding(TargetCXXABI ABI, const CXXRecordDecl *RD) {
-  switch (ABI.getTailPaddingUseRules()) {
-  case TargetCXXABI::AlwaysUseTailPadding:
+static bool mustSkipTailPadding(const TargetInfo &TI, const CXXRecordDecl *RD) {
+  switch (TI.getTailPaddingUseRules()) {
+  case TargetInfo::AlwaysUseTailPadding:
     return false;
 
-  case TargetCXXABI::UseTailPaddingUnlessPOD03:
+  case TargetInfo::UseTailPaddingUnlessPOD03:
     // FIXME: To the extent that this is meant to cover the Itanium ABI
     // rules, we should implement the restrictions about over-sized
     // bitfields:
     //
     // http://itanium-cxx-abi.github.io/cxx-abi/abi.html#POD :
     //   In general, a type is considered a POD for the purposes of
     //   layout if it is a POD type (in the sense of ISO C++
     //   [basic.types]). However, a POD-struct or POD-union (in the
     //   sense of ISO C++ [class]) with a bitfield member whose
     //   declared width is wider than the declared type of the
     //   bitfield is not a POD for the purpose of layout.  Similarly,
     //   an array type is not a POD for the purpose of layout if the
     //   element type of the array is not a POD for the purpose of
     //   layout.
     //
     //   Where references to the ISO C++ are made in this paragraph,
     //   the Technical Corrigendum 1 version of the standard is
     //   intended.
     return RD->isPOD();
 
-  case TargetCXXABI::UseTailPaddingUnlessPOD11:
+  case TargetInfo::UseTailPaddingUnlessPOD11:
     // This is equivalent to RD->getTypeForDecl().isCXX11PODType(),
     // but with a lot of abstraction penalty stripped off.  This does
     // assume that these properties are set correctly even in C++98
     // mode; fortunately, that is true because we want to assign
     // consistently semantics to the type-traits intrinsics (or at
     // least as many of them as possible).
     return RD->isTrivial() && RD->isCXX11StandardLayout();
   }
if (const auto *RD = dyn_cast<CXXRecordDecl>(D)) {
       EmptySubobjectMap EmptySubobjects(*this, RD);
       ItaniumRecordLayoutBuilder Builder(*this, &EmptySubobjects);
       Builder.Layout(RD);
 
       // In certain situations, we are allowed to lay out objects in the
       // tail-padding of base classes.  This is ABI-dependent.
       // FIXME: this should be stored in the record layout.
       bool skipTailPadding =
-          mustSkipTailPadding(getTargetInfo().getCXXABI(), RD);
+          mustSkipTailPadding(getTargetInfo(), RD);
 
       // FIXME: This should be done in FinalizeLayout.
       CharUnits DataSize =
           skipTailPadding ? Builder.getSize() : Builder.getDataSize();
       CharUnits NonVirtualSize =
           skipTailPadding ? DataSize : Builder.NonVirtualSize;
       NewEntry = new (*this) ASTRecordLayout(
           *this, Builder.getSize(), Builder.Alignment,

clang/lib/Basic/Targets/OSTargets.h

 //===--- OSTargets.h - Declare OS target feature support --------*- 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares OS specific TargetInfo types.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_BASIC_TARGETS_OSTARGETS_H
 #define LLVM_CLANG_LIB_BASIC_TARGETS_OSTARGETS_H
 
 #include "Targets.h"
+#include "llvm/ADT/Triple.h"
 
 namespace clang {
 namespace targets {
 
 template <typename TgtInfo>
 class LLVM_LIBRARY_VISIBILITY OSTargetInfo : public TgtInfo {
 protected:
   virtual void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
public:
   TargetInfo::IntType getLeastIntTypeByWidth(unsigned BitWidth,
                                              bool IsSigned) const final {
     // Darwin uses `long long` for `int_least64_t` and `int_fast64_t`.
     return BitWidth == 64
                ? (IsSigned ? TargetInfo::SignedLongLong
                            : TargetInfo::UnsignedLongLong)
                : TargetInfo::getLeastIntTypeByWidth(BitWidth, IsSigned);
   }
+
+  TargetInfo::TailPaddingUseRules getTailPaddingUseRules() const override {
+    auto CXXABI = this->getCXXABI().getKind();
+    if (CXXABI == TargetCXXABI::iOS)
+      return TargetInfo::UseTailPaddingUnlessPOD03;
+    if (CXXABI == TargetCXXABI::WatchOS ||
+        CXXABI == TargetCXXABI::AppleARM64)
+      return TargetInfo::UseTailPaddingUnlessPOD11;
+    return TargetInfo::UseTailPaddingUnlessPOD03;
+  }
 };
 
 // DragonFlyBSD Target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY DragonFlyBSDTargetInfo
     : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
protected:
   }
 
 public:
   WindowsTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : OSTargetInfo<Target>(Triple, Opts) {
     this->WCharType = TargetInfo::UnsignedShort;
     this->WIntType = TargetInfo::UnsignedShort;
   }
+  TargetInfo::TailPaddingUseRules getTailPaddingUseRules() const override {
+    if (this->getTriple().isWindowsMSVCEnvironment())
+      return TargetInfo::AlwaysUseTailPadding;
+    return TargetInfo::UseTailPaddingUnlessPOD03;
+  }
 };
 
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY NaClTargetInfo : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const override {
     if (Opts.POSIXThreads)
protected:
   }
 
 public:
   FuchsiaTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : OSTargetInfo<Target>(Triple, Opts) {
     this->MCountName = "__mcount";
     this->TheCXXABI.set(TargetCXXABI::Fuchsia);
   }
+  TargetInfo::TailPaddingUseRules getTailPaddingUseRules() const override {
+    return TargetInfo::UseTailPaddingUnlessPOD11;
+  }
 };
 
 // WebAssembly target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY WebAssemblyOSTargetInfo
     : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,

 public:
   explicit WebAssemblyOSTargetInfo(const llvm::Triple &Triple,
                                    const TargetOptions &Opts)
       : OSTargetInfo<Target>(Triple, Opts) {
     this->MCountName = "__mcount";
     this->TheCXXABI.set(TargetCXXABI::WebAssembly);
     this->HasFloat128 = true;
   }
+  TargetInfo::TailPaddingUseRules getTailPaddingUseRules() const override {
+    return TargetInfo::UseTailPaddingUnlessPOD11;
+  }
 };
 
 // WASI target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY WASITargetInfo
     : public WebAssemblyOSTargetInfo<Target> {
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const final {