User account creation filtered due to spam.

Bug 50199 - [5 Regression] wrong code with -flto -fno-merge-constants
Summary: [5 Regression] wrong code with -flto -fno-merge-constants
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.7.0
: P2 normal
Target Milestone: 5.5
Assignee: Jakub Jelinek
URL:
Keywords: lto, wrong-code
Depends on:
Blocks:
 
Reported: 2011-08-26 19:36 UTC by Zdenek Sojka
Modified: 2017-08-09 12:14 UTC (History)
6 users (show)

See Also:
Host: x86_64-pc-linux-gnu
Target: x86_64-pc-linux-gnu
Build:
Known to work: 4.6.1
Known to fail: 4.7.0
Last reconfirmed: 2011-08-27 00:00:00


Attachments
reduced testcase (128 bytes, text/plain)
2011-08-26 19:36 UTC, Zdenek Sojka
Details
gimple predicate patches (631 bytes, patch)
2011-12-08 15:02 UTC, Richard Biener
Details | Diff
gcc7-pr50199.patch (690 bytes, patch)
2017-01-10 13:48 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zdenek Sojka 2011-08-26 19:36:17 UTC
Created attachment 25111 [details]
reduced testcase

Output:
$ gcc -O2 -flto -fno-merge-constants --param lto-min-partition=1 testcase.c
$ ./a.out 
Aborted

Tested revisions:
r178096 - fail
r178005 - fail
r177982 - OK
Comment 1 Richard Biener 2011-08-27 08:20:47 UTC
Hmmm.  Partitioning unshares string constants and I suppose ipa-cp propagates
one of it to the callee.

Not sure if this is a well-defined testcase though.  If it is we'd probably
need to add CONST_DECLs for address-taken constants and register them with
the varpool (and disallow &"..." in gimple).

It should be possible to make a non-LTO testcase that fails similarly
by instructing the assembler/linker not to merge strings.

Joseph, does C require that a string literal "..." has a single underlying
object or would the testcase be undefined?
Comment 2 joseph@codesourcery.com 2011-08-27 09:46:39 UTC
On Sat, 27 Aug 2011, rguenth at gcc dot gnu.org wrote:

> Hmmm.  Partitioning unshares string constants and I suppose ipa-cp propagates
> one of it to the callee.
> 
> Not sure if this is a well-defined testcase though.  If it is we'd probably
> need to add CONST_DECLs for address-taken constants and register them with
> the varpool (and disallow &"..." in gimple).
> 
> It should be possible to make a non-LTO testcase that fails similarly
> by instructing the assembler/linker not to merge strings.
> 
> Joseph, does C require that a string literal "..." has a single underlying
> object or would the testcase be undefined?

It's completely clear that once a pointer to a string literal has been 
assigned to a variable, that variable has a well-defined value pointing to 
that particular copy of the string literal, and so compares equal to 
itself.  So the testcase ought to pass.

I believe each instance of a string literal in the source code after 
preprocessing corresponds to just one object of static storage duration 
(so the values from different calls to the same function containing a 
string literal must also compare equal, for example) though the objects 
from separate string literals in the source code may or may not overlap or 
coincide.  (There is no requirement for, or restriction against, 
unification between copies of the same inline function included in 
different translation units.)  So the value of "foo" == "foo" is 
unspecified, but given

const char *return_foo (void) { return "foo"; }

the value of return_foo () == return_foo () must be 1.
Comment 3 Richard Biener 2011-10-27 10:12:59 UTC
So it means that for constants we cannot really propagate address-taken
string-csts (or any other csts) but we have to use CONST_DECLs, give them
global scope and eventually register them with the varpool.  Even without
LTO and for

const char *return_foo (void) { return "foo"; }

int main()
{
  if (return_foo () != return_foo ())
    abort ();
}

if we inline return_foo into main twice and fail to constant-propagate
and thus fold the equality on the tree level (we can do that, C does
say whether "foo" != "foo" should be true or false), with -fno-merge-constants
we would fail the testcase if we did not (by luck, I guess) share
the STRING_CST.

For LTO we cannot rely on that "luck".

Honza, do we register CONST_DECLs with the varpool at all?  Or how would
we be able to guarantee that we emit them only once?

