Patchwork PR tree-optimization/47190: ICE on weakref missing alias info

login
register
mail settings
Submitter Jan Hubicka
Date Jan. 22, 2011, 5:48 p.m.
Message ID <20110122174850.GB31129@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/80010/
State New
Headers show

Comments

Jan Hubicka - Jan. 22, 2011, 5:48 p.m.
Hi,
this PR shows ICE on invalid
static int i __attribute__ ((weakref));

according to docs, weakref attribute should be accompanied with an alias.
This patch adds checking into cgraphunit.c.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	PR tree-optimization/47190
	* cgraphunit.c (process_common_attributes): New function.
	(process_function_and_variable_attributes): Use it.
	* gcc.dg/attr-weakref-3.c: New testcase.
Richard Guenther - Jan. 24, 2011, 12:40 p.m.
On Sat, 22 Jan 2011, Jan Hubicka wrote:

> Hi,
> this PR shows ICE on invalid
> static int i __attribute__ ((weakref));
> 
> according to docs, weakref attribute should be accompanied with an alias.
> This patch adds checking into cgraphunit.c.
> 
> Bootstrapped/regtested x86_64-linux, OK?

It's bad this is not possible in c-common when building the
weakref attribute.  Can't we simply ignore weakref without
alias?  That would be a tiny bit cleaner than verifying
attributes from cgraph ...

Richard.

> Honza
> 
> 	PR tree-optimization/47190
> 	* cgraphunit.c (process_common_attributes): New function.
> 	(process_function_and_variable_attributes): Use it.
> 	* gcc.dg/attr-weakref-3.c: New testcase.
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c	(revision 169127)
> +++ cgraphunit.c	(working copy)
> @@ -791,6 +791,23 @@ cgraph_analyze_function (struct cgraph_n
>    current_function_decl = save;
>  }
>  
> +/* Process attributes common for vars and functions.  */
> +
> +static void
> +process_common_attributes (tree decl)
> +{
> +  tree weakref = lookup_attribute ("weakref", DECL_ATTRIBUTES (decl));
> +
> +  if (weakref && !lookup_attribute ("alias", DECL_ATTRIBUTES (decl)))
> +    {
> +      error_at (DECL_SOURCE_LOCATION (decl),
> +		"%<weakref%> attribute should be accompanied with"
> +		" an %<alias%> attribute");
> +      DECL_WEAK (decl) = 0;
> +      remove_attribute ("weakref", DECL_ATTRIBUTES (decl));
> +    }
> +}
> +
>  /* Look for externally_visible and used attributes and mark cgraph nodes
>     accordingly.
>  
> @@ -843,6 +860,7 @@ process_function_and_variable_attributes
>  	  else if (node->local.finalized)
>  	     cgraph_mark_needed_node (node);
>  	}
> +      process_common_attributes (decl);
>      }
>    for (vnode = varpool_nodes; vnode != first_var; vnode = vnode->next)
>      {
> @@ -869,6 +887,7 @@ process_function_and_variable_attributes
>  	  else if (vnode->finalized)
>  	    varpool_mark_needed_node (vnode);
>  	}
> +      process_common_attributes (decl);
>      }
>  }
>  
> Index: testsuite/gcc.dg/attr-weakref-3.c
> ===================================================================
> --- testsuite/gcc.dg/attr-weakref-3.c	(revision 0)
> +++ testsuite/gcc.dg/attr-weakref-3.c	(revision 0)
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +/* { dg-require-weak "" } */
> +static int i __attribute__ ((weakref)); /* { dg-error "should be accompanied" } */
> 
>
Jan Hubicka - Jan. 24, 2011, 2:18 p.m.
> On Sat, 22 Jan 2011, Jan Hubicka wrote:
> 
> > Hi,
> > this PR shows ICE on invalid
> > static int i __attribute__ ((weakref));
> > 
> > according to docs, weakref attribute should be accompanied with an alias.
> > This patch adds checking into cgraphunit.c.
> > 
> > Bootstrapped/regtested x86_64-linux, OK?
> 
> It's bad this is not possible in c-common when building the
> weakref attribute.  Can't we simply ignore weakref without
> alias?  That would be a tiny bit cleaner than verifying
> attributes from cgraph ...
Docs explicitly shows the testcase:
static int x() __attribute__ ((weakref));
static int x() __attribute__ ((alias ("y")));

