Bug 89009 - Miscompilation (missing function call) with -fvisibility=hidden -fpic -O2 -fno-inline
Summary: Miscompilation (missing function call) with -fvisibility=hidden -fpic -O2 -fn...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 8.2.1
: P2 normal
Target Milestone: 7.5
Assignee: Martin Liška
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2019-01-23 10:26 UTC by Stephan Bergmann
Modified: 2019-05-28 11:55 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.9.4, 7.4.0, 8.2.1, 9.0
Known to fail: 5.5.0, 6.4.0
Last reconfirmed: 2019-01-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Bergmann 2019-01-23 10:26:56 UTC
On Linux x86-64 with at least with GCC 8.2.1 (gcc-8.2.1-6.fc29.x86_64) and recent trunk towards GCC 9, compiling

> $ cat test.cc
> void f1();
> struct S1 { static void f2(); };
> struct __attribute__ ((visibility("default"))) S2: S1 { static void f2(); };
> struct S3: S1 { static void f2(); };
> struct S4: S3 { static void f2(); };
> void S2::f2() { S1::f2(); }
> void S3::f2() { S1::f2(); }
> void S4::f2() {
>     f1();
>     S3::f2(); // MISSING
> }
> 
> $ g++ -fvisibility=hidden -fpic -O2 -fno-inline -S test.cc

causes the call to S3::f2 to be missing from the code generated for S4::f2:

>         .globl  _ZN2S42f2Ev
>         .hidden _ZN2S42f2Ev
>         .type   _ZN2S42f2Ev, @function
> _ZN2S42f2Ev:
> .LFB2:
>         .cfi_startproc
>         jmp     _Z2f1v@PLT
>         .cfi_endproc
> .LFE2:
>         .size   _ZN2S42f2Ev, .-_ZN2S42f2Ev
Comment 1 Richard Biener 2019-01-23 12:34:09 UTC
Oddly with just -O2 -fno-inline we call S2::f2 () (it got ICFed with S3::f2
it seems).

Disabling ICF also fixes the omission of the call for -fpic -fvisibility=hidden.
Comment 2 Martin Liška 2019-01-23 13:07:47 UTC
Let me take a look..
Comment 3 Martin Liška 2019-01-24 15:51:04 UTC
I slightly renamed the functions to have it more readable:

$ cat pr89009.c
void foo();
struct S1 { static void bar(); };
struct __attribute__ ((visibility("default"))) S2: S1 { static void bar(); };
struct S3: S1 { static void bar(); };
struct S4: S3 { static void bar(); };
void S2::bar() { S1::bar(); }
void S3::bar() { S1::bar(); }
void S4::bar() {
    foo();
    S3::bar(); // MISSING
}

Now ICF does following folding:
Equals called for: static void S3::bar()/1:static void S2::bar()/0 with result: true

and new localalias is added:

_ZN2S23barEv.localalias/5 (static void S2::_ZN2S23barEv.localalias()) @0x7ffff6b559d8
  Type: function definition analyzed alias
  Visibility: visibility_specified
  References: _ZN2S23barEv/0 (alias)
  Referring: 
  Availability: available
  Function flags: icf_merged
  Called by: static void S3::bar()/1 (1073741824 (estimated locally),1.00 per call) (can throw external) 
  Calls: 

and I see the call in S4::f2 is removed here:
pr89009.c.103t.dce2:

;; Function S4::bar (_ZN2S43barEv, funcdef_no=2, decl_uid=2311, cgraph_uid=3, symbol_order=2)

Marking useful stmt: foo ();

Marking useful stmt: return;


Processing worklist:
processing: return;

processing: foo ();


Eliminating unnecessary statements:
Deleting : S3::bar ();

Removed 1 of 3 statements (33%)
Removed 0 of 0 PHI nodes (0%)
S4::bar ()
{
  <bb 2> [local count: 1073741824]:
  foo ();
  return;

}

Richi do you have any idea what?
Comment 4 Martin Liška 2019-01-24 16:32:18 UTC
> Richi do you have any idea what?

So it's caused by fact that IPA pure-const promotes S3::bar to be const:

  Visiting static void S3::bar()/1 state:const looping 0
    Call to static void S2::_ZN2S23barEv.localalias()/5 state:const looping:0
Result const looping 0
Function found not to call free: static void S3::bar()
Function found to be const: static void S3::bar()
Declaration updated to be const: static void S3::bar()

