[Bug target/101324] New: powerpc64le: hashst appears before mflr at -O1 or higher

[Bug target/101324] New: powerpc64le: hashst appears before mflr at -O1 or higher

[tuliom at ascii dot art.br]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

            Bug ID: 101324
           Summary: powerpc64le: hashst appears before mflr at -O1 or
                    higher
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tuliom at ascii dot art.br
  Target Milestone: ---

Created attachment 51105
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51105&action=edit
__memmove_ppc extracted from glibc

When using ROP (-mrop-protect), it's expected that generated code reads the
value from LR (mflr) and hash it later (hashst).

This works well at -O0.
However, at -O1 and higher, we're seeing cases where hashst appears before
mflr.

I'm attaching an example extracted from glibc.

You can reproduce the issue with command:
gcc -S -O1 -mrop-protect -mcpu=power10 memmove-ppc64.i -o -


The generated asm contains the following:

__memmove_ppc:
.LFB6:
        .cfi_startproc
        .localentry     __memmove_ppc,1
        hashst 0,-40(1)
        std 28,-32(1)
        stdu 1,-80(1)
        .cfi_def_cfa_offset 80
        .cfi_offset 28, -32
        mr 28,3
        subf 9,4,3
        cmpld 0,9,5
        bge 0,.L17
        std 31,72(1)
        .cfi_offset 31, -8
        add 4,4,5
        add 31,3,5
        cmpldi 0,5,15
        ble 0,.L4
        mflr 0
...

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #1 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Could you attach something that is a valid input to the compiler?  Something
that does not include the preprocessor directives...  did you use -dM?
Don't :-)

(I can reproduce with this code, but there are many warnings :-) )

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #2 from Segher Boessenkool <segher at gcc dot gnu.org> ---
So, even if I use
  -fno-shrink-wrap
and, although rs6000.c has anyway
  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
  if (rs6000_rop_protect)
    flag_shrink_wrap = 0;
we *still* get shrink-wrapping done here?!  But that cannot work with the
ROP protection stuff.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mliska at suse dot cz

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Cc:ing Martin because this is options stuff.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[tuliom at ascii dot art.br]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Tulio Magno Quites Machado Filho <tuliom at ascii dot art.br> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51105|0                           |1
        is obsolete|                            |

--- Comment #4 from Tulio Magno Quites Machado Filho <tuliom at ascii dot art.br> ---
Created attachment 51109
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51109&action=edit
__memmove_ppc extracted from glibc (without -g3)

I removed -g3 from the command that generated memmove-ppc64.i.
I think this won't generate warnings now.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|mliska at suse dot cz              |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2021-07-12

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
Mine then.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
Heh, another example of __attribute__((optimize("..."))) problem:

__attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
__memmove_ppc ( void *dest, const void *src, size_t len)
{
...
}

-mrop-protect is dropped when optimize attribute is parsed, for more
information see:
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577113.html

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|marxin at gcc dot gnu.org          |unassigned at gcc dot gnu.org

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #7 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #6)
> Heh, another example of __attribute__((optimize("..."))) problem:
> 
> __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
> __memmove_ppc ( void *dest, const void *src, size_t len)
> {
> ...
> }
> 
> -mrop-protect is dropped when optimize attribute is parsed, for more
> information see:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577113.html

So what is the status with this?  IIUC, looking at the thread, it seems like
you and richi agreed that the optimize flags should be treated as if they were
appended to the command line, but that doesn't seem how trunk works right now. 
So does GCC still need fixing or is the test case using the attribute optimize
incorrectly or ???

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #8 from Peter Bergner <bergner at gcc dot gnu.org> ---
FYI, here's a smaller test case that still shows the issue with today's trunk:

extern void foo (void);
long int
__attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
__memmove_ppc (long int cond)
{
  if (cond)
    foo ();
  return cond;
}

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #9 from Martin Liška <marxin at gcc dot gnu.org> ---
> So what is the status with this?  IIUC, looking at the thread, it seems like
> you and richi agreed that the optimize flags should be treated as if they
> were appended to the command line, but that doesn't seem how trunk works
> right now.  So does GCC still need fixing or is the test case using the
> attribute optimize incorrectly or ???