I think whatever we do during parsing would refuse the first declaration.

Since we do this attribute finalization there anyway, I don't think it is that bad.

Honza
Richard Guenther - Jan. 24, 2011, 2:43 p.m.
On Mon, 24 Jan 2011, Jan Hubicka wrote:

> > On Sat, 22 Jan 2011, Jan Hubicka wrote:
> > 
> > > Hi,
> > > this PR shows ICE on invalid
> > > static int i __attribute__ ((weakref));
> > > 
> > > according to docs, weakref attribute should be accompanied with an alias.
> > > This patch adds checking into cgraphunit.c.
> > > 
> > > Bootstrapped/regtested x86_64-linux, OK?
> > 
> > It's bad this is not possible in c-common when building the
> > weakref attribute.  Can't we simply ignore weakref without
> > alias?  That would be a tiny bit cleaner than verifying
> > attributes from cgraph ...
> Docs explicitly shows the testcase:
> static int x() __attribute__ ((weakref));
> static int x() __attribute__ ((alias ("y")));
> 
> I think whatever we do during parsing would refuse the first declaration.
> 
> Since we do this attribute finalization there anyway, I don't think it is that bad.

Any reason we can't ignore non-aliased weakrefs at uses?

Richard.
Jan Hubicka - Jan. 24, 2011, 4:30 p.m.
> On Mon, 24 Jan 2011, Jan Hubicka wrote:
> 
> > > On Sat, 22 Jan 2011, Jan Hubicka wrote:
> > > 
> > > > Hi,
> > > > this PR shows ICE on invalid
> > > > static int i __attribute__ ((weakref));
> > > > 
> > > > according to docs, weakref attribute should be accompanied with an alias.
> > > > This patch adds checking into cgraphunit.c.
> > > > 
> > > > Bootstrapped/regtested x86_64-linux, OK?
> > > 
> > > It's bad this is not possible in c-common when building the
> > > weakref attribute.  Can't we simply ignore weakref without
> > > alias?  That would be a tiny bit cleaner than verifying
> > > attributes from cgraph ...
> > Docs explicitly shows the testcase:
> > static int x() __attribute__ ((weakref));
> > static int x() __attribute__ ((alias ("y")));
> > 
> > I think whatever we do during parsing would refuse the first declaration.
> > 
> > Since we do this attribute finalization there anyway, I don't think it is that bad.
> 
> Any reason we can't ignore non-aliased weakrefs at uses?

The ICE is about fact that we drop WEAK flag on static var:

      if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
        error_at (DECL_SOURCE_LOCATION (*node),
                  "weakref attribute must appear before alias attribute");

      /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
         and that isn't supported; and because it wants to add it to
         the list of weak decls, which isn't helpful.  */
      DECL_WEAK (*node) = 1;

and my sanity check declares it as nonsential that it indeed is.
So if we want to define that weakref alone is no-op (normal static var), we can
just keep the code removing WEAK flag I added and remove the error.

Honza
> 
> Richard.
Richard Guenther - Jan. 24, 2011, 4:40 p.m.
On Mon, 24 Jan 2011, Jan Hubicka wrote:

> > On Mon, 24 Jan 2011, Jan Hubicka wrote:
> > 
> > > > On Sat, 22 Jan 2011, Jan Hubicka wrote:
> > > > 
> > > > > Hi,
> > > > > this PR shows ICE on invalid
> > > > > static int i __attribute__ ((weakref));
> > > > > 
> > > > > according to docs, weakref attribute should be accompanied with an alias.
> > > > > This patch adds checking into cgraphunit.c.
> > > > > 
> > > > > Bootstrapped/regtested x86_64-linux, OK?
> > > > 
> > > > It's bad this is not possible in c-common when building the
> > > > weakref attribute.  Can't we simply ignore weakref without
> > > > alias?  That would be a tiny bit cleaner than verifying
> > > > attributes from cgraph ...
> > > Docs explicitly shows the testcase:
> > > static int x() __attribute__ ((weakref));
> > > static int x() __attribute__ ((alias ("y")));
> > > 
> > > I think whatever we do during parsing would refuse the first declaration.
> > > 
> > > Since we do this attribute finalization there anyway, I don't think it is that bad.
> > 
> > Any reason we can't ignore non-aliased weakrefs at uses?
> 
> The ICE is about fact that we drop WEAK flag on static var:
> 
>       if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
>         error_at (DECL_SOURCE_LOCATION (*node),
>                   "weakref attribute must appear before alias attribute");
> 
>       /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
>          and that isn't supported; and because it wants to add it to
>          the list of weak decls, which isn't helpful.  */
>       DECL_WEAK (*node) = 1;
> 
> and my sanity check declares it as nonsential that it indeed is.
> So if we want to define that weakref alone is no-op (normal static var), we can
> just keep the code removing WEAK flag I added and remove the error.

