detect references to statics in inline function signatures (PR 88718)

The attached patch extends the detection of references to static
variables in inline functions to the function signatures, including
their return type.  Since the declaration of a function need not be
yet available at the time the static is referenced (e.g., when it's
referenced in the functions return type), the patch introduces
the concept of "tentative records" of such references and either
completes the records when the full declaration of the function,
including its definition, is known to be inline, or removes it
when it isn't.

Martin

On Fri, 11 Jan 2019, Martin Sebor wrote:

> gcc/testsuite/ChangeLog:
> 
> 	PR c/88718
> 	* gcc.dg/inline-40.c: New test.
> 	* gcc.dg/inline-41.c: New test.

We already have tests inline-40.c and inline-41.c; these need to be 
renumbered accordingly.

> +      if (add)
> +	{
> +	  if (csi->function == func
> +	      || csi->function)
> +	    continue;

Since func is non-NULL, this is equivalent to "if (csi->function)".

Don't you want to break, not continue, in the case where csi->function (as 
all the tentative entries come first)?  As-is, this looks like it would 
have quadratic cost if a file has many inline functions each referencing 
some file-scope static.  (The "Avoid adding another tentative record for 
this DECL if one already exists." also looks quadratic, but having very 
many functions in a file seems more likely than very many references to 
statics in a single declaration that is not a definition.)

> @@ -5165,6 +5259,11 @@ finish_decl (tree decl, location_t init_loc, tree
>  	    set_user_assembler_name (decl, asmspec);
>  	}
>  
> +      /* Remove any tentative record of a non-static inline function
> +	 referencing a static decl made a DECL that is not a definition.  */
> +      if (TREE_CODE (decl) == FUNCTION_DECL)
> +	update_tentative_inline_static (decl);
> +	

You also need to do the update when the declaration wasn't a function 
declaration at all.  The present patch produces spurious warnings for:

static int i;
int a[sizeof i];
inline void f (void) {}

because the reference to i in the declaration of a gets treated as if it 
were in the definition of f.

Also, you need to distinguish the case of updating for a function 
definition, and updating for a declaration that's not a definition, for a 
function that was previously defined.  E.g.

inline void f (int a[sizeof (int)]) {}
static int i;
inline void f (int a[sizeof i]);

must not diagnose a reference to i in f (you have such tests with the 
declaration before the definition, but none that I can see with the 
declaration after the definition).  I think this means the caller of 
update_tentative_inline_static should specify whether to add or remove, 
rather than update_tentative_inline_static trying to deduce it from 
properties of a DECL (that don't say whether the particular declaration in 
question is a definition or not, just whether a definition has been seen).

On 1/11/19 2:27 PM, Joseph Myers wrote:
> On Fri, 11 Jan 2019, Martin Sebor wrote:
> 
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/88718
>> 	* gcc.dg/inline-40.c: New test.
>> 	* gcc.dg/inline-41.c: New test.
> 
> We already have tests inline-40.c and inline-41.c; these need to be
> renumbered accordingly.

Done.

> 
>> +      if (add)
>> +	{
>> +	  if (csi->function == func
>> +	      || csi->function)
>> +	    continue;
> 
> Since func is non-NULL, this is equivalent to "if (csi->function)".

Yes.  Done.

> 
> Don't you want to break, not continue, in the case where csi->function (as
> all the tentative entries come first)?  As-is, this looks like it would
> have quadratic cost if a file has many inline functions each referencing
> some file-scope static.  (The "Avoid adding another tentative record for
> this DECL if one already exists." also looks quadratic, but having very
> many functions in a file seems more likely than very many references to
> statics in a single declaration that is not a definition.)

Yes, good catch!

> 
>> @@ -5165,6 +5259,11 @@ finish_decl (tree decl, location_t init_loc, tree
>>   	    set_user_assembler_name (decl, asmspec);
>>   	}
>>   
>> +      /* Remove any tentative record of a non-static inline function
>> +	 referencing a static decl made a DECL that is not a definition.  */
>> +      if (TREE_CODE (decl) == FUNCTION_DECL)
>> +	update_tentative_inline_static (decl);
>> +	
> 
> You also need to do the update when the declaration wasn't a function
> declaration at all.  The present patch produces spurious warnings for:
> 
> static int i;
> int a[sizeof i];
> inline void f (void) {}
> 
> because the reference to i in the declaration of a gets treated as if it
> were in the definition of f.

