[stdlib][SR-2239] Refactor AAPCS64 variable argument list support.

[drodriguez]

This refactoring uses large portions of the already existing code for
x86_64 and s390 to implement AAPCS64 __VaListBuilder. The parts where
each implementation differ are the x86_64 header, and the order of the
general and vector registers.

The changes also addresses many of the request from the reviewers in
#20862 which were not addressed originally, like the reuse of the
already existing code, and the generalizations for the code to be useful
for platforms different than Linux (Android, for example).

Some of the previous reviewers concerns were not addressed. Below are my explanations.

@compnerd:

• _fpRegisterWords should not be MemoryLayout<Double>.size / MemoryLayout<Int>.size, because the size of Double is just 8 bytes, while the vector registers seems to be 16 bytes wide.
• About using the same indexing than x86_64 (“Should this not be offset by gpRegistersUsed like L554?”), I tried, but I couldn’t make it work. However the organization VR, then GR, and then stack seems to pass the tests, which seems pretty complete (there’s at least an example than goes beyond the 8 register limit which starts using the stack).
• I keep using literal -64 and -128. If you want me to add the math is fine, but I think the real math according to the standard is confussing in this case (MemoryLayout<Double>.size * _countFPRegisters was also incorrect for the reason above).

@rjmccall:

• I didn’t implement the @_va_list_kind you were proposing, but I’m not against it. I think, however, that it is of limited utility (it will only be used here, and probably nowhere else). If you still think it is a good idea, can I implement it as a follow up?

@futurejones: I hope you don’t mind I’m modifying the code and adopting the feedback from the previous PR. The code is generally useful to more platforms than Linux, so it will be a pity not to use it. My only intention is to have a better support for all the platforms with a cleaner code.

[futurejones]

@drodriguez This looks really good and the refactoring is more than welcome. I don't have the time or access to hardware to test changes on platforms other than AArch64/Linux. This was the reason for the focus of the original PR to only AArch64/Linux. If this PR can address the issues raised by @compnerd and @rjmccall in the previous PR and allow for support of more platforms then this is absolutely awesome.

[rjmccall]

I think it's a good idea because it helps to immediately identify a common source of problems in porting, but you can absolutely do it in a follow-up.

[futurejones]

✅ All checks and tests have passed on AArch64/Linux.

[rjmccall]

@drodriguez This looks really good and the refactoring is more than welcome. I don't have the time or access to hardware to test changes on platforms other than AArch64/Linux.

To be clear, we didn't expect that you would do that kind of testing. The goal is to (1) get your port working (2) without harming any existing ports while (3) trying to keep the code maintainable and (4) setting the stage to make future ports easier.

One of the advantages of @_va_list_kind is that the stdlib can be easily written to both (1) work automatically whenever we've already implemented that VA list kind and (2) fail automatically on platforms using a new VA list kind, thus sparing future porting efforts the debugging task of figuring out why C varargs aren't working on their target.

[rjmccall]

@swift-ci Please test.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 219c499961a944b6c6012fa33fe4a7d483cf55d5

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 219c499961a944b6c6012fa33fe4a7d483cf55d5

[drodriguez]

I will have a look at those errors (probably) on Monday. I must have screw up the logic and write the opposite of what I needed.

[drodriguez]

Hopefully fixed. x86-64 va_list is a struct, it should have gone in the last group. Tried it against my Linux x86-64 and passes the tests.

[jrose-apple]

[just running tests, not reviewing]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 219c499961a944b6c6012fa33fe4a7d483cf55d5

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 219c499961a944b6c6012fa33fe4a7d483cf55d5

[drodriguez]

I will try to figure out that failure in Foundation in Linux today.

[drodriguez]

I cannot reproduce the segfault in my machine (obviously).

From reading the logs it seems to fail in TestXMLDocument.test_xpath, but as far as I can see, there doesn’t seem to be a usage of variable argument list in there (neither in the test, nor in the files that the test seems to use). I will try to look further, but if there’s a way of recovering more information about the segfault it would be nice. I have been clicking around in Jenkins and I don’t think anything is accessible.

[jrose-apple]

It does seem like TestXMLDocument.test_xpath just timed out, which seems like a weird result for a varargs bug. Let's try two ways of finding this:

@swift-ci Please test Linux

[jrose-apple]

@swift-ci Please asan test

[jrose-apple]

I don't see anything obviously wrong in your refactoring either, although John (or you, or Neil) probably would have already spotted if something were clearly off.

[jrose-apple]

(ASan test did get queued; it's just waiting for a macOS executor.)

[drodriguez]

@rjmccall, @jrose-apple I was hoping to get this ball moving again, if possible. I have rebased the changes into a recent master to avoid merge problems (there was no problem), and I will love for this to get into 5.0, if possible (since the previous version was hold).

@rjmccall: I tried to implement @_va_list_kind in drodriguez@83e4453 but I’m not sure if it is going to be possible. All the platform conditions come from value in llvm::Triple, however the *BuiltinVaList values come from a clang::TargetInfo. Creating one of those seems not to be easy in LangOptions.cpp because the compilation haven’t seem to have started yet. If someone knows a way to either delay all the code until a TargetInfo is available, or some other solution, I will appreciate some pointers.

[jrose-apple]

@swift-ci Please asan test

[jrose-apple]

@swift-ci Please test

[jrose-apple]

I'll let John respond; I think he's the primary reviewer at this point.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - d0cf4d3d6938246110d9f23edd84d557dc36e631

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - d0cf4d3d6938246110d9f23edd84d557dc36e631

[rjmccall]

@rjmccall: I tried to implement @_va_list_kind in drodriguez@83e4453 but I’m not sure if it is going to be possible. All the platform conditions come from value in llvm::Triple, however the *BuiltinVaList values come from a clang::TargetInfo. Creating one of those seems not to be easy in LangOptions.cpp because the compilation haven’t seem to have started yet. If someone knows a way to either delay all the code until a TargetInfo is available, or some other solution, I will appreciate some pointers.

Hmm. This seems to be a layering problem, since for some reason LangOptions is in the Basic library instead of AST. I don't think there's any situation where we need to be asking about platform conditionals without an ASTContext available, and I believe it should always be possible to get to this sort of C ABI information from an ASTContext. I think moving this code between libraries and refactoring it to propagate in an ASTContext would be a bit unreasonable to ask you to take on; thank you for taking it this far.

[drodriguez]

Not completely unreasonable, and it gives me an idea of what can be done. I will check if the LangOptions is created too early, and if it can be delayed until the ASTContext is available, which might be enough for creating the TargetInfo (I remember needing some context for creating it, but I did that some time ago so I might misremember). It might be the solution that I was looking for. I cannot promise that it will be soon, but I will try to have a look at that approach.

In any case, I consider this refactor should be independent from those changes. If you disagree, and you want the change in LangOptions to happen before this can be merge, please tell me and I will try to figure out. Thanks!

[rjmccall]

No, I think it's fine to commit this. We probably wouldn't dream of taking that refactor for 5 anyway, whereas this might have a shot.

[bob-wilson]

We are rapidly running out of time for this kind of change in Swift 5.0. John, if you really think this might be safe enough for 5.0, we should get it merged to master ASAP and start the discussion of whether to take it for 5.0.

[rjmccall]

Alright, let me re-run tests just for completeness.

@swift-ci Please test.

[rjmccall]

Merged. Daniel, do you want to open a 5.0 PR? You should do that relatively soon if you want it to actually be merged.

[drodriguez]

I will work on it tomorrow morning. Thanks for merging!