Patchwork Fix PR rtl-optimization/pr60663

login
register
mail settings
Submitter Zhenqiang Chen
Date March 26, 2014, 6:16 a.m.
Message ID <CACgzC7BAgdoKmKdNTnqoB+WgihgrAhSc4=FdXVUCRB7Pw6T+6Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/333787/
State New
Headers show

Comments

Zhenqiang Chen - March 26, 2014, 6:16 a.m.
Hi,

The patch checks the number of the expected operands in
ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to
make sure the ASM_OPERANDS are legal.

Bootstrap and no make check regression on X86-64 and ARM chromebook.

OK for trunk?

Thanks!
-Zhenqiang

ChangeLog:
2014-03-26  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

    PR rtl-optimization/pr60663
    * recog.c (check_asm_operands): Check the number of expected operands.

testsuite/ChangeLog:
2014-03-26  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * gcc.dg/pr60663: New testcase.
Jakub Jelinek - March 26, 2014, 7 a.m.
On Wed, Mar 26, 2014 at 02:16:16PM +0800, Zhenqiang Chen wrote:
> The patch checks the number of the expected operands in
> ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to
> make sure the ASM_OPERANDS are legal.
> 
> Bootstrap and no make check regression on X86-64 and ARM chromebook.
> 
> OK for trunk?

No, this is very wrong.  How many operands you refer to and how many times
to each is completely unrelated to the fact that CSE should never modify
asm insns to drop some of the outputs.
You can refer to no operands at all and still it would be invalid to drop
the outputs, or you can have output template like "%0 %0 %0 %0 %0 ... million times %0".

	Jakub
Zhenqiang Chen - March 26, 2014, 7:30 a.m.
On 26 March 2014 15:00, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 26, 2014 at 02:16:16PM +0800, Zhenqiang Chen wrote:
>> The patch checks the number of the expected operands in
>> ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to
>> make sure the ASM_OPERANDS are legal.
>>
>> Bootstrap and no make check regression on X86-64 and ARM chromebook.
>>
>> OK for trunk?
>
> No, this is very wrong.  How many operands you refer to and how many times

This is how the output_asm_insn (final.c) check and output the error
message. The logic in my patch is the same as it in output_asm_insn.

> to each is completely unrelated to the fact that CSE should never modify
> asm insns to drop some of the outputs.

Agree. CSE should never modify asm insns to drop some of the outputs.

But in this case, CSE does not drop any of the outputs. It just takes
the SRC of a set and replace the reference of the set. And the
instruction validation tells CSE that it is legal instruction after
replacement. (The original correct asm insn is optimized away after
this replacement)

I think it is common for most rtl-optimizations to do such kind of
validation. So to avoid such kind of bug, check_asm_operands must tell
the optimizer the asm is illegal.

> You can refer to no operands at all and still it would be invalid to drop
> the outputs, or you can have output template like "%0 %0 %0 %0 %0 ... million times %0".

I think it does not check the number of %0, but the max "n" in %n.

Thanks!
-Zhenqiang

>         Jakub
Jakub Jelinek - March 26, 2014, 7:45 a.m.
On Wed, Mar 26, 2014 at 03:30:44PM +0800, Zhenqiang Chen wrote:
> Agree. CSE should never modify asm insns to drop some of the outputs.

So the right fix is top prevent this from happening, not papering over about
it.
> 
> But in this case, CSE does not drop any of the outputs. It just takes
> the SRC of a set and replace the reference of the set. And the
> instruction validation tells CSE that it is legal instruction after
> replacement. (The original correct asm insn is optimized away after
> this replacement)
> 
> I think it is common for most rtl-optimizations to do such kind of
> validation. So to avoid such kind of bug, check_asm_operands must tell
> the optimizer the asm is illegal.

As it is wrong if CSE does that even with asm ("" : "=r" (i), "=r" (j));,
your patch is not the right place to fix this.  CSE just must check where
the ASM_OPERANDS is coming from and if it comes from a PARALLEL with
multiple outputs, either give up or duplicate all outputs (if it makes sense
at all).  Or just don't enter into the hash tables ASM_OPERANDS with
multiple outputs.

	Jakub
Zhenqiang Chen - March 26, 2014, 9:11 a.m.
On 26 March 2014 15:45, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 26, 2014 at 03:30:44PM +0800, Zhenqiang Chen wrote:
>> Agree. CSE should never modify asm insns to drop some of the outputs.
>
> So the right fix is top prevent this from happening, not papering over about
> it.
>>
>> But in this case, CSE does not drop any of the outputs. It just takes
>> the SRC of a set and replace the reference of the set. And the
>> instruction validation tells CSE that it is legal instruction after
>> replacement. (The original correct asm insn is optimized away after
>> this replacement)
>>
>> I think it is common for most rtl-optimizations to do such kind of
>> validation. So to avoid such kind of bug, check_asm_operands must tell
>> the optimizer the asm is illegal.
>
> As it is wrong if CSE does that even with asm ("" : "=r" (i), "=r" (j));,
> your patch is not the right place to fix this.  CSE just must check where
> the ASM_OPERANDS is coming from and if it comes from a PARALLEL with
> multiple outputs, either give up or duplicate all outputs (if it makes sense
> at all).  Or just don't enter into the hash tables ASM_OPERANDS with
> multiple outputs.

Thanks for the comments. I will rework the patch to check it in CSE.

-Zhenqiang

>         Jakub

Patch

diff --git a/gcc/recog.c b/gcc/recog.c
index f9040dc..65078ad 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -135,8 +135,8 @@  check_asm_operands (rtx x)
 {
   int noperands;
   rtx *operands;
-  const char **constraints;
-  int i;
+  const char **constraints, *templ;
+  int i, c;

   if (!asm_labels_ok (x))
     return 0;
@@ -159,7 +159,29 @@  check_asm_operands (rtx x)
   operands = XALLOCAVEC (rtx, noperands);
   constraints = XALLOCAVEC (const char *, noperands);

-  decode_asm_operands (x, operands, NULL, constraints, NULL, NULL);
+  templ = decode_asm_operands (x, operands, NULL, constraints, NULL, NULL);
+  /* The following logic is similar with it in output_asm_insn (final.c).
+     It checks the number of expected operands in ASM_OPERANDS_TEMPLATE.  */
+  if (*templ)
+    {
+      const char* p = templ;
+      while ((c = *p++))
+       {
+         if (c == '%')
+           if (ISDIGIT (*p))
+             {
+               int opnum;
+               char *endptr;
+
+               opnum = strtoul (p, &endptr, 10);
+               if (opnum >= noperands)
+                 return 0;
+
+                p = endptr;
+                c = *p;
+             }
+       }
+    }

   for (i = 0; i < noperands; i++)
     {

diff --git a/gcc/testsuite/gcc.dg/pr60663.c b/gcc/testsuite/gcc.dg/pr60663.c
new file mode 100644
index 0000000..6c01084
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lp.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options " -O2 " } */
+
+int g (void)
+{
+  unsigned i, j;
+  asm("// %0 %1" : "=r" (i), "=r"(j));
+  return i;
+}