[PATCH] gcc: Disallow trampolines when -fhardened

[PATCH] gcc: Disallow trampolines when -fhardened

[Marek Polacek]

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --
It came up that a good hardening strategy is to disable trampolines
which may require executable stack.  Therefore the following patch
adds -Werror=trampolines to -fhardened.

gcc/ChangeLog:

	* common.opt (Wtrampolines): Enable by -fhardened.
	* doc/invoke.texi: Reflect that -fhardened enables -Werror=trampolines.
	* opts.cc (print_help_hardened): Add -Werror=trampolines.
	* toplev.cc (process_options): Enable -Werror=trampolines for
	-fhardened.

gcc/testsuite/ChangeLog:

	* gcc.dg/fhardened-1.c: New test.
	* gcc.dg/fhardened-2.c: New test.
	* gcc.dg/fhardened-3.c: New test.
	* gcc.dg/fhardened-4.c: New test.
	* gcc.dg/fhardened-5.c: New test.
---
 gcc/common.opt                     |  2 +-
 gcc/doc/invoke.texi                |  1 +
 gcc/opts.cc                        |  1 +
 gcc/testsuite/gcc.dg/fhardened-1.c | 27 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/fhardened-2.c | 25 +++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/fhardened-3.c | 25 +++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/fhardened-4.c | 25 +++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/fhardened-5.c | 27 +++++++++++++++++++++++++++
 gcc/toplev.cc                      |  8 +++++++-
 9 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/fhardened-1.c
 create mode 100644 gcc/testsuite/gcc.dg/fhardened-2.c
 create mode 100644 gcc/testsuite/gcc.dg/fhardened-3.c
 create mode 100644 gcc/testsuite/gcc.dg/fhardened-4.c
 create mode 100644 gcc/testsuite/gcc.dg/fhardened-5.c

diff --git a/gcc/common.opt b/gcc/common.opt
index 161a035d736..9b09c7cb3df 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -807,7 +807,7 @@ Common Var(warn_system_headers) Warning
 Do not suppress warnings from system headers.
 
 Wtrampolines
-Common Var(warn_trampolines) Warning
+Common Var(warn_trampolines) Warning EnabledBy(fhardened)
 Warn whenever a trampoline is generated.
 
 Wtrivial-auto-var-init
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 2fab4c5d71f..c1664a1a0f1 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -17745,6 +17745,7 @@ may change between major releases of GCC, but are currently:
 -fstack-protector-strong
 -fstack-clash-protection
 -fcf-protection=full @r{(x86 GNU/Linux only)}
+-Werror=trampolines
 }
 
 The list of options enabled by @option{-fhardened} can be generated using
diff --git a/gcc/opts.cc b/gcc/opts.cc
index 5d5efaf1b9e..aa062b87cef 100644
--- a/gcc/opts.cc
+++ b/gcc/opts.cc
@@ -2517,6 +2517,7 @@ print_help_hardened ()
   printf ("  %s\n", "-fstack-protector-strong");
   printf ("  %s\n", "-fstack-clash-protection");
   printf ("  %s\n", "-fcf-protection=full");
+  printf ("  %s\n", "-Werror=trampolines");
   putchar ('\n');
 }
 
