[PATCH] i386: Fix ICE caused by ix86_emit_i387_log1p [PR105214]

[PATCH] i386: Fix ICE caused by ix86_emit_i387_log1p [PR105214]

[Jakub Jelinek]

Hi!

The following testcase ICEs, because ix86_emit_i387_log1p attempts to
emit something like
  if (cond)
    some_code1;
  else
    some_code2;
and emits a conditional jump using emit_jump_insn (standard way in
the file) and an unconditional jump using emit_jump.
The problem with that is that if there is pending stack adjustment,
it isn't emitted before the conditional jump, but is before the
unconditional jump and therefore stack is adjusted only conditionally
(at the end of some_code1 above), which makes dwarf2 pass unhappy about it
but is a serious wrong-code even if it doesn't ICE.

This can be fixed either by emitting pending stack adjust before the
conditional jump as the following patch does, or by not using
  emit_jump (label2);
and instead hand inlining what that function does except for the
pending stack adjustment, like:
  emit_jump_insn (targetm.gen_jump (label2));
  emit_barrier ();
In that case there will be no stack adjustment in the sequence and
it will be done later on somewhere else.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or do you prefer the other version?

2022-04-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/105214
	* config/i386/i386-expand.cc (ix86_emit_i387_log1p): Call
	do_pending_stack_adjust.

	* gcc.dg/asan/pr105214.c: New test.

--- gcc/config/i386/i386-expand.cc.jj	2022-04-03 21:50:36.001635947 +0200
+++ gcc/config/i386/i386-expand.cc	2022-04-11 15:17:43.943430658 +0200
@@ -17291,6 +17291,8 @@ void ix86_emit_i387_log1p (rtx op0, rtx
   rtx cst, cstln2, cst1;
   rtx_insn *insn;
 
+  do_pending_stack_adjust ();
+
   cst = const_double_from_real_value
     (REAL_VALUE_ATOF ("0.29289321881345247561810596348408353", XFmode), XFmode);
   cstln2 = force_reg (XFmode, standard_80387_constant_rtx (4)); /* fldln2 */
--- gcc/testsuite/gcc.dg/asan/pr105214.c.jj	2022-04-11 15:21:05.467608711 +0200
+++ gcc/testsuite/gcc.dg/asan/pr105214.c	2022-04-11 15:22:10.559697224 +0200
@@ -0,0 +1,16 @@
+/* PR target/105214 */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-Ofast -fnon-call-exceptions -fexceptions -fstack-check=generic -fsanitize=address -fno-finite-math-only -fsignaling-nans -fno-associative-math" } */
+
+float f;
+void bar (int *);
+
+void
+foo (void)
+{
+  int a[1600], b[1];
+  f += __builtin_log1pf (f);
+  bar (a);
+  bar (b);
+}

	Jakub

Re: [PATCH] i386: Fix ICE caused by ix86_emit_i387_log1p [PR105214]

[Uros Bizjak]

On Mon, Apr 11, 2022 at 6:50 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The following testcase ICEs, because ix86_emit_i387_log1p attempts to
> emit something like
>   if (cond)
>     some_code1;
>   else
>     some_code2;
> and emits a conditional jump using emit_jump_insn (standard way in
> the file) and an unconditional jump using emit_jump.
> The problem with that is that if there is pending stack adjustment,
> it isn't emitted before the conditional jump, but is before the
> unconditional jump and therefore stack is adjusted only conditionally
> (at the end of some_code1 above), which makes dwarf2 pass unhappy about it
> but is a serious wrong-code even if it doesn't ICE.
>
> This can be fixed either by emitting pending stack adjust before the
> conditional jump as the following patch does, or by not using
>   emit_jump (label2);
> and instead hand inlining what that function does except for the
> pending stack adjustment, like:
>   emit_jump_insn (targetm.gen_jump (label2));
>   emit_barrier ();
> In that case there will be no stack adjustment in the sequence and
> it will be done later on somewhere else.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> Or do you prefer the other version?

No, this looks like the correct approach to me. Perhaps a small
comment should be added, since the reason to call
do_pending_stack_adjust is not that obvious.

> 2022-04-11  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/105214
>         * config/i386/i386-expand.cc (ix86_emit_i387_log1p): Call
>         do_pending_stack_adjust.
>
>         * gcc.dg/asan/pr105214.c: New test.

OK with the comment.

Thanks,
Uros.

>
> --- gcc/config/i386/i386-expand.cc.jj   2022-04-03 21:50:36.001635947 +0200
> +++ gcc/config/i386/i386-expand.cc      2022-04-11 15:17:43.943430658 +0200
> @@ -17291,6 +17291,8 @@ void ix86_emit_i387_log1p (rtx op0, rtx
>    rtx cst, cstln2, cst1;
>    rtx_insn *insn;
>
> +  do_pending_stack_adjust ();
> +
>    cst = const_double_from_real_value
>      (REAL_VALUE_ATOF ("0.29289321881345247561810596348408353", XFmode), XFmode);
>    cstln2 = force_reg (XFmode, standard_80387_constant_rtx (4)); /* fldln2 */
> --- gcc/testsuite/gcc.dg/asan/pr105214.c.jj     2022-04-11 15:21:05.467608711 +0200
> +++ gcc/testsuite/gcc.dg/asan/pr105214.c        2022-04-11 15:22:10.559697224 +0200
> @@ -0,0 +1,16 @@
> +/* PR target/105214 */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
> +/* { dg-options "-Ofast -fnon-call-exceptions -fexceptions -fstack-check=generic -fsanitize=address -fno-finite-math-only -fsignaling-nans -fno-associative-math" } */
> +
> +float f;
> +void bar (int *);
> +
> +void
> +foo (void)
> +{
> +  int a[1600], b[1];
> +  f += __builtin_log1pf (f);
> +  bar (a);
> +  bar (b);
> +}
>
>         Jakub
>