On Tue, Jul 13, 2021 at 9:27 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> An existing, previously xfailed test that I recently removed
> the xfail from made me realize that -Wstringop-overflow doesn't
> properly detect buffer overflow resulting from vectorized stores.
> Because of a difference in the IL the test passes on x86_64 but
> fails on targets like aarch64. Other examples can be constructed
> that -Wstringop-overflow fails to diagnose even on x86_64. For
> INSTANCE, the overflow in the following function isn't diagnosed
> when the loop is vectorized:
>
> void* f (void)
> {
> char *p = __builtin_malloc (8);
> for (int i = 0; i != 16; ++i)
> p[i] = 1 << i;
> return p;
> }
>
> The attached change enhances the warning to detect those as well.
> It found a few bugs in vectorizer tests that the patch corrects.
> Tested on x86_64-linux and with an aarch64 cross.
- dest = gimple_call_arg (stmt, 0);
+ if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
+ && gimple_call_num_args (stmt))
+ dest = gimple_call_arg (stmt, 0);
+ else
+ dest = gimple_call_lhs (stmt);
+
+ if (!dest)
+ return;
so this uses arg0 for memcpy (dst, src, 4) and also for bcopy (src, dst, 4)?
It looks quite fragile to me. I think you want to use the LHS only if it is
aggregate (and not a pointer or some random other value). Likewise
you should only use arg0 for a whitelist of builtins, not for any random one.
It's bad enough that compute_objsize decides for itself whether it is
passed a pointer or an object rather than the API being explicit about this.
if (VAR_P (exp) || TREE_CODE (exp) == CONST_DECL)
{
- exp = ctor_for_folding (exp);
- if (!exp)
- return false;
+ /* If EXP can be folded into a constant use the result. Otherwise
+ proceed to use EXP to determine a range of the result. */
+ if (tree fold_exp = ctor_for_folding (exp))
+ if (fold_exp != error_mark_node)
+ exp = fold_exp;
fold_exp can be NULL, meaning a zero-initializer but below you'll run into
const char *prep = NULL;
if (TREE_CODE (exp) == STRING_CST)
{
and crash. Either you handle a NULL fold_expr explicitely or conservatively
continue to return false.
+ /* The LHS and RHS of the store. The RHS is null if STMT is a function
+ call. RHSTYPE is the type of the store. */
+ tree lhs, rhs, rhstype;
+ if (is_gimple_assign (stmt))
+ {
+ lhs = gimple_assign_lhs (stmt);
+ rhs = gimple_assign_rhs1 (stmt);
+ rhstype = TREE_TYPE (rhs);
+ }
+ else if (is_gimple_call (stmt))
+ {
+ lhs = gimple_call_lhs (stmt);
+ rhs = NULL_TREE;
+ rhstype = TREE_TYPE (gimple_call_fntype (stmt));
+ }
The type of the store in a call is better determined from the LHS.
For internal function calls the above will crash.
Otherwise looks like reasonable changes.
Richard.
> Martin
