[PR25509] can't disable __attribute__((warn_unused_result))

Subject: Re: New: can't voidify __attribute__((warn_unused_result))

On Tue, 20 Dec 2005, mueller at kde dot org wrote:

> casting to (void) doesn't avoid the unused_result warning. testcase:

Why do you think this is a bug? warn_unused_result is for cases where
"not checking the result is either a security problem or always a bug".

background: glibc 2.3 CVS attributes "fwrite" and "write" with it, and it causes a lot (in the hundreds/thousands) of false positives for bigger software projects, because while it is indeed the case that they ignore the return value, it simply doesn't matter for the application (if, for example, it is used as debug output). Yes, write(2) can fail, but there are cases where the application can't possibly do anything about it, or even cares.

This is a glibc bug and not a GCC bug then.

Care to explain how it is a glibc bug? its not documented that there shouldn't be a way to suppress the warning.

I agree glibc is overly paranoid and pedantic, but that doesn't make it less of a gcc issue.

Actually it is documented that this is acting the way it is acting, just not with the docs of the attributes:
Warning when a non-void function value is ignored.
C contains many standard functions that return a value that most programs choose to ignore. One obvious example is printf. Warning about this practice only leads the defensive programmer to clutter programs with dozens of casts to void. Such casts are required so frequently that they become visual noise. Writing those casts becomes so automatic that they no longer convey useful information about the intentions of the programmer. For functions where the return value should never be ignored, use the warn_unused_result function attribute (see Function Attributes).

"should never" means it cannot be the result cannot be ignored at all (well assigning to a variable and ignoring that is a work around).

As shown this is not a GCC bug as GCC is acting as documented.

The reason why it is a glibc bug is that it is very over the top of adding the attribute here.

Subject: Re: can't voidify __attribute__((warn_unused_result))

On Wed, 21 Dec 2005, pinskia at gcc dot gnu dot org wrote:

> The reason why it is a glibc bug is that it is very over the top of adding the
> attribute here.

And indeed there is no logical difference between printf and fwrite here,
but glibc is marking fwrite and not printf.

In both cases, a valid programming style is to use fflush and ferror at
the end to check for errors, rather than checking on every write, or to
check the return value of fclose. A program that uses fwrite without
checking the return value or such a subsequent error is buggy - so is one
using printf and failing later to check for errors on stdout. (GCC is
among such buggy programs; "gcc --help >/dev/full" does not return error
status as it should.) But checking at the end suffices (albeit losing
information about the value of errno for the original error), you don't
need to check at every call.

ok, lets assume that you meant with "can not be ignored" actually "must not be ignored". now thats where the definitions in RFC2119 kick in:

2. MUST NOT This phrase, or the phrase "SHALL NOT", mean that the
   definition is an absolute prohibition of the specification.

4. SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
   there may exist valid reasons in particular circumstances when the
   particular behavior is acceptable or even useful, but the full
   implications should be understood and the case carefully weighed
   before implementing any behavior described with this label.

The documentation correctly states SHOULD NOT, and thats distinctively different from MUST NOT.

I already agreed that glibc is over the top, nevertheless the (void)ify trick doesn't suppress the warning, and either that behaviour is a bug or the behaviour that assigning to a dummy variable (which is never read and therefore dead storage) doesn't warn is a bug. you choose.

> ok, lets assume that you meant with "can not be ignored" actually "must not > be ignored". now thats where the definitions in RFC2119 kick in:

Hmm, that wasn't meant so harsh than it sounds after rereading. sorry about that.

The (technical) problem that the void cast does not work is that the warning
is applied after gimplification, which strips the cast to void. So, this is
another case where warnings from the middle-end show their bad side - not that
it would be easy to move to the frontend(s).

This is at least a bug because the warning cannot be disabled:

      if (lookup_attribute ("warn_unused_result", TYPE_ATTRIBUTES (ftype)))
        {
          if (fdecl)
            warning (0, "%Hignoring return value of %qD, "
                     "declared with attribute warn_unused_result",
                     EXPR_LOCUS (t), fdecl);
          else
            warning (0, "%Hignoring return value of function "
                     "declared with attribute warn_unused_result",
                     EXPR_LOCUS (t));
        }

so, confirmed. Suggestions for a proper -Wno-XXX identifier? -Wno-unused-result?

This "fixes" it:

*** gimplify.c (revision 108853)
--- gimplify.c (working copy)
*************** gimplify_expr (tree *expr_p, tree *pre_p
*** 4203,4210 ****
              break;
            }

