[Bug c/57880] New: cp/parser.c: 6 * missing break ?

[Bug c/57880] New: cp/parser.c: 6 * missing break ?

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

            Bug ID: 57880
           Summary: cp/parser.c: 6 * missing break ?
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com

I just ran the static analysis checker "cppcheck" over the source code
of trunk 20130710. It said

Checking trunk/gcc/cp/parser.c...
[trunk/gcc/cp/parser.c:12465] -> [trunk/gcc/cp/parser.c:12468]: (warning)
Variable 'string_len' is reassigned a value before the old one has been used.
'break;' missing?
[trunk/gcc/cp/parser.c:12468] -> [trunk/gcc/cp/parser.c:12470]: (warning)
Variable 'string_len' is reassigned a value before the old one has been used.
'break;' missing?
[trunk/gcc/cp/parser.c:12508] -> [trunk/gcc/cp/parser.c:12511]: (warning)
Variable 'string_len' is reassigned a value before the old one has been used.
'break;' missing?
[trunk/gcc/cp/parser.c:12511] -> [trunk/gcc/cp/parser.c:12513]: (warning)
Variable 'string_len' is reassigned a value before the old one has been used.
'break;' missing?

Source code is

    case CPP_WSTRING_USERDEF:
      string_len = 3;
    case CPP_STRING16_USERDEF:
    case CPP_STRING32_USERDEF:
      string_len = 5;
    case CPP_UTF8STRING_USERDEF:
      string_len = 4;
      bad_encoding_prefix = true;
    case CPP_STRING_USERDEF:

Code obviously broken. Suggest rework and maybe a look at related bug # 
7652 would also help.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |emsr at gcc dot gnu.org
          Component|c                           |c++

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Indeed something seems wrong. CC-ing Ed.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Ed?

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #3 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Paolo Carlini from comment #2)
> Ed?

He's had eleven days to think about it and said nothing.

I'd be happy to supply the obvious patch.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #4 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Maybe he is simply traveling or unable to read email, don't you think?

Still, if you want to send a patch to the mailing list, of course it's fine.
For a quicker review, I would recommend CC-ing Jason and adding [C++ Patch] to
the subject line. Thanks.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #5 from David Binderman <dcb314 at hotmail dot com> ---
Created attachment 30545
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30545&action=edit
source code patch

This patch seems to shut up cppcheck.

I did a bootstrap and that seemed ok.

It seems good to go to me.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #6 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Paolo Carlini from comment #4)
> For a quicker review, I would recommend CC-ing Jason and adding [C++ Patch]
> to the subject line.

I'm not on the inner wheel for C++ patches, so I am guessing that
you mean Jason Merrill.

If so, is jason at redhat dot com a good email address for him ?

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, or at gcc.gnu.org

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-07-24
     Ever confirmed|0                           |1

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> ---
If we have a straightforward explanation for why the very weird code isn't
causing problems, I think the fix almost qualifies as obvious, if properly
tested. Do we? Does it only amount to a small memory leak or a small buffer
overrun, something similar? In any case, yes, gcc-patches@ CC Jason Merrill
(there aren't *that* many C++ maintainers on gcc-patches named Jason ;)

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> ---
If we have a straightforward explanation for why the very weird code isn't
causing problems, I think the fix almost qualifies as obvious, if properly
tested. Do we? Does it only amount to a small memory leak or a small buffer
overrun, something similar? In any case, yes, gcc-patches@ CC Jason Merrill
(there aren't *that* many C++ maintainers on gcc-patches named Jason ;)

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |paolo.carlini at oracle dot com

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> ---
I had a closer look and I don't think the proposed patchlet is correct: for,
eg, case CPP_WSTRING we want string_len = 3 and then fall through to the
checks.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.0

--- Comment #11 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Fixed for 4.9.0.

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #12 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Paolo Carlini from comment #11)
> Fixed for 4.9.0.

Good news. Worth referencing the patch that fixed it ?

[Bug c++/57880] cp/parser.c: 6 * missing break ?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57880

--- Comment #13 from Paolo Carlini <paolo.carlini at oracle dot com> ---
I added c++/57880 to the svn CL. Sorry, I'm not going to reference by hand each
and every patch I commit (svn should do it automatically, send a message to
Bugzilla, as it used to).