drop weakref attribute on function definitions (PR 92799)

Message ID ea430884-deec-e239-611a-a761d82518ac@gmail.com
State New
Headers show
Series
  • drop weakref attribute on function definitions (PR 92799)
Related show

Commit Message

Martin Sebor Feb. 14, 2020, 10:41 p.m.
Because attribute weakref introduces a kind of a definition, it can
only be applied to declarations of symbols that are not defined.  GCC
normally issues a warning when the attribute is applied to a defined
symbol, but PR 92799 shows that it misses some cases on which it then
leads to an ICE.

The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such
invalid definitions and silently dropped the weakref attribute.

The attached patch avoids the ICE while again dropping the invalid
attribute from the definition, except with the (now) usual warning.

Tested on x86_64-linux.

I also looked for code bases that make use of attribute weakref to
rebuild them as another test but couldn't find any.  (There are
a couple of instances in the Linux kernel but they look #ifdef'd
out).  Does anyone know of any that do use it that I could try to
build on Linux?

Martin

Comments

Martin Sebor Feb. 21, 2020, 4:49 p.m. | #1
Ping: https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00883.html

On 2/14/20 3:41 PM, Martin Sebor wrote:
> Because attribute weakref introduces a kind of a definition, it can

> only be applied to declarations of symbols that are not defined.  GCC

> normally issues a warning when the attribute is applied to a defined

> symbol, but PR 92799 shows that it misses some cases on which it then

> leads to an ICE.

> 

> The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

> invalid definitions and silently dropped the weakref attribute.

> 

> The attached patch avoids the ICE while again dropping the invalid

> attribute from the definition, except with the (now) usual warning.

> 

> Tested on x86_64-linux.

> 

> I also looked for code bases that make use of attribute weakref to

> rebuild them as another test but couldn't find any.  (There are

> a couple of instances in the Linux kernel but they look #ifdef'd

> out).  Does anyone know of any that do use it that I could try to

> build on Linux?

> 

> Martin
Martin Sebor March 2, 2020, 10:15 p.m. | #2
Ping: https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00883.html

On 2/21/20 9:49 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00883.html

> 

> On 2/14/20 3:41 PM, Martin Sebor wrote:

>> Because attribute weakref introduces a kind of a definition, it can

>> only be applied to declarations of symbols that are not defined.  GCC

>> normally issues a warning when the attribute is applied to a defined

>> symbol, but PR 92799 shows that it misses some cases on which it then

>> leads to an ICE.

>>

>> The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

>> invalid definitions and silently dropped the weakref attribute.

>>

>> The attached patch avoids the ICE while again dropping the invalid

>> attribute from the definition, except with the (now) usual warning.

>>

>> Tested on x86_64-linux.

>>

>> I also looked for code bases that make use of attribute weakref to

>> rebuild them as another test but couldn't find any.  (There are

>> a couple of instances in the Linux kernel but they look #ifdef'd

>> out).  Does anyone know of any that do use it that I could try to

>> build on Linux?

>>

>> Martin

>
Jeff Law March 6, 2020, 12:26 a.m. | #3
On Fri, 2020-02-14 at 15:41 -0700, Martin Sebor wrote:
> Because attribute weakref introduces a kind of a definition, it can

> only be applied to declarations of symbols that are not defined.  GCC

> normally issues a warning when the attribute is applied to a defined

> symbol, but PR 92799 shows that it misses some cases on which it then

> leads to an ICE.

> 

> The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

> invalid definitions and silently dropped the weakref attribute.

> 

> The attached patch avoids the ICE while again dropping the invalid

> attribute from the definition, except with the (now) usual warning.

> 

> Tested on x86_64-linux.

> 

> I also looked for code bases that make use of attribute weakref to

> rebuild them as another test but couldn't find any.  (There are

> a couple of instances in the Linux kernel but they look #ifdef'd

> out).  Does anyone know of any that do use it that I could try to

> build on Linux?

So you added this check

... || DECL_INITIAL (decl) != error_mark_node

Do you need to check that DECL_INITIAL is not NULL?  IIUC DECL_INITIAL in this
context is a tri-state.

NULL -- DECL is not a function definition
error_mark_node -- it was a function definition, but the body was free'd
everything else -- the function definition

Jeff
>
Martin Sebor March 9, 2020, 8:33 p.m. | #4
On 3/5/20 5:26 PM, Jeff Law wrote:
> On Fri, 2020-02-14 at 15:41 -0700, Martin Sebor wrote:

>> Because attribute weakref introduces a kind of a definition, it can

>> only be applied to declarations of symbols that are not defined.  GCC

>> normally issues a warning when the attribute is applied to a defined

>> symbol, but PR 92799 shows that it misses some cases on which it then

>> leads to an ICE.

>>

>> The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

>> invalid definitions and silently dropped the weakref attribute.

>>

>> The attached patch avoids the ICE while again dropping the invalid

>> attribute from the definition, except with the (now) usual warning.

>>

>> Tested on x86_64-linux.

>>

>> I also looked for code bases that make use of attribute weakref to

>> rebuild them as another test but couldn't find any.  (There are