! if (VOID_TYPE_P (TREE_TYPE (*expr_p))
! || fallback == fb_none)
            {
              /* Just strip a conversion to void (or in void context) and
                 try again. */
--- 4203,4211 ----
              break;
            }

! if ((VOID_TYPE_P (TREE_TYPE (*expr_p))
! || fallback == fb_none)
! && ! TREE_CODE (TREE_OPERAND (*expr_p, 0)) == CALL_EXPR)
            {
              /* Just strip a conversion to void (or in void context) and
                 try again. */

it makes the gimplifier output

main ()
{
  int D.1519;
  int D.1520;

  D.1519 = foo ();
  D.1520 = 0;
  return D.1520;
}

instead of (with the (void) cast removed):

main ()
{
  int D.1519;

  foo ();
  D.1519 = 0;
  return D.1519;
}

The question whether this is an appropriate fix or just my astonishing ability
to find ugly workarounds remains to be answered ;)

Hm, we even check in the testsuite that we still warn for (void) foo():

  check1 (); /* { dg-warning "ignoring return value of" } */
  (void) check1 (); /* { dg-warning "ignoring return value of" } */
  check1 (), bar (); /* { dg-warning "ignoring return value of" } */

Subject: Re: can't voidify __attribute__((warn_unused_result))

On Wed, 21 Dec 2005, mueller at kde dot org wrote:

> ok, lets assume that you meant with "can not be ignored" actually "must not be
> ignored". now thats where the definitions in RFC2119 kick in:

The documentation isn't written in terms of RFC2119.

> The documentation correctly states SHOULD NOT, and thats distinctively
> different from MUST NOT.

It says "should never", not "should not".

For the sort of functions this is intended for, if you really want to
ignore the return value then you should probably have a conditional and a
??? comment in every place you do so. Not simply a cast to void which as
the manual notes is visual noise.

  if (error_return()) {
    /* ??? For reason X we can't handle this error sensibly. */
    abort();
  }

(I wouldn't recommend omitting the abort there; the comment would need a
more detailed justification of why in the particular case it's safe to
carry on if the abort is omitted.)

ok, then, lets see if we can get this fixed in glibc.

Subject: Re: can't voidify __attribute__((warn_unused_result))

"mueller at kde dot org" <email address hidden> writes:

| ok, then, lets see if we can get this fixed in glibc.

good luck.

This still seems fishy to me FWIW: both gcc's implementation and documentation appear to be needlessly aggressive.

It does not matter if it is a "security" issue; if void-ifying is not an acceptable workaround, there must be at the very least a Wno-* option to disable it.

Subject: Re: can't disable __attribute__((warn_unused_result))

On Fri, 17 Oct 2008, bonzini at gnu dot org wrote:

> It does not matter if it is a "security" issue; if void-ifying is not an
> acceptable workaround, there must be at the very least a Wno-* option to
> disable it.

The workaround is to change the header declaring the function with the
attribute. There isn't an option to disable the error for calling a
prototyped function with the wrong number of arguments either; if you feel
you know better than the library author how many arguments the function
should take for a particular use case in the program, you'll need to
change the library or conform to the API it specifies. This attribute is
giving further information about the API for a function.

In the case of fwrite, for example, the only obvious case where checking
would be useless is if you already are writing an error message before
exiting with error status and so an error writing the error message could
not usefully be reported anywhere and wouldn't lead to a change of exit
status. This suggests you might have an xfwrite function that looks at
the return value and acts on it unless a static flag is set to say the
program is in the process of exiting with an error. Coding in the style
suggested by the library API should be easier than trying to work around
the API to code in another style.

> In the case of fwrite, for example, the only obvious case where checking
> would be useless is if you already are writing an error message before
> exiting with error status and so an error writing the error message could
> not usefully be reported anywhere and wouldn't lead to a change of exit
> status.

Not really. The return code of fwrite is not only useless: worse, it gives a *false* sense of security. Stuff can stay in the buffers, only to give errors when you do an fflush or an fclose, which do not have the attribute in glibc (as of July 2007).

It is much better to do

  fwrite (buf, m, n, f);
  if (fflush (f) != EOF)
    perror ("write");
  if (fclose (f) != EOF)
    perror ("close");

than checking the return code of fwrite, and that's more or less what coreutils does. Anyway this is OT, because this would be a glibc bug.

Back to the GCC point-of-view, the situation is similar to setting a format(printf) attribute on a printf-like function that also has some extension. It would work for some calls, maybe most, but not for all of them. So the solution would be to use -Wno-format, either directly or via #pragma GCC diagnostic. This warning is not mandated by any standard, after all.

