[Bug c/96468] New: Warn when an empty while loop could have been a do-while

[Bug c/96468] New: Warn when an empty while loop could have been a do-while

[josephcsible at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

            Bug ID: 96468
           Summary: Warn when an empty while loop could have been a
                    do-while
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Keywords: diagnostic
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: josephcsible at gmail dot com
  Target Milestone: ---

Consider this C code:

typedef int sig_atomic_t;
volatile sig_atomic_t signaled;
_Bool getX(int *);
void processX(int);
void f(void)
{
    {
        int x;
        if(getX(&x))
            processX(x);
    }
    while(!signaled);
    /* do some other stuff */
}

There's two possibilities for what the author meant for it to do:
1. Do getX (and maybe processX) once (in a block just to minimize the scope of
x), then busy-wait until signaled becomes true, and then do some other stuff.
This is what it actually does.
2. Keep doing getX (and maybe processX) in a loop until signaled becomes true,
and then do some other stuff. This isn't what it actually does, because the
author forgot the "do" keyword.

We currently emit no warnings for this code, even when compiled with "-Wall
-Wextra". I propose that when we see "while(condition);", we warn that a "do"
may be missing, and if it's not, that you should use "while(condition){}"
instead (except in cases where it's impossible to just be missing the "do"
keyword, like if the "while" is at the beginning of a block).

[Bug c/96468] Warn when an empty while loop could have been a do-while

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-08-05
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.

[Bug c/96468] Warn when an empty while loop could have been a do-while

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
It's not clear whether the request is just for the specific case for while
loops with a volatile control variable or independent of the qualifier.  I
think issuing a warning in either case would be worthwhile but for different
reasons, and with different messages making a distinction between the kinds of
the potential problems.  (The mention of the suggestion to use
"while(condition){}" instead makes me think it might be the former.)

Thanks to the volatile access in the example in comment #0, the loop will
terminate once signaled is set but the intent may have been that it iterate
over the body of the preceding block.  signaled can be set either in one of the
calls in the block in some signal handler, or even in another thread (in
addition to being set prior to the block of course).

But in the absence of the volatile (and atomic) qualifier, unless the initial
value of signaled is non-zero the loop will never terminate regardless of any
extraneous accesses to it outside it because all but the first access to it in
the loop is eliminated:

  int signaled;

  void f (void)
  {
    while (!signaled);
  }

[Bug c/96468] Warn when an empty while loop could have been a do-while

[josephcsible at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

--- Comment #3 from Joseph C. Sible <josephcsible at gmail dot com> ---
I didn't intend to restrict this to only volatile control variables. If you
have "_Bool canMoveOn(void);", I'd like "while(!canMoveOn());" in place of
"while(!signaled);" to warn too for the exact same reasons.

[Bug tree-optimization/96468] Warn when an empty while loop could have been a do-while

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |tree-optimization

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
I see.  In that case, I don't think such a warning can be implemented in the
front end (as suggested by the choice of the Component) because if signaled
were neither atomic nor volatile (and not a function call as in your new
example) the only way to determine whether the loop might terminate is by
analyzing the potential accesses in the body of the prior block for those to
it.  Such analysis is beyond what the C front end can handle.  For example, in

  int signaled;

  void f (double *a)
  {
    {
      for (int i = 0; i != 7; ++i)
        a[i] = 0;
    }
    while (!signaled);

including the block in the loop wouldn't make it finite but the front end can't
easily determine that.

My point is that issuing a warning suggesting the while loop might have been
intended to be a do-while would be misleading, as would be suggesting to
rewrite the loop as "while (!signaled) { }"  This is not a concern if the
condition accesses an atomic/volatile object or is a call to a
non-const/non-pure function which is readily available in the front end.  For
others, the warning would need to do quite a bit more work.  With that, I'll
change the component to tree-optimization where I believe implementing this
might be more feasible.

So just to be clear, I'm not objecting to the request, just clarifying what it
asks for and how difficult it might be implement.

[Bug tree-optimization/96468] Warn when an empty while loop could have been a do-while

[josephcsible at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

--- Comment #5 from Joseph C. Sible <josephcsible at gmail dot com> ---
I didn't have termination checking in mind at all for this. I envisioned the
warning triggering any time a "while" loop's body was just a semicolon, when
the "while" isn't the first statement in its block.

[Bug c/96468] Warn when an empty while loop could have been a do-while

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |c

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
Oh.  So you're just looking for a simple lexical check to see if a while
statement is preceded by a block { } and a warning if it is and (presumably)
isn't the result of macro expansion.  E.g., issue a warning here:

  void f (void)
  {
    { debug ("in f ()"); }
    while (!signaled);   // warn
  }

but not here (:

  void f (void)
  {
    DEBUG   // expands to { debug (...); }
    while (!signaled);   // don't warn
  }

That should be simpler and doable in the front end (so back to C for component)
but also seems considerably less useful to me as well as more prone to noise. 

Front end isn't really my area so I don't expect to work on this but a couple
of questions occur to me: should blank lines between the closing curly or the
while have any effect on the warnig?  If yes as I would expect it starts to
feel like it's in the same area as -Wmisleading-indentation.  What about other
iteration statements -- should those be considered as well?

  while (condition);   // warning? (spurious semicolon)
  { }

  for (...; condition; ...);   // ditto
  { }

[Bug c/96468] Warn when an empty while loop could have been a do-while

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96468

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #7 from Eric Gallager <egallager at gcc dot gnu.org> ---
Huh, I thought it might have been possible to get a warning from -Wempty-body
for this code with some other version of gcc or clang, but I can't seem to find
which one at the moment...