Yes, I like that more.

Richard.
Jan Hubicka - Jan. 24, 2011, 4:43 p.m.
> > The ICE is about fact that we drop WEAK flag on static var:
> > 
> >       if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
> >         error_at (DECL_SOURCE_LOCATION (*node),
> >                   "weakref attribute must appear before alias attribute");
> > 
> >       /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
> >          and that isn't supported; and because it wants to add it to
> >          the list of weak decls, which isn't helpful.  */
> >       DECL_WEAK (*node) = 1;
> > 
> > and my sanity check declares it as nonsential that it indeed is.
> > So if we want to define that weakref alone is no-op (normal static var), we can
> > just keep the code removing WEAK flag I added and remove the error.
> 
> Yes, I like that more.

Why we want to silently accept use of attribute that has no effect? (and makes no
sense?).  I can only imagine this to be result of not understanding what weakref
means.

Honza
> 
> Richard.
Richard Guenther - Jan. 24, 2011, 4:56 p.m.
On Mon, 24 Jan 2011, Jan Hubicka wrote:

> > > The ICE is about fact that we drop WEAK flag on static var:
> > > 
> > >       if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
> > >         error_at (DECL_SOURCE_LOCATION (*node),
> > >                   "weakref attribute must appear before alias attribute");
> > > 
> > >       /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
> > >          and that isn't supported; and because it wants to add it to
> > >          the list of weak decls, which isn't helpful.  */
> > >       DECL_WEAK (*node) = 1;
> > > 
> > > and my sanity check declares it as nonsential that it indeed is.
> > > So if we want to define that weakref alone is no-op (normal static var), we can
> > > just keep the code removing WEAK flag I added and remove the error.
> > 
> > Yes, I like that more.
> 
> Why we want to silently accept use of attribute that has no effect? (and makes no
> sense?).  I can only imagine this to be result of not understanding what weakref
> means.

Then it should be a warning, certainly not an error.

Richard.
Jan Hubicka - Jan. 24, 2011, 5:15 p.m.
> On Mon, 24 Jan 2011, Jan Hubicka wrote:
> 
> > > > The ICE is about fact that we drop WEAK flag on static var:
> > > > 
> > > >       if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
> > > >         error_at (DECL_SOURCE_LOCATION (*node),
> > > >                   "weakref attribute must appear before alias attribute");
> > > > 
> > > >       /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
> > > >          and that isn't supported; and because it wants to add it to
> > > >          the list of weak decls, which isn't helpful.  */
> > > >       DECL_WEAK (*node) = 1;
> > > > 
> > > > and my sanity check declares it as nonsential that it indeed is.
> > > > So if we want to define that weakref alone is no-op (normal static var), we can
> > > > just keep the code removing WEAK flag I added and remove the error.
> > > 
> > > Yes, I like that more.
> > 
> > Why we want to silently accept use of attribute that has no effect? (and makes no
> > sense?).  I can only imagine this to be result of not understanding what weakref
> > means.
> 
> Then it should be a warning, certainly not an error.

Would be the patch fine with
warning (OPT_Wattributes, ....);
then?
Ths is how we output the attribute ignored warnings.

Note that we have related error here:
  if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
    { 
      tree alias = DECL_ASSEMBLER_NAME (decl);

      is_weakref = true;

      ultimate_transparent_alias_target (&target);

      if (alias == target)
        error ("weakref %q+D ultimately targets itself", decl);
      else
        {
#ifndef ASM_OUTPUT_WEAKREF
          IDENTIFIER_TRANSPARENT_ALIAS (alias) = 1;
          TREE_CHAIN (alias) = target;
#endif  
        }
      if (TREE_PUBLIC (decl))
        error ("weakref %q+D must have static linkage", decl);
    }