diff --git a/gcc/testsuite/gcc.dg/fhardened-1.c b/gcc/testsuite/gcc.dg/fhardened-1.c
new file mode 100644
index 00000000000..8710959b6f1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fhardened-1.c
@@ -0,0 +1,27 @@
+/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-fhardened -O" } */
+
+static void
+baz (int (*bar) (void))
+{
+  bar ();
+}
+
+int
+main (void)
+{
+  int a = 6;
+
+  int
+  bar (void)	// { dg-error "trampoline" }
+  {
+    return a;
+  }
+
+  baz (bar);
+
+  return 0;
+}
+
+/* { dg-prune-output "some warnings being treated as errors" } */
diff --git a/gcc/testsuite/gcc.dg/fhardened-2.c b/gcc/testsuite/gcc.dg/fhardened-2.c
new file mode 100644
index 00000000000..d47512aa47f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fhardened-2.c
@@ -0,0 +1,25 @@
+/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-fhardened -O -Wno-trampolines" } */
+
+static void
+baz (int (*bar) (void))
+{
+  bar ();
+}
+
+int
+main (void)
+{
+  int a = 6;
+
+  int
+  bar (void)	// { dg-bogus "trampoline" }
+  {
+    return a;
+  }
+
+  baz (bar);
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/fhardened-3.c b/gcc/testsuite/gcc.dg/fhardened-3.c
new file mode 100644
index 00000000000..cebae13d8be
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fhardened-3.c
@@ -0,0 +1,25 @@
+/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-fhardened -O -Wno-error" } */
+
+static void
+baz (int (*bar) (void))
+{
+  bar ();
+}
+
+int
+main (void)
+{
+  int a = 6;
+
+  int
+  bar (void)	// { dg-warning "trampoline" }
+  {
+    return a;
+  }
+
+  baz (bar);
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/fhardened-4.c b/gcc/testsuite/gcc.dg/fhardened-4.c
new file mode 100644
index 00000000000..7e62ed3385d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fhardened-4.c
@@ -0,0 +1,25 @@
+/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-fhardened -O -Wno-error=trampolines" } */
+
+static void
+baz (int (*bar) (void))
+{
+  bar ();
+}
+
+int
+main (void)
+{
+  int a = 6;
+
+  int
+  bar (void)	// { dg-warning "trampoline" }
+  {
+    return a;
+  }
+
+  baz (bar);
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/fhardened-5.c b/gcc/testsuite/gcc.dg/fhardened-5.c
new file mode 100644
index 00000000000..5d3f0dcae8e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fhardened-5.c
@@ -0,0 +1,27 @@
+/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
+/* { dg-require-effective-target trampolines } */
+/* { dg-options "-fhardened -O -Wtrampolines" } */
+
+static void
+baz (int (*bar) (void))
+{
+  bar ();
+}
+
+int
+main (void)
+{
+  int a = 6;
+
+  int
+  bar (void)	// { dg-error "trampoline" }
+  {
+    return a;
+  }
+
+  baz (bar);
+
+  return 0;
+}
+
+/* { dg-prune-output "some warnings being treated as errors" } */
diff --git a/gcc/toplev.cc b/gcc/toplev.cc
index 85450d97a1a..2f0ac74dee0 100644
--- a/gcc/toplev.cc
+++ b/gcc/toplev.cc
@@ -1682,7 +1682,7 @@ process_options ()
     flag_ipa_ra = 0;
 
   /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
-     have not been set.  */
+     have not been set.  Also enable -Werror=trampolines for -fhardened.  */
   if (!OPTION_SET_P (warnings_are_errors))
     {
       if (warn_coverage_mismatch
@@ -1693,6 +1693,12 @@ process_options ()
 	  && option_unspecified_p (OPT_Wcoverage_invalid_line_number))
 	diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_invalid_line_number,
 					DK_ERROR, UNKNOWN_LOCATION);
+
+      if (flag_hardened
+	  && warn_trampolines
+	  && option_unspecified_p (OPT_Wtrampolines))
+	diagnostic_classify_diagnostic (global_dc, OPT_Wtrampolines,
+					DK_ERROR, UNKNOWN_LOCATION);
     }
 
   /* Save the current optimization options.  */

base-commit: b8edb812ff4934c609fdfafe2e1c7f932bc18305
-- 
2.42.0

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 7698 bytes --]

On Fri, Dec 1, 2023, 11:36 Marek Polacek <polacek@redhat.com> wrote:

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> -- >8 --
> It came up that a good hardening strategy is to disable trampolines
> which may require executable stack.  Therefore the following patch
> adds -Werror=trampolines to -fhardened.
>

It might make sense to add a fortran testcase too. Especially when that and
Ada are 2 biggest users of trampolines.

Thanks,
Andrew




> gcc/ChangeLog:
>
>         * common.opt (Wtrampolines): Enable by -fhardened.
>         * doc/invoke.texi: Reflect that -fhardened enables
> -Werror=trampolines.
>         * opts.cc (print_help_hardened): Add -Werror=trampolines.
>         * toplev.cc (process_options): Enable -Werror=trampolines for
>         -fhardened.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/fhardened-1.c: New test.
>         * gcc.dg/fhardened-2.c: New test.
>         * gcc.dg/fhardened-3.c: New test.
>         * gcc.dg/fhardened-4.c: New test.
>         * gcc.dg/fhardened-5.c: New test.
> ---
>  gcc/common.opt                     |  2 +-
>  gcc/doc/invoke.texi                |  1 +
>  gcc/opts.cc                        |  1 +
>  gcc/testsuite/gcc.dg/fhardened-1.c | 27 +++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-2.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-3.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-4.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-5.c | 27 +++++++++++++++++++++++++++
>  gcc/toplev.cc                      |  8 +++++++-
>  9 files changed, 139 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-3.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-4.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-5.c
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 161a035d736..9b09c7cb3df 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -807,7 +807,7 @@ Common Var(warn_system_headers) Warning
>  Do not suppress warnings from system headers.
>
>  Wtrampolines
> -Common Var(warn_trampolines) Warning
> +Common Var(warn_trampolines) Warning EnabledBy(fhardened)
>  Warn whenever a trampoline is generated.
>
>  Wtrivial-auto-var-init
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 2fab4c5d71f..c1664a1a0f1 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -17745,6 +17745,7 @@ may change between major releases of GCC, but are
> currently:
>  -fstack-protector-strong
>  -fstack-clash-protection
>  -fcf-protection=full @r{(x86 GNU/Linux only)}
> +-Werror=trampolines
>  }
>
>  The list of options enabled by @option{-fhardened} can be generated using
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 5d5efaf1b9e..aa062b87cef 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -2517,6 +2517,7 @@ print_help_hardened ()
>    printf ("  %s\n", "-fstack-protector-strong");
>    printf ("  %s\n", "-fstack-clash-protection");
>    printf ("  %s\n", "-fcf-protection=full");
> +  printf ("  %s\n", "-Werror=trampolines");
>    putchar ('\n');
>  }
>
> diff --git a/gcc/testsuite/gcc.dg/fhardened-1.c
> b/gcc/testsuite/gcc.dg/fhardened-1.c
> new file mode 100644
> index 00000000000..8710959b6f1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-1.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)   // { dg-error "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> +
> +/* { dg-prune-output "some warnings being treated as errors" } */
> diff --git a/gcc/testsuite/gcc.dg/fhardened-2.c
> b/gcc/testsuite/gcc.dg/fhardened-2.c
> new file mode 100644
> index 00000000000..d47512aa47f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-2.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-trampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)   // { dg-bogus "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-3.c
> b/gcc/testsuite/gcc.dg/fhardened-3.c
> new file mode 100644
> index 00000000000..cebae13d8be
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-3.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-error" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)   // { dg-warning "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-4.c
> b/gcc/testsuite/gcc.dg/fhardened-4.c
> new file mode 100644
> index 00000000000..7e62ed3385d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-4.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-error=trampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)   // { dg-warning "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-5.c
> b/gcc/testsuite/gcc.dg/fhardened-5.c
> new file mode 100644
> index 00000000000..5d3f0dcae8e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-5.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wtrampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)   // { dg-error "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> +
> +/* { dg-prune-output "some warnings being treated as errors" } */
> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
> index 85450d97a1a..2f0ac74dee0 100644
> --- a/gcc/toplev.cc
> +++ b/gcc/toplev.cc
> @@ -1682,7 +1682,7 @@ process_options ()
>      flag_ipa_ra = 0;
>
>    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> -     have not been set.  */
> +     have not been set.  Also enable -Werror=trampolines for -fhardened.
> */
>    if (!OPTION_SET_P (warnings_are_errors))
>      {
>        if (warn_coverage_mismatch
> @@ -1693,6 +1693,12 @@ process_options ()
>           && option_unspecified_p (OPT_Wcoverage_invalid_line_number))
>         diagnostic_classify_diagnostic (global_dc,
> OPT_Wcoverage_invalid_line_number,
>                                         DK_ERROR, UNKNOWN_LOCATION);
> +
> +      if (flag_hardened
> +         && warn_trampolines
> +         && option_unspecified_p (OPT_Wtrampolines))
> +       diagnostic_classify_diagnostic (global_dc, OPT_Wtrampolines,
> +                                       DK_ERROR, UNKNOWN_LOCATION);
>      }
>
>    /* Save the current optimization options.  */
>
> base-commit: b8edb812ff4934c609fdfafe2e1c7f932bc18305
> --
> 2.42.0
>
>

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Marek Polacek]

On Fri, Dec 01, 2023 at 11:44:28AM -0800, Andrew Pinski wrote:
> On Fri, Dec 1, 2023, 11:36 Marek Polacek <polacek@redhat.com> wrote:
> 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > -- >8 --
> > It came up that a good hardening strategy is to disable trampolines
> > which may require executable stack.  Therefore the following patch
> > adds -Werror=trampolines to -fhardened.
> >
> 
> It might make sense to add a fortran testcase too. Especially when that and
> Ada are 2 biggest users of trampolines.

I don't know either of these languages to write a test, and I don't see
anything that mentions the word trampoline in gfortran.dg/.  Ada has
gnat.dg/trampoline3.adb but:

$ gcc -c -Wtrampolines trampoline3.adb
trampoline3.adb:6:03: warning: variable "A" is read but never assigned [-gnatwv]

so there is no warning.

Marek

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Jakub Jelinek]

On Fri, Dec 01, 2023 at 03:53:14PM -0500, Marek Polacek wrote:
> On Fri, Dec 01, 2023 at 11:44:28AM -0800, Andrew Pinski wrote:
> > On Fri, Dec 1, 2023, 11:36 Marek Polacek <polacek@redhat.com> wrote:
> > 
> > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > >
> > > -- >8 --
> > > It came up that a good hardening strategy is to disable trampolines
> > > which may require executable stack.  Therefore the following patch
> > > adds -Werror=trampolines to -fhardened.
> > >
> > 
> > It might make sense to add a fortran testcase too. Especially when that and
> > Ada are 2 biggest users of trampolines.
> 
> I don't know either of these languages to write a test, and I don't see
> anything that mentions the word trampoline in gfortran.dg/.  Ada has

program nesting
  integer :: i
  procedure(), pointer :: p
  p => foo
  i = 5
  call p
  if (i.ne.6) stop 1
contains
  subroutine foo
    i = i + 1
  end subroutine
end program

(obviously at -O0 only)?

	Jakub

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Eric Botcazou]

