Re: [PATCH] drop weakref attribute on function definitions (PR 92799)
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
Re: [PATCH] drop weakref attribute on function definitions (PR 92799)
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
Re: [PATCH] drop weakref attribute on function definitions (PR 92799)
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
Re: [PATCH] drop weakref attribute on function definitions (PR 92799)
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 >
[PING #2][PATCH] drop weakref attribute on function definitions (PR 92799)
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
PING [PATCH] drop weakref attribute on function definitions (PR 92799)
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
[PATCH] drop weakref attribute on function definitions (PR 92799)
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
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,
"% 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 000..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; }
