Bug 110132 - aarch64: Bogus -Wbuiltin-declaration-mismatch with ls64 builtins
Summary: aarch64: Bogus -Wbuiltin-declaration-mismatch with ls64 builtins
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 13.0
: P3 normal
Target Milestone: ---
Assignee: Alex Coplan
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2023-06-05 21:32 UTC by Alex Coplan
Modified: 2023-06-22 10:21 UTC (History)
0 users

See Also:
Host:
Target: aarch64-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-06-06 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Coplan 2023-06-05 21:32:57 UTC
If one subverts arm_acle.h and tries to create a minimal preprocessed test case using ls64 builtins:

$ cat ls64.i
#pragma GCC aarch64 "arm_acle.h"
typedef __arm_data512_t data512_t;
void f(void *p, data512_t d) {
  __builtin_aarch64_st64b(p, d);
}
./xgcc -B . -c ls64.i -S -o /dev/null -march=armv8.7-a
ls64.i: In function ‘f’:
ls64.i:4:3: warning: implicit declaration of function ‘__builtin_aarch64_st64b’ [-Wimplicit-function-declaration]
    4 |   __builtin_aarch64_st64b(p, d);
      |   ^~~~~~~~~~~~~~~~~~~~~~~
ls64.i:4:3: warning: incompatible implicit declaration of built-in function ‘__builtin_aarch64_st64b’ [-Wbuiltin-declaration-mismatch]

we get these bogus warnings. The normal flow for e.g. Advanced SIMD builtins is:
 - Backend calls add_builtin_function.
 - c_builtin_function adds the builtin to the list of visible_builtins.
 - push_file_scope gets called and builtins in visible_builtins are made visible.

The problem here is that aarch64_init_ls64_builtins is called when processing the pragma for arm_acle.h, which is *after* push_file_scope gets called, so the builtins never get made visible by the C FE.