> I don't know either of these languages to write a test, and I don't see
> anything that mentions the word trampoline in gfortran.dg/.  Ada has
> gnat.dg/trampoline3.adb but:
> 
> $ gcc -c -Wtrampolines trampoline3.adb
> trampoline3.adb:6:03: warning: variable "A" is read but never assigned
> [-gnatwv]
> 
> so there is no warning.

Look at the last line of the test (Ada has not used trampolines for ages!).

-- 
Eric Botcazou

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Martin Uecker]

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> It came up that a good hardening strategy is to disable trampolines
> which may require executable stack.  Therefore the following patch
> adds -Werror=trampolines to -fhardened.

This would add a warning about specific code (where it is then
unclear whether rewriting it is feasible or even an improvement),
which seems different to all the other flags -fhardening has
now.

GCC now has an option to allocate trampolines on the heap,
which would seem to be a better fit.  On the other hand,
it does not work with longjmp which may be a limitation.

Martin


> 
> gcc/ChangeLog:
> 
> 	* common.opt (Wtrampolines): Enable by -fhardened.
> 	* doc/invoke.texi: Reflect that -fhardened enables -Werror=trampolines.
> 	* opts.cc (print_help_hardened): Add -Werror=trampolines.
> 	* toplev.cc (process_options): Enable -Werror=trampolines for
> 	-fhardened.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/fhardened-1.c: New test.
> 	* gcc.dg/fhardened-2.c: New test.
> 	* gcc.dg/fhardened-3.c: New test.
> 	* gcc.dg/fhardened-4.c: New test.
> 	* gcc.dg/fhardened-5.c: New test.
> ---
>  gcc/common.opt                     |  2 +-
>  gcc/doc/invoke.texi                |  1 +
>  gcc/opts.cc                        |  1 +
>  gcc/testsuite/gcc.dg/fhardened-1.c | 27 +++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-2.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-3.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-4.c | 25 +++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/fhardened-5.c | 27 +++++++++++++++++++++++++++
>  gcc/toplev.cc                      |  8 +++++++-
>  9 files changed, 139 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-3.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-4.c
>  create mode 100644 gcc/testsuite/gcc.dg/fhardened-5.c
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 161a035d736..9b09c7cb3df 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -807,7 +807,7 @@ Common Var(warn_system_headers) Warning
>  Do not suppress warnings from system headers.
>  
>  Wtrampolines
> -Common Var(warn_trampolines) Warning
> +Common Var(warn_trampolines) Warning EnabledBy(fhardened)
>  Warn whenever a trampoline is generated.
>  
>  Wtrivial-auto-var-init
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 2fab4c5d71f..c1664a1a0f1 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -17745,6 +17745,7 @@ may change between major releases of GCC, but are currently:
>  -fstack-protector-strong
>  -fstack-clash-protection
>  -fcf-protection=full @r{(x86 GNU/Linux only)}
> +-Werror=trampolines
>  }
>  
>  The list of options enabled by @option{-fhardened} can be generated using
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 5d5efaf1b9e..aa062b87cef 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -2517,6 +2517,7 @@ print_help_hardened ()
>    printf ("  %s\n", "-fstack-protector-strong");
>    printf ("  %s\n", "-fstack-clash-protection");
>    printf ("  %s\n", "-fcf-protection=full");
> +  printf ("  %s\n", "-Werror=trampolines");
>    putchar ('\n');
>  }
>  
> diff --git a/gcc/testsuite/gcc.dg/fhardened-1.c b/gcc/testsuite/gcc.dg/fhardened-1.c
> new file mode 100644
> index 00000000000..8710959b6f1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-1.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)	// { dg-error "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> +
> +/* { dg-prune-output "some warnings being treated as errors" } */
> diff --git a/gcc/testsuite/gcc.dg/fhardened-2.c b/gcc/testsuite/gcc.dg/fhardened-2.c
> new file mode 100644
> index 00000000000..d47512aa47f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-2.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-trampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)	// { dg-bogus "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-3.c b/gcc/testsuite/gcc.dg/fhardened-3.c
> new file mode 100644
> index 00000000000..cebae13d8be
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-3.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-error" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)	// { dg-warning "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-4.c b/gcc/testsuite/gcc.dg/fhardened-4.c
> new file mode 100644
> index 00000000000..7e62ed3385d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-4.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wno-error=trampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)	// { dg-warning "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/fhardened-5.c b/gcc/testsuite/gcc.dg/fhardened-5.c
> new file mode 100644
> index 00000000000..5d3f0dcae8e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/fhardened-5.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
> +/* { dg-require-effective-target trampolines } */
> +/* { dg-options "-fhardened -O -Wtrampolines" } */
> +
> +static void
> +baz (int (*bar) (void))
> +{
> +  bar ();
> +}
> +
> +int
> +main (void)
> +{
> +  int a = 6;
> +
> +  int
> +  bar (void)	// { dg-error "trampoline" }
> +  {
> +    return a;
> +  }
> +
> +  baz (bar);
> +
> +  return 0;
> +}
> +
> +/* { dg-prune-output "some warnings being treated as errors" } */
> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
> index 85450d97a1a..2f0ac74dee0 100644
> --- a/gcc/toplev.cc
> +++ b/gcc/toplev.cc
> @@ -1682,7 +1682,7 @@ process_options ()
>      flag_ipa_ra = 0;
>  
>    /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> -     have not been set.  */
> +     have not been set.  Also enable -Werror=trampolines for -fhardened.  */
>    if (!OPTION_SET_P (warnings_are_errors))
>      {
>        if (warn_coverage_mismatch
> @@ -1693,6 +1693,12 @@ process_options ()
>  	  && option_unspecified_p (OPT_Wcoverage_invalid_line_number))
>  	diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_invalid_line_number,
>  					DK_ERROR, UNKNOWN_LOCATION);
> +
> +      if (flag_hardened
> +	  && warn_trampolines
> +	  && option_unspecified_p (OPT_Wtrampolines))
> +	diagnostic_classify_diagnostic (global_dc, OPT_Wtrampolines,
> +					DK_ERROR, UNKNOWN_LOCATION);
>      }
>  
>    /* Save the current optimization options.  */
> 
> base-commit: b8edb812ff4934c609fdfafe2e1c7f932bc18305
> -- 
> 2.42.0
> 

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Iain Sandoe]

> On 2 Dec 2023, at 09:42, Martin Uecker <ma.uecker@gmail.com> wrote:
> 
> 
>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>> 
>> -- >8 --
>> It came up that a good hardening strategy is to disable trampolines
>> which may require executable stack.  Therefore the following patch
>> adds -Werror=trampolines to -fhardened.
> 
> This would add a warning about specific code (where it is then
> unclear whether rewriting it is feasible or even an improvement),
> which seems different to all the other flags -fhardening has
> now.
> 
> GCC now has an option to allocate trampolines on the heap,
> which would seem to be a better fit.

Indeed, I was thinking of mentioning this.

>  On the other hand,
> it does not work with longjmp which may be a limitation.

I suspect that we can make this work using handlers and forced unwind,
but unfortunately do not have time to work on it at the moment.

Iain