Then DCE is happy to remote the function as it's supposed to not have a side effect.
Comment 5 Martin Liška 2019-01-24 16:52:31 UTC
Well, I believe the test-case is invalid as one can't have hidden visibility and then defined S1::f2 in a different translation unit. I would like to see an example which would be possible to link. This works for me:

$ cat pr89009.h
void foo();
struct S1 { static void bar(); };
struct __attribute__ ((visibility("default"))) S2: S1 { static void bar(); };
struct S3: S1 { static void bar(); };
struct S4: S3 { static void bar(); };

$ cat pr89009.c
#include "pr89009.h"

void S2::bar() { S1::bar(); }
void S3::bar() { S1::bar(); }
void S4::bar() {
    foo();
    S3::bar(); // MISSING
}

int main()
{
  S4::bar ();
}

$ cat pr89009-2.c
#include "pr89009.h"

void foo() { __builtin_printf ("foo called\n"); }
void S1::bar() { __builtin_printf ("S1::bar\n"); }
Comment 6 Stephan Bergmann 2019-01-24 17:07:07 UTC
(In reply to Martin Liška from comment #5)
> Well, I believe the test-case is invalid as one can't have hidden visibility
> and then defined S1::f2 in a different translation unit.

Why?  Isn't hidden visibility just about symbol visibility at the executable/dynamic library level?
Comment 7 Martin Liška 2019-01-24 17:14:11 UTC
(In reply to Stephan Bergmann from comment #6)
> (In reply to Martin Liška from comment #5)
> > Well, I believe the test-case is invalid as one can't have hidden visibility
> > and then defined S1::f2 in a different translation unit.
> 
> Why?  Isn't hidden visibility just about symbol visibility at the
> executable/dynamic library level?

I might be wrong. But I would like to see a linkable test-case.
Comment 8 Stephan Bergmann 2019-01-24 17:16:27 UTC
...and adding to the test.cc from comment 0 an additional

> $ cat main.cc
> void f1();
> struct S1 { static void f2(); };
> struct __attribute__ ((visibility("default"))) S2: S1 { static void f2(); };
> struct S3: S1 { static void f2(); };
> struct S4: S3 { static void f2(); };
> void f1() { __builtin_printf("f1\n"); }
> void S1::f2() { __builtin_printf("S1::f2\n"); }
> int main() { S4::f2(); }
> 
> $ g++ -fvisibility=hidden -fpic -O2 -fno-inline test.cc main.cc
> $ ./a.out
> f1

to form a complete program still fails for me (GCC 8.2.1).
Comment 9 Martin Liška 2019-01-24 17:22:42 UTC
Thanks for it, confirmed! Started with r244273, but the revision probably only exposed a latent issue.
Comment 10 Martin Liška 2019-01-24 17:54:52 UTC
Ok, so it's as old as ICF. There's a minimal test-case:

$ cat test.cc
#pragma GCC visibility push(default)
void foo1() { __builtin_printf ("foo\n"); }
#pragma GCC visibility pop
void foo2() { __builtin_printf ("foo\n"); }

int main() { foo2(); }

(for older revisions than r244273 one needs:

$ cat test.cc
void foo2() { __builtin_printf ("foo\n"); }

#pragma GCC visibility push(default)
void foo1() { __builtin_printf ("foo\n"); }
#pragma GCC visibility pop
int main() { foo2(); }
Comment 11 Jakub Jelinek 2019-02-01 11:55:46 UTC
So, shall we punt to ICF functions/methods with different visibility, or do we need to treat it specially during later IPA optimizations?
Comment 12 Martin Liška 2019-02-04 08:23:30 UTC
(In reply to Jakub Jelinek from comment #11)
> So, shall we punt to ICF functions/methods with different visibility, or do
> we need to treat it specially during later IPA optimizations?

I'll discuss that with Honza this week and propose a patch for it.
Comment 13 Martin Liška 2019-02-08 12:13:58 UTC
I've got a patch candidate for it, will send it on Monday.
Comment 14 Martin Liška 2019-02-11 08:13:34 UTC
Author: marxin
Date: Mon Feb 11 08:13:03 2019
New Revision: 268762

URL: https://gcc.gnu.org/viewcvs?rev=268762&root=gcc&view=rev
Log:
Construct ipa_reduced_postorder always for overwritable (PR ipa/89009).

2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* ipa-cp.c (build_toporder_info): Remove usage of a param.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (propagate_pure_const): Likewise.
	(propagate_nothrow): Likewise.
	* ipa-reference.c (propagate): Likewise.
	* ipa-utils.c (struct searchc_env): Remove unused field.
	(searchc): Always search across AVAIL_INTERPOSABLE.
	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
	the only called IPA pure const can properly not propagate
	across interposable boundary.
	* ipa-utils.h (ipa_reduced_postorder): Remove param.
2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* g++.dg/ipa/pr89009.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ipa/pr89009.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-cp.c
    trunk/gcc/ipa-inline.c
    trunk/gcc/ipa-pure-const.c
    trunk/gcc/ipa-reference.c
    trunk/gcc/ipa-utils.c
    trunk/gcc/ipa-utils.h
    trunk/gcc/testsuite/ChangeLog
Comment 15 Martin Liška 2019-02-11 08:14:54 UTC
Fixed on trunk so far.
Comment 16 Martin Liška 2019-02-14 11:25:30 UTC
Author: marxin
Date: Thu Feb 14 11:24:45 2019
New Revision: 268871

URL: https://gcc.gnu.org/viewcvs?rev=268871&root=gcc&view=rev
Log:
Backport r268762

2019-02-14  Martin Liska  <mliska@suse.cz>

	Backport from mainline
	2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* ipa-cp.c (build_toporder_info): Remove usage of a param.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (propagate_pure_const): Likewise.
	(propagate_nothrow): Likewise.
	* ipa-reference.c (propagate): Likewise.
	* ipa-utils.c (struct searchc_env): Remove unused field.
	(searchc): Always search across AVAIL_INTERPOSABLE.
	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
	the only called IPA pure const can properly not propagate
	across interposable boundary.
	* ipa-utils.h (ipa_reduced_postorder): Remove param.
2019-02-14  Martin Liska  <mliska@suse.cz>

	Backport from mainline
	2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* g++.dg/ipa/pr89009.C: New test.

Added:
    branches/gcc-8-branch/gcc/testsuite/g++.dg/ipa/pr89009.C
Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/ipa-cp.c
    branches/gcc-8-branch/gcc/ipa-inline.c
    branches/gcc-8-branch/gcc/ipa-pure-const.c
    branches/gcc-8-branch/gcc/ipa-reference.c
    branches/gcc-8-branch/gcc/ipa-utils.c
    branches/gcc-8-branch/gcc/ipa-utils.h
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
Comment 17 Martin Liška 2019-02-15 10:59:20 UTC
Author: marxin
Date: Fri Feb 15 10:58:49 2019
New Revision: 268937

URL: https://gcc.gnu.org/viewcvs?rev=268937&root=gcc&view=rev
Log:
Backport r268762

2019-02-15  Martin Liska  <mliska@suse.cz>

	Backport from mainline
	2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* ipa-cp.c (build_toporder_info): Remove usage of a param.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (propagate_pure_const): Likewise.
	(propagate_nothrow): Likewise.
	* ipa-reference.c (propagate): Likewise.
	* ipa-utils.c (struct searchc_env): Remove unused field.
	(searchc): Always search across AVAIL_INTERPOSABLE.
	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
	the only called IPA pure const can properly not propagate
	across interposable boundary.
	* ipa-utils.h (ipa_reduced_postorder): Remove param.
2019-02-15  Martin Liska  <mliska@suse.cz>

	Backport from mainline
	2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* g++.dg/ipa/pr89009.C: New test.

Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/ipa-cp.c
    branches/gcc-7-branch/gcc/ipa-inline.c
    branches/gcc-7-branch/gcc/ipa-pure-const.c
    branches/gcc-7-branch/gcc/ipa-reference.c
    branches/gcc-7-branch/gcc/ipa-utils.c
    branches/gcc-7-branch/gcc/ipa-utils.h
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 18 Martin Liška 2019-02-15 11:00:23 UTC
Fixed on all active branches.
Comment 19 Martin Liška 2019-02-18 09:16:54 UTC
Fixed.
Comment 20 Martin Liška 2019-05-28 11:55:05 UTC
Author: marxin
Date: Tue May 28 11:54:33 2019
New Revision: 271697

URL: https://gcc.gnu.org/viewcvs?rev=271697&root=gcc&view=rev
Log:
Backport r268762 (test-suite)

2019-05-28  Martin Liska  <mliska@suse.cz>

	Backport from mainline
	2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* g++.dg/ipa/pr89009.C: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/g++.dg/ipa/pr89009.C
Modified:
    branches/gcc-7-branch/gcc/testsuite/ChangeLog