The issue is still present and I can reproduce it. You are right, the flags are
treated as if they were appended to the command line. We need a better place
where the -mrop-protect is handled, let me take a look.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #8)
> FYI, here's a smaller test case that still shows the issue with today's
> trunk:
> 
> extern void foo (void);
> long int
> __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
> __memmove_ppc (long int cond)
> {
>   if (cond)
>     foo ();
>   return cond;
> }

All right, I have a patch candidate. Would it be possible to prepare a GCC
test-suite test-case?

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #11 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #10)
> (In reply to Peter Bergner from comment #8)
> > FYI, here's a smaller test case that still shows the issue with today's
> > trunk:
> > 
> > extern void foo (void);
> > long int
> > __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
> > __memmove_ppc (long int cond)
> > {
> >   if (cond)
> >     foo ();
> >   return cond;
> > }
> 
> All right, I have a patch candidate. Would it be possible to prepare a GCC
> test-suite test-case?

Great!  I'm not sure we can detect the assembly directly, but I think a
runnable test case should work.  I'll work on that.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #12 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #10)
> All right, I have a patch candidate. Would it be possible to prepare a GCC
> test-suite test-case?

Forgot to mention, if you want me to test your patch, I can do that on a
POWER10 system to ensure the -mrop-protect stuff is working.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #13 from Martin Liška <marxin at gcc dot gnu.org> ---
Created attachment 51668
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51668&action=edit
Patch candidate

Please test the patch on a power10 machine, thanks.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #14 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #13)
> Created attachment 51668 [details]
> Patch candidate
> 
> Please test the patch on a power10 machine, thanks.

I'll kick it off now.  Thanks!

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #15 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #14)
> (In reply to Martin Liška from comment #13)
> > Created attachment 51668 [details]
> > Patch candidate
> > 
> > Please test the patch on a power10 machine, thanks.
> 
> I'll kick it off now.  Thanks!

This patch is okay for trunk if you (Peter) are happy with it, and it works ;-)

Thank guys!

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #16 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #15)
> (In reply to Peter Bergner from comment #14)
> > (In reply to Martin Liška from comment #13)
> > > Please test the patch on a power10 machine, thanks.
> > 
> > I'll kick it off now.  Thanks!
> 
> This patch is okay for trunk if you (Peter) are happy with it, and it works

The patch bootstrapped and regtested with no regressions.  I have also
conformed it fixes the issue, so I think this is good to commit.

I'm having trouble with the runnable test case, so let's push the fix now and
I'll submit the test case when I get it working?

Thanks for the fix Martin!

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #17 from Peter Bergner <bergner at gcc dot gnu.org> ---
Here's a compile time only test case that correctly FAILs using the unpatched
compiler and passes using the patched compiler (requires a small change to
target-supports.exp to add the rop_ok test):

/* { dg-require-effective-target rop_ok } */
/* { dg-options "-O1 -mrop-protect -mdejagnu-cpu=power10" } */

extern void foo (void);

long int
__attribute__ ((__optimize__ ("no-inline")))
func (long int cond)
{
  if (cond)
    foo ();
  return cond;
}

/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1).  */
/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */
/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #18 from Peter Bergner <bergner at gcc dot gnu.org> ---
Namely this:

diff --git a/gcc/testsuite/lib/target-supports.exp
b/gcc/testsuite/lib/target-supports.exp
index 1c8b1ebb86e..0d9a3ba67ce 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -6625,6 +6625,13 @@ proc check_effective_target_powerpc_elfv2 { } {
     }
 }

+# Return 1 if this is a PowerPC target supporting -mrop-protect
+
+proc check_effective_target_rop_ok { } {
+    return [check_effective_target_power10_ok]
+           && [check_effective_target_powerpc_elfv2]
+}
+
 # The VxWorks SPARC simulator accepts only EM_SPARC executables and
 # chokes on EM_SPARC32PLUS or EM_SPARCV9 executables.  Return 1 if the
 # test environment appears to run executables on such a simulator.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2021-October
                   |                            |/582755.html

--- Comment #19 from Peter Bergner <bergner at gcc dot gnu.org> ---
I posted Martin's already approved patch to the gcc-patches mailing list along
with a test case which need approval.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>:

https://gcc.gnu.org/g:cff7879a381d3f5ed6556206896e6a6229800167

commit r12-5781-gcff7879a381d3f5ed6556206896e6a6229800167
Author: Martin Liska <mliska@suse.cz>
Date:   Fri Dec 3 11:58:25 2021 -0600

    rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect
[PR101324]

    PR101324 shows a problem in disabling shrink-wrapping when using
-mrop-protect
    when there is a attribute optimize/pragma.  The fix envolves moving the
handling
    of flag_shrink_wrap so it gets re-disbled when we change or add options.

    2021-12-03  Martin Liska  <mliska@suse.cz>

    gcc/
            PR target/101324
            * config/rs6000/rs6000.c (rs6000_option_override_internal): Move
the
            disabling of shrink-wrapping when using -mrop-protect from here...
            (rs6000_override_options_after_change): ...to here.

    2021-12-03  Peter Bergner  <bergner@linux.ibm.com>

    gcc/testsuite/
            PR target/101324
            * gcc.target/powerpc/pr101324.c: New test.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #21 from Peter Bergner <bergner at gcc dot gnu.org> ---
Fixed on trunk.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |11.0
   Target Milestone|12.0                        |11.4

--- Comment #22 from Peter Bergner <bergner at gcc dot gnu.org> ---
So this is also broken in GCC11, so I'm testing the simple backport.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #23 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #22)
> So this is also broken in GCC11, so I'm testing the simple backport.

Regression testing of the backport was clean.  Just need approval for the
backport.

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[raoni at linux dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #24 from Raoni Fassina Firmino <raoni at linux dot ibm.com> ---
(In reply to Peter Bergner from comment #21)
> Fixed on trunk.

I tested gcc trunk with glibc master and I confirm that it fix the problem with
__memmove_ppc. I tested both running glibc check with a custom kernel with rop
enable and doing a visual check of the disassembled function.

The exact versions I used were:
- binutils 2.36.1 build from binutils-2_36_1 (f35674005e60)
- gcc (future 12) build from master (b7815aa9108)
- glibc (future 2.35) build from master (268d812c19)

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

--- Comment #25 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Peter Bergner
<bergner@gcc.gnu.org>:

https://gcc.gnu.org/g:c56c398c39f6195c3d158f02514c33b7da73ebc2

commit r11-9560-gc56c398c39f6195c3d158f02514c33b7da73ebc2
Author: Martin Liska <mliska@suse.cz>
Date:   Fri Dec 3 11:58:25 2021 -0600

    rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect
[PR101324]

    PR101324 shows a problem in disabling shrink-wrapping when using
-mrop-protect
    when there is a attribute optimize/pragma.  The fix envolves moving the
handling
    of flag_shrink_wrap so it gets re-disbled when we change or add options.

    2021-12-03  Martin Liska  <mliska@suse.cz>

    gcc/
            PR target/101324
            * config/rs6000/rs6000.c (rs6000_option_override_internal): Move
the
            disabling of shrink-wrapping when using -mrop-protect from here...
            (rs6000_override_options_after_change): ...to here.

    2021-12-03  Peter Bergner  <bergner@linux.ibm.com>

    gcc/testsuite/
            PR target/101324
            * gcc.target/powerpc/pr101324.c: New test.

    (cherry picked from commit cff7879a381d3f5ed6556206896e6a6229800167)

[Bug target/101324] powerpc64le: hashst appears before mflr at -O1 or higher

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101324

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #26 from Peter Bergner <bergner at gcc dot gnu.org> ---
Fixed everywhere now.