Patchwork Add new warning -Wtrampolines

login
register
mail settings
Submitter Magnus Granberg
Date June 18, 2010, 2:02 a.m.
Message ID <201006180402.05708.zorry@gentoo.org>
Download mbox | patch
Permalink /patch/56099/
State New
Headers show

Comments

Magnus Granberg - June 18, 2010, 2:02 a.m.
söndag 30 maj 2010 02.21.53 skrev  Magnus Granberg:
> torsdag 06 maj 2010 09.24.46 skrev  Eric Botcazou:
> > It would be better to issue it from
> > 
> >  tree-nested.c:convert_tramp_reference_op so that it is attached to the
> >  token that causes it to be created.  For extra points, you could even
> >  make
> > 
> >  it reference the nested function:
> > p2.adb: In function 'P2':
> > p2.adb:17:12: warning: trampoline generated for 'P2.F'
> > p2.adb:26:11: warning: address taken from here
> 
> Have updated the patch to make two warnings one for the trampoline and the
> second one for makeing the stack executable. Did't add the reference to the
> nested function for extra points. Could not get the testcase ignore the
> second warning in the same line.
>
Have updated the patch. Can some one review it and commit it. 
If the patch is okey. Tested on x86_64-unknown-linux-gnu

/Magnus
----
2010-06-18	Magnus Granberg		<zorry@gentoo.org>, Kevin F. Quinn <kevquinn@gentoo.org>

		* builtins.c:  (expand_builtin_init_trampoline):	if warn_trampolines make a warning.
		* common.opt:	Add -Wtrampolines.
		* varasm.c:		(file_end_indicate_exec_stack): if warn_trampolines make a warning.

gcc/doc

2010-06-18	Magnus Granberg		<zorry@gentoo.org>

		* invoke.texi:	Add -Wtrampolines.

gcc/testsuite/

2010-06-18	Magnus Granberg		<zorry@gentoo.org>

		* gcc.dg/Wtrampolines.c:	New.
Manuel López-Ibáñez - July 2, 2010, 11:07 a.m.
Magnus,

Was this reviewed? If nobody offers to commit it, I will try to find
some time to do it.

Cheers,

Manuel.

On 18 June 2010 04:02, Magnus Granberg <zorry@gentoo.org> wrote:
> söndag 30 maj 2010 02.21.53 skrev  Magnus Granberg:
>> torsdag 06 maj 2010 09.24.46 skrev  Eric Botcazou:
>> > It would be better to issue it from
>> >
>> >  tree-nested.c:convert_tramp_reference_op so that it is attached to the
>> >  token that causes it to be created.  For extra points, you could even
>> >  make
>> >
>> >  it reference the nested function:
>> > p2.adb: In function 'P2':
>> > p2.adb:17:12: warning: trampoline generated for 'P2.F'
>> > p2.adb:26:11: warning: address taken from here
>>
>> Have updated the patch to make two warnings one for the trampoline and the
>> second one for makeing the stack executable. Did't add the reference to the
>> nested function for extra points. Could not get the testcase ignore the
>> second warning in the same line.
>>
> Have updated the patch. Can some one review it and commit it.
> If the patch is okey. Tested on x86_64-unknown-linux-gnu
>
> /Magnus
> ----
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>, Kevin F. Quinn <kevquinn@gentoo.org>
>
>                * builtins.c:  (expand_builtin_init_trampoline):        if warn_trampolines make a warning.
>                * common.opt:   Add -Wtrampolines.
>                * varasm.c:             (file_end_indicate_exec_stack): if warn_trampolines make a warning.
>
> gcc/doc
>
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>
>
>                * invoke.texi:  Add -Wtrampolines.
>
> gcc/testsuite/
>
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>
>
>                * gcc.dg/Wtrampolines.c:        New.
>
Gabriel Dos Reis - July 2, 2010, 8:33 p.m.
On Thu, Jun 17, 2010 at 9:02 PM, Magnus Granberg <zorry@gentoo.org> wrote:
> söndag 30 maj 2010 02.21.53 skrev  Magnus Granberg:
>> torsdag 06 maj 2010 09.24.46 skrev  Eric Botcazou:
>> > It would be better to issue it from
>> >
>> >  tree-nested.c:convert_tramp_reference_op so that it is attached to the
>> >  token that causes it to be created.  For extra points, you could even
>> >  make
>> >
>> >  it reference the nested function:
>> > p2.adb: In function 'P2':
>> > p2.adb:17:12: warning: trampoline generated for 'P2.F'
>> > p2.adb:26:11: warning: address taken from here
>>
>> Have updated the patch to make two warnings one for the trampoline and the
>> second one for makeing the stack executable. Did't add the reference to the
>> nested function for extra points. Could not get the testcase ignore the
>> second warning in the same line.
>>
> Have updated the patch. Can some one review it and commit it.
> If the patch is okey. Tested on x86_64-unknown-linux-gnu
>
> /Magnus
> ----
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>, Kevin F. Quinn <kevquinn@gentoo.org>
>
>                * builtins.c:  (expand_builtin_init_trampoline):        if warn_trampolines make a warning.
>                * common.opt:   Add -Wtrampolines.
>                * varasm.c:             (file_end_indicate_exec_stack): if warn_trampolines make a warning.
>
> gcc/doc
>
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>
>
>                * invoke.texi:  Add -Wtrampolines.
>
> gcc/testsuite/
>
> 2010-06-18      Magnus Granberg         <zorry@gentoo.org>
>
>                * gcc.dg/Wtrampolines.c:        New.
>

Hi,

  I guess a question I have had since the beginning is:
Why are you warning all the time a trampoline is generated, and not just
for the case where the trampoline would require an executable stack?

