Re: [PATCH] Permit DW_OP_GNU_uninit to be used with DW_OP_piece

[Kevin Buettner <kevinb@redhat.com>]

On Mon, 05 Jun 2023 07:17:08 -0600
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Kevin" == Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> writes:  
> 
> Kevin> The test case in that bug report involved running gdb against the 'jq'
> Kevin> program (which is a command-line JSON processor) on Fedora 37.  Since
> Kevin> the debug info is compiler (and compile-time option) dependent, it
> Kevin> won't necessarily show up in other distributions or even past or
> Kevin> future versions of Fedora.  (E.g. when trying the example shown below
> Kevin> on Fedora 38, GDB says that the value of 'value' has been optimized
> Kevin> out.  I.e. it does not demonstrate the same DWARF error that can be
> Kevin> see when using Fedora 37.)  
> 
> Ages ago, during one of the rewrites of the DWARF expression evaluator,
> I found out that DW_OP_GNU_uninit was not really documented.  So, I just
> made it continue to work the way it had been described in the gdb
> sources.
> 
> My recollection is that it required a special compiler argument to even
> get gcc to emit it.  It's still undocumented as far as I can tell.
> 
> So I'd suggest perhaps you could get someone to remove it from GCC, or
> from the jq build.  It's barely used and IMO not really useful.
> 
> That said, it's fine by me if you want to keep it around.

So long as there's a producer for it, even if only rarely used,
I think we should keep it around.

> 
> Kevin>  	case DW_OP_GNU_uninit:
> Kevin> -	  if (op_ptr != op_end)
> Kevin> +	  if (op_ptr != op_end && *op_ptr != DW_OP_piece)
> Kevin>  	    error (_("DWARF-2 expression error: DW_OP_GNU_uninit must always "
> Kevin>  		   "be the very last op."));  
> 
> If DW_OP_GNU_uninit can end a piece then the error message here should
> be updated.

I've updated the error message - see below.

> Also did you look at the other spots that understand DWARF expressions?
> There are 3 or 4 in gdb.

I found another spot in gdb/dwarf2/expr.c which deals DW_OP_GNU_uninit.
It has already been updated to handle both DW_OP_piece and DW_OP_bit_piece.
I updated my patch to also handle DW_OP_bit_piece.

The other occurrences of DW_OP_GNU_uninit also didn't need updating. 
They were just cases in switch statements.

> Aside from this the patch is OK.

Thanks - I've pushed it.

The test case stayed the same, but this is what the change to
gdb/dwarf2/expr.c ended up looking like:

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index ccdd87f3a55..7e1666165d7 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -2200,9 +2200,11 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	  goto no_push;
 
 	case DW_OP_GNU_uninit:
-	  if (op_ptr != op_end)
+	  if (op_ptr != op_end && *op_ptr != DW_OP_piece
+	      && *op_ptr != DW_OP_bit_piece)
 	    error (_("DWARF-2 expression error: DW_OP_GNU_uninit must always "
-		   "be the very last op."));
+		   "be the very last op in a DWARF expression or "
+		   "DW_OP_piece/DW_OP_bit_piece piece."));
 
 	  this->m_initialized = false;
 	  goto no_push;