Bug 65765 - [5/6 Regression] Compiling Firefox with GCC 5 leads to broken javascript engine on x86-64
Summary: [5/6 Regression] Compiling Firefox with GCC 5 leads to broken javascript engi...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 5.0
: P1 normal
Target Milestone: 5.0
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-14 22:14 UTC by Mike Hommey
Modified: 2015-04-15 12:18 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-04-15 00:00:00


Attachments
reduced.tgz (780 bytes, application/x-compressed-tar)
2015-04-14 22:14 UTC, Mike Hommey
Details
gcc5-pr65765.patch (939 bytes, patch)
2015-04-15 07:22 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Hommey 2015-04-14 22:14:27 UTC
Created attachment 35314 [details]
reduced.tgz

When building Firefox with GCC 5 [1], some parts of the test suite fail because of what might be a compiler bug. It seems to be involving __attribute__((cold)). Dan Gohman got a reduced testcase, attached here.

Unpack and build with g++ -O2 *.cpp, then run a.out.

This doesn't happen at -O1, or with -O2 -Dcold=.

1. GCC 5.1.0-RC-20150412, compiled with "--enable-languages=c,c++  --disable-nls --disable-gnu-unique-object --enable-__cxa_atexit --with-arch-32=pentiumpro".
I've also reproduced with the reduced testcase with a snapshot of 5-20150111 (building Firefox with that snapshot fails for other reasons, unfortunately).
Comment 1 Mike Hommey 2015-04-15 00:52:18 UTC
git bisect identified this commit as the first one where the reduced testcase fails:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=52200d03c231f0bddbd4bbc5cd3608c6a1dd4598

svn+ssh://gcc.gnu.org/svn/gcc/trunk@216305
Comment 2 Markus Trippelsdorf 2015-04-15 04:56:36 UTC
Confirmed. -fno-ipa-icf "fixes" the issue.
Comment 3 Markus Trippelsdorf 2015-04-15 05:41:17 UTC
markus@x4 tc % cat main.ii
int a, b, c, d, e;
unsigned char h[] = { 1, 1 };

__attribute__ ((cold)) int ModRM_Mode () { return a; }

int
ModRM_RM (int p1)
{
  return p1;
}

__attribute__ ((cold)) static bool ModRM_hasSIB (unsigned char p1)
{
  return ModRM_Mode () != 1 && ModRM_RM (p1);
}

__attribute__ ((cold)) static bool ModRM_hasRIP (unsigned char p1)
{
  return ModRM_Mode () && ModRM_RM (p1);
}

unsigned char *
DisassembleHeapAccess (unsigned char *p1)
{
  b = *p1++;
  if (ModRM_hasSIB (b))
    c = *p1++;
  int f = c, g = 0;
  d = ModRM_hasRIP (g);
  e = f == 0;
  if (e)
    p1 += sizeof 0;
  return p1;
}

int
main ()
{
  DisassembleHeapAccess (h) == h + 2 ? static_cast<void> (0)
                                     : __builtin_abort ();
}

markus@x4 tc % g++ -O2 main.ii && ./a.out
[1]    19854 abort      ./a.out
markus@x4 tc % g++ -O2 -fno-ipa-icf main.ii && ./a.out
markus@x4 tc %
Comment 4 Jakub Jelinek 2015-04-15 06:55:42 UTC
Testing the obvious fix.
Comment 5 Mike Hommey 2015-04-15 07:04:26 UTC
I can confirm that building Firefox with -fno-ipa-icf "fixes" the issue as well (that is, that the testcase is correctly related to the Firefox breakage)
Comment 6 Jakub Jelinek 2015-04-15 07:22:06 UTC
Created attachment 35316 [details]
gcc5-pr65765.patch

Untested fix.  The main bug has been a return true; for GIMPLE_NOP/GIMPLE_PREDICT, that means compare_bb ignored anything in the rest of the basic block, which is obviously completely wrong.
While looking at it, I think we should compare for GIMPLE_PREDICT the predictor/outcome, if somebody puts an effort to mark something as hot or cold, unifying it if they differ sounds wrong to me.  And similarly, IMHO for GIMPLE_EH_DISPATCH we should compare the region number.
Comment 7 Richard Biener 2015-04-15 07:52:37 UTC
Looks good to me, though the "obvious" part alone would be fine for 5.1 as well
(even obvious - heh).
Comment 8 Jan Hubicka 2015-04-15 08:33:15 UTC
Seems fine to me, thanks for looking into it! (I jsut arrived to china).
It is safe to ignore GIMPLE_PREDICT at ipa-ICF time. Noone is using it afterwards.  We are stripping it at some point, I am surprised it is not done before ICF's analysis.
Comment 9 Jan Hubicka 2015-04-15 08:34:34 UTC
GIMPLE_PREDICT at this time is already converted to the profile (just to explain).  If we start to do tail merging in early opts, matching gimple predict before profile is built is probably good idea.
Comment 10 Jakub Jelinek 2015-04-15 08:45:01 UTC
I can certainly change that hunk to just do break; for GIMPLE_PREDICT (except for the already bootstrapped and in progress tested patch, which I'd have to redo).
Richard suggested that perhaps for 5.1 it would be safer to just do the break for GIMPLE_PREDICT/GIMPLE_NOP and leave the rest for 6/5.2.
I'm worried about the GIMPLE_EH_DISPATCH missing comparison in that case though.
compare_bb compares the EH regions the stmts are in, but not the EH_DISPATCH argument.
Comment 11 Jakub Jelinek 2015-04-15 11:48:15 UTC
Author: jakub
Date: Wed Apr 15 11:47:44 2015
New Revision: 222123

URL: https://gcc.gnu.org/viewcvs?rev=222123&root=gcc&view=rev
Log:
	PR ipa/65765
	* ipa-icf-gimple.c (func_checker::compare_bb): For GIMPLE_NOP
	and GIMPLE_PREDICT use break instead of return true. For
	GIMPLE_EH_DISPATCH, compare dispatch region.

	* g++.dg/ipa/pr65765.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ipa/pr65765.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-icf-gimple.c
    trunk/gcc/testsuite/ChangeLog
Comment 12 Jakub Jelinek 2015-04-15 12:10:27 UTC
Author: jakub
Date: Wed Apr 15 12:09:56 2015
New Revision: 222124

URL: https://gcc.gnu.org/viewcvs?rev=222124&root=gcc&view=rev
Log:
	PR ipa/65765
	* ipa-icf-gimple.c (func_checker::compare_bb): For GIMPLE_NOP
	and GIMPLE_PREDICT use break instead of return true. For
	GIMPLE_EH_DISPATCH, compare dispatch region.

	* g++.dg/ipa/pr65765.C: New test.

Added:
    branches/gcc-5-branch/gcc/testsuite/g++.dg/ipa/pr65765.C
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/ipa-icf-gimple.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 13 Jakub Jelinek 2015-04-15 12:18:46 UTC
Should be fixed for 5.1+.