Bug 25140 - aliases, including weakref, break alias analysis
Summary: aliases, including weakref, break alias analysis
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.1.0
: P3 normal
Target Milestone: ---
Assignee: Richard Biener
URL:
Keywords: alias, wrong-code
: 60214 (view as bug list)
Depends on:
Blocks: 24332 61886
  Show dependency treegraph
 
Reported: 2005-11-28 17:03 UTC by Geoff Keating
Modified: 2015-12-16 19:23 UTC (History)
5 users (show)

See Also:
Host: *-*-*
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-05-09 16:49:58


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Geoff Keating 2005-11-28 17:03:24 UTC
The following program:

extern void abort (void);
int x = 0;
extern int y __attribute((weakref("x")));

int main(void)
{ 
  x = 1;
  y = 2;
  if (x != 2)
    abort ();
  return 0;
}

when compiled with -O2 on powerpc-darwin, unconditionally calls abort().  It should not.
Comment 1 Andrew Pinski 2005-11-28 17:06:36 UTC
Confirmed, the problem is that middle-end does not know that x and y are really the same variable
Comment 2 Alexandre Oliva 2005-11-28 18:40:00 UTC
I'd change the testcase s/weakref/alias/, so as to make it clear that the error has nothing to do with the newly-introduced weakref, but rather with regular aliases, that weakref builds upon.
Comment 3 Geoff Keating 2005-11-28 19:22:27 UTC
Subject: Re:  aliases, including weakref, break alias analysis


On 28/11/2005, at 10:40 AM, aoliva at gcc dot gnu dot org wrote:

> ------- Comment #2 from aoliva at gcc dot gnu dot org  2005-11-28  
> 18:40 -------
> I'd change the testcase s/weakref/alias/, so as to make it clear  
> that the error
> has nothing to do with the newly-introduced weakref, but rather  
> with regular
> aliases, that weakref builds upon.

If you do that, it won't work on Darwin, because Darwin doesn't have  
aliases.

Comment 4 Alexandre Oliva 2005-11-29 13:55:16 UTC
Make it a weak alias, then.

Anyhow, the point is that the alias infrastructure in GCC is available for all ports.  If some port limits what kinds of aliases you can use, then, well, too bad.  But the smarts to get alias analysis to follow symbol aliases is missing for all kinds of aliases, not only weakrefs, and the good news is that fixing it for regular and weak aliases will fix it for weakrefs as well, unless you try really hard to keep weakrefs broken just to make a point :-)
Comment 5 Geoff Keating 2005-11-30 01:01:09 UTC
Subject: Re:  aliases, including weakref, break alias analysis


On 29/11/2005, at 5:55 AM, aoliva at gcc dot gnu dot org wrote:

> Make it a weak alias, then.

A weak alias is still an alias and still not supported by Mach-O.
Comment 6 Richard Biener 2009-09-26 11:29:09 UTC
Whoever designed this feature should be beaten with a cluebat for not asking
people on how this will interact with aliasing.

IMHO the testcase should be declared invalid and it should be documented that
an aliased or weakreffed var may be only accessed via its original declaration
(x in this case).

Case closed for me.  Any other way not only breaks points-to analysis but
also trivial alias checks throughout the compiler.
Comment 7 Geoff Keating 2009-09-26 11:51:17 UTC
I looked up 'weakref' in the GCC documentation because I'd forgotten exactly what it was supposed to do, and noticed that it's actually documented as applying only to functions.  So, maybe we could just say that this attribute is only allowed for functions, and produce an error on line 3.

Alexandre's original mail proposing the extension, <http://gcc.gnu.org/ml/gcc/2005-10/msg00235.html>, suggests the attribute for both variables and functions, but the explanation of why you'd want this feature only talks about functions.

The weakref documentation does specifically talk about the case where the function is referenced both via the weakref and directly through the original symbol, and I think the typical use cases actually do this.
Comment 8 Richard Biener 2009-09-26 12:01:21 UTC
Hm, I only can see references to "symbol" not to either function or variable
declaration in the documentation.  Can you cite the part that makes you think
it restricts the use to functions?
Comment 9 Geoff Keating 2009-09-26 12:15:20 UTC
(In reply to comment #8)
> Hm, I only can see references to "symbol" not to either function or variable
> declaration in the documentation.  Can you cite the part that makes you think
> it restricts the use to functions?

It's documented in the section on function attributes, but not listed in the section on variable attributes.  Compare 'deprecated' or 'weak', which are listed in both places.

It does say 'symbol' in referring to the target, but I doubt it's really supposed to mean that you can declare a function as a weakref to a variable (and that couldn't work anyway on most platforms, the variable will be in a non-executable region of memory).
Comment 10 Richard Biener 2010-05-09 16:49:58 UTC
For globals we could track this properly by using the varpool nodes instead of
the DECL_UID to do disambiguation.

