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
70 msx64 debian > Clang.CodeGen::darwin_abi.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple aarch64-pc-linux /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/darwin_abi.c -emit-llvm -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/darwin_abi.c
90 msx64 debian > Clang.CodeGenCXX::darwinabi-returnthis.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple aarch64-pc-linux-gnu -mconstructor-aliases /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/darwinabi-returnthis.cpp -emit-llvm -O1 -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCXX/darwinabi-returnthis.cpp
110 msx64 windows > Clang.CodeGen::darwin_abi.c
Script: -- : 'RUN: at line 1'; c:\ws\w1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -triple aarch64-pc-linux C:\ws\w1\llvm-project\premerge-checks\clang\test\CodeGen\darwin_abi.c -emit-llvm -o - | c:\ws\w1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w1\llvm-project\premerge-checks\clang\test\CodeGen\darwin_abi.c
140 msx64 windows > Clang.CodeGenCXX::darwinabi-returnthis.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -triple aarch64-pc-linux-gnu -mconstructor-aliases C:\ws\w1\llvm-project\premerge-checks\clang\test\CodeGenCXX\darwinabi-returnthis.cpp -emit-llvm -O1 -o - | c:\ws\w1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w1\llvm-project\premerge-checks\clang\test\CodeGenCXX\darwinabi-returnthis.cpp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 15 2020, 12:09 PM
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
388

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
388

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
886

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
388

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
886

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
2233

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
886

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
886

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
1510

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

clang/include/clang/Basic/AttrDocs.td
2327

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
252

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
3119

I'd remove these as well.

clang/lib/CodeGen/CGCall.cpp
47

These too.

237

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
5431

Drop the top-level const in the parameter.

clang/lib/Sema/SemaDeclAttr.cpp
4780

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

llvm/lib/IR/AsmWriter.cpp
388

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
237

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
237

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

aguinet updated this revision to Diff 308202.Nov 29 2020, 12:56 AM
aguinet marked 10 inline comments as done.
aguinet added inline comments.
clang/include/clang/Basic/Attr.td
1510

Changed in new patch.

clang/include/clang/Basic/AttrDocs.td
2327

Do you know if we can we put http links into this documentation? Would this render correctly in the final clang documentation?

clang/include/clang/Basic/Specifiers.h
252

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?

clang/lib/CodeGen/ItaniumCXXABI.cpp
70

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?

78–79

Changed in new patch!

clang/lib/CodeGen/TargetInfo.cpp
5431

Changed in new patch!

clang/lib/Sema/SemaDeclAttr.cpp
4780

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.

aguinet updated this revision to Diff 308203.Nov 29 2020, 1:23 AM

Relax checks in CodeGenCXX/darwinabi-returnthis.cpp Clang test (to adapt to new attributes), and removes some useless brackets in if statements.

aaron.ballman added a subscriber: rjmccall.

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.

clang/include/clang/Basic/AttrDocs.td
2327

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

clang/include/clang/Basic/Specifiers.h
252

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.

clang/lib/CodeGen/ItaniumCXXABI.cpp
70

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

aguinet updated this revision to Diff 310006.Dec 7 2020, 1:34 PM
aguinet marked 3 inline comments as done.

Multiple things:

  • remove clang-format tags
  • remove const usage
  • enhanced attribute documentation

Thanks @aaron.ballman for the comments!

I fixed all your comments in the new patch. I will upload a new one with the Sema tests!

aguinet updated this revision to Diff 313838.Dec 28 2020, 2:33 AM

Rebased on current master branch, and added clang sema tests cc @aaron.ballman .

aguinet updated this revision to Diff 313841.Dec 28 2020, 2:36 AM

Clang format fixes

aguinet updated this revision to Diff 313842.Dec 28 2020, 2:41 AM

Replace a just introduced const auto usage.

Some minor nits while double-checking the attribute bits.

clang/include/clang/Basic/AttrDocs.td
2334
clang/test/CodeGen/darwin_abi.c
2
clang/test/CodeGen/darwin_abi_empty_structs.cpp
5
clang/test/CodeGen/darwin_abi_vaarg.c
2
clang/test/Sema/callingconv-darwin_abi.c
11

