Bug 65765

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

Confirmed. -fno-ipa-icf "fixes" the issue.

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 %

Testing the obvious fix.

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)

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.

Looks good to me, though the "obvious" part alone would be fine for 5.1 as well
(even obvious - heh).

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.

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.

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.

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

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

Should be fixed for 5.1+.