[Bug middle-end/50199] New: [4.7 Regression] wrong code with -flto -fno-merge-constants

[Bug middle-end/50199] New: [4.7 Regression] wrong code with -flto -fno-merge-constants

[zsojka at seznam dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

             Bug #: 50199
           Summary: [4.7 Regression] wrong code with -flto
                    -fno-merge-constants
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: zsojka@seznam.cz
              Host: x86_64-pc-linux-gnu
            Target: x86_64-pc-linux-gnu


Created attachment 25111
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25111
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

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-08-27
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jsm28 at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
   Target Milestone|---                         |4.7.0
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 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?

Re: [Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[Joseph S. Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #2 from joseph at codesourcery dot com <joseph at codesourcery dot 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.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 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).

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 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.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-12-08 15:02:34 UTC ---
Created attachment 26022
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26022
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.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[hubicka at gcc dot gnu.org]

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.

Honza

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #7 from rguenther at suse dot de <rguenther at suse dot 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.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[hubicka at gcc dot gnu.org]

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.

Honza

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot 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.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #10 from Jan Hubicka <hubicka at ucw dot cz> 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

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2
   Target Milestone|4.7.0                       |4.8.0

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-09 14:27:02 UTC ---
Pushing this to 4.8, it requires too invasive surgery.

[Bug middle-end/50199] [4.7 Regression] wrong code with -flto -fno-merge-constants

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #12 from Jan Hubicka <hubicka at ucw dot cz> 2012-01-09 15:19:30 UTC ---
-flto-partition=none is a workaround for 4.7 compiler.

[Bug middle-end/50199] [4.7/4.8 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

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.

[Bug middle-end/50199] [4.7/4.8 Regression] wrong code with -flto -fno-merge-constants

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #14 from rguenther at suse dot de <rguenther at suse dot 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).

[Bug middle-end/50199] [4.7/4.8 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 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.

[Bug middle-end/50199] [4.7/4.8/4.9 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.0                       |4.8.1

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-03-22 14:42:18 UTC ---
GCC 4.8.0 is being released, adjusting target milestone.

[Bug middle-end/50199] [4.7/4.8/4.9 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.1                       |4.8.2

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.1 has been released.

[Bug middle-end/50199] [4.7/4.8/4.9 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.2                       |4.8.3

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.2 has been released.

[Bug middle-end/50199] [4.7/4.8/4.9/4.10 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.3                       |4.8.4

--- Comment #19 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 4.8.3 is being released, adjusting target milestone.

[Bug middle-end/50199] [4.8/4.9/5 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.4                       |4.8.5

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.4 has been released.

[Bug middle-end/50199] [4.8/4.9/5/6 Regression] wrong code with -flto -fno-merge-constants

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.5                       |4.9.3

--- Comment #21 from Richard Biener <rguenth at gcc dot gnu.org> ---
The gcc-4_8-branch is being closed, re-targeting regressions to 4.9.3.

[Bug middle-end/50199] [4.9/5/6 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.9.3 has been released.

[Bug middle-end/50199] [4.9/5/6 Regression] wrong code with -flto -fno-merge-constants

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50199

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.3                       |4.9.4