On 5/30/19 9:13 AM, Jeff Law wrote:
On 5/30/19 8:52 AM, Martin Sebor wrote:
On 5/29/19 11:45 AM, Jeff Law wrote:
On 5/22/19 3:34 PM, Martin Sebor wrote:
-Wreturn-local-addr detects a subset of instances of returning
the address of a local object from a function but the warning
doesn't try to handle alloca or VLAs, or some non-trivial cases
of ordinary automatic variables[1].
The attached patch extends the implementation of the warning to
detect those. It still doesn't detect instances where the address
is the result of a built-in such strcpy[2].
Tested on x86_64-linux.
Martin
[1] For example, this is only diagnosed with the patch:
void* f (int i)
{
struct S { int a[2]; } s[2];
return &s->a[i];
}
[2] The following is not diagnosed even with the patch:
void sink (void*);
void* f (int i)
{
char a[6];
char *p = __builtin_strcpy (a, "123");
sink (p);
return p;
}
I would expect detecting to be possible and useful. Maybe as
a follow-up.
gcc-71924.diff
PR middle-end/71924 - missing -Wreturn-local-addr returning alloca
result
PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an
address of a local array plus offset
gcc/ChangeLog:
PR c/71924
* gimple-ssa-isolate-paths.c (is_addr_local): New function.
(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
(find_implicit_erroneous_behavior): Call
warn_return_addr_local_phi_arg.
(find_explicit_erroneous_behavior): Call warn_return_addr_local.
gcc/testsuite/ChangeLog:
PR c/71924
* gcc.dg/Wreturn-local-addr-2.c: New test.
* gcc.dg/Walloca-4.c: Prune expected warnings.
* gcc.dg/pr41551.c: Same.
* gcc.dg/pr59523.c: Same.
* gcc.dg/tree-ssa/pr88775-2.c: Same.
* gcc.dg/winline-7.c: Same.
diff --git a/gcc/gimple-ssa-isolate-paths.c
b/gcc/gimple-ssa-isolate-paths.c
index 33fe352bb23..2933ecf502e 100644
--- a/gcc/gimple-ssa-isolate-paths.c
+++ b/gcc/gimple-ssa-isolate-paths.c
@@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple
*stmt)
return false;
}
+/* Return true if EXPR is a expression of pointer type that refers
+ to the address of a variable with automatic storage duration.
+ If so, set *PLOC to the location of the object or the call that
+ allocated it (for alloca and VLAs). When PMAYBE is non-null,
+ also consider PHI statements and set *PMAYBE when some but not
+ all arguments of such statements refer to local variables, and
+ to clear it otherwise. */
+
+static bool
+is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
+ hash_set<gphi *> *visited = NULL)
+{
+ if (TREE_CODE (exp) == ADDR_EXPR)
+ {
+ tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
+ if (TREE_CODE (baseaddr) == MEM_REF)
+ return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe,
visited);
+
+ if ((!VAR_P (baseaddr)
+ || is_global_var (baseaddr))
+ && TREE_CODE (baseaddr) != PARM_DECL)
+ return false;
+
+ *ploc = DECL_SOURCE_LOCATION (baseaddr);
+ return true;
+ }
+
+ if (TREE_CODE (exp) == SSA_NAME)
+ {
+ gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
+ enum gimple_code code = gimple_code (def_stmt);
+
+ if (is_gimple_assign (def_stmt))
+ {
+ tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
+ if (POINTER_TYPE_P (type))
+ {
+ tree ptr = gimple_assign_rhs1 (def_stmt);
+ return is_addr_local (ptr, ploc, pmaybe, visited);
+ }
+ return false;
+ }
+
+ if (code == GIMPLE_CALL
+ && gimple_call_builtin_p (def_stmt))
+ {
+ tree fn = gimple_call_fndecl (def_stmt);
+ int code = DECL_FUNCTION_CODE (fn);
+ if (code != BUILT_IN_ALLOCA
+ && code != BUILT_IN_ALLOCA_WITH_ALIGN)
+ return false;
+
+ *ploc = gimple_location (def_stmt);
+ return true;
+ }
+
+ if (code == GIMPLE_PHI && pmaybe)
+ {
+ unsigned count = 0;
+ gphi *phi_stmt = as_a <gphi *> (def_stmt);
+
+ unsigned nargs = gimple_phi_num_args (phi_stmt);
+ for (unsigned i = 0; i < nargs; ++i)
+ {
+ if (!visited->add (phi_stmt))
+ {
+ tree arg = gimple_phi_arg_def (phi_stmt, i);
+ if (is_addr_local (arg, ploc, pmaybe, visited))
+ ++count;
+ }
+ }
+
+ *pmaybe = count && count < nargs;
+ return count != 0;
+ }
+ }
+
+ return false;
+}
Is there some reason you didn't query the alias oracle here? It would
seem a fairly natural fit. Ultimately given a pointer (which will be an
SSA_NAME) you want to ask whether or not it conclusively points into the
stack.
That would seem to dramatically simplify is_addr_local.
I did think about it but decided against changing the existing
design (iterating over PHI arguments), for a couple of reasons:
1) It feels like a bigger change than my simple "bug fix" calls
for.
I suspect the net result will be simpler though ;-) I think you just
get a pt solution and iterate over the things the solution says the
pointer can point to.
2) I'm not familiar enough with the alias oracle to say for sure
if it can be used to give the same results as the existing
implementation. I.e., make it possible to identify and
isolate each path that returns a local address (rather than
just answering: yes, this pointer may point to some local
in this function).
Precision of the oracle is certainly the big question.
If the alias oracle can be used to give the same results without
excessive false positives then I think it would be fine to make
use of it. Is that something you consider a prerequisite for
this change or should I look into it as a followup?
I think we should explore it a bit before making a final decision. It
may guide us for other work in this space (like detecting escaping
locals). I think a dirty prototype to see if it's even in the right
ballpark would make sense.
Okay, let me look into it.
FWIW, I'm working on enhancing this to detect returning freed
pointers (under a different option). That seems like a good
opportunity to also look into making use of the alias oracle.
Yes. I think there's two interesting cases here to ponder. If we free
a pointer that must point to a local, then we can warn & trap. If we
free a pointer that may point to a local, then we can only warn (unless
we can isolate the path).
I wasn't actually thinking of freeing locals but it sounds like
a useful enhancement as well. Thanks for the suggestion! :)
To be clear, what I'm working on is detecting:
void* f (void *p)
{
free (p); // note: pointer was freed here
// ...
return p; // warning: returning a freed pointer
}
Besides these enhancements, there's also a request to diagnose
dereferencing pointers to compound literals whose lifetime has
ended (PR 89990), or more generally, those to any such local
object. It's about preventing essentially the same problem
(accessing bad data or corrupting others) but it seems that
both the analysis and the handling will be sufficiently
different to consider implementing it somewhere else. What
are your thoughts on that?
I think the tough problem here is we lose binding scopes as we lower
into gimple, so solving it in the general case may be tough. We've
started adding some clobbers into the IL to denote object death points,
but I'm not sure if they're sufficient to implement this kind of warning.
I was afraid that might be a problem.
Thanks
Martin