Done.

> 
> Also, you need to distinguish the case of updating for a function
> definition, and updating for a declaration that's not a definition, for a
> function that was previously defined.  E.g.
> 
> inline void f (int a[sizeof (int)]) {}
> static int i;
> inline void f (int a[sizeof i]);
> 
> must not diagnose a reference to i in f (you have such tests with the
> declaration before the definition, but none that I can see with the
> declaration after the definition).  I think this means the caller of
> update_tentative_inline_static should specify whether to add or remove,
> rather than update_tentative_inline_static trying to deduce it from
> properties of a DECL (that don't say whether the particular declaration in
> question is a definition or not, just whether a definition has been seen).

Also true.  Done.

Attached is a revision with these fixes.

Thanks for the careful review!

Martin
PR c/88718 - Strange inconsistency between old style and new style definitions of inline functions.

gcc/c/ChangeLog:

	PR c/88718
	* c-decl.c (update_tentative_inline_static): New function.
	(check_inline_statics): Handle tentative records for inline
	declarations without definitions.
	Print static declaration location.
	(finish_decl): Add tentative records of references to statics.
	(finish_function): Same.
	* c-typeck.c (build_external_ref): Handle all references to statics.

gcc/testsuite/ChangeLog:

	PR c/88718
	* gcc.dg/inline-42.c: New test.
	* gcc.dg/inline-43.c: New test.

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 57049f80073..ddc09abb669 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -826,9 +826,11 @@ c_finish_incomplete_decl (tree decl)
     }
 }
 
-/* Record that inline function FUNC contains a reference (location
-   LOC) to static DECL (file-scope or function-local according to
-   TYPE).  */
+/* Record that inline function FUNC either does contain or may contain
+   a reference (location LOC) to static DECL (file-scope or function-local
+   according to TYPE).  For a null FUNC, a tentative record is created that
+   reflects a reference in the function signature and that is either updated
+   or removed when the function declaration is complete.  */
 
 void
 record_inline_static (location_t loc, tree func, tree decl,
@@ -843,6 +845,51 @@ record_inline_static (location_t loc, tree func, tree decl,
   c_inline_statics = csi;
 }
 
+/* Update tentative records of references to static declarations in
+   an inline declaration function DECL, or remove them if DECL isn't
+   a function declared inline or when ADD is false.  A tentative
+   record is one whose FUNCTION is null.  */
+
+static void
+update_tentative_inline_static (tree decl, bool add)
+{
+  gcc_assert (decl);
+
+  if (!c_inline_statics)
+    return;
+
+  /* True to associate DECL with the tentative records, false to remove
+     them.  */
+  add = (add
+	 && TREE_CODE (decl) == FUNCTION_DECL
+	 && DECL_DECLARED_INLINE_P (decl)
+	 && DECL_EXTERNAL (decl)
+	 && DECL_INITIAL (decl));
+
+  /* Iterate over tentative records (all those at the head of the list
+     with a null FUNCTION) and either associate them with DECL when ADD
+     is set or remove them from it otherwise.  */
+  for (c_inline_static *csi = c_inline_statics, *last = csi;
+       csi; csi = csi->next)
+    {
+      if (csi->function)
+	break;
+
+      if (add)
+	{
+	  if (csi->static_decl)
+	    csi->function = decl;
+	}
+      else
+	{
+	  if (last == c_inline_statics)
+	    c_inline_statics = last = csi->next;
+	  else
+	    last->next = csi->next;
+	}
+    }
+}
+
 /* Check for references to static declarations in inline functions at
    the end of the translation unit and diagnose them if the functions
    are still inline definitions.  */
@@ -853,22 +900,41 @@ check_inline_statics (void)
   struct c_inline_static *csi;
   for (csi = c_inline_statics; csi; csi = csi->next)
     {
-      if (DECL_EXTERNAL (csi->function))
-	switch (csi->type)
-	  {
-	  case csi_internal:
-	    pedwarn (csi->location, 0,
-		     "%qD is static but used in inline function %qD "
-		     "which is not static", csi->static_decl, csi->function);
-	    break;
-	  case csi_modifiable:
-	    pedwarn (csi->location, 0,
-		     "%q+D is static but declared in inline function %qD "
-		     "which is not static", csi->static_decl, csi->function);
-	    break;
-	  default:
-	    gcc_unreachable ();
-	  }
+      /* FUNCTION is unset for a declaration whose signature references
+	 a static variable and for which a definition wasn't provided.  */
+      if (!csi->function)
+	continue;
+
+      /* Only extern functions are of interest.  STATIC_DECL is null
+	 for inline functions that have been defined that contain
+	 no references to statics (but whose subsequent declarations
+	 might).  */
+      if (!DECL_EXTERNAL (csi->function)
+	  || !csi->static_decl)
+	continue;
+
+      bool warned;
+      switch (csi->type)
+	{
+	case csi_internal:
+	  warned = pedwarn (csi->location, 0,
+			    "%qD is static but used in inline function %qD "
+			    "which is not static",
+			    csi->static_decl, csi->function);
+	break;
+	case csi_modifiable:
+	  warned = pedwarn (csi->location, 0,
+			    "%q+D is static but declared in inline function %qD "
+			    "which is not static",
+			    csi->static_decl, csi->function);
+	  break;
+	default:
+	  gcc_unreachable ();
+	}
+
+      if (warned)
+	inform (DECL_SOURCE_LOCATION (csi->static_decl),
+		"%qD declared here", csi->static_decl);
     }
   c_inline_statics = NULL;
 }
