Re: [PATCH v4 5/6] bpf,btf: enable BTF pruning by default for BPF

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

Hi Faust.
Thanks for the patch.
Please see a question below.

> This patch enables -gprune-btf by default in the BPF backend when
> generating BTF information, and fixes BPF CO-RE generation when using
> -gprune-btf.
>
> When generating BPF CO-RE information, we must ensure that types used
> in CO-RE relocations always have sufficient BTF information emited so
> that the CO-RE relocations can be processed by a BPF loader.  The BTF
> pruning algorithm on its own does not have sufficient information to
> determine which types are used in a BPF CO-RE relocation, so this
> information must be supplied by the BPF backend, using a new
> btf_mark_type_used function.
>
> Co-authored-by: Cupertino Miranda <cupertino.miranda@oracle.com>
>
> gcc/
> 	* btfout.cc (btf_mark_type_used): New.
> 	* ctfc.h (btf_mark_type_used): Declare it here.
> 	* config/bpf/bpf.cc (bpf_option_override): Enable -gprune-btf
> 	by default if -gbtf is enabled.
> 	* config/bpf/core-builtins.cc (extra_fn): New typedef.
> 	(compute_field_expr): Add callback parameter, and call it if supplied.
> 	Fix computation for MEM_REF.
> 	(mark_component_type_as_used): New.
> 	(bpf_mark_types_as_used): Likewise.
> 	(bpf_expand_core_builtin): Call here.
> 	* doc/invoke.texi (Debugging Options): Note that -gprune-btf is
> 	enabled by default for BPF target when generating BTF.
>
> gcc/testsuite/
> 	* gcc.dg/debug/btf/btf-variables-5.c: Adjust one test for bpf-*-*
> 	target.
> ---
>  gcc/btfout.cc                                 | 22 ++++++
>  gcc/config/bpf/bpf.cc                         |  5 ++
>  gcc/config/bpf/core-builtins.cc               | 71 +++++++++++++++++--
>  gcc/ctfc.h                                    |  1 +
>  gcc/doc/invoke.texi                           |  3 +
>  .../gcc.dg/debug/btf/btf-variables-5.c        |  6 +-
>  6 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index 34d8cec0a2e3..083ca48d6279 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -1503,6 +1503,28 @@ btf_assign_datasec_ids (ctf_container_ref ctfc)
>      }
>  }
>  
> +
> +/* Manually mark that type T is used to ensure it will not be pruned.
> +   Used by the BPF backend when generating BPF CO-RE to mark types used
> +   in CO-RE relocations.  */
> +
> +void
> +btf_mark_type_used (tree t)
> +{
> +  /* If we are not going to prune anyway, this is a no-op.  */
> +  if (!debug_prune_btf)
> +    return;
> +
> +  gcc_assert (TYPE_P (t));
> +  ctf_container_ref ctfc = ctf_get_tu_ctfc ();
> +  ctf_dtdef_ref dtd = ctf_lookup_tree_type (ctfc, t);
> +
> +  if (!dtd)
> +    return;
> +
> +  btf_add_used_type (ctfc, dtd, false, false, true);
> +}
> +
>  /* Callback used for assembling the only-used-types list.  Note that this is
>     the same as btf_type_list_cb above, but the hash_set traverse requires a
>     different function signature.  */
> diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
> index dd1bfe38d29b..c62af7a6efa7 100644
> --- a/gcc/config/bpf/bpf.cc
> +++ b/gcc/config/bpf/bpf.cc
> @@ -221,6 +221,11 @@ bpf_option_override (void)
>        && !(target_flags_explicit & MASK_BPF_CORE))
>      target_flags |= MASK_BPF_CORE;
>  
> +  /* -gbtf implies -gprune-btf for BPF target.  */
> +  if (btf_debuginfo_p ())
> +    SET_OPTION_IF_UNSET (&global_options, &global_options_set,
> +			 debug_prune_btf, true);
> +
>    /* Determine available features from ISA setting (-mcpu=).  */
>    if (bpf_has_jmpext == -1)
>      bpf_has_jmpext = (bpf_isa >= ISA_V2);
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index 232bebcadbd5..86e2e9d6e39f 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -624,13 +624,20 @@ bpf_core_get_index (const tree node, bool *valid)
>  
>     ALLOW_ENTRY_CAST is an input arguments and specifies if the function should
>     consider as valid expressions in which NODE entry is a cast expression (or
> -   tree code nop_expr).  */
> +   tree code nop_expr).
> +
> +   EXTRA_FN is a callback function to allow extra functionality with this
> +   function traversal.  Currently used for marking used type during expand
> +   pass.  */
> +
> +typedef void (*extra_fn) (tree);
>  
>  static unsigned char
>  compute_field_expr (tree node, unsigned int *accessors,
>  		    bool *valid,
>  		    tree *access_node,
> -		    bool allow_entry_cast = true)
> +		    bool allow_entry_cast = true,
> +		    extra_fn callback = NULL)
>  {
>    unsigned char n = 0;
>    unsigned int fake_accessors[MAX_NR_ACCESSORS];
> @@ -647,6 +654,9 @@ compute_field_expr (tree node, unsigned int *accessors,
>  
>    *access_node = node;
>  
> +  if (callback != NULL)
> +    callback (node);
> +
>    switch (TREE_CODE (node))
>      {
>      case INDIRECT_REF:
> @@ -664,17 +674,19 @@ compute_field_expr (tree node, unsigned int *accessors,
>      case COMPONENT_REF:
>        n = compute_field_expr (TREE_OPERAND (node, 0), accessors,
>  			      valid,
> -			      access_node, false);
> +			      access_node, false, callback);
>        accessors[n] = bpf_core_get_index (TREE_OPERAND (node, 1), valid);
>        return n + 1;
>      case ARRAY_REF:
>      case ARRAY_RANGE_REF:
> -    case MEM_REF:
>        n = compute_field_expr (TREE_OPERAND (node, 0), accessors,
>  			      valid,
> -			      access_node, false);
> +			      access_node, false, callback);
>        accessors[n++] = bpf_core_get_index (node, valid);
>        return n;
> +    case MEM_REF:
> +      accessors[0] = bpf_core_get_index (node, valid);
> +      return 1;
>      case NOP_EXPR:
>        if (allow_entry_cast == true)
>  	{
> @@ -683,7 +695,7 @@ compute_field_expr (tree node, unsigned int *accessors,
>  	}
>        n = compute_field_expr (TREE_OPERAND (node, 0), accessors,
>  			      valid,
> -			      access_node, false);
> +			      access_node, false, callback);
>        return n;
>  
>      case ADDR_EXPR:
> @@ -1549,6 +1561,51 @@ bpf_resolve_overloaded_core_builtin (location_t loc, tree fndecl,
>    return construct_builtin_core_reloc (loc, fndecl, args, argsvec->length ());
>  }
>  
> +/* Callback function for bpf_mark_field_expr_types_as_used.  */
> +
> +static void
> +mark_component_type_as_used (tree node)
> +{
> +  if (TREE_CODE (node) == COMPONENT_REF)
> +    btf_mark_type_used (TREE_TYPE (TREE_OPERAND (node, 0)));
> +}

This means that the callback is only marking as used these types
reachable from the CO-RE builtin arguments that are referenced by
indexation or field name or pointer?

> +
> +/* Mark types needed for BPF CO-RE relocations as used.  Doing so ensures that
> +   these types do not get pruned from the BTF information.  */
> +
> +static void
> +bpf_mark_types_as_used (struct cr_builtins *data)
> +{
> +  tree expr = data->expr;
> +  switch (data->kind)
> +    {
> +    case BPF_RELO_FIELD_BYTE_OFFSET:
> +    case BPF_RELO_FIELD_BYTE_SIZE:
> +    case BPF_RELO_FIELD_EXISTS:
> +    case BPF_RELO_FIELD_SIGNED:
> +    case BPF_RELO_FIELD_LSHIFT_U64:
> +    case BPF_RELO_FIELD_RSHIFT_U64:
> +      if (TREE_CODE (expr) == ADDR_EXPR)
> +	expr = TREE_OPERAND (expr, 0);
> +
> +      expr = root_for_core_field_info (expr);
> +      compute_field_expr (data->expr, NULL, NULL, NULL, false,
> +			  mark_component_type_as_used);
> +      break;
> +    case BPF_RELO_TYPE_ID_LOCAL:
> +    case BPF_RELO_TYPE_ID_TARGET:
> +    case BPF_RELO_TYPE_EXISTS:
> +    case BPF_RELO_TYPE_SIZE:
> +    case BPF_RELO_ENUMVAL_EXISTS:
> +    case BPF_RELO_ENUMVAL_VALUE:
> +    case BPF_RELO_TYPE_MATCHES:
> +      btf_mark_type_used (data->type);
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }
> +}
> +
>  /* Used in bpf_expand_builtin.  This function is called in RTL expand stage to
>     convert the internal __builtin_core_reloc in unspec:UNSPEC_CORE_RELOC RTL,
>     which will contain a third argument that is the index in the vec collected
> @@ -1567,6 +1624,8 @@ bpf_expand_core_builtin (tree exp, enum bpf_builtins code)
>  	tree index = CALL_EXPR_ARG (exp, 0);
>  	struct cr_builtins *data = get_builtin_data (TREE_INT_CST_LOW (index));
>  
> +	bpf_mark_types_as_used (data);
> +
>  	rtx v = expand_normal (data->default_value);
>  	rtx i = expand_normal (index);
>  	  return gen_rtx_UNSPEC (DImode,
> diff --git a/gcc/ctfc.h b/gcc/ctfc.h
> index 29267dc036d1..41e1169f271d 100644
> --- a/gcc/ctfc.h
> +++ b/gcc/ctfc.h
> @@ -457,6 +457,7 @@ extern ctf_dtdef_ref ctf_lookup_tree_type (ctf_container_ref, const tree);
>  
>  typedef bool (*funcs_traverse_callback) (ctf_dtdef_ref, void *);
>  bool traverse_btf_func_types (funcs_traverse_callback, void *);
> +extern void btf_mark_type_used (tree);
>  
>  /* CTF section does not emit location information; at this time, location
>     information is needed for BTF CO-RE use-cases.  */
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 8479fd5cf2b8..0afd686733d0 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -12023,6 +12023,9 @@ It is primarily useful when compiling for the BPF target, to minimize
>  the size of the resulting object, and to eliminate BTF information
>  which is not immediately relevant to the BPF program loading process.
>  
> +This option is enabled by default for the BPF target when generating
> +BTF information.
> +
>  @opindex gctf
>  @item -gctf
>  @itemx -gctf@var{level}
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-5.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-5.c
> index 8aae76cacabd..a08130cfc072 100644
> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-5.c
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-5.c
> @@ -11,9 +11,11 @@
>  /* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
>  /* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>  
> -/* Expect 2 array types, one of which is unsized.  */
> +/* Expect 2 array types, one of which is unsized.  For BPF target, -gprune-btf
> +   is the default and will remove the unsized array type.  */
>  /* { dg-final { scan-assembler-times "\[\t \]0x4\[\t \]+\[^\n\]*bta_nelems" 1 } } */
> -/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*bta_nelems" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*bta_nelems" 1 { target { !bpf-*-* } } } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*bta_nelems" 0 { target { bpf-*-* } } } } */
>  
>  extern const char FOO[];
>  const char FOO[] = "foo";