[PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Eric Botcazou]

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

Hi,

DW_OP_not is a bitwise, not a logical NOT, so it computes the wrong result in 
a DWARF conditional expression.

Tested (GCC + GDB° on x86-64/Linux, OK for the mainline?


2022-05-13  Eric Botcazou  <ebotcazou@adacore.com>

	* dwarf2out.c (loc_list_from_tree_1) <COND_EXPR>: Swap the operands
	if the condition is a TRUTH_NOT_EXPR.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 836 bytes --]

diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 5681b01749a..affa2b5e52e 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -19497,6 +19497,15 @@ loc_list_from_tree_1 (tree loc, int want_address,
 	  list_ret
 	    = loc_list_from_tree_1 (TREE_OPERAND (TREE_OPERAND (loc, 0), 0),
 				    0, context);
+	/* DW_OP_not is a bitwise, not a logical NOT, so avoid it.  */
+	else if (TREE_CODE (TREE_OPERAND (loc, 0)) == TRUTH_NOT_EXPR)
+	  {
+	    lhs = loc_descriptor_from_tree (TREE_OPERAND (loc, 2), 0, context);
+	    rhs = loc_list_from_tree_1 (TREE_OPERAND (loc, 1), 0, context);
+	    list_ret
+	      = loc_list_from_tree_1 (TREE_OPERAND (TREE_OPERAND (loc, 0), 0),
+				      0, context);
+	  }
 	else
 	  list_ret = loc_list_from_tree_1 (TREE_OPERAND (loc, 0), 0, context);
 	if (list_ret == 0 || lhs == 0 || rhs == 0)

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Richard Biener]

On Fri, May 13, 2022 at 10:21 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> DW_OP_not is a bitwise, not a logical NOT, so it computes the wrong result in
> a DWARF conditional expression.
>
> Tested (GCC + GDB° on x86-64/Linux, OK for the mainline?

But this doesn't fix

    case TRUTH_NOT_EXPR:
    case BIT_NOT_EXPR:
      op = DW_OP_not;
      goto do_unop;

I also wonder where we get the TRUTH_NOT_EXPR to expand from?  I suspect
some non-gimplified global tree?

Thanks,
Richard.

>
> 2022-05-13  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * dwarf2out.c (loc_list_from_tree_1) <COND_EXPR>: Swap the operands
>         if the condition is a TRUTH_NOT_EXPR.
>
> --
> Eric Botcazou

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Eric Botcazou]

> But this doesn't fix
> 
>     case TRUTH_NOT_EXPR:
>     case BIT_NOT_EXPR:
>       op = DW_OP_not;
>       goto do_unop;

Nope (I couldn't trigger it after my change).

> I also wonder where we get the TRUTH_NOT_EXPR to expand from?  I suspect
> some non-gimplified global tree?

Yes, it's in the TYPE_SIZE of a global type:

package P is

  type Rec (Defined : Boolean) is record
    case Defined is
       when false => null;
       when others => I : Integer;
    end case;
  end record;

  A : access Rec;

end P;

-- 
Eric Botcazou

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Jakub Jelinek]

On Fri, May 13, 2022 at 10:25:02AM +0200, Richard Biener via Gcc-patches wrote:
> On Fri, May 13, 2022 at 10:21 AM Eric Botcazou via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > DW_OP_not is a bitwise, not a logical NOT, so it computes the wrong result in
> > a DWARF conditional expression.
> >
> > Tested (GCC + GDB° on x86-64/Linux, OK for the mainline?
> 
> But this doesn't fix
> 
>     case TRUTH_NOT_EXPR:
>     case BIT_NOT_EXPR:
>       op = DW_OP_not;
>       goto do_unop;
> 
> I also wonder where we get the TRUTH_NOT_EXPR to expand from?  I suspect
> some non-gimplified global tree?

So shouldn't we expand TRUTH_NOT_EXPR as
(push argument)
DW_OP_lit0
DW_OP_eq
then?

	Jakub

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Eric Botcazou]

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

> But this doesn't fix
> 
>     case TRUTH_NOT_EXPR:
>     case BIT_NOT_EXPR:
>       op = DW_OP_not;
>       goto do_unop;