You should also add some tests like:

__attribute__((darwin_abi)) int i; // error
__attribute__((darwin_abi(12))) void func(void); // error
emaste added a subscriber: emaste.Jan 4 2021, 1:14 PM

For now, only Linux/ARM64 is supported/tested.

Is there any reason this is Linux-specific (as far as support; I understand if it's not easy for you to test on non-Linux arm64).

For now, only Linux/ARM64 is supported/tested.

Is there any reason this is Linux-specific (as far as support; I understand if it's not easy for you to test on non-Linux arm64).

It's mainly due to this: it was easier to first test support on Linux. I think support for Windows/ARM64 could appear in another patch.

aguinet updated this revision to Diff 314565.Jan 5 2021, 4:27 AM
aguinet marked 4 inline comments as done.

Rebase on current master, and take into account @aaron.ballman 's fixes (thanks!).

clang/test/Sema/callingconv-darwin_abi.c
11

If I am correct, this is tested in clang/test/Sema/darwin_abi-sysv_abi.c

The frontend parts LGTM, but you should wait for someone else to review the backend parts before landing.

clang/test/Sema/callingconv-darwin_abi.c
11

So it is! Sorry for the noise. :-)

I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't.

If you really need to build code with the Darwin ABI that runs on Linux, can you not achieve it by building with -S and then hacking up the resulting assembly files?

aguinet added a comment.EditedJan 5 2021, 11:28 PM

I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't.

I'd say it's as much as a niche use case as __attribute__((ms_abi))? But the "niche use case" is Wine, which I believe has a consequent user base. I'm not saying the goal of this patch is to be able to do Wine for ARM64/iOS, but I believe that will help make very helpful tools for developers that target that OS (and don't have access to development devices, that is, basically everyone not working at Apple).

If you really need to build code with the Darwin ABI that runs on Linux, can you not achieve it by building with -S and then hacking up the resulting assembly files?

After having written this patch and looked at the differences in assembly, that seems far from trivial to achieve. But I'd be happy to look at POC of this.

I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.

Thinking about it some more, it might be much easier to hack LLVM IR output than assembly output.

aguinet added a comment.EditedJan 7 2021, 12:43 AM

I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.

I am not saying this exact feature is needed by Wine. What I am doing is a parallel with __attribute__((ms_abi)), which is exactly the same as this exact feature, but to target the MS ABI from a foreign OS (instead of the Darwin one). Wine just can't work without that attribute.

Let's imagine that, at the time people had wanted to introduce __attribute__((ms_abi)), we would have told them "this is a niche, hack <insert compiler representation> instead". Do you really think that would have been a serious solution?

I'd say that we are in a chicken and egg problem: we don't have users for this, but maybe because we don't have the feature. At my company, we already have internal tools that are using this feature, and have lots of hopes it will simplify some of our more complex internal CI and debugging processes. We plan to open source all of this, because we also believe that will also help others with the same kind of issues. But I agree that we might be wrong, I honestly don't know.

I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.

I am not saying this exact feature is needed by Wine. What I am doing is a parallel with __attribute__((ms_abi)), which is exactly the same as this exact feature, but to target the MS ABI from a foreign OS (instead of the Darwin one). Wine just can't work without that attribute.

Let's imagine that, at the time people had wanted to introduce __attribute__((ms_abi)), we would have told them "this is a niche, hack <insert compiler representation> instead". Do you really think that would have been a serious solution?

I'd say that we are in a chicken and egg problem: we don't have users for this, but maybe because we don't have the feature. At my company, we already have internal tools that are using this feature, and have lots of hopes it will simplify some of our more complex internal CI and debugging processes. We plan to open source all of this, because we also believe that will also help others with the same kind of issues. But I agree that we might be wrong, I honestly don't know.

I may be over-reacting to the way the patch seemed to be touching on the C++ ABI in multiple places. My understanding is that ms_abi is just a calling-convention attribute; it's basically "use the (default) calling convention that MSVC would use for this function". If that's all you want, then this is reasonable, although I am worried about creating a new attribute for every system that Wine chooses to target.