Probably not really P1, as a non-stage1 fix is likely a bit invasive
(not sure if surgery at lto streaming time is possible).
Comment 4 Richard Biener 2011-12-08 14:37:14 UTC
Which means - taking addresses of literals should be disallowed (that
includes the prominent example of STRING_CSTs, but possibly that is not
the only existing case?).  Instead when a literal needs its address taken
it needs to be put into a CONST_DECL of which we can take the address
(thus, entered into the "constant pool").  Those CONST_DECLs should be
entered into the varpool so that we properly partition them with LTO.

The C frontend is probably the oldest and most prominent offender
of creating ADDR_EXPR <STRING_CST>.

GIMPLE-side "fix":

Index: gimple.c
===================================================================
--- gimple.c    (revision 182107)
+++ gimple.c    (working copy)
@@ -2903,9 +2903,7 @@ is_gimple_id (tree t)
   return (is_gimple_variable (t)
          || TREE_CODE (t) == FUNCTION_DECL
          || TREE_CODE (t) == LABEL_DECL
-         || TREE_CODE (t) == CONST_DECL
-         /* Allow string constants, since they are addressable.  */
-         || TREE_CODE (t) == STRING_CST);
+         || TREE_CODE (t) == CONST_DECL);
 }
 

(and watch it explode, obviously)

Probably not 4.7 material to change though.
Comment 5 Richard Biener 2011-12-08 15:02:34 UTC
Created attachment 26022 [details]
gimple predicate patches

GIMPLE predicate patches - that will now commit STRING_CSTs to a local
temporary via the

