| Message ID | 20231201193359.108618-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | gcc: Disallow trampolines when -fhardened | expand |
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 > >
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
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
> 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 >
> 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 >> >
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
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).
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
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
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
[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
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
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
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 >
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
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
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
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.
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 >
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
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.
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 >
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 > > >
> 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!).
> 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.
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
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. */