About "darwin": technically, every Apple platform has a different ABI. Our current ARM64 platforms do all agree about things like the calling convention, at least if you count newer watches (which use a 32-on-64 ABI in userspace) as not ARM64. That is not true of other architectures, most notably on ARM32, where the 32-bit iOS ABI is very different from the armv7k Apple Watch ABI; and it is certainly conceivable that Apple might release a new ARM64 platform in the future with a different calling convention. The more technically correct and future-proof thing would be to use the OS name (or maybe even the triple!) in the attribute, probably as an argument to the attribute, like __attribute__((target_abi("arm64-apple-ios"))).

I may be over-reacting to the way the patch seemed to be touching on the C++ ABI in multiple places. My understanding is that ms_abi is just a calling-convention attribute; it's basically "use the (default) calling convention that MSVC would use for this function". If that's all you want, then this is reasonable, although I am worried about creating a new attribute for every system that Wine chooses to target.

I literally based this patch on how ms_abi was implemented. It's unfortunately more than just teaching clang to change the calling convention on LLVM IR functions. The fact that ABI implementations are spread all over the place between various places in LLVM is, as far as I remember, a known problem discussed many times on llvm-dev, and looks like a hard one to fix.

About "darwin": technically, every Apple platform has a different ABI. Our current ARM64 platforms do all agree about things like the calling convention, at least if you count newer watches (which use a 32-on-64 ABI in userspace) as not ARM64. That is not true of other architectures, most notably on ARM32, where the 32-bit iOS ABI is very different from the armv7k Apple Watch ABI; and it is certainly conceivable that Apple might release a new ARM64 platform in the future with a different calling convention. The more technically correct and future-proof thing would be to use the OS name (or maybe even the triple!) in the attribute, probably as an argument to the attribute, like __attribute__((target_abi("arm64-apple-ios"))).

I'm a bit afraid that __attribute__((target_abi(XX))) would conflict with the existing __attribute__((ms_abi)). Maybe, not to conflict with the MS ABI implementation, we could introduce __attribute__((darwin_abi("arm64-ios"))) (and arm64-tvos, arm64-osx, ...)?

About Apple that would create as much ABIs as products, I guess we have a living example: is the ABI for OSX/ARM64 different than the ABI for iOS/ARM64? I can't seem find any official documentation about this :/ (only talking about arm64 here, but I get your points about armv7 targets).

Thanks!

I may be over-reacting to the way the patch seemed to be touching on the C++ ABI in multiple places. My understanding is that ms_abi is just a calling-convention attribute; it's basically "use the (default) calling convention that MSVC would use for this function". If that's all you want, then this is reasonable, although I am worried about creating a new attribute for every system that Wine chooses to target.

I literally based this patch on how ms_abi was implemented. It's unfortunately more than just teaching clang to change the calling convention on LLVM IR functions. The fact that ABI implementations are spread all over the place between various places in LLVM is, as far as I remember, a known problem discussed many times on llvm-dev, and looks like a hard one to fix.

Right, I understand that — I've even given an LLVM talk about it. I was just confused about your intent because you made some effort to match other parts of the ABI besides what we might traditionally consider the calling convention. I suppose some of those changes could be thought of as feature-specific calling conventions, but I don't usually think of them that way.

About "darwin": technically, every Apple platform has a different ABI. Our current ARM64 platforms do all agree about things like the calling convention, at least if you count newer watches (which use a 32-on-64 ABI in userspace) as not ARM64. That is not true of other architectures, most notably on ARM32, where the 32-bit iOS ABI is very different from the armv7k Apple Watch ABI; and it is certainly conceivable that Apple might release a new ARM64 platform in the future with a different calling convention. The more technically correct and future-proof thing would be to use the OS name (or maybe even the triple!) in the attribute, probably as an argument to the attribute, like __attribute__((target_abi("arm64-apple-ios"))).

I'm a bit afraid that __attribute__((target_abi(XX))) would conflict with the existing __attribute__((ms_abi)).

