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(-)
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. */
--
2.21.0
