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