They don't *conflict*. It's a more general scheme than the existing attribute, but the existing attribute can be thought of as a shorthand for one case of that more general scheme, so I don't see a problem here. I would like to not have to add a new attribute for every OS that Wine decides to support.

About Apple that would create as much ABIs as products, I guess we have a living example: is the ABI for OSX/ARM64 different than the ABI for iOS/ARM64? I can't seem find any official documentation about this :/ (only talking about arm64 here, but I get your points about armv7 targets).

No, we've used the same basic ABI on iOS, macOS, and tvOS for ARM64. But that's not surprising because those products were all released within a relatively short period of time from each other, so we don't have much of a laundry list of things we'd like to improve about them.

aguinet added a comment.EditedFeb 3 2021, 11:32 AM

I may be over-reacting to the way the patch seemed to be touching on the C++ ABI in multiple places. My understanding is that ms_abi is just a calling-convention attribute; it's basically "use the (default) calling convention that MSVC would use for this function". If that's all you want, then this is reasonable, although I am worried about creating a new attribute for every system that Wine chooses to target.

I literally based this patch on how ms_abi was implemented. It's unfortunately more than just teaching clang to change the calling convention on LLVM IR functions. The fact that ABI implementations are spread all over the place between various places in LLVM is, as far as I remember, a known problem discussed many times on llvm-dev, and looks like a hard one to fix.

Right, I understand that — I've even given an LLVM talk about it. I was just confused about your intent because you made some effort to match other parts of the ABI besides what we might traditionally consider the calling convention. I suppose some of those changes could be thought of as feature-specific calling conventions, but I don't usually think of them that way.

We could have a long discussion on what do we exactly call a "calling convention" :) IIRC I did implement a few C++ bits, but they could be removed from this patch for further implementation.

About "darwin": technically, every Apple platform has a different ABI. Our current ARM64 platforms do all agree about things like the calling convention, at least if you count newer watches (which use a 32-on-64 ABI in userspace) as not ARM64. That is not true of other architectures, most notably on ARM32, where the 32-bit iOS ABI is very different from the armv7k Apple Watch ABI; and it is certainly conceivable that Apple might release a new ARM64 platform in the future with a different calling convention. The more technically correct and future-proof thing would be to use the OS name (or maybe even the triple!) in the attribute, probably as an argument to the attribute, like __attribute__((target_abi("arm64-apple-ios"))).

I'm a bit afraid that __attribute__((target_abi(XX))) would conflict with the existing __attribute__((ms_abi)).

They don't *conflict*. It's a more general scheme than the existing attribute, but the existing attribute can be thought of as a shorthand for one case of that more general scheme, so I don't see a problem here. I would like to not have to add a new attribute for every OS that Wine decides to support.

You're right, let me rephrase what I wanted to say: with this, there will be two ways to target the windows ABI. I guess we can be fine with it, but that adds some kind of complexity that needs to be explained to the end user.

There is also the support the equivalent of the __builtin_ms_va_list type that needs to be considered, which would make a type that could look like __builtin_abi_va_list_INSERT_TRIPLE_HERE, with the fact that we can't have '-' in types. It might be better to do something similar than ext_vector_type, and have something like:

typedef va_list __attribute__((target_abi("arm64-apple-ios"))) arm64_ios_abi_va_list;

This means that __builtin_ms_abi_va_list would just be a typedef (and that might have some implication on legacy code?).

Another question I would have with that setup is how much this is "transposed" into the LLVM world. My feeling is that can be, at least as a first implementation, just a "frontend" to assign the darwin or MS calling conventions to functions.

About Apple that would create as much ABIs as products, I guess we have a living example: is the ABI for OSX/ARM64 different than the ABI for iOS/ARM64? I can't seem find any official documentation about this :/ (only talking about arm64 here, but I get your points about armv7 targets).

No, we've used the same basic ABI on iOS, macOS, and tvOS for ARM64. But that's not surprising because those products were all released within a relatively short period of time from each other, so we don't have much of a laundry list of things we'd like to improve about them.

