[llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
Alex Kornienko proposed enabling this warning back in 2012 here:

At the time, Chris Lattner said he didn't feel it was worth annotating
all of LLVM and Clang with a new macro to enable this warning.

However, GCC 7 added this warning as part of -Wextra, and we've slowly
annotated most of LLVM and Clang with LLVM_FALLTHROUGH.

At this point, I think we should re-evaluate this decision and enable
this warning for both clang and GCC.

Since our codebase is already annotated, it will help developers (like
me), who use Clang locally, to find unintended fallthrough bugs in their
code. For example, I committed r345676, which had to be reverted because
of an unintended fallthrough. This warning would've helped.

There is also marginal benefit to aligning warnings between GCC and
Clang. While there will always be divergence in warnings between GCC and
Clang, when possible, it saves time when clang can diagnose things that
would later become a -Werror warning on some GCC 7 buildbot.

This is a summary of differences in the behavior of
-Wimplicit-fallthrough between clang and GCC:
1. GCC recognizes comments that say "fall through" as annotations, clang
   doesn't
2. GCC doesn't warn on "case N: foo(); default: break;", clang does
3. GCC doesn't warn when the case contains a switch, but falls through
   the outer case. See the AArch64ISelLowering.cpp change for an
   instance where this almost caused a bug, but a redundant check saved
   us. I've removed the redundant check.
4. Clang warns on LLVM_FALLTHROUGH after llvm_unreachable. GCC doesn't,
   so I removed the one instance of this that I found.

Changing Clang's behavior in light of these differences is out of scope
for me. I want developers who compile with any of the last 4 years of
clang releases to be able to use this warning, and those releases have
the behavior described above. If you want to discuss changing Clang to
be more like GCC here, please file a bug or start a thread on cfe-dev.

I posted a patch with the this RFC as the commit message here so you can see what this looks like now:

To summarize, this warning is already enabled for GCC and
we've already annotated most of our codebase for it, so let's enable the
warning for clang.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
+1. I tried to do this a couple of years ago (not knowing about the
proposal in 2012) but there was too much to annotate at the time. It
seems like this is easy to do now.

Reid Kleckner via llvm-dev <[hidden email]> writes:

> Alex Kornienko proposed enabling this warning back in 2012 here:
> http://lists.llvm.org/pipermail/llvm-dev/2012-July/051386.html
>
> At the time, Chris Lattner said he didn't feel it was worth annotating
> all of LLVM and Clang with a new macro to enable this warning.
>
> However, GCC 7 added this warning as part of -Wextra, and we've slowly
> annotated most of LLVM and Clang with LLVM_FALLTHROUGH.
>
> At this point, I think we should re-evaluate this decision and enable
> this warning for both clang and GCC.
>
> Since our codebase is already annotated, it will help developers (like
> me), who use Clang locally, to find unintended fallthrough bugs in their
> code. For example, I committed r345676, which had to be reverted because
> of an unintended fallthrough. This warning would've helped.
>
> There is also marginal benefit to aligning warnings between GCC and
> Clang. While there will always be divergence in warnings between GCC and
> Clang, when possible, it saves time when clang can diagnose things that
> would later become a -Werror warning on some GCC 7 buildbot.
>
> This is a summary of differences in the behavior of
> -Wimplicit-fallthrough between clang and GCC:
> 1. GCC recognizes comments that say "fall through" as annotations, clang
>    doesn't
> 2. GCC doesn't warn on "case N: foo(); default: break;", clang does
> 3. GCC doesn't warn when the case contains a switch, but falls through
>    the outer case. See the AArch64ISelLowering.cpp change for an
>    instance where this almost caused a bug, but a redundant check saved
>    us. I've removed the redundant check.
> 4. Clang warns on LLVM_FALLTHROUGH after llvm_unreachable. GCC doesn't,
>    so I removed the one instance of this that I found.
>
> Changing Clang's behavior in light of these differences is out of scope
> for me. I want developers who compile with any of the last 4 years of
> clang releases to be able to use this warning, and those releases have
> the behavior described above. If you want to discuss changing Clang to
> be more like GCC here, please file a bug or start a thread on cfe-dev.
>
> I posted a patch with the this RFC as the commit message here so you can
> see what this looks like now:
> https://reviews.llvm.org/D53950
>
> To summarize, this warning is already enabled for GCC and
> we've already annotated most of our codebase for it, so let's enable the
> warning for clang.
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
+1! Accidental fallthrough is a common bug class in my experience and this a great tool to avoid them.

In reality you already get mail from some buildbots anyway when you forget LLVM_FALLTHROUGH AFAIK so this just codifies existing practice.

> On Oct 31, 2018, at 3:25 PM, Justin Bogner via llvm-dev <[hidden email]> wrote:
>
> +1. I tried to do this a couple of years ago (not knowing about the
> proposal in 2012) but there was too much to annotate at the time. It
> seems like this is easy to do now.
>
> Reid Kleckner via llvm-dev <[hidden email]> writes:
>> Alex Kornienko proposed enabling this warning back in 2012 here:
>> http://lists.llvm.org/pipermail/llvm-dev/2012-July/051386.html
>>
>> At the time, Chris Lattner said he didn't feel it was worth annotating
>> all of LLVM and Clang with a new macro to enable this warning.
>>
>> However, GCC 7 added this warning as part of -Wextra, and we've slowly
>> annotated most of LLVM and Clang with LLVM_FALLTHROUGH.
>>
>> At this point, I think we should re-evaluate this decision and enable
>> this warning for both clang and GCC.
>>
>> Since our codebase is already annotated, it will help developers (like
>> me), who use Clang locally, to find unintended fallthrough bugs in their
>> code. For example, I committed r345676, which had to be reverted because
>> of an unintended fallthrough. This warning would've helped.
>>
>> There is also marginal benefit to aligning warnings between GCC and
>> Clang. While there will always be divergence in warnings between GCC and
>> Clang, when possible, it saves time when clang can diagnose things that
>> would later become a -Werror warning on some GCC 7 buildbot.
>>
>> This is a summary of differences in the behavior of
>> -Wimplicit-fallthrough between clang and GCC:
>> 1. GCC recognizes comments that say "fall through" as annotations, clang
>>   doesn't
>> 2. GCC doesn't warn on "case N: foo(); default: break;", clang does
>> 3. GCC doesn't warn when the case contains a switch, but falls through
>>   the outer case. See the AArch64ISelLowering.cpp change for an
>>   instance where this almost caused a bug, but a redundant check saved
>>   us. I've removed the redundant check.
>> 4. Clang warns on LLVM_FALLTHROUGH after llvm_unreachable. GCC doesn't,
>>   so I removed the one instance of this that I found.
>>
>> Changing Clang's behavior in light of these differences is out of scope
>> for me. I want developers who compile with any of the last 4 years of
>> clang releases to be able to use this warning, and those releases have
>> the behavior described above. If you want to discuss changing Clang to
>> be more like GCC here, please file a bug or start a thread on cfe-dev.
>>
>> I posted a patch with the this RFC as the commit message here so you can
>> see what this looks like now:
>> https://reviews.llvm.org/D53950
>>
>> To summarize, this warning is already enabled for GCC and
>> we've already annotated most of our codebase for it, so let's enable the
>> warning for clang.
>> _______________________________________________
>> LLVM Developers mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
In reply to this post by David Jones via llvm-dev
+1 obviously ;) Thanks for picking this up!

