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

[Adrien Guinet via Phabricator via cfe-commits]

aguinet added inline comments.


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


================
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
----------------
aaron.ballman wrote:
> 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.
Do you know if we can we put http links into this documentation? Would this render correctly in the final clang documentation? 


================
Comment at: clang/include/clang/Basic/Specifiers.h:269
 
+  // clang-format off
   /// CallingConv - Specifies the calling convention that a function uses.
----------------
aaron.ballman wrote:
> 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.
I don't have strong opinions about this, but so we agree that `clang-format` won't be happy about this patch but we're fine with it?


================
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()) &&
----------------
aaron.ballman wrote:
> We don't typically go with top-level `const` on non-pointer/reference types, so I'd drop the `const`.
I am not sure to understand the reasoning behind it. IMHO it's interesting when reading the code to know that this value will never change accross the function. Is there an issue I am missing?


================
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;
+  }
----------------
aaron.ballman wrote:
> `return FTy->getCallConv() == CallingConv::CC_Aarch64Darwin;`
> 
> (Removes a questionable use of `auto` and a top-level `const` at the same time.)
Changed in new patch!


================
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;
----------------
aaron.ballman wrote:
> Drop the top-level `const` in the parameter.
Changed in new patch!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4640
+  case ParsedAttr::AT_DarwinABI:
+    CC = Context.getTargetInfo().getTriple().isOSDarwin() ? CC_C
+                                                          : CC_AArch64Darwin;
----------------
aaron.ballman wrote:
> Similar confusion on my part here -- if the OS is Darwin, we want to default to cdecl?
Yes! The idea is to implement the C ABI of Darwin/AArch64 on foreign OSes. So, if we are already targeting Darwin, just use the C ABI. This is the same logic used just above with `AT_MSABI`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89490