Fix VaList and disable va_arg for AArch64 #9422

jhass · 2020-06-04T13:23:29Z

The va_arg LLVM instruction is a hoax really.

The official docs have this to say:

Note that the code generator does not yet fully support va_arg on many targets. Also, it does not currently support va_arg with aggregate types on any target.
http://llvm.org/docs/LangRef.html#id332

At least for AArch64 not fully supported is an understatement. It just returns garbage data. (for LLVM debug builds it may run into an assertion from what I've seen elsewhere.) Hence Clang does this: https://github.com/llvm/llvm-project/blob/a574edbba2b24fcfb733aa2d82308131f5b7d2d6/clang/lib/CodeGen/TargetInfo.cpp#L5677-L5921 If somebody wants to attempt porting this, be my guest. It makes use of metadata about the argument type which is computed god knows where in Clang, porting all of that sounds like fun. Not. You could also go by https://static.docs.arm.com/ihi0055/c/IHI0055C_beta_aapcs64.pdf Section 5.4, if that's more your thing.

Anyways, let's not expose broken functionality for now.

asterite · 2020-06-04T13:41:07Z

Man, LLVM looks so nice from the outside, and it really has a great optimizer, but it's full of holes. I wonder if we should have made Crystal generate C code instead, or have used GCC as a backend...

jhass · 2020-06-04T13:46:14Z

Ah yeah, I forgot I wanted to share this little exchange I had in the LLVM IRC channel after asking why Clang would do the above rather than use tthe va_arg instruction (excerpt, there was more in the vein before and after, by different people too):

<jhass> it almost feels like va_arg is just broken on aarch64 and rather than fixing that somebody went through the effort of reimplemnting it like this for clang
<nbjoerg> no
<nbjoerg> the IR stuff is really only appropiate and meant for the easy cases
<jhass> a bunch of i32's is an easy case, no? I can't get that to work
<nbjoerg> I don't know the aarch64 ABI from memory
<nbjoerg> in general, the IR is only really applicable if you have a nice stack based calling convention
<jhass> well, the call instruction seems to be implemented fine for the aarch64 variadic calling convention?
<jhass> why is it inappropriate for the va_arg instruction to handle it as well?
<jhass> why does it exist then?
<nbjoerg> jhass: legacy
<nbjoerg> jhass: noone bothered enough to remove it

jhass · 2020-06-04T13:47:45Z

Man, LLVM looks so nice from the outside, and it really has a great optimizer

A great optimizer that crashes in the default configuration on some targets for code that works just fine on others, see #9401

jhass · 2020-06-04T13:48:54Z

Oh and did I mention that the original code in stdarg.cr (well, usage of it) also crashes LLVM on AArch64? https://bugs.llvm.org/show_bug.cgi?id=46175

oprypin · 2020-06-04T16:56:53Z

spec/compiler/codegen/primitives_spec.cr


-    pending_win32 "works with C code" do


The current state of this spec represents that it's not passing but really should be made to pass. This change drops that distinction.
(But I don't have an immediate suggestion how to keep that in a nice way, so feel free to disregard)

I didn't immediately get why this was pending but the above was completely wrapped. I knew I wanted to wrap this one, so I unified to the majority.

spec/std/va_list_spec.cr

oprypin · 2020-06-04T16:59:48Z

src/va_list.cr

+    # Clang implements va_arg on AArch64 like this: https://github.com/llvm/llvm-project/blob/a574edbba2b24fcfb733aa2d82308131f5b7d2d6/clang/lib/CodeGen/TargetInfo.cpp#L5677-L5921
+    # If somebody wants to fix the va_arg instruction on AArch64 upstream, or port the above here, that would be welcome
+    def next(type)
+      \{% raise "The LLVM va_arg instruction is broken on AArch64, please implement wrappers in C calling the va_arg macro for the types you need and bind to them" %}


Could you reconcile this line with this one:

crystal/src/compiler/crystal/semantic/main_visitor.cr

Line 2435 in 48e9289

node.raise "va_arg is not yet supported on Windows"

as I'm pretty sure only one of them is needed (and I certainly don't insist that mine is the correct one)

Both seems to be functionally equivalent. What I like about my version is that if somebody would generate docs on the affected platform, they'd see the note there. I picked documenting it in stdlib because in theory it could be solved as a standard library with some macro type inspection help, I think. Just some insane effort. (Either way, solving it in the compiler as well).

Also in the unlikely case the va_arg instruction gets actually fixed upstream for the platform, somebody could bypass the stdlib check here by re-implementing the primitive binding.

But I don't really know what's the preferred way. Any other opinions?

oprypin · 2020-06-04T17:03:54Z

src/va_list.cr

+    # Clang implements va_arg on AArch64 like this: https://github.com/llvm/llvm-project/blob/a574edbba2b24fcfb733aa2d82308131f5b7d2d6/clang/lib/CodeGen/TargetInfo.cpp#L5677-L5921
+    # If somebody wants to fix the va_arg instruction on AArch64 upstream, or port the above here, that would be welcome
+    def next(type)
+      \{% raise "The LLVM va_arg instruction is broken on AArch64, please implement wrappers in C calling the va_arg macro for the types you need and bind to them" %}


I really don't like how this message reads. Even as a knowledgeable user it's just hard to get the point.

Particularly (as I assume this is a user-facing message):

I don't immediately care that it's LLVM's fault.

I don't immediately want to see what the workaround is
(also takes a while to realize that this is just a suggestion of a workaround).

I want to foremost plainly see what is not working.

But unfortunately, the entirety of the message is about the prior two irrelevant points.

Suggestions for a better message rather than just pointing out what you don't like here?

I don't agree suggesting how to workaround this limitation is irrelevant.

oprypin

Thanks for bearing with me!

va_arg is broken on most platforms, including AArch64

oprypin · 2020-06-13T15:36:47Z

Why force push? Just commit (possibly merge) and push normally

jhass · 2020-06-13T15:37:51Z

Uh just habit, I tend to maintain clean commit histories. What's the harm without pending reviews?

jhass · 2020-08-03T08:56:35Z

Rust just added a native implementation for AAPCS VaArg, if anyone wants to port that over: rust-lang/rust#73655

bcardiff added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:aarch64 topic:compiler labels Jun 4, 2020

bcardiff approved these changes Jun 4, 2020

View reviewed changes

jhass added the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 4, 2020

oprypin reviewed Jun 4, 2020

View reviewed changes

jhass removed the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 4, 2020

oprypin approved these changes Jun 4, 2020

View reviewed changes

jhass added the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 4, 2020

jhass mentioned this pull request Jun 8, 2020

Fix C ABI for AArch64 #9430

Merged

Fix VaList and disable va_arg for AArch64

759f6a3

va_arg is broken on most platforms, including AArch64

jhass force-pushed the aarch64_vaarg branch from 9ad9733 to 759f6a3 Compare June 13, 2020 15:30

jhass mentioned this pull request Jun 18, 2020

AArch64 CI #9508

Merged

5 tasks

waj approved these changes Jun 22, 2020

View reviewed changes

bcardiff added this to the 1.0.0 milestone Jun 22, 2020

bcardiff removed the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 22, 2020

bcardiff merged commit 3cdb9a9 into crystal-lang:master Jun 22, 2020

jhass deleted the aarch64_vaarg branch June 22, 2020 19:24

HertzDevil mentioned this pull request Mar 7, 2023

AArch64 Android support #13065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VaList and disable va_arg for AArch64 #9422

Fix VaList and disable va_arg for AArch64 #9422

jhass commented Jun 4, 2020

asterite commented Jun 4, 2020

jhass commented Jun 4, 2020

jhass commented Jun 4, 2020

jhass commented Jun 4, 2020 •

edited

Loading

oprypin Jun 4, 2020

jhass Jun 4, 2020

oprypin Jun 4, 2020

jhass Jun 4, 2020 •

edited

Loading

oprypin Jun 4, 2020

jhass Jun 4, 2020 •

edited

Loading

oprypin left a comment

oprypin commented Jun 13, 2020

jhass commented Jun 13, 2020 •

edited

Loading

jhass commented Aug 3, 2020

Fix VaList and disable va_arg for AArch64 #9422

Fix VaList and disable va_arg for AArch64 #9422

Conversation

jhass commented Jun 4, 2020

asterite commented Jun 4, 2020

jhass commented Jun 4, 2020

jhass commented Jun 4, 2020

jhass commented Jun 4, 2020 • edited Loading

oprypin Jun 4, 2020

Choose a reason for hiding this comment

jhass Jun 4, 2020

Choose a reason for hiding this comment

oprypin Jun 4, 2020

Choose a reason for hiding this comment

jhass Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

oprypin Jun 4, 2020

Choose a reason for hiding this comment

jhass Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

oprypin left a comment

Choose a reason for hiding this comment

oprypin commented Jun 13, 2020

jhass commented Jun 13, 2020 • edited Loading

jhass commented Aug 3, 2020

jhass commented Jun 4, 2020 •

edited

Loading

jhass Jun 4, 2020 •

edited

Loading

jhass Jun 4, 2020 •

edited

Loading

jhass commented Jun 13, 2020 •

edited

Loading