This patch is accompanied by more than 2000 lines of new tests to get the
warning covered though I'm sure folks will come up with other test cases
that I hadn't considered (hi Martin S. ;).
This warning is enabled by default for C/C++. I was more inclined to put this
into -Wall, but our common.opt machinery doesn't seem to allow that (ugh!).
The clang version I have installed doesn't have this warning so I couldn't
compare my implementation with others.
I plan to plunge into the C++ [[fallthrough]] thingie after this core part is
dealt with.
I think this is a useful feature though like others I'm not
entirely sure that a built-in is the right interface. I think
I would find a pragma or an attribute preferable. At a minimum,
it would obviate some questions raised by my testing (i.e.,
whether the built-in be accepted when an attribute would
otherwise not be syntactically allowed).
I applied the core patch and ran a few experiments with it on
the assumption that __builtin_fallthrough() should behave roughly
correspondingly to [[fallthrough]]. I.e., be rejected where
[[fallthrough]] is rejected (but where attributes are otherwise
valid) and be accepted where the latter is accepted. This may
not be intended but the text added to the manual is too vague
to tell. I also compared the results to Clang's [[fallthrough]]
to make sure I wasn't missing something (or too much).
I ran into a few of what seems like subtle bugs or limitations
some of which I'm not sure are going to be solvable in the middle
end (at least not after the control flow graph has been built)
because by the time the middle end sees the code (certainly by
the time it gets to expansion) some of it has been eliminated.
An illustrative example of this class of problems might be this
function:
void f (void)
{
if (0) __builtin_fallthrough (); // never diagnosed
int i = 0;
if (i) __builtin_fallthrough (); // not diagnosed with -O
}
With the built-in replaced by [[fallthrough]] Clang diagnoses
both of these.
This may be an okay limitation for __builtin_fallthrough since
it's GCC-specific, but it won't do for the C++ attribute or for
the C attribute if one is added with matching constraints.
The tests I tried are in the attached file. Most of them are
edge cases but some I think point out more serious problems
(the checker getting confused/disabled by a prior switch
statement).
Hopefully this will be useful.
Martin
#ifdef __clang__
# define FALLTHROUGH [[fallthrough]]
# define __builtin_fallthrough() (void)0
#else
# define FALLTHROUGH __builtin_fallthrough ()
#endif
void f0 (void)
{
// This would be an invalid [[fallthrough]] and GCC rejects it
// as expected with:
// error: invalid use of '__builtin_fallthrough ()'
FALLTHROUGH;
{
// This would be invalid [[fallthrough]] but is silently accepted
// by GCC.
0 ? __builtin_fallthrough () : (void)0;
}
{
switch (0)
// This is invalid with [[fallthrough]] because there's no
// preceding statement within the switch statement, and no
// subsequent one. Clang rejects it but GCC silently accepts
// it (it issues -Wswitch-unreachable).
FALLTHROUGH;
}
{
// This is invalid but GCC gives:
// warning: not preceding a label
// It seems that it should give an error like in the first case
// above. I suspect it's confused by the preceding switch.
// Clang gives an error.
FALLTHROUGH;
}
}
void f1 (void)
{
{
switch (0)
// This also seems invalid because there is no preceding
// statement and no subsequent case-labeled statement but
// GCC silently accepts it (as does Clang). It seems that
// GCC gets tricked by the A label.
default: FALLTHROUGH; A: ;
}
{
// Unlike the same block at the end of f0 this is an error
// as it should be`.
FALLTHROUGH;
}
}
void f2 (int i)
{
{
switch (i)
{
case 0:
// Both of the following are invalid with [[fallthrough]]
// because neither is followed by a case-labeled statement.
// GCC only only rejects the second of the two. Clang
// accepts both.
FALLTHROUGH;
FALLTHROUGH;
}
}
}
// The following function triggers an ICE.
void f3 (void)
{
{
// Invalid but without the ICE silently accepted by GCC.
int i = 0;
if (i) __builtin_fallthrough ();
}
{
switch (0)
__builtin_fallthrough ();
}
__builtin_fallthrough ();
}
