PATCH [0/2] Convert unavailable vector to be bit, not byte, based.

PATCH [0/2] Convert unavailable vector to be bit, not byte, based.

[Andrew Burgess]

This patch set is inspired by two previous patches I've posted:

  https://sourceware.org/ml/gdb-patches/2013-08/msg00305.html
  https://sourceware.org/ml/gdb-patches/2013-08/msg00309.html

but I think can exist as a separate piece of work.

The vector of unavailable parts of a value is currently byte based.
Given that we can model a value down to the bit level, we can
potentially loose information with the current implementation.  After
the patch we model the unavailable information in bits.

Tested on x86-64 linux, including against native gdbserver.

Thanks,
Andrew

[PATCH [1/2] Add support for DW_OP_bit_piece and DW_OP_plus_uconst to DWARF assembler.

[Andrew Burgess]

Add some additional DW_OP_ support to the DWARF assembler.

OK to apply?

Thanks,
Andrew

gdb/testsuite/ChangeLog

	* lib/gdb.exp (_location): Handle DW_OP_bit_piece and
	DW_OP_plus_uconst.

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index c28b986..e916477 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -716,10 +716,19 @@ namespace eval Dwarf {
 		    _op .sleb128 [lindex $line 1]
 		}
 
+		DW_OP_plus_uconst {
+		    _op .uleb128 [lindex $line 1]
+		}
+
 		DW_OP_piece {
 		    _op .uleb128 [lindex $line 1]
 		}
 
+		DW_OP_bit_piece {
+		    _op .uleb128 [lindex $line 1]
+		    _op .uleb128 [lindex $line 2]
+		}
+
 		DW_OP_GNU_implicit_pointer {
 		    if {[llength $line] != 3} {
 			error "usage: DW_OP_GNU_implicit_pointer LABEL OFFSET"

[PATCH [0/2] Convert the unavailable vector to be bit, not byte, based.

[Andrew Burgess]

This is the actual conversion, and a test.

OK to apply?

Thanks,
Andrew

    gdb/testfile/ChangeLog
    
    	* gdb.trace/unavailable-dwarf-piece.c: New file.
    	* gdb.trace/unavailable-dwarf-piece.exp: New file.
    
    gdb/ChangeLog
    
    	* gdb/dwarf2loc.c (read_pieced_value): Mark bits, not bytes
    	unavailable, use correct bit length.
    	* gdb/value.c (value_bits_available): New function.
    	(mark_value_bits_unavailable): New function.
    	(mark_value_bytes_unavailable): Move contents to
    	mark_value_bits_unavailable.
    	(value_available_contents_eq): Take account of the unavailable
    	vector being bit based.
    	(value_contents_copy_raw): Likewise.
    	(unpack_value_bits_as_long_1): Check availability in bits, bot
    	bytes.
    	* gdb/value.h (int value_bits_available): Declare new function.
    	(mark_value_bits_unavailable): Declare new function.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 2d15546..ba5f338 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1709,7 +1709,7 @@ read_pieced_value (struct value *v)
 		    if (optim)
 		      set_value_optimized_out (v, 1);
 		    if (unavail)
-		      mark_value_bytes_unavailable (v, offset, this_size);
+		      mark_value_bits_unavailable (v, offset, this_size_bits);
 		  }
 	      }
 	    else
diff --git a/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c
new file mode 100644
index 0000000..f55b094
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c
@@ -0,0 +1,86 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+struct s
+{
+  unsigned char a;
+  unsigned char b;
+  unsigned char c;
+};
+
+struct t
+{
+  /* First, a complete byte.  */
+  unsigned char a;
+  /* Next, 8 single bits.  */
+  unsigned char b : 1;
+  unsigned char c : 1;
+  unsigned char d : 1;
+  unsigned char e : 1;
+  unsigned char f : 1;
+  unsigned char g : 1;
+  unsigned char h : 1;
+  unsigned char i : 1;
+  /* Now another byte.  */
+  unsigned char j;
+};
+
+void
+end ()
+{
+  /* Nothing.  */
+}
+
+void
+dummy ()
+{
+  /* Nothing.  */
+}
+
+int
+foo (struct s x, struct s y, struct s z)
+{
+  dummy ();
+  asm (".global foo_end_lbl\nfoo_end_lbl:");
+  return 0;
+}
+
+int
+bar (struct t x, struct t y, struct t z)
+{
+  dummy ();
+  asm (".global bar_end_lbl\nbar_end_lbl:");
+  return 0;
+}
+
+int
+main ()
+{
+  struct s v = { 0, 1, 2};
+  struct t w = { 5, 0, 1, 0, 1, 0, 1, 0, 1, 7 };
+  int ans;
+
+  ans = foo (v, v, v);
+
+  end ();
+
+  ans = bar (w, w, w);
+
+  end ();
+
+  return ans;
+}
diff --git a/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp
new file mode 100644
index 0000000..ff0614e
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp
@@ -0,0 +1,334 @@
+# Copyright (C) 2013 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/>.
+
+load_lib "trace-support.exp"
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c
+
+set asm_file "${testfile}-dbg.s"
+set opts {}
+
+if  { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
+	   object {nodebug}] != "" } {
+    return -1
+}
+
+Dwarf::assemble $asm_file {
+    declare_labels uchar_label struct_s_label foo_label struct_t_label bar_label
+
+    cu {} {
+	compile_unit {{language @DW_LANG_C}} {
+	    uchar_label: DW_TAG_base_type {
+		{name "unsigned char"}
+		{byte_size 1 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned_char}
+	    }
+
+	    struct_s_label: DW_TAG_structure_type {
+		{name s}
+		{byte_size 3 DW_FORM_sdata}
+		{decl_file 1}
+		{decl_line 1}
+	    } {
+		DW_TAG_member {
+		    {name a}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 0
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name b}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name c}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    struct_t_label: DW_TAG_structure_type {
+		{name t}
+		{byte_size 3 DW_FORM_sdata}
+		{decl_file 1}
+		{decl_line 1}
+	    } {
+		DW_TAG_member {
+		    {name a}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 0
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name b}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 7 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name c}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 6 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name d}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 5 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name e}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 4 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name f}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 3 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name g}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 2 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name h}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 1 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name i}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 0 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name j}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    DW_TAG_subprogram {
+		{name foo}
+		{decl_file 1}
+		{low_pc foo addr}
+		{high_pc foo_end_lbl addr}
+	    } {
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name x}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 2
+			DW_OP_reg0
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name y}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_reg0
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name z}
+		    {location {
+			DW_OP_reg0
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+
+	    DW_TAG_subprogram {
+		{name bar}
+		{decl_file 1}
+		{low_pc bar addr}
+		{high_pc bar_end_lbl addr}
+	    } {
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name x}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 7 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name y}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 3 0
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 4 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name z}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 7 0
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+	    }
+
+	}
+    }
+}
+
+if  { [gdb_compile ${asm_file} ${binfile}2.o object {nodebug}] != "" } {
+    return -1
+}
+
+if  { [gdb_compile [list ${binfile}1.o ${binfile}2.o] ${binfile} \
+	   executable {}] != "" } {
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+gdb_breakpoint "end"
+
+with_test_prefix "tracing foo" {
+    gdb_test "trace foo" ".*"
+    gdb_test_no_output "tstart"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*"
+    gdb_test_no_output "tstop"
+
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+    gdb_test "p x" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = <unavailable>}"
+    gdb_test "p y" "\\\$${decimal} = {a = 0 '\\\\000', b = <unavailable>, c = 0 '\\\\000'}"
+    gdb_test "p z" "\\\$${decimal} = {a = <unavailable>, b = 0 '\\\\000', c = 0 '\\\\000'}"
+
+    gdb_test "tfind none" "No longer looking at any trace frame.*"
+}
+
+with_test_prefix "tracing bar" {
+    gdb_test "trace bar" ".*"
+    gdb_test_no_output "tstart"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*"
+    gdb_test_no_output "tstop"
+
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+    gdb_test "p x" "\\\$${decimal} = {a = 0 '\\\\000', b = <unavailable>, c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
+    gdb_test "p y" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = <unavailable>, f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
+    gdb_test "p z" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = <unavailable>, j = 0 '\\\\000'}"
+
+    gdb_test "tfind none" "No longer looking at any trace frame.*"
+}
diff --git a/gdb/value.c b/gdb/value.c
index a64e7e1..3878f37 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -338,7 +338,7 @@ struct value
 };
 
 int
