Tweak dwarf2out const_ok_for_output* (PR debug/88635)

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?

2019-01-03  Jakub Jelinek  <jakub@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.


	Jakub

On January 3, 2019 11:36:00 PM GMT+01:00, Jakub Jelinek <jakub@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  <jakub@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