| Message ID | 20110122174850.GB31129@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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" } */ > >
> 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
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.
> 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.
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.
> > 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.
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.
> 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
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.
> > 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.
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. > >
> > 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
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.
> 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
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.
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" } */