> 
> Martin
> 
> 
>> 
>> gcc/ChangeLog:
>> 
>> 	* common.opt (Wtrampolines): Enable by -fhardened.
>> 	* doc/invoke.texi: Reflect that -fhardened enables -Werror=trampolines.
>> 	* opts.cc (print_help_hardened): Add -Werror=trampolines.
>> 	* toplev.cc (process_options): Enable -Werror=trampolines for
>> 	-fhardened.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	* gcc.dg/fhardened-1.c: New test.
>> 	* gcc.dg/fhardened-2.c: New test.
>> 	* gcc.dg/fhardened-3.c: New test.
>> 	* gcc.dg/fhardened-4.c: New test.
>> 	* gcc.dg/fhardened-5.c: New test.
>> ---
>> gcc/common.opt                     |  2 +-
>> gcc/doc/invoke.texi                |  1 +
>> gcc/opts.cc                        |  1 +
>> gcc/testsuite/gcc.dg/fhardened-1.c | 27 +++++++++++++++++++++++++++
>> gcc/testsuite/gcc.dg/fhardened-2.c | 25 +++++++++++++++++++++++++
>> gcc/testsuite/gcc.dg/fhardened-3.c | 25 +++++++++++++++++++++++++
>> gcc/testsuite/gcc.dg/fhardened-4.c | 25 +++++++++++++++++++++++++
>> gcc/testsuite/gcc.dg/fhardened-5.c | 27 +++++++++++++++++++++++++++
>> gcc/toplev.cc                      |  8 +++++++-
>> 9 files changed, 139 insertions(+), 2 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/fhardened-1.c
>> create mode 100644 gcc/testsuite/gcc.dg/fhardened-2.c
>> create mode 100644 gcc/testsuite/gcc.dg/fhardened-3.c
>> create mode 100644 gcc/testsuite/gcc.dg/fhardened-4.c
>> create mode 100644 gcc/testsuite/gcc.dg/fhardened-5.c
>> 
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index 161a035d736..9b09c7cb3df 100644
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -807,7 +807,7 @@ Common Var(warn_system_headers) Warning
>> Do not suppress warnings from system headers.
>> 
>> Wtrampolines
>> -Common Var(warn_trampolines) Warning
>> +Common Var(warn_trampolines) Warning EnabledBy(fhardened)
>> Warn whenever a trampoline is generated.
>> 
>> Wtrivial-auto-var-init
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 2fab4c5d71f..c1664a1a0f1 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -17745,6 +17745,7 @@ may change between major releases of GCC, but are currently:
>> -fstack-protector-strong
>> -fstack-clash-protection
>> -fcf-protection=full @r{(x86 GNU/Linux only)}
>> +-Werror=trampolines
>> }
>> 
>> The list of options enabled by @option{-fhardened} can be generated using
>> diff --git a/gcc/opts.cc b/gcc/opts.cc
>> index 5d5efaf1b9e..aa062b87cef 100644
>> --- a/gcc/opts.cc
>> +++ b/gcc/opts.cc
>> @@ -2517,6 +2517,7 @@ print_help_hardened ()
>>   printf ("  %s\n", "-fstack-protector-strong");
>>   printf ("  %s\n", "-fstack-clash-protection");
>>   printf ("  %s\n", "-fcf-protection=full");
>> +  printf ("  %s\n", "-Werror=trampolines");
>>   putchar ('\n');
>> }
>> 
>> diff --git a/gcc/testsuite/gcc.dg/fhardened-1.c b/gcc/testsuite/gcc.dg/fhardened-1.c
>> new file mode 100644
>> index 00000000000..8710959b6f1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/fhardened-1.c
>> @@ -0,0 +1,27 @@
>> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
>> +/* { dg-require-effective-target trampolines } */
>> +/* { dg-options "-fhardened -O" } */
>> +
>> +static void
>> +baz (int (*bar) (void))
>> +{
>> +  bar ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 6;
>> +
>> +  int
>> +  bar (void)	// { dg-error "trampoline" }
>> +  {
>> +    return a;
>> +  }
>> +
>> +  baz (bar);
>> +
>> +  return 0;
>> +}
>> +
>> +/* { dg-prune-output "some warnings being treated as errors" } */
>> diff --git a/gcc/testsuite/gcc.dg/fhardened-2.c b/gcc/testsuite/gcc.dg/fhardened-2.c
>> new file mode 100644
>> index 00000000000..d47512aa47f
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/fhardened-2.c
>> @@ -0,0 +1,25 @@
>> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
>> +/* { dg-require-effective-target trampolines } */
>> +/* { dg-options "-fhardened -O -Wno-trampolines" } */
>> +
>> +static void
>> +baz (int (*bar) (void))
>> +{
>> +  bar ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 6;
>> +
>> +  int
>> +  bar (void)	// { dg-bogus "trampoline" }
>> +  {
>> +    return a;
>> +  }
>> +
>> +  baz (bar);
>> +
>> +  return 0;
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/fhardened-3.c b/gcc/testsuite/gcc.dg/fhardened-3.c
>> new file mode 100644
>> index 00000000000..cebae13d8be
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/fhardened-3.c
>> @@ -0,0 +1,25 @@
>> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
>> +/* { dg-require-effective-target trampolines } */
>> +/* { dg-options "-fhardened -O -Wno-error" } */
>> +
>> +static void
>> +baz (int (*bar) (void))
>> +{
>> +  bar ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 6;
>> +
>> +  int
>> +  bar (void)	// { dg-warning "trampoline" }
>> +  {
>> +    return a;
>> +  }
>> +
>> +  baz (bar);
>> +
>> +  return 0;
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/fhardened-4.c b/gcc/testsuite/gcc.dg/fhardened-4.c
>> new file mode 100644
>> index 00000000000..7e62ed3385d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/fhardened-4.c
>> @@ -0,0 +1,25 @@
>> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
>> +/* { dg-require-effective-target trampolines } */
>> +/* { dg-options "-fhardened -O -Wno-error=trampolines" } */
>> +
>> +static void
>> +baz (int (*bar) (void))
>> +{
>> +  bar ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 6;
>> +
>> +  int
>> +  bar (void)	// { dg-warning "trampoline" }
>> +  {
>> +    return a;
>> +  }
>> +
>> +  baz (bar);
>> +
>> +  return 0;
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/fhardened-5.c b/gcc/testsuite/gcc.dg/fhardened-5.c
>> new file mode 100644
>> index 00000000000..5d3f0dcae8e
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/fhardened-5.c
>> @@ -0,0 +1,27 @@
>> +/* { dg-do compile { target *-*-linux* *-*-gnu* } } */
>> +/* { dg-require-effective-target trampolines } */
>> +/* { dg-options "-fhardened -O -Wtrampolines" } */
>> +
>> +static void
>> +baz (int (*bar) (void))
>> +{
>> +  bar ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> +  int a = 6;
>> +
>> +  int
>> +  bar (void)	// { dg-error "trampoline" }
>> +  {
>> +    return a;
>> +  }
>> +
>> +  baz (bar);
>> +
>> +  return 0;
>> +}
>> +
>> +/* { dg-prune-output "some warnings being treated as errors" } */
>> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
>> index 85450d97a1a..2f0ac74dee0 100644
>> --- a/gcc/toplev.cc
>> +++ b/gcc/toplev.cc
>> @@ -1682,7 +1682,7 @@ process_options ()
>>     flag_ipa_ra = 0;
>> 
>>   /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
>> -     have not been set.  */
>> +     have not been set.  Also enable -Werror=trampolines for -fhardened.  */
>>   if (!OPTION_SET_P (warnings_are_errors))
>>     {
>>       if (warn_coverage_mismatch
>> @@ -1693,6 +1693,12 @@ process_options ()
>> 	  && option_unspecified_p (OPT_Wcoverage_invalid_line_number))
>> 	diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_invalid_line_number,
>> 					DK_ERROR, UNKNOWN_LOCATION);
>> +
>> +      if (flag_hardened
>> +	  && warn_trampolines
>> +	  && option_unspecified_p (OPT_Wtrampolines))
>> +	diagnostic_classify_diagnostic (global_dc, OPT_Wtrampolines,
>> +					DK_ERROR, UNKNOWN_LOCATION);
>>     }
>> 
>>   /* Save the current optimization options.  */
>> 
>> base-commit: b8edb812ff4934c609fdfafe2e1c7f932bc18305
>> -- 
>> 2.42.0
>> 
> 

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Siddhesh Poyarekar]

On 2023-12-02 04:42, Martin Uecker wrote:
> 
>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>
>> -- >8 --
>> It came up that a good hardening strategy is to disable trampolines
>> which may require executable stack.  Therefore the following patch
>> adds -Werror=trampolines to -fhardened.
> 
> This would add a warning about specific code (where it is then
> unclear whether rewriting it is feasible or even an improvement),
> which seems different to all the other flags -fhardening has
> now.

It's actually -Werror=trampolines, not just -Wtrampolines; the aim is to 
hard fail on producing trampolines and consequently, an executable 
stack.  In general the goal of -fhardened is to produce hardened code 
and the nested function trampolines do the exact reverse of that, so 
-Werror=trampolines seems to align perfectly with that goal, doesn't it?

> GCC now has an option to allocate trampolines on the heap,
> which would seem to be a better fit.  On the other hand,
> it does not work with longjmp which may be a limitation.

For hardened code in C, I think we really should look to step away from 
nested functions instead of adding ways to continue supporting it. 
There's probably a larger conversation to be had about the utility of 
nested functions in general for C (and whether this GCC extension should 
be deprecated altogether in future), but I feel like the -fhardened 
subset gives us the opportunity to enforce at least a safe subset for 
now, possibly extending it in future.

Thanks,
Sid

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Andreas Schwab]

