PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

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

PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
Hi llvmdev, llvm-commits,

There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.

I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases. 

INTRODUCTION

The switch construct of C/C++ languages allows fall-throughs between switch labels when control flow is not directed elsewhere by a breakreturn or continue statement or other ways. There are certain coding idioms which utilize this behavior, e.g. Duff's device, but much more frequently the use of switch statements doesn't involve a fall-through. (A commonly used case - when several switch labels mark one statement - is not considered a fall-through here.) As there's no special way to mark a fall-through, it's relatively easy to make a mistake by just missing a break/return statement before the next switch label. This kind of mistake is sometimes difficult to spot by manual code examination, and the compiler can't provide any help either. It's common to mark fall-through locations with a comment, but parsing comments would incur a significant performance penalty and is probably an unhealthy approach overall. So the use of comment-only annotations is limited to manual inspections.

Some time ago I've added the 'Unintended fall-through in switch statement' diagnostics to clang (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html).

This is a proposal to introduce clang's unintended fall-through diagnostic to llvm/clang code.

DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS' DIAGNOSTIC IN CLANG

Related functionality in clang introduces several diagnostic messages and a syntax for intended fall-through annotation, based on C++11 attributes ([dcl.attr], http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf, 7.6).

The [[clang::fallthrough]] attribute should be applied to a null statement placed in a point of execution where fall-through to the next switch label occurs, e.g.:

switch (n) {
  case 0:
    ...  // no break here
  case 1:  // warning: unannotated fall-through between switch labels
    if (x)
      break;
    else if (y) {
      ...
      [[clang::fallthrough]];  // annotated fall-through to case 2
    }
    else
      return 13;
  case 2:  // no warning here
    ...
    [[clang::fallthrough]];  // annotated fall-through to case 3
  case 3:
    ...
}

So, the [[clang::fallthrough]]; annotation can be used in mostly in the same way as break; or return; statements (but it doesn't change control-flow, it just annotates a fall-through). The following rules are checked for this annotation:
  * the clang::fallthrough attribute can only be attached to a null-statement;
  * the [[clang::fallthrough]]; annotation should be placed inside a switch body;
  * it should be placed on an execution path between any statement inside a switch body and a case/default label (this means there is a fall-through to the corresponding case/default label);
  * no statements should exist on an execution path between the [[clang::fallthrough]]; annotation and the next case/default label.

PROPOSAL FOR LLVM/CLANG CODE

I ran the unintended fall-through diagnostics against llvm/clang code and found a number of fall-through locations, most of them are probably intended. But some look like bugs (see the attached fallthrough-bugs-llvm.diff file).

To start using the proposed diagnostics code has to be prepared.

First, we need to mark all intended fall-through locations with the clang::fallthrough attribute. It makes sense to wrap [[clang::fallthrough]] to a macro, so the code would continue to work in c++98 mode, but in c++11 mode it would also allow to run this diagnostic (btw, is llvm compilable in c++11 mode?). Sample implementation of the macro (see fallthrough-macro.diff):
#ifdef __clang__
#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#endif
#endif
#ifndef LLVM_FALLTHROUGH
#define LLVM_FALLTHROUGH do { } while (0)
#endif


After this we can start using -Wimplicit-fallthrough.

Please express your opinions on this proposal.

I'm also ready to provide diffs for marking all fall-through locations with the LLVM_FALLTHROUGH macro, if this proposal gets approval.

--
Best regards,
Alexander Kornienko

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

fallthrough-bugs-llvm.diff (3K) Download Attachment
fallthrough-macro.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
Is anyone interested in introducing fall-through annotations and the related diagnostic in LLVM/Clang code?


On Mon, Jul 2, 2012 at 6:59 PM, Alexander Kornienko <[hidden email]> wrote:
Hi llvmdev, llvm-commits,

There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.

I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases. 

INTRODUCTION

The switch construct of C/C++ languages allows fall-throughs between switch labels when control flow is not directed elsewhere by a breakreturn or continue statement or other ways. There are certain coding idioms which utilize this behavior, e.g. Duff's device, but much more frequently the use of switch statements doesn't involve a fall-through. (A commonly used case - when several switch labels mark one statement - is not considered a fall-through here.) As there's no special way to mark a fall-through, it's relatively easy to make a mistake by just missing a break/return statement before the next switch label. This kind of mistake is sometimes difficult to spot by manual code examination, and the compiler can't provide any help either. It's common to mark fall-through locations with a comment, but parsing comments would incur a significant performance penalty and is probably an unhealthy approach overall. So the use of comment-only annotations is limited to manual inspections.

Some time ago I've added the 'Unintended fall-through in switch statement' diagnostics to clang (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html).

This is a proposal to introduce clang's unintended fall-through diagnostic to llvm/clang code.

DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS' DIAGNOSTIC IN CLANG

Related functionality in clang introduces several diagnostic messages and a syntax for intended fall-through annotation, based on C++11 attributes ([dcl.attr], http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf, 7.6).

The [[clang::fallthrough]] attribute should be applied to a null statement placed in a point of execution where fall-through to the next switch label occurs, e.g.:

switch (n) {
  case 0:
    ...  // no break here
  case 1:  // warning: unannotated fall-through between switch labels
    if (x)
      break;
    else if (y) {
      ...
      [[clang::fallthrough]];  // annotated fall-through to case 2
    }
    else
      return 13;
  case 2:  // no warning here
    ...
    [[clang::fallthrough]];  // annotated fall-through to case 3
  case 3:
    ...
}

So, the [[clang::fallthrough]]; annotation can be used in mostly in the same way as break; or return; statements (but it doesn't change control-flow, it just annotates a fall-through). The following rules are checked for this annotation:
  * the clang::fallthrough attribute can only be attached to a null-statement;
  * the [[clang::fallthrough]]; annotation should be placed inside a switch body;
  * it should be placed on an execution path between any statement inside a switch body and a case/default label (this means there is a fall-through to the corresponding case/default label);
  * no statements should exist on an execution path between the [[clang::fallthrough]]; annotation and the next case/default label.

PROPOSAL FOR LLVM/CLANG CODE

I ran the unintended fall-through diagnostics against llvm/clang code and found a number of fall-through locations, most of them are probably intended. But some look like bugs (see the attached fallthrough-bugs-llvm.diff file).

To start using the proposed diagnostics code has to be prepared.

First, we need to mark all intended fall-through locations with the clang::fallthrough attribute. It makes sense to wrap [[clang::fallthrough]] to a macro, so the code would continue to work in c++98 mode, but in c++11 mode it would also allow to run this diagnostic (btw, is llvm compilable in c++11 mode?). Sample implementation of the macro (see fallthrough-macro.diff):
#ifdef __clang__
#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#endif
#endif
#ifndef LLVM_FALLTHROUGH
#define LLVM_FALLTHROUGH do { } while (0)
#endif


After this we can start using -Wimplicit-fallthrough.

Please express your opinions on this proposal.

I'm also ready to provide diffs for marking all fall-through locations with the LLVM_FALLTHROUGH macro, if this proposal gets approval.

--
Best regards,
Alexander Kornienko



--
Alexander Kornienko | Software Engineer | [hidden email] | +49 151 221 77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
On Thu, Jul 26, 2012 at 8:53 PM, Cameron McInally <[hidden email]> wrote:
Hey Alex,

Sorry if this is a silly question... are you asking if anyone "wants the functionality proposed" or "wants to write the code for the functionality proposed"?
-Wimplicit-fallthrough diagnostic is already implemented, and the patch in this thread is a portability macro for [[clang::fallthrough]] attribute (which is also implemented, of course). I only need an approval from community to add this macro and replace comment-only fall-through annotations with the LLVM_FALLTHROUGH; macro.
 
-Cameron

On Thu, Jul 26, 2012 at 2:17 PM, Alexander Kornienko <[hidden email]> wrote:
Is anyone interested in introducing fall-through annotations and the related diagnostic in LLVM/Clang code?


On Mon, Jul 2, 2012 at 6:59 PM, Alexander Kornienko <[hidden email]> wrote:
Hi llvmdev, llvm-commits,

There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.

I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases. 

INTRODUCTION

The switch construct of C/C++ languages allows fall-throughs between switch labels when control flow is not directed elsewhere by a breakreturn or continue statement or other ways. There are certain coding idioms which utilize this behavior, e.g. Duff's device, but much more frequently the use of switch statements doesn't involve a fall-through. (A commonly used case - when several switch labels mark one statement - is not considered a fall-through here.) As there's no special way to mark a fall-through, it's relatively easy to make a mistake by just missing a break/return statement before the next switch label. This kind of mistake is sometimes difficult to spot by manual code examination, and the compiler can't provide any help either. It's common to mark fall-through locations with a comment, but parsing comments would incur a significant performance penalty and is probably an unhealthy approach overall. So the use of comment-only annotations is limited to manual inspections.

Some time ago I've added the 'Unintended fall-through in switch statement' diagnostics to clang (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057180.html).

This is a proposal to introduce clang's unintended fall-through diagnostic to llvm/clang code.

DESCRIPTION OF 'UNINTENDED FALL-THROUGH IN SWITCH STATEMENTS' DIAGNOSTIC IN CLANG

Related functionality in clang introduces several diagnostic messages and a syntax for intended fall-through annotation, based on C++11 attributes ([dcl.attr], http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf, 7.6).

The [[clang::fallthrough]] attribute should be applied to a null statement placed in a point of execution where fall-through to the next switch label occurs, e.g.:

switch (n) {
  case 0:
    ...  // no break here
  case 1:  // warning: unannotated fall-through between switch labels
    if (x)
      break;
    else if (y) {
      ...
      [[clang::fallthrough]];  // annotated fall-through to case 2
    }
    else
      return 13;
  case 2:  // no warning here
    ...
    [[clang::fallthrough]];  // annotated fall-through to case 3
  case 3:
    ...
}

So, the [[clang::fallthrough]]; annotation can be used in mostly in the same way as break; or return; statements (but it doesn't change control-flow, it just annotates a fall-through). The following rules are checked for this annotation:
  * the clang::fallthrough attribute can only be attached to a null-statement;
  * the [[clang::fallthrough]]; annotation should be placed inside a switch body;
  * it should be placed on an execution path between any statement inside a switch body and a case/default label (this means there is a fall-through to the corresponding case/default label);
  * no statements should exist on an execution path between the [[clang::fallthrough]]; annotation and the next case/default label.

PROPOSAL FOR LLVM/CLANG CODE

I ran the unintended fall-through diagnostics against llvm/clang code and found a number of fall-through locations, most of them are probably intended. But some look like bugs (see the attached fallthrough-bugs-llvm.diff file).

To start using the proposed diagnostics code has to be prepared.

First, we need to mark all intended fall-through locations with the clang::fallthrough attribute. It makes sense to wrap [[clang::fallthrough]] to a macro, so the code would continue to work in c++98 mode, but in c++11 mode it would also allow to run this diagnostic (btw, is llvm compilable in c++11 mode?). Sample implementation of the macro (see fallthrough-macro.diff):
#ifdef __clang__
#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
#define LLVM_FALLTHROUGH [[clang::fallthrough]]
#endif
#endif
#ifndef LLVM_FALLTHROUGH
#define LLVM_FALLTHROUGH do { } while (0)
#endif


After this we can start using -Wimplicit-fallthrough.

Please express your opinions on this proposal.

I'm also ready to provide diffs for marking all fall-through locations with the LLVM_FALLTHROUGH macro, if this proposal gets approval.

--
Best regards,
Alexander Kornienko

--
Best regards,
Alexander Kornienko

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Chris Lattner-2
In reply to this post by Alexander Kornienko
<dropping llvm-commits>

On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:

> Hi llvmdev, llvm-commits,
>
> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.

I missed the earlier discussion, so I'm sorry for chiming in late.

> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.

I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.

-Chris

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Jim Grosbach

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

-Jim

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Richard Smith-33
On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <[hidden email]> wrote:

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

I believe Alex has already gone through the hits from this diagnostic; see the fallthrough-bugs-llvm.diff patch attached to the original mail on this thread for the bugs he found.

Alex: can you tell us how many FALLTHROUGH annotations would be required, and how many bugs you found, when running this over LLVM and Clang?

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <[hidden email]> wrote:
On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <[hidden email]> wrote:

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

I believe Alex has already gone through the hits from this diagnostic; see the fallthrough-bugs-llvm.diff patch attached to the original mail on this thread for the bugs he found.

Alex: can you tell us how many FALLTHROUGH annotations would be required, and how many bugs you found, when running this over LLVM and Clang?
Two months ago it was like this:
I compiled llvm and clang with the recently added -Wimplicit-fallthrough diagnostic and analyzed the results. For those who are curious: there are about 400 'unannotated fall-through' warnings in total, about 290 fall-through locations are annotated in comments, about 30 locations fall-through to an empty block (case 1: ....<no break>  case 2: break;), 7 locations have assert(false) not followed by break;
From the remaining ~70 locations 6 look like real bugs. I've prepared two patches: for llvm and clang, which add break; for all these locations. I've also removed two unnecessary fall-throughs in headers to reduce total amount of 'unannotated fall-through' warning messages. 
I can't guarantee that all these 6 locations are real bugs, but they look very much like unintended fall-throughs.

The patch with fixes is attached, it's unchecked, not reviewed, and most probably out-of-sync with current HEAD. But it shows the idea of how these bugs look like, and that it's not always easy to spot them manually. The number of actual bugs I found (5 in LLVM + 1 in Clang) isn't overwhelming, but I'm sure that many of similar bugs never got to the repository, but took enough debugging time to be found.

If the idea seems to be not bad in general, I can do all the comments to macro conversion routine, it should be totally about 400 single line changes, which doesn't seem to be a great problem for reviewers. As for me, I'm going to come up with a tool for mostly automatic transition.

--
Best regards,
Alexander Kornienko

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

fallthrough-bugs-llvm.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
Ping.

On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <[hidden email]> wrote:
On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <[hidden email]> wrote:
On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <[hidden email]> wrote:

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

I believe Alex has already gone through the hits from this diagnostic; see the fallthrough-bugs-llvm.diff patch attached to the original mail on this thread for the bugs he found.

Alex: can you tell us how many FALLTHROUGH annotations would be required, and how many bugs you found, when running this over LLVM and Clang?
Two months ago it was like this:
I compiled llvm and clang with the recently added -Wimplicit-fallthrough diagnostic and analyzed the results. For those who are curious: there are about 400 'unannotated fall-through' warnings in total, about 290 fall-through locations are annotated in comments, about 30 locations fall-through to an empty block (case 1: ....<no break>  case 2: break;), 7 locations have assert(false) not followed by break;
From the remaining ~70 locations 6 look like real bugs. I've prepared two patches: for llvm and clang, which add break; for all these locations. I've also removed two unnecessary fall-throughs in headers to reduce total amount of 'unannotated fall-through' warning messages. 
I can't guarantee that all these 6 locations are real bugs, but they look very much like unintended fall-throughs.

The patch with fixes is attached, it's unchecked, not reviewed, and most probably out-of-sync with current HEAD. But it shows the idea of how these bugs look like, and that it's not always easy to spot them manually. The number of actual bugs I found (5 in LLVM + 1 in Clang) isn't overwhelming, but I'm sure that many of similar bugs never got to the repository, but took enough debugging time to be found.

If the idea seems to be not bad in general, I can do all the comments to macro conversion routine, it should be totally about 400 single line changes, which doesn't seem to be a great problem for reviewers. As for me, I'm going to come up with a tool for mostly automatic transition.

--
Best regards,
Alexander Kornienko


--
Best regards,
Alexander Kornienko


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Alexander Kornienko
Ping.

On Thu, Aug 9, 2012 at 3:04 PM, Alexander Kornienko <[hidden email]> wrote:
Ping.


On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <[hidden email]> wrote:
On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <[hidden email]> wrote:
On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <[hidden email]> wrote:

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

I believe Alex has already gone through the hits from this diagnostic; see the fallthrough-bugs-llvm.diff patch attached to the original mail on this thread for the bugs he found.

Alex: can you tell us how many FALLTHROUGH annotations would be required, and how many bugs you found, when running this over LLVM and Clang?
Two months ago it was like this:
I compiled llvm and clang with the recently added -Wimplicit-fallthrough diagnostic and analyzed the results. For those who are curious: there are about 400 'unannotated fall-through' warnings in total, about 290 fall-through locations are annotated in comments, about 30 locations fall-through to an empty block (case 1: ....<no break>  case 2: break;), 7 locations have assert(false) not followed by break;
From the remaining ~70 locations 6 look like real bugs. I've prepared two patches: for llvm and clang, which add break; for all these locations. I've also removed two unnecessary fall-throughs in headers to reduce total amount of 'unannotated fall-through' warning messages. 
I can't guarantee that all these 6 locations are real bugs, but they look very much like unintended fall-throughs.

The patch with fixes is attached, it's unchecked, not reviewed, and most probably out-of-sync with current HEAD. But it shows the idea of how these bugs look like, and that it's not always easy to spot them manually. The number of actual bugs I found (5 in LLVM + 1 in Clang) isn't overwhelming, but I'm sure that many of similar bugs never got to the repository, but took enough debugging time to be found.

If the idea seems to be not bad in general, I can do all the comments to macro conversion routine, it should be totally about 400 single line changes, which doesn't seem to be a great problem for reviewers. As for me, I'm going to come up with a tool for mostly automatic transition.

--
Best regards,
Alexander Kornienko


--
Best regards,
Alexander Kornienko




--
Alexander Kornienko | Software Engineer | [hidden email] | +49 151 221 77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

Jim Grosbach
This is Chris' call to make, imo.

-j
On Aug 22, 2012, at 3:59 PM, Alexander Kornienko <[hidden email]> wrote:

Ping.

On Thu, Aug 9, 2012 at 3:04 PM, Alexander Kornienko <[hidden email]> wrote:
Ping.


On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <[hidden email]> wrote:
On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <[hidden email]> wrote:
On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <[hidden email]> wrote:

On Jul 26, 2012, at 4:07 PM, Chris Lattner <[hidden email]> wrote:

> <dropping llvm-commits>
>
> On Jul 2, 2012, at 9:59 AM, Alexander Kornienko wrote:
>
>> Hi llvmdev, llvm-commits,
>>
>> There was a discussion on this topic a while ago, and now I've decided to make a formal proposal and post it here.
>
> I missed the earlier discussion, so I'm sorry for chiming in late.
>
>> I propose to add the LLVM_FALLTHROUGH macro for specifying intended fall-through locations between switch cases.
>
> I don't really see that the tradeoff here is worthwhile.  It is possible that we have some fallthrough bugs, but the cost of sprinkling this macro everywhere doesn't seem like the right tradeoff.
>

While I tend to agree with you, it's also true that for many (most?) of the locations where we have an intentional fall through, there's already a comment to that effect as a matter of style. This would simply formalize that currently voluntary bit of style and use the macro rather than a comment. Depending on how many bugs this would enable find (perhaps some experiments are in order?), I could be convinced the tradeoff is worthwhile.

I believe Alex has already gone through the hits from this diagnostic; see the fallthrough-bugs-llvm.diff patch attached to the original mail on this thread for the bugs he found.

Alex: can you tell us how many FALLTHROUGH annotations would be required, and how many bugs you found, when running this over LLVM and Clang?
Two months ago it was like this:
I compiled llvm and clang with the recently added -Wimplicit-fallthrough diagnostic and analyzed the results. For those who are curious: there are about 400 'unannotated fall-through' warnings in total, about 290 fall-through locations are annotated in comments, about 30 locations fall-through to an empty block (case 1: ....<no break>  case 2: break;), 7 locations have assert(false) not followed by break;
From the remaining ~70 locations 6 look like real bugs. I've prepared two patches: for llvm and clang, which add break; for all these locations. I've also removed two unnecessary fall-throughs in headers to reduce total amount of 'unannotated fall-through' warning messages. 
I can't guarantee that all these 6 locations are real bugs, but they look very much like unintended fall-throughs.

The patch with fixes is attached, it's unchecked, not reviewed, and most probably out-of-sync with current HEAD. But it shows the idea of how these bugs look like, and that it's not always easy to spot them manually. The number of actual bugs I found (5 in LLVM + 1 in Clang) isn't overwhelming, but I'm sure that many of similar bugs never got to the repository, but took enough debugging time to be found.

If the idea seems to be not bad in general, I can do all the comments to macro conversion routine, it should be totally about 400 single line changes, which doesn't seem to be a great problem for reviewers. As for me, I'm going to come up with a tool for mostly automatic transition.

--
Best regards,
Alexander Kornienko


--
Best regards,
Alexander Kornienko




--
Alexander Kornienko | Software Engineer | [hidden email] | +49 151 221 77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev