Bug 50199

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?

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.

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).

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.

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.

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

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.

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

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.

> > 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

Pushing this to 4.8, it requires too invasive surgery.

-flto-partition=none is a workaround for 4.7 compiler.

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.

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).

(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.

GCC 4.8.0 is being released, adjusting target milestone.

GCC 4.8.1 has been released.

GCC 4.8.2 has been released.

GCC 4.8.3 is being released, adjusting target milestone.

GCC 4.8.4 has been released.

The gcc-4_8-branch is being closed, re-targeting regressions to 4.9.3.

GCC 4.9.3 has been released.

GCC 4.9 branch is being closed

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?

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?

Created attachment 40491 [details]
gcc7-pr50199.patch

Untested workaround.

works for me

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

Fixed on the trunk.

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

Fixed for 6.4+ too.

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

Fixed.