[PATCH 1/9] Add test for modifiable DWARF locations

[PATCH 1/9] Add test for modifiable DWARF locations

[Andreas Arnez]

This adds a test for read/write access to variables with various types of
DWARF locations.  It uses register- and memory locations and composite
locations with register- and memory pieces.

Since the new test calls gdb_test_no_output with commands that contain
braces, it is necessary for string_to_regexp to quote braces as well.
This was not done before.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-access.c: New file.
	* gdb.dwarf2/var-access.exp: New test.
	* lib/gdb-utils.exp (string_to_regexp): Quote braces as well.
---
 gdb/testsuite/gdb.dwarf2/var-access.c   |  25 +++++
 gdb/testsuite/gdb.dwarf2/var-access.exp | 193 ++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb-utils.exp         |   2 +-
 3 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.exp

diff --git a/gdb/testsuite/gdb.dwarf2/var-access.c b/gdb/testsuite/gdb.dwarf2/var-access.c
new file mode 100644
index 0000000..108be82
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/var-access.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+char buf[] = {0, 1, 2, 3, 4, 5, 6, 7};
+
+int
+main (void)
+{
+  asm volatile ("main_label: .globl main_label");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
new file mode 100644
index 0000000..ee93b93
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -0,0 +1,193 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test reading/writing variables with non-trivial DWARF locations.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+# Choose suitable integer registers for the test.
+
+set dwarf_regnum {0 1}
+
+if { [istarget "aarch64*-*-*"] } {
+    set regname {x0 x1}
+} elseif { [istarget "arm*-*-*"]
+	   || [istarget "s390*-*-*" ]
+	   || [istarget "powerpc*-*-*"]
+	   || [istarget "rs6000*-*-aix*"] } {
+    set regname {r0 r1}
+} elseif { [istarget "i?86-*-*"] } {
+    set regname {eax edx}
+} elseif { [istarget "x86_64-*-*"] } {
+    set regname {rax rdx}
+} else {
+    verbose "Skipping tests for accessing DWARF-described variables."
+    return
+}
+
+standard_testfile .c ${gdb_test_file_name}-dw.S
+
+# Make some DWARF for the test.
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    global dwarf_regnum regname
+
+    set main_func \
+	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
+    set buf_var [gdb_target_symbol buf]
+
+    cu {} {
+	DW_TAG_compile_unit {
+		{DW_AT_name var-pieces-dw.c}
+		{DW_AT_comp_dir /tmp}
+	} {
+	    declare_labels char_type_label
+	    declare_labels int_type_label short_type_label
+	    declare_labels array_a8_label struct_s_label
+
+	    char_type_label: base_type {
+		{name "char"}
+		{encoding @DW_ATE_unsigned_char}
+		{byte_size 1 DW_FORM_sdata}
+	    }
+
+	    int_type_label: base_type {
+		{name "int"}
+		{encoding @DW_ATE_signed}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    array_a8_label: array_type {
+		{type :$char_type_label}
+	    } {
+		subrange_type {
+		    {type :$int_type_label}
+		    {upper_bound 7 DW_FORM_udata}
+		}
+	    }
+
+	    struct_s_label: structure_type {
+		{name "s"}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name "a"}
+		    {type :$char_type_label}
+		    {data_member_location 0 DW_FORM_udata}
+		}
+		member {
+		    {name "b"}
+		    {type :$char_type_label}
+		    {data_member_location 1 DW_FORM_udata}
+		}
+		member {
+		    {name "c"}
+		    {type :$char_type_label}
+		    {data_member_location 2 DW_FORM_udata}
+		}
+		member {
+		    {name "d"}
+		    {type :$char_type_label}
+		    {data_member_location 3 DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_subprogram {
+		{name "main"}
+		{DW_AT_external 1 flag}
+		{low_pc [lindex $main_func 0] DW_FORM_addr}
+		{high_pc [lindex $main_func 1] DW_FORM_udata}
+	    } {
+		# Simple memory location.
+		DW_TAG_variable {
+		    {name "a"}
+		    {type :$array_a8_label}
+		    {location {
+			addr $buf_var
+		    } SPECIAL_expr}
+		}
+		# Memory pieces.
+		DW_TAG_variable {
+		    {name "s1"}
+		    {type :$struct_s_label}
+		    {location {
+			addr $buf_var
+			plus_uconst 2
+			piece 2
+			addr $buf_var
+			piece 2
+		    } SPECIAL_expr}
+		}
+		# Register- and memory pieces.
+		DW_TAG_variable {
+		    {name "s2"}
+		    {type :$struct_s_label}
+		    {location {
+			regx [lindex $dwarf_regnum 0]
+			piece 1
+			addr "$buf_var + 4"
+			piece 1
+			regx [lindex $dwarf_regnum 1]
+			piece 1
+			addr "$buf_var + 5"
+			piece 1
+		    } SPECIAL_expr}
+		}
+	    }
+	}
+    }
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Byte-aligned memory pieces.
+gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
+    "s1 == re-ordered buf"
+gdb_test_no_output "set var s1.a = 63"
+gdb_test "print/d s1" " = \\{a = 63, b = 3, c = 0, d = 1\\}" \
+    "verify s1.a"
+gdb_test "print/d a" " = \\{0, 1, 63, 3, 4, 5, 6, 7\\}" \
+    "verify s1.a through a"
+
+# Byte-aligned register- and memory pieces.
+gdb_test_no_output "set var \$[lindex $regname 0] = 81" \
+    "init reg for s2.a"
+gdb_test_no_output "set var \$[lindex $regname 1] = 28" \
+    "init reg for s2.c"
+gdb_test "print/d s2" " = \\{a = 81, b = 4, c = 28, d = 5\\}" \
+    "initialized s2 from mem and regs"
+gdb_test_no_output "set var s2.c += s2.a + s2.b - s2.d"
+gdb_test "print/d s2" " = \\{a = 81, b = 4, c = 108, d = 5\\}" \
+    "verify s2.c"
+gdb_test "print/d \$[lindex $regname 1]" " = 108" \
+    "verify s2.c through reg"
+gdb_test_no_output "set var s2 = {191, 73, 231, 123}" \
+    "re-initialize s2"
+gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
+    "verify re-initialized s2"
diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index ff1b24a..37abb14 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -34,6 +34,6 @@ proc gdb_init_commands {} {
 
 proc string_to_regexp {str} {
     set result $str
-    regsub -all {[]*+.|()^$\[\\]} $str {\\&} result
+    regsub -all {[]*+.|(){}^$\[\\]} $str {\\&} result
     return $result
 }
-- 
2.3.0

[PATCH 0/9] Various DWARF piece fixes

[Andreas Arnez]

This patch series follows up on the proposed fix for PR21226:

  https://sourceware.org/ml/gdb-patches/2017-03/msg00110.html

The fix for that issue is now provided by patch #3.  More fixes for
various other DWARF piece handling issues are provided by patches #2, #5,
#6, and #8.  Patch #1 establishes a new test case that is extended by some
of the other patches.  Patches #4, #7, and #9 perform some clean-up.

Andreas Arnez (9):
  Add test for modifiable DWARF locations
  Fix size capping in write_pieced_value
  PR gdb/21226: Take DWARF stack value pieces from LSB end
  Remove addr_size field from struct piece_closure
  Fix issues in write_pieced_value when targeting bit-fields
  Fix handling of DWARF register pieces on big-endian targets
  Improve logic for buffer allocation in read/write_pieced_value
  Respect piece offset for DW_OP_bit_piece
  Remove unnecessary copies of variables in read/write_pieced_value

 gdb/dwarf2loc.c                            | 294 +++++++++++++--------------
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp |  21 +-
 gdb/testsuite/gdb.dwarf2/var-access.c      |  25 +++
 gdb/testsuite/gdb.dwarf2/var-access.exp    | 315 +++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb-utils.exp            |   2 +-
 5 files changed, 503 insertions(+), 154 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.exp

-- 
2.3.0

[PATCH 2/9] Fix size capping in write_pieced_value

[Andreas Arnez]

A field in a structure composed of DWARF pieces overlaps one or more of
those pieces.  When writing to the field, the beginning of the first and
the end of the last of those pieces may have to be skipped.  But the
logic in write_pieced_value for handling this is flawed when there are
actually bits to skip at the beginning of the first piece: it truncates
the piece size towards the end *before* accounting for the skipped bits
at the beginning instead of the other way around.

Note that the same bug was already found in read_pieced_value and fixed
there (but not in write_pieced_value), see PR 15391.

This patch swaps the calculations, bringing them into the same (correct)
order as in read_pieced_value.

gdb/ChangeLog:

	* dwarf2loc.c (write_pieced_value): Fix order of calculations for
	size capping.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-pieces.exp: Add test case for modifying a
	variable at nonzero offset.
---
 gdb/dwarf2loc.c                         | 4 ++--
 gdb/testsuite/gdb.dwarf2/var-access.exp | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 93c45a7..496400a 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1963,8 +1963,6 @@ write_pieced_value (struct value *to, struct value *from)
 	  bits_to_skip -= this_size_bits;
 	  continue;
 	}
-      if (this_size_bits > type_len - offset)
-	this_size_bits = type_len - offset;
       if (bits_to_skip > 0)
 	{
 	  dest_offset_bits = bits_to_skip;
@@ -1977,6 +1975,8 @@ write_pieced_value (struct value *to, struct value *from)
 	  dest_offset_bits = 0;
 	  source_offset_bits = offset;
 	}
+      if (this_size_bits > type_len - offset)
+	this_size_bits = type_len - offset;
 
       this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
       source_offset = source_offset_bits / 8;
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index ee93b93..56a635a 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -174,6 +174,11 @@ gdb_test "print/d s1" " = \\{a = 63, b = 3, c = 0, d = 1\\}" \
     "verify s1.a"
 gdb_test "print/d a" " = \\{0, 1, 63, 3, 4, 5, 6, 7\\}" \
     "verify s1.a through a"
+gdb_test_no_output "set var s1.b = 42"
+gdb_test "print/d s1" " = \\{a = 63, b = 42, c = 0, d = 1\\}" \
+    "verify s1.b"
+gdb_test "print/d a" " = \\{0, 1, 63, 42, 4, 5, 6, 7\\}" \
+    "verify s1.b through a"
 
 # Byte-aligned register- and memory pieces.
 gdb_test_no_output "set var \$[lindex $regname 0] = 81" \
-- 
2.3.0

[PATCH 4/9] Remove addr_size field from struct piece_closure

[Andreas Arnez]

The addr_size field in the piece_closure data structure is a relic from
before introducing the typed DWARF stack.  It is obsolete now.  This patch
removes it.

gdb/ChangeLog:

	* dwarf2loc.c (struct piece_closure) <addr_size>: Remove field.
	(allocate_piece_closure): Drop addr_size parameter.
	(dwarf2_evaluate_loc_desc_full): Adjust call to
	allocate_piece_closure.
---
 gdb/dwarf2loc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 09938c4..76a58a3 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1485,9 +1485,6 @@ struct piece_closure
   /* The number of pieces used to describe this variable.  */
   int n_pieces;
 
-  /* The target address size, used only for DWARF_VALUE_STACK.  */
-  int addr_size;
-
   /* The pieces themselves.  */
   struct dwarf_expr_piece *pieces;
 
@@ -1502,7 +1499,7 @@ struct piece_closure
 static struct piece_closure *
 allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
 			int n_pieces, struct dwarf_expr_piece *pieces,
-			int addr_size, struct frame_info *frame)
+			struct frame_info *frame)
 {
   struct piece_closure *c = XCNEW (struct piece_closure);
   int i;
@@ -1510,7 +1507,6 @@ allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
   c->refc = 1;
   c->per_cu = per_cu;
   c->n_pieces = n_pieces;
-  c->addr_size = addr_size;
   c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
   if (frame == NULL)
     c->frame_id = null_frame_id;
@@ -2412,7 +2408,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	invalid_synthetic_pointer ();
 
       c = allocate_piece_closure (per_cu, ctx.num_pieces, ctx.pieces,
-				  ctx.addr_size, frame);
+				  frame);
       /* We must clean up the value chain after creating the piece
 	 closure but before allocating the result.  */
       free_values.free_to_mark ();
-- 
2.3.0

[PATCH 3/9] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value, the
existing logic always takes the piece from the lowest-addressed end, which
is wrong on big-endian targets.  The DWARF standard states that the
"DW_OP_bit_piece operation describes a sequence of bits using the least
significant bits of that value", and this also matches the current logic
in GCC.  For instance, the GCC guality test case pr54970.c fails on s390x
because of this.

This fix adjusts the piece accordingly on big-endian targets.  It is
assumed that:

* DW_OP_piece shall take the piece from the LSB end as well;

* pieces reaching outside the stack value bits are considered undefined,
  and a zero value can be used instead.

gdb/ChangeLog:

	PR gdb/21226
	* dwarf2loc.c (read_pieced_value): Anchor stack value pieces at
	the LSB end, independent of endianness.

gdb/testsuite/ChangeLog:

	PR gdb/21226
	* gdb.dwarf2/nonvar-access.exp: Add checks for verifying that
	stack value pieces are taken from the LSB end.
---
 gdb/dwarf2loc.c                            | 43 ++++++++++++++++--------------
 gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 21 ++++++++++++---
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 496400a..09938c4 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1857,6 +1857,10 @@ read_pieced_value (struct value *v)
 		if (unavail)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
 	      }
+
+	    copy_bitwise (contents, dest_offset_bits,
+			  intermediate_buffer, source_offset_bits % 8,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1865,26 +1869,28 @@ read_pieced_value (struct value *v)
 			     p->v.mem.in_stack_memory,
 			     p->v.mem.addr + source_offset,
 			     buffer.data (), this_size);
+	  copy_bitwise (contents, dest_offset_bits,
+			intermediate_buffer, source_offset_bits % 8,
+			this_size_bits, bits_big_endian);
 	  break;
 
 	case DWARF_VALUE_STACK:
 	  {
-	    size_t n = this_size;
+	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
+	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
+	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
 
-	    if (n > c->addr_size - source_offset)
-	      n = (c->addr_size >= source_offset
-		   ? c->addr_size - source_offset
-		   : 0);
-	    if (n == 0)
-	      {
-		/* Nothing.  */
-	      }
-	    else
-	      {
-		const gdb_byte *val_bytes = value_contents_all (p->v.value);
+	    /* Use zeroes if piece reaches beyond stack value.  */
+	    if (p->size > obj_size)
+	      break;
 
-		intermediate_buffer = val_bytes + source_offset;
-	      }
+	    /* Piece is anchored at least significant bit end.  */
+	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
+	      source_offset_bits += obj_size - p->size;
+	    copy_bitwise (contents, dest_offset_bits,
+			  value_contents_all (p->v.value),
+			  source_offset_bits,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1898,6 +1904,9 @@ read_pieced_value (struct value *v)
 		   : 0);
 	    if (n != 0)
 	      intermediate_buffer = p->v.literal.data + source_offset;
+	    copy_bitwise (contents, dest_offset_bits,
+			  intermediate_buffer, source_offset_bits % 8,
+			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
@@ -1914,12 +1923,6 @@ read_pieced_value (struct value *v)
 	  internal_error (__FILE__, __LINE__, _("invalid location type"));
 	}
 
-      if (p->location != DWARF_VALUE_OPTIMIZED_OUT
-	  && p->location != DWARF_VALUE_IMPLICIT_POINTER)
-	copy_bitwise (contents, dest_offset_bits,
-		      intermediate_buffer, source_offset_bits % 8,
-		      this_size_bits, bits_big_endian);
-
       offset += this_size_bits;
     }
 }
diff --git a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
index 99f63cc..5406029 100644
--- a/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/nonvar-access.exp
@@ -128,14 +128,26 @@ Dwarf::assemble $asm_file {
 		    {name def_t}
 		    {type :$struct_t_label}
 		    {location {
-			const1u 0
+			const2s -184
 			stack_value
 			bit_piece 9 0
-			const1s -1
+			const4u 1752286
 			stack_value
 			bit_piece 23 0
 		    } SPECIAL_expr}
 		}
+		# Composite location with some empty pieces.
+		DW_TAG_variable {
+		    {name part_def_a}
+		    {type :$array_a9_label}
+		    {location {
+			piece 3
+			const4u 0xf1927314
+			stack_value
+			piece 4
+			piece 2
+		    } SPECIAL_expr}
+		}
 		# Implicit location: immediate value.
 		DW_TAG_variable {
 		    {name def_implicit_s}
@@ -221,9 +233,12 @@ gdb_test "print/x *implicit_b_ptr" " = $val"
 
 # Byte-aligned fields, pieced together from DWARF stack values.
 gdb_test "print def_s" " = \\{a = 0, b = -1\\}"
+switch $endian { big {set val 0x92} little {set val 0x73} }
+gdb_test "print/x part_def_a\[4\]" " = $val"
+gdb_test "print/x part_def_a\[8\]" " = <optimized out>"
 
 # Non-byte-aligned fields, pieced together from DWARF stack values.
-gdb_test "print def_t" " = \\{a = 0, b = -1\\}"
+gdb_test "print def_t" " = \\{a = -184, b = 1752286\\}"
 
 # Simple variable without location.
 gdb_test "print undef_int" " = <optimized out>"
-- 
2.3.0

[PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

[Andreas Arnez]

There are multiple issues in write_pieced_value when dealing with a
bit-field as the target value:

(1) The number of bits preceding the bit-field is calculated without
    considering the relative offset of the value's parent.

(2) On big-endian targets the source value's *most* significant bits are
    transferred to the target value, instead of its least significant
    bits.

(3) The number of bytes containing a portion of the bit-field in a given
    piece is calculated with the wrong starting offset; thus the result
    may be off by one.

(4) When checking whether the data can be transferred byte-wise, the
    transfer size is not verified to be byte-aligned.

(5) When transferring the data via a buffer, the bit offset within the
    target value is not reduced to its sub-byte fraction before using it
    as a bit offset into the buffer.

These issues are fixed.  For consistency, the affected logic that exists
in read_pieced_value as well is changed there in the same way.

gdb/ChangeLog:

	* dwarf2loc.c (write_pieced_value): Fix various issues with
	bit-field handling.
	(read_pieced_value): Sync with changes in write_pieced_value.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-access.exp: Add test for accessing bit-fields
	whose containing structure is located in several DWARF pieces.
---
 gdb/dwarf2loc.c                         | 54 +++++++++++++++------------------
 gdb/testsuite/gdb.dwarf2/var-access.exp | 50 +++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 30 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 76a58a3..1f89a08 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1775,7 +1775,8 @@ read_pieced_value (struct value *v)
   bits_to_skip = 8 * value_offset (v);
   if (value_bitsize (v))
     {
-      bits_to_skip += value_bitpos (v);
+      bits_to_skip += (8 * value_offset (value_parent (v))
+		       + value_bitpos (v));
       type_len = value_bitsize (v);
     }
   else
@@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
 	  bits_to_skip -= this_size_bits;
 	  continue;
 	}
-      if (bits_to_skip > 0)
-	{
-	  dest_offset_bits = 0;
-	  source_offset_bits = bits_to_skip;
-	  this_size_bits -= bits_to_skip;
-	  bits_to_skip = 0;
-	}
-      else
-	{
-	  dest_offset_bits = offset;
-	  source_offset_bits = 0;
-	}
+      source_offset_bits = bits_to_skip;
+      this_size_bits -= bits_to_skip;
+      bits_to_skip = 0;
+      dest_offset_bits = offset;
+
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
@@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct value *from)
   bits_to_skip = 8 * value_offset (to);
   if (value_bitsize (to))
     {
-      bits_to_skip += value_bitpos (to);
+      bits_to_skip += (8 * value_offset (value_parent (to))
+		       + value_bitpos (to));
       type_len = value_bitsize (to);
+      /* Use the least significant bits of FROM.  */
+      if (gdbarch_byte_order (get_type_arch (value_type (from)))
+	  == BFD_ENDIAN_BIG)
+	{
+	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
+	  type_len += offset;
+	}
     }
   else
     type_len = 8 * TYPE_LENGTH (value_type (to));
