[RFC] printing/setting flag register fields

[RFC] printing/setting flag register fields

[Doug Evans]

Hi.

Before I go too far down this path, I'm looking for early feedback.

What do folks think?

(gdb) pty $eflags
type = builtin_type_i386_eflags
     [ CF@0 PF@2 AF@4 ZF@6 SF@7 TF@8 IF@9 DF@10 OF@11 NT@14 RF@16 VM@17 AC@18 VIF@19 VIP@20 ID@21 ]
(gdb) p $eflags.CF
$1 = 0
(gdb) set $eflags.CF = 1
(gdb) p $eflags
$2 = [ CF PF ZF IF ]
(gdb) p/x $eflags
$3 = 0x247
(gdb) p $eflags.foo
There is no member named foo.
(gdb) 

"pty $eflags" output is a bit hard to read, but I thought I'd play
with printing the field names *and* field positions.
If folks just want just the field names, ok.
And I need to add a wrap_here call to avoid running past
the column width.
Since this is an artificial type we can print it however we want,
I don't have much of a preference on the format.
Since "p $eflags" currently wraps the fields in [ ... ],
I chose to wrap the pty output similarily.

One improvement I like is to have append_flags_type_flag fill in
more stuff, so that TYPE_CODE_FLAGS types look more like other types.

Calling value_flags_elt from value_struct_elt feels clumsy,
but only mildly so.  I can rewrite this however folks like.

