Re: [PATCH] AArch64: Add support for -mdirect-extern-access

[Fangrui Song <maskray@google.com>]

On Thu, Nov 17, 2022 at 1:55 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Thu, Nov 17, 2022 at 1:46 PM Fangrui Song <maskray@google.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 1:37 PM Andrew Pinski <pinskia@gmail.com> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 1:21 PM maskray--- via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > >
> > > > > +.. 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.
> > > >
> > > > I think current GCC and Clang's behavior is:
> > > >
> > > > * -mdirect-extern-access is the default for -fno-pic. This is to enable optimizations for -static programs but may introduce copy relocations.
> > > > * -mno-direct-extern-access is the default for -fpie and -fpic. This uses some GOT-generating relocations which can be optimized out (lld, see https://maskray.me/blog/2021-08-29-all-about-global-offset-table) but the instruction is nevertheless slightly longer.
> > > >
> > > > (-mdirect-extern-access for -fpic probably doesn't make sense.)
> > > >
> > > > The option I introduced to Clang is -fdirect-access-external-data
> > > > (see https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected).
> > > > If -mdirect-extern-access gets more popular, I can add a Clang alias.
> > > > But I am opposed to forcing a GNU property for -mdirect-extern-access/-mno-direct-extern-access.
> > > >
> > > > FWIW I used https://gist.github.com/MaskRay/c03a90922003df666551589f1629df22 to test my Clang changes related to -fno-semantic-interposition
> > > > on various visibility attributes x non-weak/weak x nopic/pie/pic x dllimport/not x ...
> > >
> > >
> > > The x86_64 discussion about this is here
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98112 .
> > > I think clang changing the ABI is just broken and should think twice
> > > before we do it for GCC.
> > >
> > > And there is a lot of visibility protected issues filed in GCC bug
> > > databases specifically about copy relocs too.
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56527
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37611
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19520
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=28875
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=28877
> > > I also suspect clang's behavior is still broken too.
> > >
> > > Thanks,
> > > Andrew
> >
> > Well, I don't think Clang changed ABI regarding -fno-pic/-fpie/-fpic.
> > As I did archaeology on
> > https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected
> > "Reflection on protected data symbols and copy relocations"
> > GCC 5 x86-64 made a change and GCC aarch64 accidentally picked up the change.
>
> You missed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248 (or
> rather r5-7961-ga5eef8e9b02474 ) was the change to fix protected .

I didn't. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248 proposed a problem.
It could be resolved as "wontfix. copy relocations are just
incompatible with protected symbols as an optimization (which is the
very purpose of inventing protected)"
but was resolved by pessimizing GCC codegen. This led to heated
discussion in several places including
https://sourceware.org/legacy-ml/binutils/2016-03/msg00312.html (which
my article linked to).

>
> >
> > """
> > On the GCC side, in -fpic mode, using GOT-generating relocations when
> > accessing a protected variable subverts the point using the protected
> > visibility. The unneeded pessimization is the foremost complaint. The
> > pessimization applies to all ports with #define TARGET_BINDS_LOCAL_P
> > default_binds_local_p_2. aarch64 moved to default_binds_local_p_2
> > accidentally by
> > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cbddf64c0243816b45e6680754a251c603245dbc.
>
> This was NOT by accident. In fact you just looked into the commit and
> NOT the actually email which submitted the patch:
> https://gcc.gnu.org/legacy-ml/gcc-patches/2015-04/msg01432.html
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65780

This aarch64 commit was by accident. The code happened to change both
COMMON symbol and protected behaviors.
While the COMMON symbol behavior was intentional, the proteced symbol
behavior was not.

> "As s390/arm/aarch64 seems to work fine
> (generate a COPY relocation and thus define symbol locally) in non-PIE
> executables, this patch changes those to a function that has been added for
> that behavior."
>
>
> Thanks,
> Andrew Pinski
>
> >
> > For GCC<5 (and all versions of Clang), direct accesses to protected
> > variables are produced in -fpic code. Mixing such object files can
> > still silently break copy relocations on protected data symbols.
> > Therefore, GNU ld made the controversial change
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5
> > to error in -shared mode.
> > """
> >
> >
> > > >
> > > > On 2022-11-17, Ramana Radhakrishnan wrote:
> > > > >On Thu, Nov 17, 2022 at 5:30 PM Richard Sandiford via Gcc-patches
> > > > ><gcc-patches@gcc.gnu.org> wrote:
> > > > >>
> > > > >> Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> > > > >> > Hi Richard,
> > > > >> >
> > > > >> >> 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?
> > > > >> >
> > > > >> > Yes, the usage model is that you'd either use it for static PIE or only on
> > > > >> > data that is not shared. If you get it wrong them you'll get the copy
> > > > >> > relocation error.
> > > > >>
> > > > >> Thanks.  I think I'm still missing something though.  If, for the
> > > > >> non-executable case, people should only use the feature on data that
> > > > >> is not shared, why do we need to relax the binds-local condition for
> > > > >> protected symbols on -fPIC?  Oughtn't the symbol to be hidden rather
> > > > >> than protected if the data isn't shared?
> > > > >>
> > > > >> I can understand the reasoning for the PIE changes but I'm still
> > > > >> struggling with the PIC-but-not-PIE bits.
> > > > >
> > > > >I think I'm with Richard S on hidden vs protected on first reading. I
> > > > >can see why this works out of the box and can even be default for
> > > > >static-pie.
> > > > >
> > > > >Any reason why this is not on by default - it's early enough in the
> > > > >stage3 cycle and we can always flip the defaults if there are more
> > > > >problems found.
> > > > >
> > > > >You probably need a rebase for the documentation bits,.
> > > > >
> > > > >regards
> > > > >Ramana
> > > > >
> > > > >
> > > > >Ramana
> > > >
> > > >
> > > > +  is :option:`-mno-direct-extern-access`.
> >
> >
> >
> > --
> > 宋方睿



-- 
宋方睿