Bug 91766 - -fvisibility=hidden during -fpic still uses GOT indirection on arm64
Summary: -fvisibility=hidden during -fpic still uses GOT indirection on arm64
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 8.2.0
: P3 normal
Target Milestone: 10.0
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization, patch, visibility
Depends on:
Blocks: visibility
  Show dependency treegraph
 
Reported: 2019-09-13 16:58 UTC by martin krastev
Modified: 2021-09-17 00:20 UTC (History)
1 user (show)

See Also:
Host:
Target: aarch64-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-09-17 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description martin krastev 2019-09-13 16:58:21 UTC
Passing -fvisibility=hidden does not stop GOT round-trips for global symbols during -fpic. Example:

#if DO_HIDE
#define HIDE __attribute__ ((visibility("hidden")))
#else
#define HIDE
#endif

int room[2] HIDE;

int* foo()
{
    int* p = room;
    *p = 0;
    return p;
}

$ gcc-8.2 -Ofast -fvisibility=hidden -fpic

foo:
        adrp    x1, _GLOBAL_OFFSET_TABLE_
        ldr     x1, [x1, #:gotpage_lo15:room]
        mov     x0, x1
        str     wzr, [x1]
        ret

Explicitly hiding it via DO_HIDE works as expected:

$ gcc-8.2 -Ofast -fvisibility=hidden -fpic -DDO_HIDE

foo:
        adrp    x1, room
        add     x0, x1, :lo12:room
        str     wzr, [x1, #:lo12:room]
        ret

Here's the godbolt link: https://godbolt.org/z/HCd2LT
Comment 1 Andrew Pinski 2019-09-13 22:10:53 UTC
Try -fno-common.
Comment 2 Andrew Pinski 2019-09-13 22:12:55 UTC
room is a common symbol which causes issues.
Comment 3 martin krastev 2019-09-14 07:04:19 UTC
So it appears to be a clash between -fcommon and -fvisibility=hidden during -fpic -- passing either -fno-common or -fno-pic drops the GOT indirection. And explicitly hiding the symbol obviously solves it. But the crux of the issue, IMO, is a multi-platform one -- that behavior deviates on gcc-8.2 from platform to platform. On amd64 it suffices to -fvisibility=hidden to stop GOT detours, whereas on aarch64 it's -fvisibility=hidden -fno-common. As a result aarch64 performance gets penalized in unsuspecting multi-plats.
Comment 4 Wilco 2019-09-14 10:32:01 UTC
(In reply to martin krastev from comment #3)
> So it appears to be a clash between -fcommon and -fvisibility=hidden during
> -fpic -- passing either -fno-common or -fno-pic drops the GOT indirection.
> And explicitly hiding the symbol obviously solves it. But the crux of the
> issue, IMO, is a multi-platform one -- that behavior deviates on gcc-8.2
> from platform to platform. On amd64 it suffices to -fvisibility=hidden to
> stop GOT detours, whereas on aarch64 it's -fvisibility=hidden -fno-common.
> As a result aarch64 performance gets penalized in unsuspecting multi-plats.

-fno-common should really become the default, -fcommon causes way too many issues.
Comment 5 martin krastev 2019-09-14 15:36:07 UTC
For reference, here what -fvisibility=hidden -fpic -fcommon produces on amd64 (https://godbolt.org/z/0aIyhl):

foo:
        movl    $0, room(%rip)
        leaq    room(%rip), %rax
        ret

Notice the absence of GOT indirection.
Comment 6 Eric Gallager 2019-09-17 01:04:47 UTC
(In reply to Wilco from comment #4)
> (In reply to martin krastev from comment #3)
> > So it appears to be a clash between -fcommon and -fvisibility=hidden during
> > -fpic -- passing either -fno-common or -fno-pic drops the GOT indirection.
> > And explicitly hiding the symbol obviously solves it. But the crux of the
> > issue, IMO, is a multi-platform one -- that behavior deviates on gcc-8.2
> > from platform to platform. On amd64 it suffices to -fvisibility=hidden to
> > stop GOT detours, whereas on aarch64 it's -fvisibility=hidden -fno-common.
> > As a result aarch64 performance gets penalized in unsuspecting multi-plats.
> 
> -fno-common should really become the default, -fcommon causes way too many
> issues.

That's bug 85678
Comment 7 Wilco 2019-09-17 11:19:12 UTC
(In reply to Eric Gallager from comment #6)
> (In reply to Wilco from comment #4)
> > (In reply to martin krastev from comment #3)
> > > So it appears to be a clash between -fcommon and -fvisibility=hidden during
> > > -fpic -- passing either -fno-common or -fno-pic drops the GOT indirection.
> > > And explicitly hiding the symbol obviously solves it. But the crux of the
> > > issue, IMO, is a multi-platform one -- that behavior deviates on gcc-8.2
> > > from platform to platform. On amd64 it suffices to -fvisibility=hidden to
> > > stop GOT detours, whereas on aarch64 it's -fvisibility=hidden -fno-common.
> > > As a result aarch64 performance gets penalized in unsuspecting multi-plats.
> > 
> > -fno-common should really become the default, -fcommon causes way too many
> > issues.
> 
> That's bug 85678

Yes, we need to push on that one again since not much happened...
Comment 8 Wilco 2019-09-17 11:21:35 UTC
Confirmed btw - the difference is due to HAVE_LD_PIE_COPYRELOC in i386.c:

/* For i386, common symbol is local only for non-PIE binaries.  For
   x86-64, common symbol is local only for non-PIE binaries or linker
   supports copy reloc in PIE binaries.   */

static bool
ix86_binds_local_p (const_tree exp)
{
  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true,
                                  (!flag_pic
                                   || (TARGET_64BIT
                                       && HAVE_LD_PIE_COPYRELOC != 0)));
}
Comment 9 Kamlesh Kumar 2019-10-24 05:07:04 UTC
This fixes it.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2e73f3515bb..6fb87d5f49f 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -12539,6 +12539,10 @@ aarch64_override_options_internal (struct gcc_options *opts)
   if (opts->x_flag_strict_volatile_bitfields < 0 && abi_version_at_least (2))
     opts->x_flag_strict_volatile_bitfields = 1;
 
+  /* defaults to flag_no_common unless disabled with fcommon. */
+  if (!global_options_set.x_flag_no_common)
+      flag_no_common = 1;
+
   if (aarch64_stack_protector_guard == SSP_GLOBAL
       && opts->x_aarch64_stack_protector_guard_offset_str)
     {
Comment 10 Andrew Pinski 2019-10-24 05:11:39 UTC
(In reply to Kamlesh Kumar from comment #9)
> This fixes it.
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 2e73f3515bb..6fb87d5f49f 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -12539,6 +12539,10 @@ aarch64_override_options_internal (struct
> gcc_options *opts)
>    if (opts->x_flag_strict_volatile_bitfields < 0 && abi_version_at_least
> (2))
>      opts->x_flag_strict_volatile_bitfields = 1;
>  
> +  /* defaults to flag_no_common unless disabled with fcommon. */
> +  if (!global_options_set.x_flag_no_common)
> +      flag_no_common = 1;
> +
>    if (aarch64_stack_protector_guard == SSP_GLOBAL
>        && opts->x_aarch64_stack_protector_guard_offset_str)
>      {

This should be a global change and not just an aarch64 change.  The reason is because then aarch64 is the odd man out when it comes to this.
Comment 11 Kamlesh Kumar 2019-10-24 11:46:06 UTC
is this good?

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 0fffe60..9fead56 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -814,6 +814,10 @@ c_common_post_options (const char **pfilename)
       && flag_unsafe_math_optimizations == 0)
     flag_fp_contract_mode = FP_CONTRACT_OFF;

+  /* defaults to flag_no_common unless disabled with fcommon. */
+  if (!global_options_set.x_flag_no_common)
+      flag_no_common = 1;
+
   /* If we are compiling C, and we are outside of a standards mode,
      we can permit the new values from ISO/IEC TS 18661-3 for
      FLT_EVAL_METHOD.  Otherwise, we must restrict the possible values to
Comment 12 Wilco 2019-10-24 11:55:32 UTC
(In reply to Andrew Pinski from comment #10)

> This should be a global change and not just an aarch64 change.  The reason
> is because then aarch64 is the odd man out when it comes to this.

Agreed, see https://gcc.gnu.org/ml/gcc-patches/2017-11/msg01549.html. It would be great to sort that out so C and C++ finally address globals identically.
Comment 13 Wilco 2019-10-25 15:48:21 UTC
(In reply to Wilco from comment #12)
> (In reply to Andrew Pinski from comment #10)
> 
> > This should be a global change and not just an aarch64 change.  The reason
> > is because then aarch64 is the odd man out when it comes to this.
> 
> Agreed, see https://gcc.gnu.org/ml/gcc-patches/2017-11/msg01549.html. It
> would be great to sort that out so C and C++ finally address globals
> identically.

Patch: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html
Comment 14 Andrew Pinski 2021-09-17 00:20:05 UTC
-fno-common became the default with GCC 10 so closing as fixed.