@@ -5166,6 +5232,11 @@ finish_decl (tree decl, location_t init_loc, tree init,
 	    set_user_assembler_name (decl, asmspec);
 	}
 
+      /* Remove any tentative records of references to static declarations
+	 made in the declaration of DECL, which can be a fuction or some
+	 other entity.  */
+      update_tentative_inline_static (decl, false);
+
       if (DECL_FILE_SCOPE_P (decl))
 	{
 	  if (DECL_INITIAL (decl) == NULL_TREE
@@ -9637,6 +9708,11 @@ finish_function (void)
 	}
     }
 
+  /* Associate any tentative records of references to static variables
+     with this function declaration if it is inline and not static, or
+     remove them otherwise.  */
+  update_tentative_inline_static (fndecl, true);
+
   if (!decl_function_context (fndecl))
     undef_nested_function = false;
 
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 63d177f7a6f..f05519d3896 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -52,6 +52,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "stringpool.h"
 #include "attribs.h"
 #include "asan.h"
+#include "c-family/c-pragma.h"
+#include "c-parser.h"
 
 /* Possible cases of implicit bad conversions.  Used to select
    diagnostic messages in convert_for_assignment.  */
@@ -2818,19 +2820,26 @@ build_external_ref (location_t loc, tree id, bool fun, tree *type)
       if (context != NULL_TREE && context != current_function_decl)
 	DECL_NONLOCAL (ref) = 1;
     }
-  /* C99 6.7.4p3: An inline definition of a function with external
-     linkage ... shall not contain a reference to an identifier with
-     internal linkage.  */
-  else if (current_function_decl != NULL_TREE
+  else if (VAR_OR_FUNCTION_DECL_P (ref)
+	   && (!VAR_P (ref) || TREE_STATIC (ref))
+	   && ! TREE_PUBLIC (ref))
+    {
+      /* C99 6.7.4p3: An inline definition of a function with external
+	 linkage ... shall not contain a reference to an identifier with
+	 internal linkage.  */
+      if ((current_function_decl
 	   && DECL_DECLARED_INLINE_P (current_function_decl)
 	   && DECL_EXTERNAL (current_function_decl)
-	   && VAR_OR_FUNCTION_DECL_P (ref)
-	   && (!VAR_P (ref) || TREE_STATIC (ref))
-	   && ! TREE_PUBLIC (ref)
 	   && DECL_CONTEXT (ref) != current_function_decl)
-    record_inline_static (loc, current_function_decl, ref,
-			  csi_internal);
-
+	  /* If the function declaration is not available yet at this
+	     point make a record of the reference if it isn't associated
+	     with any context yet and either fill in the details or
+	     discard the record when the declaration has been completed.  */
+	  || (!current_function_decl
+	      && !DECL_CONTEXT (ref)))
+	record_inline_static (loc, current_function_decl, ref,
+			      csi_internal);
+    }
   return ref;
 }
 
