potential patch for gdb bug c++/20020

potential patch for gdb bug c++/20020

[Bob Steagall]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

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   <bob.steagall.cpp@gmail.com>

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

[-- Attachment #2: fix-for-20020.patch --]
[-- Type: application/x-patch, Size: 790 bytes --]

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

[Andrew Burgess]

* Bob Steagall <bob.steagall.cpp@gmail.com> [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   <bob.steagall.cpp@gmail.com>
> 
>         * 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

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

[Bob Steagall]

On Thu, Dec 6, 2018 at 2:20 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> * Bob Steagall <bob.steagall.cpp@gmail.com> [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   <bob.steagall.cpp@gmail.com>
> >
> >         * 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

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

[Tom Tromey]

>>>>> "Bob" == Bob Steagall <bob.steagall.cpp@gmail.com> 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

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 <tom@tromey.com> wrote:
>
> >>>>> "Bob" == Bob Steagall <bob.steagall.cpp@gmail.com> 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