@@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct value *from)
 	  bits_to_skip -= this_size_bits;
 	  continue;
 	}
-      if (bits_to_skip > 0)
-	{
-	  dest_offset_bits = bits_to_skip;
-	  source_offset_bits = 0;
-	  this_size_bits -= bits_to_skip;
-	  bits_to_skip = 0;
-	}
-      else
-	{
-	  dest_offset_bits = 0;
-	  source_offset_bits = offset;
-	}
+      dest_offset_bits = bits_to_skip;
+      this_size_bits -= bits_to_skip;
+      bits_to_skip = 0;
+      source_offset_bits = offset;
+
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
+      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
       source_offset = source_offset_bits / 8;
       dest_offset = dest_offset_bits / 8;
-      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0)
+      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
+	  && this_size_bits % 8 == 0)
 	{
 	  source_buffer = contents + source_offset;
 	  need_bitwise = 0;
@@ -2049,7 +2045,7 @@ write_pieced_value (struct value *to, struct value *from)
 	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
 	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
 			   &buffer[this_size - 1], 1);
-	      copy_bitwise (buffer.data (), dest_offset_bits,
+	      copy_bitwise (buffer.data (), dest_offset_bits % 8,
 			    contents, source_offset_bits,
 			    this_size_bits,
 			    bits_big_endian);
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index 56a635a..c6abc87 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -62,7 +62,7 @@ Dwarf::assemble $asm_file {
 	} {
 	    declare_labels char_type_label
 	    declare_labels int_type_label short_type_label
-	    declare_labels array_a8_label struct_s_label
+	    declare_labels array_a8_label struct_s_label struct_t_label
 
 	    char_type_label: base_type {
 		{name "char"}
@@ -111,6 +111,34 @@ Dwarf::assemble $asm_file {
 		}
 	    }
 
+	    struct_t_label: structure_type {
+		{name "t"}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		member {
+		    {name u}
+		    {type :$int_type_label}
+		}
+		member {
+		    {name x}
+		    {type :$int_type_label}
+		    {data_member_location 4 DW_FORM_udata}
+		    {bit_size 9 DW_FORM_udata}
+		}
+		member {
+		    {name y}
+		    {type :$int_type_label}
+		    {data_bit_offset 41 DW_FORM_udata}
+		    {bit_size 13 DW_FORM_udata}
+		}
+		member {
+		    {name z}
+		    {type :$int_type_label}
+		    {data_bit_offset 54 DW_FORM_udata}
+		    {bit_size 10 DW_FORM_udata}
+		}
+	    }
+
 	    DW_TAG_subprogram {
 		{name "main"}
 		{DW_AT_external 1 flag}
@@ -152,6 +180,18 @@ Dwarf::assemble $asm_file {
 			piece 1
 		    } SPECIAL_expr}
 		}
+		# Memory pieces for bitfield access.
+		DW_TAG_variable {
+		    {name "t1"}
+		    {type :$struct_t_label}
+		    {location {
+			piece 4
+			addr "$buf_var + 1"
+			piece 3
+			addr "$buf_var"
+			piece 1
+		    } SPECIAL_expr}
+		}
 	    }
 	}
     }
@@ -196,3 +236,11 @@ gdb_test_no_output "set var s2 = {191, 73, 231, 123}" \
     "re-initialize s2"
 gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
     "verify re-initialized s2"
+
+# Unaligned bitfield access through byte-aligned pieces.
+gdb_test_no_output "set var t1.x = -7"
+gdb_test_no_output "set var t1.z = 340"
+gdb_test_no_output "set var t1.y = 1234"
+gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 340\\}" \
+    "verify t1"
+
-- 
2.3.0

[PATCH 7/9] Improve logic for buffer allocation in read/write_pieced_value

[Andreas Arnez]

So far the main loop in read_pieced_value and write_pieced_value is
structured like this:

(1) Prepare a buffer and some variables we may need;

(2) depending on the DWARF piece type to be handled, use the buffer and
    the prepared variables, ignore them, or even recalculate them.

This approach reduces readability and may also lead to unnecessary copying
of data.  This patch moves the preparations to the places where sufficient
information is available and removes some of the variables involved.

gdb/ChangeLog:

	* dwarf2loc.c (read_pieced_value): Move the buffer allocation and
	some other preparations to the places where sufficient information
	is available.
	(write_pieced_value): Likewise.
---
 gdb/dwarf2loc.c | 111 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 56 insertions(+), 55 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index d13c0c5..67df598 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1794,8 +1794,7 @@ read_pieced_value (struct value *v)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
       size_t this_size, this_size_bits;
-      long dest_offset_bits, source_offset_bits, source_offset;
-      const gdb_byte *intermediate_buffer;
+      long dest_offset_bits, source_offset_bits;
 
       /* Compute size, source, and destination offsets for copying, in
 	 bits.  */
@@ -1813,11 +1812,6 @@ read_pieced_value (struct value *v)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
-      buffer.reserve (this_size);
-      source_offset = source_offset_bits / 8;
-      intermediate_buffer = buffer.data ();
-
       /* Copy from the source to DEST_BUFFER.  */
       switch (p->location)
 	{
@@ -1843,13 +1837,11 @@ read_pieced_value (struct value *v)
 					  this_size, buffer.data (),
 					  &optim, &unavail))
 	      {
-		/* Just so garbage doesn't ever shine through.  */
-		memset (buffer.data (), 0, this_size);
-
 		if (optim)
 		  mark_value_bits_optimized_out (v, offset, this_size_bits);
 		if (unavail)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
+		break;
 	      }
 	    copy_bitwise (contents, dest_offset_bits,
 			  buffer.data (), source_offset_bits % 8,
@@ -1858,12 +1850,15 @@ read_pieced_value (struct value *v)
 	  break;
 
 	case DWARF_VALUE_MEMORY:
+	  this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+	  buffer.reserve (this_size);
+
 	  read_value_memory (v, offset,
 			     p->v.mem.in_stack_memory,
-			     p->v.mem.addr + source_offset,
+			     p->v.mem.addr + source_offset_bits / 8,
 			     buffer.data (), this_size);
 	  copy_bitwise (contents, dest_offset_bits,
-			intermediate_buffer, source_offset_bits % 8,
+			buffer.data (), source_offset_bits % 8,
 			this_size_bits, bits_big_endian);
 	  break;
 
@@ -1889,17 +1884,18 @@ read_pieced_value (struct value *v)
 
 	case DWARF_VALUE_LITERAL:
 	  {
-	    size_t n = this_size;
-
-	    if (n > p->v.literal.length - source_offset)
-	      n = (p->v.literal.length >= source_offset
-		   ? p->v.literal.length - source_offset
-		   : 0);
-	    if (n != 0)
-	      intermediate_buffer = p->v.literal.data + source_offset;
+	    ULONGEST obj_size = 8 * p->v.literal.length;
+	    size_t n = this_size_bits;
+
+	    /* Cut off at the end of the implicit value.  */
+	    if (source_offset_bits >= obj_size)
+	      break;
+	    if (n > obj_size - source_offset_bits)
+	      n = obj_size - source_offset_bits;
+
 	    copy_bitwise (contents, dest_offset_bits,
-			  intermediate_buffer, source_offset_bits % 8,
-			  this_size_bits, bits_big_endian);
+			  p->v.literal.data, source_offset_bits,
+			  n, bits_big_endian);
 	  }
 	  break;
 
@@ -1956,9 +1952,7 @@ write_pieced_value (struct value *to, struct value *from)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
       size_t this_size_bits, this_size;
-      long dest_offset_bits, source_offset_bits, dest_offset, source_offset;
-      int need_bitwise;
-      const gdb_byte *source_buffer;
+      long dest_offset_bits, source_offset_bits;
 
       this_size_bits = p->size;
       if (bits_to_skip > 0 && bits_to_skip >= this_size_bits)
@@ -1974,22 +1968,6 @@ write_pieced_value (struct value *to, struct value *from)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
-      source_offset = source_offset_bits / 8;
-      dest_offset = dest_offset_bits / 8;
-      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
-	  && this_size_bits % 8 == 0)
-	{
-	  source_buffer = contents + source_offset;
-	  need_bitwise = 0;
-	}
-      else
-	{
-	  buffer.reserve (this_size);
-	  source_buffer = buffer.data ();
-	  need_bitwise = 1;
-	}
-
       switch (p->location)
 	{
 	case DWARF_VALUE_REGISTER:
@@ -2040,21 +2018,44 @@ write_pieced_value (struct value *to, struct value *from)
 	  }
 	  break;
 	case DWARF_VALUE_MEMORY:
-	  if (need_bitwise)
-	    {
-	      /* Only the first and last bytes can possibly have any
-		 bits reused.  */
-	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
-	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
-			   &buffer[this_size - 1], 1);
-	      copy_bitwise (buffer.data (), dest_offset_bits % 8,
-			    contents, source_offset_bits,
-			    this_size_bits,
-			    bits_big_endian);
-	    }
+	  {
+	    CORE_ADDR start_addr = p->v.mem.addr + dest_offset_bits / 8;
 
-	  write_memory (p->v.mem.addr + dest_offset,
-			source_buffer, this_size);
+	    if (dest_offset_bits % 8 == 0 && this_size_bits % 8 == 0
+		&& source_offset_bits % 8 == 0)
+	      {
+		/* Everything is byte-aligned; no buffer needed.  */
+		write_memory (start_addr,
+			      contents + source_offset_bits / 8,
+			      this_size_bits / 8);
+		break;
+	      }
+
+	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
+	    buffer.reserve (this_size);
+
+	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
+	      {
+		if (this_size <= 8)
+		  {
+		    /* Perform a single read for small sizes.  */
+		    read_memory (start_addr, buffer.data (), this_size);
+		  }
+		else
+		  {
+		    /* Only the first and last bytes can possibly have any
+		       bits reused.  */
+		    read_memory (start_addr, buffer.data (), 1);
+		    read_memory (start_addr + this_size - 1,
+				 &buffer[this_size - 1], 1);
+		  }
+	      }
+
+	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
+			  contents, source_offset_bits,
+			  this_size_bits, bits_big_endian);
+	    write_memory (start_addr, buffer.data (), this_size);
+	  }
 	  break;
 	default:
 	  mark_value_bytes_optimized_out (to, 0, TYPE_LENGTH (value_type (to)));
-- 
2.3.0

[PATCH 6/9] Fix handling of DWARF register pieces on big-endian targets

[Andreas Arnez]

For big-endian targets the logic in read/write_pieced_value tries to take
a register piece from the LSB end.  This requires offsets and sizes to be
adjusted accordingly, and that's where the current implementation has some
issues:

* The formulas for recalculating the bit- and byte-offsets into the
  register are wrong.  They just happen to yield correct results if
  everything is byte-aligned and the piece's last byte belongs to the
  given value.

* After recalculating the bit offset into the register, the number of
  bytes to be copied from the register is not recalculated.  Of course
  this does not matter if everything (particularly the piece size) is
  byte-aligned.

These issues are fixed.  The size calculation is performed with a new
helper function bits_to_bytes().  For simplification, the variable
buffer_size is dropped.

gdb/ChangeLog:

	* dwarf2loc.c (bits_to_bytes): New function.
	(read_pieced_value): Fix offset calculations for register pieces
	on big-endian targets.
	(write_pieced_value): Likewise.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-access.exp: Add test for non-byte-aligned
	register pieces.
---
 gdb/dwarf2loc.c                         | 74 +++++++++++++++++----------------
 gdb/testsuite/gdb.dwarf2/var-access.exp | 32 ++++++++++++++
 2 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 1f89a08..d13c0c5 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1751,6 +1751,15 @@ copy_bitwise_tests (void)
 
 #endif /* GDB_SELF_TEST */
 
+/* Return the number of bytes overlapping a contiguous chunk of N_BITS
+   bits whose first bit is located at bit offset START.  */
+
+static size_t
+bits_to_bytes (ULONGEST start, ULONGEST n_bits)
+{
+  return (start % 8 + n_bits + 7) / 8;
+}
+
 static void
 read_pieced_value (struct value *v)
 {
@@ -1761,7 +1770,6 @@ read_pieced_value (struct value *v)
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
   size_t type_len;
-  size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
@@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
+      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+      buffer.reserve (this_size);
       source_offset = source_offset_bits / 8;
-      if (buffer_size < this_size)
-	{
-	  buffer_size = this_size;
-	  buffer.reserve (buffer_size);
-	}
       intermediate_buffer = buffer.data ();
 
       /* Copy from the source to DEST_BUFFER.  */
@@ -1822,22 +1826,22 @@ read_pieced_value (struct value *v)
 	    struct frame_info *frame = frame_find_by_id (c->frame_id);
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
+	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 	    int optim, unavail;
-	    LONGEST reg_offset = source_offset;
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size < register_size (arch, gdb_regnum))
+		&& p->size < reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
-		/* We want the lower-order THIS_SIZE_BITS of the bytes
-		   we extract from the register.  */
-		source_offset_bits += 8 * this_size - this_size_bits;
+		source_offset_bits += reg_bits - p->size;
 	      }
+	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+	    buffer.reserve (this_size);
 
-	    if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
-					   this_size, buffer.data (),
-					   &optim, &unavail))
+	    if (!get_frame_register_bytes (frame, gdb_regnum,
+					  source_offset_bits / 8,
+					  this_size, buffer.data (),
+					  &optim, &unavail))
 	      {
 		/* Just so garbage doesn't ever shine through.  */
 		memset (buffer.data (), 0, this_size);
@@ -1847,9 +1851,8 @@ read_pieced_value (struct value *v)
 		if (unavail)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
 	      }
-
 	    copy_bitwise (contents, dest_offset_bits,
-			  intermediate_buffer, source_offset_bits % 8,
+			  buffer.data (), source_offset_bits % 8,
 			  this_size_bits, bits_big_endian);
 	  }
 	  break;
@@ -1927,7 +1930,6 @@ write_pieced_value (struct value *to, struct value *from)
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (to);
   size_t type_len;
-  size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
@@ -1972,7 +1974,7 @@ write_pieced_value (struct value *to, struct value *from)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
+      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
       source_offset = source_offset_bits / 8;
       dest_offset = dest_offset_bits / 8;
       if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
@@ -1983,11 +1985,7 @@ write_pieced_value (struct value *to, struct value *from)
 	}
       else
 	{
-	  if (buffer_size < this_size)
-	    {
-	      buffer_size = this_size;
-	      buffer.reserve (buffer_size);
-	    }
+	  buffer.reserve (this_size);
 	  source_buffer = buffer.data ();
 	  need_bitwise = 1;
 	}
@@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct value *from)
 	    struct frame_info *frame = frame_find_by_id (c->frame_id);
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
-	    int reg_offset = dest_offset;
+	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size <= register_size (arch, gdb_regnum))
+		&& p->size <= reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
+		dest_offset_bits += reg_bits - p->size;
 	      }
+	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
+	    buffer.reserve (this_size);
 
-	    if (need_bitwise)
+	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
 	      {
+		/* Need some bits from original register value.  */
 		int optim, unavail;
 
-		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
+		if (!get_frame_register_bytes (frame, gdb_regnum,
+					       dest_offset_bits / 8,
 					       this_size, buffer.data (),
 					       &optim, &unavail))
 		  {
@@ -2027,14 +2029,14 @@ write_pieced_value (struct value *to, struct value *from)
 				     "bitfield; containing word "
 				     "is unavailable"));
 		  }
-		copy_bitwise (buffer.data (), dest_offset_bits,
-			      contents, source_offset_bits,
-			      this_size_bits,
-			      bits_big_endian);
 	      }
 
-	    put_frame_register_bytes (frame, gdb_regnum, reg_offset, 
-				      this_size, source_buffer);
+	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
+			  contents, source_offset_bits,
+			  this_size_bits, bits_big_endian);
+	    put_frame_register_bytes (frame, gdb_regnum,
+				      dest_offset_bits / 8,
+				      this_size, buffer.data ());
 	  }
 	  break;
 	case DWARF_VALUE_MEMORY:
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index c6abc87..4787dfb 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -192,6 +192,18 @@ Dwarf::assemble $asm_file {
 			piece 1
 		    } SPECIAL_expr}
 		}
