diff mbox series

[committed] Fix ICE in maybe_record_trace_start

Message ID ad22d7c5-a3a3-ac9d-bbfb-4decc5c25c5e@redhat.com
State New
Headers show
Series [committed] Fix ICE in maybe_record_trace_start | expand

Commit Message

Jeff Law Feb. 12, 2018, 6:32 p.m. UTC
This was something my tester was tripping over on h8-elf.  I was hoping
it was going to fix the similar ICEs for the SH port, but alas those are
different.

The fundamental problem is generic code generated something like this:

(set (temp) (plus (stack_pointer_rtx) (const_int))
(set (stack_pointer_rtx) (temp))  REG_ARGS_SIZE note


The backward propagation step in cse.c turns the first insn into:

(set (stack_pointer_rtx) (plus (stack_pointer_rtx) (const_int))

And the second insn gets deleted, losing the REG_ARGS_SIZE note.

We then cross jump the tail of that block with the tail of another block
which has REG_ARGS_SIZE notes that do not get deleted.

The net result is at the commonized tail we have two paths which
different notions of REG_ARGS_SIZE and thus different CFIs, triggering
the ICE.

The most sensible way to fix this is to move the REG_ARGS_SIZE note
during the backward propagation step in cse.c

That allows the H8 port to build libgcc/newlib across all its multilib
variants.

I've also bootstrapped and regression tested on x86_64-linux-gnu, though
I doubt it really got exercised there.

Installing on the trunk.  Now back to the SH, rx and mips problems with
maybe_record_trace_start.

Jeff

Comments

Tom de Vries Feb. 22, 2018, 10:59 a.m. UTC | #1
On 02/12/2018 07:32 PM, Jeff Law wrote:
> diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> new file mode 100644
> index 00000000000..0ca0b9f034b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> @@ -0,0 +1,36 @@
> +int foo;
> +typedef long unsigned int size_t;
> +typedef short unsigned int wchar_t;
> +struct tm
> +{
> +  int tm_mday;
> +  int tm_mon;
> +  int tm_year;
> +};
> +size_t
> +__strftime (wchar_t * s, size_t maxsize, const wchar_t * format, const struct tm *tim_p)
> +{
> +  size_t count = 0;
> +  int len = 0;
> +  size_t i, ctloclen;
> +  unsigned long width;
> +  {
> +    if (foo)
> +      {
> +	{
> +	  wchar_t *fmt = L"%s%.*d";
> +	  len = swprintf (&s[count], maxsize, fmt, "-", width, 0);
> +	}
> +	if ((count) >= maxsize)
> +	  return 0;
> +      }
> +    else
> +      {
> +	len =
> +	  swprintf (&s[count], maxsize - count, L"%.2d/%.2d/%.2d", 42, 99, 0);
> +	if ((count) >= maxsize)
> +	  return 0;
> +
> +      }
> +  }
> +}

Hi,

when compiling this test for nvptx, the missing declaration for swprintf 
results in this default declaration in the .s file based on the 
arguments of the first call:
...
         .extern .func (.param.u32 %value_out) swprintf (.param.u64 
%in_ar0, .param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3, 
.param.u64 %in_ar4, .param.u32 %in_ar5);
...

and this error message when ptxas process the second call:
...
ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument 
does not match formal parameter '%in_ar3'
ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument 
does not match formal parameter '%in_ar4'
ptxas fatal   : Ptx assembly aborted due to errors
nvptx-as: ptxas returned 255 exit status
...

When adding a declaration in the source like so:
...
diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c 
b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
index 0ca0b9f034b..81943a2c459 100644
--- a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
+++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
@@ -1,6 +1,7 @@
  int foo;
  typedef long unsigned int size_t;
  typedef short unsigned int wchar_t;
+extern int swprintf(wchar_t *wcs, size_t maxlen, const wchar_t *format, 
...);
  struct tm
  {
    int tm_mday;
...

the declaration changes to:
...
.extern .func (.param.u32 %value_out) swprintf (.param.u64 %in_ar0, 
.param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3);
...
and test test-case passes.

Is it ok to update the test-case like this, or should it require 
effective target 'untyped_assembly' (which is false for nvptx).

Thanks,
- Tom
Jeff Law Feb. 23, 2018, 3:59 p.m. UTC | #2
On 02/22/2018 03:59 AM, Tom de Vries wrote:
> On 02/12/2018 07:32 PM, Jeff Law wrote:
>> diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> new file mode 100644
>> index 00000000000..0ca0b9f034b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> @@ -0,0 +1,36 @@
>> +int foo;
>> +typedef long unsigned int size_t;
>> +typedef short unsigned int wchar_t;
>> +struct tm
>> +{
>> +  int tm_mday;
>> +  int tm_mon;
>> +  int tm_year;
>> +};
>> +size_t
>> +__strftime (wchar_t * s, size_t maxsize, const wchar_t * format,
>> const struct tm *tim_p)
>> +{
>> +  size_t count = 0;
>> +  int len = 0;
>> +  size_t i, ctloclen;
>> +  unsigned long width;
>> +  {
>> +    if (foo)
>> +      {
>> +    {
>> +      wchar_t *fmt = L"%s%.*d";
>> +      len = swprintf (&s[count], maxsize, fmt, "-", width, 0);
>> +    }
>> +    if ((count) >= maxsize)
>> +      return 0;
>> +      }
>> +    else
>> +      {
>> +    len =
>> +      swprintf (&s[count], maxsize - count, L"%.2d/%.2d/%.2d", 42,
>> 99, 0);
>> +    if ((count) >= maxsize)
>> +      return 0;
>> +
>> +      }
>> +  }
>> +}
> 
> Hi,
> 
> when compiling this test for nvptx, the missing declaration for swprintf
> results in this default declaration in the .s file based on the
> arguments of the first call:
> ...
>         .extern .func (.param.u32 %value_out) swprintf (.param.u64
> %in_ar0, .param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3,
> .param.u64 %in_ar4, .param.u32 %in_ar5);
> ...
> 
> and this error message when ptxas process the second call:
> ...
> ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument
> does not match formal parameter '%in_ar3'
> ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument
> does not match formal parameter '%in_ar4'
> ptxas fatal   : Ptx assembly aborted due to errors
> nvptx-as: ptxas returned 255 exit status
> ...
> 
> When adding a declaration in the source like so:
> ...
> diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> index 0ca0b9f034b..81943a2c459 100644
> --- a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> +++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
> @@ -1,6 +1,7 @@
>  int foo;
>  typedef long unsigned int size_t;
>  typedef short unsigned int wchar_t;
> +extern int swprintf(wchar_t *wcs, size_t maxlen, const wchar_t *format,
> ...);
>  struct tm
>  {
>    int tm_mday;
> ...
> 
> the declaration changes to:
> ...
> .extern .func (.param.u32 %value_out) swprintf (.param.u64 %in_ar0,
> .param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3);
> ...
> and test test-case passes.
> 
> Is it ok to update the test-case like this, or should it require
> effective target 'untyped_assembly' (which is false for nvptx).
It's OK to update the test in either way -- I don't think either change
would compromise the test.  Adding the prototype seems like the better
choice to me.

I need to remember that PTX (and the PA) are much more sensitive to
these issues than any other port and once a testcase minimization is
complete to go back and make sure we have suitable prototypes.

jeff
Tom de Vries Feb. 26, 2018, 4:08 p.m. UTC | #3
On 02/23/2018 04:59 PM, Jeff Law wrote:
> On 02/22/2018 03:59 AM, Tom de Vries wrote:
>> On 02/12/2018 07:32 PM, Jeff Law wrote:
>>> diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>>> b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>>> new file mode 100644
>>> index 00000000000..0ca0b9f034b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>>> @@ -0,0 +1,36 @@
>>> +int foo;
>>> +typedef long unsigned int size_t;
>>> +typedef short unsigned int wchar_t;
>>> +struct tm
>>> +{
>>> +  int tm_mday;
>>> +  int tm_mon;
>>> +  int tm_year;
>>> +};
>>> +size_t
>>> +__strftime (wchar_t * s, size_t maxsize, const wchar_t * format,
>>> const struct tm *tim_p)
>>> +{
>>> +  size_t count = 0;
>>> +  int len = 0;
>>> +  size_t i, ctloclen;
>>> +  unsigned long width;
>>> +  {
>>> +    if (foo)
>>> +      {
>>> +    {
>>> +      wchar_t *fmt = L"%s%.*d";
>>> +      len = swprintf (&s[count], maxsize, fmt, "-", width, 0);
>>> +    }
>>> +    if ((count) >= maxsize)
>>> +      return 0;
>>> +      }
>>> +    else
>>> +      {
>>> +    len =
>>> +      swprintf (&s[count], maxsize - count, L"%.2d/%.2d/%.2d", 42,
>>> 99, 0);
>>> +    if ((count) >= maxsize)
>>> +      return 0;
>>> +
>>> +      }
>>> +  }
>>> +}
>>
>> Hi,
>>
>> when compiling this test for nvptx, the missing declaration for swprintf
>> results in this default declaration in the .s file based on the
>> arguments of the first call:
>> ...
>>          .extern .func (.param.u32 %value_out) swprintf (.param.u64
>> %in_ar0, .param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3,
>> .param.u64 %in_ar4, .param.u32 %in_ar5);
>> ...
>>
>> and this error message when ptxas process the second call:
>> ...
>> ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument
>> does not match formal parameter '%in_ar3'
>> ptxas regs-arg-size.o, line 97; error   : Type or alignment of argument
>> does not match formal parameter '%in_ar4'
>> ptxas fatal   : Ptx assembly aborted due to errors
>> nvptx-as: ptxas returned 255 exit status
>> ...
>>
>> When adding a declaration in the source like so:
>> ...
>> diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> index 0ca0b9f034b..81943a2c459 100644
>> --- a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> +++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
>> @@ -1,6 +1,7 @@
>>   int foo;
>>   typedef long unsigned int size_t;
>>   typedef short unsigned int wchar_t;
>> +extern int swprintf(wchar_t *wcs, size_t maxlen, const wchar_t *format,
>> ...);
>>   struct tm
>>   {
>>     int tm_mday;
>> ...
>>
>> the declaration changes to:
>> ...
>> .extern .func (.param.u32 %value_out) swprintf (.param.u64 %in_ar0,
>> .param.u64 %in_ar1, .param.u64 %in_ar2, .param.u64 %in_ar3);
>> ...
>> and test test-case passes.
>>
>> Is it ok to update the test-case like this, or should it require
>> effective target 'untyped_assembly' (which is false for nvptx).
> It's OK to update the test in either way -- I don't think either change
> would compromise the test.  Adding the prototype seems like the better
> choice to me.
> 

Done.

> I need to remember that PTX (and the PA) are much more sensitive to
> these issues than any other port and once a testcase minimization is
> complete to go back and make sure we have suitable prototypes.

I think the root cause here is that compile.exp uses "-w". Without that, 
a warning triggers:
...
regs-arg-size.c:22:10: warning: implicit declaration of function 
'swprintf' [-Wimplicit-function-declaration]...
...
and the test for excess errors fails.

Thanks,
- Tom
[testsuite] Add missing function decl to regs-arg-size.c

2018-02-26  Tom de Vries  <tom@codesourcery.com>

	* gcc.c-torture/compile/regs-arg-size.c (swprintf): Declare.

---
 gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
index 0ca0b9f..f5f0111 100644
--- a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
+++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
@@ -1,6 +1,7 @@
 int foo;
 typedef long unsigned int size_t;
 typedef short unsigned int wchar_t;
+extern int swprintf (wchar_t *wcs, size_t maxlen, const wchar_t *format, ...);
 struct tm
 {
   int tm_mday;
diff mbox series

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 5a264391268..d5913d0a7db 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-02-12  Jeff Law  <law@redhat.com>
+
+	* cse.c (try_back_substitute_reg): Move any REG_ARGS_SIZE note when
+	successfully back substituting a reg.
+
 2018-02-12  Richard Biener  <rguenther@suse.de>
 
 	PR tree-optimization/84037
diff --git a/gcc/cse.c b/gcc/cse.c
index 825b0bd8989..a73a771041a 100644
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -4256,6 +4256,15 @@  try_back_substitute_reg (rtx set, rtx_insn *insn)
 		  && (reg_mentioned_p (dest, XEXP (note, 0))
 		      || rtx_equal_p (src, XEXP (note, 0))))
 		remove_note (insn, note);
+
+	      /* If INSN has a REG_ARGS_SIZE note, move it to PREV.  */
+	      note = find_reg_note (insn, REG_ARGS_SIZE, NULL_RTX);
+	      if (note != 0)
+		{
+		  remove_note (insn, note);
+		  gcc_assert (!find_reg_note (prev, REG_ARGS_SIZE, NULL_RTX));
+		  set_unique_reg_note (prev, REG_ARGS_SIZE, XEXP (note, 0));
+		}
 	    }
 	}
     }
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index dba0bedb7cf..8f22a65c7bb 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-02-12  Jeff Law  <law@redhat.com>
+
+	* gcc.c-torture/compile/reg-args-size.c: New test.
+
 2018-02-12  Carl Love  <cel@us.ibm.com>
 
 	* gcc.target/powerpc/builtins-4-runnable.c (main): Move int128 and
diff --git a/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
new file mode 100644
index 00000000000..0ca0b9f034b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/regs-arg-size.c
@@ -0,0 +1,36 @@ 
+int foo;
+typedef long unsigned int size_t;
+typedef short unsigned int wchar_t;
+struct tm
+{
+  int tm_mday;
+  int tm_mon;
+  int tm_year;
+};
+size_t
+__strftime (wchar_t * s, size_t maxsize, const wchar_t * format, const struct tm *tim_p)
+{
+  size_t count = 0;
+  int len = 0;
+  size_t i, ctloclen;
+  unsigned long width;
+  {
+    if (foo)
+      {
+	{
+	  wchar_t *fmt = L"%s%.*d";
+	  len = swprintf (&s[count], maxsize, fmt, "-", width, 0);
+	}
+	if ((count) >= maxsize)
+	  return 0;
+      }
+    else
+      {
+	len =
+	  swprintf (&s[count], maxsize - count, L"%.2d/%.2d/%.2d", 42, 99, 0);
+	if ((count) >= maxsize)
+	  return 0;
+
+      }
+  }
+}