[llvm-commits] [llvm] r162230 - in /llvm/trunk: lib/CodeGen/MachineVerifier.cpp lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp test/CodeGen/ARM/constants.ll

[Richard Smith]

On Wed, Dec 19, 2012 at 2:19 PM, Eli Bendersky <eliben at google.com> wrote:
> On Wed, Dec 19, 2012 at 2:16 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> On Wed, Dec 19, 2012 at 11:10 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>> On Wed, Dec 19, 2012 at 1:04 PM, Dmitri Gribenko <gribozavr at gmail.com>
>>> wrote:
>>>>
>>>> On Wed, Dec 19, 2012 at 11:02 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk>
>>>> wrote:
>>>> >
>>>> > On Dec 19, 2012, at 11:01 AM, Dmitri Gribenko <gribozavr at gmail.com>
>>>> > wrote:
>>>> >
>>>> >> On Tue, Aug 21, 2012 at 12:39 AM, Jakob Stoklund Olesen
>>>> >> <stoklund at 2pi.dk> wrote:
>>>> >>>       ++MBBI;
>>>> >>>       if (MBBI == MF->end()) {
>>>> >>>         report("MBB conditionally falls through out of function!",
>>>> >>> MBB);
>>>> >>> -      } if (MBB->succ_size() != 2) {
>>>> >>> +      } if (MBB->succ_size() == 1) {
>>>> >>
>>>> >> Hello Jakob,
>>>> >>
>>>> >> Sorry to comment on an old commit, but '} if (...' looks suspicious --
>>>> >> 'else' is probably missing.  I have just fixed two bugs of this kind
>>>> >> in Clang, found by grep '} if'.
>>>> >
>>>> > Yikes, that does indeed look like a missing else.
>>>> >
>>>> > Please go ahead and fix it.
>>>>
>>>> OK to commit without a testcase?
>>>
>>>
>>> I'd like a testcase. =]
>>>
>>> Dmitri, want to try your hand at building a Clang warning for this? I just
>>> chatted with Richard and he seemed to think it could work by warning in the
>>> Parser when looking for an 'else' token, finding an 'if' token, and the '}'
>>> and 'if' tokens' spelling locations are on the same line. (or checking the
>>> start-of-line flag on the 'if' token, but Richard thought there might be
>>> problems there).
>>
>> That's a good idea.  So far LLVM/Clang had three cases of this, and
>> all are true bugs.
>>
>
> I wonder if other open-source projects have this bug somewhere as
> well. Maybe some code-search online thingy can help find out. If you
> go ahead and add a new Clang warning, it's important to know whether
> there are going to be legit uses that will be warned about.

Yes, there are several popular open-source projects with this bug. My
survey of the hits for "} if" found a double-digit number of bugs,
some cases where adding the 'else' would make no semantic difference
(the previous block ended in return, continue, break, exit, etc), and
only one case where the intent was clearly to have two separate if
statements (and even in that case, the newline appears to have been
accidentally forgotten).