+		# Register pieces for bitfield access.
+		DW_TAG_variable {
+		    {name "t2"}
+		    {type :$struct_t_label}
+		    {location {
+			piece 4
+			regx [lindex $dwarf_regnum 0]
+			piece 3
+			regx [lindex $dwarf_regnum 1]
+			piece 1
+		    } SPECIAL_expr}
+		}
 	    }
 	}
     }
@@ -206,6 +218,15 @@ if ![runto_main] {
     return -1
 }
 
+# Determine endianness.
+set endian "little"
+gdb_test_multiple "show endian" "determine endianness" {
+    -re ".* (big|little) endian.*$gdb_prompt $" {
+	set endian $expect_out(1,string)
+	pass "endianness: $endian"
+    }
+}
+
 # Byte-aligned memory pieces.
 gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
     "s1 == re-ordered buf"
@@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234"
 gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 340\\}" \
     "verify t1"
 
+switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }
+gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
+    "init t2, first piece"
+gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
+    "init t2, second piece"
+gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z = 0\\}" \
+    "initialized t2 from regs"
+gdb_test_no_output "set var t2.y = 2641"
+gdb_test_no_output "set var t2.z = -400"
+gdb_test_no_output "set var t2.x = 200"
+gdb_test "print t2.x + t2.y + t2.z" " = 2441"
-- 
2.3.0

[PATCH 8/9] Respect piece offset for DW_OP_bit_piece

[Andreas Arnez]

So far GDB ignores the piece offset of all kinds of DWARF bit
pieces (DW_OP_bit_piece) and treats such pieces as if the offset was zero.

This is fixed, and an appropriate test is added.

gdb/ChangeLog:

	* dwarf2loc.c (read_pieced_value): Respect the piece offset, as
	given by DW_OP_bit_piece.
	(write_pieced_value): Likewise.

  Andreas Arnez  <arnez@linux.vnet.ibm.com>

	* gdb.dwarf2/var-access.exp: Add test for composite location with
	nonzero piece offsets.
---
 gdb/dwarf2loc.c                         | 22 ++++++++++++++------
 gdb/testsuite/gdb.dwarf2/var-access.exp | 37 +++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 67df598..045c2d3 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1824,11 +1824,13 @@ read_pieced_value (struct value *v)
 	    int optim, unavail;
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& p->size < reg_bits)
+		&& p->offset + p->size < reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		source_offset_bits += reg_bits - p->size;
+		source_offset_bits += reg_bits - (p->offset + p->size);
 	      }
+	    else
+	      source_offset_bits += p->offset;
 	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
 	    buffer.reserve (this_size);
 
@@ -1850,6 +1852,7 @@ read_pieced_value (struct value *v)
 	  break;
 
 	case DWARF_VALUE_MEMORY:
+	  source_offset_bits += p->offset;
 	  this_size = bits_to_bytes (source_offset_bits, this_size_bits);
 	  buffer.reserve (this_size);
 
@@ -1869,12 +1872,14 @@ read_pieced_value (struct value *v)
 	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
 
 	    /* Use zeroes if piece reaches beyond stack value.  */
-	    if (p->size > obj_size)
+	    if (p->offset + p->size > obj_size)
 	      break;
 
 	    /* Piece is anchored at least significant bit end.  */
 	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
-	      source_offset_bits += obj_size - p->size;
+	      source_offset_bits += obj_size - (p->offset + p->size);
+	    else
+	      source_offset_bits += p->offset;
 	    copy_bitwise (contents, dest_offset_bits,
 			  value_contents_all (p->v.value),
 			  source_offset_bits,
@@ -1888,6 +1893,7 @@ read_pieced_value (struct value *v)
 	    size_t n = this_size_bits;
 
 	    /* Cut off at the end of the implicit value.  */
+	    source_offset_bits += p->offset;
 	    if (source_offset_bits >= obj_size)
 	      break;
 	    if (n > obj_size - source_offset_bits)
@@ -1978,11 +1984,13 @@ write_pieced_value (struct value *to, struct value *from)
 	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& p->size <= reg_bits)
+		&& p->offset + p->size < reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		dest_offset_bits += reg_bits - p->size;
+		dest_offset_bits += reg_bits - (p->offset + p->size);
 	      }
+	    else
+	      dest_offset_bits += p->offset;
 	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
 	    buffer.reserve (this_size);
 
@@ -2019,6 +2027,8 @@ write_pieced_value (struct value *to, struct value *from)
 	  break;
 	case DWARF_VALUE_MEMORY:
 	  {
+	    dest_offset_bits += p->offset;
+
 	    CORE_ADDR start_addr = p->v.mem.addr + dest_offset_bits / 8;
 
 	    if (dest_offset_bits % 8 == 0 && this_size_bits % 8 == 0
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index 4787dfb..29d61f8 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -204,6 +204,22 @@ Dwarf::assemble $asm_file {
 			piece 1
 		    } SPECIAL_expr}
 		}
+		# One piece per bitfield.  Use piece offsets.
+		DW_TAG_variable {
+		    {name "t3"}
+		    {type :$struct_t_label}
+		    {location {
+			implicit_value 0x12 0x34 0x56 0x78 0x9a
+			bit_piece 32 4
+			const2s -280
+			stack_value
+			bit_piece 9 2
+			regx [lindex $dwarf_regnum 0]
+			bit_piece 13 14
+			addr $buf_var
+			bit_piece 10 42
+		    } SPECIAL_expr}
+		}
 	    }
 	}
     }
@@ -276,3 +292,24 @@ gdb_test_no_output "set var t2.y = 2641"
 gdb_test_no_output "set var t2.z = -400"
 gdb_test_no_output "set var t2.x = 200"
 gdb_test "print t2.x + t2.y + t2.z" " = 2441"
+
+# Bitfield access through pieces with nonzero piece offsets.
+gdb_test_no_output "set var \$[lindex $regname 0] = 0xa8000" \
+    "init reg for t3.y"
+gdb_test_no_output "set var *(char \[2\] *) (a + 5) = { 70, 82 }" \
+    "init mem for t3.z"
+switch $endian {
+    little {set val "u = -1484430527, x = -70, y = 42, z = 145"}
+    big {set val "u = 591751049, x = -70, y = 42, z = 101"}
+}
+gdb_test "print t3" " = \\{$val\\}" \
+    "initialized t3 from reg and mem"
+gdb_test_no_output "set var t3.y = -1" \
+    "overwrite t3.y"
+gdb_test "print/x \$[lindex $regname 0]" " = 0x7ffc000" \
+    "verify t3.y through reg"
+gdb_test_no_output "set var t3.z = -614" \
+    "overwrite t3.z"
+switch $endian {big {set val "0x59, 0xa2"} little {set val "0x6a, 0x56"}}
+gdb_test "print/x *(char \[2\] *) (a + 5)" " = \\{$val\\}" \
+    "verify t3.z through mem"
-- 
2.3.0

[PATCH 9/9] Remove unnecessary copies of variables in read/write_pieced_value

[Andreas Arnez]