Subject: Re: can't disable __attribute__((warn_unused_result))

On Fri, 17 Oct 2008, bonzini at gnu dot org wrote:

> > In the case of fwrite, for example, the only obvious case where checking
> > would be useless is if you already are writing an error message before
> > exiting with error status and so an error writing the error message could
> > not usefully be reported anywhere and wouldn't lead to a change of exit
> > status.
>
> Not really. The return code of fwrite is not only useless: worse, it gives a
> *false* sense of security. Stuff can stay in the buffers, only to give errors
> when you do an fflush or an fclose, which do not have the attribute in glibc
> (as of July 2007).
>
> It is much better to do
>
> fwrite (buf, m, n, f);
> if (fflush (f) != EOF)
> perror ("write");
> if (fclose (f) != EOF)
> perror ("close");
>
> than checking the return code of fwrite, and that's more or less what coreutils
> does. Anyway this is OT, because this would be a glibc bug.

Yes, I previously noted this as an alternative valid style in comment#6.
glibc has chosen to make one style much easier than the other and that's a
matter for the glibc maintainers, not for GCC to work around glibc.

> Back to the GCC point-of-view, the situation is similar to setting a
> format(printf) attribute on a printf-like function that also has some
> extension. It would work for some calls, maybe most, but not for all of them.
> So the solution would be to use -Wno-format, either directly or via #pragma GCC
> diagnostic. This warning is not mandated by any standard, after all.

Yes, all warnings in GCC should have options to control them as a general
principle of warning control, but some (such as in this case) would be
there more for a general principle than because they should actually be
used. GNU software should not be working around other GNU software; if
some GNU software has a problem with attributes used in glibc then in the
first instance the maintainers of both packages should try to ensure that
glibc's headers and the other software's coding style work well together.

There minimally needs to be a way of turning this warning off in GCC.

GCC should not be trying to micromanage coding styles - either of the rest of gnu software or anywhere else, but at least until you clean up every bit of your own code, there should be a way of disabling the warning clutter.

HUNDREDS OF WARNINGS ARE NO BETTER THAN NO WARNINGS AT ALL.

I can't even find errors in the pages bilge that now spews out from a normal compile. It might be and probably is appropriate with -Wall turned on.

And I really would like to be able to treat warnings as errors when they are legitimate warnings.

For now, I've hexedited cc1 to change the string so it won't be found and have to add -Wno-attributes so I don't get errors from things I might need.

I'm getting it even with -Wall turned off (version 4.3.2). And I still should be able to disable it.

Somehow GCC and gnu thinks

    int dummy93857 = fwrite( buf, 1, 1, fp );

is so far superior code to just

    fwrite( buf, 1, 1, fp );

that it now must enforce it on every possible line.

Sometimes ignoring returns is the right (or better) thing to do instead of cluttering up the code. Not every line of code is critical kernel or system code that can introduce security holes. Not every call needs to have its exact behavior on the particular instance carefully monitored.

The author of the libraries can often make a bad choice. And there are hundreds of instances - maybe 99% of them are good, but the bad ones on common functions are causing a great deal of noise. And there is not a pedantic_warn_unused_result (with a -Wunused-result which would promote it), which would be perfect for the instances noted here and more easily made. And perhaps even an error_unused_result.

I think it would be easy to argue for the large bodies of code that certain functions have return values that are conventionally ignored so should only warn at a higher level of checking than ordinary warnings. Right now I have to argue each individual case with the only options to keep it (and the pages of new warnings) or remove it (and in the few cases where it might be critical be silent).

gcc currently has no middle option.

Sometimes return values are at a point where you can't do anything anyway like the exit example. Somehow, if a printf, or an equivalent fwrite of a formatted string to stdout or stderr fails, what do you do? Errors have both probability and criticality. And there are a lot of highly improbable cases, and lots of non-critical sections. If my CPU is melting down or my memory giving errors, I have worse problems. If the number of parameters doesn't match a function declaration, it is likely an error that will cause things to fail 90% of the time. 99.99% of the time, f//read/write will return the expected value. If fclose fails, what do you do? And fwrite won't return the error, fflush might (but if it doesn't do a sync(), and writes are cached to a failing disk...).

Perhaps it is because we don't have a finer gradation (an INFO or MAY equivalent to the SHOULD/WARNING, MUST/ERROR). The lack of checking a return, at least in the cases where the functions are mainly the side-effect (and if fw...

