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 <***@gmail.com>

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

[Andrew Burgess]

Post by 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.
* 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.

Post by Bob Steagall
}
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.

Post by Bob Steagall
+ 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

[Bob Steagall]

On Thu, Dec 6, 2018 at 2:20 PM Andrew Burgess

Post by Andrew Burgess

Post by 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.
* 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.

Post by Andrew Burgess

Post by Bob Steagall
}
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"

Post by Andrew Burgess

Post by Bob Steagall
+ 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

[Tom Tromey]

Post by Andrew Burgess
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

[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

Post by Tom Tromey

Post by Andrew Burgess
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> "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
if (blah)
function_call (spanning,
multiple,
lines);
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