Bug 90020

Summary: [7/8 regression] -O2 -Os x86-64 wrong code generated for GNU Emacs
Product: gcc Reporter: eggert
Component: tree-optimizationAssignee: Richard Biener <rguenth>
Status: ASSIGNED ---    
Severity: normal CC: dimhen, iains, marxin, rguenth, vhaisman
Priority: P2 Keywords: wrong-code
Version: 8.3.1   
Target Milestone: 7.5   
Host: Target:
Build: Known to work: 9.0
Known to fail: 7.4.0, 8.3.0 Last reconfirmed: 2019-04-09 00:00:00
Attachments: source code illustrating the bug
gcc -O2 -Os compiled output for x86-64
Reduced test-case #0
Reduced test-case #1

Description eggert 2019-04-09 04:32:02 UTC
Created attachment 46106 [details]
source code illustrating the bug

In:

https://lists.gnu.org/r/emacs-devel/2019-04/msg00209.html

Madhu reports several crashes when compiling applications with 'gcc -O2 -Os' on x86-64. Madhu isolated the example to a crash with GNU Emacs, and I narrowed this down to 'gcc -O2 -Os' generating incorrect code for Emacs. I further narrowed it down to the attached file x.i. I used gcc 8.3.1 20190223 (Red Hat 8.3.1-2) on x86-64 to compile it like this:

gcc -O2 -Os -S x.i

and am attaching a copy of the input x.i and the output x.s. GCC miscompiled the subroutine 'select_window'. The source code looks like this:

Lisp_Object
select_window (Lisp_Object window)
{
  CHECK_TYPE (WINDOWP (window) && BUFFERP (XWINDOW (window)->contents),
	      builtin_lisp_symbol (1214), window);
  struct window *w = XWINDOW (window);
  return w->contents;
}

but the assembly language looks like this:

select_window:
	pushq	%rbp
	pushq	%rbx
	movq	%rdi, %rbx
	pushq	%rcx
	call	WINDOWP
	movq	3(%rbx), %rbp
	...

and that last movq is incorrect, because it dereferences %rbx ('window' in the source code) even though it is not known whether WINDOWP returned true. In the source code, if WINDOWP(window) returns false then 'window' is never used except as an undereferenced pointer value, and it is possible for WINDOWP(window) to return false without dereferencing 'window' so it is not safe for the caller to assume that 'window' is dereferenceable. The incorrect code causes GCC to dump core.

This problem does not occur with GCC 7.3.0 (Ubuntu 7.3.0-27ubuntu1~18.04). Madhu observed the problem with GCC 8.2 so I suspect the problem was introduced in GCC 8.

I don't know which GCC component is causing the problem and am guessing tree-optimization.
Comment 1 eggert 2019-04-09 04:33:18 UTC
Created attachment 46107 [details]
gcc -O2 -Os compiled output for x86-64

The attached x.s file shows the incorrect generated code, compiled with gcc -O2 -Os.
Comment 2 Martin Liška 2019-04-09 06:48:16 UTC
Let me take a look.
Comment 3 Martin Liška 2019-04-09 07:57:04 UTC
I can confirm that even though the code is not so nice :)
I have 2 versions of the reduced test-case:

1)
$ gcc emacs0.c -Os -fno-strict-aliasing -g && valgrind ./a.out
==20746== Memcheck, a memory error detector
==20746== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==20746== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==20746== Command: ./a.out
==20746== 
==20746== Invalid read of size 8
==20746==    at 0x4011CF: select_window (emacs0.c:116)
==20746==    by 0x40105A: main (emacs0.c:126)
==20746==  Address 0xa is not stack'd, malloc'd or (recently) free'd

fails for 4.8.0+ except 4.9.x releases.

2)
$ gcc emacs.c -Os -fno-strict-aliasing && ./a.out

started failing with r238242

