Page MenuHomePhabricator

Introduce __attribute__((darwin_abi))
Needs ReviewPublic

Authored by aguinet on Oct 15 2020, 12:09 PM.

Details

Summary

This patch introduces the "darwin_abi" function attribute, to be able to compile functions for the Apple ARM64 ABI when targeting other ARM64 OSes. For now, only Linux/ARM64 is supported/tested.

I explained the motivation behind this and some limitations in this mail on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2020-October/145680.html . Please note that, in this mail, I call this attribute "apple_abi". I decided to change it to "darwin_abi", because everything Apple-related is called "darwin" in clang/llvm. That being said, I don't have any strong opinion on this, and will be willing to hear any argument in favoir of one or the other.

It does not allow to target all the differences that exist in the Darwin ARM64 ABI against the standard AAPCS one (see [1] for the exhaustive list).

What I beleive is implemented:

  • targeting the Darwin AAPCS ABI for C functions, especially those with variadic arguments. This means everything in section "Pass Arguments to Functions Correctly" and "Update Code that Passes Arguments to Variadic Functions" in [1].
  • "The ABI requires the complete object (C1) and base-object (C2) constructors to return this to their callers. Similarly, the complete object (D1) and base-object (D2) destructors return this. This behavior matches the ARM 32-bit C++ ABI." (quoting [1]). I am not sure this would be useful to anyone, but that was not that hard to implement it, so I put it. The C++ support isn't complete in this patch though (see below), so maybe it's better to remove it.
  • "When passing parameters to a function, Apple platforms ignore empty structures unless those structures have a nontrivial destructor or copy constructor. When passing such nontrivial structures, treat them as aggregates with one byte member in the generic manner." (quoting [1])
  • properly forwarding variadic arguments from a "darwin_abi function" to a linux one using va_list, by zeroing the relevant fields in the linux va_list structure to make sure only stack arguments will be used (cf. discussions in the aforementioned ML thread)

What isn't implemented and isn't supported (quoting [1]):

  • "The ABI provides a fixed layout of two size_t words for array cookies, with no extra alignment requirements. This behavior matches the ARM 32-bit C++ ABI." => this would require allowing the "darwin_abi" to be set on class types, and I think the overall feature would make a non-trivial patch. I prefer doing the C ABI right and eventually implement this afterwards.
  • "A pointer to a function declared as extern “C” isn’t interchangeable with a function declared as extern “c++”. This behavior differs from the ARM64 ABI, in which the functions are interchangeable." => I'm not sure to find where this is tested in clang, let alone this would be possible to be tested.
  • My understanding is that we should keep the Itanium mangling, even for functions with the "darwin_abi" attributes, so I didn't implemented the mangling differences. For instance, if I understood correctly, setting the "ms_abi" on a C++ function doesn't use the MSVC mangling.
  • Everything remaining in the "Handle C++ Differences" section in [1]

[1] https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms for reference

Diff Detail

Unit TestsFailed

TimeTest
280 mswindows > LLVM.tools/llvm-cov::warnings.h
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llvm-cov.exe show C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/prevent_false_instantiations.covmapping -instr-profile C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-cov\warnings.h -allow-empty -check-prefix=FAKE-FILE-STDOUT

Event Timeline

aguinet created this revision.Oct 15 2020, 12:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
aguinet requested review of this revision.Oct 15 2020, 12:09 PM

I see that the llvm side of the patch lacks tests?

llvm/lib/IR/AsmWriter.cpp
378

The new code here has a different indentation than the rest; the same in a couple other places throughout the patch.

I see that the llvm side of the patch lacks tests?

Sorry I forgot to add them indeed... I will push a patch with these.

About the formatting, git clang-format HEAD~1 did this, and it looks by the automated test that some issues are still there.

aguinet updated this revision to Diff 298461.Oct 15 2020, 2:04 PM

Update format + llvm tests

aguinet added inline comments.Oct 15 2020, 11:22 PM
llvm/lib/IR/AsmWriter.cpp
378

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?

llvm/lib/Target/AArch64/AArch64ISelLowering.h
876

Same problem as with clang-format: clang-tidy suggests "lowerAapcsFromDarwinVastart", which would mean modifying the whole file for consistency. Should we set clang-tidy to ignore this file?

mstorsjo added inline comments.Oct 15 2020, 11:28 PM
llvm/lib/IR/AsmWriter.cpp
378

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).

llvm/lib/Target/AArch64/AArch64ISelLowering.h
876

I think the churn generally isn't considered worth it regarding such things; such changes can be quite disruptive to downstream users (with a lot of non-upstream code) for little benefit. Same thing here, not sure what the policy is regarding annotations.

aguinet updated this revision to Diff 298548.Oct 15 2020, 11:30 PM

Fix one clang-tidy warning

aguinet added inline comments.Oct 15 2020, 11:37 PM
llvm/lib/AsmParser/LLParser.cpp
2175

Again here this "big" diff is a result of clang-format. We can see that "kw_aarch64_sve_vector_pcs" has been "clang-formated" but not the rest. I would prefer just add a one-line diff and maybe also add annotations?

llvm/lib/Target/AArch64/AArch64ISelLowering.h
876

I do agree that a big diff just for this is counter productive. There are few places where we already have clang-format annotations, like https://github.com/llvm/llvm-project/blob/master/llvm/lib/TextAPI/MachO/TextStub.cpp#L262 . Maybe we can add one here?

mstorsjo added inline comments.Oct 15 2020, 11:45 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.h
876

Sure, I guess that can be done.

Normally, I don't strictly follow what clang-format suggests blindly, but only selectively keep the bits that look sensible and don't unnecessarily munge otherwise untouched code. But annotations to avoid manually filtering it too much probably is better.

aguinet updated this revision to Diff 298568.Oct 16 2020, 2:18 AM

Add some clang-format tags, and restore back the "original" formatting for these cases

aguinet updated this revision to Diff 298569.Oct 16 2020, 2:21 AM

One missing formatting case...

aaron.ballman added inline comments.Oct 19 2020, 7:24 AM
clang/include/clang/Basic/Attr.td
1481

I suspect this should be using the Clang spelling as I don't believe GCC supports this attribute.

clang/include/clang/Basic/AttrDocs.td
2258

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.

clang/include/clang/Basic/Specifiers.h
269

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.

clang/lib/AST/Type.cpp
3115

I'd remove these as well.

clang/lib/CodeGen/CGCall.cpp
46

These too.

238

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?

clang/lib/CodeGen/ItaniumCXXABI.cpp
70

We don't typically go with top-level const on non-pointer/reference types, so I'd drop the const.

78–79

return FTy->getCallConv() == CallingConv::CC_Aarch64Darwin;

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

clang/lib/CodeGen/TargetInfo.cpp
5449

Drop the top-level const in the parameter.

clang/lib/Sema/SemaDeclAttr.cpp
4640

Similar confusion on my part here -- if the OS is Darwin, we want to default to cdecl?

llvm/lib/IR/AsmWriter.cpp
378

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).

mstorsjo added inline comments.Oct 19 2020, 1:05 PM
clang/lib/CodeGen/CGCall.cpp
238

This (here and in the other similar places) matches the existing logic for the ms_abi attribute; if you're on a platform where the attribute selects what already is the default (what CC_C implies on this platform), we just use that instead of the more explicit calling convention.

aaron.ballman added inline comments.Oct 20 2020, 11:54 AM
clang/lib/CodeGen/CGCall.cpp
238

Ahhhh, thank you for the clarification, that makes sense now.