On Dez 04 2023, Siddhesh Poyarekar wrote:

> For hardened code in C, I think we really should look to step away from
> nested functions instead of adding ways to continue supporting it. There's
> probably a larger conversation to be had about the utility of nested
> functions in general for C (and whether this GCC extension should be
> deprecated altogether in future), but I feel like the -fhardened subset
> gives us the opportunity to enforce at least a safe subset for now,
> possibly extending it in future.

Nested functions by itself don't need a trampoline, only if the address
of it is passed outside the containing function's scope (as a callback,
for example).

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Jakub Jelinek]

On Mon, Dec 04, 2023 at 05:39:04PM +0100, Andreas Schwab wrote:
> On Dez 04 2023, Siddhesh Poyarekar wrote:
> 
> > For hardened code in C, I think we really should look to step away from
> > nested functions instead of adding ways to continue supporting it. There's
> > probably a larger conversation to be had about the utility of nested
> > functions in general for C (and whether this GCC extension should be
> > deprecated altogether in future), but I feel like the -fhardened subset
> > gives us the opportunity to enforce at least a safe subset for now,
> > possibly extending it in future.
> 
> Nested functions by itself don't need a trampoline, only if the address
> of it is passed outside the containing function's scope (as a callback,
> for example).

And only if the code to which it is passed can't be inlined back.

I'm afraid contained functions in Fortran or in Ada (whatever it is called
there) aren't going away any time soon and having the possibility to test it
also in C and not just Fortran/Ada is very useful at least from compiler
testing POV.

	Jakub

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Siddhesh Poyarekar]

On 2023-12-04 11:39, Andreas Schwab wrote:
> On Dez 04 2023, Siddhesh Poyarekar wrote:
> 
>> For hardened code in C, I think we really should look to step away from
>> nested functions instead of adding ways to continue supporting it. There's
>> probably a larger conversation to be had about the utility of nested
>> functions in general for C (and whether this GCC extension should be
>> deprecated altogether in future), but I feel like the -fhardened subset
>> gives us the opportunity to enforce at least a safe subset for now,
>> possibly extending it in future.
> 
> Nested functions by itself don't need a trampoline, only if the address
> of it is passed outside the containing function's scope (as a callback,
> for example).

Yes, that's why I said that the conversation about deprecating the C 
nested functions extension is a broader one (and hence for gcc 15) that 
will likely involve the question of whether dropping the extension 
altogether gives any benefit or if dropping support for on-stack 
trampolines is sufficient.  On-heap trampolines are maybe slightly 
better in that they don't need an executable stack, but defaulting to 
on-heap trampolines for -fhardened seems like a lost opportunity to 
enforce better user code.

Thanks,
Sid

Re: [PATCH] gcc: Disallow trampolines when -fhardened

[Martin Uecker]

Am Montag, dem 04.12.2023 um 11:46 -0500 schrieb Siddhesh Poyarekar:
> On 2023-12-04 11:39, Andreas Schwab wrote:
> > On Dez 04 2023, Siddhesh Poyarekar wrote:
> > 
> > > For hardened code in C, I think we really should look to step away from
> > > nested functions instead of adding ways to continue supporting it. There's
> > > probably a larger conversation to be had about the utility of nested
> > > functions in general for C (and whether this GCC extension should be
> > > deprecated altogether in future), but I feel like the -fhardened subset
> > > gives us the opportunity to enforce at least a safe subset for now,
> > > possibly extending it in future.
> > 
> > Nested functions by itself don't need a trampoline, only if the address
> > of it is passed outside the containing function's scope (as a callback,
> > for example).
> 
> Yes, that's why I said that the conversation about deprecating the C 
> nested functions extension is a broader one (and hence for gcc 15) that 
> will likely involve the question of whether dropping the extension 
> altogether gives any benefit or if dropping support for on-stack 
> trampolines is sufficient.  On-heap trampolines are maybe slightly 
> better in that they don't need an executable stack, but defaulting to 
> on-heap trampolines for -fhardened seems like a lost opportunity to 
> enforce better user code.

I do not really agree with that.  Nested functions can substantially
improve code quality and in C can avoid type unsafe use of
void* pointers in callbacks. The code is often much better with
nested functions than without.  Nested functions and lambdas
(i.e. anonymous nested functions) are used in many languages
because they make code better and GNU's nested function are no
exception.

So I disagree with the idea that discouraging nested functions leads 
to better code - I think the exact opposite is true.

I am generally wary of mitigations that may make exploitation of
buffer overflows a bit harder  while increasing the likelihood
of buffer overflows by reducing type safety and/or code quality.

But I would agree that trampolines are generally problematic. A
better strategy would be wide function pointer type (as in Apple'
Blocks extension). Alternatively, an explicit way to obtain the
static chain for a nested function which could be used with 
__builtin_call_with_static_chain  could also work.

But in any case, I think it diminishes the value of -fhardening 
it if requires source code changes, because then it is not as easy
to simply turn it on in larger projects / distributitions. 

Martin



> 
> Thanks,
> Sid

[gcc15] nested functions in C

[Siddhesh Poyarekar]

