diff mbox

dwarf2out: Represent bound_info with normal constant values if possible.

Message ID 1395337588-28129-1-git-send-email-mjw@redhat.com
State New
Headers show

Commit Message

Mark Wielaard March 20, 2014, 5:46 p.m. UTC 
Hi,

The following patch made it so that 64-bit bounds could be output on
32-bit hosts with DWARF.

2009-07-11  Eric Botcazou  <ebotcazou@adacore.com>

	* dwarf2out.c (enum dw_val_class): Replace dw_val_class_long_long
	with dw_val_class_const_double.
	(struct dw_long_long_struct): Delete.
	(struct dw_val_struct): Adjust for above change.
	(add_AT_long_long): Rename into...
	(add_AT_double): ...this.
	(print_die): Replace dw_val_class_long_long case with
	dw_val_class_const_double case.
	(attr_checksum): Likewise.
	(same_dw_val_p): Likewise.
	(size_of_die): Likewise.
	(value_format): Likewise.
	(output_die): Likewise.
	(add_const_value_attribute) <CONST_DOUBLE>: Call add_AT_double
	instead of add_AT_long_long.
	(add_bound_info) <INTEGER_CST>: Generate the bound as an unsigned
	value with the precision of its type.

http://gcc.gnu.org/ml/gcc-patches/2009-07/msg00653.html

This was done by using add_AT_double whenever a constant needed to be
output that didn't fit a HOST_WIDE_INT. It also changed the representation
of DW_AT_lower_bound and DW_AT_higher_bound when the range did fit in a
HOST_WIDE_INT. Up to that patch the range values were added
using add_AT_unsigned. But to represent signed values it didn't use
add_AT_int, but used an unsigned value with the precision of its type.

As the comment added at the time says:
"The precision and signedness of the type will be necessary to re-interpret
it unambiguously."

This is a little unfortunate. Since in other cases this isn't necessary.
In particular in all other cases gdb and other DWARF consumers (elfutils at
least) assume that when they encounter a DW_FORM_data[1248] they can simply
zero-extend it to get the actual constant value. Or that DW_FORM_sdata is
used to represent an actual negative constant. (There are a couple of
comments about relying on this behavior in both the gcc and gdb sources.)

So the patch below makes it so that if HOST_WIDE_INT is wide enough then,
depending on whether the range type is signed or not, add_AT_unsigned or
add_AT_int is used. This is more efficient for small ranges. And makes it
so that the value can be deduced from the DW_FORM by the consumer (which
can assume again that DW_FORM_data[1248] are simply zero-extended and
that negative constant values are represented by DW_FORM_sdata).

I tested this on x86_64 with --enable-languages=c,ada,c++,fortran,java,objc
without regressions. I also made sure that the example ada program range
is recognized correctly by gdb with this patch.

A couple of questions:

- Are there more ada DWARF tests? Something like guality used for c/fortran?
- What values of HOST_BITS_PER_WIDE_INT are actually supported in GCC?
  The dwarf2out.c code tries to handle 8, 16, 32 and 64 bits for
  dw_val_class_const_double.
- Which setups use 32bit (or lower?) HOST_BITS_PER_WIDE_INT?
  i686 seems to require 64BIT HOST_WIDE_INTs too these days.

I would like to test a bit on a setup that uses HOST_BITS_PER_WIDE_INT < 64
because currently I believe the use of add_AT_double is slightly wrong in
dwarf2out. It looks like a DWARF consumer can no longer rely on simply
zero-extending DW_FORM_data[1248] values if add_AT_double is used. And
add_AT_double is currently used for two slightly different things. To output
constant class values (such as bounds) that don't fit a HOST_WIDE_INT and
to output large (block class) values for DW_AT_const_value. But these are
not always similar (constants class values cannot be represented by
DW_FORM_block for example). So it would be good to at least have a
add_AT_double_unsigned and add_AT_double_int that can be used just like
add_AT_unsigned and add_AT_int. That seems to also require some additions
to be able to output DW_FORM_sdata for constants larger than HOST_WIDE_INT
in dwarf2asm. Any thoughts on this?

Thanks,

Mark

---
 gcc/dwarf2out.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

Comments

Eric Botcazou March 21, 2014, 11:47 a.m. UTC | #1 
> So the patch below makes it so that if HOST_WIDE_INT is wide enough then,
> depending on whether the range type is signed or not, add_AT_unsigned or
> add_AT_int is used. This is more efficient for small ranges. And makes it
> so that the value can be deduced from the DW_FORM by the consumer (which
> can assume again that DW_FORM_data[1248] are simply zero-extended and
> that negative constant values are represented by DW_FORM_sdata).

FWIW this looks an improvement to me.

> I tested this on x86_64 with --enable-languages=c,ada,c++,fortran,java,objc
> without regressions. I also made sure that the example ada program range
> is recognized correctly by gdb with this patch.
> 
> A couple of questions:
> 
> - Are there more ada DWARF tests? Something like guality used for c/fortran?

In the compiler proper no, but there is (of course) the GDB testsuite.

> - What values of HOST_BITS_PER_WIDE_INT are actually supported in GCC? The
> dwarf2out.c code tries to handle 8, 16, 32 and 64 bits for
>   dw_val_class_const_double.

32 and 64

> - Which setups use 32bit (or lower?) HOST_BITS_PER_WIDE_INT?
>   i686 seems to require 64BIT HOST_WIDE_INTs too these days.

Right, pure 32-bit hosted compilers are an endangered species and GNAT is 
probably not fully functional for these architectures.

