| Message ID | 20160613111111.79B3D4022A42D@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 13 Jun 2016 13:11, Florian Weimer wrote: > +static void > +write_message (const char *message) > +{ > + ssize_t unused __attribute__ ((unused)); > + for (int i = 0; i < pass; ++i) > + unused = write (STDOUT_FILENO, " ", 1); > + unused = write (STDOUT_FILENO, message, strlen (message)); are you just trying to ignore the wur markings on write ? you can use this style instead: if (write (...)) { /* ignored */ } which i'm sure can be macroed. -mike
On 06/13/2016 04:03 PM, Mike Frysinger wrote: > On 13 Jun 2016 13:11, Florian Weimer wrote: >> +static void >> +write_message (const char *message) >> +{ >> + ssize_t unused __attribute__ ((unused)); >> + for (int i = 0; i < pass; ++i) >> + unused = write (STDOUT_FILENO, " ", 1); >> + unused = write (STDOUT_FILENO, message, strlen (message)); > > are you just trying to ignore the wur markings on write ? you can use > this style instead: > if (write (...)) > { > /* ignored */ > } > > which i'm sure can be macroed. I believe the __attribute_ ((unused)) approach is the recommended way to suppress this warning. Everything else is just gaming the GCC optimizers. Thanks, Florian
On 06/13/2016 07:11 AM, Florian Weimer wrote: > Currently, printf needs more stack space than what is available with > SIGSTKSZ. This commit use the the write system call directly instead. > > Also use sig_atomic_t for the “pass” variable (for general > correctness), and restore signal handlers to their defaults, to avoid > masking crashes. > > 2016-06-13 Florian Weimer <fweimer@redhat.com> > > [BZ #20248] > * debug/tst-longjmp_chk2.c (pass): Use volatile sig_atomic_t. > (write_message): New function. > (stackoverflow_handler): Call it instead of printf, to avoid > excessive stack usage by printf. > (do_test): Restore SIGSEGV, SIGBUS default handlers. Looks good to me.
On 13 Jun 2016 16:23, Florian Weimer wrote: > On 06/13/2016 04:03 PM, Mike Frysinger wrote: > > On 13 Jun 2016 13:11, Florian Weimer wrote: > >> +static void > >> +write_message (const char *message) > >> +{ > >> + ssize_t unused __attribute__ ((unused)); > >> + for (int i = 0; i < pass; ++i) > >> + unused = write (STDOUT_FILENO, " ", 1); > >> + unused = write (STDOUT_FILENO, message, strlen (message)); > > > > are you just trying to ignore the wur markings on write ? you can use > > this style instead: > > if (write (...)) > > { > > /* ignored */ > > } > > > > which i'm sure can be macroed. > > I believe the __attribute_ ((unused)) approach is the recommended way to > suppress this warning. Everything else is just gaming the GCC optimizers. it's the game that a number of GNU project rely upon quite a bit last i looked -mike
On 06/13/2016 08:42 PM, Mike Frysinger wrote: > On 13 Jun 2016 16:23, Florian Weimer wrote: >> On 06/13/2016 04:03 PM, Mike Frysinger wrote: >>> On 13 Jun 2016 13:11, Florian Weimer wrote: >>>> +static void >>>> +write_message (const char *message) >>>> +{ >>>> + ssize_t unused __attribute__ ((unused)); >>>> + for (int i = 0; i < pass; ++i) >>>> + unused = write (STDOUT_FILENO, " ", 1); >>>> + unused = write (STDOUT_FILENO, message, strlen (message)); >>> >>> are you just trying to ignore the wur markings on write ? you can use >>> this style instead: >>> if (write (...)) >>> { >>> /* ignored */ >>> } >>> >>> which i'm sure can be macroed. >> >> I believe the __attribute_ ((unused)) approach is the recommended way to >> suppress this warning. Everything else is just gaming the GCC optimizers. > > it's the game that a number of GNU project rely upon quite a bit last > i looked Thanks. I brought this up on the GCC list. But I don't think there will be any movement unless I submit a patch … Florian
Florian Weimer wrote: > I brought this up on the GCC list. But I don't think there will be any movement > unless I submit a patch … > > Florian For what it's worth, Gnulib uses a different approach in its ignore-value module. To ignore the value returned by (say) fchown, one writes this: ignore_value (fchown (fd, 0, 0)); and the ignore_value macro is defined this way: #if 3 < __GNUC__ + (4 <= __GNUC_MINOR__) # define ignore_value(x) \ (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) #else # define ignore_value(x) ((void) (x)) #endif I never understood why GCC generated warnings for expressions cast to void, as that was the longstanding idiom for "I know this returns a value, and I don't want it."
On Wed, 15 Jun 2016, Paul Eggert wrote: > I never understood why GCC generated warnings for expressions cast to void, as > that was the longstanding idiom for "I know this returns a value, and I don't > want it." The point is that longstanding idiom implies people seeing a warning and blindly applying it, without thinking about whether there is a real bug present or whether it's really an exceptional case where ignoring the warning is OK. Unfortunately it's difficult to take technical measures against stupidity, or to enforce a particular project's coding standards for exceptions (like glibc's DIAG_*_NEEDS_COMMENT macros needing a comment to explain why they are being used).
On 06/15/2016 03:10 PM, Joseph Myers wrote: > that longstanding idiom implies people seeing a warning and > blindly applying it, without thinking about whether there is a real bug Sure, but now with Gnulib, ignore_value (E) is the new idiom for (void) (E). A careless developer blindly applying ignore_value now is like a careless developer blindly applying a void cast in the good old days.Nothing has changed, really. If GCC changes again in this area, I hope Gnulib's ignore_value continues to work. It would be a pain to have yet another musical-chairs period during which there is needless confusion.
diff --git a/debug/tst-longjmp_chk2.c b/debug/tst-longjmp_chk2.c index dae9ca0..243568c 100644 --- a/debug/tst-longjmp_chk2.c +++ b/debug/tst-longjmp_chk2.c @@ -6,15 +6,25 @@ #include <signal.h> #include <stdio.h> #include <stdlib.h> +#include <string.h> #include <sys/types.h> #include <sys/time.h> #include <sys/resource.h> +#include <unistd.h> static jmp_buf mainloop; static sigset_t mainsigset; -static int pass; +static volatile sig_atomic_t pass; +static void +write_message (const char *message) +{ + ssize_t unused __attribute__ ((unused)); + for (int i = 0; i < pass; ++i) + unused = write (STDOUT_FILENO, " ", 1); + unused = write (STDOUT_FILENO, message, strlen (message)); +} static void stackoverflow_handler (int sig) @@ -25,11 +35,9 @@ stackoverflow_handler (int sig) pass++; assert (pass < 5); sigaltstack (NULL, &altstack); - /* Using printf is not really kosher in signal handlers but we know - it will work. */ - printf ("%*sin signal handler\n", pass, ""); + write_message ("in signal handler\n"); if (altstack.ss_flags & SS_ONSTACK) - printf ("%*son alternate stack\n", pass, ""); + write_message ("on alternate stack\n"); siglongjmp (mainloop, pass); } @@ -112,6 +120,11 @@ do_test (void) else printf ("disabling alternate stack succeeded \n"); + /* Restore the signal handlers, in case we trigger a crash after the + tests above. */ + signal (SIGBUS, SIG_DFL); + signal (SIGSEGV, SIG_DFL); + return 0; }