Queued.
Comment 11 Peter A. Bigot 2010-08-13 20:11:30 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Hm, I only can see references to "symbol" not to either function or variable
> > declaration in the documentation.  Can you cite the part that makes you think
> > it restricts the use to functions?
> 
> It's documented in the section on function attributes, but not listed in the
> section on variable attributes.  Compare 'deprecated' or 'weak', which are
> listed in both places.

Is there any intention to restrict use of weakref to functions?  It seems to
be exactly what I want to use to allow a translation unit to reference a
memory-mapped register by its vendor-defined name, while not making that
name a global symbol that impacts other translation units, nor providing the
actual register address until the final link phase:

static volatile uint16_t P1IN __attribute((weakref("__P1IN")));
uint16_t c3 () { return P1IN; }

with __P1IN = 0x0020; in a linker script.

Other approaches seem to require that I have a definition for __P1IN
available at the time the object file is generated, which means I'd have a
potential for conflict if a different object file happened to include a
header that gave the register a different address; or that I use:

volatile uint16_t P1IN __attribute((weak));
uint16_t c3 () { return P1IN; }

which clutters the namespace.

Heck, I'll submit a patch to gcc/doc/extend.texi that explicitly allows use
of weakref on variables if that'd help.
Comment 12 Richard Biener 2014-02-17 12:25:52 UTC
*** Bug 60214 has been marked as a duplicate of this bug. ***
Comment 13 Jan Hubicka 2015-12-09 06:28:15 UTC
I am looking into this now as it is necessary to be solved to resolve PR61886. And also the bug is 10 years old now :)
Comment 14 Jan Hubicka 2015-12-09 07:34:48 UTC
Author: hubicka
Date: Wed Dec  9 07:34:16 2015
New Revision: 231442

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

	PR ipa/61886
	PR middle-end/25140
	* ipa-reference.c (is_improper): Break out from ...
	(is_proper_for_analysis): ... here; fix WRT aliases.
	(analyze_function, generate_summary,
	ipa_reference_write_optimization_summary,
	ipa_reference_read_optimization_summary): Use ipa_reference_var_uid.
	* ipa-refrence.h (ipa_reference_var_uid): New inline.
	* tree-ssa-alias.c (ref_maybe_used_by_call_p_1,
	call_may_clobber_ref_p_1): Use ipa_reference_var_uid.

	* gcc.c-torture/execute/alias-3.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/alias-3.c
Modified:
    trunk/gcc/ipa-reference.c
    trunk/gcc/ipa-reference.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c
Comment 15 Jan Hubicka 2015-12-09 19:30:11 UTC
Author: hubicka
Date: Wed Dec  9 19:29:38 2015
New Revision: 231471

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

	PR ipa/61886
	PR middle-end/25140
	* ipa-reference.c (ipa_reference_get_not_read_global,
	ipa_reference_get_not_read_global): Fix WRT aliases.
	(is_improper): Break out from ...
	(is_proper_for_analysis): ... here; fix WRT aliases.
	(analyze_function, generate_summary,
	ipa_reference_write_optimization_summary,
	ipa_reference_read_optimization_summary): Use ipa_reference_var_uid.
	* ipa-refrence.h (ipa_reference_var_uid): New inline.
	* tree-ssa-alias.c: Revert my accidental previous commit.
	(ref_maybe_used_by_call_p_1,
	call_may_clobber_ref_p_1): Use ipa_reference_var_uid.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-reference.c
    trunk/gcc/tree-ssa-alias.c
Comment 16 Jan Hubicka 2015-12-09 23:28:34 UTC
Author: hubicka
Date: Wed Dec  9 23:28:01 2015
New Revision: 231478

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

	PR ipa/61886
	PR middle-end/25140
	* tree-ssa-alias.c (ptr_deref_may_alias_decl_p): Use compare_base_decls
	(nonoverlapping_component_refs_of_decl_p): Update sanity check.
	(decl_refs_may_alias_p): Use compare_base_decls.
	* alias.c: Include cgraph.h
	(rtx_equal_for_memref_p): Use rtx_equal_for_memref_p.
	(compare_base_decls): New function.
	(base_alias_check): Likewise.
	(memrefs_conflict_p): Likewise.
	(nonoverlapping_memrefs_p): Likewise.
	* alias.h (compare_base_decls): Declare.

	* gcc.c-torture/execute/alias-2.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/alias-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/alias.c
    trunk/gcc/alias.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c
Comment 17 Jan Hubicka 2015-12-16 19:23:45 UTC
The issue described in this PR is fixed now.  I made 68832 separate. To fix that one we still need to use assembler name hash to introduce the aliases.  Have path for that but need to do more testing to see if any problems surfaces.

Honza