[PATCH] dwarf2out: Represent bound_info with normal constant values if possible.

[PATCH] dwarf2out: Represent bound_info with normal constant values if possible.

[Mark Wielaard]

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(-)

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));
       }
-- 
1.7.1

Re: [PATCH] dwarf2out: Represent bound_info with normal constant values if possible.

[Eric Botcazou]

> 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)?

-- 
Eric Botcazou

Re: [PATCH] dwarf2out: Represent bound_info with normal constant values if possible.

[Mark Wielaard]

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

[PATCH] dwarf2out: Use normal constant values in bound_info if possible.

[Mark Wielaard]

	* dwarf2out.c (add_bound_info): If HOST_WIDE_INT is big enough,
	then represent the bound as normal constant value.
---
 gcc/ChangeLog   |    5 +++++
 gcc/dwarf2out.c |   32 ++++++++++++++++++++------------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3ab789a..8a31187 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-03-20  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf2out.c (add_bound_info): If HOST_WIDE_INT is big enough,
+	then represent the bound as normal constant value.
+
 2014-03-19  Marek Polacek  <polacek@redhat.com>
 
 	PR sanitizer/60569
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));
       }
-- 
1.7.1

Re: [PATCH] dwarf2out: Use normal constant values in bound_info if possible.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

Hi,

On Sun, 2014-03-23 at 12:15 +0100, Mark Wielaard wrote:
> 	* dwarf2out.c (add_bound_info): If HOST_WIDE_INT is big enough,
> 	then represent the bound as normal constant value.

Since stage 1 opened up I would like to request approval again to push
this. Patch rebased to current master attached. Previous discussion
here: http://gcc.gnu.org/ml/gcc-patches/2014-03/msg01192.html

Thanks,

Mark

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2726 bytes --]

commit 668472db0186f79c4e50c6e422c5e90f82017b16
Author: Mark Wielaard <mjw@redhat.com>
Date:   Thu Mar 20 18:02:36 2014 +0100

    dwarf2out: Use normal constant values in bound_info if possible.
    
    	* dwarf2out.c (add_bound_info): If HOST_WIDE_INT is big enough,
    	then represent the bound as normal constant value.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index f2c127c..b1706ce 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-03-20  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf2out.c (add_bound_info): If HOST_WIDE_INT is big enough,
+	then represent the bound as normal constant value.
+
 2014-04-14  Paolo Carlini  <paolo.carlini@oracle.com>
 
 	* tree.h (TYPE_IDENTIFIER): Declare.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 721f761..7eef56c 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -16203,21 +16203,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));
       }

Re: [PATCH] dwarf2out: Use normal constant values in bound_info if possible.

[Cary Coutant]

> +       /* 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.  */

This comment confuses me. By "we need to make sure the type is signed
or unsigned" (what else can it be?), I think you mean "we need to
choose a form based on whether the type is signed or unsigned." And by
"This is only for DW_AT_lower_bound, ...", I think you mean "This is
needed only for DW_AT_{lower,upper}_bound, since for most other
attributes, consumers will treat DW_FORM_data[1248] as unsigned
values, regardless of the underlying type."

Otherwise, the patch looks OK to me.

-cary

Re: [PATCH] dwarf2out: Use normal constant values in bound_info if possible.

[Mark Wielaard]

On Tue, 2014-04-15 at 14:24 -0700, Cary Coutant wrote:
> > +       /* 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.  */
> 
> This comment confuses me.

Sorry, obviously not my intention. But I see what I was trying to say
and how I said it didn't make things very clear. Apologies.

>  By "we need to make sure the type is signed
> or unsigned" (what else can it be?), I think you mean "we need to
> choose a form based on whether the type is signed or unsigned."

Yes, right. I was confusing matters in my comment because I was thinking
of non-constants (reference or exprlocs) that are handled elsewhere
later on in the code.

>  And by "This is only for DW_AT_lower_bound, ...", I think you mean "This is
> needed only for DW_AT_{lower,upper}_bound, since for most other
> attributes, consumers will treat DW_FORM_data[1248] as unsigned
> values, regardless of the underlying type."

Yes, right again.

> Otherwise, the patch looks OK to me.

Thanks I pushed it with the comment changed to how you expressed things.
It now reads:

/* If HOST_WIDE_INT is big enough then represent the bound as           
   a constant value.  We need to choose a form based on                 
   whether the type is signed or unsigned.  We cannot just              
   call add_AT_unsigned if the value itself is positive                 
   (add_AT_unsigned might add the unsigned value encoded as             
   DW_FORM_data[1248]).  Some DWARF consumers will lookup the           
   bounds type and then sign extend any unsigned values found           
   for signed types.  This is needed only for                           
   DW_AT_{lower,upper}_bound, since for most other attributes,          
   consumers will treat DW_FORM_data[1248] as unsigned values,          
   regardless of the underlying type.  */

Thanks,

Mark