From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id BA652382EF20 for ; Wed, 16 Nov 2022 10:54:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BA652382EF20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B665E1477; Wed, 16 Nov 2022 02:54:56 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B56C83F663; Wed, 16 Nov 2022 02:54:49 -0800 (PST) From: Richard Sandiford To: Wilco Dijkstra Mail-Followup-To: Wilco Dijkstra ,GCC Patches , Kyrylo Tkachov , "maskray\@google.com" , richard.sandiford@arm.com Cc: GCC Patches , Kyrylo Tkachov , "maskray\@google.com" Subject: Re: [PATCH] AArch64: Add support for -mdirect-extern-access References: Date: Wed, 16 Nov 2022 10:54:48 +0000 In-Reply-To: (Wilco Dijkstra's message of "Fri, 11 Nov 2022 14:48:24 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-39.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,KAM_SHORT,KAM_STOCKGEN,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Wilco Dijkstra writes: > Add a new option -mdirect-extern-access similar to other targets. This removes > GOT indirections on external symbols with -fPIE, resulting in significantly > better code quality. With -fPIC it only affects protected symbols, allowing > for more efficient shared libraries which can be linked with standard PIE > binaries (this is what LLVM does by default, so this improves interoperability > with LLVM). This patch doesn't affect ABI, but in the future GCC and LLVM > should converge to using the same ABI. Can you go into more detail about: Use :option:`-mdirect-extern-access` either in shared libraries or in executables, but not in both. Protected symbols used both in a shared library and executable may cause linker errors or fail to work correctly If this is LLVM's default for PIC (and by assumption shared libraries), is it then invalid to use -mdirect-extern-access for any PIEs that are linked against those shared libraries and use protected symbols from those libraries? How would a user know that one of the shared libraries they're linking against was built in this way? It looks like the main difference between this implementation and the x86 one is that x86 allows direct accesses to common symbols. What's the reason for not doing that for AArch64? Does it not work, is it a false optimisation (i.e. pessimisation), or did it not seem important now that -fno-common is the default? Thanks, Richard > > Regress and bootstrap pass, OK for commit? > > gcc/ > * config/aarch64/aarch64.cc (aarch64_binds_local_p): New function. > (aarch64_symbol_binds_local_p): Refactor, support direct extern access. > * config/aarch64/aarch64-linux.h (TARGET_BINDS_LOCAL_P): > Use aarch64_binds_local_p. > * config/aarch64/aarch64-freebsd.h (TARGET_BINDS_LOCAL_P): Likewise. > * config/aarch64/aarch64-protos.h: Add aarch64_binds_local_p. > * doc/gcc/gcc-command-options/option-summary.rst: Add > -mdirect-extern-access. > * doc/gcc/gcc-command-options/machine-dependent-options/aarch64-options.rst: > Add description of -mdirect-extern-access. > > gcc/testsuite/ > * gcc.target/aarch64/pr66912-2.c: New test. > > --- > > diff --git a/gcc/config/aarch64/aarch64-freebsd.h b/gcc/config/aarch64/aarch64-freebsd.h > index 13beb3781b61afd82d767884f3c16ff8eead09cc..20bc0f48e484686cd3754613bf20bb3521079d48 100644 > --- a/gcc/config/aarch64/aarch64-freebsd.h > +++ b/gcc/config/aarch64/aarch64-freebsd.h > @@ -71,7 +71,7 @@ > strong definitions in dependent shared libraries, will resolve > to COPY relocated symbol in the executable. See PR65780. */ > #undef TARGET_BINDS_LOCAL_P > -#define TARGET_BINDS_LOCAL_P default_binds_local_p_2 > +#define TARGET_BINDS_LOCAL_P aarch64_binds_local_p > > /* Use the AAPCS type for wchar_t, override the one from > config/freebsd.h. */ > diff --git a/gcc/config/aarch64/aarch64-linux.h b/gcc/config/aarch64/aarch64-linux.h > index 5e4553d79f5053f2da0eb381e0805f47aec964ae..6c962402155d60b82610d4f65af5182d6faa47ad 100644 > --- a/gcc/config/aarch64/aarch64-linux.h > +++ b/gcc/config/aarch64/aarch64-linux.h > @@ -70,7 +70,7 @@ > strong definitions in dependent shared libraries, will resolve > to COPY relocated symbol in the executable. See PR65780. */ > #undef TARGET_BINDS_LOCAL_P > -#define TARGET_BINDS_LOCAL_P default_binds_local_p_2 > +#define TARGET_BINDS_LOCAL_P aarch64_binds_local_p > > /* Define this to be nonzero if static stack checking is supported. */ > #define STACK_CHECK_STATIC_BUILTIN 1 > diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h > index 238820581c5ee7617f8eed1df2cf5418b1127e19..fac754f78c1d7606ba90e1034820a62466b96b63 100644 > --- a/gcc/config/aarch64/aarch64-protos.h > +++ b/gcc/config/aarch64/aarch64-protos.h > @@ -1072,5 +1072,6 @@ const char *aarch64_sls_barrier (int); > const char *aarch64_indirect_call_asm (rtx); > extern bool aarch64_harden_sls_retbr_p (void); > extern bool aarch64_harden_sls_blr_p (void); > +extern bool aarch64_binds_local_p (const_tree); > > #endif /* GCC_AARCH64_PROTOS_H */ > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index d1f979ebcf80333d957f8ad8631deef47dc693a5..ab4c42c34da5b15f6739c9b0a7ebaafda9488f2d 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -19185,9 +19185,29 @@ aarch64_tlsdesc_abi_id () > static bool > aarch64_symbol_binds_local_p (const_rtx x) > { > - return (SYMBOL_REF_DECL (x) > - ? targetm.binds_local_p (SYMBOL_REF_DECL (x)) > - : SYMBOL_REF_LOCAL_P (x)); > + if (!SYMBOL_REF_DECL (x)) > + return SYMBOL_REF_LOCAL_P (x); > + > + if (targetm.binds_local_p (SYMBOL_REF_DECL (x))) > + return true; > + > + /* In PIE binaries avoid a GOT indirection on non-weak data symbols if > + aarch64_direct_extern_access is true. */ > + if (flag_pie && aarch64_direct_extern_access && !SYMBOL_REF_WEAK (x) > + && !SYMBOL_REF_FUNCTION_P (x)) > + return true; > + > + return false; > +} > + > +/* Implement TARGET_BINDS_LOCAL_P hook. */ > + > +bool > +aarch64_binds_local_p (const_tree exp) > +{ > + /* Protected symbols are local if aarch64_direct_extern_access is true. */ > + return default_binds_local_p_3 (exp, flag_shlib != 0, true, > + !aarch64_direct_extern_access, !flag_pic); > } > > /* Return true if SYMBOL_REF X is thread local */ > diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt > index b89b20450710592101b93f4f3b5dc33d152d1eb6..6251a36b544a03955361b445c9f5dfad3740eea8 100644 > --- a/gcc/config/aarch64/aarch64.opt > +++ b/gcc/config/aarch64/aarch64.opt > @@ -299,3 +299,7 @@ Constant memset size in bytes from which to start using MOPS sequence. > -param=aarch64-vect-unroll-limit= > Target Joined UInteger Var(aarch64_vect_unroll_limit) Init(4) Param > Limit how much the autovectorizer may unroll a loop. > + > +mdirect-extern-access > +Target Var(aarch64_direct_extern_access) Init(0) > +Do not indirect accesses to external symbols via the GOT. > diff --git a/gcc/doc/gcc/gcc-command-options/machine-dependent-options/aarch64-options.rst b/gcc/doc/gcc/gcc-command-options/machine-dependent-options/aarch64-options.rst > index c2b23a6ee97ef2b7c74119f22c1d3e3d85385f4d..599c37fe299dc142d25d2133a4cd0b861e34fd01 100644 > --- a/gcc/doc/gcc/gcc-command-options/machine-dependent-options/aarch64-options.rst > +++ b/gcc/doc/gcc/gcc-command-options/machine-dependent-options/aarch64-options.rst > @@ -389,6 +389,20 @@ These options are defined for AArch64 implementations: > The default is :samp:`-msve-vector-bits=scalable`, which produces > vector-length agnostic code. > > +.. option:: -mdirect-extern-access, -mno-direct-extern-access > + > + Use direct accesses for external data symbols. It avoids a GOT indirection > + on all external data symbols with :option:`-fpie` or :option:`-fPIE`. This is > + useful for executables linked with :option:`-static` or :option:`-static-pie`. > + With :option:`-fpic` or :option:`-fPIC`, it only affects accesses to protected > + data symbols. It has no effect on non-position independent code. The default > + is :option:`-mno-direct-extern-access`. > + > + .. warning:: > + > + Use :option:`-mdirect-extern-access` either in shared libraries or in > + executables, but not in both. Protected symbols used both in a shared > + library and executable may cause linker errors or fail to work correctly. > > .. _aarch64-feature-modifiers: > > diff --git a/gcc/doc/gcc/gcc-command-options/option-summary.rst b/gcc/doc/gcc/gcc-command-options/option-summary.rst > index d068f98feac27d95f1402a530a78b553d623d2e9..dbc9b45ae1db12737aca3a6fd246b88a0e9467c2 100644 > --- a/gcc/doc/gcc/gcc-command-options/option-summary.rst > +++ b/gcc/doc/gcc/gcc-command-options/option-summary.rst > @@ -634,7 +634,8 @@ in the following sections. > :option:`-moverride=string` :option:`-mverbose-cost-dump` |gol| > :option:`-mstack-protector-guard=guard` :option:`-mstack-protector-guard-reg=sysreg` |gol| > :option:`-mstack-protector-guard-offset=offset` :option:`-mtrack-speculation` |gol| > - :option:`-moutline-atomics` > + :option:`-moutline-atomics` |gol| > + :option:`-mdirect-extern-access` > > *Adapteva Epiphany Options* > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr66912-2.c b/gcc/testsuite/gcc.target/aarch64/pr66912-2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..58e6e1f37116bff77015a7321890ece30c9e6a5c > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr66912-2.c > @@ -0,0 +1,41 @@ > +/* { dg-do compile { target *-*-linux* } } */ > +/* { dg-options "-O2 -fpic" } */ > +/* { dg-require-effective-target fpic } */ > + > +__attribute__((visibility("protected"))) > +int n_common; > + > +__attribute__((weak, visibility("protected"))) > +int n_weak_common; > + > +__attribute__((visibility("protected"))) > +int n_init = -1; > + > +__attribute__((weak, visibility("protected"))) > +int n_weak_init = -1; > + > +int > +f1 () > +{ > + return n_common; > +} > + > +int > +f2 () > +{ > + return n_weak_common; > +} > + > +int > +f3 () > +{ > + return n_init; > +} > + > +int > +f4 () > +{ > + return n_weak_init; > +} > + > +/* { dg-final { scan-assembler-times ":got" 0 } } */