On Wed, Oct 31, 2018 at 10:24 PM Reid Kleckner <[hidden email]> wrote:
Alex Kornienko proposed enabling this warning back in 2012 here:

At the time, Chris Lattner said he didn't feel it was worth annotating
all of LLVM and Clang with a new macro to enable this warning.

However, GCC 7 added this warning as part of -Wextra, and we've slowly
annotated most of LLVM and Clang with LLVM_FALLTHROUGH.

At this point, I think we should re-evaluate this decision and enable
this warning for both clang and GCC.

Since our codebase is already annotated, it will help developers (like
me), who use Clang locally, to find unintended fallthrough bugs in their
code. For example, I committed r345676, which had to be reverted because
of an unintended fallthrough. This warning would've helped.

There is also marginal benefit to aligning warnings between GCC and
Clang. While there will always be divergence in warnings between GCC and
Clang, when possible, it saves time when clang can diagnose things that
would later become a -Werror warning on some GCC 7 buildbot.

This is a summary of differences in the behavior of
-Wimplicit-fallthrough between clang and GCC:
1. GCC recognizes comments that say "fall through" as annotations, clang
   doesn't
2. GCC doesn't warn on "case N: foo(); default: break;", clang does
3. GCC doesn't warn when the case contains a switch, but falls through
   the outer case. See the AArch64ISelLowering.cpp change for an
   instance where this almost caused a bug, but a redundant check saved
   us. I've removed the redundant check.
