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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a reviewer: rjmccall.
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.

You should add some frontend Sema tests for the attribute. The usual tests are: correct usage without diagnostics (as both a declaration attribute and a type attribute), applying the attribute to the wrong subject, passing arguments to the attribute when it doesn't accept any.

Adding @rjmccall to see if he spots any Darwin-specific concerns, but the frontend side of things looks reasonable to me aside from a few nits.



================
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
----------------
aguinet wrote:
> 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? 
Yup! The attribute documentation content bits form a .rst file that gets compiled by Sphinx, so you can use any Sphinx markup you'd like (https://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html).


================
Comment at: clang/include/clang/Basic/Specifiers.h:269
 
+  // clang-format off
   /// CallingConv - Specifies the calling convention that a function uses.
----------------
aguinet wrote:
> 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?
Yup, that's fine -- the linter is already noisy in these sort of areas as it stands, so this won't add any new noise we're not already used to seeing. Reviewers are usually pretty good about asking for specific instances of clang-format complaints to be resolved if they're important.


================
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()) &&
----------------
aguinet wrote:
> 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?
It's not that it's an unreasonable coding style, it's that we don't want to introduce new inconsistencies in the existing code. If the local code already does it, we let it slide, but otherwise, we typically don't do it for local variables or parameters (but will sometimes do it for member declarations because that can lead to better codegen).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89490