Patchwork Diagnose pr54694

login
register
mail settings
Submitter Richard Henderson
Date Jan. 15, 2014, 9:43 p.m.
Message ID <52D700F6.2060609@redhat.com>
Download mbox | patch
Permalink /patch/311277/
State New
Headers show

Comments

Richard Henderson - Jan. 15, 2014, 9:43 p.m.
On 01/15/2014 08:37 AM, H.J. Lu wrote:
> We should add a testcase to verify this.
> 

I included the following testcase with the commit.  I couldn't find a way
to test this properly generically, so I just went with the obvious i386 test.


r~
Jakub Jelinek - Jan. 15, 2014, 9:58 p.m.
On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr54694.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O" } */
> +
> +register void *hfp __asm__("%ebp");	/* { dg-message "note: for" } */

Shouldn't that be %rbp for x86_64?  Or do we treat it the same?

> +extern void g(void *);
> +
> +void f(int x)			/* { dg-error "frame pointer required" } */
> +{
> +  g(__builtin_alloca(x));
> +}


	Jakub
Marek Polacek - Jan. 15, 2014, 10:02 p.m.
On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
> On 01/15/2014 08:37 AM, H.J. Lu wrote:
> > We should add a testcase to verify this.
> > 
> 
> I included the following testcase with the commit.  I couldn't find a way
> to test this properly generically, so I just went with the obvious i386 test.

Seems like the test was missing a ChangeLog entry.  I've fixed it up.

	Marek
Richard Henderson - Jan. 16, 2014, 4:06 p.m.
On 01/15/2014 01:58 PM, Jakub Jelinek wrote:
> On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/pr54694.c
>> @@ -0,0 +1,11 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O" } */
>> +
>> +register void *hfp __asm__("%ebp");	/* { dg-message "note: for" } */
> 
> Shouldn't that be %rbp for x86_64?  Or do we treat it the same?

We treat it the same.


r~
Jakub Jelinek - Jan. 16, 2014, 5:35 p.m.
On Thu, Jan 16, 2014 at 08:06:07AM -0800, Richard Henderson wrote:
> On 01/15/2014 01:58 PM, Jakub Jelinek wrote:
> > On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/i386/pr54694.c
> >> @@ -0,0 +1,11 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O" } */
> >> +
> >> +register void *hfp __asm__("%ebp");	/* { dg-message "note: for" } */
> > 
> > Shouldn't that be %rbp for x86_64?  Or do we treat it the same?
> 
> We treat it the same.

BTW, your fix broke the gcc.target/i386/pr9771-1.c
test on i686-linux, the problem is that main normally dynamically realigns
the stack.  Wonder if the test should be turned into dg-do compile,
or perhaps a hack like:
int xmain() __asm__ ("main");
int xmain()
instead of
int main()
to avoid the dynamic stack realigning in main (limit the test to *linux*
then?), supply main written in assembly, something else?

	Jakub
H.J. Lu - Jan. 16, 2014, 5:40 p.m.
On Thu, Jan 16, 2014 at 9:35 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Jan 16, 2014 at 08:06:07AM -0800, Richard Henderson wrote:
>> On 01/15/2014 01:58 PM, Jakub Jelinek wrote:
>> > On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
>> >> --- /dev/null
>> >> +++ b/gcc/testsuite/gcc.target/i386/pr54694.c
>> >> @@ -0,0 +1,11 @@
>> >> +/* { dg-do compile } */
>> >> +/* { dg-options "-O" } */
>> >> +
>> >> +register void *hfp __asm__("%ebp");       /* { dg-message "note: for" } */
>> >
>> > Shouldn't that be %rbp for x86_64?  Or do we treat it the same?
>>
>> We treat it the same.
>
> BTW, your fix broke the gcc.target/i386/pr9771-1.c
> test on i686-linux, the problem is that main normally dynamically realigns
> the stack.  Wonder if the test should be turned into dg-do compile,
> or perhaps a hack like:
> int xmain() __asm__ ("main");
> int xmain()
> instead of
> int main()
> to avoid the dynamic stack realigning in main (limit the test to *linux*
> then?), supply main written in assembly, something else?
>
>         Jakub

gcc.target/i386/pr9771-1.c has

register long *B asm ("ebp");