>> a couple of instances in the Linux kernel but they look #ifdef'd

>> out).  Does anyone know of any that do use it that I could try to

>> build on Linux?

> So you added this check

> 

> ... || DECL_INITIAL (decl) != error_mark_node

> 

> Do you need to check that DECL_INITIAL is not NULL?  IIUC DECL_INITIAL in this

> context is a tri-state.

> 

> NULL -- DECL is not a function definition

> error_mark_node -- it was a function definition, but the body was free'd

> everything else -- the function definition


I've only seen two values come up for a function declared weakref in
the test suite: error_mark_node and something with the TREE_CODE of
BLOCK (the block where the weakref function is used when it's also
explicitly defined in the code, and when the attribute is subsequently
diagnosed by the warning).

The weakref attribute provides a definition for a declaration that
is not a definition.  DECL_INITIAL is set to error_mark_node in
handle_alias_ifunc_attribute called (indirectly) from
handle_weakref_attribute.  So case 1 never comes up and cases 2 and
3 above are an error.  Once defined by the means of the attribute,
the function can be redeclared but its DECL_INITIAL is still
error_mark_node.

For DECL_INITIAL (decl) weakref variables DECL_INITIAL is set to null
(in this case GCC issues a warning and ignores the attribute).  I don't
know why functions have this special treatment and there's no comment
to explain.

Anyway, I don't have a problem adding the check but (as always) I'd
like to be able to test it.  Let me know if you know how, or what
you want me to do.

Martin
Jose E. Marchesi via Gcc-patches March 12, 2020, 8:10 p.m. | #5
On Mon, 2020-03-09 at 14:33 -0600, Martin Sebor wrote:
> On 3/5/20 5:26 PM, Jeff Law wrote:

> > On Fri, 2020-02-14 at 15:41 -0700, Martin Sebor wrote:

> > > Because attribute weakref introduces a kind of a definition, it can

> > > only be applied to declarations of symbols that are not defined.  GCC

> > > normally issues a warning when the attribute is applied to a defined

> > > symbol, but PR 92799 shows that it misses some cases on which it then

> > > leads to an ICE.

> > > 

> > > The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

> > > invalid definitions and silently dropped the weakref attribute.

> > > 

> > > The attached patch avoids the ICE while again dropping the invalid

> > > attribute from the definition, except with the (now) usual warning.

> > > 

> > > Tested on x86_64-linux.

> > > 

> > > I also looked for code bases that make use of attribute weakref to

> > > rebuild them as another test but couldn't find any.  (There are

> > > a couple of instances in the Linux kernel but they look #ifdef'd

> > > out).  Does anyone know of any that do use it that I could try to

> > > build on Linux?

> > So you added this check

> > 

> > ... || DECL_INITIAL (decl) != error_mark_node

> > 

> > Do you need to check that DECL_INITIAL is not NULL?  IIUC DECL_INITIAL in

> > this

> > context is a tri-state.

> > 

> > NULL -- DECL is not a function definition

> > error_mark_node -- it was a function definition, but the body was free'd

> > everything else -- the function definition

> 

> I've only seen two values come up for a function declared weakref in

> the test suite: error_mark_node and something with the TREE_CODE of

> BLOCK (the block where the weakref function is used when it's also

> explicitly defined in the code, and when the attribute is subsequently

> diagnosed by the warning).

So when it's a BLOCK it's giving you the outermost block scope for the function
which we usually use to generate debug info.

The key point is that we use ERROR_MARK_NODE because a non-NULL DECL_INITIAL
denotes an actual function definition, so we can't set it to NULL blindly.  See
cgraph_node::expand, cgraph_node:release_body, rest_of_handle_final and perhaps
other places.  Another example would be setting up the ifunc resolver.  It's a
real function so DECL_INITIAL must be non-NULL, but we don't really have a block
structure we can point to, so instead we set it to error_mark_node
(make_dispatcher_decl).

I wonder if the earlier node->definition check ultimately prevents DECL_INITIAL
from being NULL at this point.  According to cgraph.h that field is true when the
symbol corresponds to a definition in the current unit.  That would seem to
indicate yes.

So I think we can go forward with your patch as-is.

jeff
Jose E. Marchesi via Gcc-patches March 18, 2020, 8:52 p.m. | #6
On 3/12/20 2:10 PM, Jeff Law wrote:
> On Mon, 2020-03-09 at 14:33 -0600, Martin Sebor wrote:

>> On 3/5/20 5:26 PM, Jeff Law wrote:

>>> On Fri, 2020-02-14 at 15:41 -0700, Martin Sebor wrote:

>>>> Because attribute weakref introduces a kind of a definition, it can

>>>> only be applied to declarations of symbols that are not defined.  GCC

>>>> normally issues a warning when the attribute is applied to a defined

>>>> symbol, but PR 92799 shows that it misses some cases on which it then

>>>> leads to an ICE.

>>>>

>>>> The ICE was introduced in GCC 4.5.  Prior to then, GCC accepted such

>>>> invalid definitions and silently dropped the weakref attribute.

>>>>