In read_pieced_value's main loop, the variables `dest_offset_bits' and
`source_offset_bits' are basically just copies of `offset' and
`bits_to_skip', respectively.  In write_pieced_value the copies are
reversed.  This is not very helpful when trying to keep the logic between
these functions in sync.  Since the copies are unnecessary, this patch
just removes them.

gdb/ChangeLog:

	* dwarf2loc.c (read_pieced_value): Remove unnecessary variables
	dest_offset_bits and source_offset_bits.
	(write_pieced_value): Likewise.
---
 gdb/dwarf2loc.c | 118 +++++++++++++++++++++++++-------------------------------
 1 file changed, 52 insertions(+), 66 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 045c2d3..d7bd166 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1790,25 +1790,16 @@ read_pieced_value (struct value *v)
   else
     type_len = 8 * TYPE_LENGTH (value_type (v));
 
-  for (i = 0; i < c->n_pieces && offset < type_len; i++)
+  /* Advance to the first non-skipped piece.  */
+  for (i = 0; i < c->n_pieces && bits_to_skip >= c->pieces[i].size; i++)
+    bits_to_skip -= c->pieces[i].size;
+
+  for (; i < c->n_pieces && offset < type_len; i++)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
       size_t this_size, this_size_bits;
-      long dest_offset_bits, source_offset_bits;
-
-      /* Compute size, source, and destination offsets for copying, in
-	 bits.  */
-      this_size_bits = p->size;
-      if (bits_to_skip > 0 && bits_to_skip >= this_size_bits)
-	{
-	  bits_to_skip -= this_size_bits;
-	  continue;
-	}
-      source_offset_bits = bits_to_skip;
-      this_size_bits -= bits_to_skip;
-      bits_to_skip = 0;
-      dest_offset_bits = offset;
 
+      this_size_bits = p->size - bits_to_skip;
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
@@ -1827,15 +1818,15 @@ read_pieced_value (struct value *v)
 		&& p->offset + p->size < reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		source_offset_bits += reg_bits - (p->offset + p->size);
+		bits_to_skip += reg_bits - (p->offset + p->size);
 	      }
 	    else
-	      source_offset_bits += p->offset;
-	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+	      bits_to_skip += p->offset;
+	    this_size = bits_to_bytes (bits_to_skip, this_size_bits);
 	    buffer.reserve (this_size);
 
 	    if (!get_frame_register_bytes (frame, gdb_regnum,
-					  source_offset_bits / 8,
+					  bits_to_skip / 8,
 					  this_size, buffer.data (),
 					  &optim, &unavail))
 	      {
@@ -1845,23 +1836,23 @@ read_pieced_value (struct value *v)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
 		break;
 	      }
-	    copy_bitwise (contents, dest_offset_bits,
-			  buffer.data (), source_offset_bits % 8,
+	    copy_bitwise (contents, offset,
+			  buffer.data (), bits_to_skip % 8,
 			  this_size_bits, bits_big_endian);
 	  }
 	  break;
 
 	case DWARF_VALUE_MEMORY:
-	  source_offset_bits += p->offset;
-	  this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+	  bits_to_skip += p->offset;
+	  this_size = bits_to_bytes (bits_to_skip, this_size_bits);
 	  buffer.reserve (this_size);
 
 	  read_value_memory (v, offset,
 			     p->v.mem.in_stack_memory,
-			     p->v.mem.addr + source_offset_bits / 8,
+			     p->v.mem.addr + bits_to_skip / 8,
 			     buffer.data (), this_size);
-	  copy_bitwise (contents, dest_offset_bits,
-			buffer.data (), source_offset_bits % 8,
+	  copy_bitwise (contents, offset,
+			buffer.data (), bits_to_skip % 8,
 			this_size_bits, bits_big_endian);
 	  break;
 
@@ -1877,12 +1868,12 @@ read_pieced_value (struct value *v)
 
 	    /* Piece is anchored at least significant bit end.  */
 	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
-	      source_offset_bits += obj_size - (p->offset + p->size);
+	      bits_to_skip += obj_size - (p->offset + p->size);
 	    else
-	      source_offset_bits += p->offset;
-	    copy_bitwise (contents, dest_offset_bits,
+	      bits_to_skip += p->offset;
+	    copy_bitwise (contents, offset,
 			  value_contents_all (p->v.value),
-			  source_offset_bits,
+			  bits_to_skip,
 			  this_size_bits, bits_big_endian);
 	  }
 	  break;
@@ -1893,14 +1884,14 @@ read_pieced_value (struct value *v)
 	    size_t n = this_size_bits;
 
 	    /* Cut off at the end of the implicit value.  */
-	    source_offset_bits += p->offset;
-	    if (source_offset_bits >= obj_size)
+	    bits_to_skip += p->offset;
+	    if (bits_to_skip >= obj_size)
 	      break;
-	    if (n > obj_size - source_offset_bits)
-	      n = obj_size - source_offset_bits;
+	    if (n > obj_size - bits_to_skip)
+	      n = obj_size - bits_to_skip;
 
-	    copy_bitwise (contents, dest_offset_bits,
-			  p->v.literal.data, source_offset_bits,
+	    copy_bitwise (contents, offset,
+			  p->v.literal.data, bits_to_skip,
 			  n, bits_big_endian);
 	  }
 	  break;
@@ -1919,6 +1910,7 @@ read_pieced_value (struct value *v)
 	}
 
       offset += this_size_bits;
+      bits_to_skip = 0;
     }
 }
 
@@ -1954,23 +1946,16 @@ write_pieced_value (struct value *to, struct value *from)
   else
     type_len = 8 * TYPE_LENGTH (value_type (to));
 
-  for (i = 0; i < c->n_pieces && offset < type_len; i++)
+  /* Advance to the first non-skipped piece.  */
+  for (i = 0; i < c->n_pieces && bits_to_skip >= c->pieces[i].size; i++)
+    bits_to_skip -= c->pieces[i].size;
+
+  for (; i < c->n_pieces && offset < type_len; i++)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
       size_t this_size_bits, this_size;
-      long dest_offset_bits, source_offset_bits;
-
-      this_size_bits = p->size;
-      if (bits_to_skip > 0 && bits_to_skip >= this_size_bits)
-	{
-	  bits_to_skip -= this_size_bits;
-	  continue;
-	}
-      dest_offset_bits = bits_to_skip;
-      this_size_bits -= bits_to_skip;
-      bits_to_skip = 0;
-      source_offset_bits = offset;
 
+      this_size_bits = p->size - bits_to_skip;
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
@@ -1987,20 +1972,20 @@ write_pieced_value (struct value *to, struct value *from)
 		&& p->offset + p->size < reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		dest_offset_bits += reg_bits - (p->offset + p->size);
+		bits_to_skip += reg_bits - (p->offset + p->size);
 	      }
 	    else
-	      dest_offset_bits += p->offset;
-	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
+	      bits_to_skip += p->offset;
+	    this_size = bits_to_bytes (bits_to_skip, this_size_bits);
 	    buffer.reserve (this_size);
 
-	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
+	    if (bits_to_skip % 8 != 0 || this_size_bits % 8 != 0)
 	      {
 		/* Need some bits from original register value.  */
 		int optim, unavail;
 
 		if (!get_frame_register_bytes (frame, gdb_regnum,
-					       dest_offset_bits / 8,
+					       bits_to_skip / 8,
 					       this_size, buffer.data (),
 					       &optim, &unavail))
 		  {
@@ -2017,34 +2002,34 @@ write_pieced_value (struct value *to, struct value *from)
 		  }
 	      }
 
-	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
-			  contents, source_offset_bits,
+	    copy_bitwise (buffer.data (), bits_to_skip % 8,
+			  contents, offset,
 			  this_size_bits, bits_big_endian);
 	    put_frame_register_bytes (frame, gdb_regnum,
-				      dest_offset_bits / 8,
+				      bits_to_skip / 8,
 				      this_size, buffer.data ());
 	  }
 	  break;
 	case DWARF_VALUE_MEMORY:
 	  {
-	    dest_offset_bits += p->offset;
+	    bits_to_skip += p->offset;
 
-	    CORE_ADDR start_addr = p->v.mem.addr + dest_offset_bits / 8;
+	    CORE_ADDR start_addr = p->v.mem.addr + bits_to_skip / 8;
 
-	    if (dest_offset_bits % 8 == 0 && this_size_bits % 8 == 0
-		&& source_offset_bits % 8 == 0)
+	    if (bits_to_skip % 8 == 0 && this_size_bits % 8 == 0
+		&& offset % 8 == 0)
 	      {
 		/* Everything is byte-aligned; no buffer needed.  */
 		write_memory (start_addr,
-			      contents + source_offset_bits / 8,
+			      contents + offset / 8,
 			      this_size_bits / 8);
 		break;
 	      }
 
-	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
+	    this_size = bits_to_bytes (bits_to_skip, this_size_bits);
 	    buffer.reserve (this_size);
 
-	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
+	    if (bits_to_skip % 8 != 0 || this_size_bits % 8 != 0)
 	      {
 		if (this_size <= 8)
 		  {
@@ -2061,8 +2046,8 @@ write_pieced_value (struct value *to, struct value *from)
 		  }
 	      }
 
-	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
-			  contents, source_offset_bits,
+	    copy_bitwise (buffer.data (), bits_to_skip % 8,
+			  contents, offset,
 			  this_size_bits, bits_big_endian);
 	    write_memory (start_addr, buffer.data (), this_size);
 	  }
@@ -2072,6 +2057,7 @@ write_pieced_value (struct value *to, struct value *from)
 	  break;
 	}
       offset += this_size_bits;
+      bits_to_skip = 0;
     }
 }
 
-- 
2.3.0

Re: [PATCH 1/9] Add test for modifiable DWARF locations

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> This adds a test for read/write access to variables with various types 
> of
> DWARF locations.  It uses register- and memory locations and composite
> locations with register- and memory pieces.
> 
> Since the new test calls gdb_test_no_output with commands that contain
> braces, it is necessary for string_to_regexp to quote braces as well.
> This was not done before.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/var-access.c: New file.
> 	* gdb.dwarf2/var-access.exp: New test.
> 	* lib/gdb-utils.exp (string_to_regexp): Quote braces as well.
> ---
>  gdb/testsuite/gdb.dwarf2/var-access.c   |  25 +++++
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 193 
> ++++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/gdb-utils.exp         |   2 +-
>  3 files changed, 219 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/var-access.exp
> 
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.c
> b/gdb/testsuite/gdb.dwarf2/var-access.c
> new file mode 100644
> index 0000000..108be82
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +char buf[] = {0, 1, 2, 3, 4, 5, 6, 7};
> +
> +int
> +main (void)
> +{
> +  asm volatile ("main_label: .globl main_label");
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> new file mode 100644
> index 0000000..ee93b93
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -0,0 +1,193 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.
> +
> +# Test reading/writing variables with non-trivial DWARF locations.
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use 
> gas.
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +
> +# Choose suitable integer registers for the test.
> +
> +set dwarf_regnum {0 1}
> +
> +if { [istarget "aarch64*-*-*"] } {
> +    set regname {x0 x1}
> +} elseif { [istarget "arm*-*-*"]
> +	   || [istarget "s390*-*-*" ]
> +	   || [istarget "powerpc*-*-*"]
> +	   || [istarget "rs6000*-*-aix*"] } {
> +    set regname {r0 r1}
> +} elseif { [istarget "i?86-*-*"] } {
> +    set regname {eax edx}
> +} elseif { [istarget "x86_64-*-*"] } {
> +    set regname {rax rdx}
> +} else {
> +    verbose "Skipping tests for accessing DWARF-described variables."
> +    return
> +}
> +
> +standard_testfile .c ${gdb_test_file_name}-dw.S
> +
> +# Make some DWARF for the test.
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    global dwarf_regnum regname
> +
> +    set main_func \
> +	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
> +    set buf_var [gdb_target_symbol buf]
> +
> +    cu {} {
> +	DW_TAG_compile_unit {
> +		{DW_AT_name var-pieces-dw.c}
> +		{DW_AT_comp_dir /tmp}
> +	} {
> +	    declare_labels char_type_label
> +	    declare_labels int_type_label short_type_label
> +	    declare_labels array_a8_label struct_s_label
> +
> +	    char_type_label: base_type {
> +		{name "char"}
> +		{encoding @DW_ATE_unsigned_char}
> +		{byte_size 1 DW_FORM_sdata}
> +	    }
> +
> +	    int_type_label: base_type {
> +		{name "int"}
> +		{encoding @DW_ATE_signed}
> +		{byte_size 4 DW_FORM_sdata}
> +	    }
> +
> +	    array_a8_label: array_type {
> +		{type :$char_type_label}
> +	    } {
> +		subrange_type {
> +		    {type :$int_type_label}
> +		    {upper_bound 7 DW_FORM_udata}
> +		}
> +	    }
> +
> +	    struct_s_label: structure_type {
> +		{name "s"}
> +		{byte_size 4 DW_FORM_sdata}
> +	    } {
> +		member {
> +		    {name "a"}
> +		    {type :$char_type_label}
> +		    {data_member_location 0 DW_FORM_udata}
> +		}
> +		member {
> +		    {name "b"}
> +		    {type :$char_type_label}
> +		    {data_member_location 1 DW_FORM_udata}
> +		}
> +		member {
> +		    {name "c"}
> +		    {type :$char_type_label}
> +		    {data_member_location 2 DW_FORM_udata}
> +		}
> +		member {
> +		    {name "d"}
> +		    {type :$char_type_label}
> +		    {data_member_location 3 DW_FORM_udata}
> +		}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{name "main"}
> +		{DW_AT_external 1 flag}
> +		{low_pc [lindex $main_func 0] DW_FORM_addr}
> +		{high_pc [lindex $main_func 1] DW_FORM_udata}
> +	    } {
> +		# Simple memory location.
> +		DW_TAG_variable {
> +		    {name "a"}
> +		    {type :$array_a8_label}
> +		    {location {
> +			addr $buf_var
> +		    } SPECIAL_expr}
> +		}
> +		# Memory pieces.
> +		DW_TAG_variable {
> +		    {name "s1"}
> +		    {type :$struct_s_label}
> +		    {location {
> +			addr $buf_var
> +			plus_uconst 2
> +			piece 2
> +			addr $buf_var
> +			piece 2
> +		    } SPECIAL_expr}
> +		}
> +		# Register- and memory pieces.
> +		DW_TAG_variable {
> +		    {name "s2"}
> +		    {type :$struct_s_label}
> +		    {location {
> +			regx [lindex $dwarf_regnum 0]
> +			piece 1
> +			addr "$buf_var + 4"
> +			piece 1
> +			regx [lindex $dwarf_regnum 1]
> +			piece 1
> +			addr "$buf_var + 5"
> +			piece 1
> +		    } SPECIAL_expr}
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} \
> +	  [list $srcfile $asm_file] {nodebug}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# Byte-aligned memory pieces.
> +gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
> +    "s1 == re-ordered buf"
> +gdb_test_no_output "set var s1.a = 63"
> +gdb_test "print/d s1" " = \\{a = 63, b = 3, c = 0, d = 1\\}" \
> +    "verify s1.a"
> +gdb_test "print/d a" " = \\{0, 1, 63, 3, 4, 5, 6, 7\\}" \
> +    "verify s1.a through a"
> +
> +# Byte-aligned register- and memory pieces.
> +gdb_test_no_output "set var \$[lindex $regname 0] = 81" \
> +    "init reg for s2.a"
> +gdb_test_no_output "set var \$[lindex $regname 1] = 28" \
> +    "init reg for s2.c"
> +gdb_test "print/d s2" " = \\{a = 81, b = 4, c = 28, d = 5\\}" \
> +    "initialized s2 from mem and regs"
> +gdb_test_no_output "set var s2.c += s2.a + s2.b - s2.d"
> +gdb_test "print/d s2" " = \\{a = 81, b = 4, c = 108, d = 5\\}" \
> +    "verify s2.c"
> +gdb_test "print/d \$[lindex $regname 1]" " = 108" \
> +    "verify s2.c through reg"
> +gdb_test_no_output "set var s2 = {191, 73, 231, 123}" \
> +    "re-initialize s2"
> +gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
> +    "verify re-initialized s2"
> diff --git a/gdb/testsuite/lib/gdb-utils.exp 
> b/gdb/testsuite/lib/gdb-utils.exp
> index ff1b24a..37abb14 100644
> --- a/gdb/testsuite/lib/gdb-utils.exp
> +++ b/gdb/testsuite/lib/gdb-utils.exp
> @@ -34,6 +34,6 @@ proc gdb_init_commands {} {
> 
>  proc string_to_regexp {str} {
>      set result $str
> -    regsub -all {[]*+.|()^$\[\\]} $str {\\&} result
> +    regsub -all {[]*+.|(){}^$\[\\]} $str {\\&} result
>      return $result
>  }

Hi Andreas,

To be honest, I didn't know what a DWARF piece was before, well, two 
hours ago.  I've spent some time reading about that, including your 
write-up on the problem [1] (very informative, thanks) and the 
implementation of read_pieced_value.  I can't say I have a very good 
grasp on the problem you are tackling, but I got enough to understand 
all the lines in this patch, and it looks good to me.

It will probably take someone with more experience about this stuff than 
me to review the more intricate parts of the series, but I'll keep on 
reading when I have time, even if it's just for my own 
knowledge/enjoyment :).  Although while going through read_pieced_value 
line by line, I've found some areas that could be improved.  I suspect 
that there might be some overlap with some of the following patches of 
your series, so I might be able to understand and review those.

Thanks,

Simon

[1] https://sourceware.org/ml/gdb/2016-01/msg00013.html

Re: [PATCH 2/9] Fix size capping in write_pieced_value

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

> A field in a structure composed of DWARF pieces overlaps one or more of
> those pieces.  When writing to the field, the beginning of the first and
> the end of the last of those pieces may have to be skipped.  But the

Do you have an example?

> logic in write_pieced_value for handling this is flawed when there are
> actually bits to skip at the beginning of the first piece: it truncates
> the piece size towards the end *before* accounting for the skipped bits
> at the beginning instead of the other way around.
>
> Note that the same bug was already found in read_pieced_value and fixed
> there (but not in write_pieced_value), see PR 15391.

Can we share the code in write_pieced_value and read_pieced_value?  The
code computing offsets and bits should be shared.  Also, we need more
comments in the code to explain these offsets and bits, a diagram about
the relationships of these bits and offsets is quite helpful.

-- 
Yao (齐尧)

Re: [PATCH 1/9] Add test for modifiable DWARF locations

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

> This adds a test for read/write access to variables with various types of
> DWARF locations.  It uses register- and memory locations and composite
> locations with register- and memory pieces.

The last line should be put into the introduction comment of
gdb.dwarf2/var-access.exp.

> +
> +# Choose suitable integer registers for the test.
> +
> +set dwarf_regnum {0 1}
> +
> +if { [istarget "aarch64*-*-*"] } {

if { [is_aarch64_target] }

> +    set regname {x0 x1}
> +} elseif { [istarget "arm*-*-*"]

else if { [is_aarch32_target]

> +	   || [istarget "s390*-*-*" ]
> +	   || [istarget "powerpc*-*-*"]
> +	   || [istarget "rs6000*-*-aix*"] } {
> +    set regname {r0 r1}
> +} elseif { [istarget "i?86-*-*"] } {
> +    set regname {eax edx}
> +} elseif { [istarget "x86_64-*-*"] } {
> +    set regname {rax rdx}
> +} else {
> +    verbose "Skipping tests for accessing DWARF-described variables."
> +    return
> +}
> +
> +standard_testfile .c ${gdb_test_file_name}-dw.S
> +
> +# Make some DWARF for the test.
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    global dwarf_regnum regname
> +
> +    set main_func \
> +	[function_range main [list ${srcdir}/${subdir}/$srcfile]]

> +
> +	    DW_TAG_subprogram {
> +		{name "main"}
> +		{DW_AT_external 1 flag}
> +		{low_pc [lindex $main_func 0] DW_FORM_addr}
> +		{high_pc [lindex $main_func 1] DW_FORM_udata}

You can use MACRO_AT_func, it can replace name, low_pc and high_pc.
Also, you don't need to call function_range above.

> +	    } {
> +		# Simple memory location.
> +		DW_TAG_variable {
> +		    {name "a"}
> +		    {type :$array_a8_label}
> +		    {location {
> +			addr $buf_var
> +		    } SPECIAL_expr}
> +		}
> +		# Memory pieces.

Nit: we can also describe where are these pieces in comments, like

                # Memory pieces, two bytes from &buf[0], and two
                # bytes from &buf[2].

> +		DW_TAG_variable {
> +		    {name "s1"}
> +		    {type :$struct_s_label}
> +		    {location {
> +			addr $buf_var
> +			plus_uconst 2
> +			piece 2
> +			addr $buf_var
> +			piece 2
> +		    } SPECIAL_expr}
> +		}
> +		# Register- and memory pieces.

Likewise.

OK with the changes.

-- 
Yao (齐尧)

Re: [PATCH 4/9] Remove addr_size field from struct piece_closure

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

> The addr_size field in the piece_closure data structure is a relic from
> before introducing the typed DWARF stack.  It is obsolete now.  This patch
> removes it.
>
> gdb/ChangeLog:
>
> 	* dwarf2loc.c (struct piece_closure) <addr_size>: Remove field.
> 	(allocate_piece_closure): Drop addr_size parameter.
> 	(dwarf2_evaluate_loc_desc_full): Adjust call to
> 	allocate_piece_closure.

LGTM.

-- 
Yao (齐尧)

Re: [PATCH 1/9] Add test for modifiable DWARF locations

[Andreas Arnez]

On Thu, Apr 13 2017, Simon Marchi wrote:

> To be honest, I didn't know what a DWARF piece was before, well, two hours
> ago.  I've spent some time reading about that, including your write-up on
> the problem [1] (very informative, thanks) and the implementation of
> read_pieced_value.  I can't say I have a very good grasp on the problem
> you are tackling, but I got enough to understand all the lines in this
> patch, and it looks good to me.

Thanks for your interest in this topic; very much appreciated.  The more
people look at this, the better our chance of getting it right this
time.

> It will probably take someone with more experience about this stuff than
> me to review the more intricate parts of the series, but I'll keep on
> reading when I have time, even if it's just for my own knowledge/enjoyment
> :).  Although while going through read_pieced_value line by line, I've
> found some areas that could be improved.  I suspect that there might be
> some overlap with some of the following patches of your series, so I might
> be able to understand and review those.

Yeah, the follow-up patches rewrite most of read/write_pieced_value.

--
Andreas

Re: [PATCH 1/9] Add test for modifiable DWARF locations

[Andreas Arnez]

On Thu, Apr 13 2017, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
>> This adds a test for read/write access to variables with various types of
>> DWARF locations.  It uses register- and memory locations and composite
>> locations with register- and memory pieces.
>
> The last line should be put into the introduction comment of
> gdb.dwarf2/var-access.exp.

OK.

[...]

>> +# Make some DWARF for the test.
>> +
>> +set asm_file [standard_output_file $srcfile2]
>> +Dwarf::assemble $asm_file {
>> +    global srcdir subdir srcfile
>> +    global dwarf_regnum regname
>> +
>> +    set main_func \
>> +	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
>
>> +
>> +	    DW_TAG_subprogram {
>> +		{name "main"}
>> +		{DW_AT_external 1 flag}
>> +		{low_pc [lindex $main_func 0] DW_FORM_addr}
>> +		{high_pc [lindex $main_func 1] DW_FORM_udata}
>
> You can use MACRO_AT_func, it can replace name, low_pc and high_pc.
> Also, you don't need to call function_range above.

Right, I missed that, thanks.

>
>> +	    } {
>> +		# Simple memory location.
>> +		DW_TAG_variable {
>> +		    {name "a"}
>> +		    {type :$array_a8_label}
>> +		    {location {
>> +			addr $buf_var
>> +		    } SPECIAL_expr}
>> +		}
>> +		# Memory pieces.
>
> Nit: we can also describe where are these pieces in comments, like
>
>                 # Memory pieces, two bytes from &buf[0], and two
>                 # bytes from &buf[2].

The other way around ;-)

>
>> +		DW_TAG_variable {
>> +		    {name "s1"}
>> +		    {type :$struct_s_label}
>> +		    {location {
>> +			addr $buf_var
>> +			plus_uconst 2
>> +			piece 2
>> +			addr $buf_var
>> +			piece 2
>> +		    } SPECIAL_expr}
>> +		}
>> +		# Register- and memory pieces.
>
> Likewise.
>
> OK with the changes.

Thanks, updated the patch with the delta patch below.  Also updated the
follow-on patches with similar comments for the composite locations in
var-access.exp.

--
Andreas


diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index ee93b93..4d6c9ca 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -13,7 +13,9 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# Test reading/writing variables with non-trivial DWARF locations.
+# Test reading/writing variables with non-trivial DWARF locations.  In
+# particular the test uses register- and memory locations as well as
+# composite locations with register- and memory pieces.
 
 load_lib dwarf.exp
 
@@ -51,8 +53,6 @@ Dwarf::assemble $asm_file {
     global srcdir subdir srcfile
     global dwarf_regnum regname
 
-    set main_func \
-	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
     set buf_var [gdb_target_symbol buf]
 
     cu {} {
@@ -112,10 +112,8 @@ Dwarf::assemble $asm_file {
 	    }
 
 	    DW_TAG_subprogram {
-		{name "main"}
+		{MACRO_AT_func { main ${srcdir}/${subdir}/${srcfile} }}
 		{DW_AT_external 1 flag}
-		{low_pc [lindex $main_func 0] DW_FORM_addr}
-		{high_pc [lindex $main_func 1] DW_FORM_udata}
 	    } {
 		# Simple memory location.
 		DW_TAG_variable {
@@ -125,7 +123,8 @@ Dwarf::assemble $asm_file {
 			addr $buf_var
 		    } SPECIAL_expr}
 		}
-		# Memory pieces.
+		# Memory pieces: two bytes from &buf[2], and two bytes
+		# from &buf[0].
 		DW_TAG_variable {
 		    {name "s1"}
 		    {type :$struct_s_label}
@@ -137,7 +136,8 @@ Dwarf::assemble $asm_file {
 			piece 2
 		    } SPECIAL_expr}
 		}
-		# Register- and memory pieces.
+		# Register- and memory pieces: one byte each from r0,
+		# &buf[4], r1, and &buf[5].
 		DW_TAG_variable {
 		    {name "s2"}
 		    {type :$struct_s_label}

Re: [PATCH 2/9] Fix size capping in write_pieced_value

[Andreas Arnez]

On Thu, Apr 13 2017, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
>> A field in a structure composed of DWARF pieces overlaps one or more of
>> those pieces.  When writing to the field, the beginning of the first and
>> the end of the last of those pieces may have to be skipped.  But the
>
> Do you have an example?

OK, I guess the commit message should be improved a bit.  How about
this?

  A field f in a structure composed of DWARF pieces may be located in
  multiple pieces, where the first and last of those may contain bits
  from other fields as well.  So when writing to f, the beginning of the
  first and the end of the last of those pieces may have to be skipped.
  But the logic in write_pieced_value for handling one of those pieces
  is flawed when the first and last piece are the same, i.e., f is
  contained in a single piece:

    < - - - - - - - - - piece_size - - - - - - - - - ->
    +-------------------------------------------------+
    | skipped_bits |   f_bits   | / / / / / / / / / / |
    +-------------------------------------------------+

  The current logic determines the size of the sub-piece to operate on
  by limiting the piece size to the bit size of f and then subtracting
  the skipped bits:

    max (piece_size, f_bits) - skipped_bits

  Instead of:

    max (piece_size - skipped_bits, f_bits)

  So the resulting sub-piece size is corrupted, leading to wrong
  handling of this piece in write_pieced_value.

>
>> logic in write_pieced_value for handling this is flawed when there are
>> actually bits to skip at the beginning of the first piece: it truncates
>> the piece size towards the end *before* accounting for the skipped bits
>> at the beginning instead of the other way around.
>>
>> Note that the same bug was already found in read_pieced_value and fixed
>> there (but not in write_pieced_value), see PR 15391.
>
> Can we share the code in write_pieced_value and read_pieced_value?  The
> code computing offsets and bits should be shared.

Yes.  I have another patch (not posted yet) that merges these two
functions.  I moved that towards the end of the patch series, so the
individual fixes can be incremental.

> Also, we need more comments in the code to explain these offsets and
> bits, a diagram about the relationships of these bits and offsets is
> quite helpful.

OK.  Some of the offset variables are removed by my patches, so I guess
I'll postpone that to the merged version.  I'll see what I can come up
with and include it in v2.

--
Andreas

Re: [PATCH 3/9] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value, 
> the
> existing logic always takes the piece from the lowest-addressed end, 
> which
> is wrong on big-endian targets.

I'd like if you could clarify this (just here not necessarily in the 
patch).  DWARF locations are computed inside GDB, on the host.  So does 
it really depend on the target endianness, or it's that of the host, or 
both?

Let's consider these cases of remote debugging:

  host -> target
  x86  -> x86
  x86  -> s390
  s390 -> x86
  s390 -> s390

In which cases is the value found at the high memory address vs low 
memory address?

> The DWARF standard states that the
> "DW_OP_bit_piece operation describes a sequence of bits using the least
> significant bits of that value", and this also matches the current 
> logic
> in GCC.  For instance, the GCC guality test case pr54970.c fails on 
> s390x
> because of this.
> 
> This fix adjusts the piece accordingly on big-endian targets.  It is
> assumed that:
> 
> * DW_OP_piece shall take the piece from the LSB end as well;
> 
> * pieces reaching outside the stack value bits are considered 
> undefined,
>   and a zero value can be used instead.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/21226
> 	* dwarf2loc.c (read_pieced_value): Anchor stack value pieces at
> 	the LSB end, independent of endianness.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/21226
> 	* gdb.dwarf2/nonvar-access.exp: Add checks for verifying that
> 	stack value pieces are taken from the LSB end.
> ---
>  gdb/dwarf2loc.c                            | 43 
> ++++++++++++++++--------------
>  gdb/testsuite/gdb.dwarf2/nonvar-access.exp | 21 ++++++++++++---
>  2 files changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 496400a..09938c4 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1857,6 +1857,10 @@ read_pieced_value (struct value *v)
>  		if (unavail)
>  		  mark_value_bits_unavailable (v, offset, this_size_bits);
>  	      }
> +
> +	    copy_bitwise (contents, dest_offset_bits,
> +			  intermediate_buffer, source_offset_bits % 8,
> +			  this_size_bits, bits_big_endian);
>  	  }
>  	  break;
> 
> @@ -1865,26 +1869,28 @@ read_pieced_value (struct value *v)
>  			     p->v.mem.in_stack_memory,
>  			     p->v.mem.addr + source_offset,
>  			     buffer.data (), this_size);
> +	  copy_bitwise (contents, dest_offset_bits,
> +			intermediate_buffer, source_offset_bits % 8,
> +			this_size_bits, bits_big_endian);
>  	  break;
> 
>  	case DWARF_VALUE_STACK:
>  	  {
> -	    size_t n = this_size;
> +	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
> +	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
> +	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));

It would be really nice for the readers if you could put some comment 
like this, even though it may seem obvious to you:

   /* The size of a DWARF stack value.  */
   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));

I found I had to add them to the code to be able to follow.

> 
> -	    if (n > c->addr_size - source_offset)
> -	      n = (c->addr_size >= source_offset
> -		   ? c->addr_size - source_offset
> -		   : 0);
> -	    if (n == 0)
> -	      {
> -		/* Nothing.  */
> -	      }
> -	    else
> -	      {
> -		const gdb_byte *val_bytes = value_contents_all (p->v.value);

> +	    /* Use zeroes if piece reaches beyond stack value.  */
> +	    if (p->size > obj_size)
> +	      break;

Does this happen, for example, if a DWARF stack value is 32 bits long, 
but the piece is 64 bits?  I suppose that's not something we'd want a 
compiler to emit, and would be considered a bug in the compiler?

How does breaking out of the loop will use zeroes?  Is the value buffer 
cleared beforehand?

> 
> -		intermediate_buffer = val_bytes + source_offset;
> -	      }
> +	    /* Piece is anchored at least significant bit end.  */
> +	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
> +	      source_offset_bits += obj_size - p->size;

Just a nit, but I find it more readable when there's an empty line 
between the if and the following lines not included in the if (so here, 
right where I cut the quote).  It reads like two separate sentences:

  - If the byte order is big endian, adjust offset in the source.
  - Copy bitwise from the source buffer to the destination buffer.

> +	    copy_bitwise (contents, dest_offset_bits,
> +			  value_contents_all (p->v.value),
> +			  source_offset_bits,
> +			  this_size_bits, bits_big_endian);

Thanks,

Simon

Re: [PATCH 4/9] Remove addr_size field from struct piece_closure

[Simon Marchi]

On 2017-04-13 05:10, Yao Qi wrote:
> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
> 
>> The addr_size field in the piece_closure data structure is a relic 
>> from
>> before introducing the typed DWARF stack.  It is obsolete now.  This 
>> patch
>> removes it.
>> 
>> gdb/ChangeLog:
>> 
>> 	* dwarf2loc.c (struct piece_closure) <addr_size>: Remove field.
>> 	(allocate_piece_closure): Drop addr_size parameter.
>> 	(dwarf2_evaluate_loc_desc_full): Adjust call to
>> 	allocate_piece_closure.
> 
> LGTM.

Since you are planning on making a v2, I think it's better if you check 
it in right now to get it out of the way (it could be obvious anyway).

Simon

Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> There are multiple issues in write_pieced_value when dealing with a
> bit-field as the target value:
> 
> (1) The number of bits preceding the bit-field is calculated without
>     considering the relative offset of the value's parent.

If others are wondering when this can happen:

struct s {
     uint64_t foo;
     struct {
	uint32_t bar;
	uint32_t bf : 10;
     } baz;
};

If "val" is a struct value representing bf:

  - value_offset(val) == 4 (sizeof bar)
  - val->parent represents the whole baz structure
  - value_offset(val->parent) == 8 (sizeof foo)

If bf was a "standard", non-bitfield variable, its offset would be 12 
directly.

There are multiple places that do "value_offset (parent) + value_offset 
(value)" when value is a bitfield.  Isn't it what we want most of the 
time?  If so, I wonder if eventually value_offset shouldn't instead 
return the computed offset, like:

LONGEST
value_offset (const struct value *value)
{
   if (value_bitsize (value))
     return value->offset + value_offset (value_parent (value));
   else
     return value->offset;
}

> (2) On big-endian targets the source value's *most* significant bits 
> are
>     transferred to the target value, instead of its least significant
>     bits.
> 
> (3) The number of bytes containing a portion of the bit-field in a 
> given
>     piece is calculated with the wrong starting offset; thus the result
>     may be off by one.
> 
> (4) When checking whether the data can be transferred byte-wise, the
>     transfer size is not verified to be byte-aligned.
> 
> (5) When transferring the data via a buffer, the bit offset within the
>     target value is not reduced to its sub-byte fraction before using 
> it
>     as a bit offset into the buffer.
> 
> These issues are fixed.  For consistency, the affected logic that 
> exists
> in read_pieced_value as well is changed there in the same way.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (write_pieced_value): Fix various issues with
> 	bit-field handling.
> 	(read_pieced_value): Sync with changes in write_pieced_value.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/var-access.exp: Add test for accessing bit-fields
> 	whose containing structure is located in several DWARF pieces.
> ---
>  gdb/dwarf2loc.c                         | 54 
> +++++++++++++++------------------
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 50 
> +++++++++++++++++++++++++++++-
>  2 files changed, 74 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 76a58a3..1f89a08 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1775,7 +1775,8 @@ read_pieced_value (struct value *v)
>    bits_to_skip = 8 * value_offset (v);
>    if (value_bitsize (v))
>      {
> -      bits_to_skip += value_bitpos (v);
> +      bits_to_skip += (8 * value_offset (value_parent (v))
> +		       + value_bitpos (v));

I guess this is related to (1).

>        type_len = value_bitsize (v);
>      }
>    else
> @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
>  	  bits_to_skip -= this_size_bits;
>  	  continue;
>  	}
> -      if (bits_to_skip > 0)
> -	{
> -	  dest_offset_bits = 0;
> -	  source_offset_bits = bits_to_skip;
> -	  this_size_bits -= bits_to_skip;
> -	  bits_to_skip = 0;
> -	}
> -      else
> -	{
> -	  dest_offset_bits = offset;
> -	  source_offset_bits = 0;
> -	}
> +      source_offset_bits = bits_to_skip;
> +      this_size_bits -= bits_to_skip;
> +      bits_to_skip = 0;
> +      dest_offset_bits = offset;
> +

Is this snippet related to one of the problems you have described?  It 
seems to me like it's just simplifying the code, but it's not changing 
the behavior.  If that's the case, I'd suggest putting it in its own 
patch (along with its equivalent in write_pieced_value).

>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct 
> value *from)
>    bits_to_skip = 8 * value_offset (to);
>    if (value_bitsize (to))
>      {
> -      bits_to_skip += value_bitpos (to);
> +      bits_to_skip += (8 * value_offset (value_parent (to))
> +		       + value_bitpos (to));
>        type_len = value_bitsize (to);
> +      /* Use the least significant bits of FROM.  */
> +      if (gdbarch_byte_order (get_type_arch (value_type (from)))
> +	  == BFD_ENDIAN_BIG)
> +	{
> +	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
> +	  type_len += offset;
> +	}

I guess this is related to (1) and (2).

>      }
>    else
>      type_len = 8 * TYPE_LENGTH (value_type (to));
> @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	  bits_to_skip -= this_size_bits;
>  	  continue;
>  	}
> -      if (bits_to_skip > 0)
> -	{
> -	  dest_offset_bits = bits_to_skip;
> -	  source_offset_bits = 0;
> -	  this_size_bits -= bits_to_skip;
> -	  bits_to_skip = 0;
> -	}
> -      else
> -	{
> -	  dest_offset_bits = 0;
> -	  source_offset_bits = offset;
> -	}
> +      dest_offset_bits = bits_to_skip;
> +      this_size_bits -= bits_to_skip;
> +      bits_to_skip = 0;
> +      source_offset_bits = offset;
> +
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
> +      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;

I guess this is related to (3).

>        source_offset = source_offset_bits / 8;
>        dest_offset = dest_offset_bits / 8;
> -      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0)
> +      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
> +	  && this_size_bits % 8 == 0)

... and this to (4).

>  	{
>  	  source_buffer = contents + source_offset;
>  	  need_bitwise = 0;
> @@ -2049,7 +2045,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>  	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
>  	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
>  			   &buffer[this_size - 1], 1);
> -	      copy_bitwise (buffer.data (), dest_offset_bits,
> +	      copy_bitwise (buffer.data (), dest_offset_bits % 8,

... and this to (5).

>  			    contents, source_offset_bits,
>  			    this_size_bits,
>  			    bits_big_endian);
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> index 56a635a..c6abc87 100644
> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -62,7 +62,7 @@ Dwarf::assemble $asm_file {
>  	} {
>  	    declare_labels char_type_label
>  	    declare_labels int_type_label short_type_label
> -	    declare_labels array_a8_label struct_s_label
> +	    declare_labels array_a8_label struct_s_label struct_t_label
> 
>  	    char_type_label: base_type {
>  		{name "char"}
> @@ -111,6 +111,34 @@ Dwarf::assemble $asm_file {
>  		}
>  	    }
> 
> +	    struct_t_label: structure_type {
> +		{name "t"}
> +		{byte_size 8 DW_FORM_sdata}
> +	    } {
> +		member {
> +		    {name u}
> +		    {type :$int_type_label}
> +		}
> +		member {
> +		    {name x}
> +		    {type :$int_type_label}
> +		    {data_member_location 4 DW_FORM_udata}
> +		    {bit_size 9 DW_FORM_udata}
> +		}
> +		member {
> +		    {name y}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 41 DW_FORM_udata}
> +		    {bit_size 13 DW_FORM_udata}
> +		}
> +		member {
> +		    {name z}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 54 DW_FORM_udata}
> +		    {bit_size 10 DW_FORM_udata}
> +		}
> +	    }
> +
>  	    DW_TAG_subprogram {
>  		{name "main"}
>  		{DW_AT_external 1 flag}
> @@ -152,6 +180,18 @@ Dwarf::assemble $asm_file {
>  			piece 1
>  		    } SPECIAL_expr}
>  		}
> +		# Memory pieces for bitfield access.
> +		DW_TAG_variable {
> +		    {name "t1"}
> +		    {type :$struct_t_label}
> +		    {location {
> +			piece 4
> +			addr "$buf_var + 1"
> +			piece 3
> +			addr "$buf_var"
> +			piece 1
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -196,3 +236,11 @@ gdb_test_no_output "set var s2 = {191, 73, 231, 
> 123}" \
>      "re-initialize s2"
>  gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
>      "verify re-initialized s2"
> +
> +# Unaligned bitfield access through byte-aligned pieces.
> +gdb_test_no_output "set var t1.x = -7"
> +gdb_test_no_output "set var t1.z = 340"
> +gdb_test_no_output "set var t1.y = 1234"
> +gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 
> 340\\}" \
> +    "verify t1"

Maybe I'm missing something, but if there's the same bug in the write 
and read implementations, it's possible it would slip through, isn't it? 
  For example, if the "set var" part messes up the bit ordering and the 
"print" part messes it up the same way, it would appear correct when 
it's not.  Reading actual data from a progam won't work.

In the other tests, you tested against known data already in memory, 
which made sense I think.

Thanks,

Simon

Re: [PATCH 6/9] Fix handling of DWARF register pieces on big-endian targets

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> For big-endian targets the logic in read/write_pieced_value tries to 
> take
> a register piece from the LSB end.  This requires offsets and sizes to 
> be
> adjusted accordingly, and that's where the current implementation has 
> some
> issues:
> 
> * The formulas for recalculating the bit- and byte-offsets into the
>   register are wrong.  They just happen to yield correct results if
>   everything is byte-aligned and the piece's last byte belongs to the
>   given value.
> 
> * After recalculating the bit offset into the register, the number of
>   bytes to be copied from the register is not recalculated.  Of course
>   this does not matter if everything (particularly the piece size) is
>   byte-aligned.
> 
> These issues are fixed.  The size calculation is performed with a new
> helper function bits_to_bytes().  For simplification, the variable
> buffer_size is dropped.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (bits_to_bytes): New function.
> 	(read_pieced_value): Fix offset calculations for register pieces
> 	on big-endian targets.
> 	(write_pieced_value): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/var-access.exp: Add test for non-byte-aligned
> 	register pieces.
> ---
>  gdb/dwarf2loc.c                         | 74 
> +++++++++++++++++----------------
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 32 ++++++++++++++
>  2 files changed, 70 insertions(+), 36 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 1f89a08..d13c0c5 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1751,6 +1751,15 @@ copy_bitwise_tests (void)
> 
>  #endif /* GDB_SELF_TEST */
> 
> +/* Return the number of bytes overlapping a contiguous chunk of N_BITS
> +   bits whose first bit is located at bit offset START.  */
> +
> +static size_t
> +bits_to_bytes (ULONGEST start, ULONGEST n_bits)
> +{
> +  return (start % 8 + n_bits + 7) / 8;
> +}
> +
>  static void
>  read_pieced_value (struct value *v)
>  {
> @@ -1761,7 +1770,6 @@ read_pieced_value (struct value *v)
>    struct piece_closure *c
>      = (struct piece_closure *) value_computed_closure (v);
>    size_t type_len;
> -  size_t buffer_size = 0;
>    std::vector<gdb_byte> buffer;
>    int bits_big_endian
>      = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
> @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
> +      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> +      buffer.reserve (this_size);
>        source_offset = source_offset_bits / 8;
> -      if (buffer_size < this_size)
> -	{
> -	  buffer_size = this_size;
> -	  buffer.reserve (buffer_size);
> -	}

The removal of buffer_size and this "if" (and equivalent in 
write_pieced_value) seems to be unrelated to the problem you are fixing, 
but rather a simplification that could be done anyway.  If that's the 
case, you should bundle it with the similar changes from the previous 
patch, in a patch that only does some refactoring but not behavior 
changes.

>        intermediate_buffer = buffer.data ();
> 
>        /* Copy from the source to DEST_BUFFER.  */
> @@ -1822,22 +1826,22 @@ read_pieced_value (struct value *v)
>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>  	    struct gdbarch *arch = get_frame_arch (frame);
>  	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
>  	    int optim, unavail;
> -	    LONGEST reg_offset = source_offset;
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& this_size < register_size (arch, gdb_regnum))
> +		&& p->size < reg_bits)
>  	      {
>  		/* Big-endian, and we want less than full size.  */
> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
> -		/* We want the lower-order THIS_SIZE_BITS of the bytes
> -		   we extract from the register.  */
> -		source_offset_bits += 8 * this_size - this_size_bits;
> +		source_offset_bits += reg_bits - p->size;
>  	      }
> +	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> +	    buffer.reserve (this_size);
> 
> -	    if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
> -					   this_size, buffer.data (),
> -					   &optim, &unavail))
> +	    if (!get_frame_register_bytes (frame, gdb_regnum,
> +					  source_offset_bits / 8,
> +					  this_size, buffer.data (),
> +					  &optim, &unavail))
>  	      {
>  		/* Just so garbage doesn't ever shine through.  */
>  		memset (buffer.data (), 0, this_size);
> @@ -1847,9 +1851,8 @@ read_pieced_value (struct value *v)
>  		if (unavail)
>  		  mark_value_bits_unavailable (v, offset, this_size_bits);
>  	      }
> -
>  	    copy_bitwise (contents, dest_offset_bits,
> -			  intermediate_buffer, source_offset_bits % 8,
> +			  buffer.data (), source_offset_bits % 8,
>  			  this_size_bits, bits_big_endian);
>  	  }
>  	  break;
> @@ -1927,7 +1930,6 @@ write_pieced_value (struct value *to, struct 
> value *from)
>    struct piece_closure *c
>      = (struct piece_closure *) value_computed_closure (to);
>    size_t type_len;
> -  size_t buffer_size = 0;
>    std::vector<gdb_byte> buffer;
>    int bits_big_endian
>      = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
> @@ -1972,7 +1974,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
> +      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
>        source_offset = source_offset_bits / 8;
>        dest_offset = dest_offset_bits / 8;
>        if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
> @@ -1983,11 +1985,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>  	}
>        else
>  	{
> -	  if (buffer_size < this_size)
> -	    {
> -	      buffer_size = this_size;
> -	      buffer.reserve (buffer_size);
> -	    }
> +	  buffer.reserve (this_size);
>  	  source_buffer = buffer.data ();
>  	  need_bitwise = 1;
>  	}
> @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>  	    struct gdbarch *arch = get_frame_arch (frame);
>  	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
> -	    int reg_offset = dest_offset;
> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& this_size <= register_size (arch, gdb_regnum))
> +		&& p->size <= reg_bits)
>  	      {
>  		/* Big-endian, and we want less than full size.  */
> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
> +		dest_offset_bits += reg_bits - p->size;
>  	      }
> +	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
> +	    buffer.reserve (this_size);
> 
> -	    if (need_bitwise)
> +	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)

I thought the "need_bitwise" variable name helped to understand.  To 
compensate, could you add a comment explaining the "why" of that if?

>  	      {
> +		/* Need some bits from original register value.  */
>  		int optim, unavail;
> 
> -		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
> +		if (!get_frame_register_bytes (frame, gdb_regnum,
> +					       dest_offset_bits / 8,
>  					       this_size, buffer.data (),
>  					       &optim, &unavail))
>  		  {
> @@ -2027,14 +2029,14 @@ write_pieced_value (struct value *to, struct
> value *from)
>  				     "bitfield; containing word "
>  				     "is unavailable"));
>  		  }
> -		copy_bitwise (buffer.data (), dest_offset_bits,
> -			      contents, source_offset_bits,
> -			      this_size_bits,
> -			      bits_big_endian);
>  	      }
> 
> -	    put_frame_register_bytes (frame, gdb_regnum, reg_offset,
> -				      this_size, source_buffer);
> +	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
> +			  contents, source_offset_bits,
> +			  this_size_bits, bits_big_endian);
> +	    put_frame_register_bytes (frame, gdb_regnum,
> +				      dest_offset_bits / 8,
> +				      this_size, buffer.data ());
>  	  }
>  	  break;
>  	case DWARF_VALUE_MEMORY:
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> index c6abc87..4787dfb 100644
> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -192,6 +192,18 @@ Dwarf::assemble $asm_file {
>  			piece 1
>  		    } SPECIAL_expr}
>  		}
> +		# Register pieces for bitfield access.
> +		DW_TAG_variable {
> +		    {name "t2"}
> +		    {type :$struct_t_label}
> +		    {location {
> +			piece 4
> +			regx [lindex $dwarf_regnum 0]
> +			piece 3
> +			regx [lindex $dwarf_regnum 1]
> +			piece 1
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -206,6 +218,15 @@ if ![runto_main] {
>      return -1
>  }
> 
> +# Determine endianness.
> +set endian "little"
> +gdb_test_multiple "show endian" "determine endianness" {
> +    -re ".* (big|little) endian.*$gdb_prompt $" {
> +	set endian $expect_out(1,string)
> +	pass "endianness: $endian"
> +    }
> +}

"pass" should be passed the test name, which should be stable as much as 
possible.  The typical way to do it would be:

   # Determine endianness.
   set endian "little"
   set test "determine endianness"
   gdb_test_multiple "show endian" $test {
       -re ".* (big|little) endian.*$gdb_prompt $" {
           set endian $expect_out(1,string)
           pass $test
       }
   }

> +
>  # Byte-aligned memory pieces.
>  gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
>      "s1 == re-ordered buf"
> @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234"
>  gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 
> 340\\}" \
>      "verify t1"
> 
> +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }

I had to draw this in order to understand, so it might be useful to 
others.

For little endian (assuming the register is 32 bits):

             3    f     f    e     0    0
xxxx xxxx  0011 1111  1111 1110  0000 0000
            ^^                    ^^^^ ^^^^
            | ^^ ^^^^  ^^^^ ^^^   ` x
            | `- y
            `- part of z


