[PATCH] D89490: Introduce __attribute__((darwin_abi))

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1481
+def DarwinABI : DeclOrTypeAttr {
+  let Spellings = [GCC<"darwin_abi">];
+//  let Subjects = [Function, ObjCMethod];
----------------
I suspect this should be using the `Clang` spelling as I don't believe GCC supports this attribute.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2258
+  let Content = [{
+On Linux ARM64 targets, this attribute changes the calling convention of
+a function to match the default convention used on Apple ARM64. This
----------------
Adding more details about what that ABI actually is, or linking to a place where the user can learn more on their own, would be appreciated.


================
Comment at: clang/include/clang/Basic/Specifiers.h:269
 
+  // clang-format off
   /// CallingConv - Specifies the calling convention that a function uses.
----------------
My personal opinion is that these formatting markers are noise. However, there's a fair number of enumerations this could cover and disabling the format behavior may be reasonable.

I think I'd rather see the formatting comments removed from this patch. They can be added in a different commit but perhaps the file can be restructured so that we don't feel the need to sprinkle so many comments around.


================
Comment at: clang/lib/AST/Type.cpp:3115
 
+// clang-format off
 StringRef FunctionType::getNameForCallConv(CallingConv CC) {
----------------
I'd remove these as well.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:46
 
+// clang-format off
 unsigned CodeGenTypes::ClangCallConvToLLVMCallConv(CallingConv CC) {
----------------
These too.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:238
+  if (D->hasAttr<DarwinABIAttr>())
+    return IsDarwin ? CC_C : CC_AArch64Darwin;
+
----------------
Can you help me understand this change a bit better? If the declaration uses the Darwin ABI and the platform is Darwin, you want to use the cdecl convention?


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:70
+    // Returns true if AArch64 Darwin ABI is explicitly used.
+    const bool IsCtorOrDtor = (isa<CXXConstructorDecl>(GD.getDecl()) ||
+                               (isa<CXXDestructorDecl>(GD.getDecl()) &&
----------------
We don't typically go with top-level `const` on non-pointer/reference types, so I'd drop the `const`.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:78-79
+        cast<FunctionDecl>(GD.getDecl())->getType()->getAs<FunctionType>();
+    const auto FCC = FTy->getCallConv();
+    return FCC == CallingConv::CC_AArch64Darwin;
+  }
----------------
`return FTy->getCallConv() == CallingConv::CC_Aarch64Darwin;`

(Removes a questionable use of `auto` and a top-level `const` at the same time.)


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5449
   bool isDarwinPCS() const { return Kind == DarwinPCS; }
+  bool isDarwinPCS(const unsigned CConv) const {
+    return isDarwinPCS() || CConv == llvm::CallingConv::AArch64Darwin;
----------------
Drop the top-level `const` in the parameter.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4640
+  case ParsedAttr::AT_DarwinABI:
+    CC = Context.getTargetInfo().getTriple().isOSDarwin() ? CC_C
+                                                          : CC_AArch64Darwin;
----------------
Similar confusion on my part here -- if the OS is Darwin, we want to default to cdecl?


================
Comment at: llvm/lib/IR/AsmWriter.cpp:379
+    Out << "aarch64_darwincc";
+    break;
   case CallingConv::SPIR_FUNC:     Out << "spir_func"; break;
----------------
mstorsjo wrote:
> aguinet wrote:
> > mstorsjo wrote:
> > > The new code here has a different indentation than the rest; the same in a couple other places throughout the patch.
> > As clang-format changed that formatting for me, should I reformat the whole switch-case to be "clang-format compatible", or should we set this section as ignored?
> As the manually laid out custom formatting here is kinda beneficial, I think it's preferred to keep it as is. If there are annotations to make clang-format stay off of it, that's probably good (but I don't know what our policy regarding use of such annotations is, if we have any).
FWIW, I'm on the side of "don't use the comments to turn off clang-format" because it adds an extra two lines of noise every time we have to use the markup. I'd rather see either clang-format improved for the cases that cause us pain or the code change to be fine with what's produced by clang-format for most cases (there will always be one-offs where it makes sense to use the comments, I'm sure).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89490/new/

https://reviews.llvm.org/D89490