[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #61 from Andrew Church --- For the record, I'll maintain a copy of my (unaccepted) patch to add -Wunused-result=strict at: https://achurch.org/patch-pile/#gcc (wur-strict.diff) This flag obviously shouldn't be relied on in released packages, but it may be helpful for individual users trying to work around this issue.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 Andrew Church changed: What|Removed |Added CC||achurch+gcc at achurch dot org --- Comment #40 from Andrew Church --- Created attachment 54906 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54906&action=edit POC patch to add -Wunused-result=strict In hopes of moving this along (after having run into it for the third or fourth time myself), here's a proof-of-concept patch against GCC 12.2.0 which adds "-Wunused-result=strict" for the current behavior and changes "-Wunused-result" to ignore cases in which the result is discarded by casting to void. My rationale for changing the default behavior is that the wider community consensus, as evidenced by things like the C++ (and C2x) [[nodiscard]] specification, the behavior of Clang, and the balance of comments on this bug, seems to be that casting a discarded return value to void should suppress any warning about the discarded value; and under the principle of least surprise, GCC should follow that consensus by default even if it also provides alternative behaviors.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #42 from Andrew Church --- (In reply to Sam James from comment #41) > Could you send it to the gcc-patches mailing list please? (Even if it is a > PoC). Sent as requested.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #44 from Andrew Church --- (In reply to Segher Boessenkool from comment #43) > That is not the consensus, no. "Consensus" does not mean doing what the > unthinking masses shout. Merriam-Webster disagrees: con.sen.sus 1 a: general agreement : UNANIMITY b: the judgment arrived at by most of those concerned I specifically clarified that with "wider" to indicate the larger group of "C compiler users", as opposed to what you seem to mean, "C compiler developers". And in whatever sense you choose to regard them, the judgment arrived at by most C compiler users does seem to be "cast to void should suppress a warning about a discarded value", as I described above. > So allowing casts to void > to suppress this warning means the warning becomes less useful, and people > will write worse code. That is not something GCC should encourage IMO. You seem to think that making the warning harder to work around will encourage programmers to change their code to fix the warning. In reality, they are more likely to either (1) disable the warning entirely or (2) disregard warnings in general, both of which result in considerably worse code - the situation you say you are trying to avoid. Thus my suggestion to follow the principle of least surprise and allow a void cast to disable the warning by default.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
--- Comment #46 from Andrew Church ---
(In reply to Andrew Pinski from comment #45)
> But there is no general agreement at all. If clang behavior agreed with gcc,
> then there would be consensus here. In fact gcc behavior is older than clang
> behavior makes this even more difficult and even points out that if clang
> said it implemented a compatible extension, it did not.
First, a single disagreement does not invalidate a consensus, at least in the
general meaning which I quoted ("the judgment arrived at by most of those
concerned" - note "most" and not "all"). As I discussed earlier, I still see
significantly wider support for the view that void casts should silence
discarded-value warnings, to the extent that I consider it fair to call that
view a consensus.
Second, an equally valid view of the meaning of Clang's behavior differing from
GCC's - barring any deeper knowledge of the LLVM team's decision-making, which
I do not have - is that the LLVM team merged the observed intent behind the
original extension (warn about values which should not be discarded) with its
knowledge of existing practice (use of a void cast to indicate a deliberately
discarded value) to arrive at their choice of behavior. If anything, I'd
suggest that https://bugs.llvm.org/show_bug.cgi?id=51228 being marked WONTFIX
because the developers consider it to be "working as intended" argues for this
view.
> You are basically saying all warnings will be ignored which is true.
That's not what I said, and I disagree at least with that particular phrasing.
I would, however, accept "warnings will be ignored if the user feels the effort
to fix the warning is not worth the benefit". By corollary, users in general
will take the path of least resistance to resolving a warning, and if allowing
void casts to silence specific instances of the warning encourages them to
leave the warning enabled in general, it will result in better code than if the
user gets so frustrated with the warning that they just turn it off completely.
> Using a void cast makes life too easy to ignore this behavior.
It still requires action on the part of the programmer. A programmer who
writes
system("foo");
may want to be warned that the return code from system() is discarded, whereas
a programmer who writes
(void) system("foo");
has clearly stated an intent to discard that return value, and warning about it
anyway is unnecessary noise. (Though an argument could be certainly be made
for a better word than "void", perhaps a construction like "[[discard]]
foo()".)
Is there a risk of cargo-cult syndrome? (A: "Hey B, how do I call this
external foo program?" B: "Just write '(void) system("foo")'.") Certainly.
But the same argument can be made for _any_ workaround, and I suspect that more
complex workarounds would increase, rather than decrease, that risk - the more
complex the code, the less likely the user is to read it and understand what
it's doing.
> If the api user wants to ignore the return
> values while the api developer tells you should not, then really the api
> user will have bugs. This is why the warning is there.
Demonstrably false (see above, and even your own caveat below).
> Now api developers
> have known to place this attribute on things that should not be placed on
> but that is not a compiler issue and the api user should talk with the api
> developer rather than try to change the compiler for their workaround.
Suppose the API developer's response is, "I don't care about your use case, I
want you to do things my way." What is the (non-compiler developer) user to do
then? Look for an alternative library, which may not even exist? Spend the
time to write their own? Disable the warning and accept the risk of bugs in
unrelated code? Look up the workaround du jour and throw it all over their
code base, cargo-cult style?
If I were faced with that choice, I would probably just switch to Clang.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #48 from Andrew Church --- (In reply to rusty from comment #47) > Civility please. I have no intention of trying to start a fight :) Like you, I'm just trying to improve the situation, and knowing that in my own open-source work I'm always happier when a user offers a patch instead of just a "please fix this", I've done the same here. That said, since I _am_ trying to improve the situation, I won't step back from debating the utility of the change if the developer disagrees. (And I freely admit that I can be tempted into a bit of snark from time to time...) > But accept that the > conversation on this issue is only a weak indication of consensus. I agree, which is why I gave other examples such as Clang's behavior and especially the C++/C2x standards. While I grant that even standards committees are small subsets of the total user community and are not immune to poor decision-making, I'd consider the facts that (1) the C++17 description of [[nodiscard]] called out void casts as a case which should not be diagnosed, (2) C++20 expanded on that with an explicit example of a cast-to-void call not being diagnosed, and (3) C2x, as of the current draft, also includes the call-out of void casts from C++17 (though not the explicit example from C++20), to collectively be a much stronger indicator of that consensus. > As Andrew Pinski says "people are mis-using this attribute", and Jakub > Jelinek makes a similar point. The use of _wur has changed from "ignoring > the result is criminally wrong" to "possibly wrong". I think "mis-using" is a bit harsh; the core concept (warning about a discarded return value) is a useful one, as evidenced by the feature's adoption into C++ and subsequently C2x. I grant that it's being used for a wider variety of purposes than originally intended, but since there was no other option until the relatively recent addition of [[nodiscard]], that's what people went with. I was thinking about adding a suggestion for multiple levels of warning, such as "ignoring this is almost certainly wrong" (e.g. realloc()) vs "ignoring this could be dangerous, you might want to doublecheck" (e.g. system()); in fact, [[nodiscard]] is effectively that since void casts do silence [[nodiscard]] warnings in GCC, though I don't know if the difference in behavior from _wur is intentional. But that ultimately wouldn't do anything about the present problem, which is API developers and users disagreeing on whether a return value is safe to discard - it might morph into something like "why is this function marked with the stricter _wur when it should just be [[nodiscard]]", and we're back to wanting to selectively silence _wur warnings again.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #53 from Andrew Church --- (In reply to Segher Boessenkool from comment #51) > And that is the core of why this issue reinflames once in a while: some > people > abuse the attribute, and the compiler cannot read minds. Ah, for a mindread() API... But ultimately, this is why I suggest that the compiler should leave as many decisions as possible to the end user (in this case, the API user as opposed to the API developer) - if the user has decided a particular result is safe to ignore, then anything the compiler tries to do to stop that is just an annoyance to be worked around or outright disabled. > Completely useless casts to void cluttered programs decades ago already, > we do not fear cargo cult, instead we observed it already existed. At the risk of starting a linguistic discussion, the phrase you're looking for is not "cargo cult", but "idiomatic". Using "(void)" to mean "I am explicitly discarding this value" is idiomatic, much the same way as "goodbye" is an idiomatic greeting and not a literal wish for God to be with the listener. I'm probably not as old as some developers here, but I'm certainly old enough to remember some compilers which gleefully spit out warnings on practically every non-void expression, and liberal use of (void) was needed if you wanted any chance at seeing the useful warnings. "Cargo cult" programming is when people start using (void) without even knowing what it does: "Hey, so what does this 'void' thing do?" "Uh, I dunno, it's all over the code so I just copied it." I wouldn't be surprised to find cases of that as well, but I don't think that's the specific thing we're worried about here. > Changing the behaviour of this attribute after all that time will not make > things better. But perhaps we can say a bit more in the documentation, > maybe at one of the three very concise quotes above? Say half a line worth? If glibc changes from _wur to [[nodiscard]], as Florian suggested may happen, that would resolve the immediate case that brought me here (a "best-effort" system() call, where the success or failure of the call or the program executed is irrelevant to the caller). I do fear leaving the current behavior as is could just be kicking the can down the road, so to speak, but perhaps a slight edit here might help: > 'warn_unused_result' > The 'warn_unused_result' attribute causes a warning to be emitted > if a caller of the function with this attribute does not use its > return value. This is useful for functions where not checking the > result is either a security problem or always a bug, such as > 'realloc'. I would suggest removing "either a security problem or", and adding something along the lines of: "For cases in which not checking the result carries risks but is not necessarily an error, use the [[nodiscard]] attribute, which allows the caller to suppress the warning by explicitly casting the result to void." Writing user documentation isn't (remotely!) my specialty, but I think something like this could both help clarify that the two behave differently and let people know that yes, the fact that you can't silence _wur calls is intentional.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #65 from Andrew Church --- (In reply to Segher Boessenkool from comment #63) > So you are asking the compiler to warn whenever you do not use the result > of a function call, and at the same time you do not use the result of a > function call (you merely cast that return value to a different type, but > you do not do anything with that). Casting something to a different type > is not using it! You're confusing two different people with "you" here. Yes, from the compiler's point of view, it's all one translation unit, but on the real-world side, the person who wrote the top-level source file (not using the result) is not the same person who wrote the function declaration (asking to warn when not using the result), and the former has effectively no control over the latter. From the former's point of view, this seems to be to be a reasonable request. (In reply to uecker from comment #64) > I think it is reasonable to want to suppress this using (void). But > [[nodiscard]] does this and I haven't heard any convincing reason why it > could not be used. As one of the advocates for this behavior, it stems (at least in my case) from pre-C23 code in which [[attribute]] syntax was not available. If [[nodiscard]] suppresses the warning, I'd accept that as a solution.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #66 from Andrew Church --- (In reply to Andrew Church from comment #65) > As one of the advocates for this behavior, it stems (at least in my case) > from pre-C23 code in which [[attribute]] syntax was not available. If > [[nodiscard]] suppresses the warning, I'd accept that as a solution. Premature reply (apologies) - I somehow gave myself the impression that [[nodiscard]] could be put at the place of use. Since it's something the library writer has to do, I think this is still not enough from the library user's point of view.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 --- Comment #68 from Andrew Church --- (In reply to uecker from comment #67) > But also the library could switch to "discard" or add a condition that the > lets the user of the library choose it. The issue here is that the library user has no control over what the library author chooses to do. If the library author does not make that change, the user currently has no recourse (other than the pragma workaround you suggest). > As a last resort, on the compiler side one can already use a pragma to turn > off the > warning at a specific point. While true, this also introduces both a compiler-specific hack and a lot of verboseness around what ought to be a simple declaration of "I wish to ignore this return value", and I feel like most code authors who encounter this problem are more likely to add -Wno-unused-result to their compiler flags (thus losing the check everywhere) than do a whole #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" system(foo); #pragma GCC diagnostic pop just to make that one instance go away. I do agree that "(void)" is very idiomatic, and something like a [[discard]] statement attribute (which would silence warnings for both __attribute__((wur)) and [[nodiscard]]) would make the intent clearer. Perhaps something to suggest for a future version of the C standard?
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
--- Comment #70 from Andrew Church ---
(In reply to Jonathan Wakely from comment #69)
> Maybe you want:
>
> [[maybe_unused]] auto _ = foo();
If I could apply that attribute to the value itself, i.e.:
[[maybe_unused]] foo();
that would do what I want. Since it only applies to a declaration, we're still
left with idiosyncratic code ("auto _ =" instead of "(void)") which ideally
should not be needed.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
--- Comment #77 from Andrew Church ---
(In reply to Segher Boessenkool from comment #72)
> if (foo()) {
> /* The return value of foo can be ignored here because X and Y. */
> }
This is just another idiom, with "if(){}" replacing "(void)"; it does not
directly indicate that the value is unused, as a hypothetical [[discard]] would
do. I would even argue that it is worse because (as Zdenek points out) it adds
a branch which would either need to be tested, potentially requiring additional
failure injection logic to trigger the failing case, or documented as not
needing to be covered by a test.
In general, I would consider any code structure with no behavioral effect but a
semantic side-effect (including casting to void, assigning to an unused
variable, or testing in a conditional with an empty block) a code smell, and
would prefer an explicit [[discard]] to make the intent clear. Given that we
have no [[discard]], I still hold that cast-to-void is the best existing option
due both to conciseness and to widespread recognition of its intent.
There's also an argument to be made that allowing the warning to be bypassed
with if(){} or assignment to an unused variable is weakening the original
intent behind WUR, as Jakub mentions.
(In reply to Jakub Jelinek from comment #76)
> (void) casts not quieting the warning was an intentional request when the
> warning has been added, I really don't think it is a good idea to change
> that.
This is why I initially suggested a compiler option (-Wunused-result=strict) to
select the behavior. It could of course be coded in reverse, defaulting to the
current behavior and having e.g. -Wunused-result=lax to inhibit WUR warnings.
The fundamental problem with the request behind this feature (in particular,
with the fact that the request comes from a library author) is that the end
user of the compiler is the library user, not the library author, and if the
end user considers the warnings useless, they will find one or another way
around them, however much collateral damage (in the form of missed errors) that
may cause. Given that, I think it's reasonable to offer a middle-ground option
that lets the end user reject the library author's original intent of forcing
return value usage but retain the ability to check for accidentally unused
return values.
[Bug c/66425] (void) cast doesn't suppress __attribute__((warn_unused_result))
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
--- Comment #79 from Andrew Church ---
(In reply to Segher Boessenkool from comment #78)
> If someone (the user, the author, anyone) used warn_unused_result where it is
> not appropriate, just fix *that*. The attribute is specifically for cases
> where not looking at the result value is a big (often hard to find) bug,
The issue here is that the library user _cannot_ (realistically) fix improper
usage of WUR by the library author. The intent of -Wunused-result=... is to
offer a low-resistance path with fewer side effects than just a blanket
-Wno-unused-result.
> or even a security problem.
The question of whether ignoring a return value from a function is a security
problem is rarely a static determination. Does the following function raise a
security problem?
void spawn_command(const char *cmd) {
(void) system(cmd);
}
In some cases certainly, but if cmd is just setting keyboard LEDs to indicate
progress, probably not. Only the library user knows for sure, so the library
author should not be using WUR here (though the weaker [[nodiscard]] would
arguably be appropriate).
If glibc had stuck to just using WUR on realloc(), this entire discussion would
probably never had arisen, because everyone can agree that ignoring the return
value from realloc() is an error (or a deliberate sticking-out-of-the-tongue to
show that there's exactly one case it's safe to ignore the return value from
realloc(), which is when it's called with a size of zero, and _that_ is a case
I'll happily disregard.)
