Message ID | 3103467.5fSG56mABF@fomalhaut |
---|---|
State | New |
Headers | show |
Series | Do not use DW_OP_not for TRUTH_NOT_EXPR in conditional expressions | expand |
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
> 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;
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
> 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.
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
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
> 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?
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
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)