Subject: Re: can't disable __attribute__((warn_unused_result))

Sent from my iPhone

On Nov 22, 2008, at 7:42 AM, "thomas at mich dot com" <<email address hidden>
 > wrote:

>
>
> ------- Comment #20 from thomas at mich dot com 2008-11-22 15:42
> -------
> There minimally needs to be a way of turning this warning off in GCC.
>
> GCC should not be trying to micromanage coding styles - either of
> the rest of
> gnu software or anywhere else, but at least until you clean up every
> bit of
> your own code, there should be a way of disabling the warning clutter.

Why GCC is not micromanaging at all, it just allows the developer of
the API to have the warning. So your complaints here are useless.

>
>
> HUNDREDS OF WARNINGS ARE NO BETTER THAN NO WARNINGS AT ALL.
>
> I can't even find errors in the pages bilge that now spews out from
> a normal
> compile. It might be and probably is appropriate with -Wall turned
> on.
>
> And I really would like to be able to treat warnings as errors when
> they are
> legitimate warnings.
>
> For now, I've hexedited cc1 to change the string so it won't be
> found and have
> to add -Wno-attributes so I don't get errors from things I might need.
>
> I'm getting it even with -Wall turned off (version 4.3.2). And I
> still should
> be able to disable it.
>
> Somehow GCC and gnu thinks
>
> int dummy93857 = fwrite( buf, 1, 1, fp );
>
> is so far superior code to just
>
> fwrite( buf, 1, 1, fp );
>
> that it now must enforce it on every possible line.

It is not GCC which thinks that, it is the providers of your headers
for fwriye that thinks that.

>
>
> Sometimes ignoring returns is the right (or better) thing to do
> instead of
> cluttering up the code. Not every line of code is critical kernel
> or system
> code that can introduce security holes. Not every call needs to
> have its exact
> behavior on the particular instance carefully monitored.

Again we just provide the author of the Api to say that.

>
>
> The author of the libraries can often make a bad choice.

Yes and you should complain to them instead of us then.

> And there are
> hundreds of instances - maybe 99% of them are good, but the bad ones
> on common
> functions are causing a great deal of noise. And there is not a
> pedantic_warn_unused_result (with a -Wunused-result which would
> promote it),
> which would be perfect for the instances noted here and more easily
> made. And
> perhaps even an error_unused_result.
>
> I think it would be easy to argue for the large bodies of code that
> certain
> functions have return values that are conventionally ignored so
> should only
> warn at a higher level of checking than ordinary warnings. Right
> now I have to
> argue each individual case with the only options to keep it (and the
> pages of
> new warnings) or remove it (and in the few cases where it might be
> critical be
> silent).
>
> gcc currently has no middle option.

Also this attribute is not on by default in glibc so you are asking to
turn on the style based warnings.

>
>
> Sometimes return values are at a point where you can't do anything
> anyway like
> the exit example. Somehow, ...