Revised patch attached, using Jakub's suggestion.  The original (buggy) DWARF 
procedure for the Ada testcase I previously posted is:

	.uleb128 0x8	# (DIE (0x5b) DW_TAG_dwarf_procedure)
	.uleb128 0xc	# DW_AT_location
	.byte	0x12	# DW_OP_dup
	.byte	0x20	# DW_OP_not
	.byte	0x28	# DW_OP_bra
	.value	0x4
	.byte	0x34	# DW_OP_lit4
	.byte	0x2f	# DW_OP_skip
	.value	0x1
	.byte	0x30	# DW_OP_lit0
	.byte	0x16	# DW_OP_swap
	.byte	0x13	# DW_OP_drop

With Jakub's fix:

	.uleb128 0x8	# (DIE (0x5b) DW_TAG_dwarf_procedure)
	.uleb128 0xd	# DW_AT_location
	.byte	0x12	# DW_OP_dup
	.byte	0x30	# DW_OP_lit0
	.byte	0x29	# DW_OP_eq
	.byte	0x28	# DW_OP_bra
	.value	0x4
	.byte	0x34	# DW_OP_lit4
	.byte	0x2f	# DW_OP_skip
	.value	0x1
	.byte	0x30	# DW_OP_lit0
	.byte	0x16	# DW_OP_swap
	.byte	0x13	# DW_OP_drop

With mine:

	.uleb128 0x8	# (DIE (0x5b) DW_TAG_dwarf_procedure)
	.uleb128 0xb	# DW_AT_location
	.byte	0x12	# DW_OP_dup
	.byte	0x28	# DW_OP_bra
	.value	0x4
	.byte	0x30	# DW_OP_lit0
	.byte	0x2f	# DW_OP_skip
	.value	0x1
	.byte	0x34	# DW_OP_lit4
	.byte	0x16	# DW_OP_swap
	.byte	0x13	# DW_OP_drop


	* dwarf2out.c (loc_list_from_tree_1) <TRUTH_NOT_EXPR>: Do a logical
	instead of a bitwise negation.
	<COND_EXPR>: Swap the operands if the condition is TRUTH_NOT_EXPR.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1308 bytes --]

diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 5681b01749a..c333c595026 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -19449,6 +19449,14 @@ loc_list_from_tree_1 (tree loc, int want_address,
       break;
 
     case TRUTH_NOT_EXPR:
+      list_ret = loc_list_from_tree_1 (TREE_OPERAND (loc, 0), 0, context);
+      if (list_ret == 0)
+	return 0;
+
+      add_loc_descr_to_each (list_ret, new_loc_descr (DW_OP_lit0, 0, 0));
+      add_loc_descr_to_each (list_ret, new_loc_descr (DW_OP_eq, 0, 0));
+      break;
+
     case BIT_NOT_EXPR:
       op = DW_OP_not;
       goto do_unop;
@@ -19497,6 +19505,15 @@ loc_list_from_tree_1 (tree loc, int want_address,
 	  list_ret
 	    = loc_list_from_tree_1 (TREE_OPERAND (TREE_OPERAND (loc, 0), 0),
 				    0, context);
+	/* Likewise, swap the operands for a logically negated condition.  */
+	else if (TREE_CODE (TREE_OPERAND (loc, 0)) == TRUTH_NOT_EXPR)
+	  {
+	    lhs = loc_descriptor_from_tree (TREE_OPERAND (loc, 2), 0, context);
+	    rhs = loc_list_from_tree_1 (TREE_OPERAND (loc, 1), 0, context);
+	    list_ret
+	      = loc_list_from_tree_1 (TREE_OPERAND (TREE_OPERAND (loc, 0), 0),
+				      0, context);
+	  }
 	else
 	  list_ret = loc_list_from_tree_1 (TREE_OPERAND (loc, 0), 0, context);
 	if (list_ret == 0 || lhs == 0 || rhs == 0)

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Richard Biener]

On Mon, May 16, 2022 at 9:06 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > But this doesn't fix
> >
> >     case TRUTH_NOT_EXPR:
> >     case BIT_NOT_EXPR:
> >       op = DW_OP_not;
> >       goto do_unop;
>
> Revised patch attached, using Jakub's suggestion.  The original (buggy) DWARF
> procedure for the Ada testcase I previously posted is:
>
>         .uleb128 0x8    # (DIE (0x5b) DW_TAG_dwarf_procedure)
>         .uleb128 0xc    # DW_AT_location
>         .byte   0x12    # DW_OP_dup
>         .byte   0x20    # DW_OP_not
>         .byte   0x28    # DW_OP_bra
>         .value  0x4
>         .byte   0x34    # DW_OP_lit4
>         .byte   0x2f    # DW_OP_skip
>         .value  0x1
>         .byte   0x30    # DW_OP_lit0
>         .byte   0x16    # DW_OP_swap
>         .byte   0x13    # DW_OP_drop
>
> With Jakub's fix:
>
>         .uleb128 0x8    # (DIE (0x5b) DW_TAG_dwarf_procedure)
>         .uleb128 0xd    # DW_AT_location
>         .byte   0x12    # DW_OP_dup
>         .byte   0x30    # DW_OP_lit0
>         .byte   0x29    # DW_OP_eq
>         .byte   0x28    # DW_OP_bra
>         .value  0x4
>         .byte   0x34    # DW_OP_lit4
>         .byte   0x2f    # DW_OP_skip
>         .value  0x1
>         .byte   0x30    # DW_OP_lit0
>         .byte   0x16    # DW_OP_swap
>         .byte   0x13    # DW_OP_drop
>
> With mine:
>
>         .uleb128 0x8    # (DIE (0x5b) DW_TAG_dwarf_procedure)
>         .uleb128 0xb    # DW_AT_location
>         .byte   0x12    # DW_OP_dup
>         .byte   0x28    # DW_OP_bra
>         .value  0x4
>         .byte   0x30    # DW_OP_lit0
>         .byte   0x2f    # DW_OP_skip
>         .value  0x1
>         .byte   0x34    # DW_OP_lit4
>         .byte   0x16    # DW_OP_swap
>         .byte   0x13    # DW_OP_drop
>
>
>         * dwarf2out.c (loc_list_from_tree_1) <TRUTH_NOT_EXPR>: Do a logical
>         instead of a bitwise negation.
>         <COND_EXPR>: Swap the operands if the condition is TRUTH_NOT_EXPR.

LGTM.

Thanks,
Richard.

> --
> Eric Botcazou

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Jakub Jelinek]

On Mon, May 16, 2022 at 09:45:18AM +0200, Richard Biener via Gcc-patches wrote:
> >         * dwarf2out.c (loc_list_from_tree_1) <TRUTH_NOT_EXPR>: Do a logical
> >         instead of a bitwise negation.
> >         <COND_EXPR>: Swap the operands if the condition is TRUTH_NOT_EXPR.
> 
> LGTM.

It won't work for types larger than size of address, it would need to use
dwarf_OP (DW_OP_const_type) instead of DW_OP_lit0 in that case.
But maybe TRUTH_NOT_EXPR will be never seen for such types and after all,
even the loc_list_from_tree_1 INTEGER_CST case doesn't handle that
(the RTL case does).
So I think at least for now it is ok.

	Jakub

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Eric Botcazou]

> It won't work for types larger than size of address, it would need to use
> dwarf_OP (DW_OP_const_type) instead of DW_OP_lit0 in that case.
> But maybe TRUTH_NOT_EXPR will be never seen for such types and after all,
> even the loc_list_from_tree_1 INTEGER_CST case doesn't handle that
> (the RTL case does).
> So I think at least for now it is ok.

Thanks.  Any objection to me installing it on the 12 branch as well?

-- 
Eric Botcazou

Re: [PATCH] Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions

[Jakub Jelinek]

On Mon, May 16, 2022 at 10:47:53AM +0200, Eric Botcazou wrote:
> > It won't work for types larger than size of address, it would need to use
> > dwarf_OP (DW_OP_const_type) instead of DW_OP_lit0 in that case.
> > But maybe TRUTH_NOT_EXPR will be never seen for such types and after all,
> > even the loc_list_from_tree_1 INTEGER_CST case doesn't handle that
> > (the RTL case does).
> > So I think at least for now it is ok.
> 
> Thanks.  Any objection to me installing it on the 12 branch as well?

Nope.  It is ok.

	Jakub