Re: [PATCH 2/8] tree-dynamic-object-size: New pass

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Oct 08, 2021 at 03:44:26AM +0530, Siddhesh Poyarekar wrote:
> A new pass is added to execute just before the tree-object-size pass
> to recognize and simplify __builtin_dynamic_object_size.  Some key
> ideas (such as multipass object size collection to detect reference
> loops) have been taken from tree-object-size but is distinct from it
> to ensure minimal impact on existing code.
> 
> At the moment, the pass only recognizes allocators and passthrough
> functions to attempt to derive object size expressions, and replaces
> the call site with those expressions.  On failure, it replaces the
> __builtin_dynamic_object_size with __builtin_object_size as a
> fallback.

Not full review, just nits for now:
I don't really like using separate passes for this, whether
it should be done in the same source or different source file
is something less clear, I guess it depends on how much from
tree-object-size.[ch] can be reused, how much could be e.g. done
by pretending the 0-3 operands of __builtin_dynamic_object_size
are actually 4-7 and have [8] arrays in tree-object-size.[ch]
to track that and how much is separate.
I think the common case is either that a function won't
contain any of the builtin calls (most likely on non-glibc targets,
but even on glibc targets for most of the code that doesn't use any
of the fortified APIs), or it uses just one of them and not both
(e.g. glibc -D_FORTIFY_SOURCE={1,2} vs. -D_FORTIFY_SOURCE=3),
so either one or the other pass would just uselessly walk the whole
IL.
The objsz passes are walk over the whole IL, if they see
__bos calls, do something and call something in the end
and your pass is similar, so hooking it into the current pass
is trivial, just if
if (!gimple_call_builtin_p (call, BUILT_IN_OBJECT_SIZE))
before doing continue; check for the other builtin and call
a function to handle it, either from the same or different file,
and then at the end destruct what is needed too.
Especially on huge functions, each IL traversal may need to page into
caches (and out of them) lots of statements...

> 	* Makefile.in (OBJS): Add tree-dynamic-object-size.o.
> 	(PLUGIN_HEADERS): Add tree-dynamic-object-size.h.
> 	* tree-dynamic-object-size.c: New file.
> 	* tree-dynamic-object-size.h: New file.
> 	* builtins.c: Use it.
> 	(fold_builtin_dyn_object_size): Call
> 	compute_builtin_dyn_object_size for
> 	__builtin_dynamic_object_size builtin.
> 	(passes.def): Add pass_dynamic_object_sizes.
> 	* tree-pass.h: Add ake_pass_dynamic_object_sizes.

Missing m in ake

> +  if (TREE_CODE (ptr) == SSA_NAME
> +      && compute_builtin_dyn_object_size (ptr, object_size_type, &bytes))

Please don't abbreviate.

> +  object_sizes[osi->object_size_type][SSA_NAME_VERSION (dest)] =
> +    object_sizes[osi->object_size_type][SSA_NAME_VERSION (orig)];

GCC coding convention don't want to see the = at the end of line, it should
be at the start of the next line after indentation.

> +/* Initialize data structures for the object size computation.  */
> +
> +void
> +init_dynamic_object_sizes (void)
> +{
> +  int object_size_type;
> +
> +  if (computed[0])
> +    return;
> +
> +  for (object_size_type = 0; object_size_type <= 3; object_size_type++)
> +    {
> +      object_sizes[object_size_type].safe_grow (num_ssa_names, true);
> +      computed[object_size_type] = BITMAP_ALLOC (NULL);
> +    }
> +}
> +
> +
> +unsigned int
> +dynamic_object_sizes_execute (function *fun, bool lower_to_bos)
> +{
> +  basic_block bb;
> +
> +  init_dynamic_object_sizes ();
> +

I'd prefer if the initialization could be done only lazily if
it sees at least one such call like the objsz pass does.  That is why
there is the if (computed[0]) return; at the start...

	Jakub