On Tue, Apr 8, 2025 at 6:59 PM Ard Biesheuvel <a...@kernel.org> wrote:
>
> On Tue, 8 Apr 2025 at 18:44, H.J. Lu <hjl.to...@gmail.com> wrote:
> >
> > On Tue, Apr 8, 2025 at 9:39 AM Ard Biesheuvel <a...@kernel.org> wrote:
> > >
> > > On Tue, 8 Apr 2025 at 15:33, H.J. Lu <hjl.to...@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 8, 2025 at 3:46 AM Ard Biesheuvel <ardb+...@google.com>
> > > > wrote:
> > > > >
> > > > > From: Ard Biesheuvel <a...@kernel.org>
> > > > >
> > > > > Commit bde21de1205 ("i386: Honour -mdirect-extern-access when calling
> > > > > __fentry__") updated the logic that emits mcount() / __fentry__()
> > > > > calls
> > > > > into function prologues when profiling is enabled, to avoid GOT-based
> > > > > indirect calls when a direct call would suffice.
> > > > >
> > > > > There are two problems with that change:
> > > > > - it relies on -mdirect-extern-access rather than -fno-plt to decide
> > > > > whether or not a direct [PLT based] call is appropriate;
> > > > > - for the PLT case, it falls through to x86_print_call_or_nop(), which
> > > > > does not emit the @PLT suffix, resulting in the wrong relocation to
> > > > > be
> > > > > used (R_X86_64_PC32 instead of R_X86_64_PLT32)
> > > > >
> > > > > Fix this by testing flag_plt instead of ix86_direct_extern_access, and
> > > > > updating x86_print_call_or_nop() to take flag_pic and flag_plt into
> > > > > account. This also ensures that -mnop-mcount works as expected when
> > > > > emitting the PLT based profiling calls.
> > > > >
> > > > > Note that only 64-bit codegen is affected by this change or by the
> > > > > commit referenced above; -m32 will yield 'call *mcount@GOT()' as
> > > > > before.
> > > > >
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119386
> > > > >
> > > > > Signed-off-by: Ard Biesheuvel <a...@kernel.org>
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > PR target/119386
> > > > > * config/i386/i386.cc (x86_print_call_or_nop): Add @PLT suffix
> > > > > where appropriate.
> > > > > (x86_function_profiler): Fall through to
> > > > > x86_print_call_or_nop()
> > > > > for PIC codegen when flag_plt is set.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > > PR target/119386
> > > > > * gcc.target/i386/pr119386-1.c: New test.
> > > > > * gcc.target/i386/pr119386-2.c: New test.
> > > > > ---
> > > > > gcc/config/i386/i386.cc | 8 +++++++-
> > > > > gcc/testsuite/gcc.target/i386/pr119386-1.c | 11 +++++++++++
> > > > > gcc/testsuite/gcc.target/i386/pr119386-2.c | 11 +++++++++++
> > > > > 3 files changed, 29 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> > > > > index be5e27fc391..0b238c3dddc 100644
> > > > > --- a/gcc/config/i386/i386.cc
> > > > > +++ b/gcc/config/i386/i386.cc
> > > > > @@ -23154,6 +23154,12 @@ x86_print_call_or_nop (FILE *file, const
> > > > > char *target)
> > > > > if (flag_nop_mcount || !strcmp (target, "nop"))
> > > > > /* 5 byte nop: nopl 0(%[re]ax,%[re]ax,1) */
> > > > > fprintf (file, "1:" ASM_BYTE "0x0f, 0x1f, 0x44, 0x00, 0x00\n");
> > > > > + else if (!TARGET_PECOFF && flag_pic)
> > > > > + {
> > > > > + gcc_assert (flag_plt);
> > > > > +
> > > > > + fprintf (file, "1:\tcall\t%s@PLT\n", target);
> > > > > + }
> > > > > else
> > > > > fprintf (file, "1:\tcall\t%s\n", target);
> > > > > }
> > > > > @@ -23317,7 +23323,7 @@ x86_function_profiler (FILE *file, int
> > > > > labelno ATTRIBUTE_UNUSED)
> > > > > break;
> > > > > case CM_SMALL_PIC:
> > > > > case CM_MEDIUM_PIC:
> > > > > - if (!ix86_direct_extern_access)
> > > > > + if (!flag_plt)
> > > > > {
> > > > > if (ASSEMBLER_DIALECT == ASM_INTEL)
> > > > > fprintf (file, "1:\tcall\t[QWORD PTR
> > > > > %s@GOTPCREL[rip]]\n",
> > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr119386-1.c
> > > > > b/gcc/testsuite/gcc.target/i386/pr119386-1.c
> > > > > new file mode 100644
> > > > > index 00000000000..7930fc6f28d
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/gcc.target/i386/pr119386-1.c
> > > > > @@ -0,0 +1,11 @@
> > > > > +/* PR target/119386 */
> > > > > +/* { dg-do compile { target *-*-linux* } } */
> > > > > +/* { dg-require-effective-target lp64 } */
> > > >
> > > > Can this be dropped?
> > > >
> > >
> > > I copied that from another test, assuming it would limit the testing
> > > to x86_64. Is there a better way to achieve that?
> >
> > -m32 should also generate "call mcount@PLT".
> >
> > >
> > >
> > > > > +/* { dg-options "-O2 -fpic -pg" } */
> > > > > +/* { dg-final { scan-assembler "mcount@PLT" } } */
> > > > > +
> > > > > +int
> > > > > +main ()
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr119386-2.c
> > > > > b/gcc/testsuite/gcc.target/i386/pr119386-2.c
> > > > > new file mode 100644
> > > > > index 00000000000..6334b9b9072
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/gcc.target/i386/pr119386-2.c
> > > > > @@ -0,0 +1,11 @@
> > > > > +/* PR target/119386 */
> > > > > +/* { dg-do compile { target *-*-linux* } } */
> > > > > +/* { dg-require-effective-target lp64 } */
> > > >
> > > > Can this be dropped?
> > > >
> > > > > +/* { dg-options "-O2 -fpic -fno-plt -pg" } */
> > > > > +/* { dg-final { scan-assembler "mcount@GOTPCREL" } } */
> > > >
> > > > Different scans for ia32 and ! ia32?
> > > >
> > >
> > > I did not consider IA32 at all - can we just omit it?
> >
> > Unless -m32 support is removed from GCC, we should support it.
> >
>
> Fair enough.
>
> Could you perhaps be more specific on how you want to see this
> changed? I am not a regular GCC contributor, and so my guesses are
> likely to be wrong.
Please try attached.
BTW: You can test single testcases (for x86_64 and i686) with:
make -k check-gcc RUNTESTFLAGS="--target_board=unix\{,-m32\}
i386.exp=pr119386-1.c"
Uros.
pr119386.tar.bz2
Description: BZip2 compressed data