-value_bytes_available (const struct value *value, int offset, int length)
+value_bits_available (const struct value *value, int offset, int length)
 {
   gdb_assert (!value->lazy);
 
@@ -346,6 +346,14 @@ value_bytes_available (const struct value *value, int offset, int length)
 }
 
 int
+value_bytes_available (const struct value *value, int offset, int length)
+{
+  return value_bits_available (value,
+			       offset * TARGET_CHAR_BIT,
+			       length * TARGET_CHAR_BIT);
+}
+
+int
 value_entirely_available (struct value *value)
 {
   /* We can only tell whether the whole value is available when we try
@@ -379,7 +387,7 @@ value_entirely_unavailable (struct value *value)
 }
 
 void
-mark_value_bytes_unavailable (struct value *value, int offset, int length)
+mark_value_bits_unavailable (struct value *value, int offset, int length)
 {
   range_s newr;
   int i;
@@ -543,6 +551,14 @@ mark_value_bytes_unavailable (struct value *value, int offset, int length)
     }
 }
 
+void
+mark_value_bytes_unavailable (struct value *value, int offset, int length)
+{
+  mark_value_bits_unavailable (value,
+			       offset * TARGET_CHAR_BIT,
+			       length * TARGET_CHAR_BIT);
+}
+
 /* Find the first range in RANGES that overlaps the range defined by
    OFFSET and LENGTH, starting at element POS in the RANGES vector,
    Returns the index into RANGES where such overlapping range was
@@ -572,6 +588,12 @@ value_available_contents_eq (const struct value *val1, int offset1,
   /* See function description in value.h.  */
   gdb_assert (!val1->lazy && !val2->lazy);
 
+  /* The offsets and length parameters are all byte based, but we store the
+     unavailable data in a bit based list, convert now.  */
+  offset1 *= TARGET_CHAR_BIT;
+  offset2 *= TARGET_CHAR_BIT;
+  length *= TARGET_CHAR_BIT;
+
   while (length > 0)
     {
       range_s *r1, *r2;
@@ -585,9 +607,9 @@ value_available_contents_eq (const struct value *val1, int offset1,
 
       /* The usual case is for both values to be completely available.  */
       if (idx1 == -1 && idx2 == -1)
-	return (memcmp (val1->contents + offset1,
-			val2->contents + offset2,
-			length) == 0);
+	return (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
+			val2->contents + (offset2 / TARGET_CHAR_BIT),
+			(length / TARGET_CHAR_BIT)) == 0);
       /* The contents only match equal if the available set matches as
 	 well.  */
       else if (idx1 == -1 || idx2 == -1)
@@ -620,9 +642,9 @@ value_available_contents_eq (const struct value *val1, int offset1,
 	return 0;
 
       /* Compare the _available_ contents.  */
-      if (memcmp (val1->contents + offset1,
-		  val2->contents + offset2,
-		  l1) != 0)
+      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
+		  val2->contents + (offset2 / TARGET_CHAR_BIT),
+		  (l1 / TARGET_CHAR_BIT)) != 0)
 	return 0;
 
       length -= h1;
@@ -972,6 +994,7 @@ value_contents_copy_raw (struct value *dst, int dst_offset,
 {
   range_s *r;
   int i;
+  int src_bit_offset, dst_bit_offset, bit_length;
 
   /* A lazy DST would make that this copy operation useless, since as
      soon as DST's contents were un-lazied (by a later value_contents
@@ -990,17 +1013,20 @@ value_contents_copy_raw (struct value *dst, int dst_offset,
 	  length);
 
   /* Copy the meta-data, adjusted.  */
+  src_bit_offset = src_offset * TARGET_CHAR_BIT;
+  dst_bit_offset = dst_offset * TARGET_CHAR_BIT;
+  bit_length = length * TARGET_CHAR_BIT;
   for (i = 0; VEC_iterate (range_s, src->unavailable, i, r); i++)
     {
       ULONGEST h, l;
 
-      l = max (r->offset, src_offset);
-      h = min (r->offset + r->length, src_offset + length);
+      l = max (r->offset, src_bit_offset);
+      h = min (r->offset + r->length, src_bit_offset + length);
 
       if (l < h)
-	mark_value_bytes_unavailable (dst,
-				      dst_offset + (l - src_offset),
-				      h - l);
+	mark_value_bits_unavailable (dst,
+				     dst_bit_offset + (l - src_bit_offset),
+				     h - l);
     }
 }
 
@@ -2884,8 +2910,8 @@ unpack_value_bits_as_long_1 (struct type *field_type, const gdb_byte *valaddr,
   read_offset = bitpos / 8;
 
   if (original_value != NULL
-      && !value_bytes_available (original_value, embedded_offset + read_offset,
-				 bytes_read))
+      && !value_bits_available (original_value, embedded_offset + bitpos,
+				bitsize))
     return 0;
 
   val = extract_unsigned_integer (valaddr + embedded_offset + read_offset,
diff --git a/gdb/value.h b/gdb/value.h
index db964e3..6b158df 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -436,6 +436,14 @@ extern int value_bits_synthetic_pointer (const struct value *value,
 extern int value_bytes_available (const struct value *value,
 				  int offset, int length);
 
+/* Given a value, determine whether the contents bits starting at
+   OFFSET and extending for LENGTH bits are available.  This returns
+   nonzero if all bits in the given range are available, zero if any
+   bit is unavailable.  */
+
+extern int value_bits_available (const struct value *value,
+				 int offset, int length);
+
 /* Like value_bytes_available, but return false if any byte in the
    whole object is unavailable.  */
 extern int value_entirely_available (struct value *value);
@@ -450,6 +458,12 @@ extern int value_entirely_unavailable (struct value *value);
 extern void mark_value_bytes_unavailable (struct value *value,
 					  int offset, int length);
 
+/* Mark VALUE's content bits starting at OFFSET and extending for
+   LENGTH bits as unavailable.  */
+
+extern void mark_value_bits_unavailable (struct value *value,
+					 int offset, int length);
+
 /* Compare LENGTH bytes of VAL1's contents starting at OFFSET1 with
    LENGTH bytes of VAL2's contents starting at OFFSET2.
 

Re: PATCH [0/2] Convert unavailable vector to be bit, not byte, based.

[Pedro Alves]

On 12/04/2013 03:22 PM, Andrew Burgess wrote:
> This patch set is inspired by two previous patches I've posted:
> 
>   https://sourceware.org/ml/gdb-patches/2013-08/msg00305.html
>   https://sourceware.org/ml/gdb-patches/2013-08/msg00309.html
> 
> but I think can exist as a separate piece of work.

Excellent!  The new test is quite nice work.

> The vector of unavailable parts of a value is currently byte based.
> Given that we can model a value down to the bit level, we can
> potentially loose information with the current implementation.  After
> the patch we model the unavailable information in bits.

It'd be good if this paragraph ended up in the commit log of patch 2.

> 
> Tested on x86-64 linux, including against native gdbserver.

This too.

> 
> Thanks,
> Andrew

-- 
Pedro Alves

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Pedro Alves]

On 12/04/2013 03:26 PM, Andrew Burgess wrote:

>     	* gdb/dwarf2loc.c (read_pieced_value): Mark bits, not bytes
>     	unavailable, use correct bit length.

No "gdb/" prefix.


>     	(unpack_value_bits_as_long_1): Check availability in bits, bot
>     	bytes.

Typo: "bot".

>     	* gdb/value.h (int value_bits_available): Declare new function.

Stale "int".

>     	(mark_value_bits_unavailable): Declare new function.


>  *val1, int offset1,
>  	return 0;
>  
>        /* Compare the _available_ contents.  */
> -      if (memcmp (val1->contents + offset1,
> -		  val2->contents + offset2,
> -		  l1) != 0)
> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
> +		  (l1 / TARGET_CHAR_BIT)) != 0)
>  	return 0;

As memcmp compares bytes, isn't this potentially comparing bits
at the beginning and end of the values' buffers, when it
should not?   That is, it looks like the
'offset1 % TARGET_CHAR_BIT != 0' and
'(offset1 + l1) % TARGET_CHAR_BIT' cases should be considered here?

> +    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
> +    gdb_test "p x" "\\\$${decimal} = {a = 0 '\\\\000', b = <unavailable>, c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
> +    gdb_test "p y" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = <unavailable>, f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
> +    gdb_test "p z" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = <unavailable>, j = 0 '\\\\000'}"

(I think you could have used "p/d" or "p/x" to simplify the output.)

> +
> +    gdb_test "tfind none" "No longer looking at any trace frame.*"


> +void
> +end ()

end (void)

There are more instances.




> +  struct s v = { 0, 1, 2};

Missing space before }.