4. Clang warns on LLVM_FALLTHROUGH after llvm_unreachable. GCC doesn't,
   so I removed the one instance of this that I found.

Changing Clang's behavior in light of these differences is out of scope
for me. I want developers who compile with any of the last 4 years of
clang releases to be able to use this warning, and those releases have
the behavior described above. If you want to discuss changing Clang to
be more like GCC here, please file a bug or start a thread on cfe-dev.

I posted a patch with the this RFC as the commit message here so you can see what this looks like now:

To summarize, this warning is already enabled for GCC and
we've already annotated most of our codebase for it, so let's enable the
warning for clang.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
In reply to this post by David Jones via llvm-dev
On Wed, Oct 31, 2018 at 5:24 PM Reid Kleckner via llvm-dev
<[hidden email]> wrote:

>
> Alex Kornienko proposed enabling this warning back in 2012 here:
> http://lists.llvm.org/pipermail/llvm-dev/2012-July/051386.html
>
> At the time, Chris Lattner said he didn't feel it was worth annotating
> all of LLVM and Clang with a new macro to enable this warning.
>
> However, GCC 7 added this warning as part of -Wextra, and we've slowly
> annotated most of LLVM and Clang with LLVM_FALLTHROUGH.
>
> At this point, I think we should re-evaluate this decision and enable
> this warning for both clang and GCC.
>
> Since our codebase is already annotated, it will help developers (like
> me), who use Clang locally, to find unintended fallthrough bugs in their
> code. For example, I committed r345676, which had to be reverted because
> of an unintended fallthrough. This warning would've helped.
>
> There is also marginal benefit to aligning warnings between GCC and
> Clang. While there will always be divergence in warnings between GCC and
> Clang, when possible, it saves time when clang can diagnose things that
> would later become a -Werror warning on some GCC 7 buildbot.
>
> This is a summary of differences in the behavior of
> -Wimplicit-fallthrough between clang and GCC:
> 1. GCC recognizes comments that say "fall through" as annotations, clang
>    doesn't
> 2. GCC doesn't warn on "case N: foo(); default: break;", clang does
> 3. GCC doesn't warn when the case contains a switch, but falls through
>    the outer case. See the AArch64ISelLowering.cpp change for an
>    instance where this almost caused a bug, but a redundant check saved
>    us. I've removed the redundant check.
> 4. Clang warns on LLVM_FALLTHROUGH after llvm_unreachable. GCC doesn't,
>    so I removed the one instance of this that I found.
>
> Changing Clang's behavior in light of these differences is out of scope
> for me. I want developers who compile with any of the last 4 years of
> clang releases to be able to use this warning, and those releases have
> the behavior described above. If you want to discuss changing Clang to
> be more like GCC here, please file a bug or start a thread on cfe-dev.
>
> I posted a patch with the this RFC as the commit message here so you can see what this looks like now:
> https://reviews.llvm.org/D53950
>
> To summarize, this warning is already enabled for GCC and
> we've already annotated most of our codebase for it, so let's enable the
> warning for clang.

+1, I think this is worth doing.

~Aaron

> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
In reply to this post by David Jones via llvm-dev
On Oct 31, 2018, at 2:24 PM, Reid Kleckner <[hidden email]> wrote:
Alex Kornienko proposed enabling this warning back in 2012 here:

At the time, Chris Lattner said he didn't feel it was worth annotating
all of LLVM and Clang with a new macro to enable this warning.

FWIW, I formally withdraw my objection, go for it! :-)

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
Great! Thanks everyone for the input, I'm going to start splitting up the patch. I'll send out the non-mechanical parts separately.

On Wed, Oct 31, 2018 at 10:03 PM Chris Lattner <[hidden email]> wrote:
On Oct 31, 2018, at 2:24 PM, Reid Kleckner <[hidden email]> wrote:
Alex Kornienko proposed enabling this warning back in 2012 here:

At the time, Chris Lattner said he didn't feel it was worth annotating
all of LLVM and Clang with a new macro to enable this warning.