Okay, thanks for the information!

So, what could be the futur of that patch? Would that be okay to merge with a target_abi Clang attribute?

Thanks!

@rjmccall @mstorsjo @aaron.ballman any advice on what to do next? Should I bring this discussion back to llvm-dev?

I don't want this to justs stall here. I would like to have a clear decision on why it is or it is not a good idea to merge this in LLVM.

Thanks!

Sorry, it's just been a busy month for me.

I may be over-reacting to the way the patch seemed to be touching on the C++ ABI in multiple places. My understanding is that ms_abi is just a calling-convention attribute; it's basically "use the (default) calling convention that MSVC would use for this function". If that's all you want, then this is reasonable, although I am worried about creating a new attribute for every system that Wine chooses to target.

I literally based this patch on how ms_abi was implemented. It's unfortunately more than just teaching clang to change the calling convention on LLVM IR functions. The fact that ABI implementations are spread all over the place between various places in LLVM is, as far as I remember, a known problem discussed many times on llvm-dev, and looks like a hard one to fix.

Right, I understand that — I've even given an LLVM talk about it. I was just confused about your intent because you made some effort to match other parts of the ABI besides what we might traditionally consider the calling convention. I suppose some of those changes could be thought of as feature-specific calling conventions, but I don't usually think of them that way.

We could have a long discussion on what do we exactly call a "calling convention" :) IIRC I did implement a few C++ bits, but they could be removed from this patch for further implementation.

About "darwin": technically, every Apple platform has a different ABI. Our current ARM64 platforms do all agree about things like the calling convention, at least if you count newer watches (which use a 32-on-64 ABI in userspace) as not ARM64. That is not true of other architectures, most notably on ARM32, where the 32-bit iOS ABI is very different from the armv7k Apple Watch ABI; and it is certainly conceivable that Apple might release a new ARM64 platform in the future with a different calling convention. The more technically correct and future-proof thing would be to use the OS name (or maybe even the triple!) in the attribute, probably as an argument to the attribute, like __attribute__((target_abi("arm64-apple-ios"))).

I'm a bit afraid that __attribute__((target_abi(XX))) would conflict with the existing __attribute__((ms_abi)).

They don't *conflict*. It's a more general scheme than the existing attribute, but the existing attribute can be thought of as a shorthand for one case of that more general scheme, so I don't see a problem here. I would like to not have to add a new attribute for every OS that Wine decides to support.

You're right, let me rephrase what I wanted to say: with this, there will be two ways to target the windows ABI. I guess we can be fine with it, but that adds some kind of complexity that needs to be explained to the end user.

There is also the support the equivalent of the __builtin_ms_va_list type that needs to be considered, which would make a type that could look like __builtin_abi_va_list_INSERT_TRIPLE_HERE, with the fact that we can't have '-' in types. It might be better to do something similar than ext_vector_type, and have something like:

typedef va_list __attribute__((target_abi("arm64-apple-ios"))) arm64_ios_abi_va_list;

Ah. That's tricky because va_list is not normally abstracted in the compiler representation; the target just gives an implicit prefix that's parsed before the translation unit, and that defines the va_list type. If you want easy OS ABI portability here, you'll need to come up with a way in the source language to spell the different va_list types and to select which va_next implementation you want (va_start has to match the current function, but va_next doesn't). Fortunately we lower va_next in the frontend, so LLVM only needs to know about va_start, and that's always determined by the current function.

Another question I would have with that setup is how much this is "transposed" into the LLVM world. My feeling is that can be, at least as a first implementation, just a "frontend" to assign the darwin or MS calling conventions to functions.

I think it really depends on how far you want to take this. If you just want to control register/stack conventions, then it seems to me that adding LLVM CCs that are explicit about which rules to use (and making Clang do its side of the lowering properly) is pretty reasonable. LLVM is already pretty well abstracted to be flexible about CCs. I'm not sure if it's well-abstracted to be flexible about varargs support, but that's still probably not too invasive. If you want to start doing function-specific overrides for features with a more complex lowering, like the C++ ABI or exception handling, that's asking a lot more.