diff --git a/gcc/testsuite/gcc.dg/inline-42.c b/gcc/testsuite/gcc.dg/inline-42.c
new file mode 100644
index 00000000000..1e61d820895
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/inline-42.c
@@ -0,0 +1,156 @@
+/* PR c/88718 - Strange inconsistency between old style and new style
+   definitions of inline functions
+   { dg-do compile }
+   { dg-options "-Wall -Wno-unused" } */
+
+extern int global;
+extern const int cst_global;
+
+static int local;
+static const int cst_local;
+
+/* Verify that inline declarations that aren't definitions are accepted
+   without a warning even if their argument lists reference static
+   variables.  */
+
+inline void global_decl_arg_ref_global (int (*)[sizeof global]);
+static inline void local_decl_arg_ref_global (int (*)[sizeof global]);
+
+inline void global_decl_arg_ref_cst_global (int (*)[sizeof cst_global]);
+static inline void local_decl_arg_ref_cst_global (int (*)[sizeof cst_global]);
+
+inline void global_decl_arg_ref_local (int (*)[sizeof local]);
+static inline void local_decl_arg_ref_local (int (*)[sizeof local]);
+
+inline void global_decl_arg_ref_cst_local (int (*)[sizeof cst_local]);
+static inline void local_decl_arg_ref_cst_local (int (*)[sizeof cst_local]);
+
+/* Verify that implicitly extern inline definitions trigger a warning
+   when their argument lists reference static (but not extern) variables.  */
+
+inline void
+global_decl_arg_ref_global (int (*p)[sizeof global]) { (void)&p; }
+
+inline void
+global_decl_arg_ref_cst_global (int (*p)[sizeof cst_global]) { (void)&p; }
+
+inline void
+global_decl_arg_ref_local (int (*p)[sizeof local])
+/* { dg-warning ".local. is static but used in inline function .global_decl_arg_ref_local." "" { target *-*-* } .-1 } */
+{
+  (void)&p;
+}
+
+inline void
+global_decl_arg_ref_cst_local (int (*p)[sizeof cst_local])
+/* { dg-warning ".cst_local. is static but used in inline function .global_decl_arg_ref_cst_local." "" { target *-*-* } .-1 } */
+{
+  (void)&p;
+}
+
+/* Verify that static inline definitions don't trigger a warning
+   when their argument lists reference static or extern variables.  */
+
+static inline void
+local_decl_arg_ref_global (int (*p)[sizeof global]) { (void)&p; }
+static inline void
+local_decl_arg_ref_cst_global (int (*p)[sizeof cst_global]) { (void)&p; }
+static inline void
+local_decl_arg_ref_local (int (*p)[sizeof local]) { (void)&p; }
+static inline void
+local_decl_arg_ref_cst_local (int (*p)[sizeof cst_local]) { (void)&p; }
+
+
+/* Verify that a function declaration that references a static that
+   follows a definition of the same function that doesn't reference
+   any variables is not diagnosed.  */
+
+inline void
+global_def_arg_ref_none (int (*p)[sizeof (int)]) { (void)&p; }
+
+static int prior_local;
+
+inline void
+global_def_arg_ref_none (int (*p)[sizeof prior_local]);
+
+
+/* Verify that a function declaration that references a static that
+   follows a definition of the same function that refernces a global
+   is not diagnosed.  */
+
+inline void
+global_def_arg_ref_cst_global (int (*p)[sizeof cst_global]) { (void)&p; }
+
+inline void
+global_def_arg_ref_cst_global (int (*p)[sizeof cst_local]);
+
+
+/* Same as above but with return types.  */
+
+extern void abort (void);
+
+/* Verify that inline declarations that aren't definitions are accepted
+   without a warning even if their return types reference static
+   variables.  */
+
+inline struct { int a[sizeof global]; }
+  global_decl_ret_ref_global (void);
+
+static inline struct { int a[sizeof global]; }
+  local_decl_ret_ref_global (void);
+
+inline struct { int a[sizeof cst_global]; }
+  global_decl_ret_ref_cst_global (void);
+
+static inline struct { int a[sizeof cst_global]; }
+  local_decl_ret_ref_cst_global (void);
+
+inline struct { int a[sizeof local]; }
+  global_decl_ret_ref_local (void);
+static inline struct { int a[sizeof local]; }
+  local_decl_ret_ref_local (void);
+
+inline struct { int a[sizeof cst_local]; }
+  global_decl_ret_ref_cst_local (void);
+
+static inline struct { int a[sizeof global]; }
+  local_decl_ret_ref_cst_local (void);
+
+
+/* Verify that implicitly extern inline definitions trigger a warning
+   when their return types reference static (but not extern) variables.  */
+
+inline struct { int a[sizeof global]; }
+  global_def_ret_ref_global (void) { abort (); }
+
+inline struct { int a[sizeof cst_global]; }
+  global_def_ret_ref_cst_global (void) { abort (); }
+
+inline struct { int a[sizeof local]; }
+  global_def_ret_ref_local (void)
+  /* { dg-warning ".local. is static but used in inline function .global_def_ret_ref_local." "" { target *-*-* } .-2 } */
+  {
+    abort ();
+  }
+
+inline struct { int a[sizeof cst_local]; }
+  global_def_ret_ref_cst_local (void)
+  /* { dg-warning ".cst_local. is static but used in inline function .global_def_ret_ref_cst_local." "" { target *-*-* } .-2 } */
+  {
+    abort ();
+  }
+
+/* Verify that static inline definitions don't trigger a warning
+   when their return types reference static or extern variables.  */
+
+static inline struct { int a[sizeof global]; }
+  local_def_ret_ref_global (void) { abort (); }
+static inline struct { int a[sizeof cst_global]; }
+  local_def_ret_ref_cst_global (void) { abort (); }
+static inline struct { int a[sizeof local]; }
+  local_def_ret_ref_local (void) { abort (); }
+static inline struct { int a[sizeof cst_local]; }
+  local_def_ret_ref_cst_local (void) { abort (); }
+
+
+/* { dg-prune-output "declared but never defined" } */
diff --git a/gcc/testsuite/gcc.dg/inline-43.c b/gcc/testsuite/gcc.dg/inline-43.c
new file mode 100644
index 00000000000..47709959fcf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/inline-43.c
@@ -0,0 +1,59 @@
+/* PR c/88718 - Strange inconsistency between old style and new style
+   definitions of inline functions
+   Verify that warnings for references to multiple static variables in
+   inline function signatures are all diagnosed and that the locations
+   of the diagnostics, including the notes, are correct.
+   { dg-do compile }
+   { dg-options "-Wall -Wno-unused" } */
+
+extern const int e1;
+extern const int e2;
+
+static const int s1 = 1;   /* { dg-message ".s1. declared here" } */
+static const int s2 = 2;   /* { dg-message ".s2. declared here" } */
+
+/* Verify that variable declarations don't cause bogus warnings
+   in subsequent inline function declarations.  */
+int extarr[sizeof e1][sizeof e2][sizeof s1][sizeof s2];
+static int locarr[sizeof e1][sizeof e2][sizeof s1][sizeof s2];
+inline void fv (void) { }
+
+inline void fes (int (*)[sizeof e1][sizeof s2]);
+
+
+inline void
+fes (int (*p)
+     [sizeof e1]
+     [sizeof s2])   /* { dg-warning ".s2. is static but used in inline function .fes. which is not static" } */
+{ }
+
+
+inline void fse (int (*)[sizeof s1][sizeof e2]);
+
+inline void
+fse (int (*p)
+     [sizeof s1]    /* { dg-warning ".s1. is static but used in inline function .fse. which is not static" } */
+     [sizeof e2])
+{ }
+
+
+static int s3 = 3;   /* { dg-message ".s3. declared here" } */
+static int s4 = 4;   /* { dg-message ".s4. declared here" } */
+
+/* Use s1 and s2 here and verify that the warnings mention s3 and s4
+   referenced in the definition.  */
+inline void fss (int (*)[sizeof s1][sizeof s2]);
+
+inline void
+fss (int (*p)
+     [sizeof s3]    /* { dg-warning ".s3. is static but used in inline function .fss. which is not static" } */
+     [sizeof s4])   /* { dg-warning ".s4. is static but used in inline function .fss. which is not static" } */
+{ }
+
+
+/* Use s1 and s2 in the declaration and verify that no warnings are
+   emitted for the definition that doesn't reference any statics.  */
+inline void fee (int (*)[sizeof s1][sizeof s2]);
+
+inline void
+fee (int (*p)[sizeof e1][sizeof e2]) { }

