Re: [PATCH] tree-object-size: Support strndup and strdup

[Siddhesh Poyarekar]

On 2022-09-22 09:02, Jakub Jelinek wrote:
On Mon, Aug 15, 2022 at 03:23:11PM -0400, Siddhesh Poyarekar wrote:
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -495,6 +495,18 @@ decl_init_size (tree decl, bool min)
    return size;
  }
  
+/* Get the outermost object that PTR may point into.  */
+
+static tree
+get_whole_object (const_tree ptr)
+{
+  tree pt_var = TREE_OPERAND (ptr, 0);
+  while (handled_component_p (pt_var))
+    pt_var = TREE_OPERAND (pt_var, 0);
+
+  return pt_var;
+}

Not sure why you want a new function for this.
This is essentially get_base_address (TREE_OPERAND (ptr, 0)).

Oh, so can addr_object_size be simplified to use get_base_address too?

  /* Compute __builtin_object_size for PTR, which is a ADDR_EXPR.
     OBJECT_SIZE_TYPE is the second argument from __builtin_object_size.
     If unknown, return size_unknown (object_size_type).  */
+  if (!size_valid_p (sz, object_size_type)
+       || size_unknown_p (sz, object_size_type))
+    {
+      tree wholesrc = NULL_TREE;
+      if (TREE_CODE (src) == ADDR_EXPR)
+       wholesrc = get_whole_object (src);
+
+      if (!(object_size_type & OST_MINIMUM)
+         || (wholesrc && TREE_CODE (wholesrc) == STRING_CST))

Is this safe?  I mean get_whole_object will also skip ARRAY_REFs with
variable indexes etc. and the STRING_CST could have embedded '\0's
in it.
Even if c_strlen (src, 1) is constant, I don't see what you can assume
for object size of strndup ("abcd\0efgh", n); for minimum, except 1.

Can't we assume MIN(5, n) for STRING_CST?

For ARRAY_REFs, it may end up being MIN(array_size, n) and not account 
for the NUL termination but I was thinking of that as being a better 
option than bailing out.  Should we try harder here and return, e.g. 
strlen or some equivalent?

But on the other side, 1 is a safe minimum for OST_MINIMUM of both
strdup and strndup if you don't find anything more specific (exact strlen
for strndup) because the terminating '\0' will be always there.

OK, I can return size_one_node as the final return value for OST_MINIMUM 
if we don't find a suitable expression.

Other than that you'd need to consider INTEGER_CST second strndup argument
or ranges of the second argument etc.
E.g. maximum for OST_DYNAMIC could be for strndup (src, n)
MIN (__bdos (src, ?), n + 1).

Yeah, that's what I return in the end:

  return fold_build2 (MIN_EXPR, sizetype,
                     fold_build2 (PLUS_EXPR, sizetype, size_one_node,n),
                     sz);

where sz is __bdos(src)


@@ -2113,7 +2177,7 @@ const pass_data pass_data_object_sizes =
    PROP_objsz, /* properties_provided */
    0, /* properties_destroyed */
    0, /* todo_flags_start */
-  0, /* todo_flags_finish */
+  TODO_update_ssa_no_phi, /* todo_flags_finish */
  };
  
  class pass_object_sizes : public gimple_opt_pass
@@ -2153,7 +2217,7 @@ const pass_data pass_data_early_object_sizes =
    0, /* properties_provided */
    0, /* properties_destroyed */
    0, /* todo_flags_start */
-  0, /* todo_flags_finish */
+  TODO_update_ssa_no_phi, /* todo_flags_finish */
  };

This is quite expensive.  Do you really need full ssa update, or just
TODO_update_ssa_only_virtuals would be enough (is it for the missing
vuse on the strlen call if you emit it)?
In any case, would be better not to do that always, but only if you
really need it (emitted the strlen call somewhere; e.g. if __bdos is
never used, only __bos, it is certainly not needed), todo flags
can be both in todo_flags_finish and in return value from execute method.

Thanks, I'll find a cheaper way to do this.

Thanks,
Sid