FWIW, I formally withdraw my objection, go for it! :-)

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
I split all the possible behavior changes out of the big cleanup patch, and committed the cleanup as r345882.

I enabled the warning in r345887, and I'll see if it sticks.

Thanks for the reviews!

On Thu, Nov 1, 2018 at 10:25 AM Reid Kleckner <[hidden email]> wrote:
Great! Thanks everyone for the input, I'm going to start splitting up the patch. I'll send out the non-mechanical parts separately.

On Wed, Oct 31, 2018 at 10:03 PM Chris Lattner <[hidden email]> wrote:
On Oct 31, 2018, at 2:24 PM, Reid Kleckner <[hidden email]> wrote:
Alex Kornienko proposed enabling this warning back in 2012 here:

At the time, Chris Lattner said he didn't feel it was worth annotating
all of LLVM and Clang with a new macro to enable this warning.

FWIW, I formally withdraw my objection, go for it! :-)

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
Hi,

Does this mean that the clang version mentioned on

 
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

should be updated since it currently says that 3.1 is ok?

I noticed that one of our build-bots that ran clang 3.5.0 failed since
it ended up in the

  #define LLVM_FALLTHROUGH

part of the ifdefs in include/llvm/Demangle/Compiler.h

#if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
#define LLVM_FALLTHROUGH [[fallthrough]]
#elif __has_cpp_attribute(gnu::fallthrough)
#define LLVM_FALLTHROUGH [[gnu::fallthrough]]
#elif !__cplusplus
// Workaround for llvm.org/PR23435, since clang 3.6 and below emit a
spurious
// error when __has_cpp_attribute is given a scoped attribute in C mode.
#define LLVM_FALLTHROUGH
#elif __has_cpp_attribute(clang::fallthrough)
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#else
#define LLVM_FALLTHROUGH
#endif

and then we of course got tons of fallthrough warnings/errors (with
-Werror).

Now we've updated the clang version on that build bot too, so it's no
problem for us, but perhaps someone else will hit the same thing.

/Mikael

On 11/1/18 9:34 PM, Reid Kleckner via llvm-dev wrote:

> I split all the possible behavior changes out of the big cleanup patch,
> and committed the cleanup as r345882.
>
> I enabled the warning in r345887, and I'll see if it sticks.
>
> Thanks for the reviews!
>
> On Thu, Nov 1, 2018 at 10:25 AM Reid Kleckner <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Great! Thanks everyone for the input, I'm going to start splitting
>     up the patch. I'll send out the non-mechanical parts separately.
>
>     On Wed, Oct 31, 2018 at 10:03 PM Chris Lattner <[hidden email]
>     <mailto:[hidden email]>> wrote:
>
>         On Oct 31, 2018, at 2:24 PM, Reid Kleckner <[hidden email]
>         <mailto:[hidden email]>> wrote:
>>         Alex Kornienko proposed enabling this warning back in 2012 here:
>>         http://lists.llvm.org/pipermail/llvm-dev/2012-July/051386.html
>>
>>         At the time, Chris Lattner said he didn't feel it was worth
>>         annotating
>>         all of LLVM and Clang with a new macro to enable this warning.
>
>         FWIW, I formally withdraw my objection, go for it! :-)
>
>         -Chris
>
>
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC Enable -Wimplicit-fallthrough for clang as well as GCC

David Jones via llvm-dev
On Wed, Nov 7, 2018 at 5:26 AM Mikael Holmén <[hidden email]> wrote:
Does this mean that the clang version mentioned on
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

should be updated since it currently says that 3.1 is ok?

I noticed that one of our build-bots that ran clang 3.5.0 failed since
it ended up in the

  #define LLVM_FALLTHROUGH

part of the ifdefs in include/llvm/Demangle/Compiler.h

No, I don't think that's necessary, it just means we aren't warning clean with those versions. I don't think "supported" has to imply that LLVM and clang build without warnings, since the user can disable warnings or disable -Werror. I recently built with GCC 7 when testing my changes to libc++, and there were a handful of warnings in clang. That doesn't mean we don't support GCC 7.

It's also very likely that clang 3.5 supports a different spelling of the attribute, like [[clang::fallthrough]], and it would be willing to take a patch to make it warning clean again.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev