llvm dev - Aug 2012 - [LLVMdev] [llvm-commits] PROPOSAL: LLVM_FALLTHROUGH macro for intended fall-throughs between switch cases

On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <richard at
metafoo.co.uk>wrote:
> On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <grosbach at apple.com>
wrote:
>
>>
>> On Jul 26, 2012, at 4:07 PM, Chris Lattner <clattner at
apple.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120727/8899c4a3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fallthrough-bugs-llvm.diff
Type: application/octet-stream
Size: 2402 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120727/8899c4a3/attachment.obj>

Ping.

On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <alexfh at
google.com>wrote:
> On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <richard at
metafoo.co.uk>wrote:
>
>> On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <grosbach at
apple.com> wrote:
>>
>>>
>>> On Jul 26, 2012, at 4:07 PM, Chris Lattner <clattner at
apple.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120809/aa660b74/attachment.html>

Ping.

On Thu, Aug 9, 2012 at 3:04 PM, Alexander Kornienko <alexfh at
google.com>wrote:
> Ping.
>
>
> On Fri, Jul 27, 2012 at 3:42 AM, Alexander Kornienko <alexfh at
google.com>wrote:
>
>> On Fri, Jul 27, 2012 at 2:02 AM, Richard Smith <richard at
metafoo.co.uk>wrote:
>>
>>> On Thu, Jul 26, 2012 at 4:24 PM, Jim Grosbach <grosbach at
apple.com>wrote:
>>>
>>>>
>>>> On Jul 26, 2012, at 4:07 PM, Chris Lattner <clattner at
apple.com> 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 | alexfh at google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120823/a172e426/attachment.html>