On Wed, 16 Jan 2019, Martin Sebor wrote:

> +  /* Iterate over tentative records (all those at the head of the list
> +     with a null FUNCTION) and either associate them with DECL when ADD
> +     is set or remove them from it otherwise.  */
> +  for (c_inline_static *csi = c_inline_statics, *last = csi;
> +       csi; csi = csi->next)

(Here we start with last == c_inline_statics.)

> +      if (add)
> +	{
> +	  if (csi->static_decl)
> +	    csi->function = decl;
> +	}

I don't see any way in which csi->static_decl can be NULL (both callers of 
record_inline_static appear only to use non-NULL values for that argument, 
and it being non-NULL seems implicit in the semantics of the function 
given by the comment above it).  That is, I don't see the use of the inner 
"if" here.

> +	  if (last == c_inline_statics)
> +	    c_inline_statics = last = csi->next;
> +	  else
> +	    last->next = csi->next;

Here, if either last or c_inline_statics changes, they remain the same, so 
I don't see how they can ever be different.

So I don't see the need for having the variable last at all, or the 
conditional "if (last == c_inline_statics)".  It's always the case that, 
if removing entries from the list, it's some number of leading entries, so 
I'd expect just "c_inline_statics = csi->next;" to be sufficient for 
removing an entry.

