diff mbox

c_value_print: Revert 'val' to a reference for TYPE_CODE_STRUCT

Message ID 1461707298-26514-1-git-send-email-martin.galvan@tallertechnologies.com
State New, archived
Headers

Commit Message

Martin Galvan April 26, 2016, 9:48 p.m. UTC 
  Currently c_value_print will turn struct reference values into pointers before doing
a set of RTTI checks. This was introduced as a fix to PR c++/15401. If there's RTTI
the pointer will be adjusted and converted back to a reference. However, if there's
no RTTI the value will still be treated as a pointer during the remainder of the function.
This patch moves the conversion down so that it's always performed when needed.

I have write access and copyright assignment. Ok to commit?

gdb/ChangeLog:
2016-04-26  Martin Galvan  <martin.galvan@tallertechnologies.com>

	* c-valprint.c (c_value_print): Always convert val back to reference
	type if we converted it to a pointer type.

---
 gdb/c-valprint.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Pedro Alves April 27, 2016, 10:01 a.m. UTC | #1 
On 04/26/2016 10:48 PM, Martin Galvan wrote:
> Currently c_value_print will turn struct reference values into pointers before doing
> a set of RTTI checks. This was introduced as a fix to PR c++/15401. If there's RTTI
> the pointer will be adjusted and converted back to a reference. However, if there's
> no RTTI the value will still be treated as a pointer during the remainder of the function.
> This patch moves the conversion down so that it's always performed when needed.

What's the motivation behind this?  Does it change anything user visible?

Thanks,
Pedro Alves
Martin Galvan April 27, 2016, 1:21 p.m. UTC | #2 
On Wed, Apr 27, 2016 at 7:01 AM, Pedro Alves <palves@redhat.com> wrote:
> What's the motivation behind this?  Does it change anything user visible?

AFAIK not directly, but I'm going to need it for the synthetic reference bug
fix. Since this is an isolated change I thought I could send it for
review now.
Pedro Alves April 27, 2016, 1:39 p.m. UTC | #3 
On 04/27/2016 02:21 PM, Martin Galvan wrote:
> On Wed, Apr 27, 2016 at 7:01 AM, Pedro Alves <palves@redhat.com> wrote:
>> What's the motivation behind this?  Does it change anything user visible?
> 
> AFAIK not directly, but I'm going to need it for the synthetic reference bug
> fix.

I see. 

> Since this is an isolated change I thought I could send it for
> review now.

Since you didn't mention whether the change had any user-visible
impact, I was left wondering if we could add a testcase to
the testsuite that exposes the need for the change.  From the original
log it kind of sounded like it would be possible.

It's better to be explicit in such cases, and say something like,
"this has no effect currently, so can be seen as a small code
cleanup, but once we do X, we'll print the wrong thing", or some such,
and mention that this causes no testsuite regressions, in the 
email/commit log.

The code change is OK.

Thanks,
Pedro Alves
Martin Galvan April 27, 2016, 3:10 p.m. UTC | #4 
On Wed, Apr 27, 2016 at 10:39 AM, Pedro Alves <palves@redhat.com> wrote:
> It's better to be explicit in such cases, and say something like,
> "this has no effect currently, so can be seen as a small code
> cleanup, but once we do X, we'll print the wrong thing", or some such,
> and mention that this causes no testsuite regressions, in the
> email/commit log.

Indeed, you're right. I mentioned it in the commit log. Here's what I committed:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git&a=commit&h=476350ba4800f1144b125f6511a5e25b223cc90b

Thanks!
diff mbox

Patch

diff --git a/gdb/c-valprint.c b/gdb/c-valprint.c
index 62552ec..e1da3d5 100644
--- a/gdb/c-valprint.c
+++ b/gdb/c-valprint.c
@@ -611,7 +611,7 @@  c_value_print (struct value *val, struct ui_file *stream,
 	  fprintf_filtered (stream, "(");
 
 	  if (value_entirely_available (val))
- 	    {
+	    {
 	      real_type = value_rtti_indirect_type (val, &full, &top,
 						    &using_enc);
 	      if (real_type)
@@ -623,18 +623,19 @@  c_value_print (struct value *val, struct ui_file *stream,
 		  val = value_from_pointer (real_type,
 					    value_as_address (val) - top);
 
-		  if (is_ref)
-		    {
-		      val = value_ref (value_ind (val));
-		      type = value_type (val);
-		    }
-
 		  /* Note: When we look up RTTI entries, we don't get
 		     any information on const or volatile
 		     attributes.  */
 		}
 	    }
-          type_print (type, "", stream, -1);
+
+	  if (is_ref)
+	    {
+	      val = value_ref (value_ind (val));
+	      type = value_type (val);
+	    }
+
+	  type_print (type, "", stream, -1);
 	  fprintf_filtered (stream, ") ");
 	  val_type = type;
 	}