diff mbox

PR79066, non-PIC code generated for powerpc glibc with -fpic

Message ID 20170116052001.GE32333@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Jan. 16, 2017, 5:20 a.m. UTC 
On Sat, Jan 14, 2017 at 09:19:52AM -0600, Segher Boessenkool wrote:
> On Sun, Jan 15, 2017 at 12:08:39AM +1030, Alan Modra wrote:
> > > Have you checked if/what this changes for some bigger code?
> > 
> > Well, the bootstrap was all langs (minus ada due to not having ada
> > installed) and ppc32 multilibs were built.  Plus the testsuite run
> > with RUNTESTFLAGS="--target_board=unix'{-m32,-m64}{-mlra,-mno-lra}'"
> > 
> > I would have bootstrapped -m32 except the machine I used lacked 32-bit
> > gmp, mpfr, mpc and I was lazy.
> > 
> > > Okay for trunk if there is nothing unexpected.  Thanks!
> > 
> > I guess I should at least build glibc.
> 
> Yes exactly, something big that uses pic -- it is pretty obvious it won't
> change anything for non-pic.

glibc built fine without elf_high/elf_low with no regressions in its
testsuite.  However, there is a problem in rs6000_emit_allocate_stack
-fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
to use the option with -fpic/PIC.  I reckon the option combination to
be little used, so it shouldn't hurt to disable -fstack-limit-symbol
for PIC.  (We were generating non-PIC for the trap, so we probably
would have gotten a complaint about text relocs in shared libraries.)

This revised patch has been bootstrapped and regression tested as
before, and tested with glibc too.  OK?

	PR target/79066
	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
	symbolic stack limit when pic.
testsuite/
	* gcc.target/powerpc/pr79066.c: New.

Comments

Segher Boessenkool Jan. 16, 2017, 7:49 p.m. UTC | #1 
On Mon, Jan 16, 2017 at 03:50:01PM +1030, Alan Modra wrote:
> > > > Okay for trunk if there is nothing unexpected.  Thanks!
> > > 
> > > I guess I should at least build glibc.
> > 
> > Yes exactly, something big that uses pic -- it is pretty obvious it won't
> > change anything for non-pic.
> 
> glibc built fine without elf_high/elf_low with no regressions in its
> testsuite.

I meant comparing the generated code, sorry.

> However, there is a problem in rs6000_emit_allocate_stack
> -fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
> to use the option with -fpic/PIC.  I reckon the option combination to
> be little used, so it shouldn't hurt to disable -fstack-limit-symbol
> for PIC.  (We were generating non-PIC for the trap, so we probably
> would have gotten a complaint about text relocs in shared libraries.)
> 
> This revised patch has been bootstrapped and regression tested as
> before, and tested with glibc too.  OK?
> 
> 	PR target/79066
> 	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
> 	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
> 	symbolic stack limit when pic.
> testsuite/
> 	* gcc.target/powerpc/pr79066.c: New.
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 11394b2..2dd6bbe 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -27668,7 +27668,8 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
>  	}
>        else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
>  	       && TARGET_32BIT
> -	       && DEFAULT_ABI == ABI_V4)
> +	       && DEFAULT_ABI == ABI_V4
> +	       && !flag_pic)
>  	{
>  	  rtx toload = gen_rtx_CONST (VOIDmode,
>  				      gen_rtx_PLUS (Pmode,

Please at least make it sorry() for ABI_V4 && flag_pic.  Or what does it
result in with the patch as-is?

Okay with that change (if needed).  Thanks!


Segher
Alan Modra Jan. 17, 2017, 2:52 a.m. UTC | #2 
On Mon, Jan 16, 2017 at 01:49:36PM -0600, Segher Boessenkool wrote:
> On Mon, Jan 16, 2017 at 03:50:01PM +1030, Alan Modra wrote:
> > > > > Okay for trunk if there is nothing unexpected.  Thanks!
> > > > 
> > > > I guess I should at least build glibc.
> > > 
> > > Yes exactly, something big that uses pic -- it is pretty obvious it won't
> > > change anything for non-pic.
> > 
> > glibc built fine without elf_high/elf_low with no regressions in its
> > testsuite.
> 
> I meant comparing the generated code, sorry.

libc.so had differences in debug sections due to libgcc source paths
being different, but other than that there were no differences.

> > However, there is a problem in rs6000_emit_allocate_stack
> > -fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
> > to use the option with -fpic/PIC.  I reckon the option combination to
> > be little used, so it shouldn't hurt to disable -fstack-limit-symbol
> > for PIC.  (We were generating non-PIC for the trap, so we probably
> > would have gotten a complaint about text relocs in shared libraries.)
> > 
> > This revised patch has been bootstrapped and regression tested as
> > before, and tested with glibc too.  OK?
> > 
> > 	PR target/79066
> > 	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
> > 	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
> > 	symbolic stack limit when pic.
> > testsuite/
> > 	* gcc.target/powerpc/pr79066.c: New.
> > 
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index 11394b2..2dd6bbe 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -27668,7 +27668,8 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
> >  	}
> >        else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
> >  	       && TARGET_32BIT
> > -	       && DEFAULT_ABI == ABI_V4)
> > +	       && DEFAULT_ABI == ABI_V4
> > +	       && !flag_pic)
> >  	{
> >  	  rtx toload = gen_rtx_CONST (VOIDmode,
> >  				      gen_rtx_PLUS (Pmode,
> 
> Please at least make it sorry() for ABI_V4 && flag_pic.  Or what does it
> result in with the patch as-is?

I think we're OK as is.  A warning is emitted "stack limit expression
is not supported".
diff mbox

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 11394b2..2dd6bbe 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -27668,7 +27668,8 @@  rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
 	}
       else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
 	       && TARGET_32BIT
-	       && DEFAULT_ABI == ABI_V4)
+	       && DEFAULT_ABI == ABI_V4
+	       && !flag_pic)
 	{
 	  rtx toload = gen_rtx_CONST (VOIDmode,
 				      gen_rtx_PLUS (Pmode,
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index b333474..f00334a 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -10581,14 +10581,14 @@  (define_insn_and_split "*tocref<mode>"
 (define_insn "elf_high"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=b*r")
 	(high:SI (match_operand 1 "" "")))]
-  "TARGET_ELF && ! TARGET_64BIT"
+  "TARGET_ELF && !TARGET_64BIT && !flag_pic"
   "lis %0,%1@ha")
 
 (define_insn "elf_low"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
 	(lo_sum:SI (match_operand:SI 1 "gpc_reg_operand" "b")
 		   (match_operand 2 "" "")))]
-   "TARGET_ELF && ! TARGET_64BIT"
+   "TARGET_ELF && !TARGET_64BIT && !flag_pic"
    "la %0,%2@l(%1)")
 
 ;; Call and call_value insns
diff --git a/gcc/testsuite/gcc.target/powerpc/pr79066.c b/gcc/testsuite/gcc.target/powerpc/pr79066.c
new file mode 100644
index 0000000..86b2014
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr79066.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile { target { fpic && ilp32 } } } */
+/* { dg-options "-O2 -fpic" } */
+/* { dg-final { scan-assembler-not "lis.*@ha" } } */
+
+union U { double x; int i[2]; };
+
+double
+foo (double x)
+{
+  union U v;
+  v.i[0] = 0x7ff00000;
+  v.i[1] = 0;
+  return x / v.x;
+}