On January 3, 2019 11:36:00 PM GMT+01:00, Jakub Jelinek <ja...@redhat.com>
wrote:
>Hi!
>
>The following testcase FAILs on 8.x branch, went latent later on on the
>trunk and I suppose Alex' i386.c ix86_const_not_ok_for_debug_p change
>would
>have prevented it too.
>
>The problem is in what the comment in const_ok_for_output* says, we
>don't do
>sufficient checking of what kind of expressions we accept inside of
>CONST
>and we could have there something that the assembler doesn't really
>like.
>In particular, in the testcase we ended up with
>(const:P (minus:P (const_int -2) (unspec [(symbol_ref ...)]
>UNSPEC_GOTOFF)))
>and as doesn't like -2-.LC0@gotoff.
>
>We already have some hacks in there, like punt on NOT or NEG. The
>following
>patch extends it, by requiring that the CONST doesn't contain e.g. .LC0
>+ .LC1
>or any kind of labels in the second operand of MINUS (which is like
>negation). In theory, we could allow .LC0 - .LC1 if both labels are in
>the
>same section, but the patch tries to be safe.
>
>After writing the patch, I've noticed Alex made
>ix86_const_not_ok_for_debug_p
>changes, but I think they aren't enough, they will still fail on e.g.
>all the cases
>where there are SYMBOL_REFs rather than UNSPECs involved, and on the
>other
>side it disallows 16+.LC0@gotoff which happens quite often.
>
>If const_ok_for_output rejects the whole CONST, there is already code
>to try
>to split it up into smaller expressions, so e.g. that .LC0 - .LC1 would
>be
>emitted then as DW_OP_addr .LC0 DW_OP_addr .LC1 DW_OP_minus, this patch
>extends it so that it can handle the UNSPECs that way as well, so the
>above
>would be DW_OP_const1s 0xff DW_OP_addr .LC0@gotoff DW_OP_minus.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and
>without the i386.c part after a while for 8.x?
OK.
Richard.
>2019-01-03 Jakub Jelinek <ja...@redhat.com>
>
> PR debug/88635
> * dwarf2out.c (const_ok_for_output_1): Reject MINUS that contains
> SYMBOL_REF, CODE_LABEL or UNSPEC in subexpressions of second argument.
> Reject PLUS that contains SYMBOL_REF, CODE_LABEL or UNSPEC in
> subexpressions of both operands.
> (mem_loc_descriptor): Handle UNSPEC if target hook acks it and all the
> subrtxes are CONSTANT_P.
> * config/i386/i386.c (ix86_const_not_ok_for_debug_p): Revert
> 2018-11-09 changes.
>
> * gcc.dg/debug/dwarf2/pr88635.c: New test.
>
>--- gcc/dwarf2out.c.jj 2019-01-03 12:04:45.720730572 +0100
>+++ gcc/dwarf2out.c 2019-01-03 14:35:02.897782400 +0100
>@@ -14464,6 +14464,41 @@ const_ok_for_output_1 (rtx rtl)
> case NOT:
> case NEG:
> return false;
>+ case PLUS:
>+ {
>+ /* Make sure SYMBOL_REFs/UNSPECs are at most in one of the
>+ operands. */
>+ subrtx_var_iterator::array_type array;
>+ bool first = false;
>+ FOR_EACH_SUBRTX_VAR (iter, array, XEXP (rtl, 0), ALL)
>+ if (SYMBOL_REF_P (*iter)
>+ || LABEL_P (*iter)
>+ || GET_CODE (*iter) == UNSPEC)
>+ {
>+ first = true;
>+ break;
>+ }
>+ if (!first)
>+ return true;
>+ FOR_EACH_SUBRTX_VAR (iter, array, XEXP (rtl, 1), ALL)
>+ if (SYMBOL_REF_P (*iter)
>+ || LABEL_P (*iter)
>+ || GET_CODE (*iter) == UNSPEC)
>+ return false;
>+ return true;
>+ }
>+ case MINUS:
>+ {
>+ /* Disallow negation of SYMBOL_REFs or UNSPECs when they
>+ appear in the second operand of MINUS. */
>+ subrtx_var_iterator::array_type array;
>+ FOR_EACH_SUBRTX_VAR (iter, array, XEXP (rtl, 1), ALL)
>+ if (SYMBOL_REF_P (*iter)
>+ || LABEL_P (*iter)
>+ || GET_CODE (*iter) == UNSPEC)
>+ return false;
>+ return true;
>+ }
> default:
> return true;
> }
>@@ -15607,6 +15642,7 @@ mem_loc_descriptor (rtx rtl, machine_mod
> pool. */
> case CONST:
> case SYMBOL_REF:
>+ case UNSPEC:
> if (!is_a <scalar_int_mode> (mode, &int_mode)
> || (GET_MODE_SIZE (int_mode) > DWARF2_ADDR_SIZE
> #ifdef POINTERS_EXTEND_UNSIGNED
>@@ -15614,6 +15650,30 @@ mem_loc_descriptor (rtx rtl, machine_mod
> #endif
> ))
> break;
>+
>+ if (GET_CODE (rtl) == UNSPEC)
>+ {
>+ /* If delegitimize_address couldn't do anything with the UNSPEC, we
>+ can't express it in the debug info. This can happen e.g. with
>some
>+ TLS UNSPECs. Allow UNSPECs formerly from CONST that the backend
>+ approves. */
>+ bool not_ok = false;
>+ subrtx_var_iterator::array_type array;
>+ FOR_EACH_SUBRTX_VAR (iter, array, rtl, ALL)
>+ if ((*iter != rtl && !CONSTANT_P (*iter))
>+ || !const_ok_for_output_1 (*iter))
>+ {
>+ not_ok = true;
>+ break;
>+ }
>+
>+ if (not_ok)
>+ break;
>+
>+ rtl = gen_rtx_CONST (GET_MODE (rtl), rtl);
>+ goto symref;
>+ }
>+
> if (GET_CODE (rtl) == SYMBOL_REF
> && SYMBOL_REF_TLS_MODEL (rtl) != TLS_MODEL_NONE)
> {
>@@ -16282,7 +16342,6 @@ mem_loc_descriptor (rtx rtl, machine_mod
> case VEC_CONCAT:
> case VEC_DUPLICATE:
> case VEC_SERIES:
>- case UNSPEC:
> case HIGH:
> case FMA:
> case STRICT_LOW_PART:
>@@ -16291,9 +16350,6 @@ mem_loc_descriptor (rtx rtl, machine_mod
> case CLRSB:
> case CLOBBER:
> case CLOBBER_HIGH:
>- /* If delegitimize_address couldn't do anything with the UNSPEC,
>we
>- can't express it in the debug info. This can happen e.g. with some
>- TLS UNSPECs. */
> break;
>
> case CONST_STRING:
>--- gcc/config/i386/i386.c.jj 2019-01-01 12:37:32.382725150 +0100
>+++ gcc/config/i386/i386.c 2019-01-03 14:32:43.050093691 +0100
>@@ -17240,18 +17240,6 @@ ix86_const_not_ok_for_debug_p (rtx x)
> if (SYMBOL_REF_P (x) && strcmp (XSTR (x, 0), GOT_SYMBOL_NAME) == 0)
> return true;
>
>- /* Reject UNSPECs within expressions. We could accept symbol@gotoff
>- + literal_constant, but that would hardly come up in practice,
>- and it's not worth the trouble of having to reject that as an
>- operand to pretty much anything else. */
>- if (UNARY_P (x)
>- && GET_CODE (XEXP (x, 0)) == UNSPEC)
>- return true;
>- if (BINARY_P (x)
>- && (GET_CODE (XEXP (x, 0)) == UNSPEC
>- || GET_CODE (XEXP (x, 1)) == UNSPEC))
>- return true;
>-
> return false;
> }
> >
>--- gcc/testsuite/gcc.dg/debug/dwarf2/pr88635.c.jj 2019-01-03
>14:32:43.050093691 +0100
>+++ gcc/testsuite/gcc.dg/debug/dwarf2/pr88635.c 2019-01-03
>14:32:43.050093691 +0100
>@@ -0,0 +1,24 @@
>+/* PR debug/88635 */
>+/* { dg-do assemble } */
>+/* { dg-options "-g -O2" } */
>+/* { dg-additional-options "-fpie" { target pie } } */
>+
>+static void
>+foo (char *b)
>+{
>+ unsigned c = 0;
>+ --c;
>+ do
>+ if (++*b++ == 0)
>+ break;
>+ while (--c);
>+ if (c == 0)
>+ while (*b++)
>+ ;
>+}
>+
>+void
>+bar (void)
>+{
>+ foo ("");
>+}
>
> Jakub