[NEWS and docs elided for now, I'm just seeking feedback on the approach.]

2009-09-18  Doug Evans  <dje@google.com>

	Add support for printing and setting individual flag register fields.
	* c-typeprint.c (c_type_print_varspec_prefix): Handle TYPE_CODE_FLAGS.
	(c_type_print_varspec_suffix): Ditto.
	(c_type_print_base): Ditto.
	* valops.c (value_flags_elt): New function.
	(value_struct_elt): Call it for TYPE_CODE_FLAGS types.
	* value.c (value_flags_field): New function.
	* value.h (value_flags_field): Declare it.

Index: c-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-typeprint.c,v
retrieving revision 1.47
diff -u -p -r1.47 c-typeprint.c
--- c-typeprint.c	2 Jul 2009 12:57:14 -0000	1.47
+++ c-typeprint.c	18 Sep 2009 23:33:36 -0000
@@ -297,6 +297,7 @@ c_type_print_varspec_prefix (struct type
     case TYPE_CODE_TEMPLATE:
     case TYPE_CODE_NAMESPACE:
     case TYPE_CODE_DECFLOAT:
+    case TYPE_CODE_FLAGS:
       /* These types need no prefix.  They are listed here so that
          gcc -Wall will reveal any types that haven't been handled.  */
       break;
@@ -621,6 +622,7 @@ c_type_print_varspec_suffix (struct type
     case TYPE_CODE_TEMPLATE:
     case TYPE_CODE_NAMESPACE:
     case TYPE_CODE_DECFLOAT:
+    case TYPE_CODE_FLAGS:
       /* These types do not need a suffix.  They are listed so that
          gcc -Wall will report types that may not have been considered.  */
       break;
@@ -1149,6 +1151,30 @@ c_type_print_base (struct type *type, st
       fputs_filtered (TYPE_TAG_NAME (type), stream);
       break;
 
+    case TYPE_CODE_FLAGS:
+      c_type_print_modifier (type, stream, 0, 1);
+      fputs_filtered (TYPE_NAME (type), stream);
+      if (show < 0)
+	fprintf_filtered (stream, " [...]");
+      else if (show > 0)
+	{
+	  wrap_here ("    ");
+	  fprintf_filtered (stream, " [");
+	  len = TYPE_NFIELDS (type);
+	  for (i = 0; i < len; ++i)
+	    {
+	      if (TYPE_FIELD_NAME (type, i) != NULL
+		  && TYPE_FIELD_BITPOS (type, i) >= 0)
+		{
+		  fputs_filtered (" ", stream);
+		  fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
+		  fprintf_filtered (stream, "@%d", TYPE_FIELD_BITPOS (type, i));
+		}
+	    }
+	  fprintf_filtered (stream, " ]");
+	}
+      break;
+
     default:
       /* Handle types not explicitly handled by the other cases,
          such as fundamental types.  For these, just print whatever
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.225
diff -u -p -r1.225 valops.c
--- valops.c	31 Aug 2009 20:18:45 -0000	1.225
+++ valops.c	18 Sep 2009 23:33:36 -0000
@@ -1799,6 +1799,27 @@ search_struct_method (char *name, struct
     return NULL;
 }
 
+/* Subroutine of value_struct_elt to simplify it.
+   Once we know we have a TYPE_CODE_FLAGS object,
+   extract the flag named NAME and return it as a value with its
+   appropriate type.  */
+
+static struct value *
+value_flags_elt (struct type *type, struct value *flags, const char *name)
+{
+  int i;
+
+  for (i = 0; i < TYPE_NFIELDS (type); ++i)
+    {
+      const char *t_field_name = TYPE_FIELD_NAME (type, i);
+
+      if (t_field_name && (strcmp_iw (t_field_name, name) == 0))
+	return value_flags_field (flags, i, type);
+    }
+
+  error (_("There is no member named %s."), name);
+}
+
 /* Given *ARGP, a value of type (pointer to a)* structure/union,
    extract the component named NAME from the ultimate target
    structure/union and return it as a value with its appropriate type.
@@ -1836,6 +1857,9 @@ value_struct_elt (struct value **argp, s
       t = check_typedef (value_type (*argp));
     }
 
+  if (TYPE_CODE (t) == TYPE_CODE_FLAGS)
+    return value_flags_elt (t, *argp, name);
+
   if (TYPE_CODE (t) != TYPE_CODE_STRUCT
       && TYPE_CODE (t) != TYPE_CODE_UNION)
     error (_("Attempt to extract a component of a value that is not a %s."), err);
Index: value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.96
diff -u -p -r1.96 value.c
--- value.c	31 Aug 2009 20:18:45 -0000	1.96
+++ value.c	18 Sep 2009 23:33:36 -0000
@@ -1859,6 +1859,26 @@ value_change_enclosing_type (struct valu
   return val;
 }
 
+struct value *
+value_flags_field (struct value *arg1, int fieldno, struct type *arg_type)
+{
+  struct type *type = builtin_type (get_type_arch (arg_type))->builtin_unsigned_int;
+  struct value *v;
+
+  v = allocate_value_lazy (type);
+  v->bitsize = 1;
+  v->bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno);
+  v->offset = 0;
+  v->parent = arg1;
+  value_incref (v->parent);
+  if (!value_lazy (arg1))
+    value_fetch_lazy (v);
+  set_value_component_location (v, arg1);
+  VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
+  VALUE_FRAME_ID (v) = VALUE_FRAME_ID (arg1);
+  return v;
+}
+
 /* Given a value ARG1 (offset by OFFSET bytes)
    of a struct or union type ARG_TYPE,
    extract and return the value of one of its (non-static) fields.
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.149
diff -u -p -r1.149 value.h
--- value.h	31 Aug 2009 20:18:45 -0000	1.149
+++ value.h	18 Sep 2009 23:33:36 -0000
@@ -451,13 +451,15 @@ extern int find_overload_match (struct t
 				struct value **valp, struct symbol **symp,
 				int *staticp);
 
+extern struct value *value_flags_field (struct value *arg1, int fieldno,
+					struct type *arg_type);
+
 extern struct value *value_field (struct value *arg1, int fieldno);
 
 extern struct value *value_primitive_field (struct value *arg1, int offset,
 					    int fieldno,
 					    struct type *arg_type);
 
-
 extern struct type *value_rtti_target_type (struct value *, int *, int *,
 					    int *);
 

Re: [RFC] printing/setting flag register fields

[Mark Kettenis]

> Date: Fri, 18 Sep 2009 16:56:32 -0700 (PDT)
> From: dje@google.com (Doug Evans)
> 
> "pty $eflags" output is a bit hard to read, but I thought I'd play
> with printing the field names *and* field positions.

There is a nasty problem here that bit numbering is inconsistent
across architectures.  People typically call the LSB '0', but on
powerpc or hppa people start numbering from the MSB.

Also, how would you handle multi-bit fields?

Re: [RFC] printing/setting flag register fields

[Andreas Schwab]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

> There is a nasty problem here that bit numbering is inconsistent
> across architectures.  People typically call the LSB '0', but on
> powerpc or hppa people start numbering from the MSB.

And on m68k you even have both.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] printing/setting flag register fields

[Daniel Jacobowitz]

On Fri, Sep 18, 2009 at 04:56:32PM -0700, Doug Evans wrote:
> Hi.
> 
> Before I go too far down this path, I'm looking for early feedback.
> 
> What do folks think?

Just my two cents - flag types were a workaround for GDB's lack of
good pretty-printing facilities.  They should be just structs
containing bitfields, with a default pretty-printer.  And/or a union
with an accessible integer value.  Anywhere that our handling of such
constructs isn't good enough for eflags, it's not good enough for user
code either, and I deal with a lot of code of this nature.

As for bitfield numbering, I think we should use the corresponding
architecture's conventions; I don't know what the m68k complication
is, though.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] printing/setting flag register fields

[Doug Evans]

On Sun, Sep 20, 2009 at 11:00 AM, Daniel Jacobowitz <drow@false.org> wrote:
> Just my two cents - flag types were a workaround for GDB's lack of
> good pretty-printing facilities.  They should be just structs
> containing bitfields, with a default pretty-printer.  And/or a union
> with an accessible integer value.  Anywhere that our handling of such
> constructs isn't good enough for eflags, it's not good enough for user
> code either, and I deal with a lot of code of this nature.

I read somewhere that one advantage of TYPE_CODE_FLAGS was that one
didn't have to deal with the vagaries of ABI-struct layout.  Alas I
can't find it now.  [And I'm not sure it's really relevant as the data
recorded in the underlying type doesn't have to follow ABI rules, so
maybe I've confused it with something else.]

Should TYPE_CODE_FLAGS be nuked?  I'm happy to do that instead if
that's what folks want.  I like it, but if we made eflags a union of a
struct and an int, then "set $eflags.ZF = 0" won't work.  Are folks
happy with "set $eflags.bits.ZF = 0"?  "works for me".

> As for bitfield numbering, I think we should use the corresponding
> architecture's conventions; I don't know what the m68k complication
> is, though.

"works for me".
My higher order question, though, is should the bitfield positions be
displayed in the output of ptype (for these objects, I wouldn't do it
by default in general of course).
I like it there so that one doesn't have to look in any manual to know
ZF, for example, is in bit 1 << 6 (gdb has so much information
internally, we should try to provide more of it to the user).
But it could be done differently.  An off-the-cuff example is an
option to ptype to print field offsets for structs in general.  [I'm
assuming such a facility doesn't already exist.]  That would probably
be more useful than always printing the offsets anyway.

Re: [RFC] printing/setting flag register fields

[Daniel Jacobowitz]

On Sun, Sep 20, 2009 at 11:35:31AM -0700, Doug Evans wrote:
> Should TYPE_CODE_FLAGS be nuked?  I'm happy to do that instead if
> that's what folks want.  I like it, but if we made eflags a union of a
> struct and an int, then "set $eflags.ZF = 0" won't work.  Are folks
> happy with "set $eflags.bits.ZF = 0"?  "works for me".

IMO what we really want is the struct - but with a way to say "print
the whole word-sized struct as a single integer".  This is something I
need often.  I'd like to see "$eflags is 0x12f, which is the A B and C
bits", without GDB syntax getting in my way.

Yes, that's vague :-)

> But it could be done differently.  An off-the-cuff example is an
> option to ptype to print field offsets for structs in general.  [I'm
> assuming such a facility doesn't already exist.]  That would probably
> be more useful than always printing the offsets anyway.

I think such an option was submitted, once upon a time.  I don't know
what became of it.  I agree this would be more useful.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] printing/setting flag register fields

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> An off-the-cuff example is an option to ptype to print field
Doug> offsets for structs in general.  [I'm assuming such a facility
Doug> doesn't already exist.]  That would probably be more useful than
Doug> always printing the offsets anyway.

I once wrote a simple, C/C++-only patch to add pahole-like output to
ptype.  I appended it.

Also in the archer repository there is a "pahole" command written in
Python.  It does something similar.

I like the idea of an option to ptype.

Tom

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 56d12f9..c38fc31 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -744,6 +744,9 @@ c_type_print_base (struct type *type, struct ui_file *stream, int show,
 	}
       else if (show > 0 || TYPE_TAG_NAME (type) == NULL)
 	{
+	  int bitpos, field_size;
+	  int is_union = TYPE_CODE (type) == TYPE_CODE_UNION;
+
 	  cp_type_print_derivation_info (stream, type);
 
 	  fprintf_filtered (stream, "{\n");
@@ -830,6 +833,7 @@ c_type_print_base (struct type *type, struct ui_file *stream, int show,
 	  /* If there is a base class for this type,
 	     do not print the field that it occupies.  */
 
+	  bitpos = 0;
 	  len = TYPE_NFIELDS (type);
 	  for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 	    {
@@ -871,6 +875,25 @@ c_type_print_base (struct type *type, struct ui_file *stream, int show,
 		    }
 		}
 
+	      if (!is_union && bitpos != TYPE_FIELD_BITPOS (type, i))
+		{
+		  fprintf_filtered (stream,
+				    "\n /* XXX %d bit hole, try to pack */\n\n",
+				    TYPE_FIELD_BITPOS (type, i) - bitpos);
+		  bitpos = TYPE_FIELD_BITPOS (type, i);
+		}
+	      if (TYPE_FIELD_PACKED (type, i))
+		field_size = TYPE_FIELD_BITSIZE (type, i);
+	      else
+		field_size = 8 * TYPE_LENGTH (TYPE_FIELD_TYPE (type, i)); /* FIXME */
+
+	      if (is_union)
+		print_spaces_filtered (14, stream);
+	      else
+		fprintf_filtered (stream, " /* %3d %3d */",
+				  bitpos / 8, field_size / 8);
+	      bitpos += field_size;
+
 	      print_spaces_filtered (level + 4, stream);
 	      if (TYPE_FIELD_STATIC (type, i))
 		{
@@ -1036,6 +1059,8 @@ c_type_print_base (struct type *type, struct ui_file *stream, int show,
 		}
 	    }
 
+	  if (level)
+	    print_spaces_filtered (14, stream);
 	  fprintfi_filtered (level, stream, "}");
 
 	  if (TYPE_LOCALTYPE_PTR (type) && show >= 0)