> +  struct t w = { 5, 0, 1, 0, 1, 0, 1, 0, 1, 7 };


Otherwise looks good to me.

-- 
Pedro Alves

Re: [PATCH [1/2] Add support for DW_OP_bit_piece and DW_OP_plus_uconst to DWARF assembler.

[Pedro Alves]

On 12/04/2013 03:24 PM, Andrew Burgess wrote:
> 	* lib/gdb.exp (_location): Handle DW_OP_bit_piece and

Wrong file, it's dwarf.exp.  Also, write (Dwarf::_location).

> 	DW_OP_plus_uconst.

Looks fine to me, but Tromey may want to take a look.

Thanks,
-- 
Pedro Alves

Re: PATCH [0/2] Convert unavailable vector to be bit, not byte, based.

[Doug Evans]

On Thu, Dec 5, 2013 at 8:17 AM, Pedro Alves <palves@redhat.com> wrote:
> On 12/04/2013 03:22 PM, Andrew Burgess wrote:
>> This patch set is inspired by two previous patches I've posted:
>>
>>   https://sourceware.org/ml/gdb-patches/2013-08/msg00305.html
>>   https://sourceware.org/ml/gdb-patches/2013-08/msg00309.html
>>
>> but I think can exist as a separate piece of work.
>
> Excellent!  The new test is quite nice work.
>
>> The vector of unavailable parts of a value is currently byte based.
>> Given that we can model a value down to the bit level, we can
>> potentially loose information with the current implementation.  After
>> the patch we model the unavailable information in bits.
>
> It'd be good if this paragraph ended up in the commit log of patch 2.

I'd prefer a comment in the code too fwiw.
[and then if one still wants to add a comment to the commit log, go for it]
A simple one-liner explaining why things are the way they are would be
sufficient.

Re: [PATCH [1/2] Add support for DW_OP_bit_piece and DW_OP_plus_uconst to DWARF assembler.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> DW_OP_plus_uconst.

Pedro> Looks fine to me, but Tromey may want to take a look.

It's good.  Thanks Andrew.

Tom

Re: [PATCH [1/2] Add support for DW_OP_bit_piece and DW_OP_plus_uconst to DWARF assembler.

[Andrew Burgess]

On 05/12/2013 6:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> DW_OP_plus_uconst.
> 
> Pedro> Looks fine to me, but Tromey may want to take a look.
> 
> It's good.  Thanks Andrew.

Thanks for the review.  Pushed with the patches that Pedro pointed out.

Andrew

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Andrew Burgess]

On 05/12/2013 4:18 PM, Pedro Alves wrote:
> On 12/04/2013 03:26 PM, Andrew Burgess wrote:
> 
>>  
>>        /* Compare the _available_ contents.  */
>> -      if (memcmp (val1->contents + offset1,
>> -		  val2->contents + offset2,
>> -		  l1) != 0)
>> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
>> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
>> +		  (l1 / TARGET_CHAR_BIT)) != 0)
>>  	return 0;
> 
> As memcmp compares bytes, isn't this potentially comparing bits
> at the beginning and end of the values' buffers, when it
> should not?   That is, it looks like the
> 'offset1 % TARGET_CHAR_BIT != 0' and
> '(offset1 + l1) % TARGET_CHAR_BIT' cases should be considered here?
> 