(In reply to comment #21)
> Sent from my iPhone

Good to know.

> > GCC should not be trying to micromanage coding styles - either of
> > the rest of gnu software or anywhere else, but at least until you
> > clean up every bit of your own code, there should be a way of disabling
> > the warning clutter.
>
> Why GCC is not micromanaging at all, it just allows the developer of
> the API to have the warning. So your complaints here are useless.

What the poster seems to be requesting is another -Wno-unused-FOO flag
to override this warning.

Subject: Bug 25509

Author: manu
Date: Fri Jul 10 07:27:32 2009
New Revision: 149458

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=149458
Log:
2009-07-10 Manuel López-Ibáñez <email address hidden>

 PR 25509
 PR 40614
 * c.opt (Wunused-result): New.
 * doc/invoke.texi: Document it.
 * c-common.c (c_warn_unused_result): Use it.
testsuite/
 * g++.dg/warn/unused-result1-Werror.c: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/unused-result1-Werror.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-common.c
    trunk/gcc/c.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog

FIXED in GCC 4.5

> > gcc currently has no middle option.
>
> Also this attribute is not on by default in glibc so you are asking to
> turn on the style based warnings.

(In reply to comment #24)
> FIXED in GCC 4.5

After having waded through this long series of comments, I am left
wondering just how this got addressed. Does --no-warn-unused-result
mean that fwrite() usage may be cast to void, or that it may be
treated as if it were a void procedure? I think it is very reasonable
to warn if a returned result is not handled. Casting to void is
a valid way to handle the result. I would like warnings when returned
results are not handled. What does the fix do?

(In reply to comment #25)
> > > gcc currently has no middle option.
> >
> > Also this attribute is not on by default in glibc so you are asking to
> > turn on the style based warnings.
>
> (In reply to comment #24)
> > FIXED in GCC 4.5
>
> After having waded through this long series of comments, I am left
> wondering just how this got addressed. Does --no-warn-unused-result
> mean that fwrite() usage may be cast to void, or that it may be
> treated as if it were a void procedure? I think it is very reasonable
> to warn if a returned result is not handled. Casting to void is
> a valid way to handle the result. I would like warnings when returned
> results are not handled. What does the fix do?

It simply adds -W[no-]unused-result and completely enables/disables all
unused result warnings.

See:
http://sourceware.org/bugzilla/show_bug.cgi?id=11959
for the glibc side of this bug (namely, fwrite() shouldn't be tagged wur).

Google ref: b/16983603.

I wouldn't call this bug fixed.

I have just found ~30 bugs in our code, where someone wrote:

  vector<int> v;
  ...
  v.empty(); // v.clear() was intended!

No problem, I'll just add warn_unused_result to vector::empty(), right?

Well, that did expose the 30 bugs above, but unfortunately I can't do that permanently, because it also exposed this false positive:

   assert(v.empty());

where assert in NDEBUG mode expanded into

  static_cast<void>(v.empty());

and triggered the warning :-(

P.S. Some of the bugs I found were in parts of the code imported from open-source projects, so it's not a problem that is specific to just Google. If the assert problem could be addressed, adding warn_unused_result to trunk libstdc++ would benefit everyone.

(In reply to Paul Pluzhnikov from comment #28)
> Well, that did expose the 30 bugs above, but unfortunately I can't do that
> permanently, because it also exposed this false positive:
>
> assert(v.empty());
>
> where assert in NDEBUG mode expanded into
>
> static_cast<void>(v.empty());

Isn't assert in NDEBUG mode guaranteed to not evaluate its argument? The above seems to violate that assumption.

In C++ you could do this:

template<typename T>
inline T ignore_result(T x __attribute__((unused)))
{
    return x;
}
extern int foo() __attribute__((warn_unused_result));

int main()
{
   ignore_result(foo());
   return 0;
}

Another alternative is to use #pragma GCC diagnostics push/ignored/pop. Ideally you could encapsulate that into a macro "ignore_result", but #pragma diagnostics does not work well in a macro definition yet (I cannot remember the PR number for this).

(In reply to Paul Pluzhnikov from comment #28)
> P.S. Some of the bugs I found were in parts of the code imported from
> open-source projects, so it's not a problem that is specific to just Google.
> If the assert problem could be addressed, adding warn_unused_result to trunk
> libstdc++ would benefit everyone.

That seems a different issue (and it will require convincing different people) than this one. Can you open a new PR if you really think it is a good idea?

See bug 66425 for the cast to (void) to ignore warn_unused_result for a single case:

It looks like clang is already doing the right thing here...
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c2

gcc should catch up.

Cheers,
Filipe

(In reply to Filipe Brandenburger from comment #31)
> gcc should catch up.

I thought Google employed some capable C/C++ engineers...

(In reply to Manuel López-Ibáñez from comment #32)
> (In reply to Filipe Brandenburger from comment #31)
> > gcc should catch up.
>
> I thought Google employed some capable C/C++ engineers...

What I meant is that those engineers, if they exist, could help GCC "catch up" (whatever that means)... gcc does not develop itself or by magic gnomes, you know.

(In reply to Manuel López-Ibáñez from comment #33)
> (In reply to Manuel López-Ibáñez from comment #32)
> > (In reply to Filipe Brandenburger from comment #31)
> > > gcc should catch up.
> >
> > I thought Google employed some capable C/C++ engineers...
>
> What I meant is that those engineers, if they exist, could help GCC "catch
> up" (whatever that means)... gcc does not develop itself or by magic gnomes,
> you know.

Hum, that still sounds worse than it sounded in my mind. Sorry, I'm dense this morning :)

Just: help us, we love you, let's make the world a better place together :)

(deleting comments in bugzilla would be helpful sometimes).

(In reply to Manuel López-Ibáñez from comments)

Don't worry, I got what you mean...

Though I don't think coming up with code to fix it is the issue here, in comment #10 a patch was provided (which admittedly I haven't tested personally) to turn a void cast into a temporary assignment (which I believe would have been optimized out later in the pipeline) so I wonder why that hasn't really gone forward...

Cheers,
Filipe