From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8509 invoked by alias); 24 Jan 2007 08:24:17 -0000 Received: (qmail 8486 invoked by uid 22791); 24 Jan 2007 08:24:09 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 24 Jan 2007 08:24:04 +0000 Received: (qmail 12812 invoked from network); 24 Jan 2007 08:23:59 -0000 Received: from unknown (HELO ?172.16.64.38?) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Jan 2007 08:23:59 -0000 From: Vladimir Prus To: gdb-patches@sources.redhat.com Subject: Lazy bitfields Date: Wed, 24 Jan 2007 08:24:00 -0000 User-Agent: KMail/1.9.1 MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_PextFiYAhNmj6fy" Message-Id: <200701241123.43649.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-01/txt/msg00494.txt.bz2 --Boundary-00=_PextFiYAhNmj6fy Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 2495 Now, when creating values corresponding to bitfields, gdb immediately fetches the values from the memory. That's not good if the memory is "read sensitive". The quick fix is just not to fetch the value, but in that case, if we have several bitfields sharing one byte, calling value_fetch_lazy on each bitfield will repeatedly read that byte -- again not good for read sensitive values. We've discussed this with Daniel and Jim, and the only workable solution appears to be adding a pointer from bitfield to the parent value. When calling value_fetch_lazy on a bitfield, the parent's content is obtained and necessary bits are extracted. If the parent is originally lazy, first access to its content fetches the bits from the memory. So, if we have 32-bit value in memory and some 10 bitfields inside it, first access to any bitfields will fetch the entire value from the memory, and accesses to other bitfields will just access already-read data. Until the first bitfield is accessed, no memory reads are performed at all. One slight problem here is if we have 'parent' pointer, we should make sure that the parent is not deleted while child still needs it. It turned out that a small bit of reference counting for parent pointers is sufficient -- no global changes in gdb are required. A patch that implements this idea is attached, no regressions on x86. One nit is that when writing a bitfield, we first fetch the entire parent value, modify bits and write entire parent value. In some *limited* set of cases it is possible to read only some of the bytes, or not read anything at all, but introducing bit-mask telling which bits are read and which are not is more trouble that its worth. Comments? - Volodya * value.c (struct value): New fields fieldno, parent and reference_count. (allocate_value): Initialize the parent and reference_count fields. (value_parent, value_fieldno): New functions. (value_free): New function. (value_copy): Copy the parent field, increase reference count for parent. (value_primitive_field): When accessing bitfield, not unconditionally fetch them. Don't copy lval type is already set inside the function. * value.h (value_parent, value_fieldno): Define. (value_free): Define as function, not as macro. * valops.c (value_fetch_lazy): Handle fetching of bitfields. (value_assign): Don't handle bitfields in lval_memory and lval_register values. Handle lval_bitfield only. * defs.h (enum lval_type): New enumerator lval_bitfield. --Boundary-00=_PextFiYAhNmj6fy Content-Type: text/x-diff; charset="us-ascii"; name="lazy_bitfields__gdb_mainline.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="lazy_bitfields__gdb_mainline.diff" Content-length: 10995 --- gdb/value.c (/patches/gdb/value_primitive_field/gdb_mainline) (revision 3210) +++ gdb/value.c (/patches/gdb/lazy_bitfields/gdb_mainline) (revision 3210) @@ -63,6 +63,10 @@ struct value struct internalvar *internalvar; } location; + /* For bitfields, this gives the index of + the field inside parent. */ + int fieldno; + /* Describes offset of a value within lval of a structure in bytes. If lval == lval_memory, this is an offset to the address. If lval == lval_register, this is a further offset from @@ -129,6 +133,18 @@ struct value int embedded_offset; int pointed_to_offset; + /* Pointer to parent variable. This pointer is + set only when a value needs access to the value of the + parent, for example for bitfields. */ + struct value *parent; + + /* The number of references for this value. This counts + all references via the 'parent' pointer, and either + 0 or 1 depending on whether there's reference to this + value from other gdb code. References from that 'other code' + are managed using explicit calls to value_free and release_value. */ + int reference_count; + /* Values are stored in a chain, so that they can be deleted easily over calls to the inferior. Values assigned to internal variables or put into the value history are taken off this @@ -233,6 +249,8 @@ allocate_value (struct type *type) val->embedded_offset = 0; val->pointed_to_offset = 0; val->modifiable = 1; + val->parent = NULL; + val->reference_count = 1; return val; } @@ -432,6 +450,16 @@ deprecated_value_internalvar_hack (struc return &value->location.internalvar; } +extern struct value *value_parent (struct value *value) +{ + return value->parent; +} + +extern int value_fieldno (struct value *value) +{ + return value->fieldno; +} + struct frame_id * deprecated_value_frame_id_hack (struct value *value) { @@ -540,6 +568,17 @@ value_release_to_mark (struct value *mar return val; } +extern void value_free (struct value *value) +{ + /* If there's a parent, drop parents's reference count + and free it if necessary. */ + if (value->parent) + value_free (value->parent); + + if (--value->reference_count == 0) + xfree (value); +} + /* Return a copy of the value ARG. It contains the same contents, for same memory address, but it's a different block of storage. */ @@ -562,6 +601,11 @@ value_copy (struct value *arg) val->embedded_offset = value_embedded_offset (arg); val->pointed_to_offset = arg->pointed_to_offset; val->modifiable = arg->modifiable; + val->fieldno = arg->fieldno; + val->parent = arg->parent; + if (val->parent) + ++val->parent->reference_count; + val->reference_count = 1; if (!value_lazy (val)) { memcpy (value_contents_all_raw (val), value_contents_all_raw (arg), @@ -1300,15 +1344,23 @@ value_primitive_field (struct value *arg if (TYPE_FIELD_BITSIZE (arg_type, fieldno)) { - v = value_from_longest (type, - unpack_field_as_long (arg_type, - value_contents (arg1) - + offset, - fieldno)); + LONGEST num; + int len; + + + v = allocate_value (type); v->bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno) % 8; v->bitsize = TYPE_FIELD_BITSIZE (arg_type, fieldno); v->offset = value_offset (arg1) + offset + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8; + v->parent = arg1; + v->lval = lval_bitfield; + v->fieldno = fieldno; + + if (!value_lazy (arg1)) + value_fetch_lazy (v); + else + set_value_lazy (v, 1); } else if (fieldno < TYPE_N_BASECLASSES (arg_type)) { @@ -1340,7 +1392,9 @@ value_primitive_field (struct value *arg v->offset = (value_offset (arg1) + offset + value_embedded_offset (arg1)); } - VALUE_LVAL (v) = VALUE_LVAL (arg1); + + if (v->lval == not_lval) + VALUE_LVAL (v) = VALUE_LVAL (arg1); if (VALUE_LVAL (arg1) == lval_internalvar) VALUE_LVAL (v) = lval_internalvar_component; v->location = arg1->location; --- gdb/value.h (/patches/gdb/value_primitive_field/gdb_mainline) (revision 3210) +++ gdb/value.h (/patches/gdb/lazy_bitfields/gdb_mainline) (revision 3210) @@ -211,6 +211,10 @@ extern CORE_ADDR *deprecated_value_addre extern struct internalvar **deprecated_value_internalvar_hack (struct value *); #define VALUE_INTERNALVAR(val) (*deprecated_value_internalvar_hack (val)) +extern struct value *value_parent (struct value *v); + +extern int value_fieldno (struct value *v); + /* Frame register value is relative to. This will be described in the lval enum above as "lval_register". */ extern struct frame_id *deprecated_value_frame_id_hack (struct value *); @@ -458,7 +462,7 @@ extern int unop_user_defined_p (enum exp extern int destructor_name_p (const char *name, const struct type *type); -#define value_free(val) xfree (val) +extern void value_free (struct value *value); extern void free_all_values (void); --- gdb/valops.c (/patches/gdb/value_primitive_field/gdb_mainline) (revision 3210) +++ gdb/valops.c (/patches/gdb/lazy_bitfields/gdb_mainline) (revision 3210) @@ -541,13 +541,30 @@ value_at_lazy (struct type *type, CORE_A int value_fetch_lazy (struct value *val) { - CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val); - int length = TYPE_LENGTH (value_enclosing_type (val)); - struct type *type = value_type (val); - if (length) - read_memory (addr, value_contents_all_raw (val), length); + if (VALUE_LVAL (val) == lval_bitfield) + { + LONGEST num; + int len; + num = unpack_field_as_long (value_type (value_parent (val)), + value_contents (value_parent (val)), + value_fieldno (val)); + + type = check_typedef (type); + len = TYPE_LENGTH (type); + + store_signed_integer (value_contents_raw (val), len, num); + } + else + { + CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val); + int length = TYPE_LENGTH (value_enclosing_type (val)); + + if (length) + read_memory (addr, value_contents_all_raw (val), length); + } + set_value_lazy (val, 0); return 0; } @@ -600,37 +617,15 @@ value_assign (struct value *toval, struc case lval_memory: { + int changed_len; const gdb_byte *dest_buffer; CORE_ADDR changed_addr; - int changed_len; gdb_byte buffer[sizeof (LONGEST)]; - if (value_bitsize (toval)) - { - /* We assume that the argument to read_memory is in units of - host chars. FIXME: Is that correct? */ - changed_len = (value_bitpos (toval) - + value_bitsize (toval) - + HOST_CHAR_BIT - 1) - / HOST_CHAR_BIT; - - if (changed_len > (int) sizeof (LONGEST)) - error (_("Can't handle bitfields which don't fit in a %d bit word."), - (int) sizeof (LONGEST) * HOST_CHAR_BIT); - - read_memory (VALUE_ADDRESS (toval) + value_offset (toval), - buffer, changed_len); - modify_field (buffer, value_as_long (fromval), - value_bitpos (toval), value_bitsize (toval)); - changed_addr = VALUE_ADDRESS (toval) + value_offset (toval); - dest_buffer = buffer; - } - else - { - changed_addr = VALUE_ADDRESS (toval) + value_offset (toval); - changed_len = TYPE_LENGTH (type); - dest_buffer = value_contents (fromval); - } + gdb_assert (!value_bitsize (toval)); + changed_addr = VALUE_ADDRESS (toval) + value_offset (toval); + changed_len = TYPE_LENGTH (type); + dest_buffer = value_contents (fromval); write_memory (changed_addr, dest_buffer, changed_len); if (deprecated_memory_changed_hook) @@ -659,38 +654,11 @@ value_assign (struct value *toval, struc } else { - if (value_bitsize (toval)) - { - int changed_len; - gdb_byte buffer[sizeof (LONGEST)]; - - changed_len = (value_bitpos (toval) - + value_bitsize (toval) - + HOST_CHAR_BIT - 1) - / HOST_CHAR_BIT; - - if (changed_len > (int) sizeof (LONGEST)) - error (_("Can't handle bitfields which don't fit in a %d bit word."), - (int) sizeof (LONGEST) * HOST_CHAR_BIT); - - get_frame_register_bytes (frame, value_reg, - value_offset (toval), - changed_len, buffer); - - modify_field (buffer, value_as_long (fromval), - value_bitpos (toval), value_bitsize (toval)); - - put_frame_register_bytes (frame, value_reg, - value_offset (toval), - changed_len, buffer); - } - else - { - put_frame_register_bytes (frame, value_reg, - value_offset (toval), - TYPE_LENGTH (type), - value_contents (fromval)); - } + gdb_assert (!value_bitsize (toval)); + put_frame_register_bytes (frame, value_reg, + value_offset (toval), + TYPE_LENGTH (type), + value_contents (fromval)); } if (deprecated_register_changed_hook) @@ -698,6 +666,43 @@ value_assign (struct value *toval, struc observer_notify_target_changed (¤t_target); break; } + + case lval_bitfield: + { + int changed_len; + gdb_byte *content; + + /* We assume that the argument to read_memory is in units of + host chars. FIXME: Is that correct? */ + changed_len = (value_bitpos (toval) + + value_bitsize (toval) + + HOST_CHAR_BIT - 1) + / HOST_CHAR_BIT; + + if (changed_len > (int) sizeof (LONGEST)) + error (_("Can't handle bitfields which don't fit in a %d bit word."), + (int) sizeof (LONGEST) * HOST_CHAR_BIT); + + content = value_contents_writeable (value_parent (toval)) + + value_offset (toval); + modify_field (content, value_as_long (fromval), + value_bitpos (toval), value_bitsize (toval)); + + if (VALUE_LVAL (value_parent (toval)) == lval_register) + { + struct frame_info *frame = + frame_find_by_id (VALUE_FRAME_ID (toval)); + put_frame_register_bytes (frame, VALUE_REGNUM (toval), + value_offset (toval), + changed_len, content); + } + else + write_memory (VALUE_ADDRESS (value_parent (toval)) + + value_offset (toval), + content, changed_len); + + break; + } default: error (_("Left operand of assignment is not an lvalue.")); --- gdb/defs.h (/patches/gdb/value_primitive_field/gdb_mainline) (revision 3210) +++ gdb/defs.h (/patches/gdb/lazy_bitfields/gdb_mainline) (revision 3210) @@ -671,7 +671,9 @@ enum lval_type /* In a gdb internal variable. */ lval_internalvar, /* Part of a gdb internal variable (structure field). */ - lval_internalvar_component + lval_internalvar_component, + /* Bitfield of a structure. */ + lval_bitfield }; /* Control types for commands */ Property changes on: ___________________________________________________________________ Name: csl:base +/all/patches/gdb/value_primitive_field/gdb_mainline Name: svk:merge +d48a11ec-ee1c-0410-b3f5-c20844f99675:/work/gdb/gdb_mainline:2531 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:160955 --Boundary-00=_PextFiYAhNmj6fy--