For big endian (assuming the register is 32 bits):

             0    0     7    f     f    c
xxxx xxxx  0000 0000  0111 1111  1111 1100
            ^^^^ ^^^^  ^                 ^^
            `- x        ^^^ ^^^^  ^^^^ ^^ `- part of z
                        `- y

Not having worked with a big endian machine before, I'm still confused 
in how bits are aligned in the register and which bit means what.

> +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
> +    "init t2, first piece"
> +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
> +    "init t2, second piece"
> +gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z = 
> 0\\}" \
> +    "initialized t2 from regs"
> +gdb_test_no_output "set var t2.y = 2641"
> +gdb_test_no_output "set var t2.z = -400"
> +gdb_test_no_output "set var t2.x = 200"
> +gdb_test "print t2.x + t2.y + t2.z" " = 2441"

Thanks,

Simon

Re: [PATCH 7/9] Improve logic for buffer allocation in read/write_pieced_value

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> So far the main loop in read_pieced_value and write_pieced_value is
> structured like this:
> 
> (1) Prepare a buffer and some variables we may need;
> 
> (2) depending on the DWARF piece type to be handled, use the buffer and
>     the prepared variables, ignore them, or even recalculate them.
> 
> This approach reduces readability and may also lead to unnecessary 
> copying
> of data.  This patch moves the preparations to the places where 
> sufficient
> information is available and removes some of the variables involved.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (read_pieced_value): Move the buffer allocation and
> 	some other preparations to the places where sufficient information
> 	is available.
> 	(write_pieced_value): Likewise.
> ---
>  gdb/dwarf2loc.c | 111 
> ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 56 insertions(+), 55 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index d13c0c5..67df598 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1794,8 +1794,7 @@ read_pieced_value (struct value *v)
>      {
>        struct dwarf_expr_piece *p = &c->pieces[i];
>        size_t this_size, this_size_bits;
> -      long dest_offset_bits, source_offset_bits, source_offset;
> -      const gdb_byte *intermediate_buffer;
> +      long dest_offset_bits, source_offset_bits;
> 
>        /* Compute size, source, and destination offsets for copying, in
>  	 bits.  */
> @@ -1813,11 +1812,6 @@ read_pieced_value (struct value *v)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> -      buffer.reserve (this_size);
> -      source_offset = source_offset_bits / 8;
> -      intermediate_buffer = buffer.data ();
> -
>        /* Copy from the source to DEST_BUFFER.  */
>        switch (p->location)
>  	{
> @@ -1843,13 +1837,11 @@ read_pieced_value (struct value *v)
>  					  this_size, buffer.data (),
>  					  &optim, &unavail))
>  	      {
> -		/* Just so garbage doesn't ever shine through.  */
> -		memset (buffer.data (), 0, this_size);
> -
>  		if (optim)
>  		  mark_value_bits_optimized_out (v, offset, this_size_bits);
>  		if (unavail)
>  		  mark_value_bits_unavailable (v, offset, this_size_bits);
> +		break;
>  	      }
>  	    copy_bitwise (contents, dest_offset_bits,
>  			  buffer.data (), source_offset_bits % 8,
> @@ -1858,12 +1850,15 @@ read_pieced_value (struct value *v)
>  	  break;
> 
>  	case DWARF_VALUE_MEMORY:
> +	  this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> +	  buffer.reserve (this_size);
> +
>  	  read_value_memory (v, offset,
>  			     p->v.mem.in_stack_memory,
> -			     p->v.mem.addr + source_offset,
> +			     p->v.mem.addr + source_offset_bits / 8,
>  			     buffer.data (), this_size);
>  	  copy_bitwise (contents, dest_offset_bits,
> -			intermediate_buffer, source_offset_bits % 8,
> +			buffer.data (), source_offset_bits % 8,
>  			this_size_bits, bits_big_endian);
>  	  break;
> 
> @@ -1889,17 +1884,18 @@ read_pieced_value (struct value *v)
> 
>  	case DWARF_VALUE_LITERAL:
>  	  {
> -	    size_t n = this_size;
> -
> -	    if (n > p->v.literal.length - source_offset)
> -	      n = (p->v.literal.length >= source_offset
> -		   ? p->v.literal.length - source_offset
> -		   : 0);
> -	    if (n != 0)
> -	      intermediate_buffer = p->v.literal.data + source_offset;
> +	    ULONGEST obj_size = 8 * p->v.literal.length;

I think this variable should be named "literal_size_bits".  And now that 
I see it, the obj_size variable in the DWARF_VALUE_STACK case (in a 
previous patch) should be named "stack_element_size_bits".

> +	    size_t n = this_size_bits;
> +
> +	    /* Cut off at the end of the implicit value.  */
> +	    if (source_offset_bits >= obj_size)
> +	      break;
> +	    if (n > obj_size - source_offset_bits)
> +	      n = obj_size - source_offset_bits;
> +
>  	    copy_bitwise (contents, dest_offset_bits,
> -			  intermediate_buffer, source_offset_bits % 8,
> -			  this_size_bits, bits_big_endian);
> +			  p->v.literal.data, source_offset_bits,
> +			  n, bits_big_endian);
>  	  }
>  	  break;
> 
> @@ -1956,9 +1952,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>      {
>        struct dwarf_expr_piece *p = &c->pieces[i];
>        size_t this_size_bits, this_size;
> -      long dest_offset_bits, source_offset_bits, dest_offset, 
> source_offset;
> -      int need_bitwise;
> -      const gdb_byte *source_buffer;
> +      long dest_offset_bits, source_offset_bits;
> 
>        this_size_bits = p->size;
>        if (bits_to_skip > 0 && bits_to_skip >= this_size_bits)
> @@ -1974,22 +1968,6 @@ write_pieced_value (struct value *to, struct 
> value *from)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
> -      source_offset = source_offset_bits / 8;
> -      dest_offset = dest_offset_bits / 8;
> -      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
> -	  && this_size_bits % 8 == 0)
> -	{
> -	  source_buffer = contents + source_offset;
> -	  need_bitwise = 0;
> -	}
> -      else
> -	{
> -	  buffer.reserve (this_size);
> -	  source_buffer = buffer.data ();
> -	  need_bitwise = 1;
> -	}
> -
>        switch (p->location)
>  	{
>  	case DWARF_VALUE_REGISTER:
> @@ -2040,21 +2018,44 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	  }
>  	  break;
>  	case DWARF_VALUE_MEMORY:
> -	  if (need_bitwise)
> -	    {
> -	      /* Only the first and last bytes can possibly have any
> -		 bits reused.  */
> -	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
> -	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
> -			   &buffer[this_size - 1], 1);
> -	      copy_bitwise (buffer.data (), dest_offset_bits % 8,
> -			    contents, source_offset_bits,
> -			    this_size_bits,
> -			    bits_big_endian);
> -	    }
> +	  {
> +	    CORE_ADDR start_addr = p->v.mem.addr + dest_offset_bits / 8;
> 
> -	  write_memory (p->v.mem.addr + dest_offset,
> -			source_buffer, this_size);
> +	    if (dest_offset_bits % 8 == 0 && this_size_bits % 8 == 0
> +		&& source_offset_bits % 8 == 0)
> +	      {
> +		/* Everything is byte-aligned; no buffer needed.  */
> +		write_memory (start_addr,
> +			      contents + source_offset_bits / 8,
> +			      this_size_bits / 8);
> +		break;
> +	      }
> +
> +	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
> +	    buffer.reserve (this_size);
> +
> +	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
> +	      {
> +		if (this_size <= 8)
> +		  {
> +		    /* Perform a single read for small sizes.  */
> +		    read_memory (start_addr, buffer.data (), this_size);
> +		  }
> +		else
> +		  {
> +		    /* Only the first and last bytes can possibly have any
> +		       bits reused.  */
> +		    read_memory (start_addr, buffer.data (), 1);
> +		    read_memory (start_addr + this_size - 1,
> +				 &buffer[this_size - 1], 1);
> +		  }
> +	      }
> +
> +	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
> +			  contents, source_offset_bits,
> +			  this_size_bits, bits_big_endian);
> +	    write_memory (start_addr, buffer.data (), this_size);
> +	  }

I believe that the complexity of read_pieced_value/write_pieced_value 
could be greatly reduced but introducing functions such as:

  - read_value_memory_bits
  - write_memory_bits
  - read_value_register_bits
  - write_register_bits

It would hide a lot of nastiness behind clear interfaces.  Those 
functions could in turn use smaller functions that could be unit-tested.

I still like the current patch the way it is, as it takes relatively 
small steps.  Breaking it up in smaller functions could be done as an 
additional patch to this series or a follow-up.

>  	  break;
>  	default:
>  	  mark_value_bytes_optimized_out (to, 0, TYPE_LENGTH (value_type 
> (to)));

Thanks,

Simon

Re: [PATCH 8/9] Respect piece offset for DW_OP_bit_piece

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> So far GDB ignores the piece offset of all kinds of DWARF bit
> pieces (DW_OP_bit_piece) and treats such pieces as if the offset was 
> zero.
> 
> This is fixed, and an appropriate test is added.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (read_pieced_value): Respect the piece offset, as
> 	given by DW_OP_bit_piece.
> 	(write_pieced_value): Likewise.
> 
>   Andreas Arnez  <arnez@linux.vnet.ibm.com>
> 
> 	* gdb.dwarf2/var-access.exp: Add test for composite location with
> 	nonzero piece offsets.
> ---
>  gdb/dwarf2loc.c                         | 22 ++++++++++++++------
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 37 
> +++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 67df598..045c2d3 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1824,11 +1824,13 @@ read_pieced_value (struct value *v)
>  	    int optim, unavail;
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& p->size < reg_bits)
> +		&& p->offset + p->size < reg_bits)
>  	      {
>  		/* Big-endian, and we want less than full size.  */
> -		source_offset_bits += reg_bits - p->size;
> +		source_offset_bits += reg_bits - (p->offset + p->size);
>  	      }
> +	    else
> +	      source_offset_bits += p->offset;
>  	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
>  	    buffer.reserve (this_size);
> 
> @@ -1850,6 +1852,7 @@ read_pieced_value (struct value *v)
>  	  break;
> 
>  	case DWARF_VALUE_MEMORY:
> +	  source_offset_bits += p->offset;
>  	  this_size = bits_to_bytes (source_offset_bits, this_size_bits);
>  	  buffer.reserve (this_size);
> 
> @@ -1869,12 +1872,14 @@ read_pieced_value (struct value *v)
>  	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
> 
>  	    /* Use zeroes if piece reaches beyond stack value.  */
> -	    if (p->size > obj_size)
> +	    if (p->offset + p->size > obj_size)
>  	      break;
> 
>  	    /* Piece is anchored at least significant bit end.  */
>  	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
> -	      source_offset_bits += obj_size - p->size;
> +	      source_offset_bits += obj_size - (p->offset + p->size);
> +	    else
> +	      source_offset_bits += p->offset;
>  	    copy_bitwise (contents, dest_offset_bits,
>  			  value_contents_all (p->v.value),
>  			  source_offset_bits,
> @@ -1888,6 +1893,7 @@ read_pieced_value (struct value *v)
>  	    size_t n = this_size_bits;
> 
>  	    /* Cut off at the end of the implicit value.  */
> +	    source_offset_bits += p->offset;
>  	    if (source_offset_bits >= obj_size)
>  	      break;
>  	    if (n > obj_size - source_offset_bits)
> @@ -1978,11 +1984,13 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& p->size <= reg_bits)
> +		&& p->offset + p->size < reg_bits)
>  	      {
>  		/* Big-endian, and we want less than full size.  */
> -		dest_offset_bits += reg_bits - p->size;
> +		dest_offset_bits += reg_bits - (p->offset + p->size);
>  	      }
> +	    else
> +	      dest_offset_bits += p->offset;
>  	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
>  	    buffer.reserve (this_size);
> 
> @@ -2019,6 +2027,8 @@ write_pieced_value (struct value *to, struct 
> value *from)
>  	  break;
>  	case DWARF_VALUE_MEMORY:
>  	  {
> +	    dest_offset_bits += p->offset;
> +
>  	    CORE_ADDR start_addr = p->v.mem.addr + dest_offset_bits / 8;
> 
>  	    if (dest_offset_bits % 8 == 0 && this_size_bits % 8 == 0
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> index 4787dfb..29d61f8 100644
> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -204,6 +204,22 @@ Dwarf::assemble $asm_file {
>  			piece 1
>  		    } SPECIAL_expr}
>  		}
> +		# One piece per bitfield.  Use piece offsets.
> +		DW_TAG_variable {
> +		    {name "t3"}
> +		    {type :$struct_t_label}
> +		    {location {
> +			implicit_value 0x12 0x34 0x56 0x78 0x9a
> +			bit_piece 32 4
> +			const2s -280
> +			stack_value
> +			bit_piece 9 2
> +			regx [lindex $dwarf_regnum 0]
> +			bit_piece 13 14
> +			addr $buf_var
> +			bit_piece 10 42
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -276,3 +292,24 @@ gdb_test_no_output "set var t2.y = 2641"
>  gdb_test_no_output "set var t2.z = -400"
>  gdb_test_no_output "set var t2.x = 200"
>  gdb_test "print t2.x + t2.y + t2.z" " = 2441"
> +
> +# Bitfield access through pieces with nonzero piece offsets.
> +gdb_test_no_output "set var \$[lindex $regname 0] = 0xa8000" \
> +    "init reg for t3.y"
> +gdb_test_no_output "set var *(char \[2\] *) (a + 5) = { 70, 82 }" \
> +    "init mem for t3.z"
> +switch $endian {
> +    little {set val "u = -1484430527, x = -70, y = 42, z = 145"}
> +    big {set val "u = 591751049, x = -70, y = 42, z = 101"}
> +}

Just a nit, I think this (at least the part about u) would be slightly 
easier to understand if the value was printed in hex, like so (for 
little endian):

   u = 0xa7856341, x = 0xffffffba, y = 0x2a, z = 0x91

since we can recognize the nibbles from the implicit_value up there.

> +gdb_test "print t3" " = \\{$val\\}" \
> +    "initialized t3 from reg and mem"
> +gdb_test_no_output "set var t3.y = -1" \
> +    "overwrite t3.y"
> +gdb_test "print/x \$[lindex $regname 0]" " = 0x7ffc000" \
> +    "verify t3.y through reg"
> +gdb_test_no_output "set var t3.z = -614" \
> +    "overwrite t3.z"
> +switch $endian {big {set val "0x59, 0xa2"} little {set val "0x6a, 
> 0x56"}}
> +gdb_test "print/x *(char \[2\] *) (a + 5)" " = \\{$val\\}" \
> +    "verify t3.z through mem"

Re: [PATCH 9/9] Remove unnecessary copies of variables in read/write_pieced_value

[Simon Marchi]

On 2017-04-07 13:38, Andreas Arnez wrote:
> In read_pieced_value's main loop, the variables `dest_offset_bits' and
> `source_offset_bits' are basically just copies of `offset' and
> `bits_to_skip', respectively.  In write_pieced_value the copies are
> reversed.  This is not very helpful when trying to keep the logic 
> between
> these functions in sync.  Since the copies are unnecessary, this patch
> just removes them.

I agree about dest_offset_bits being the same as offset in 
read_pieced_value and source_offset_bits being the same as offset in 
write_pieced_value, I had also identified this as a potential 
simplification.  But I'd keep the names 
source_offset_bits/dest_offset_bits rather than offset.  Or maybe 
offset_in_value_bits, something like that.

Also, it might be clearer if bits_to_skip was named source_bits_to_skip 
in read_pieced_value and dest_bits_to_skip in write_pieced_value, to 
clarify what is skipped exactly.

Thanks,

Simon

Re: [PATCH 3/9] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Andreas Arnez]

On Thu, Apr 13 2017, Simon Marchi wrote:

> On 2017-04-07 13:38, Andreas Arnez wrote:
>> When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value,
>> the
>> existing logic always takes the piece from the lowest-addressed end,
>> which
>> is wrong on big-endian targets.
>
> I'd like if you could clarify this (just here not necessarily in the
> patch).  DWARF locations are computed inside GDB, on the host.  So does it
> really depend on the target endianness, or it's that of the host, or both?
>
> Let's consider these cases of remote debugging:
>
>  host -> target
>  x86  -> x86
>  x86  -> s390
>  s390 -> x86
>  s390 -> s390
>
> In which cases is the value found at the high memory address vs low memory
> address?

DWARF stack values are represented in the target architecture's
endianness, independent from the host.  For instance, if the target is
x86, then the DWARF stack uses little-endian byte order, even on s390
hosts.  See also dwarf_expr_context::execute_stack_op().

[...]

>>
>>  	case DWARF_VALUE_STACK:
>>  	  {
>> -	    size_t n = this_size;
>> +	    struct objfile *objfile = dwarf2_per_cu_objfile (c->per_cu);
>> +	    struct gdbarch *objfile_gdbarch = get_objfile_arch (objfile);
>> +	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>
> It would be really nice for the readers if you could put some comment like
> this, even though it may seem obvious to you:
>
>   /* The size of a DWARF stack value.  */
>   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>
> I found I had to add them to the code to be able to follow.

OK, but it seems that the variable name 'obj_size' causes the confusion;
so I'd rather skip the comment, change the variable name to
stack_value_size_bits instead and let it speak for itself.

>
>>
>> -	    if (n > c->addr_size - source_offset)
>> -	      n = (c->addr_size >= source_offset
>> -		   ? c->addr_size - source_offset
>> -		   : 0);
>> -	    if (n == 0)
>> -	      {
>> -		/* Nothing.  */
>> -	      }
>> -	    else
>> -	      {
>> -		const gdb_byte *val_bytes = value_contents_all (p->v.value);
>
>> +	    /* Use zeroes if piece reaches beyond stack value.  */
>> +	    if (p->size > obj_size)
>> +	      break;
>
> Does this happen, for example, if a DWARF stack value is 32 bits long, but
> the piece is 64 bits?  I suppose that's not something we'd want a compiler
> to emit, and would be considered a bug in the compiler?

Yes, right now I would consider it a compiler bug.  And I haven't seen a
compiler emit such DWARF code yet.  This is just to avoid crashing GDB,
and to behave predictably instead if it occurs anyway.  (If you're
interested, I've written more thoughts about this topic in section 5,
"padding", of this article:
https://sourceware.org/ml/gdb/2016-01/msg00013.html)

>
> How does breaking out of the loop will use zeroes?  Is the value buffer
> cleared beforehand?

Yes, value_contents_raw returns a zeroed buffer.

>
>>
>> -		intermediate_buffer = val_bytes + source_offset;
>> -	      }
>> +	    /* Piece is anchored at least significant bit end.  */
>> +	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
>> +	      source_offset_bits += obj_size - p->size;
>
> Just a nit, but I find it more readable when there's an empty line between
> the if and the following lines not included in the if (so here, right
> where I cut the quote).  It reads like two separate sentences:
>
>  - If the byte order is big endian, adjust offset in the source.
>  - Copy bitwise from the source buffer to the destination buffer.

OK.

>
>> +	    copy_bitwise (contents, dest_offset_bits,
>> +			  value_contents_all (p->v.value),
>> +			  source_offset_bits,
>> +			  this_size_bits, bits_big_endian);
>
> Thanks,
>
> Simon

Thanks,
Andreas

Re: [PATCH 3/9] PR gdb/21226: Take DWARF stack value pieces from LSB end

[Simon Marchi]

On 2017-04-18 12:31, Andreas Arnez wrote:
>> It would be really nice for the readers if you could put some comment 
>> like
>> this, even though it may seem obvious to you:
>> 
>>   /* The size of a DWARF stack value.  */
>>   ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>> 
>> I found I had to add them to the code to be able to follow.
> 
> OK, but it seems that the variable name 'obj_size' causes the 
> confusion;
> so I'd rather skip the comment, change the variable name to
> stack_value_size_bits instead and let it speak for itself.

I agree.

Thanks,

Simon

Re: [PATCH 4/9] Remove addr_size field from struct piece_closure

[Andreas Arnez]

On Thu, Apr 13 2017, Simon Marchi wrote:

> On 2017-04-13 05:10, Yao Qi wrote:
>> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>>
>>> The addr_size field in the piece_closure data structure is a relic from
>>> before introducing the typed DWARF stack.  It is obsolete now.  This
>>> patch
>>> removes it.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* dwarf2loc.c (struct piece_closure) <addr_size>: Remove field.
>>> 	(allocate_piece_closure): Drop addr_size parameter.
>>> 	(dwarf2_evaluate_loc_desc_full): Adjust call to
>>> 	allocate_piece_closure.
>>
>> LGTM.
>
> Since you are planning on making a v2, I think it's better if you check it
> in right now to get it out of the way (it could be obvious anyway).

I would, but it depends on the patch before -- "[PATCH 3/9] PR
gdb/21226: Take DWARF stack value pieces from LSB end".

(Maybe it wasn't clear, but the current code for handling DWARF stack
values is slightly more broken than just not taking them from the
correct end.  It also uses the relic "addr_size" instead of the actual
DWARF stack value size to determine whether the piece is contained in
its underlying object.  And if it's *partially* contained, like taking a
1MB piece from a 64-bit stack value, the current code accesses invalid
memory anyway.)

--
Andreas

Re: [PATCH 4/9] Remove addr_size field from struct piece_closure

[Simon Marchi]

On 2017-04-18 13:24, Andreas Arnez wrote:
>> Since you are planning on making a v2, I think it's better if you 
>> check it
>> in right now to get it out of the way (it could be obvious anyway).
> 
> I would, but it depends on the patch before -- "[PATCH 3/9] PR
> gdb/21226: Take DWARF stack value pieces from LSB end".
> 
> (Maybe it wasn't clear, but the current code for handling DWARF stack
> values is slightly more broken than just not taking them from the
> correct end.  It also uses the relic "addr_size" instead of the actual
> DWARF stack value size to determine whether the piece is contained in
> its underlying object.  And if it's *partially* contained, like taking 
> a
> 1MB piece from a 64-bit stack value, the current code accesses invalid
> memory anyway.)

Ah yes indeed, my bad.

Re: [PATCH 2/9] Fix size capping in write_pieced_value

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

> OK, I guess the commit message should be improved a bit.  How about
> this?
>

Hi Andreas,
The description to the logic can go to comments, so that we don't need
to do "git blame/log" to understand the code.

>   A field f in a structure composed of DWARF pieces may be located in
>   multiple pieces, where the first and last of those may contain bits
>   from other fields as well.  So when writing to f, the beginning of the
>   first and the end of the last of those pieces may have to be skipped.
>   But the logic in write_pieced_value for handling one of those pieces
>   is flawed when the first and last piece are the same, i.e., f is
>   contained in a single piece:
>
>     < - - - - - - - - - piece_size - - - - - - - - - ->
>     +-------------------------------------------------+
>     | skipped_bits |   f_bits   | / / / / / / / / / / |
>     +-------------------------------------------------+
>
>   The current logic determines the size of the sub-piece to operate on
>   by limiting the piece size to the bit size of f and then subtracting
>   the skipped bits:
>
>     max (piece_size, f_bits) - skipped_bits
>
>   Instead of:
>
>     max (piece_size - skipped_bits, f_bits)
>

Given this example, the result is the same, which is
"piece_size - skipped_bits", am I missing something?

>   So the resulting sub-piece size is corrupted, leading to wrong
>   handling of this piece in write_pieced_value.
>
>>
>>> logic in write_pieced_value for handling this is flawed when there are
>>> actually bits to skip at the beginning of the first piece: it truncates
>>> the piece size towards the end *before* accounting for the skipped bits
>>> at the beginning instead of the other way around.
>>>
>>> Note that the same bug was already found in read_pieced_value and fixed
>>> there (but not in write_pieced_value), see PR 15391.
>>
>> Can we share the code in write_pieced_value and read_pieced_value?  The
>> code computing offsets and bits should be shared.
>
> Yes.  I have another patch (not posted yet) that merges these two
> functions.  I moved that towards the end of the patch series, so the
> individual fixes can be incremental.
>

I'd like to merge the code first, then don't need to fix the same
problem in two functions read_pieced_value and write_pieced_value (your
patch 4/9 ~ 9/9 touches both two functions).

>> Also, we need more comments in the code to explain these offsets and
>> bits, a diagram about the relationships of these bits and offsets is
>> quite helpful.
>
> OK.  Some of the offset variables are removed by my patches, so I guess
> I'll postpone that to the merged version.  I'll see what I can come up
> with and include it in v2.

Please include it in V2.

-- 
Yao (齐尧)

Re: [PATCH 2/9] Fix size capping in write_pieced_value

[Andreas Arnez]

On Wed, Apr 19 2017, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
>> OK, I guess the commit message should be improved a bit.  How about
>> this?
>>
>
> Hi Andreas,
> The description to the logic can go to comments, so that we don't need
> to do "git blame/log" to understand the code.

Right, I'll add some general explanation and a diagram about the various
bits and offsets (as requested below).

However, most of the commit message explains a specific bug in a piece
of code that won't exist any more.  This aspect doesn't make sense to be
included in the comments, I think.

>
>>   A field f in a structure composed of DWARF pieces may be located in
>>   multiple pieces, where the first and last of those may contain bits
>>   from other fields as well.  So when writing to f, the beginning of the
>>   first and the end of the last of those pieces may have to be skipped.
>>   But the logic in write_pieced_value for handling one of those pieces
>>   is flawed when the first and last piece are the same, i.e., f is
>>   contained in a single piece:
>>
>>     < - - - - - - - - - piece_size - - - - - - - - - ->
>>     +-------------------------------------------------+
>>     | skipped_bits |   f_bits   | / / / / / / / / / / |
>>     +-------------------------------------------------+
>>
>>   The current logic determines the size of the sub-piece to operate on
>>   by limiting the piece size to the bit size of f and then subtracting
>>   the skipped bits:
>>
>>     max (piece_size, f_bits) - skipped_bits
>>
>>   Instead of:
>>
>>     max (piece_size - skipped_bits, f_bits)
>>
>
> Given this example, the result is the same, which is
> "piece_size - skipped_bits", am I missing something?

Argh, the "max" above is obviously meant to be "min".  Sorry for the
confusion.

>
>>   So the resulting sub-piece size is corrupted, leading to wrong
>>   handling of this piece in write_pieced_value.
>>
>>>
>>>> logic in write_pieced_value for handling this is flawed when there are
>>>> actually bits to skip at the beginning of the first piece: it truncates
>>>> the piece size towards the end *before* accounting for the skipped bits
>>>> at the beginning instead of the other way around.
>>>>
>>>> Note that the same bug was already found in read_pieced_value and fixed
>>>> there (but not in write_pieced_value), see PR 15391.
>>>
>>> Can we share the code in write_pieced_value and read_pieced_value?  The
>>> code computing offsets and bits should be shared.
>>
>> Yes.  I have another patch (not posted yet) that merges these two
>> functions.  I moved that towards the end of the patch series, so the
>> individual fixes can be incremental.
>>
>
> I'd like to merge the code first, then don't need to fix the same
> problem in two functions read_pieced_value and write_pieced_value (your
> patch 4/9 ~ 9/9 touches both two functions).

Not sure I understand.  Do you mean to merge the functions first while
preserving existing logic, including all the bugs and differences?  I
had started along this path and gave up on it, because I found it too
complicated.  From that attempt I've concluded that the current approach
is much less error-prone and easier to follow.

>
>>> Also, we need more comments in the code to explain these offsets and
>>> bits, a diagram about the relationships of these bits and offsets is
>>> quite helpful.
>>
>> OK.  Some of the offset variables are removed by my patches, so I guess
>> I'll postpone that to the merged version.  I'll see what I can come up
>> with and include it in v2.
>
> Please include it in V2.

Sure.

--
Andreas

Re: [PATCH 2/9] Fix size capping in write_pieced_value

[Yao Qi]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

>> Hi Andreas,
>> The description to the logic can go to comments, so that we don't need
>> to do "git blame/log" to understand the code.
>
> Right, I'll add some general explanation and a diagram about the various
> bits and offsets (as requested below).
>
> However, most of the commit message explains a specific bug in a piece
> of code that won't exist any more.  This aspect doesn't make sense to be
> included in the comments, I think.
>

OK, no problem.

>>>>> logic in write_pieced_value for handling this is flawed when there are
>>>>> actually bits to skip at the beginning of the first piece: it truncates
>>>>> the piece size towards the end *before* accounting for the skipped bits
>>>>> at the beginning instead of the other way around.
>>>>>
>>>>> Note that the same bug was already found in read_pieced_value and fixed
>>>>> there (but not in write_pieced_value), see PR 15391.
>>>>
>>>> Can we share the code in write_pieced_value and read_pieced_value?  The
>>>> code computing offsets and bits should be shared.
>>>
>>> Yes.  I have another patch (not posted yet) that merges these two
>>> functions.  I moved that towards the end of the patch series, so the
>>> individual fixes can be incremental.
>>>
>>
>> I'd like to merge the code first, then don't need to fix the same
>> problem in two functions read_pieced_value and write_pieced_value (your
>> patch 4/9 ~ 9/9 touches both two functions).
>
> Not sure I understand.  Do you mean to merge the functions first while
> preserving existing logic, including all the bugs and differences?  I

What I meant is that 1) make two parts identical (but not introducing
new bugs) 2) merge the code to a single function, 3) fix the rest of
bugs in the single function,

> had started along this path and gave up on it, because I found it too
> complicated.  From that attempt I've concluded that the current approach
> is much less error-prone and easier to follow.

There may be some complexity I didn't realize.  I don't want to make
your life harder.  If you are comfortable on this approach, fine by me.

-- 
Yao (齐尧)

Re: [PATCH 6/9] Fix handling of DWARF register pieces on big-endian targets

[Andreas Arnez]

On Fri, Apr 14 2017, Simon Marchi wrote:

> On 2017-04-07 13:38, Andreas Arnez wrote:

[...]

>> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
>> index 1f89a08..d13c0c5 100644
>> --- a/gdb/dwarf2loc.c
>> +++ b/gdb/dwarf2loc.c

[...]

>> @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v)
>>        if (this_size_bits > type_len - offset)
>>  	this_size_bits = type_len - offset;
>>
>> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
>> +      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
>> +      buffer.reserve (this_size);
>>        source_offset = source_offset_bits / 8;
>> -      if (buffer_size < this_size)
>> -	{
>> -	  buffer_size = this_size;
>> -	  buffer.reserve (buffer_size);
>> -	}
>
> The removal of buffer_size and this "if" (and equivalent in
> write_pieced_value) seems to be unrelated to the problem you are fixing,
> but rather a simplification that could be done anyway.  If that's the
> case, you should bundle it with the similar changes from the previous
> patch, in a patch that only does some refactoring but not behavior
> changes.

This particular simplification is triggered by the fact that we'd
otherwise have to duplicate this code, now that the buffer reservation
is potentially repeated.  So it is in fact related to the fix.

But you're right that it could be done anyway.  Thus I'll extract a tiny
patch just with that change and insert it before this one.

[...]

>> @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct
>> value *from)
>>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>>  	    struct gdbarch *arch = get_frame_arch (frame);
>>  	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
>> -	    int reg_offset = dest_offset;
>> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
>>
>>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> -		&& this_size <= register_size (arch, gdb_regnum))
>> +		&& p->size <= reg_bits)
>>  	      {
>>  		/* Big-endian, and we want less than full size.  */
>> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
>> +		dest_offset_bits += reg_bits - p->size;
>>  	      }
>> +	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
>> +	    buffer.reserve (this_size);
>>
>> -	    if (need_bitwise)
>> +	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
>
> I thought the "need_bitwise" variable name helped to understand.  To
> compensate, could you add a comment explaining the "why" of that if?

That's what the next comment is intended for.  I can enhance it, though:

		/* Data is copied non-byte-aligned into the register.
		   Need some bits from original register value.  */

>
>>  	      {
>> +		/* Need some bits from original register value.  */
>>  		int optim, unavail;
>>

[...]

>> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> b/gdb/testsuite/gdb.dwarf2/var-access.exp
>> index c6abc87..4787dfb 100644
>> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp

[...]

>> @@ -206,6 +218,15 @@ if ![runto_main] {
>>      return -1
>>  }
>>
>> +# Determine endianness.
>> +set endian "little"
>> +gdb_test_multiple "show endian" "determine endianness" {
>> +    -re ".* (big|little) endian.*$gdb_prompt $" {
>> +	set endian $expect_out(1,string)
>> +	pass "endianness: $endian"
>> +    }
>> +}
>
> "pass" should be passed the test name, which should be stable as much as
> possible.  The typical way to do it would be:
>
>   # Determine endianness.
>   set endian "little"
>   set test "determine endianness"
>   gdb_test_multiple "show endian" $test {
>       -re ".* (big|little) endian.*$gdb_prompt $" {
>           set endian $expect_out(1,string)
>           pass $test
>       }
>   }

The endianness determination should be extracted into a library function
anyway, since it exists many times already.  I'll do that in V2.

>
>> +
>>  # Byte-aligned memory pieces.
>>  gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
>>      "s1 == re-ordered buf"
>> @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234"
>>  gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z =
>> 340\\}" \
>>      "verify t1"
>>
>> +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }
>
> I had to draw this in order to understand, so it might be useful to
> others.
>
> For little endian (assuming the register is 32 bits):
>
>             3    f     f    e     0    0
> xxxx xxxx  0011 1111  1111 1110  0000 0000
>            ^^                    ^^^^ ^^^^
>            | ^^ ^^^^  ^^^^ ^^^   ` x
>            | `- y
>            `- part of z
>
>
> For big endian (assuming the register is 32 bits):
>
>             0    0     7    f     f    c
> xxxx xxxx  0000 0000  0111 1111  1111 1100
>            ^^^^ ^^^^  ^                 ^^
>            `- x        ^^^ ^^^^  ^^^^ ^^ `- part of z
>                        `- y

Very nice :-)

>
> Not having worked with a big endian machine before, I'm still confused in
> how bits are aligned in the register and which bit means what.

Note that your little-endian picture shows bytes in reversed addressing
order, i.e., highest-addressed byte left, and lowest-addressed byte
right.  This is OK, and the Intel ISA does it in the same way, but I
find this aspect of the little-endian notation confusing as well ;-)

>
>> +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
>> +    "init t2, first piece"
>> +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
>> +    "init t2, second piece"
>> +gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z =
>> 0\\}" \
>> +    "initialized t2 from regs"
>> +gdb_test_no_output "set var t2.y = 2641"
>> +gdb_test_no_output "set var t2.z = -400"
>> +gdb_test_no_output "set var t2.x = 200"
>> +gdb_test "print t2.x + t2.y + t2.z" " = 2441"
>
> Thanks,
>
> Simon

Thanks,
Andreas

Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

[Andreas Arnez]

On Fri, Apr 14 2017, Simon Marchi wrote:

> On 2017-04-07 13:38, Andreas Arnez wrote:
>> There are multiple issues in write_pieced_value when dealing with a
>> bit-field as the target value:
>>
>> (1) The number of bits preceding the bit-field is calculated without
>>     considering the relative offset of the value's parent.
>
> If others are wondering when this can happen:
>
> struct s {
>     uint64_t foo;
>     struct {
> 	uint32_t bar;
> 	uint32_t bf : 10;
>     } baz;
> };
>
> If "val" is a struct value representing bf:
>
>  - value_offset(val) == 4 (sizeof bar)
>  - val->parent represents the whole baz structure
>  - value_offset(val->parent) == 8 (sizeof foo)
>
> If bf was a "standard", non-bitfield variable, its offset would be 12
> directly.

Right.  Now that you mention it, I realize that the test case doesn't
cover this yet.  So I'm enhancing it.

>
> There are multiple places that do "value_offset (parent) + value_offset
> (value)" when value is a bitfield.  Isn't it what we want most of the
> time?  If so, I wonder if eventually value_offset shouldn't instead return
> the computed offset, like:
>
> LONGEST
> value_offset (const struct value *value)
> {
>   if (value_bitsize (value))
>     return value->offset + value_offset (value_parent (value));
>   else
>     return value->offset;
> }

Maybe, but what would set_value_offset do then?

To be honest, I don't completely understand the definition of
value_offset, so I decided not to change anything there with this patch.
Maybe we should spend some time refactoring the value API, but I think
this is a separate task.

>
>> (2) On big-endian targets the source value's *most* significant bits are
>>     transferred to the target value, instead of its least significant
>>     bits.
>>
>> (3) The number of bytes containing a portion of the bit-field in a given
>>     piece is calculated with the wrong starting offset; thus the result
>>     may be off by one.
>>
>> (4) When checking whether the data can be transferred byte-wise, the
>>     transfer size is not verified to be byte-aligned.
>>
>> (5) When transferring the data via a buffer, the bit offset within the
>>     target value is not reduced to its sub-byte fraction before using it
>>     as a bit offset into the buffer.
>>
>> These issues are fixed.  For consistency, the affected logic that exists
>> in read_pieced_value as well is changed there in the same way.
>>
>> gdb/ChangeLog:
>>
>> 	* dwarf2loc.c (write_pieced_value): Fix various issues with
>> 	bit-field handling.
>> 	(read_pieced_value): Sync with changes in write_pieced_value.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.dwarf2/var-access.exp: Add test for accessing bit-fields
>> 	whose containing structure is located in several DWARF pieces.
>> ---
>>  gdb/dwarf2loc.c                         | 54
>> +++++++++++++++------------------
>>  gdb/testsuite/gdb.dwarf2/var-access.exp | 50
>> +++++++++++++++++++++++++++++-
>>  2 files changed, 74 insertions(+), 30 deletions(-)
>>
>> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
>> index 76a58a3..1f89a08 100644
>> --- a/gdb/dwarf2loc.c
>> +++ b/gdb/dwarf2loc.c
>> @@ -1775,7 +1775,8 @@ read_pieced_value (struct value *v)
>>    bits_to_skip = 8 * value_offset (v);
>>    if (value_bitsize (v))
>>      {
>> -      bits_to_skip += value_bitpos (v);
>> +      bits_to_skip += (8 * value_offset (value_parent (v))
>> +		       + value_bitpos (v));
>
> I guess this is related to (1).

Right.

>
>>        type_len = value_bitsize (v);
>>      }
>>    else
>> @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
>>  	  bits_to_skip -= this_size_bits;
>>  	  continue;
>>  	}
>> -      if (bits_to_skip > 0)
>> -	{
>> -	  dest_offset_bits = 0;
>> -	  source_offset_bits = bits_to_skip;
>> -	  this_size_bits -= bits_to_skip;
>> -	  bits_to_skip = 0;
>> -	}
>> -      else
>> -	{
>> -	  dest_offset_bits = offset;
>> -	  source_offset_bits = 0;
>> -	}
>> +      source_offset_bits = bits_to_skip;
>> +      this_size_bits -= bits_to_skip;
>> +      bits_to_skip = 0;
>> +      dest_offset_bits = offset;
>> +
>
> Is this snippet related to one of the problems you have described?  It
> seems to me like it's just simplifying the code, but it's not changing the
> behavior.  If that's the case, I'd suggest putting it in its own patch
> (along with its equivalent in write_pieced_value).

This is just to mirror the change in write_pieced_value.  See below for
the rationale of that.

>
>>        if (this_size_bits > type_len - offset)
>>  	this_size_bits = type_len - offset;
>>
>> @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct
>> value *from)
>>    bits_to_skip = 8 * value_offset (to);
>>    if (value_bitsize (to))
>>      {
>> -      bits_to_skip += value_bitpos (to);
>> +      bits_to_skip += (8 * value_offset (value_parent (to))
>> +		       + value_bitpos (to));
>>        type_len = value_bitsize (to);
>> +      /* Use the least significant bits of FROM.  */
>> +      if (gdbarch_byte_order (get_type_arch (value_type (from)))
>> +	  == BFD_ENDIAN_BIG)
>> +	{
>> +	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
>> +	  type_len += offset;
>> +	}
>
> I guess this is related to (1) and (2).

Right.  Note that 'offset' may now be nonzero upon entering the loop.

>
>>      }
>>    else
>>      type_len = 8 * TYPE_LENGTH (value_type (to));
>> @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct
>> value *from)
>>  	  bits_to_skip -= this_size_bits;
>>  	  continue;
>>  	}
>> -      if (bits_to_skip > 0)
>> -	{
>> -	  dest_offset_bits = bits_to_skip;
>> -	  source_offset_bits = 0;
>> -	  this_size_bits -= bits_to_skip;
>> -	  bits_to_skip = 0;
>> -	}
>> -      else
>> -	{
>> -	  dest_offset_bits = 0;
>> -	  source_offset_bits = offset;
>> -	}
>> +      dest_offset_bits = bits_to_skip;
>> +      this_size_bits -= bits_to_skip;
>> +      bits_to_skip = 0;
>> +      source_offset_bits = offset;
>> +

This is related to (2).  The old version assumed that 'offset' is still
zero when there are bits to skip, so a literal zero (instead of
'offset') could be copied into source_offset_bits.  This assumption
doesn't hold any longer, now that 'offset' may start with a nonzero
value.

>>        if (this_size_bits > type_len - offset)
>>  	this_size_bits = type_len - offset;
>>
>> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
>> +      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
>
> I guess this is related to (3).

Yes.  Note that this looks like a classical copy & paste error.  The
line was probably copied from read_pieced_value, where
'source_offset_bits' is actually correct.

>
>>        source_offset = source_offset_bits / 8;
>>        dest_offset = dest_offset_bits / 8;
>> -      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0)
>> +      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
>> +	  && this_size_bits % 8 == 0)
>
> ... and this to (4).

Correct.

>
>>  	{
>>  	  source_buffer = contents + source_offset;
>>  	  need_bitwise = 0;
>> @@ -2049,7 +2045,7 @@ write_pieced_value (struct value *to, struct value
>> *from)
>>  	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
>>  	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
>>  			   &buffer[this_size - 1], 1);
>> -	      copy_bitwise (buffer.data (), dest_offset_bits,
>> +	      copy_bitwise (buffer.data (), dest_offset_bits % 8,
>
> ... and this to (5).

Also correct.

As you can see, the patch fixes 5 different, fairly independent bugs.
Thus it could have been split into 5 patches, but I found that a bit
extreme...

>
>>  			    contents, source_offset_bits,
>>  			    this_size_bits,
>>  			    bits_big_endian);
>> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> b/gdb/testsuite/gdb.dwarf2/var-access.exp
>> index 56a635a..c6abc87 100644
>> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
>> @@ -62,7 +62,7 @@ Dwarf::assemble $asm_file {
>>  	} {
>>  	    declare_labels char_type_label
>>  	    declare_labels int_type_label short_type_label
>> -	    declare_labels array_a8_label struct_s_label
>> +	    declare_labels array_a8_label struct_s_label struct_t_label
>>
>>  	    char_type_label: base_type {
>>  		{name "char"}
>> @@ -111,6 +111,34 @@ Dwarf::assemble $asm_file {
>>  		}
>>  	    }
>>
>> +	    struct_t_label: structure_type {
>> +		{name "t"}
>> +		{byte_size 8 DW_FORM_sdata}
>> +	    } {
>> +		member {
>> +		    {name u}
>> +		    {type :$int_type_label}
>> +		}
>> +		member {
>> +		    {name x}
>> +		    {type :$int_type_label}
>> +		    {data_member_location 4 DW_FORM_udata}
>> +		    {bit_size 9 DW_FORM_udata}
>> +		}
>> +		member {
>> +		    {name y}
>> +		    {type :$int_type_label}
>> +		    {data_bit_offset 41 DW_FORM_udata}
>> +		    {bit_size 13 DW_FORM_udata}
>> +		}
>> +		member {
>> +		    {name z}
>> +		    {type :$int_type_label}
>> +		    {data_bit_offset 54 DW_FORM_udata}
>> +		    {bit_size 10 DW_FORM_udata}
>> +		}
>> +	    }
>> +
>>  	    DW_TAG_subprogram {
>>  		{name "main"}
>>  		{DW_AT_external 1 flag}
>> @@ -152,6 +180,18 @@ Dwarf::assemble $asm_file {
>>  			piece 1
>>  		    } SPECIAL_expr}
>>  		}
>> +		# Memory pieces for bitfield access.
>> +		DW_TAG_variable {
>> +		    {name "t1"}
>> +		    {type :$struct_t_label}
>> +		    {location {
>> +			piece 4
>> +			addr "$buf_var + 1"
>> +			piece 3
>> +			addr "$buf_var"
>> +			piece 1
>> +		    } SPECIAL_expr}
>> +		}
>>  	    }
>>  	}
>>      }
>> @@ -196,3 +236,11 @@ gdb_test_no_output "set var s2 = {191, 73, 231,
>> 123}" \
>>      "re-initialize s2"
>>  gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
>>      "verify re-initialized s2"
>> +
>> +# Unaligned bitfield access through byte-aligned pieces.
>> +gdb_test_no_output "set var t1.x = -7"
>> +gdb_test_no_output "set var t1.z = 340"
>> +gdb_test_no_output "set var t1.y = 1234"
>> +gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z =
>> 340\\}" \
>> +    "verify t1"
>
> Maybe I'm missing something, but if there's the same bug in the write and
> read implementations, it's possible it would slip through, isn't it? For
> example, if the "set var" part messes up the bit ordering and the "print"
> part messes it up the same way, it would appear correct when it's not.
> Reading actual data from a progam won't work.
>
> In the other tests, you tested against known data already in memory, which
> made sense I think.

Yeah, OK, I can do that.

--
Andreas

Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

[Simon Marchi]

On 2017-04-27 13:54, Andreas Arnez wrote:
> On Fri, Apr 14 2017, Simon Marchi wrote:
> 
>> On 2017-04-07 13:38, Andreas Arnez wrote:
>>> There are multiple issues in write_pieced_value when dealing with a
>>> bit-field as the target value:
>>> 
>>> (1) The number of bits preceding the bit-field is calculated without
>>>     considering the relative offset of the value's parent.
>> 
>> If others are wondering when this can happen:
>> 
>> struct s {
>>     uint64_t foo;
>>     struct {
>> 	uint32_t bar;
>> 	uint32_t bf : 10;
>>     } baz;
>> };
>> 
>> If "val" is a struct value representing bf:
>> 
>>  - value_offset(val) == 4 (sizeof bar)
>>  - val->parent represents the whole baz structure
>>  - value_offset(val->parent) == 8 (sizeof foo)
>> 
>> If bf was a "standard", non-bitfield variable, its offset would be 12
>> directly.
> 
> Right.  Now that you mention it, I realize that the test case doesn't
> cover this yet.  So I'm enhancing it.

I did not realize that, but I'm glad you did :).

>> 
>> There are multiple places that do "value_offset (parent) + 
>> value_offset
>> (value)" when value is a bitfield.  Isn't it what we want most of the
>> time?  If so, I wonder if eventually value_offset shouldn't instead 
>> return
>> the computed offset, like:
>> 
>> LONGEST
>> value_offset (const struct value *value)
>> {
>>   if (value_bitsize (value))
>>     return value->offset + value_offset (value_parent (value));
>>   else
>>     return value->offset;
>> }
> 
> Maybe, but what would set_value_offset do then?

Indeed, it's probably better not to break the symmetry.

> To be honest, I don't completely understand the definition of
> value_offset, so I decided not to change anything there with this 
> patch.
> Maybe we should spend some time refactoring the value API, but I think
> this is a separate task.

Yes, it was just brainstorming for future enhancements.

>>>        type_len = value_bitsize (v);
>>>      }
>>>    else
>>> @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
>>>  	  bits_to_skip -= this_size_bits;
>>>  	  continue;
>>>  	}
>>> -      if (bits_to_skip > 0)
>>> -	{
>>> -	  dest_offset_bits = 0;
>>> -	  source_offset_bits = bits_to_skip;
>>> -	  this_size_bits -= bits_to_skip;
>>> -	  bits_to_skip = 0;
>>> -	}
>>> -      else
>>> -	{
>>> -	  dest_offset_bits = offset;
>>> -	  source_offset_bits = 0;
>>> -	}
>>> +      source_offset_bits = bits_to_skip;
>>> +      this_size_bits -= bits_to_skip;
>>> +      bits_to_skip = 0;
>>> +      dest_offset_bits = offset;
>>> +
>> 
>> Is this snippet related to one of the problems you have described?  It
>> seems to me like it's just simplifying the code, but it's not changing 
>> the
>> behavior.  If that's the case, I'd suggest putting it in its own patch
>> (along with its equivalent in write_pieced_value).
> 
> This is just to mirror the change in write_pieced_value.  See below for
> the rationale of that.
> 
>> 
>>>        if (this_size_bits > type_len - offset)
>>>  	this_size_bits = type_len - offset;
>>> 
>>> @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct
>>> value *from)
>>>    bits_to_skip = 8 * value_offset (to);
>>>    if (value_bitsize (to))
>>>      {
>>> -      bits_to_skip += value_bitpos (to);
>>> +      bits_to_skip += (8 * value_offset (value_parent (to))
>>> +		       + value_bitpos (to));
>>>        type_len = value_bitsize (to);
>>> +      /* Use the least significant bits of FROM.  */
>>> +      if (gdbarch_byte_order (get_type_arch (value_type (from)))
>>> +	  == BFD_ENDIAN_BIG)
>>> +	{
>>> +	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
>>> +	  type_len += offset;
>>> +	}
>> 
>> I guess this is related to (1) and (2).
> 
> Right.  Note that 'offset' may now be nonzero upon entering the loop.
> 
>> 
>>>      }
>>>    else
>>>      type_len = 8 * TYPE_LENGTH (value_type (to));
>>> @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct
>>> value *from)
>>>  	  bits_to_skip -= this_size_bits;
>>>  	  continue;
>>>  	}
>>> -      if (bits_to_skip > 0)
>>> -	{
>>> -	  dest_offset_bits = bits_to_skip;
>>> -	  source_offset_bits = 0;
>>> -	  this_size_bits -= bits_to_skip;
>>> -	  bits_to_skip = 0;
>>> -	}
>>> -      else
>>> -	{
>>> -	  dest_offset_bits = 0;
>>> -	  source_offset_bits = offset;
>>> -	}
>>> +      dest_offset_bits = bits_to_skip;
>>> +      this_size_bits -= bits_to_skip;
>>> +      bits_to_skip = 0;
>>> +      source_offset_bits = offset;
>>> +
> 
> This is related to (2).  The old version assumed that 'offset' is still
> zero when there are bits to skip, so a literal zero (instead of
> 'offset') could be copied into source_offset_bits.  This assumption
> doesn't hold any longer, now that 'offset' may start with a nonzero
> value.

Ok, I though it was just some simplification.  The new code is 
equivalent to both branches of the if/else that's being removed for 
dest_offset_bits, this_size_bits and bits_to_skip.  But you're right, 
for source_offset_bits it changes the behaviour.  In any case, I like 
the new code better, less branches :).

> As you can see, the patch fixes 5 different, fairly independent bugs.
> Thus it could have been split into 5 patches, but I found that a bit
> extreme...

I think it's worth splitting it*, it will help understand each issue and 
corresponding fix independently.  It's some non-trivial work you're 
doing here, so think about external observers that want to take a look 
at it :). Patches are cheap after all.

*unless it adds to much complexity to have these intermediary states.

Thanks,

Simon