Thanks for the review.  You're right, this isn't doing the correct thing
here, I should have written something like this:


+      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
+		  val2->contents + (offset2 / TARGET_CHAR_BIT),
+		  ((l1 + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)) != 0)

That is round the start down to the nearest byte boundary, and round the
length up to the nearest byte boundary.

I figured that as the value content buffer is always a round number of
bytes then there will always be memory backing the access, and as the
content buffer is zero initialized comparing the "unavailable" bits will
not cause the compare to fail.

Alternatively, I can update to only access the bits we'd like to compare.

Which approach would would be best?

Thanks for your advice,
Andrew

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Pedro Alves]

On 12/09/2013 09:04 PM, Andrew Burgess wrote:
> On 05/12/2013 4:18 PM, Pedro Alves wrote:
>> On 12/04/2013 03:26 PM, Andrew Burgess wrote:
>>
>>>  
>>>        /* Compare the _available_ contents.  */
>>> -      if (memcmp (val1->contents + offset1,
>>> -		  val2->contents + offset2,
>>> -		  l1) != 0)
>>> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
>>> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
>>> +		  (l1 / TARGET_CHAR_BIT)) != 0)
>>>  	return 0;
>>
>> As memcmp compares bytes, isn't this potentially comparing bits
>> at the beginning and end of the values' buffers, when it
>> should not?   That is, it looks like the
>> 'offset1 % TARGET_CHAR_BIT != 0' and
>> '(offset1 + l1) % TARGET_CHAR_BIT' cases should be considered here?
>>
> 
> Thanks for the review.  You're right, this isn't doing the correct thing
> here, I should have written something like this:
> 
> 
> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
> +		  ((l1 + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)) != 0)
> 
> That is round the start down to the nearest byte boundary, and round the
> length up to the nearest byte boundary.
> 
> I figured that as the value content buffer is always a round number of
> bytes then there will always be memory backing the access, and as the
> content buffer is zero initialized comparing the "unavailable" bits will
> not cause the compare to fail.

Yeah, given value_available_contents_eq's interface works with bytes,
not bits, we know we'll always be comparing up to byte boundaries, so
even though this iteration may compare more bits than the current
iterated range covers, it's fine, we'd compare them the next
iteration anyway.  Something like:

     bit offset:   0 1 2 3  4 5 6 7
val1's contents: [ 1 U U U  2 2 2 2 ]
val2's contents: [ 1 U U U  5 5 5 5 ]
bits to compare: [ X X X X  X X X X ]

The first time, we'll memcmp bits bits 0-7 and return false,
even though the first available range (bit 0), compares equal.

This deserves a comment though.

> Alternatively, I can update to only access the bits we'd like to compare.

Hmm.  That'd be necessary if value_available_contents_eq accepted
bit offsets instead of byte offsets.  E.g,. consider two values
with these contents:

     bit offset:   0 1 2 3  4 5 6 7
val1's contents: [ 1 U U U  5 5 5 5 ]
val2's contents: [ 0 U U U  5 5 5 5 ]
bits to compare: [       X  X X X X ]

We'd have to make sure to not compare (or mask off before
comparing) the bit at offset 0.

I wonder if it wouldn't make it for more readable code
(and less fragile, e.g., what if we want to stop zero
initializing the contents buffer) if we handled this.  That is,
rename value_available_contents_eq to something like
value_available_contents_bits_eq (and make it static), have it accept
bit offsets, and take that case into consideration.  Then,
make value_available_contents_eq a wrapper that multiplies offsets/lengths
by TARGET_CHAR_BIT.

		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
					       this_size, buffer,
					       &optim, &unavail))
		  {
		    /* Just so garbage doesn't ever shine through.  */
		    memset (buffer, 0, this_size);


> Which approach would would be best?

I'm fine either way, but if we go the former route, then
please add a comment mentioning the assumptions.

> 
> Thanks for your advice,
> Andrew
> 

-- 
Pedro Alves

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Andrew Burgess]

