Bug 69602 - [8/9/10 Regression] over-ambitious logical-op warning on EAGAIN vs EWOULDBLOCK
Summary: [8/9/10 Regression] over-ambitious logical-op warning on EAGAIN vs EWOULDBLOCK
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 6.0
: P2 normal
Target Milestone: 9.3
Assignee: Not yet assigned to anyone
URL:
Keywords: deferred, diagnostic
: 79708 (view as bug list)
Depends on: 43486 61534
Blocks: Wall-Wextra
  Show dependency treegraph
 
Reported: 2016-02-01 17:51 UTC by Eric Blake
Modified: 2019-08-12 08:57 UTC (History)
11 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-02-01 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Blake 2016-02-01 17:51:12 UTC
POSIX says that EAGAIN and EWOULDBLOCK may be identical, but also that they may be distinct.  Therefore, well-written portable code MUST check for both values in some circumstances.

However, as shown by the sample code below, gcc 6.0's new warning is over-ambitious, and is likely to _cause_ rather than cure user bugs, when uninformed users unaware that Linux has the two errno values equal dumb down the code to silence the warning, but in the process break their code on other platforms where it is important to check for both values.

$ cat foo.c
#include <errno.h>
int main(void) {
  if (errno == EAGAIN || errno == EWOULDBLOCK)
    return 1;
  return 0;
}
$ gcc -o foo foo.c -Werror=logical-op
foo.c: In function 'main':
foo.c:3:23: error: logical 'or' of equal expressions [-Werror=logical-op]
   if (errno == EAGAIN || errno == EWOULDBLOCK)
                       ^~