Patch

gcc/
--- gcc/builtins.c.zorry	2010-04-13 15:47:11.000000000 +0200
+++ gcc/builtins.c			2010-06-16 12:33:54.000000000 +0200
@@ -5150,6 +5150,10 @@ 
   targetm.calls.trampoline_init (m_tramp, t_func, r_chain);
 
   trampolines_created = 1;
+
+  if (warn_trampolines)
+    warning (OPT_Wtrampolines, "trampoline generated for nested function %s", (IDENTIFIER_POINTER(DECL_NAME(t_func))));
+
   return const0_rtx;
 }
 
--- gcc/common.opt.zorry	2010-03-18 04:01:09.000000000 +0100
+++ gcc/common.opt			2010-05-06 00:44:18.000000000 +0200
@@ -192,6 +192,10 @@ 
 Common Var(warn_system_headers) Warning
 Do not suppress warnings from system headers
 
+Wtrampolines
+Common Var(warn_trampolines) Warnings
+Warn whenever a trampoline is generated and warn if it requires executable stack to
+
 Wtype-limits
 Common Var(warn_type_limits) Init(-1) Warning
 Warn if a comparison is always true or always false due to the limited range of the data type
--- gcc/varasm.c.zorry	2010-03-27 12:56:30.000000000 +0100
+++ gcc/varasm.c		2010-05-29 15:06:33.000000000 +0200
@@ -6768,7 +6768,11 @@ 
 {
   unsigned int flags = SECTION_DEBUG;
   if (trampolines_created)
+    {
     flags |= SECTION_CODE;
+    if (warn_trampolines)
+      warning (OPT_Wtrampolines, "setting the stack as executable stack");
+	 }
 
   switch_to_section (get_section (".note.GNU-stack", flags, NULL));
 }
--- gcc/doc/invoke.texi.zorry	2010-04-06 16:02:22.000000000 +0200
+++ gcc/doc/invoke.texi			2010-05-06 00:20:25.000000000 +0200
@@ -258,8 +258,8 @@ 
 -Wstrict-aliasing -Wstrict-aliasing=n @gol
 -Wstrict-overflow -Wstrict-overflow=@var{n} @gol
 -Wswitch  -Wswitch-default  -Wswitch-enum -Wsync-nand @gol
--Wsystem-headers  -Wtrigraphs  -Wtype-limits  -Wundef  -Wuninitialized @gol
--Wunknown-pragmas  -Wno-pragmas @gol
+-Wsystem-headers  -Wtrampolines  -Wtrigraphs  -Wtype-limits  -Wundef @gol
+-Wuninitialized  -Wunknown-pragmas  -Wno-pragmas @gol
 -Wunsuffixed-float-constants  -Wunused  -Wunused-function @gol
 -Wunused-label  -Wunused-parameter -Wno-unused-result -Wunused-value  -Wunused-variable @gol
 -Wvariadic-macros -Wvla @gol
@@ -3603,6 +3603,19 @@ 
 option will @emph{not} warn about unknown pragmas in system
 headers---for that, @option{-Wunknown-pragmas} must also be used.
 
+@item -Wtrampolines
+@opindex Wtrampolines
+@opindex Wno-trampolines
+ Warn about trampolines generated for pointers to nested functions.
+ Warn when the trampoline requires the stack to be made executable.
+ 
+ A trampoline is a small piece of data or code that is created at run
+ time on the stack when the address of a nested function is taken, and
+ is used to call the nested function indirectly.  For some targets, it
+ is made up of data only and thus requires no special treatment.  But,
+ for most targets, it is made up of code and thus requires the stack
+ to be made executable in order for the program to work properly.
+
 @item -Wfloat-equal
 @opindex Wfloat-equal
 @opindex Wno-float-equal
--- gcc/testsuite/gcc.dg/Wtrampolines.c.zorry	2010-05-05 12:53:11.000000000 +0200
+++ gcc/testsuite/gcc.dg/Wtrampolines.c			2010-05-06 00:26:05.000000000 +0200
@@ -0,0 +1,59 @@ 
+/* Origin: trampoline-1.c Waldek Hebisch <hebisch@math.uni.wroc.pl> */
+/* Ported to test -Wtrampolines Magnus Granberg <zorry@gentoo.org> */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-O2 -Wtrampolines" } */
+/* { dg-warning "trampoline" "" { target *-*-* } 31 } */
+/* { dg-prune-output "stack" } */
+
+#ifndef NO_TRAMPOLINES
+
+/* This used to fail on various versions of Solaris 2 because the
+   trampoline couldn't be made executable.  */
+
+extern void abort(void);
+extern double fabs(double);
+
+void foo (void)
+{
+  const int correct[1100] = {1, 0, -2, 0, 1, 0, 1, -1, -10, -30, -67};
+  int i;
+
+  double x1 (void) {return 1; }
+  double x2 (void) {return -1;}
+  double x3 (void) {return -1;}
+  double x4 (void) {return 1; }
+  double x5 (void) {return 0; }
+
+  typedef double pfun(void);
+
+  double a (int k, pfun x1, pfun x2, pfun x3, pfun x4, pfun x5)
+  {
+    double b (void)
+    {
+      k = k - 1;
+      return a (k, b, x1, x2, x3, x4 );
+    }
+
+    if (k <= 0)
+      return x4 () + x5 ();
+    else
+      return b ();
+  }
+
+  for (i=0; i<=10; i++)
+  {
+    if (fabs(a( i, x1, x2, x3, x4, x5 ) - correct [i]) > 0.1)
+      abort();
+  }
+}
+#endif
+
+int main (void)
+{
+#ifndef NO_TRAMPOLINES
+  foo ();
+#endif
+  return 0;
+}