static enum gimplify_status
gimplify_addr_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
{
...
    default:
      /* We use fb_either here because the C frontend sometimes takes
         the address of a call that returns a struct; see
         gcc.dg/c99-array-lval-1.c.  The gimplifier will correctly make
         the implied temporary explicit.  */

      /* Make the operand addressable.  */
      ret = gimplify_expr (&TREE_OPERAND (expr, 0), pre_p, post_p,
                           is_gimple_addressable, fb_either);

which is of course bogus.
Comment 6 Jan Hubicka 2012-01-03 18:06:28 UTC
Hmm, adding CONST_DECLs into varpool would be fun: we would have to ensure that most of target machinery is ready to output CONST_DECLs promoted to hidden vars by ltrans partitioning + there will be some additional surprises for sure (i.e. CONST_DECLs being constructed very late in optimization).

The problem is not specific to ipa-cp, ale ipa-split and inlining can migrate same CONST_DECL across function bodies that can end up in different ltrans partitions.

I wonder if we should not simply promote CONST_DECLs into initialized const vars for this purpose by local tree pass running just before early inliner?  I don't see much of downsides of this transform at them moment, except for lack of sharing of const pool in between early and late const decls.

Honza
Comment 7 rguenther@suse.de 2012-01-04 09:32:28 UTC
On Tue, 3 Jan 2012, hubicka at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199
> 
> --- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> 2012-01-03 18:06:28 UTC ---
> Hmm, adding CONST_DECLs into varpool would be fun: we would have to ensure that
> most of target machinery is ready to output CONST_DECLs promoted to hidden vars
> by ltrans partitioning + there will be some additional surprises for sure (i.e.
> CONST_DECLs being constructed very late in optimization).
> 
> The problem is not specific to ipa-cp, ale ipa-split and inlining can migrate
> same CONST_DECL across function bodies that can end up in different ltrans
> partitions.
> 
> I wonder if we should not simply promote CONST_DECLs into initialized const
> vars for this purpose by local tree pass running just before early inliner?  I
> don't see much of downsides of this transform at them moment, except for lack
> of sharing of const pool in between early and late const decls.

The problem is we do not have CONST_DECLs at all at the moment but
we have ADDR_EXPRs of STRING_CSTs.  I think we should simply ignore this
PR for 4.7 and work on it in the 4.8 timeframe (making the FEs to emit
&CONST_DECL instead of &STRING_CST, how we deal with them wrt the
varpool is a completely different issue).

Richard.
Comment 8 Jan Hubicka 2012-01-04 12:57:28 UTC
I agree that ignoring the bug or adding sorry refusing -fno-merge-constants -flto is probably only way to deal with it in 4.7.

If we go the way translating into initialized vars, we probably could translate string constants as easilly as const_decls, so perhaps there is no frontend change needed at all.

In general it would be very useful to have pass tagging ADDR_EXPRs on whether the address of object taken needs to be unique or can change freely. This would be useful at several places in IPA and also we could then only promote some of the const decls.

Honza
Comment 9 rguenther@suse.de 2012-01-04 13:02:26 UTC
On Wed, 4 Jan 2012, hubicka at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199
> 
> --- Comment #8 from Jan Hubicka <hubicka at gcc dot gnu.org> 2012-01-04 12:57:28 UTC ---
> I agree that ignoring the bug or adding sorry refusing -fno-merge-constants
> -flto is probably only way to deal with it in 4.7.
> 
> If we go the way translating into initialized vars, we probably could translate
> string constants as easilly as const_decls, so perhaps there is no frontend
> change needed at all.
> 
> In general it would be very useful to have pass tagging ADDR_EXPRs on whether
> the address of object taken needs to be unique or can change freely. This would
> be useful at several places in IPA and also we could then only promote some of
> the const decls.

Well, whether or not is the Frontends burden to decide.  And if not,
it has to use an ADDR_EXPR of a CONST_DECL.

Richard.
Comment 10 Jan Hubicka 2012-01-04 13:14:17 UTC
> > In general it would be very useful to have pass tagging ADDR_EXPRs on whether
> > the address of object taken needs to be unique or can change freely. This would
> > be useful at several places in IPA and also we could then only promote some of
> > the const decls.
> 
> Well, whether or not is the Frontends burden to decide.  And if not,
> it has to use an ADDR_EXPR of a CONST_DECL.

What I meant here is pass that tracks uses of ADDR_EXPR to see if it possibly
can be compared for equality that would break when the object gets duplicated
for some reason.  This would strenghten some stuff, like hidding COMDAT
functions (where we special case already virtual functions, but we could do
more)

Honza
Comment 11 Richard Biener 2012-01-09 14:27:02 UTC
Pushing this to 4.8, it requires too invasive surgery.
Comment 12 Jan Hubicka 2012-01-09 15:19:30 UTC
-flto-partition=none is a workaround for 4.7 compiler.
Comment 13 Jakub Jelinek 2013-01-11 16:02:24 UTC
I guess it is again too late to really fix this for 4.8, can we at least error out on -fno-merge-constants used together with -flto ?  We'll still fail for targets that don't support mergeable string sections, but generally, it makes no sense to use -fno-merge-constants together with -flto.
Comment 14 rguenther@suse.de 2013-01-14 14:45:48 UTC
On 1/11/13 5:02 PM, jakub at gcc dot gnu.org wrote:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-11 16:02:24 UTC ---
> I guess it is again too late to really fix this for 4.8, can we at least error
> out on -fno-merge-constants used together with -flto ?  We'll still fail for
> targets that don't support mergeable string sections, but generally, it makes
> no sense to use -fno-merge-constants together with -flto.

We could, I suppose.  I'm not sure this isn't an issue without LTO
though (possibly way harder to trigger though).
Comment 15 Jakub Jelinek 2013-01-14 14:55:45 UTC
(In reply to comment #14)
> I'm not sure this isn't an issue without LTO
> though (possibly way harder to trigger though).

I don't see how.  -fno-merge-constants doesn't say that constants aren't merged within the same TU, they are merged always, -fno-merge-constants is about not allowing constants to be merged between different object files.  As without LTO we operate at the level of individual TUs, we don't have issues with that.
-fno-merge-constants is the only possible thing on targets that don't have needed support on the assembler/linker side though.
Comment 16 Jakub Jelinek 2013-03-22 14:42:18 UTC
GCC 4.8.0 is being released, adjusting target milestone.
Comment 17 Jakub Jelinek 2013-05-31 10:57:44 UTC
GCC 4.8.1 has been released.
Comment 18 Jakub Jelinek 2013-10-16 09:49:56 UTC
GCC 4.8.2 has been released.
Comment 19 Richard Biener 2014-05-22 09:00:58 UTC
GCC 4.8.3 is being released, adjusting target milestone.
Comment 20 Jakub Jelinek 2014-12-19 13:34:33 UTC
GCC 4.8.4 has been released.
Comment 21 Richard Biener 2015-06-23 08:17:24 UTC
The gcc-4_8-branch is being closed, re-targeting regressions to 4.9.3.
Comment 22 Jakub Jelinek 2015-06-26 19:58:32 UTC
GCC 4.9.3 has been released.
Comment 23 Richard Biener 2016-08-03 10:53:38 UTC
GCC 4.9 branch is being closed
Comment 24 Jakub Jelinek 2017-01-10 12:41:25 UTC
Another option is to silently force -flto-partition=none (or would -flto-partition=1to1 work too?) if !flag_merge_constants or HAVE_GAS_SHF_MERGE is not defined.  Or silently turn on flag_merge_constants (set to 1 if 0) in lto compilation, and/or force -flto-partition=none (or 1to1) if HAVE_GAS_SHF_MERGE is not defined.
Honza/Richard, what do you prefer here?
Comment 25 rguenther@suse.de 2017-01-10 13:03:37 UTC
On Tue, 10 Jan 2017, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199
> 
> --- Comment #24 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> Another option is to silently force -flto-partition=none (or would
> -flto-partition=1to1 work too?) if !flag_merge_constants or HAVE_GAS_SHF_MERGE
> is not defined.  Or silently turn on flag_merge_constants (set to 1 if 0) in
> lto compilation, and/or force -flto-partition=none (or 1to1) if
> HAVE_GAS_SHF_MERGE is not defined.
> Honza/Richard, what do you prefer here?

I prefer handling constants correctly, forcing STRING_CSTs to only appear
via CONST_DECL DECL_INITIAL and putting CONST_DECLs into a symbol-table
like constant pool on the cgraph level.

That's quite some work and the question is whether this bug is important
enough to warrant a fix like you propose.  Of your proposals I prefer
forcing -fmerge-constants (and/or simply documenting the restriction
of LTO when working w/o constant merging).

At some point I started the &STRING_CST -> &CONST_DECL thing (I have
partial patches somewhere) but there was some fallout I didn't fully
address.

The issue with LTO is that we fail to merge in-TU constants because
we have those &STRING_CSTs and tear apart the TU.

So - force flag_merge_constants to 1 if in_lto_p?  Or do that from
lto-wrapper, diagnosing that we do that?
Comment 26 Jakub Jelinek 2017-01-10 13:48:16 UTC
Created attachment 40491 [details]
gcc7-pr50199.patch

Untested workaround.
Comment 27 Richard Biener 2017-01-11 08:13:48 UTC
works for me
Comment 28 Jakub Jelinek 2017-01-11 08:41:33 UTC
Author: jakub
Date: Wed Jan 11 08:40:59 2017
New Revision: 244304

URL: https://gcc.gnu.org/viewcvs?rev=244304&root=gcc&view=rev
Log:
	PR middle-end/50199
	* lto-lang.c (lto_post_options): Force flag_merge_constants = 1
	if it was 0.

	* gcc.dg/lto/pr50199_0.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/lto/pr50199_0.c
Modified:
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lto-lang.c
    trunk/gcc/testsuite/ChangeLog
Comment 29 Jakub Jelinek 2017-01-13 22:58:39 UTC
Fixed on the trunk.
Comment 30 Jakub Jelinek 2017-01-17 20:33:48 UTC
Author: jakub
Date: Tue Jan 17 20:33:16 2017
New Revision: 244553

URL: https://gcc.gnu.org/viewcvs?rev=244553&root=gcc&view=rev
Log:
	Backported from mainline
	2017-01-11  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/50199
	* lto-lang.c (lto_post_options): Force flag_merge_constants = 1
	if it was 0.

	* gcc.dg/lto/pr50199_0.c: New test.

Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/lto/pr50199_0.c
Modified:
    branches/gcc-6-branch/gcc/lto/ChangeLog
    branches/gcc-6-branch/gcc/lto/lto-lang.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 31 Jakub Jelinek 2017-01-18 16:26:04 UTC
Fixed for 6.4+ too.
Comment 32 Jakub Jelinek 2017-05-30 07:54:53 UTC
Author: jakub
Date: Tue May 30 07:54:21 2017
New Revision: 248638

URL: https://gcc.gnu.org/viewcvs?rev=248638&root=gcc&view=rev
Log:
	Backported from mainline
	2017-01-11  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/50199
	* lto-lang.c (lto_post_options): Force flag_merge_constants = 1
	if it was 0.

	* gcc.dg/lto/pr50199_0.c: New test.

Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/lto/pr50199_0.c
Modified:
    branches/gcc-5-branch/gcc/lto/ChangeLog
    branches/gcc-5-branch/gcc/lto/lto-lang.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 33 Jakub Jelinek 2017-05-30 09:10:05 UTC
Fixed.