Second attempt for converting the unavailable vector to be bit,
not byte based.

I've addressed the review comments from:
  https://sourceware.org/ml/gdb-patches/2013-12/msg00187.html

And have implemented a compare mechanism that should handle partial
byte compares if needed, as discussed in these mails:
  https://sourceware.org/ml/gdb-patches/2013-12/msg00351.html
  https://sourceware.org/ml/gdb-patches/2013-12/msg00367.html

I've left the new function 'value_available_contents_bits_eq' as
static within value.c for now, if we need access to this in the
future it can always be opened up then.

Again, tested on x86-64 linux, including using native gdbserver.

OK to apply?

Thanks,
Andrew


    gdb/testfile/ChangeLog
    
    	* gdb.trace/unavailable-dwarf-piece.c: New file.
    	* gdb.trace/unavailable-dwarf-piece.exp: New file.
    
    gdb/ChangeLog
    
    	* dwarf2loc.c (read_pieced_value): Mark bits, not bytes
    	unavailable, use correct bit length.
    	* value.c (struct value): Extend comment on unavailable to
    	indicate that it is bit based.
    	(value_bits_available): New function.
    	(value_bytes_available): Call value_bits_available.
    	(value_entirely_available): Check against the bit length, not byte
    	length.
    	(mark_value_bits_unavailable): New function.
    	(mark_value_bytes_unavailable): Move contents to
    	mark_value_bits_unavailable, call to same.
    	(memcmp_with_bit_offsets): New function.
    	(value_available_contents_bits_eq): New function, takes the
    	functionality from value_available_contents_eq but uses
    	memcmp_with_bit_offsets now, and is bit not byte based.
    	(value_available_contents_eq): Move implementation into
    	value_available_contents_bits_eq, call to same.
    	(value_contents_copy_raw): Work on bits, not bytes.
    	(unpack_value_bits_as_long_1): Check availability in bits, not
    	bytes.
    	* value.h (value_bits_available): Declare new function.
    	(mark_value_bits_unavailable): Declare new function.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 2b1f323..7b911b1 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1709,7 +1709,7 @@ read_pieced_value (struct value *v)
 		    if (optim)
 		      set_value_optimized_out (v, 1);
 		    if (unavail)
-		      mark_value_bytes_unavailable (v, offset, this_size);
+		      mark_value_bits_unavailable (v, offset, this_size_bits);
 		  }
 	      }
 	    else
diff --git a/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c
new file mode 100644
index 0000000..46474f2
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.c
@@ -0,0 +1,86 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+struct s
+{
+  unsigned char a;
+  unsigned char b;
+  unsigned char c;
+};
+
+struct t
+{
+  /* First, a complete byte.  */
+  unsigned char a;
+  /* Next, 8 single bits.  */
+  unsigned char b : 1;
+  unsigned char c : 1;
+  unsigned char d : 1;
+  unsigned char e : 1;
+  unsigned char f : 1;
+  unsigned char g : 1;
+  unsigned char h : 1;
+  unsigned char i : 1;
+  /* Now another byte.  */
+  unsigned char j;
+};
+
+void
+end (void)
+{
+  /* Nothing.  */
+}
+
+void
+dummy (void)
+{
+  /* Nothing.  */
+}
+
+int
+foo (struct s x, struct s y, struct s z)
+{
+  dummy ();
+  asm (".global foo_end_lbl\nfoo_end_lbl:");
+  return 0;
+}
+
+int
+bar (struct t x, struct t y, struct t z)
+{
+  dummy ();
+  asm (".global bar_end_lbl\nbar_end_lbl:");
+  return 0;
+}
+
+int
+main (void)
+{
+  struct s v = { 0, 1, 2 };
+  struct t w = { 5, 0, 1, 0, 1, 0, 1, 0, 1, 7 };
+  int ans;
+
+  ans = foo (v, v, v);
+
+  end ();
+
+  ans = bar (w, w, w);
+
+  end ();
+
+  return ans;
+}
diff --git a/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp
new file mode 100644
index 0000000..be38588
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/unavailable-dwarf-piece.exp
@@ -0,0 +1,334 @@
+# Copyright (C) 2013 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/>.
+
+load_lib "trace-support.exp"
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c
+
+set asm_file "${testfile}-dbg.s"
+set opts {}
+
+if  { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile}1.o \
+	   object {nodebug}] != "" } {
+    return -1
+}
+
+Dwarf::assemble $asm_file {
+    declare_labels uchar_label struct_s_label foo_label struct_t_label bar_label
+
+    cu {} {
+	compile_unit {{language @DW_LANG_C}} {
+	    uchar_label: DW_TAG_base_type {
+		{name "unsigned char"}
+		{byte_size 1 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned_char}
+	    }
+
+	    struct_s_label: DW_TAG_structure_type {
+		{name s}
+		{byte_size 3 DW_FORM_sdata}
+		{decl_file 1}
+		{decl_line 1}
+	    } {
+		DW_TAG_member {
+		    {name a}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 0
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name b}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name c}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    struct_t_label: DW_TAG_structure_type {
+		{name t}
+		{byte_size 3 DW_FORM_sdata}
+		{decl_file 1}
+		{decl_line 1}
+	    } {
+		DW_TAG_member {
+		    {name a}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 0
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name b}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 7 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name c}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 6 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name d}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 5 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name e}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 4 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name f}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 3 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name g}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 2 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name h}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 1 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name i}
+		    {type :$uchar_label}
+		    {byte_size 1 DW_FORM_sdata}
+		    {bit_size 1 DW_FORM_sdata}
+		    {bit_offset 0 DW_FORM_sdata}
+		    {data_member_location {
+			DW_OP_plus_uconst 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_member {
+		    {name j}
+		    {type :$uchar_label}
+		    {data_member_location {
+			DW_OP_plus_uconst 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    DW_TAG_subprogram {
+		{name foo}
+		{decl_file 1}
+		{low_pc foo addr}
+		{high_pc foo_end_lbl addr}
+	    } {
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name x}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 2
+			DW_OP_reg0
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name y}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_reg0
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_s_label}
+		    {name z}
+		    {location {
+			DW_OP_reg0
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 2
+		    } SPECIAL_expr}
+		}
+	    }
+
+
+	    DW_TAG_subprogram {
+		{name bar}
+		{decl_file 1}
+		{low_pc bar addr}
+		{high_pc bar_end_lbl addr}
+	    } {
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name x}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 7 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name y}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 3 0
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 4 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+		DW_TAG_formal_parameter {
+		    {type :$struct_t_label}
+		    {name z}
+		    {location {
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_bit_piece 7 0
+			DW_OP_reg0
+			DW_OP_bit_piece 1 0
+			DW_OP_lit0
+			DW_OP_stack_value
+			DW_OP_piece 1
+		    } SPECIAL_expr}
+		}
+	    }
+
+	}
+    }
+}
+
+if  { [gdb_compile ${asm_file} ${binfile}2.o object {nodebug}] != "" } {
+    return -1
+}
+
+if  { [gdb_compile [list ${binfile}1.o ${binfile}2.o] ${binfile} \
+	   executable {}] != "" } {
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+gdb_breakpoint "end"
+
+with_test_prefix "tracing foo" {
+    gdb_test "trace foo" ".*"
+    gdb_test_no_output "tstart"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*"
+    gdb_test_no_output "tstop"
+
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+    gdb_test "p/d x" "\\\$${decimal} = {a = 0, b = 0, c = <unavailable>}"
+    gdb_test "p/d y" "\\\$${decimal} = {a = 0, b = <unavailable>, c = 0}"
+    gdb_test "p/d z" "\\\$${decimal} = {a = <unavailable>, b = 0, c = 0}"
+
+    gdb_test "tfind none" "No longer looking at any trace frame.*"
+}
+
+with_test_prefix "tracing bar" {
+    gdb_test "trace bar" ".*"
+    gdb_test_no_output "tstart"
+    gdb_test "continue" "Continuing\\.\[ \r\n\]+Breakpoint.*"
+    gdb_test_no_output "tstop"
+
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
+    gdb_test "p/d x" "\\\$${decimal} = {a = 0, b = <unavailable>, c = 0, d = 0, e = 0, f = 0, g = 0, h = 0, i = 0, j = 0}"
+    gdb_test "p/d y" "\\\$${decimal} = {a = 0, b = 0, c = 0, d = 0, e = <unavailable>, f = 0, g = 0, h = 0, i = 0, j = 0}"
+    gdb_test "p/d z" "\\\$${decimal} = {a = 0, b = 0, c = 0, d = 0, e = 0, f = 0, g = 0, h = 0, i = <unavailable>, j = 0}"
+
+    gdb_test "tfind none" "No longer looking at any trace frame.*"
+}
diff --git a/gdb/value.c b/gdb/value.c
index a64e7e1..9338971 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -333,12 +333,13 @@ struct value
 
   /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
      rather than available, since the common and default case is for a
-     value to be available.  This is filled in at value read time.  */
+     value to be available.  This is filled in at value read time.  The
+     unavailable ranges are tracked in bits.  */
   VEC(range_s) *unavailable;
 };
 
 int
