Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] no-FP ABI check blames caller's declaration #102983

Closed
jroelofs opened this issue Aug 12, 2024 · 6 comments · Fixed by #103392
Closed

[AArch64] no-FP ABI check blames caller's declaration #102983

jroelofs opened this issue Aug 12, 2024 · 6 comments · Fixed by #103392
Labels
ABI Application Binary Interface backend:AArch64 clang:codegen

Comments

@jroelofs
Copy link
Contributor

jroelofs commented Aug 12, 2024

$ cat >vaargs.c <<EOF
#include <stdarg.h>
#include <stdbool.h>

double takes_double(double);

static bool takes_va_list_ptr(va_list * ap) {
  double val = va_arg(*ap, double);
  return takes_double(val);
}

int main() {
  va_list ap;
  takes_va_list_ptr(&ap);
}
EOF
$ ./bin/clang vaargs.c -c -o /dev/null -march=armv8-a+nofp --target=arm64-apple-ios
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
    6 | static bool takes_va_list_ptr(va_list * ap) {
      |             ^
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
2 errors generated.

It would be less confusing if the diagnostic pointed at the call site for takes_double, with a note pointing at takes_double's declaration.

@jroelofs
Copy link
Contributor Author

cc @ostannard

@jroelofs jroelofs added clang Clang issues not falling into any other category backend:AArch64 labels Aug 12, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 12, 2024

@llvm/issue-subscribers-backend-aarch64

Author: Jon Roelofs (jroelofs)

``` $ cat >vaargs.c <<EOF #include <stdarg.h> #include <stdbool.h>

double takes_double(double);

static bool takes_va_list_ptr(va_list * ap) {
double val = va_arg(*ap, double);
return takes_double(val);
}

float foo(float);

int main() {
va_list ap;
takes_va_list_ptr(&ap);
}
EOF
$ ./bin/clang vaargs.c -c -o /dev/null -march=armv8-a+nofp --target=arm64-apple-ios
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
6 | static bool takes_va_list_ptr(va_list * ap) {
| ^
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
2 errors generated.


It would be less confusing if the diagnostic pointed at the call site for `takes_double`, with a note pointing at `takes_double`'s declaration.
</details>

@jroelofs
Copy link
Contributor Author

diff --git a/clang/lib/CodeGen/Targets/AArch64.cpp b/clang/lib/CodeGen/Targets/AArch64.cpp
index 800a55e1b962..5b6c1e98b043 100644
--- a/clang/lib/CodeGen/Targets/AArch64.cpp
+++ b/clang/lib/CodeGen/Targets/AArch64.cpp
@@ -931,11 +931,11 @@ void AArch64TargetCodeGenInfo::checkFunctionCallABISoftFloat(
     return;
 
   diagnoseIfNeedsFPReg(CGM.getDiags(), TI.getABI(), ABIInfo, ReturnType,
-                       Caller);
+                       Callee);
 
   for (const CallArg &Arg : Args)
     diagnoseIfNeedsFPReg(CGM.getDiags(), TI.getABI(), ABIInfo, Arg.getType(),
-                         Caller);
+                         Callee);
 }
 
 void AArch64TargetCodeGenInfo::checkFunctionCallABI(CodeGenModule &CGM,

I'll put up a PR with tests tomorrow.

@porglezomp
Copy link
Contributor

porglezomp commented Aug 12, 2024

     diagnoseIfNeedsFPReg(CGM.getDiags(), TI.getABI(), ABIInfo, Arg.getType(),
-                         Caller);
+                         Callee);

Hm, that patch doesn't look right—we want to diagnose the caller—the issue is just that Caller is the function declaration not the call-site, right?

@porglezomp
Copy link
Contributor

The actual point I came here to make: This should probably be backported to 19.x because this is a new diagnostic in 19.x from @ostannard, 1fd196c.

jroelofs added a commit to jroelofs/llvm-project that referenced this issue Aug 13, 2024
... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Aug 14, 2024
…lvm#103392)

... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983
(cherry picked from commit 019ef52)
@EugeneZelenko EugeneZelenko added clang:codegen ABI Application Binary Interface and removed clang Clang issues not falling into any other category labels Aug 14, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 14, 2024

@llvm/issue-subscribers-clang-codegen

Author: Jon Roelofs (jroelofs)

``` $ cat >vaargs.c <<EOF #include <stdarg.h> #include <stdbool.h>

double takes_double(double);

static bool takes_va_list_ptr(va_list * ap) {
double val = va_arg(*ap, double);
return takes_double(val);
}

int main() {
va_list ap;
takes_va_list_ptr(&ap);
}
EOF
$ ./bin/clang vaargs.c -c -o /dev/null -march=armv8-a+nofp --target=arm64-apple-ios
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
6 | static bool takes_va_list_ptr(va_list * ap) {
| ^
vaargs.c:6:13: error: 'takes_va_list_ptr' requires 'double' type support, but ABI 'darwinpcs' does not support it
2 errors generated.


It would be less confusing if the diagnostic pointed at the call site for `takes_double`, with a note pointing at `takes_double`'s declaration.
</details>

jroelofs added a commit to swiftlang/llvm-project that referenced this issue Aug 14, 2024
…lvm#103392)

... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983

(cherry picked from commit 019ef52)
jroelofs added a commit to swiftlang/llvm-project that referenced this issue Aug 14, 2024
…lvm#103392)

... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983

(cherry picked from commit 019ef52)
jroelofs added a commit to jroelofs/llvm-project that referenced this issue Aug 14, 2024
bwendling pushed a commit to bwendling/llvm-project that referenced this issue Aug 15, 2024
…lvm#103392)

... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983
tru pushed a commit to llvmbot/llvm-project that referenced this issue Aug 15, 2024
…lvm#103392)

... whereever we have the Decl for it, and even when we don't keep the
SourceLocation of it aimed at the call site.

Fixes: llvm#102983
(cherry picked from commit 019ef52)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Application Binary Interface backend:AArch64 clang:codegen
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants