calls.c: refactor special_function_p for use by analyzer (v2)

Message ID 20200131013225.28335-1-dmalcolm@redhat.com
State New
Headers show
Series
  • calls.c: refactor special_function_p for use by analyzer (v2)
Related show

Commit Message

David Malcolm Jan. 31, 2020, 1:32 a.m.
On Tue, 2020-01-28 at 08:36 +0100, Jakub Jelinek wrote:
> On Mon, Jan 27, 2020 at 09:09:37PM -0500, David Malcolm wrote:

> > > Please see calls.c (special_function_p), you should treat

> > > certainly

> > > also sigsetjmp as a setjmp call, and similarly to

> > > special_function_p,

> > > skip over _ or __ prefixes before the setjmp or sigsetjmp name.

> > > Similarly for longjmp/siglongjmp.

> > 

> > This patch refactors some code in special_function_p that checks

> > for

> > the function being sane to match by name, splitting it out into a

> > new

> > maybe_special_function_p, and using it it two places in the

> > analyzer.

> > 

> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;

> > OK for master?

> 

> Not sure it is worth it to factor out the

>       DECL_NAME (fndecl)

>       && (DECL_CONTEXT (fndecl) == NULL_TREE

>           || TREE_CODE (DECL_CONTEXT (fndecl)) ==

> TRANSLATION_UNIT_DECL)

>       && TREE_PUBLIC (fndecl)

> check, that seems like simple enough that it could be duplicated.

> And, even if there is a strong reason not to, at least it ought to be

> defined inline in the header, not everyone will use LTO and without

> LTO it

> will need to be an out of line call.


What's motivating this is that I found a second place in the analyzer
that should be doing this check [1], which means it's needed in three
places in the code (calls.c, and the two places in the analyzer).

Here's an updated version of the patch which puts it inline in the
header.  It does change the ordering of the checks in calls.c, in
that the status quo has a very early reject on
   IDENTIFIER_LENGTH (name_decl) <= 11
which seems like a micro-optimization for calls.c that would break
things for the general case.  I don't know how significant moving that
later is.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for master?  Or, given it's stage 4, should I simply add the new
helper function to the analyzer and leave a copy in calls.c, as a
way of fixing the analyzer bug w/o touching calls.c?

Thanks
Dave

[1] in function_set which is used e.g. for capturing
"all known async-signal-unsafe functions".

> Ack on removing the fndecl && check from special_function_p, the

> callers

> ensure it is non-NULL already, and even if they didn't, after the if

> (fndecl

> && ...) guarded if there is unconditional dereferencing of fndecl.

> 

> 	Jakub



This patch refactors some code in special_function_p that checks for
the function being sane to match by name, splitting it out into a new
maybe_special_function_p, and using it it two places in the analyzer.

gcc/analyzer/ChangeLog:
	* analyzer.cc (is_named_call_p): Replace tests for fndecl being
	extern at file scope and having a non-NULL DECL_NAME with a call
	to maybe_special_function_p.
	* function-set.cc (function_set::contains_decl_p): Add call to
	maybe_special_function_p.

gcc/ChangeLog:
	* calls.c (special_function_p): Split out the check for DECL_NAME
	being non-NULL and fndecl being extern at file scope into a
	new maybe_special_function_p and call it.  Drop check for fndecl
	being non-NULL that was after a usage of DECL_NAME (fndecl).
	* tree.h (maybe_special_function_p): New inline function.
---
 gcc/analyzer/analyzer.cc     | 10 +---------
 gcc/analyzer/function-set.cc |  2 ++
 gcc/calls.c                  | 14 ++------------
 gcc/tree.h                   | 25 +++++++++++++++++++++++++
 4 files changed, 30 insertions(+), 21 deletions(-)

-- 
2.21.0

Comments

Jakub Jelinek Jan. 31, 2020, 9:31 a.m. | #1
On Thu, Jan 30, 2020 at 08:32:25PM -0500, David Malcolm wrote:
> gcc/analyzer/ChangeLog:

> 	* analyzer.cc (is_named_call_p): Replace tests for fndecl being

> 	extern at file scope and having a non-NULL DECL_NAME with a call

> 	to maybe_special_function_p.

> 	* function-set.cc (function_set::contains_decl_p): Add call to

> 	maybe_special_function_p.

> 

> gcc/ChangeLog:

> 	* calls.c (special_function_p): Split out the check for DECL_NAME

> 	being non-NULL and fndecl being extern at file scope into a

> 	new maybe_special_function_p and call it.  Drop check for fndecl

> 	being non-NULL that was after a usage of DECL_NAME (fndecl).

> 	* tree.h (maybe_special_function_p): New inline function.


LGTM.

	Jakub

Patch

diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc
index 1b5e4c9ecf8..5cf745ea632 100644
--- a/gcc/analyzer/analyzer.cc
+++ b/gcc/analyzer/analyzer.cc
@@ -65,18 +65,10 @@  is_named_call_p (tree fndecl, const char *funcname)
   gcc_assert (fndecl);
   gcc_assert (funcname);
 
-  /* Exclude functions not at the file scope, or not `extern',
-     since they are not the magic functions we would otherwise
-     think they are.  */
-  if (!((DECL_CONTEXT (fndecl) == NULL_TREE
-	 || TREE_CODE (DECL_CONTEXT (fndecl)) == TRANSLATION_UNIT_DECL)
-	&& TREE_PUBLIC (fndecl)))
+  if (!maybe_special_function_p (fndecl))
     return false;
 
   tree identifier = DECL_NAME (fndecl);
-  if (identifier == NULL)
-    return false;
-
   const char *name = IDENTIFIER_POINTER (identifier);
   const char *tname = name;
 
diff --git a/gcc/analyzer/function-set.cc b/gcc/analyzer/function-set.cc
index 6ed15ae95ad..1b6b5d9f9c1 100644
--- a/gcc/analyzer/function-set.cc
+++ b/gcc/analyzer/function-set.cc
@@ -59,6 +59,8 @@  bool
 function_set::contains_decl_p (tree fndecl) const
 {
   gcc_assert (fndecl && DECL_P (fndecl));
+  if (!maybe_special_function_p (fndecl))
+    return false;
   return contains_name_p (IDENTIFIER_POINTER (DECL_NAME (fndecl)));
 }
 
diff --git a/gcc/calls.c b/gcc/calls.c
index 1336f49ea5e..d1c53171176 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -586,18 +586,8 @@  special_function_p (const_tree fndecl, int flags)
 {
   tree name_decl = DECL_NAME (fndecl);
 
-  if (fndecl && name_decl
-      && IDENTIFIER_LENGTH (name_decl) <= 11
-      /* Exclude functions not at the file scope, or not `extern',
-	 since they are not the magic functions we would otherwise
-	 think they are.
-	 FIXME: this should be handled with attributes, not with this
-	 hacky imitation of DECL_ASSEMBLER_NAME.  It's (also) wrong
-	 because you can declare fork() inside a function if you
-	 wish.  */
-      && (DECL_CONTEXT (fndecl) == NULL_TREE
-	  || TREE_CODE (DECL_CONTEXT (fndecl)) == TRANSLATION_UNIT_DECL)
-      && TREE_PUBLIC (fndecl))
+  if (maybe_special_function_p (fndecl)
+      && IDENTIFIER_LENGTH (name_decl) <= 11)
     {
       const char *name = IDENTIFIER_POINTER (name_decl);
       const char *tname = name;
diff --git a/gcc/tree.h b/gcc/tree.h
index 93422206b63..85ce6b3bd6a 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5611,6 +5611,31 @@  builtin_decl_declared_p (enum built_in_function fncode)
 	  && builtin_info[uns_fncode].declared_p);
 }
 
+/* Determine if the function identified by FNDECL is one that
+   makes sense to match by name, for those places where we detect
+   "magic" functions by name.
+
+   Return true if FNDECL has a name and is an extern fndecl at file scope.
+   FNDECL must be a non-NULL decl.
+
+   Avoid using this, as it's generally better to use attributes rather
+   than to check for functions by name.  */
+
+static inline bool
+maybe_special_function_p (const_tree fndecl)
+{
+  tree name_decl = DECL_NAME (fndecl);
+  if (name_decl
+      /* Exclude functions not at the file scope, or not `extern',
+	 since they are not the magic functions we would otherwise
+	 think they are.  */
+      && (DECL_CONTEXT (fndecl) == NULL_TREE
+	  || TREE_CODE (DECL_CONTEXT (fndecl)) == TRANSLATION_UNIT_DECL)
+      && TREE_PUBLIC (fndecl))
+    return true;
+  return false;
+}
+
 /* Return true if T (assumed to be a DECL) is a global variable.
    A variable is considered global if its storage is not automatic.  */