It won't work without -maccumulate-outgoing-args.
Jakub Jelinek - Jan. 16, 2014, 5:43 p.m.
On Thu, Jan 16, 2014 at 09:40:33AM -0800, H.J. Lu wrote:
> On Thu, Jan 16, 2014 at 9:35 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Jan 16, 2014 at 08:06:07AM -0800, Richard Henderson wrote:
> >> On 01/15/2014 01:58 PM, Jakub Jelinek wrote:
> >> > On Wed, Jan 15, 2014 at 01:43:18PM -0800, Richard Henderson wrote:
> >> >> --- /dev/null
> >> >> +++ b/gcc/testsuite/gcc.target/i386/pr54694.c
> >> >> @@ -0,0 +1,11 @@
> >> >> +/* { dg-do compile } */
> >> >> +/* { dg-options "-O" } */
> >> >> +
> >> >> +register void *hfp __asm__("%ebp");       /* { dg-message "note: for" } */
> >> >
> >> > Shouldn't that be %rbp for x86_64?  Or do we treat it the same?
> >>
> >> We treat it the same.
> >
> > BTW, your fix broke the gcc.target/i386/pr9771-1.c
> > test on i686-linux, the problem is that main normally dynamically realigns
> > the stack.  Wonder if the test should be turned into dg-do compile,
> > or perhaps a hack like:
> > int xmain() __asm__ ("main");
> > int xmain()
> > instead of
> > int main()
> > to avoid the dynamic stack realigning in main (limit the test to *linux*
> > then?), supply main written in assembly, something else?
> >
> >         Jakub
> 
> gcc.target/i386/pr9771-1.c has
> 
> register long *B asm ("ebp");
> 
> It won't work without -maccumulate-outgoing-args.

Actually it does, with the above written hack for main it works
all of:
-m32 -O2 -fomit-frame-pointer -ffixed-ebp 
-m32 -O2 -fomit-frame-pointer -ffixed-ebp -maccumulate-outgoing-args
-m32 -O2 -fomit-frame-pointer -ffixed-ebp -mno-accumulate-outgoing-args

	Jakub
Richard Henderson - Jan. 16, 2014, 5:51 p.m.
On 01/16/2014 09:35 AM, Jakub Jelinek wrote:
> Wonder if the test should be turned into dg-do compile,
> or perhaps a hack like:
> int xmain() __asm__ ("main");
> int xmain()
> instead of
> int main()
> to avoid the dynamic stack realigning in main (limit the test to *linux*
> then?), supply main written in assembly, something else?


The __asm__ hack seems reasonable, although you'll also have to deal
with __USER_LABEL_PREFIX__.


r~

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c93bf23..075582a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@ 
+2014-01-15  Richard Henderson <rth@redhat.com>
+
+	PR debug/54694
+	* reginfo.c (global_regs_decl): Globalize.
+	* rtl.h (global_regs_decl): Declare.
+	* ira.c (do_reload): Diagnose frame_pointer_needed and it
+	reserved via global_regs.
+
 2014-01-15  Teresa Johnson  <tejohnson@google.com>
 
 	* tree-ssa-sccvn.c (visit_reference_op_call): Handle NULL vdef.
diff --git a/gcc/ira.c b/gcc/ira.c
index 41e05f4..ee6010a 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5532,6 +5532,18 @@  do_reload (void)
   if (need_dce && optimize)
     run_fast_dce ();
 
+  /* Diagnose uses of the hard frame pointer when it is used as a global
+     register.  Often we can get away with letting the user appropriate
+     the frame pointer, but we should let them know when code generation
+     makes that impossible.  */
+  if (global_regs[HARD_FRAME_POINTER_REGNUM] && frame_pointer_needed)
+    {
+      tree decl = global_regs_decl[HARD_FRAME_POINTER_REGNUM];
+      error_at (DECL_SOURCE_LOCATION (current_function_decl),
+                "frame pointer required, but reserved");
+      inform (DECL_SOURCE_LOCATION (decl), "for %qD", decl);
+    }
+
   timevar_pop (TV_IRA);
 }
 
diff --git a/gcc/reginfo.c b/gcc/reginfo.c
index efaa0cb..bdb980d 100644
--- a/gcc/reginfo.c
+++ b/gcc/reginfo.c
@@ -86,7 +86,7 @@  static const char initial_call_really_used_regs[] = CALL_REALLY_USED_REGISTERS;
 char global_regs[FIRST_PSEUDO_REGISTER];
 
 /* Declaration for the global register. */
-static tree GTY(()) global_regs_decl[FIRST_PSEUDO_REGISTER];
+tree global_regs_decl[FIRST_PSEUDO_REGISTER];
 
 /* Same information as REGS_INVALIDATED_BY_CALL but in regset form to be used
    in dataflow more conveniently.  */
diff --git a/gcc/rtl.h b/gcc/rtl.h
index e7d60ee..10ee818 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2795,6 +2795,8 @@  extern void _fatal_insn (const char *, const_rtx, const char *, int, const char
 #define fatal_insn_not_found(insn) \
 	_fatal_insn_not_found (insn, __FILE__, __LINE__, __FUNCTION__)
 
+/* reginfo.c */
+extern tree GTY(()) global_regs_decl[FIRST_PSEUDO_REGISTER];
 
 
 #endif /* ! GCC_RTL_H */
diff --git a/gcc/testsuite/gcc.target/i386/pr54694.c b/gcc/testsuite/gcc.target/i386/pr54694.c
new file mode 100644
index 0000000..bcf82c2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr54694.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+
+register void *hfp __asm__("%ebp");	/* { dg-message "note: for" } */
+
+extern void g(void *);
+
+void f(int x)			/* { dg-error "frame pointer required" } */
+{
+  g(__builtin_alloca(x));
+}