How did you run into the problem?  Can't you conduct some minimal testing on 
64-bit platforms by using 128-bit integers (not in Ada unfortunately)?
Mark Wielaard March 23, 2014, 8:54 a.m. UTC | #2 
On Fri, Mar 21, 2014 at 12:47:10PM +0100, Eric Botcazou wrote:
> > So the patch below makes it so that if HOST_WIDE_INT is wide enough then,
> > depending on whether the range type is signed or not, add_AT_unsigned or
> > add_AT_int is used. This is more efficient for small ranges. And makes it
> > so that the value can be deduced from the DW_FORM by the consumer (which
> > can assume again that DW_FORM_data[1248] are simply zero-extended and
> > that negative constant values are represented by DW_FORM_sdata).
> 
> FWIW this looks an improvement to me.

Thanks. I'll propose it as a real patch with ChangeLog.

> > I tested this on x86_64 with --enable-languages=c,ada,c++,fortran,java,objc
> > without regressions. I also made sure that the example ada program range
> > is recognized correctly by gdb with this patch.
> > 
> > A couple of questions:
> > 
> > - Are there more ada DWARF tests? Something like guality used for c/fortran?
> 
> In the compiler proper no, but there is (of course) the GDB testsuite.

Of course, thanks. I ran gdb make check against a gcc with and without this
patch without any test result changes.

> > - What values of HOST_BITS_PER_WIDE_INT are actually supported in GCC? The
> > dwarf2out.c code tries to handle 8, 16, 32 and 64 bits for
> >   dw_val_class_const_double.
> 
> 32 and 64

Phew. OK, that makes things simpler. I was afraid we had to handle much
more combinations.

> > - Which setups use 32bit (or lower?) HOST_BITS_PER_WIDE_INT?
> >   i686 seems to require 64BIT HOST_WIDE_INTs too these days.
> 
> Right, pure 32-bit hosted compilers are an endangered species and GNAT is 
> probably not fully functional for these architectures.
> 
> How did you run into the problem?  Can't you conduct some minimal testing on 
> 64-bit platforms by using 128-bit integers (not in Ada unfortunately)?

I admit that the problem was somewhat theoretical. I tried to convince
Joel that he didn't have to lookup the type each time in a GDB patch.
But he pointed out GCC sometimes outputs constant values as DW_FORM_data
that had to be sign-extended to be interpreted correctly. In other cases
GCC expects consumers to just zero-extend the value when DW_FORM_data is
used. I also work on another DWARF consumer (elfutils/libdw) that does
have that expectation. So I wanted to make GCC consistent. It now is for
64 HOST_BITS_PER_WIDE_INT arches, but not yet for 

Using 128-bit integers on 64-bit platforms doesn't expose the same issue.
On 64 HOST_BITS_PER_WIDE_INT arches a dw_val_class_const_double is encoded
as a DW_FORM_BLOCK (which would actually be an invalid encoding for a
DW_AT_higher or DW_AT_lower bound, but I don't know any language/construct
that would generate such 128-bit bounds ranges).

Cheers,

Mark
> 
> -- 
> Eric Botcazou
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 2b584a5..9f8b3b0 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -16198,21 +16198,29 @@  add_bound_info (dw_die_ref subrange_die, enum dwarf_attribute bound_attr, tree b
 	    && tree_to_shwi (bound) == dflt)
 	  ;
 
-	/* Otherwise represent the bound as an unsigned value with the
-	   precision of its type.  The precision and signedness of the
-	   type will be necessary to re-interpret it unambiguously.  */
-	else if (prec < HOST_BITS_PER_WIDE_INT)
+	/* If HOST_WIDE_INT is big enough then represent the bound as
+	   a constant value.  Note that we need to make sure the type
+	   is signed or unsigned.  We cannot just add an unsigned
+	   constant if the value itself is positive.  Some DWARF
+	   consumers will lookup the bounds type and then sign extend
+	   any unsigned values found for signed types.  This is only
+	   for DW_AT_lower_bound, normally unsigned values
+	   (DW_FORM_data[1248]) are assumed to not need
+	   sign-extension.  */
+	else if (prec <= HOST_BITS_PER_WIDE_INT
+		 || TREE_INT_CST_HIGH (bound) == 0)
 	  {
-	    unsigned HOST_WIDE_INT mask
-	      = ((unsigned HOST_WIDE_INT) 1 << prec) - 1;
-	    add_AT_unsigned (subrange_die, bound_attr,
-		  	     TREE_INT_CST_LOW (bound) & mask);
+	    if (TYPE_UNSIGNED (TREE_TYPE (bound)))
+	      add_AT_unsigned (subrange_die, bound_attr,
+			       TREE_INT_CST_LOW (bound));
+	    else
+	      add_AT_int (subrange_die, bound_attr, TREE_INT_CST_LOW (bound));
 	  }
-	else if (prec == HOST_BITS_PER_WIDE_INT
-		 || TREE_INT_CST_HIGH (bound) == 0)
-	  add_AT_unsigned (subrange_die, bound_attr,
-		  	   TREE_INT_CST_LOW (bound));
 	else
+	  /* Otherwise represent the bound as an unsigned value with
+	     the precision of its type.  The precision and signedness
+	     of the type will be necessary to re-interpret it
+	     unambiguously.  */
 	  add_AT_double (subrange_die, bound_attr, TREE_INT_CST_HIGH (bound),
 		         TREE_INT_CST_LOW (bound));
       }