I think it was supposed to catch the those weakrefs, too. It doesn't because
we get into assemble_alias only for "alias" attribute, not weakref.
So I guess we end up with
int test __attribute__ ((weakref));
as this new warning
for
int test __attribute__ ((weakref)) __attribute__((alias("test")));
as undetected nonsense
and
int test __attribute__ ((weakref)) asm("test") __attribute__((alias("test")));
with the error above.

Honza
Richard Guenther - Jan. 25, 2011, 10:05 a.m.
On Mon, 24 Jan 2011, Jan Hubicka wrote:

> > On Mon, 24 Jan 2011, Jan Hubicka wrote:
> > 
> > > > > The ICE is about fact that we drop WEAK flag on static var:
> > > > > 
> > > > >       if (lookup_attribute ("alias", DECL_ATTRIBUTES (*node)))
> > > > >         error_at (DECL_SOURCE_LOCATION (*node),
> > > > >                   "weakref attribute must appear before alias attribute");
> > > > > 
> > > > >       /* Can't call declare_weak because it wants this to be TREE_PUBLIC,
> > > > >          and that isn't supported; and because it wants to add it to
> > > > >          the list of weak decls, which isn't helpful.  */
> > > > >       DECL_WEAK (*node) = 1;
> > > > > 
> > > > > and my sanity check declares it as nonsential that it indeed is.
> > > > > So if we want to define that weakref alone is no-op (normal static var), we can
> > > > > just keep the code removing WEAK flag I added and remove the error.
> > > > 
> > > > Yes, I like that more.
> > > 
> > > Why we want to silently accept use of attribute that has no effect? (and makes no
> > > sense?).  I can only imagine this to be result of not understanding what weakref
> > > means.
> > 
> > Then it should be a warning, certainly not an error.
> 
> Would be the patch fine with
> warning (OPT_Wattributes, ....);
> then?
> Ths is how we output the attribute ignored warnings.
> 
> Note that we have related error here:
>   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
>     { 
>       tree alias = DECL_ASSEMBLER_NAME (decl);
> 
>       is_weakref = true;
> 
>       ultimate_transparent_alias_target (&target);
> 
>       if (alias == target)
>         error ("weakref %q+D ultimately targets itself", decl);
>       else
>         {
> #ifndef ASM_OUTPUT_WEAKREF
>           IDENTIFIER_TRANSPARENT_ALIAS (alias) = 1;
>           TREE_CHAIN (alias) = target;
> #endif  
>         }
>       if (TREE_PUBLIC (decl))
>         error ("weakref %q+D must have static linkage", decl);
>     }

But this is not in cgraphunit.c or called from visibility attribute
processing.  It would be ok to move the error to assemble_alias,
but if we accepted the code before then we should only issue a
warning that the wearkref is ignored.

> I think it was supposed to catch the those weakrefs, too. It doesn't because
> we get into assemble_alias only for "alias" attribute, not weakref.

Well.  Still process_function_and_variable_attributes is not the
proper place to emit the error.

If we want to emit errors from the middle-end and not from the frontend
then cgraph_finalize_function is probably the time to do it (we already
dispatch to do_warn_unused_parameter there).

Richard.
Jan Hubicka - Jan. 25, 2011, 10:24 a.m.
> 
> But this is not in cgraphunit.c or called from visibility attribute
> processing.  It would be ok to move the error to assemble_alias,
> but if we accepted the code before then we should only issue a
> warning that the wearkref is ignored.

assemble_alias is never called on the variable since we dispatch to it
only on existence of "alias" attribute, not "weakref", from rest_of_decl_compilation.

We probably could try to do the warning from rest_of_decl_compilation, but I think this
function is still not unit-at-a-time, so we might have problem with attributes added
after the initialization.  Will check this.
> 
> > I think it was supposed to catch the those weakrefs, too. It doesn't because
> > we get into assemble_alias only for "alias" attribute, not weakref.
> 
> Well.  Still process_function_and_variable_attributes is not the
> proper place to emit the error.

Well, process_function_and_variable_attributes was invented to finalize attributes
that can not be handled well at parsing time because of decl merging issues.
(we orginally tried to handle the attributes processed there in FE too and had
funny bugs)

You are concerned that the error is too late?
> 
> If we want to emit errors from the middle-end and not from the frontend
> then cgraph_finalize_function is probably the time to do it (we already
> dispatch to do_warn_unused_parameter there).

Since those functions are not defined, cgraph_finalize_function is never called
on the decl.  Also we support weakrefs on vars, too, as far as I can tell.

Honza
> 
> Richard.
Richard Guenther - Jan. 25, 2011, 11:26 a.m.
On Tue, 25 Jan 2011, Jan Hubicka wrote:

> > 
> > But this is not in cgraphunit.c or called from visibility attribute
> > processing.  It would be ok to move the error to assemble_alias,
> > but if we accepted the code before then we should only issue a
> > warning that the wearkref is ignored.
> 
> assemble_alias is never called on the variable since we dispatch to it
> only on existence of "alias" attribute, not "weakref", from rest_of_decl_compilation.
> 
> We probably could try to do the warning from rest_of_decl_compilation, but I think this
> function is still not unit-at-a-time, so we might have problem with attributes added
> after the initialization.  Will check this.
> > 
> > > I think it was supposed to catch the those weakrefs, too. It doesn't because
> > > we get into assemble_alias only for "alias" attribute, not weakref.
> > 
> > Well.  Still process_function_and_variable_attributes is not the
> > proper place to emit the error.
> 
> Well, process_function_and_variable_attributes was invented to finalize attributes
> that can not be handled well at parsing time because of decl merging issues.
> (we orginally tried to handle the attributes processed there in FE too and had
> funny bugs)
> 
> You are concerned that the error is too late?

Yes.  It also doesn't "fit".

I'd rather do it when first analyzing the compilation unit, maybe
when we build the cgraph and references?

> > 
> > If we want to emit errors from the middle-end and not from the frontend
> > then cgraph_finalize_function is probably the time to do it (we already
> > dispatch to do_warn_unused_parameter there).
> 
> Since those functions are not defined, cgraph_finalize_function is never called
> on the decl.  Also we support weakrefs on vars, too, as far as I can tell.
> 
> Honza
> > 
> > Richard.
> 
>
Jan Hubicka - Jan. 25, 2011, 3:30 p.m.
> > Well, process_function_and_variable_attributes was invented to finalize attributes
> > that can not be handled well at parsing time because of decl merging issues.
> > (we orginally tried to handle the attributes processed there in FE too and had
> > funny bugs)
> > 
> > You are concerned that the error is too late?
> 
> Yes.  It also doesn't "fit".
> 
> I'd rather do it when first analyzing the compilation unit, maybe
> when we build the cgraph and references?

Well, when I was fixing the issues with externally visible attributes, I also
decided it should go to first place when we analyze compilation unit and it is
where process_function_and_variable_attributes is called.  See
cgraph_analyze_functions.
It is done just before cgraph construction and outputs other warnings too.

HOnza
Richard Guenther - Jan. 25, 2011, 3:34 p.m.
On Tue, 25 Jan 2011, Jan Hubicka wrote:

> > > Well, process_function_and_variable_attributes was invented to finalize attributes
> > > that can not be handled well at parsing time because of decl merging issues.
> > > (we orginally tried to handle the attributes processed there in FE too and had
> > > funny bugs)
> > > 
> > > You are concerned that the error is too late?
> > 
> > Yes.  It also doesn't "fit".
> > 
> > I'd rather do it when first analyzing the compilation unit, maybe
> > when we build the cgraph and references?
> 
> Well, when I was fixing the issues with externally visible attributes, I also
> decided it should go to first place when we analyze compilation unit and it is
> where process_function_and_variable_attributes is called.  See
> cgraph_analyze_functions.
> It is done just before cgraph construction and outputs other warnings too.

Hmm, ok.  I guess your original patch is ok then.

Thanks for bearing with me ;)
Richard.
Jan Hubicka - Jan. 25, 2011, 3:46 p.m.
> On Tue, 25 Jan 2011, Jan Hubicka wrote:
> 
> > > > Well, process_function_and_variable_attributes was invented to finalize attributes
> > > > that can not be handled well at parsing time because of decl merging issues.
> > > > (we orginally tried to handle the attributes processed there in FE too and had
> > > > funny bugs)
> > > > 
> > > > You are concerned that the error is too late?
> > > 
> > > Yes.  It also doesn't "fit".
> > > 
> > > I'd rather do it when first analyzing the compilation unit, maybe
> > > when we build the cgraph and references?
> > 
> > Well, when I was fixing the issues with externally visible attributes, I also
> > decided it should go to first place when we analyze compilation unit and it is
> > where process_function_and_variable_attributes is called.  See
> > cgraph_analyze_functions.
> > It is done just before cgraph construction and outputs other warnings too.
> 
> Hmm, ok.  I guess your original patch is ok then.

