potential patch for gdb bug c++/20020

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

potential patch for gdb bug c++/20020

Bob Steagall
Description: This patch, against released version 8.2, fixes the
problem reported in gdb bug c++/20020, using the approach described in
comment 1 of that report.

Changelog entry:

2018-12-06  Bob Steagall   <[hidden email]>

        * cp-valprint.c: Fixes bug c++/20020.

fix-for-20020.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: potential patch for gdb bug c++/20020

Andrew Burgess
* Bob Steagall <[hidden email]> [2018-12-06 13:29:31 -0500]:

> Description: This patch, against released version 8.2, fixes the
> problem reported in gdb bug c++/20020, using the approach described in
> comment 1 of that report.
>
> Changelog entry:
>
> 2018-12-06  Bob Steagall   <[hidden email]>
>
>         * cp-valprint.c: Fixes bug c++/20020.

> --- gdb/cp-valprint.c 2018-09-05 03:27:13.000000000 -0400
> +++ gdb/cp-valprint.c.new 2018-12-06 13:01:06.819266165 -0500
> @@ -326,12 +326,16 @@ cp_print_value_fields (struct type *type
>        fprintf_filtered (stream,
>   _("<error reading variable: %s>"),
>   ex.message);
> +                      v = NULL;

I don't think this NULL assignment should be needed.  `v` starts as
NULL, and we only end in this block if `value_static_field` throws an
exception, which will be before `v` is assigned too.

>      }
>    END_CATCH
>  
> -  cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> - v, stream, recurse + 1,
> - options);
> +                  if (v != NULL)
> +                    {

You should drop the '{' and '}' here for a single statement block.

> +                      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> +                                             v, stream, recurse + 1,
> +                                             options);
> +                    }
>   }
>        else if (i == vptr_fieldno && type == vptr_basetype)
>   {

I'm not a maintainer so can't approve patches, but this seems sensible
to me.

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: potential patch for gdb bug c++/20020

Bob Steagall
On Thu, Dec 6, 2018 at 2:20 PM Andrew Burgess
<[hidden email]> wrote:

>
> * Bob Steagall <[hidden email]> [2018-12-06 13:29:31 -0500]:
>
> > Description: This patch, against released version 8.2, fixes the
> > problem reported in gdb bug c++/20020, using the approach described in
> > comment 1 of that report.
> >
> > Changelog entry:
> >
> > 2018-12-06  Bob Steagall   <[hidden email]>
> >
> >         * cp-valprint.c: Fixes bug c++/20020.
>
> > --- gdb/cp-valprint.c 2018-09-05 03:27:13.000000000 -0400
> > +++ gdb/cp-valprint.c.new     2018-12-06 13:01:06.819266165 -0500
> > @@ -326,12 +326,16 @@ cp_print_value_fields (struct type *type
> >                     fprintf_filtered (stream,
> >                                       _("<error reading variable: %s>"),
> >                                       ex.message);
> > +                      v = NULL;
>
> I don't think this NULL assignment should be needed.  `v` starts as
> NULL, and we only end in this block if `value_static_field` throws an
> exception, which will be before `v` is assigned too.

Agreed.  I was being, perhaps, too paranoid here.

> >                   }
> >                 END_CATCH
> >
> > -               cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> > -                                      v, stream, recurse + 1,
> > -                                      options);
> > +                  if (v != NULL)
> > +                    {
>
> You should drop the '{' and '}' here for a single statement block.

Disagree.  The gdb coding standard document specifically calls out
lines of code,
not statements:

    "Any two or more lines in code should be wrapped in braces, even if they
     are comments, as they look like separate statements"

> > +                      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> > +                                             v, stream, recurse + 1,
> > +                                             options);
> > +                    }
> >               }
> >             else if (i == vptr_fieldno && type == vptr_basetype)
> >               {
>
> I'm not a maintainer so can't approve patches, but this seems sensible
> to me.
>
> Thanks,
> Andrew

I will update and re-send.

Thanks for the review,
--Bob
Reply | Threaded
Open this post in threaded view
|

Re: potential patch for gdb bug c++/20020

Tom Tromey-2
>>>>> "Bob" == Bob Steagall <[hidden email]> writes:

>> You should drop the '{' and '}' here for a single statement block.

Bob> Disagree.  The gdb coding standard document specifically calls out
Bob> lines of code,
Bob> not statements:

Bob>     "Any two or more lines in code should be wrapped in braces, even if they
Bob>      are comments, as they look like separate statements"

I think this is just slightly mis-worded -- Andrew's interpretation is
the prevailing one.  That is, no brace for:

  if (blah)
     function_call (spanning,
                    multiple,
                    lines);

... but do have braces for:

  if (blah)
    {
      /* Just a comment.  */
      anything ();
    }

I agree the patch is good.  I think a test case would be good to have,
unless for some reason it's difficult to write one.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: potential patch for gdb bug c++/20020

Bob Steagall
OK, I will update to include both suggestions.

Regarding the test case, I don't know how to write a unit test case if
that's what you're asking for.  However, there are directions for
reproducing the problem I experience in my original bug report
c++/23953 (subsequently closed as a duplicate of 20020).

I'll send the updated patch momentarily.

--Bob
On Thu, Dec 6, 2018 at 4:07 PM Tom Tromey <[hidden email]> wrote:

>
> >>>>> "Bob" == Bob Steagall <[hidden email]> writes:
>
> >> You should drop the '{' and '}' here for a single statement block.
>
> Bob> Disagree.  The gdb coding standard document specifically calls out
> Bob> lines of code,
> Bob> not statements:
>
> Bob>     "Any two or more lines in code should be wrapped in braces, even if they
> Bob>      are comments, as they look like separate statements"
>
> I think this is just slightly mis-worded -- Andrew's interpretation is
> the prevailing one.  That is, no brace for:
>
>   if (blah)
>      function_call (spanning,
>                     multiple,
>                     lines);
>
> ... but do have braces for:
>
>   if (blah)
>     {
>       /* Just a comment.  */
>       anything ();
>     }
>
> I agree the patch is good.  I think a test case would be good to have,
> unless for some reason it's difficult to write one.
>
> Tom