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

[David Faust <david.faust@oracle.com>]

On 6/12/24 09:55, Jose E. Marchesi wrote:
> 
> 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?
The callback is only marking as used the types which are present in the
CO-RE builtin argument by the chain of field accesses.  It is not
marking other types which may be part of those structures but are not
used by the CO-RE builtin.  The intent is to ensure type information
needed to perform the CO-RE relocation is present, while allowing other
extraneous types to be pruned.

For example if we have:

struct A {
  struct B *b;
  struct C *c;
};

struct B {
  int x;
  int y;
};

struct C {
  int w;
  int z;
  char stuff[400];
  struct some_other_huge_struct *ptr_to_huge;
  ...
  int member_number_800_at_offset_10620;
};

struct A my_a = ...;
__builtin_preserve_access_index (my_a->b.x);

In this case, we will mark struct B as used, but not struct C.

Supposing struct C is otherwise unused, then in the pruned BTF
we may get like:

[1] BTF_KIND_INT "int" bits=32 ..
[2] BTF_KIND_STRUCT "A"
      member "b" type=3
      member "c" type=5
[3] BTF_KIND_PTR type=4
[4] BTF_KIND_STRUCT "B"
      member "x" type=1
      member "y" type=1
[5] BTF_KIND_PTR type=6
[6] BTF_KIND_FWD "C" fwd_kind=struct

Where the full type information for C is pruned because it is not
necessary for this program, yet everything necessary to perform the
CO-RE relocation correctly is present.

Does that make sense to answer your question?

> 
>> +
>> +/* 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";