>>>> The attached patch avoids the ICE while again dropping the invalid

>>>> attribute from the definition, except with the (now) usual warning.

>>>>

>>>> Tested on x86_64-linux.

>>>>

>>>> I also looked for code bases that make use of attribute weakref to

>>>> rebuild them as another test but couldn't find any.  (There are

>>>> a couple of instances in the Linux kernel but they look #ifdef'd

>>>> out).  Does anyone know of any that do use it that I could try to

>>>> build on Linux?

>>> So you added this check

>>>

>>> ... || DECL_INITIAL (decl) != error_mark_node

>>>

>>> Do you need to check that DECL_INITIAL is not NULL?  IIUC DECL_INITIAL in

>>> this

>>> context is a tri-state.

>>>

>>> NULL -- DECL is not a function definition

>>> error_mark_node -- it was a function definition, but the body was free'd

>>> everything else -- the function definition

>>

>> I've only seen two values come up for a function declared weakref in

>> the test suite: error_mark_node and something with the TREE_CODE of

>> BLOCK (the block where the weakref function is used when it's also

>> explicitly defined in the code, and when the attribute is subsequently

>> diagnosed by the warning).

> So when it's a BLOCK it's giving you the outermost block scope for the function

> which we usually use to generate debug info.

> 

> The key point is that we use ERROR_MARK_NODE because a non-NULL DECL_INITIAL

> denotes an actual function definition, so we can't set it to NULL blindly.  See

> cgraph_node::expand, cgraph_node:release_body, rest_of_handle_final and perhaps

> other places.  Another example would be setting up the ifunc resolver.  It's a

> real function so DECL_INITIAL must be non-NULL, but we don't really have a block

> structure we can point to, so instead we set it to error_mark_node

> (make_dispatcher_decl).

> 

> I wonder if the earlier node->definition check ultimately prevents DECL_INITIAL

> from being NULL at this point.  According to cgraph.h that field is true when the

> symbol corresponds to a definition in the current unit.  That would seem to

> indicate yes.

> 

> So I think we can go forward with your patch as-is.


Thanks.  I committed it in r10-7267.  I'll give it a few days and then
backport it to release branches unless there's any concern with it.

Martin

Patch

PR ipa/92799 - ICE on a weakref function definition followed by a declaration

gcc/testsuite/ChangeLog:

	PR ipa/92799
	* gcc.dg/attr-weakref-5.c: New test.

gcc/ChangeLog:

	PR ipa/92799
	* cgraphunit.c (process_function_and_variable_attributes): Also
	complain about weakref function definitions and drop all effects
	of the attribute.

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index a9dd288be57..fd586366bb9 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -861,14 +861,23 @@  process_function_and_variable_attributes (cgraph_node *first,
 			" attribute have effect only on public objects");
 	}
       if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl))
-	  && (node->definition && !node->alias))
+	  && node->definition
+	  && (!node->alias || DECL_INITIAL (decl) != error_mark_node))
 	{
-	  warning_at (DECL_SOURCE_LOCATION (node->decl), OPT_Wattributes,
+	  /* NODE->DEFINITION && NODE->ALIAS is nonzero for valid weakref
+	     function declarations; DECL_INITIAL is non-null for invalid
+	     weakref functions that are also defined.  */
+	  warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
 		      "%<weakref%> attribute ignored"
 		      " because function is defined");
 	  DECL_WEAK (decl) = 0;
 	  DECL_ATTRIBUTES (decl) = remove_attribute ("weakref",
 						     DECL_ATTRIBUTES (decl));
+	  DECL_ATTRIBUTES (decl) = remove_attribute ("alias",
+						     DECL_ATTRIBUTES (decl));
+	  node->alias = false;
+	  node->weakref = false;
+	  node->transparent_alias = false;
 	}
       else if (lookup_attribute ("alias", DECL_ATTRIBUTES (decl))
 	  && node->definition
diff --git a/gcc/testsuite/gcc.dg/attr-weakref-5.c b/gcc/testsuite/gcc.dg/attr-weakref-5.c
new file mode 100644
index 00000000000..e2f04068230
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-weakref-5.c
@@ -0,0 +1,31 @@ 
+/* PR middle-end/92799 - ICE on a weakref function definition followed
+   by a declaration
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+static __attribute__ ((weakref ("bar"))) void f0 (void) { }   // { dg-warning "'weakref' attribute ignored because function is defined" }
+
+extern void f0 (void);
+
+void* use_f0 (void) { return f0; }
+
+
+static __attribute__ ((weakref ("bar"))) void f1 (void) { }   // { dg-warning "'weakref' attribute ignored because function is defined" }
+
+static void f1 (void);
+
+void* use_f1 (void) { return f1; }
+
+
+static __attribute__ ((weakref ("bar"))) void f2 (void);
+
+static void f2 (void) { }                                     // { dg-error "redefinition" }
+
+void* use_f2 (void) { return f2; }
+
+
+static __attribute__ ((weakref ("bar"))) void f3 (void);
+
+void f3 (void) { }                                            // { dg-error "redefinition" }
+
+void* use_f3 (void) { return f3; }