diff mbox

Allow DW_OP_GNU_uninit in dwarf_expr_require_composition

Message ID m3lh3zuf30.fsf@oc1027705133.ibm.com
State New
Headers show

Commit Message

Andreas Arnez April 27, 2016, 5:38 p.m. UTC 
In DWARF expression handling, some operators are required to be either
at the end of an expression or followed by a composition operator.  So
far only the operators DW_OP_reg0-31 were allowed to be followed by
DW_OP_GNU_uninit instead, and particularly DW_OP_regx was not, which is
obviously inconsistent.

This patch allows DW_OP_GNU_uninit after all operators requiring a
composition, to simplify the code and make it more consistent.  This
policy may be more permissive than necessary, but in the worst case just
leads to a DWARF location description resulting in an uninitialized
value instead of an error message.

gdb/ChangeLog:

	* dwarf2expr.c (dwarf_expr_require_composition): Allow
	DW_OP_GNU_uninit.
	(execute_stack_op): Use dwarf_expr_require_composition instead of
	copying its logic.
---
 gdb/dwarf2expr.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Pedro Alves Oct. 4, 2016, 5:33 p.m. UTC | #1 
On 04/27/2016 06:38 PM, Andreas Arnez wrote:
> In DWARF expression handling, some operators are required to be either
> at the end of an expression or followed by a composition operator.  So
> far only the operators DW_OP_reg0-31 were allowed to be followed by
> DW_OP_GNU_uninit instead, and particularly DW_OP_regx was not, which is
> obviously inconsistent.
> 
> This patch allows DW_OP_GNU_uninit after all operators requiring a
> composition, to simplify the code and make it more consistent.  This
> policy may be more permissive than necessary, but in the worst case just
> leads to a DWARF location description resulting in an uninitialized
> value instead of an error message.

Fine with me.  DW_OP_GNU_uninit is underspecified, anyway.

> 
> gdb/ChangeLog:
> 
> 	* dwarf2expr.c (dwarf_expr_require_composition): Allow
> 	DW_OP_GNU_uninit.
> 	(execute_stack_op): Use dwarf_expr_require_composition instead of
> 	copying its logic.

OK.

Thanks,
Pedro Alves
Andreas Arnez Oct. 5, 2016, 10:42 a.m. UTC | #2 
On Tue, Oct 04 2016, Pedro Alves wrote:

>> gdb/ChangeLog:
>> 
>> 	* dwarf2expr.c (dwarf_expr_require_composition): Allow
>> 	DW_OP_GNU_uninit.
>> 	(execute_stack_op): Use dwarf_expr_require_composition instead of
>> 	copying its logic.
>
> OK.

Thanks, pushed.

--
Andreas
Tom Tromey Oct. 9, 2016, 5:36 p.m. UTC | #3 
>>>>> "Andreas" == Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

Andreas> This patch allows DW_OP_GNU_uninit after all operators
Andreas> requiring a composition, to simplify the code and make it more
Andreas> consistent.  This policy may be more permissive than necessary,
Andreas> but in the worst case just leads to a DWARF location
Andreas> description resulting in an uninitialized value instead of an
Andreas> error message.

I think it would be best to allow DW_OP_GNU_uninit to terminate any
piece, rather than require it to be at the end of the expression.  This
seems compatible and clearly more consistent with other DWARF
operations.

That is, assuming DW_OP_GNU_uninit is useful at all.
Another option would be to deprecate it.

Tom
Andreas Arnez Oct. 10, 2016, 12:24 p.m. UTC | #4 
On Sun, Oct 09 2016, Tom Tromey wrote:

>>>>>> "Andreas" == Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
> Andreas> This patch allows DW_OP_GNU_uninit after all operators
> Andreas> requiring a composition, to simplify the code and make it more
> Andreas> consistent.  This policy may be more permissive than necessary,
> Andreas> but in the worst case just leads to a DWARF location
> Andreas> description resulting in an uninitialized value instead of an
> Andreas> error message.
>
> I think it would be best to allow DW_OP_GNU_uninit to terminate any
> piece, rather than require it to be at the end of the expression.  This
> seems compatible and clearly more consistent with other DWARF
> operations.

You mean to allow DW_OP_GNU_uninit to terminate any simple location
description?  Right, this would allow marking individual structure
members as uninitialized, for instance.

> That is, assuming DW_OP_GNU_uninit is useful at all.
> Another option would be to deprecate it.

Right, I wonder about its usefulness as well.  For a variable with fixed
location it may cover a small window where the compiler can be certain
that the variable is uninitialized.  I guess this *might* be useful
sometimes?

Is there even a DWARF issue for this?  Or a formal specification?

--
Andreas
Tom Tromey Oct. 10, 2016, 10:40 p.m. UTC | #5 
>>>>> "Andreas" == Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

Andreas> You mean to allow DW_OP_GNU_uninit to terminate any simple
Andreas> location description?

Yes.

>> That is, assuming DW_OP_GNU_uninit is useful at all.
>> Another option would be to deprecate it.

Andreas> Right, I wonder about its usefulness as well.  For a variable
Andreas> with fixed location it may cover a small window where the
Andreas> compiler can be certain that the variable is uninitialized.  I
Andreas> guess this *might* be useful sometimes?

Andreas> Is there even a DWARF issue for this?  Or a formal
Andreas> specification?

I don't think so.  Last time I looked into this all I was able to find
were the patch submissions to gcc and gdb.  IIRC they weren't all that
informative though.

Tom
diff mbox

Patch

diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 7bf3c78..bf2107a 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -401,16 +401,15 @@  safe_skip_leb128 (const gdb_byte *buf, const gdb_byte *buf_end)
 
 
 /* Check that the current operator is either at the end of an
-   expression, or that it is followed by a composition operator.  */
+   expression, or that it is followed by a composition operator or by
+   DW_OP_GNU_uninit (which should terminate the expression).  */
 
 void
 dwarf_expr_require_composition (const gdb_byte *op_ptr, const gdb_byte *op_end,
 				const char *op_name)
 {
-  /* It seems like DW_OP_GNU_uninit should be handled here.  However,
-     it doesn't seem to make sense for DW_OP_*_value, and it was not
-     checked at the other place that this function is called.  */
-  if (op_ptr != op_end && *op_ptr != DW_OP_piece && *op_ptr != DW_OP_bit_piece)
+  if (op_ptr != op_end && *op_ptr != DW_OP_piece && *op_ptr != DW_OP_bit_piece
+      && *op_ptr != DW_OP_GNU_uninit)
     error (_("DWARF-2 expression error: `%s' operations must be "
 	     "used either alone or in conjunction with DW_OP_piece "
 	     "or DW_OP_bit_piece."),
@@ -818,13 +817,7 @@  execute_stack_op (struct dwarf_expr_context *ctx,
 	case DW_OP_reg29:
 	case DW_OP_reg30:
 	case DW_OP_reg31:
-	  if (op_ptr != op_end 
-	      && *op_ptr != DW_OP_piece
-	      && *op_ptr != DW_OP_bit_piece
-	      && *op_ptr != DW_OP_GNU_uninit)
-	    error (_("DWARF-2 expression error: DW_OP_reg operations must be "
-		     "used either alone or in conjunction with DW_OP_piece "
-		     "or DW_OP_bit_piece."));
+	  dwarf_expr_require_composition (op_ptr, op_end, "DW_OP_reg");
 
 	  result = op - DW_OP_reg0;
 	  result_val = value_from_ulongest (address_type, result);