-value_bytes_available (const struct value *value, int offset, int length)
+value_bits_available (const struct value *value, int offset, int length)
 {
   gdb_assert (!value->lazy);
 
@@ -346,6 +347,14 @@ value_bytes_available (const struct value *value, int offset, int length)
 }
 
 int
+value_bytes_available (const struct value *value, int offset, int length)
+{
+  return value_bits_available (value,
+			       offset * TARGET_CHAR_BIT,
+			       length * TARGET_CHAR_BIT);
+}
+
+int
 value_entirely_available (struct value *value)
 {
   /* We can only tell whether the whole value is available when we try
@@ -371,7 +380,8 @@ value_entirely_unavailable (struct value *value)
       struct range *t = VEC_index (range_s, value->unavailable, 0);
 
       if (t->offset == 0
-	  && t->length == TYPE_LENGTH (value_enclosing_type (value)))
+	  && t->length == (TARGET_CHAR_BIT *
+			   TYPE_LENGTH (value_enclosing_type (value))))
 	return 1;
     }
 
@@ -379,7 +389,7 @@ value_entirely_unavailable (struct value *value)
 }
 
 void
-mark_value_bytes_unavailable (struct value *value, int offset, int length)
+mark_value_bits_unavailable (struct value *value, int offset, int length)
 {
   range_s newr;
   int i;
@@ -543,6 +553,14 @@ mark_value_bytes_unavailable (struct value *value, int offset, int length)
     }
 }
 
+void
+mark_value_bytes_unavailable (struct value *value, int offset, int length)
+{
+  mark_value_bits_unavailable (value,
+			       offset * TARGET_CHAR_BIT,
+			       length * TARGET_CHAR_BIT);
+}
+
 /* Find the first range in RANGES that overlaps the range defined by
    OFFSET and LENGTH, starting at element POS in the RANGES vector,
    Returns the index into RANGES where such overlapping range was
@@ -562,10 +580,125 @@ find_first_range_overlap (VEC(range_s) *ranges, int pos,
   return -1;
 }
 
-int
-value_available_contents_eq (const struct value *val1, int offset1,
-			     const struct value *val2, int offset2,
-			     int length)
+/* Compare LENGTH_BITS of memory at PTR1 + OFFSET1_BITS with the memory at
+   PTR2 + OFFSET2_BITS.  Return 0 if the memory is the same, otherwise
+   return non-zero.
+
+   It must always be the case that:
+     OFFSET1_BITS % TARGET_CHAR_BIT == OFFSET2_BITS % TARGET_CHAR_BIT
+
+   It is assumed that memory can be accessed from:
+     PTR + (OFFSET_BITS / TARGET_CHAR_BIT)
+   to:
+     PTR + ((OFFSET_BITS + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)  */
+static int
+memcmp_with_bit_offsets (const void *ptr1, size_t offset1_bits,
+			 const void *ptr2, size_t offset2_bits,
+			 size_t length_bits)
+{
+  gdb_assert (offset1_bits % TARGET_CHAR_BIT
+	      == offset2_bits % TARGET_CHAR_BIT);
+
+  if (offset1_bits % TARGET_CHAR_BIT != 0)
+    {
+      size_t bits;
+      bfd_byte mask, b1, b2;
+
+      /* The offset from the base pointers PTR1 and PTR2 is not a complete
+	 number of bytes.  A number of bits up to either the next exact
+	 byte boundary, or LENGTH_BITS (which ever is sooner) will be
+	 compared.  */
+      bits = TARGET_CHAR_BIT - (offset1_bits % TARGET_CHAR_BIT);
+      gdb_assert (bits < sizeof (mask) * TARGET_CHAR_BIT);
+      mask = (1 << bits) - 1;
+
+      if (length_bits < bits)
+	{
+	  mask &= ~((bfd_byte) ((1 << (bits - length_bits)) - 1));
+	  bits = length_bits;
+	}
+
+      /* Now load the two bytes and mask off the bits we care about.  */
+      b1 = *((bfd_byte *) ((uintptr_t) ptr1
+			   + (offset1_bits / TARGET_CHAR_BIT)));
+      b2 = *((bfd_byte *) ((uintptr_t) ptr2
+			   + (offset2_bits / TARGET_CHAR_BIT)));
+      b1 &= mask;
+      b2 &= mask;
+
+      if (b1 != b2)
+	return 1;
+
+      /* Now update the length and offsets to take account of the bits
+	 we've just compared.  */
+      length_bits -= bits;
+      offset1_bits += bits;
+      offset2_bits += bits;
+    }
+
+  if (length_bits % TARGET_CHAR_BIT != 0)
+    {
+      size_t bits;
+      size_t o1, o2;
+      bfd_byte mask, b1, b2;
+
+      /* The length is not an exact number of bytes.  After the previous
+	 IF.. block then the offsets are byte aligned, or the
+	 length is zero (in which case this code is not reached).  Compare
+	 a number of bits at the end of the region, starting from an exact
+	 byte boundary.  */
+      bits = length_bits % TARGET_CHAR_BIT;
+      o1 = offset1_bits + length_bits - bits;
+      o2 = offset2_bits + length_bits - bits;
+
+      gdb_assert (bits < sizeof (mask) * TARGET_CHAR_BIT);
+      mask = ((1 << bits) - 1) << (TARGET_CHAR_BIT - bits);
+
+      gdb_assert (o1 % TARGET_CHAR_BIT == 0);
+      gdb_assert (o2 % TARGET_CHAR_BIT == 0);
+
+      b1 = *((bfd_byte *) ((uintptr_t) ptr1 + (o1 / TARGET_CHAR_BIT)));
+      b2 = *((bfd_byte *) ((uintptr_t) ptr2 + (o2 / TARGET_CHAR_BIT)));
+      b1 &= mask;
+      b2 &= mask;
+
+      if (b1 != b2)
+	return 1;
+
+      length_bits -= bits;
+    }
+
+  if (length_bits > 0)
+    {
+      /* We've now taken care of any stray "bits" at the start, or end of
+	 the region to compare, the remainder can be covered with a simple
+	 memcmp.  */
+      gdb_assert (offset1_bits % TARGET_CHAR_BIT == 0);
+      gdb_assert (offset2_bits % TARGET_CHAR_BIT == 0);
+      gdb_assert (length_bits % TARGET_CHAR_BIT == 0);
+
+      return memcmp ((void*) ((uintptr_t) ptr1
+			      + (offset1_bits / TARGET_CHAR_BIT)),
+		     (void*) ((uintptr_t) ptr2
+			      + (offset2_bits / TARGET_CHAR_BIT)),
+		     length_bits / TARGET_CHAR_BIT);
+    }
+
+  /* Length is zero, regions match.  */
+  return 0;
+}
+
+/* Helper function for value_available_contents_eq. The only difference is
+   that this function is bit rather than byte based.
+
+   Compare LENGTH bits of VAL1's contents starting at OFFSET1 bits with
+   LENGTH bits of VAL2's contents starting at OFFSET2 bits.  Return true
+   if the available bits match.  */
+
+static int
+value_available_contents_bits_eq (const struct value *val1, int offset1,
+				  const struct value *val2, int offset2,
+				  int length)
 {
   int idx1 = 0, idx2 = 0;
 
@@ -585,9 +718,9 @@ value_available_contents_eq (const struct value *val1, int offset1,
 
       /* The usual case is for both values to be completely available.  */
       if (idx1 == -1 && idx2 == -1)
-	return (memcmp (val1->contents + offset1,
-			val2->contents + offset2,
-			length) == 0);
+	return (memcmp_with_bit_offsets (val1->contents, offset1,
+					 val2->contents, offset2,
+					 length) == 0);
       /* The contents only match equal if the available set matches as
 	 well.  */
       else if (idx1 == -1 || idx2 == -1)
@@ -620,9 +753,8 @@ value_available_contents_eq (const struct value *val1, int offset1,
 	return 0;
 
       /* Compare the _available_ contents.  */
-      if (memcmp (val1->contents + offset1,
-		  val2->contents + offset2,
-		  l1) != 0)
+      if (memcmp_with_bit_offsets (val1->contents, offset1,
+				   val2->contents, offset2, l1) != 0)
 	return 0;
 
       length -= h1;
@@ -633,6 +765,16 @@ value_available_contents_eq (const struct value *val1, int offset1,
   return 1;
 }
 
+int
+value_available_contents_eq (const struct value *val1, int offset1,
+			     const struct value *val2, int offset2,
+			     int length)
+{
+  return value_available_contents_bits_eq (val1, offset1 * TARGET_CHAR_BIT,
+					   val2, offset2 * TARGET_CHAR_BIT,
+					   length * TARGET_CHAR_BIT);
+}
+
 /* Prototypes for local functions.  */
 
 static void show_values (char *, int);
@@ -972,6 +1114,7 @@ value_contents_copy_raw (struct value *dst, int dst_offset,
 {
   range_s *r;
   int i;
+  int src_bit_offset, dst_bit_offset, bit_length;
 
   /* A lazy DST would make that this copy operation useless, since as
      soon as DST's contents were un-lazied (by a later value_contents
@@ -990,17 +1133,20 @@ value_contents_copy_raw (struct value *dst, int dst_offset,
 	  length);
 
   /* Copy the meta-data, adjusted.  */
+  src_bit_offset = src_offset * TARGET_CHAR_BIT;
+  dst_bit_offset = dst_offset * TARGET_CHAR_BIT;
+  bit_length = length * TARGET_CHAR_BIT;
   for (i = 0; VEC_iterate (range_s, src->unavailable, i, r); i++)
     {
       ULONGEST h, l;
 
-      l = max (r->offset, src_offset);
-      h = min (r->offset + r->length, src_offset + length);
+      l = max (r->offset, src_bit_offset);
+      h = min (r->offset + r->length, src_bit_offset + bit_length);
 
       if (l < h)
-	mark_value_bytes_unavailable (dst,
-				      dst_offset + (l - src_offset),
-				      h - l);
+	mark_value_bits_unavailable (dst,
+				     dst_bit_offset + (l - src_bit_offset),
+				     h - l);
     }
 }
 
@@ -2884,8 +3030,8 @@ unpack_value_bits_as_long_1 (struct type *field_type, const gdb_byte *valaddr,
   read_offset = bitpos / 8;
 
   if (original_value != NULL
-      && !value_bytes_available (original_value, embedded_offset + read_offset,
-				 bytes_read))
+      && !value_bits_available (original_value, embedded_offset + bitpos,
+				bitsize))
     return 0;
 
   val = extract_unsigned_integer (valaddr + embedded_offset + read_offset,
diff --git a/gdb/value.h b/gdb/value.h
index db964e3..6b158df 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -436,6 +436,14 @@ extern int value_bits_synthetic_pointer (const struct value *value,
 extern int value_bytes_available (const struct value *value,
 				  int offset, int length);
 
+/* Given a value, determine whether the contents bits starting at
+   OFFSET and extending for LENGTH bits are available.  This returns
+   nonzero if all bits in the given range are available, zero if any
+   bit is unavailable.  */
+
+extern int value_bits_available (const struct value *value,
+				 int offset, int length);
+
 /* Like value_bytes_available, but return false if any byte in the
    whole object is unavailable.  */
 extern int value_entirely_available (struct value *value);
@@ -450,6 +458,12 @@ extern int value_entirely_unavailable (struct value *value);
 extern void mark_value_bytes_unavailable (struct value *value,
 					  int offset, int length);
 
+/* Mark VALUE's content bits starting at OFFSET and extending for
+   LENGTH bits as unavailable.  */
+
+extern void mark_value_bits_unavailable (struct value *value,
+					 int offset, int length);
+
 /* Compare LENGTH bytes of VAL1's contents starting at OFFSET1 with
    LENGTH bytes of VAL2's contents starting at OFFSET2.
 

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Pedro Alves]

On 12/11/2013 04:28 PM, Andrew Burgess wrote:
> +
> +   It is assumed that memory can be accessed from:
> +     PTR + (OFFSET_BITS / TARGET_CHAR_BIT)
> +   to:
> +     PTR + ((OFFSET_BITS + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)  */

The "to:" part doesn't look right -- LENGTH_BITS seems to be
missing somewhere?  Also, missing space before second "/".

> +      bits = TARGET_CHAR_BIT - (offset1_bits % TARGET_CHAR_BIT);

Unnecessary parens.

> +      b1 = *((bfd_byte *) ((uintptr_t) ptr1 + (o1 / TARGET_CHAR_BIT)));
> +      b2 = *((bfd_byte *) ((uintptr_t) ptr2 + (o2 / TARGET_CHAR_BIT)));

No need for uintptr_t, nor the rightmost set of parens:

      b1 = *((bfd_byte *) ptr1 + o1 / TARGET_CHAR_BIT);
      b2 = *((bfd_byte *) ptr2 + o2 / TARGET_CHAR_BIT);

There may be other instances of unneeded parens.  I won't point
them all out.

> +      return memcmp ((void*) ((uintptr_t) ptr1
> +			      + (offset1_bits / TARGET_CHAR_BIT)),
> +		     (void*) ((uintptr_t) ptr2
> +			      + (offset2_bits / TARGET_CHAR_BIT)),
> +		     length_bits / TARGET_CHAR_BIT);

Space before *.  Parens.

Otherwise looks OK.  Thanks a lot for doing this!

-- 
Pedro Alves

Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Andrew Burgess]

On 11/12/2013 6:45 PM, Pedro Alves wrote:
> On 12/11/2013 04:28 PM, Andrew Burgess wrote:
>> +
>> +   It is assumed that memory can be accessed from:
>> +     PTR + (OFFSET_BITS / TARGET_CHAR_BIT)
>> +   to:
>> +     PTR + ((OFFSET_BITS + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)  */
> 
> The "to:" part doesn't look right -- LENGTH_BITS seems to be
> missing somewhere?  Also, missing space before second "/".


Fixed as:

+   It is assumed that memory can be accessed from:
+     PTR + (OFFSET_BITS / TARGET_CHAR_BIT)
+   to:
+     PTR + ((OFFSET_BITS + LENGTH_BITS + TARGET_CHAR_BIT - 1)
+            / TARGET_CHAR_BIT)  */

> 
>> +      bits = TARGET_CHAR_BIT - (offset1_bits % TARGET_CHAR_BIT);
> 
> Unnecessary parens.

Tightened up on this throughout.

> 
>> +      b1 = *((bfd_byte *) ((uintptr_t) ptr1 + (o1 / TARGET_CHAR_BIT)));
>> +      b2 = *((bfd_byte *) ((uintptr_t) ptr2 + (o2 / TARGET_CHAR_BIT)));
> 
> No need for uintptr_t, nor the rightmost set of parens:

Fixed.

> 
> Otherwise looks OK.  Thanks a lot for doing this!

I've pushed a version with those minor fixes.

Thanks for all your reviews.

Andrew