Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Dec 3, 2013 at 12:45 AM, Michael Zolotukhin
<michael.v.zolotukhin@gmail.com> wrote:
> Hi Uros, HJ,
>
> I checked expand_movmem_epilogue - it seemingly doesn't have such a
> problem, so the patch is ok.
>
> We might want to add similar adjust_automodify_address_nv call to here as well:
>     if (TARGET_64BIT)
>         {
>           dest = change_address (destmem, DImode, destptr);
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>         }
>       else
>         {
>           dest = change_address (destmem, SImode, destptr);
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>         }
> (code snippet from previous HJ's comment in bugzilla).
> I think it's needed here, but I didn't manage to exploit the bug in
> this code. Maybe Uros or Jan can comment whether it's needed in such
> code or not.

It is almost impossible to reach those codes with current
memset/memcpy strategy.  expand_setmem_epilogue is
called either with a constant count or max_size > 32.
None of them will trigger those codes.  But we should
fix them with proper address.  Otherwise, we will run
into the similar problem if they are used one day in
the future.

> Thanks,
> Michael
>
>
> On 3 December 2013 12:11, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 2:05 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>
>>> emit_memset fails to adjust destination address after gen_strset, which
>>> leads to the wrong address in aliasing info.  This patch fixes it.
>>> Tested on Linux/x86-64.  OK to install?
>>>
>>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/59363
>>>         * config/i386/i386.c (emit_memset): Adjust destination address
>>>         after gen_strset.
>>>
>>> gcc/testsuite/
>>>
>>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/59363
>>>         * gcc.target/i386/pr59363.c: New file.
>>
>> OK, but according to [1], there are other places where similar issues
>> should be fixed. I propose to wait for Michael's analysis and eventual
>> patch, and fix them all together.
>>
>> [1] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59363#c21
>>

Here is the updated patch.  Tested on Linux/x86-64.  It
fixed git.  OK to install?

Thanks.

-- 
H.J.
---
gcc/

2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>

    PR target/59363
    * config/i386/i386.c (emit_memset): Adjust destination address
    after gen_strset.
    (expand_setmem_epilogue): Likewise.

gcc/testsuite/

2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>

    PR target/59363
    * gcc.target/i386/pr59363.c: New file.

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b11363be..d048511 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -22806,6 +22806,8 @@ emit_memset (rtx destmem, rtx destptr, rtx promoted_val,
       if (piece_size <= GET_MODE_SIZE (word_mode))
     {
       emit_insn (gen_strset (destptr, dst, promoted_val));
+      dst = adjust_automodify_address_nv (dst, move_mode, destptr,
+                          piece_size);
       continue;
     }

@@ -22875,14 +22877,18 @@ expand_setmem_epilogue (rtx destmem, rtx
destptr, rtx value, rtx vec_value,
     {
       dest = change_address (destmem, DImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, DImode, destptr, 8);
       emit_insn (gen_strset (destptr, dest, value));
     }
       else
     {
       dest = change_address (destmem, SImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 8);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 12);
       emit_insn (gen_strset (destptr, dest, value));
     }
       emit_label (label);
@@ -22900,6 +22906,7 @@ expand_setmem_epilogue (rtx destmem, rtx
destptr, rtx value, rtx vec_value,
     {
       dest = change_address (destmem, SImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
       emit_insn (gen_strset (destptr, dest, value));
     }
       emit_label (label);
diff --git a/gcc/testsuite/gcc.target/i386/pr59363.c
b/gcc/testsuite/gcc.target/i386/pr59363.c
new file mode 100644
index 0000000..a4e1240
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr59363.c
@@ -0,0 +1,24 @@
+/* PR target/59363 */
+/* { dg-do run } */
+/* { dg-options "-O2 -mtune=amdfam10" } */
+
+typedef struct {
+  int ctxlen;
+  long interhunkctxlen;
+  int flags;
+  long find_func;
+  void *find_func_priv;
+  int hunk_func;
+} xdemitconf_t;
+
+__attribute__((noinline))
+int xdi_diff(xdemitconf_t *xecfg) {
+  if (xecfg->hunk_func == 0)
+    __builtin_abort();
+  return 0;
+}
+int main() {
+  xdemitconf_t xecfg = {0};
+  xecfg.hunk_func = 1;
+  return xdi_diff(&xecfg);
+}