I think it might be easiest to make the ls64 handling more SVE-like and use the simulate_builtin_function_decl langhook (which doesn't have this deferred visibility situation), dropping the wrapper functions from arm_acle.h altogether.
Comment 1 Alex Coplan 2023-06-05 21:43:08 UTC
I should add, I believe (but haven't confirmed for sure) that the only reason we don't see these warnings when including "arm_acle.h" is the warning suppression that is done by the FE when parsing system headers.
Comment 2 Alex Coplan 2023-06-06 13:56:06 UTC
Testing a patch.
Comment 3 GCC Commits 2023-06-07 16:46:00 UTC
The master branch has been updated by Alex Coplan <acoplan@gcc.gnu.org>:

https://gcc.gnu.org/g:9963029a24f2d2510b82e7106fae3f364da33c5d

commit r14-1617-g9963029a24f2d2510b82e7106fae3f364da33c5d
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Tue Jun 6 15:19:03 2023 +0100

    aarch64: Allow compiler to define ls64 builtins [PR110132]
    
    This patch refactors the ls64 builtins to allow the compiler to define them
    directly instead of having wrapper functions in arm_acle.h. This should be not
    only easier to maintain, but it makes two important correctness fixes:
     - It fixes PR110132, where the builtins ended up getting declared with
       invisible bindings in the C FE, so the FE ended up synthesizing
       incompatible implicit definitions for these builtins.
     - It allows the builtins to be used with LTO, which didn't work previously.
    
    We also take the opportunity to add test coverage from C++ for these
    builtins.
    
    gcc/ChangeLog:
    
            PR target/110132
            * config/aarch64/aarch64-builtins.cc (aarch64_general_simulate_builtin):
            New. Use it ...
            (aarch64_init_ls64_builtins): ... here. Switch to declaring public ACLE
            names for builtins.
            (aarch64_general_init_builtins): Ensure we invoke the arm_acle.h
            setup if in_lto_p, just like we do for SVE.
            * config/aarch64/arm_acle.h: (__arm_ld64b): Delete.
            (__arm_st64b): Delete.
            (__arm_st64bv): Delete.
            (__arm_st64bv0): Delete.
    
    gcc/testsuite/ChangeLog:
    
            PR target/110132
            * lib/target-supports.exp (check_effective_target_aarch64_asm_FUNC_ok):
            Extend to ls64.
            * g++.target/aarch64/acle/acle.exp: New.
            * g++.target/aarch64/acle/ls64.C: New test.
            * g++.target/aarch64/acle/ls64_lto.C: New test.
            * gcc.target/aarch64/acle/ls64_lto.c: New test.
            * gcc.target/aarch64/acle/pr110132.c: New test.
Comment 4 Alex Coplan 2023-06-07 16:50:39 UTC
Fixed on trunk, keeping open for backports (I think we need this back to GCC 12).
Comment 5 GCC Commits 2023-06-20 21:22:50 UTC
The releases/gcc-13 branch has been updated by Alex Coplan <acoplan@gcc.gnu.org>:

https://gcc.gnu.org/g:4eb01f987606e82ba4b7696f6cf79266d9e242ad

commit r13-7462-g4eb01f987606e82ba4b7696f6cf79266d9e242ad
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Tue Jun 6 15:19:03 2023 +0100

    aarch64: Allow compiler to define ls64 builtins [PR110132]
    
    This patch refactors the ls64 builtins to allow the compiler to define them
    directly instead of having wrapper functions in arm_acle.h. This should be not
    only easier to maintain, but it makes two important correctness fixes:
     - It fixes PR110132, where the builtins ended up getting declared with
       invisible bindings in the C FE, so the FE ended up synthesizing
       incompatible implicit definitions for these builtins.
     - It allows the builtins to be used with LTO, which didn't work previously.
    
    We also take the opportunity to add test coverage from C++ for these
    builtins.
    
    gcc/ChangeLog:
    
            PR target/110132
            * config/aarch64/aarch64-builtins.cc (aarch64_general_simulate_builtin):
            New. Use it ...
            (aarch64_init_ls64_builtins): ... here. Switch to declaring public ACLE
            names for builtins.
            (aarch64_general_init_builtins): Ensure we invoke the arm_acle.h
            setup if in_lto_p, just like we do for SVE.
            * config/aarch64/arm_acle.h: (__arm_ld64b): Delete.
            (__arm_st64b): Delete.
            (__arm_st64bv): Delete.
            (__arm_st64bv0): Delete.
    
    gcc/testsuite/ChangeLog:
    
            PR target/110132
            * lib/target-supports.exp (check_effective_target_aarch64_asm_FUNC_ok):
            Extend to ls64.
            * g++.target/aarch64/acle/acle.exp: New.
            * g++.target/aarch64/acle/ls64.C: New test.
            * g++.target/aarch64/acle/ls64_lto.C: New test.
            * gcc.target/aarch64/acle/ls64_lto.c: New test.
            * gcc.target/aarch64/acle/pr110132.c: New test.
    
    (cherry picked from commit 9963029a24f2d2510b82e7106fae3f364da33c5d)
Comment 6 GCC Commits 2023-06-22 10:19:52 UTC
The releases/gcc-12 branch has been updated by Alex Coplan <acoplan@gcc.gnu.org>:

https://gcc.gnu.org/g:4481d70c9edcd89a8d9f6c0d705b05230aa080e3

commit r12-9720-g4481d70c9edcd89a8d9f6c0d705b05230aa080e3
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Tue Jun 6 15:19:03 2023 +0100

    aarch64: Allow compiler to define ls64 builtins [PR110132]
    
    This patch refactors the ls64 builtins to allow the compiler to define them
    directly instead of having wrapper functions in arm_acle.h. This should be not
    only easier to maintain, but it makes two important correctness fixes:
     - It fixes PR110132, where the builtins ended up getting declared with
       invisible bindings in the C FE, so the FE ended up synthesizing
       incompatible implicit definitions for these builtins.
     - It allows the builtins to be used with LTO, which didn't work previously.
    
    We also take the opportunity to add test coverage from C++ for these
    builtins.
    
    gcc/ChangeLog:
    
            PR target/110132
            * config/aarch64/aarch64-builtins.cc (aarch64_general_simulate_builtin):
            New. Use it ...
            (aarch64_init_ls64_builtins): ... here. Switch to declaring public ACLE
            names for builtins.
            (aarch64_general_init_builtins): Ensure we invoke the arm_acle.h
            setup if in_lto_p, just like we do for SVE.
            * config/aarch64/arm_acle.h: (__arm_ld64b): Delete.
            (__arm_st64b): Delete.
            (__arm_st64bv): Delete.
            (__arm_st64bv0): Delete.
    
    gcc/testsuite/ChangeLog:
    
            PR target/110132
            * lib/target-supports.exp (check_effective_target_aarch64_asm_FUNC_ok):
            Extend to ls64.
            * g++.target/aarch64/acle/acle.exp: New.
            * g++.target/aarch64/acle/ls64.C: New test.
            * g++.target/aarch64/acle/ls64_lto.C: New test.
            * gcc.target/aarch64/acle/ls64_lto.c: New test.
            * gcc.target/aarch64/acle/pr110132.c: New test.
    
    (cherry picked from commit 9963029a24f2d2510b82e7106fae3f364da33c5d)
Comment 7 Alex Coplan 2023-06-22 10:21:49 UTC
Fixed on all affected branches.