> +      /* FUNCTION is unset for a declaration whose signature references
> +	 a static variable and for which a definition wasn't provided.  */
> +      if (!csi->function)
> +	continue;

But those should have been removed after the declaration in all cases via 
a call to update_tentative_inline_static.  So why is this conditional 
needed here?

> +      /* Only extern functions are of interest.  STATIC_DECL is null
> +	 for inline functions that have been defined that contain
> +	 no references to statics (but whose subsequent declarations
> +	 might).  */
> +      if (!DECL_EXTERNAL (csi->function)
> +	  || !csi->static_decl)
> +	continue;

As above, I don't see the need for the !csi->static_decl check, as 
csi->static_decl should never be NULL.

On 1/11/19 1:10 PM, Martin Sebor wrote:
> The attached patch extends the detection of references to static
> variables in inline functions to the function signatures, including
> their return type.  Since the declaration of a function need not be
> yet available at the time the static is referenced (e.g., when it's
> referenced in the functions return type), the patch introduces
> the concept of "tentative records" of such references and either
> completes the records when the full declaration of the function,
> including its definition, is known to be inline, or removes it
> when it isn't.
> 
> Martin
> 
> gcc-88718.diff
> 
> PR c/88718 - Strange inconsistency between old style and new style definitions of inline functions.
> 
> gcc/c/ChangeLog:
> 
> 	PR c/88718
> 	* c-decl.c (reset_inline_statics): New function.
> 	(record_inline_static): Optimize.
> 	(check_inline_statics): Handle tentative records for inline
> 	declarations without definitions.
> 	Print static declaration location.
> 	(push_file_scope): Clear records of references to statics.
> 	(finish_decl): Add tentative records of references to statics.
> 	(finish_function): Same.
> 	* c-typeck.c (build_external_ref): Handle all references to statics.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/88718
> 	* gcc.dg/inline-40.c: New test.
> 	* gcc.dg/inline-41.c: New test.
> 
> Index: gcc/c/c-decl.c
> ===================================================================
> --- gcc/c/c-decl.c	(revision 267616)
> +++ gcc/c/c-decl.c	(working copy)
> @@ -826,14 +826,47 @@ c_finish_incomplete_decl (tree decl)
>      }
>  }
>  
> -/* Record that inline function FUNC contains a reference (location
> -   LOC) to static DECL (file-scope or function-local according to
> -   TYPE).  */
> +/* Free records of references to static variables gathered so far.  */
>  
> +static void
> +reset_inline_statics (void)
> +{
> +  if (!c_inline_statics)
> +    return;
> +
> +  for (c_inline_static *csi = c_inline_statics, *next = csi->next;
> +       csi; csi = next)
> +    ggc_free (csi);
> +
> +  c_inline_statics = NULL;
> +}
> +
> +/* Record that inline function FUNC either does contain or may contain
> +   a reference (location LOC) to static DECL (file-scope or function-local
> +   according to TYPE).  For a null FUNC, a tentative record is created that
> +   reflects a reference in the function signature and that is either updated
> +   or removed when the function declaration is complete.  */
> +
>  void
>  record_inline_static (location_t loc, tree func, tree decl,
>  		      enum c_inline_static_type type)
>  {
> +  gcc_assert (decl);
> +
> +  if (c_inline_statics)
> +    {
> +      /* Avoid adding another tentative record for this DECL if one
> +	 already exists.  */
> +      for (c_inline_static *csi = c_inline_statics; csi; csi = csi->next)
> +	{
> +	  if (c_inline_statics->function)
> +	    break;
> +
> +	  if (decl == c_inline_statics->static_decl)
> +	    return;
> +	}
> +    }
> +
I'm going to assume this isn't called enough for the linear search to
matter from a compile-time standpoint.

OK for the trunk.

Jeff

On Fri, May 31, 2019 at 9:48 PM Jeff Law <law@redhat.com> wrote:
>
> On 1/11/19 1:10 PM, Martin Sebor wrote:
> > The attached patch extends the detection of references to static
> > variables in inline functions to the function signatures, including
> > their return type.  Since the declaration of a function need not be
> > yet available at the time the static is referenced (e.g., when it's
> > referenced in the functions return type), the patch introduces
> > the concept of "tentative records" of such references and either
> > completes the records when the full declaration of the function,
> > including its definition, is known to be inline, or removes it
> > when it isn't.
> >
> > Martin
> >
> > gcc-88718.diff
> >
> > PR c/88718 - Strange inconsistency between old style and new style definitions of inline functions.
> >
> > gcc/c/ChangeLog:
> >
> >       PR c/88718
> >       * c-decl.c (reset_inline_statics): New function.
> >       (record_inline_static): Optimize.
> >       (check_inline_statics): Handle tentative records for inline
> >       declarations without definitions.
> >       Print static declaration location.
> >       (push_file_scope): Clear records of references to statics.
> >       (finish_decl): Add tentative records of references to statics.
> >       (finish_function): Same.
> >       * c-typeck.c (build_external_ref): Handle all references to statics.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR c/88718
> >       * gcc.dg/inline-40.c: New test.
> >       * gcc.dg/inline-41.c: New test.
> >
> > Index: gcc/c/c-decl.c
> > ===================================================================
> > --- gcc/c/c-decl.c    (revision 267616)
> > +++ gcc/c/c-decl.c    (working copy)
> > @@ -826,14 +826,47 @@ c_finish_incomplete_decl (tree decl)
> >      }
> >  }
> >
> > -/* Record that inline function FUNC contains a reference (location
> > -   LOC) to static DECL (file-scope or function-local according to
> > -   TYPE).  */
> > +/* Free records of references to static variables gathered so far.  */
> >
> > +static void
> > +reset_inline_statics (void)
> > +{
> > +  if (!c_inline_statics)
> > +    return;
> > +
> > +  for (c_inline_static *csi = c_inline_statics, *next = csi->next;
> > +       csi; csi = next)
> > +    ggc_free (csi);
> > +
> > +  c_inline_statics = NULL;
> > +}
> > +
> > +/* Record that inline function FUNC either does contain or may contain
> > +   a reference (location LOC) to static DECL (file-scope or function-local
> > +   according to TYPE).  For a null FUNC, a tentative record is created that
> > +   reflects a reference in the function signature and that is either updated
> > +   or removed when the function declaration is complete.  */
> > +
> >  void
> >  record_inline_static (location_t loc, tree func, tree decl,
> >                     enum c_inline_static_type type)
> >  {
> > +  gcc_assert (decl);
> > +
> > +  if (c_inline_statics)
> > +    {
> > +      /* Avoid adding another tentative record for this DECL if one
> > +      already exists.  */
> > +      for (c_inline_static *csi = c_inline_statics; csi; csi = csi->next)
> > +     {
> > +       if (c_inline_statics->function)
> > +         break;
> > +
> > +       if (decl == c_inline_statics->static_decl)
> > +         return;
> > +     }
> > +    }
> > +
> I'm going to assume this isn't called enough for the linear search to
> matter from a compile-time standpoint.

Err - but there must be a better way, no?  It's trivial to write a testcase
running into quadratic behavior here.

Richard.

>
> OK for the trunk.
>
> Jeff

I already reviewed two versions of this patch.  As far as I know, no third 
version has been posted to address my comments on the second version 
<https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00970.html>.

I posted the patch right on the cusp of stage 4, thinking it was
still stage 3.  I thought I could finish it quickly but after
the last round of comments I decided to move on to stage 4 stuff.
I've contemplating coming back to it at some point.

That being said, the patch fixes the reported bug with no issues.
While the comments may be valid, as often happens, by insisting
on perfection we let the perfect be the enemy of the good enough:
the correctness bug remains unresolved and the effort that went
into the fix may have been for naught.

Martin

On 6/3/19 9:20 AM, Joseph Myers wrote:
> I already reviewed two versions of this patch.  As far as I know, no third
> version has been posted to address my comments on the second version
> <https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00970.html>.
>