cc1: some warnings being treated as errors
$ gcc --version | head -n1
gcc (GCC) 6.0.0 20160129 (Red Hat 6.0.0-0.7)
Comment 1 Marek Polacek 2016-02-01 18:01:18 UTC
Confirmed, though there are other PRs similar in nature.  Fixing this isn't trivial at all.  E.g. -Wduplicated-cond has the same problem -- that's the reason these warnings aren't enabled by -Wall/-Wextra. :(
Comment 2 Manuel López-Ibáñez 2016-02-01 21:32:10 UTC
(In reply to Marek Polacek from comment #1)
> Confirmed, though there are other PRs similar in nature.  Fixing this isn't
> trivial at all.  E.g. -Wduplicated-cond has the same problem -- that's the
> reason these warnings aren't enabled by -Wall/-Wextra. :(

It isn't trivial, but it may be possible now by seeing that the operators come from the expansion of different macros, no?
Comment 3 Manuel López-Ibáñez 2016-02-01 21:35:49 UTC
I wonder when/why it started warning, since -Wlogical-op is not new in GCC 6.

This is just a more complex case of PR61534.
Comment 4 Manuel López-Ibáñez 2016-02-01 21:42:43 UTC
Since EAGAIN and EWOULDBLOCK probably expand from a macro to a constant (or are they enums? do we track the original form of the enum or only the underlying value?), this is as hard as:

extern int xxx;
#define XXX xxx
int test (void)
{
  if (!XXX && xxx)
    return 4;
  else
    return 0;
}

thus, if this is not a regression caused by something else, it is a duplicate.
Comment 5 Jakub Jelinek 2016-02-02 10:51:23 UTC
Even if we look through macros, I'd actually think we should warn here.
Because this is actually:
#define EAGAIN 11
#define EWOULDBLOCK EAGAIN
extern int *__errno_location (void) __attribute__ ((__nothrow__, __leaf__, __const__));
#define errno (*__errno_location ())

int
foo ()
{
  if (errno == EAGAIN || errno == EWOULDBLOCK)
    return 1;
  return 0;
}
and even errno.h claims that EWOULDBLOCK is EAGAIN.
The warning on this started with r222408.
Comment 6 Manuel López-Ibáñez 2016-02-02 12:29:31 UTC
(In reply to Jakub Jelinek from comment #5)
> Even if we look through macros, I'd actually think we should warn here.

I think we should NOT look through macros. The purpose of the warning is to catch mistakes like xxx && !!xxx or 0 < A || A > 0, but if the user writes f() && g() and both functions return the same value, it is clearly not a bug, even if GCC knows that they are the same function. For the purposes of this warning (and probably several others), we should treat macros as variables and functions. Thus,

(errno == EAGAIN || errno == EWOULDBLOCK) /* no warning: EAGAIN and EWOULDBLOCK are not the same macro */

(errno == EAGAIN || EAGAIN == errno) /* warn:  logical 'or' of the same expressions */


Of course, this is not trivial to implement at the moment (mostly because we don't have locations for macros that expand to constants, thus even finding that something comes from a macro is not possible in most cases).
Comment 7 Manuel López-Ibáñez 2016-02-02 12:37:59 UTC
(In reply to Eric Blake from comment #0)
> However, as shown by the sample code below, gcc 6.0's new warning is
> over-ambitious, and is likely to _cause_ rather than cure user bugs, when
> uninformed users unaware that Linux has the two errno values equal dumb down
> the code to silence the warning, but in the process break their code on
> other platforms where it is important to check for both values.

As a work-around, something like:

if (0 || errno == EAGAIN || errno == EWOULDBLOCK)

silences the warning (although it should not).
Comment 8 Jakub Jelinek 2016-02-02 12:41:37 UTC
(In reply to Manuel López-Ibáñez from comment #7)
> (In reply to Eric Blake from comment #0)
> > However, as shown by the sample code below, gcc 6.0's new warning is
> > over-ambitious, and is likely to _cause_ rather than cure user bugs, when
> > uninformed users unaware that Linux has the two errno values equal dumb down
> > the code to silence the warning, but in the process break their code on
> > other platforms where it is important to check for both values.
> 
> As a work-around, something like:
> 
> if (0 || errno == EAGAIN || errno == EWOULDBLOCK)
> 
> silences the warning (although it should not).

That is something that should be fixed.
But
  if (errno == EAGAIN || (EWOULDBLOCK != EAGAIN && errno == EWOULDBLOCK))
could be better workaround.
Comment 9 Manuel López-Ibáñez 2016-02-07 18:39:47 UTC
If not a duplicate, then they are co-dependent.
Comment 10 Gerald Pfeifer 2016-02-07 18:56:12 UTC
(In reply to Manuel López-Ibáñez from comment #6)
> I think we should NOT look through macros. The purpose of the warning is to
> catch mistakes like xxx && !!xxx or 0 < A || A > 0, but if the user writes
> f() && g() and both functions return the same value, it is clearly not a
> bug, even if GCC knows that they are the same function.

I agree with Manuel (and had similar cases in Wine).  Sometimes APIs
may richer than an actual implementation thereof, as is the case in
this example.  Still we would want developers to write general code
that matches the API instead of manually optimizing things (and in
consequence breaking them on other platforms/in other scenarios).

(In reply to Jakub Jelinek from comment #8)
>   if (errno == EAGAIN || (EWOULDBLOCK != EAGAIN && errno == EWOULDBLOCK))
> could be better workaround.

This is WAY more complicated to read and understand than the original
approach.  I understand you only called it a workaround, but I am pretty
sure both GCC and Wine maintainers (and presumably others) would shoot
such obfuscation down if it's for the sake of avoiding a warning.
Comment 11 Manuel López-Ibáñez 2016-02-07 19:20:51 UTC
(In reply to Gerald Pfeifer from comment #10)
> (In reply to Jakub Jelinek from comment #8)
> >   if (errno == EAGAIN || (EWOULDBLOCK != EAGAIN && errno == EWOULDBLOCK))
> > could be better workaround.
> 
> This is WAY more complicated to read and understand than the original
> approach.  I understand you only called it a workaround, but I am pretty
> sure both GCC and Wine maintainers (and presumably others) would shoot
> such obfuscation down if it's for the sake of avoiding a warning.

Unfortunately, we are very far away from implementing anything better [*], short of completely disabling the warning, which can be done by users with #pragma GCC diagnostic for specific cases or simply by not using -Wlogical-op. As Marek said, many warnings suffer from this inability to see macros before expansion, and this is why they are not enabled by -Wall (or even by -Wextra).

[*] Fixing this will not only require making the folding procedures recognize macros and handle them specially for the sake of some warnings which want to look at code as written by the user, not as represented internally by gcc; it will also require adding virtual locations to a lot of source level entities that don't have them (PR43486). I would not expect to see the latter in the near future; it has proven to be a very hard problem to crack.
Comment 12 Sérgio Basto 2016-03-10 23:56:33 UTC
if EAGAIN and EWOULDBLOCK may be identical, but also that they may be distinct, this warning doesn't make sense, IMHO, look at this virtualbox solution :

https://www.virtualbox.org/changeset/59960/vbox/trunk 

it is worst than before, just to avoid one warning.
Comment 13 Marek Polacek 2016-03-16 15:10:00 UTC
Changing target milestone; it's not viable to fix this in stage4.
Comment 14 Martin Sebor 2017-01-24 21:30:04 UTC
I would suggest to keep the warning simple and avoid overdesigning it with workarounds for this case.  The solution in comment #12 (copied below) seems like a good approach for avoiding the duplication and the warning.  It's already commonly used to deal with non-standard implementation-specific macros that portable programs cannot otherwise rely on.

 	1149	    if (   cchRead < 0 
 	1150	        && (   errno == EAGAIN 
 	1151	#if EAGAIN != EWOULDBLOCK 
 	1152	            || errno == EWOULDBLOCK 
 	1153	#endif 
 	1154	            ))
Comment 15 Eric Blake 2017-01-24 21:36:59 UTC
(In reply to Martin Sebor from comment #14)
> I would suggest to keep the warning simple and avoid overdesigning it with
> workarounds for this case.  The solution in comment #12 (copied below) seems
> like a good approach for avoiding the duplication and the warning.  It's
> already commonly used to deal with non-standard implementation-specific
> macros that portable programs cannot otherwise rely on.
> 
>  	1149	    if (   cchRead < 0 
>  	1150	        && (   errno == EAGAIN 
>  	1151	#if EAGAIN != EWOULDBLOCK 
>  	1152	            || errno == EWOULDBLOCK 
>  	1153	#endif 
>  	1154	            ))

Yuck. Mid-expression #ifdefs are awful - I much prefer it when #ifdefs can be limited to a per-statement or per-function scope.  You're seriously proposing forcing software to uglify their code with mid-expression warnings to shut up compiler warnings that only occur on platforms where the two values are synonyms, and which can't even be portably seen as necessary when testing as platforms where the two values are distinct?
Comment 16 Martin Sebor 2017-01-24 22:36:12 UTC
There are straightforward ways to avoid interspersing code with preprocessor conditionals:

  #if EAGAIN == EWOULDBLOCK 
  #  #define EAGAIN_OR_WOULDBLOCK(e) (e == EAGAIN)
  #else
  #  #define EAGAIN_OR_WOULDBLOCK(e) (e == EAGAIN || e == EWOULDBLOCK)
  #endif

  ...

  if (cchRead < 0 && EAGAIN_OR_WOULDBLOCK (errno))

The point is that the warning is simple and works as designed.  As others have noted, handling this case is non-trivial and as with any non-trivial solution could easily have unforeseen side-effects and even lead to bugs (at a minimum, likely false negatives).  I ended up here because I've been researching just such a problem.  So unless the idiom in comment #0 is pervasive and leads to many warnings I would suggest to leave it to programs to deal with.  Marek and Jakub will likely have an idea of how common it is once they rebuild Fedora with GCC 7.  If you have data of your own that might be helpful as well.
Comment 17 Manuel López-Ibáñez 2017-01-26 23:51:48 UTC
(In reply to Martin Sebor from comment #16)
> There are straightforward ways to avoid interspersing code with preprocessor
> conditionals:

Will GCC adopt this approach so that the warning can be enabled with -Wextra? Because the reason it was moved out of -Wextra was due to warning for this type of code in GCC itself.

> Marek and Jakub will likely have an idea of how common it is
> once they rebuild Fedora with GCC 7.  If you have data of your own that
> might be helpful as well.

Most people use just "-Wall -Wextra". This warning needs to be enabled explicitly, so I doubt many programs use it.  The reason is precisely these false positives. Well, at least PR61534.

A similar case is:

static const int EWOULDBLOCK = 1;
static const int EAGAIN = 1;
int errno;
int foo(void) {
  if (errno == EAGAIN || errno == EWOULDBLOCK)
    return 1;
  return 0;
}

for which we also warn but I honestly think we should not.
Comment 18 Martin Sebor 2017-01-27 00:56:23 UTC
I don't know the full history behind this particular warning but I think a good way to find out if it can be adopted by GCC is to see the extent of the changes required to clean up the code base so it compiles with it on (I see just 2 instances of it in gcc/config/i386/i386.md in my x86_64 build).   If the cleanup isn't excessive, then post a patch with it and add it to -Wextra at the same time and see what people think.  Given the background it might not be a bad idea to also build a few other packages.  If it turns out that the effort to clean it up is extensive due to this idiom then it might appropriate to consider tweaking the implementation of the warning and suppressing it on this case.  But I would think we'd want to expose a decent amount of everyday code to the warning and see more than a couple of instances of it before making the call.

My main concern is that the suppression logic not end up trading off a small number of false positives for a much larger class of false negatives.  I have no data to back it up but it seems to me that disabling the warning when the operands are macros could have just such an effect.  Unlike with false positives, false negatives are hard to find.  (As much as I dislike one-off solutions perhaps recognizing just the EXXX == EYYY errno case could be a compromise.)
Comment 19 Marek Polacek 2017-02-24 18:52:47 UTC
*** Bug 79708 has been marked as a duplicate of this bug. ***
Comment 20 Jakub Jelinek 2017-05-02 15:57:43 UTC
GCC 7.1 has been released.
Comment 21 Richard Biener 2018-01-25 08:27:49 UTC
GCC 7.3 is being released, adjusting target milestone.
Comment 22 Jeffrey A. Law 2018-02-19 21:14:14 UTC
With no real design/implementation in sight, this is being taken off the table for gcc-8.
Comment 23 Manuel López-Ibáñez 2018-10-19 20:41:30 UTC
This prevents enabling -Wlogical-op with -Wextra.
Comment 24 Jakub Jelinek 2019-05-03 09:18:07 UTC
GCC 9.1 has been released.
Comment 25 Jakub Jelinek 2019-08-12 08:57:42 UTC
GCC 9.2 has been released.