[Branching this into a separate conversation to avoid derailing the 
patch, which isn't directly related]

On 2023-12-04 12:21, Martin Uecker wrote:
> I do not really agree with that.  Nested functions can substantially
> improve code quality and in C can avoid type unsafe use of
> void* pointers in callbacks. The code is often much better with
> nested functions than without.  Nested functions and lambdas
> (i.e. anonymous nested functions) are used in many languages
> because they make code better and GNU's nested function are no
> exception.
> 
> So I disagree with the idea that discouraging nested functions leads
> to better code - I think the exact opposite is true.

I would argue that GNU's nested functions *are* an exception because 
they're like feathers stuck on a pig to try and make it fly; I think a 
significant specification effort is required to actually make it a 
cleanly usable feature.  It *may* be possible to implement patterns that 
use C nested functions well enough *and* result in readable code, but 
IMO it is easier to write clunky and unmaintainable code with it.

I empathize with Jakub's stated use case though of keeping the C 
frontend support for testing purposes, but that could easily be done 
behind a flag, or by putting nested C func deprecation behind a flag.

> I am generally wary of mitigations that may make exploitation of
> buffer overflows a bit harder  while increasing the likelihood
> of buffer overflows by reducing type safety and/or code quality.
> 
> But I would agree that trampolines are generally problematic. A
> better strategy would be wide function pointer type (as in Apple'
> Blocks extension). Alternatively, an explicit way to obtain the
> static chain for a nested function which could be used with
> __builtin_call_with_static_chain  could also work.
> 
> But in any case, I think it diminishes the value of -fhardening
> it if requires source code changes, because then it is not as easy
> to simply turn it on in larger projects / distributitions.

I suppose you mean source code changes even in correct code just to 
comply with the flag?  I don't disagree for cases like -Warray-bounds, 
but for warnings/errors that are more deterministic in nature (like 
-Werror=trampolines), they're going to point at actual problems and 
larger projects and distributions will usually prefer to at least track 
them, if not actually fix them.  For Fedora we tend to provide macro 
overrides for packages that need to explicitly disable a security 
related flag.

Thanks,
Sid

Re: [gcc15] nested functions in C

[Martin Uecker]

Am Montag, dem 04.12.2023 um 13:27 -0500 schrieb Siddhesh Poyarekar:
> [Branching this into a separate conversation to avoid derailing the 
> patch, which isn't directly related]
> 
> On 2023-12-04 12:21, Martin Uecker wrote:
> > I do not really agree with that.  Nested functions can substantially
> > improve code quality and in C can avoid type unsafe use of
> > void* pointers in callbacks. The code is often much better with
> > nested functions than without.  Nested functions and lambdas
> > (i.e. anonymous nested functions) are used in many languages
> > because they make code better and GNU's nested function are no
> > exception.
> > 
> > So I disagree with the idea that discouraging nested functions leads
> > to better code - I think the exact opposite is true.
> 
> I would argue that GNU's nested functions *are* an exception because 
> they're like feathers stuck on a pig to try and make it fly; I think a 
> significant specification effort is required to actually make it a 
> cleanly usable feature.  It *may* be possible to implement patterns that 
> use C nested functions well enough *and* result in readable code, but 
> IMO it is easier to write clunky and unmaintainable code with it.

I use them in my code a lot and I think they improve
code quality.  For example:

int foo_find(int N, struct foo in_array[N], const char* *key)
{
  bool cond(struct foo* x)
  {
    return 0 == strcmp(x->name, key); 
  }
  return find(N, in_array, cond);
}

is a lot cleaner and safer than what you need to write
without nested functions:

struct foo_find {
  const char* name;
}; 

int foo_cond(void *vdata, struct foo* a)
{
  struct foo *key = data;
  return 0 == strcmp(x->name, key->name);  
}

void foo_sort(int N, struct foo in_array[N], const char* key)
{
  struct foo_find data = { key };
  sort(N, in_array, foo_cond, &data);
}

and this is a toy example, the improvement gets more 
substantial with more complicated logic.

> 
> I empathize with Jakub's stated use case though of keeping the C 
> frontend support for testing purposes, but that could easily be done 
> behind a flag, or by putting nested C func deprecation behind a flag.

I am relatively sure C will get some form of nested functions.
Maybe as anonymous nested functions, i.e. lambdas, but I do
not see a fundamental difference here (I personally like naming
things for clarity, so i prefer named nested functions)

> > I am generally wary of mitigations that may make exploitation of
> > buffer overflows a bit harder  while increasing the likelihood
> > of buffer overflows by reducing type safety and/or code quality.
> > 
> > But I would agree that trampolines are generally problematic. A
> > better strategy would be wide function pointer type (as in Apple'
> > Blocks extension). Alternatively, an explicit way to obtain the
> > static chain for a nested function which could be used with
> > __builtin_call_with_static_chain  could also work.
> > 
> > But in any case, I think it diminishes the value of -fhardening
> > it if requires source code changes, because then it is not as easy
> > to simply turn it on in larger projects / distributitions.
> 
> I suppose you mean source code changes even in correct code just to 
> comply with the flag?  

Yes

> I don't disagree for cases like -Warray-bounds, 
> but for warnings/errors that are more deterministic in nature (like 
> -Werror=trampolines), they're going to point at actual problems and 
> larger projects and distributions will usually prefer to at least track 
> them, if not actually fix them.  For Fedora we tend to provide macro 
> overrides for packages that need to explicitly disable a security 
> related flag.

In projects such as mine, this will lead to a lot of code
transformations as indicated above, i.e. much worse code. 

One could get away with it, since nested functions are rarely
used, but I think this is bad, because a lot of code would
improve if it used them.

Martin

> 
> Thanks,
> Sid

Re: [gcc15] nested functions in C

[Siddhesh Poyarekar]

On 2023-12-04 13:48, Martin Uecker wrote:
>> I empathize with Jakub's stated use case though of keeping the C
>> frontend support for testing purposes, but that could easily be done
>> behind a flag, or by putting nested C func deprecation behind a flag.
> 
> I am relatively sure C will get some form of nested functions.
> Maybe as anonymous nested functions, i.e. lambdas, but I do
> not see a fundamental difference here (I personally like naming
> things for clarity, so i prefer named nested functions)

If (assuming from them being called lambdas) they are primarily for 
small functions without side-effects then it's already a significantly 
stronger specification than what we have right now with C nested 
functions.  That would end up enforcing what you demonstrate as the good 
way to use nested functions.

I suppose minimal, contained side-effects (such as atomically updating a 
structure) may also constitute sound design, but that should be made 
explicit in the language.

>> I don't disagree for cases like -Warray-bounds,
>> but for warnings/errors that are more deterministic in nature (like
>> -Werror=trampolines), they're going to point at actual problems and
>> larger projects and distributions will usually prefer to at least track
>> them, if not actually fix them.  For Fedora we tend to provide macro
>> overrides for packages that need to explicitly disable a security
>> related flag.
> 
> In projects such as mine, this will lead to a lot of code
> transformations as indicated above, i.e. much worse code.
> 
> One could get away with it, since nested functions are rarely
> used, but I think this is bad, because a lot of code would
> improve if it used them.

If nested functions are eventually going to make it into the C standard 
then effort is probably better spent in porting the C nested functions 
to use descriptors instead of executable stacks or heaps.

Thanks,
Sid

Re: [gcc15] nested functions in C

[Martin Uecker]

Am Montag, dem 04.12.2023 um 15:35 -0500 schrieb Siddhesh Poyarekar:
> On 2023-12-04 13:48, Martin Uecker wrote:
> > > I empathize with Jakub's stated use case though of keeping the C
> > > frontend support for testing purposes, but that could easily be done
> > > behind a flag, or by putting nested C func deprecation behind a flag.
> > 
> > I am relatively sure C will get some form of nested functions.
> > Maybe as anonymous nested functions, i.e. lambdas, but I do
> > not see a fundamental difference here (I personally like naming
> > things for clarity, so i prefer named nested functions)
> 
> If (assuming from them being called lambdas) they are primarily for 
> small functions without side-effects then it's already a significantly 
> stronger specification than what we have right now with C nested 
> functions.  That would end up enforcing what you demonstrate as the good 
> way to use nested functions.

The proposal we have seen for C23 (which was not accepted into
C23 mostly due to timing and lack of implementation experience)
were similar to C++'s lambdas and did not have any such restriction.

> 
> I suppose minimal, contained side-effects (such as atomically updating a 
> structure) may also constitute sound design, but that should be made 
> explicit in the language.

Updating some variable is useful for example for contractions, e.g.
summing over a certain range of values in an array, etc.

> 
> > > I don't disagree for cases like -Warray-bounds,
> > > but for warnings/errors that are more deterministic in nature (like
> > > -Werror=trampolines), they're going to point at actual problems and
> > > larger projects and distributions will usually prefer to at least track
> > > them, if not actually fix them.  For Fedora we tend to provide macro
> > > overrides for packages that need to explicitly disable a security
> > > related flag.
> > 
> > In projects such as mine, this will lead to a lot of code
> > transformations as indicated above, i.e. much worse code.
> > 
> > One could get away with it, since nested functions are rarely
> > used, but I think this is bad, because a lot of code would
> > improve if it used them.
> 
> If nested functions are eventually going to make it into the C standard 
> then effort is probably better spent in porting the C nested functions 
> to use descriptors instead of executable stacks or heaps.

I submitted a patch for this a long time ago which was based
on the code for Ada that uses a bit in the pointer to differentiate
between conventional pointers and descriptors.

I would now prefer an approach that uses a qualifier on the
function type to indicate that the static chain has to be
set. A pointer to such a qualified function would a descriptor
that consists of the address and the value for the static chain.

This would be useful for many things.

Martin

Re: [gcc15] nested functions in C

[Siddhesh Poyarekar]

On 2023-12-04 16:31, Martin Uecker wrote:
>> If (assuming from them being called lambdas) they are primarily for
>> small functions without side-effects then it's already a significantly
>> stronger specification than what we have right now with C nested
>> functions.  That would end up enforcing what you demonstrate as the good
>> way to use nested functions.
> 
> The proposal we have seen for C23 (which was not accepted into
> C23 mostly due to timing and lack of implementation experience)
> were similar to C++'s lambdas and did not have any such restriction.

Oh well :/

>> If nested functions are eventually going to make it into the C standard
>> then effort is probably better spent in porting the C nested functions
>> to use descriptors instead of executable stacks or heaps.
> 
> I submitted a patch for this a long time ago which was based
> on the code for Ada that uses a bit in the pointer to differentiate
> between conventional pointers and descriptors.
> 
> I would now prefer an approach that uses a qualifier on the
> function type to indicate that the static chain has to be
> set. A pointer to such a qualified function would a descriptor
> that consists of the address and the value for the static chain.
> 
> This would be useful for many things.

Ack, this probably becomes a gcc15 thing then, given that stage 1 has 
closed.  Are you planning to revive your work and repost?

Thanks,
Sid

Re: [gcc15] nested functions in C

[Joseph Myers]

On Mon, 4 Dec 2023, Siddhesh Poyarekar wrote:

> On 2023-12-04 13:48, Martin Uecker wrote:
> > > I empathize with Jakub's stated use case though of keeping the C
> > > frontend support for testing purposes, but that could easily be done
> > > behind a flag, or by putting nested C func deprecation behind a flag.
> > 
> > I am relatively sure C will get some form of nested functions.
> > Maybe as anonymous nested functions, i.e. lambdas, but I do
> > not see a fundamental difference here (I personally like naming
> > things for clarity, so i prefer named nested functions)
> 
> If (assuming from them being called lambdas) they are primarily for small
> functions without side-effects then it's already a significantly stronger
> specification than what we have right now with C nested functions.  That would
> end up enforcing what you demonstrate as the good way to use nested functions.

The key feature of lambdas (which failed to make it into C23) for this 
purpose is that you can't convert them to function pointers, which 
eliminates any need for trampolines.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [gcc15] nested functions in C

[Martin Uecker]

Am Montag, dem 04.12.2023 um 21:33 +0000 schrieb Joseph Myers:
> On Mon, 4 Dec 2023, Siddhesh Poyarekar wrote:
> 
> > On 2023-12-04 13:48, Martin Uecker wrote:
> > > > I empathize with Jakub's stated use case though of keeping the C
> > > > frontend support for testing purposes, but that could easily be done
> > > > behind a flag, or by putting nested C func deprecation behind a flag.
> > > 
> > > I am relatively sure C will get some form of nested functions.
> > > Maybe as anonymous nested functions, i.e. lambdas, but I do
> > > not see a fundamental difference here (I personally like naming
> > > things for clarity, so i prefer named nested functions)
> > 
> > If (assuming from them being called lambdas) they are primarily for small
> > functions without side-effects then it's already a significantly stronger
> > specification than what we have right now with C nested functions.  That would
> > end up enforcing what you demonstrate as the good way to use nested functions.
> 
> The key feature of lambdas (which failed to make it into C23) for this 
> purpose is that you can't convert them to function pointers, which 
> eliminates any need for trampolines.

And also makes them useful only for template-like macro programming,
but not much else. So my understanding was that this needs to be 
addressed at some point. 

Martin

> 

Re: [gcc15] nested functions in C

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Mon, 4 Dec 2023, Martin Uecker wrote:

> > The key feature of lambdas (which failed to make it into C23) for this 
> > purpose is that you can't convert them to function pointers, which 
> > eliminates any need for trampolines.
> 
> And also makes them useful only for template-like macro programming,
> but not much else. So my understanding was that this needs to be 
> addressed at some point. 

Where "addressed" probably means some kind of callable object that stores 
more than just a function pointer in order to be able to encapsulate both 
the code address of a lambda and the context it needs to receive 
implicitly.  So still not needing trampolines.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [gcc15] nested functions in C

[Martin Uecker]

Am Dienstag, dem 05.12.2023 um 21:08 +0000 schrieb Joseph Myers:
> On Mon, 4 Dec 2023, Martin Uecker wrote:
> 
> > > The key feature of lambdas (which failed to make it into C23) for this 
> > > purpose is that you can't convert them to function pointers, which 
> > > eliminates any need for trampolines.
> > 
> > And also makes them useful only for template-like macro programming,
> > but not much else. So my understanding was that this needs to be 
> > addressed at some point. 
> 
> Where "addressed" probably means some kind of callable object that stores 
> more than just a function pointer in order to be able to encapsulate both 
> the code address of a lambda and the context it needs to receive 
> implicitly.  So still not needing trampolines.

Yes, a wide function pointer type similar to C++'s std::function.

This would also be a way to eliminate the need for trampolines
for GCC's nested function.

Martin
> 

Re: [gcc15] nested functions in C

[Richard Biener]

On Tue, Dec 5, 2023 at 10:16 PM Martin Uecker <uecker@tugraz.at> wrote:
>
> Am Dienstag, dem 05.12.2023 um 21:08 +0000 schrieb Joseph Myers:
> > On Mon, 4 Dec 2023, Martin Uecker wrote:
> >
> > > > The key feature of lambdas (which failed to make it into C23) for this
> > > > purpose is that you can't convert them to function pointers, which
> > > > eliminates any need for trampolines.
> > >
> > > And also makes them useful only for template-like macro programming,
> > > but not much else. So my understanding was that this needs to be
> > > addressed at some point.
> >
> > Where "addressed" probably means some kind of callable object that stores
> > more than just a function pointer in order to be able to encapsulate both
> > the code address of a lambda and the context it needs to receive
> > implicitly.  So still not needing trampolines.
>
> Yes, a wide function pointer type similar to C++'s std::function.
>
> This would also be a way to eliminate the need for trampolines
> for GCC's nested function.

And conversion to ordinary function pointer types would be still
possible by using (on heap) trampolines then and would offer
backward compatibility.  I wonder how much implementation work
would it be to add the wide function pointer types (please hide
details in the C frontend).

Richard.

> Martin
> >
>

Re: [gcc15] nested functions in C

[Jakub Jelinek]

On Mon, Dec 04, 2023 at 01:27:32PM -0500, Siddhesh Poyarekar wrote:
> [Branching this into a separate conversation to avoid derailing the patch,
> which isn't directly related]
> 
> On 2023-12-04 12:21, Martin Uecker wrote:
> > I do not really agree with that.  Nested functions can substantially
> > improve code quality and in C can avoid type unsafe use of
> > void* pointers in callbacks. The code is often much better with
> > nested functions than without.  Nested functions and lambdas
> > (i.e. anonymous nested functions) are used in many languages
> > because they make code better and GNU's nested function are no
> > exception.
> > 
> > So I disagree with the idea that discouraging nested functions leads
> > to better code - I think the exact opposite is true.
> 
> I would argue that GNU's nested functions *are* an exception because they're
> like feathers stuck on a pig to try and make it fly; I think a significant
> specification effort is required to actually make it a cleanly usable
> feature.

Why?  The syntax doesn't seem to be something unexpected, and as C doesn't
have lambdas, one can use the nested functions instead.
The only problem is if you need to pass function pointers somewhere else
(and target doesn't have function descriptors or something similar), if it
is only done to make code more readable compared to say use of macros, I
think the nested functions are better, one doesn't have to worry about
multiple evaluations of argument side-effects etc.  And if everything is
inlined and SRA optimized, there is no extra cost.
The problem of passing it as a function pointer to other functions is
common with C++, only lambdas which don't capture anything actually can be
convertible to function pointer, for anything else you need a template and
instantiate it for a particular lambda (which is something you can't do in
C).

	Jakub

Re: [gcc15] nested functions in C

[Martin Uecker]

Am Montag, dem 04.12.2023 um 19:51 +0100 schrieb Jakub Jelinek:
> On Mon, Dec 04, 2023 at 01:27:32PM -0500, Siddhesh Poyarekar wrote:
> > [Branching this into a separate conversation to avoid derailing the patch,
> > which isn't directly related]
> > 
> > On 2023-12-04 12:21, Martin Uecker wrote:
> > > I do not really agree with that.  Nested functions can substantially
> > > improve code quality and in C can avoid type unsafe use of
> > > void* pointers in callbacks. The code is often much better with
> > > nested functions than without.  Nested functions and lambdas
> > > (i.e. anonymous nested functions) are used in many languages
> > > because they make code better and GNU's nested function are no
> > > exception.
> > > 
> > > So I disagree with the idea that discouraging nested functions leads
> > > to better code - I think the exact opposite is true.
> > 
> > I would argue that GNU's nested functions *are* an exception because they're
> > like feathers stuck on a pig to try and make it fly; I think a significant
> > specification effort is required to actually make it a cleanly usable
> > feature.
> 
> Why?  The syntax doesn't seem to be something unexpected, and as C doesn't
> have lambdas, one can use the nested functions instead.
> The only problem is if you need to pass function pointers somewhere else
> (and target doesn't have function descriptors or something similar), if it
> is only done to make code more readable compared to say use of macros, I
> think the nested functions are better, one doesn't have to worry about
> multiple evaluations of argument side-effects etc.  And if everything is
> inlined and SRA optimized, there is no extra cost.
> The problem of passing it as a function pointer to other functions is
> common with C++, only lambdas which don't capture anything actually can be
> convertible to function pointer, for anything else you need a template and
> instantiate it for a particular lambda (which is something you can't do in
> C).

In C++ you can erase the type with std::function.  C is missing a 
function pointer type which can encapsulate the static chain on
all archs (not only for nested functions, also for language 
interoperability).

Martin

> 

Re: [gcc15] nested functions in C

[Siddhesh Poyarekar]

On 2023-12-04 13:51, Jakub Jelinek wrote:
> Why?  The syntax doesn't seem to be something unexpected, and as C doesn't
> have lambdas, one can use the nested functions instead.
> The only problem is if you need to pass function pointers somewhere else
> (and target doesn't have function descriptors or something similar), if it
> is only done to make code more readable compared to say use of macros, I
> think the nested functions are better, one doesn't have to worry about
> multiple evaluations of argument side-effects etc.  And if everything is
> inlined and SRA optimized, there is no extra cost.
> The problem of passing it as a function pointer to other functions is
> common with C++, only lambdas which don't capture anything actually can be
> convertible to function pointer, for anything else you need a template and
> instantiate it for a particular lambda (which is something you can't do in
> C).

I think from a language standpoint, the general idea that nested 
functions are just any functions inside functions (which is how the C 
nested functions essentially behave) is too broad and they should be 
restricted to minimal implementations that, e.g. don't have side-effects 
or if they do, there's explicit syntactic sugar to make it clearer.

If (like Martin stated earlier), nested functions are in fact going to 
enter the C standard in future then maybe this discussion is moot and we 
probably are better off implementing descriptor support for C to replace 
the on-stack trampolines instead of adding -Werror=trampolines in a hurry.

Thanks,
Sid

Re: [gcc15] nested functions in C

[Eric Botcazou]

> I think from a language standpoint, the general idea that nested
> functions are just any functions inside functions (which is how the C
> nested functions essentially behave) is too broad and they should be
> restricted to minimal implementations that, e.g. don't have side-effects
> or if they do, there's explicit syntactic sugar to make it clearer.

That sounds totally arbitrary though.  Algol-derived languages have had nested 
subprograms for ages, e.g. Pascal or Ada, and they can be very useful.

-- 
Eric Botcazou

Re: [gcc15] nested functions in C

[Siddhesh Poyarekar]

On 2023-12-07 10:42, Eric Botcazou wrote:
>> I think from a language standpoint, the general idea that nested
>> functions are just any functions inside functions (which is how the C
>> nested functions essentially behave) is too broad and they should be
>> restricted to minimal implementations that, e.g. don't have side-effects
>> or if they do, there's explicit syntactic sugar to make it clearer.
> 
> That sounds totally arbitrary though.  Algol-derived languages have had nested
> subprograms for ages, e.g. Pascal or Ada, and they can be very useful.

I'll admit that it is a subjective preference and is probably not in the 
spirit of traditional C.

Sid