The problematic transformation:

  <bb 2> [local count: 1073741824]:
  _1 = PSEUDOVECTORP (window_6(D));
  pretmp_9 = MEM[(struct window *)window_6(D)].contents;
  if (_1 != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 4>; [50.00%]

happens in PRE. Richi can you please take a look?
Comment 4 Martin Liška 2019-04-09 07:57:31 UTC
Created attachment 46108 [details]
Reduced test-case #0
Comment 5 Martin Liška 2019-04-09 07:57:51 UTC
Created attachment 46109 [details]
Reduced test-case #1
Comment 6 Martin Liška 2019-04-09 08:18:28 UTC
I bisected GCC 4.9.x branch and it started with r215059, which is a backport of 3 patches. I reverted changes in:
patching file gcc/recog.c
patching file gcc/tree-ssa-loop-niter.c
patching file gcc/tree-vect-slp.c

and so that it points to backport of PR61672.
Comment 7 Richard Biener 2019-04-09 11:14:32 UTC
So looking at one issue I can see is code-hoisting hoisting
MEM[(struct window *)window_6(D) + -5B].contents across a call that might
not return.  This can only happen for calls we can alias-disambiguate
against which means in this case pure calls.  For example for divisions
we guard against this case my checking whether it may trap and there
was an earlier call that might not return.  That is missing for memory
referneces.

  <bb 2> [local count: 1073741824]:
  # VUSE <.MEM_5(D)>
  _1 = WINDOWP (window_6(D));
  if (_1 != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 4>; [50.00%]

  <bb 3> [local count: 536870913]:
  # VUSE <.MEM_5(D)>
  _2 = MEM[(struct window *)window_6(D) + -5B].contents;
  # VUSE <.MEM_5(D)>
  _3 = BUFFERP (_2);
  _8 = (int) _3;

  <bb 4> [local count: 1073741824]:
  # iftmp.1_4 = PHI <_8(3), 0(2)>
  # VUSE <.MEM_5(D)>
  CHECK_TYPE (iftmp.1_4, 4856B, window_6(D));
  # VUSE <.MEM_5(D)>
  _7 = MEM[(struct window *)window_6(D) + -5B].contents;
  # VUSE <.MEM_5(D)>
  return _7;

But fixing that on the GIMPLE level doesn't make the issue go away since
we have similar functionality on RTL which triggers (and is the older
issue since GIMPLE can do hoisting only since GCC 7).
Comment 8 Richard Biener 2019-04-09 11:15:00 UTC
I have a patch for GIMPLE and will produce a nicer testcase for that.  Will also look at the RTL hoisting issue.
Comment 9 Richard Biener 2019-04-09 11:43:17 UTC
/* { dg-do run } */
/* { dg-require-weak "" } */

void __attribute__((noinline,noclone))
check (int i)
{
  if (i == 0)
    __builtin_exit (0);
}

int i;
extern int x __attribute__((weak));

int main(int argc, char **argv)
{
  if (argc)
    {
      check (i);
      return x;
    }
  else
    {
      check (i);
      return x-1;
    }
  return 0;
}


FAILs at -O2 due to GIMPLE PRE and at -Os due to RTL hoist (if GIMPLE PRE
is fixed).
Comment 10 Richard Biener 2019-04-09 11:49:34 UTC
(In reply to Martin Liška from comment #6)
> I bisected GCC 4.9.x branch and it started with r215059, which is a backport
> of 3 patches. I reverted changes in:
> patching file gcc/recog.c
> patching file gcc/tree-ssa-loop-niter.c
> patching file gcc/tree-vect-slp.c
> 
> and so that it points to backport of PR61672.

Note that was a fix for the fallout of r208113 so before that rev. the issue
should "re-appear" in the past.
Comment 11 Richard Biener 2019-04-09 12:13:47 UTC
For the RTL issue there's

compute_hash_table_work (struct gcse_hash_table_d *table)
{
...
      /* First pass over the instructions records information used to
         determine when registers and memory are first and last set.  */
      FOR_BB_INSNS (current_bb, insn)
        {
          if (!NONDEBUG_INSN_P (insn))
            continue;

          if (CALL_P (insn))
            {
              hard_reg_set_iterator hrsi;
              EXECUTE_IF_SET_IN_HARD_REG_SET (regs_invalidated_by_call,
                                              0, regno, hrsi)
                record_last_reg_set_info (insn, regno);

              if (! RTL_CONST_OR_PURE_CALL_P (insn))
                record_last_mem_set_info (insn);

which eventually initializes blocks_with_calls which prunes transp.  But
the calls in question are marked PURE but also RTL_LOOPING_CONST_OR_PURE_CALL_P.
So the obvious thing for the above is to still mark the block for
RTL_LOOPING_CONST_OR_PURE_CALL_P.

Testing overall patch.
Comment 12 Richard Biener 2019-04-11 07:34:52 UTC
Author: rguenth
Date: Thu Apr 11 07:34:20 2019
New Revision: 270275

URL: https://gcc.gnu.org/viewcvs?rev=270275&root=gcc&view=rev
Log:
2019-04-11  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/90020
	* tree-ssa-sccvn.c (vn_reference_may_trap): New function.
	* tree-ssa-sccvn.h (vn_reference_may_trap): Declare.
	* tree-ssa-pre.c (compute_avail): Use it to not put
	possibly trapping references after a call that might not
	return into EXP_GEN.
	* gcse.c (compute_hash_table_work): Do not elide
	marking a block containing a call if the call might not
	return.

	* gcc.dg/torture/pr90020.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr90020.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gcse.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-pre.c
    trunk/gcc/tree-ssa-sccvn.c
    trunk/gcc/tree-ssa-sccvn.h
Comment 13 Richard Biener 2019-04-11 07:35:29 UTC
Fixed on trunk sofar.
Comment 14 Dominique d'Humieres 2019-04-13 11:26:59 UTC
The test gcc.dg/torture/pr90020.c fails on darwin:

Undefined symbols for architecture x86_64:
  "_x", referenced from:
      _main in ccemobuO.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
Comment 15 rguenther@suse.de 2019-04-15 07:31:26 UTC
On Sat, 13 Apr 2019, dominiq at lps dot ens.fr wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90020
> 
> --- Comment #14 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> The test gcc.dg/torture/pr90020.c fails on darwin:
> 
> Undefined symbols for architecture x86_64:
>   "_x", referenced from:
>       _main in ccemobuO.o
> ld: symbol(s) not found for architecture x86_64
> collect2: error: ld returned 1 exit status

/* { dg-require-weak "" } */

was supposed to make it UNSUPPORTED on targets where it doesn't work.
Comment 16 Dominique d'Humieres 2019-04-15 07:42:29 UTC
> /* { dg-require-weak "" } */
>
> was supposed to make it UNSUPPORTED on targets where it doesn't work.

Apparently this not enough. From gcc.dg/attr-weakref-1.c I see

// { dg-additional-options "-Wl,-undefined,dynamic_lookup" { target *-*-darwin* } }
// { dg-additional-options "-Wl,-flat_namespace" { target *-*-darwin[89]* } }

The test links on x86_64-apple-darwin18 if I add -Wl,-undefined,dynamic_lookup.
Comment 17 rguenther@suse.de 2019-04-15 07:44:39 UTC
On Mon, 15 Apr 2019, dominiq at lps dot ens.fr wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90020
> 
> --- Comment #16 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > /* { dg-require-weak "" } */
> >
> > was supposed to make it UNSUPPORTED on targets where it doesn't work.
> 
> Apparently this not enough. From gcc.dg/attr-weakref-1.c I see
> 
> // { dg-additional-options "-Wl,-undefined,dynamic_lookup" { target *-*-darwin*
> } }
> // { dg-additional-options "-Wl,-flat_namespace" { target *-*-darwin[89]* } }
> 
> The test links on x86_64-apple-darwin18 if I add -Wl,-undefined,dynamic_lookup.

Can you commit that change then?  It's pre-approved since it only
affects -darwin and you tested that.  Thx.
Comment 18 dominiq 2019-04-15 07:57:15 UTC
Author: dominiq
Date: Mon Apr 15 07:56:43 2019
New Revision: 270360

URL: https://gcc.gnu.org/viewcvs?rev=270360&root=gcc&view=rev
Log:
2019-04-15  Dominique d'Humieres  <dominiq@gcc.gnu.org>

	PR tree-optimization/90020
	* gcc.dg/torture/pr90020.c: Add linker options for darwin.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/torture/pr90020.c
Comment 19 Dominique d'Humieres 2019-04-15 08:00:13 UTC
> Can you commit that change then?  It's pre-approved since it only
> affects -darwin and you tested that.  Thx.

Done at r270360.

Note that I cannot test the additional option for darwin[89].
Comment 20 Iain Sandoe 2019-05-02 07:51:51 UTC
(In reply to Dominique d'Humieres from comment #19)
> > Can you commit that change then?  It's pre-approved since it only
> > affects -darwin and you tested that.  Thx.
> 
> Done at r270360.
> 
> Note that I cannot test the additional option for darwin[89].

test passes on powerpc-darwin9, see https://gcc.gnu.org/ml/gcc-testresults/2019-05/msg00103.html (it will be some time before I can test darwin8).