... with error changed to warning, right?

> 
> Thanks for bearing with me ;)

:) It is all bit confusing.
I think we have more issues with the attributes and declaration merging.  The
problem is that the way attributes are handled in c-common is to do checks at
the time the attribute is added, i.e. on incomplette version of declaration.
Later one can change the declaration in a way it is no longer compatible with
the argument.

IMO it would make a lot of sense to move all this attribute sanity checking to
time parsing is done and move all the stuff currently done in the handle_*
functions there.

Honza
Richard Guenther - Jan. 25, 2011, 3:47 p.m.
On Tue, 25 Jan 2011, Jan Hubicka wrote:

> > On Tue, 25 Jan 2011, Jan Hubicka wrote:
> > 
> > > > > Well, process_function_and_variable_attributes was invented to finalize attributes
> > > > > that can not be handled well at parsing time because of decl merging issues.
> > > > > (we orginally tried to handle the attributes processed there in FE too and had
> > > > > funny bugs)
> > > > > 
> > > > > You are concerned that the error is too late?
> > > > 
> > > > Yes.  It also doesn't "fit".
> > > > 
> > > > I'd rather do it when first analyzing the compilation unit, maybe
> > > > when we build the cgraph and references?
> > > 
> > > Well, when I was fixing the issues with externally visible attributes, I also
> > > decided it should go to first place when we analyze compilation unit and it is
> > > where process_function_and_variable_attributes is called.  See
> > > cgraph_analyze_functions.
> > > It is done just before cgraph construction and outputs other warnings too.
> > 
> > Hmm, ok.  I guess your original patch is ok then.
> 
> ... with error changed to warning, right?

Yes.

> > 
> > Thanks for bearing with me ;)
> 
> :) It is all bit confusing.
> I think we have more issues with the attributes and declaration merging.  The
> problem is that the way attributes are handled in c-common is to do checks at
> the time the attribute is added, i.e. on incomplette version of declaration.
> Later one can change the declaration in a way it is no longer compatible with
> the argument.
> 
> IMO it would make a lot of sense to move all this attribute sanity checking to
> time parsing is done and move all the stuff currently done in the handle_*
> functions there.

Well, that of course would have the issue that the errors/warnings appear
out-of-order with other diagnostics ...

Richard.

Patch

Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 169127)
+++ cgraphunit.c	(working copy)
@@ -791,6 +791,23 @@  cgraph_analyze_function (struct cgraph_n
   current_function_decl = save;
 }
 
+/* Process attributes common for vars and functions.  */
+
+static void
+process_common_attributes (tree decl)
+{
+  tree weakref = lookup_attribute ("weakref", DECL_ATTRIBUTES (decl));
+
+  if (weakref && !lookup_attribute ("alias", DECL_ATTRIBUTES (decl)))
+    {
+      error_at (DECL_SOURCE_LOCATION (decl),
+		"%<weakref%> attribute should be accompanied with"
+		" an %<alias%> attribute");
+      DECL_WEAK (decl) = 0;
+      remove_attribute ("weakref", DECL_ATTRIBUTES (decl));
+    }
+}
+
 /* Look for externally_visible and used attributes and mark cgraph nodes
    accordingly.
 
@@ -843,6 +860,7 @@  process_function_and_variable_attributes
 	  else if (node->local.finalized)
 	     cgraph_mark_needed_node (node);
 	}
+      process_common_attributes (decl);
     }
   for (vnode = varpool_nodes; vnode != first_var; vnode = vnode->next)
     {
@@ -869,6 +887,7 @@  process_function_and_variable_attributes
 	  else if (vnode->finalized)
 	    varpool_mark_needed_node (vnode);
 	}
+      process_common_attributes (decl);
     }
 }
 
Index: testsuite/gcc.dg/attr-weakref-3.c
===================================================================
--- testsuite/gcc.dg/attr-weakref-3.c	(revision 0)
+++ testsuite/gcc.dg/attr-weakref-3.c	(revision 0)
@@ -0,0 +1,3 @@ 
+/* { dg-do compile } */
+/* { dg-require-weak "" } */
+static int i __attribute__ ((weakref)); /* { dg-error "should be accompanied" } */