Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]

[Indu Bhagat <indu.bhagat@oracle.com>]

On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find entries for functions declared as extern
> in the corresponding BTF_KIND_DATASEC record, but we were not generating
> these entries.
> 
> This patch adds support for the 'extern' linkage of function types in
> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
> 
> 	PR target/106773
> 
> gcc/
> 
> 	* btfout.cc (get_section_name): New function.
> 	(btf_collect_datasec): Use it here. Process functions, marking them
> 	'extern' and generating DATASEC entries for them as appropriate. Move
> 	creation of BTF_KIND_FUNC records to here...
> 	(btf_dtd_emit_preprocess_cb): ... from here.
> 
> gcc/testsuite/
> 
> 	* gcc.dg/debug/btf/btf-datasec-2.c: New test.
> 	* gcc.dg/debug/btf/btf-function-6.c: New test.
> 
> include/
> 
> 	* btf.h (struct btf_var_secinfo): Update comments with notes about
> 	extern functions.
> ---
>   gcc/btfout.cc                                 | 129 ++++++++++++------
>   .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 ++++
>   .../gcc.dg/debug/btf/btf-function-6.c         |  19 +++
>   include/btf.h                                 |   9 +-
>   4 files changed, 139 insertions(+), 46 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index 05f3a3f9b6e..d7ead377ec5 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>     ds.entries.safe_push (info);
>   
>     datasecs.safe_push (ds);
> -  num_types_created++;
> +}
> +
> +
> +/* Return the section name, as of interest to btf_collect_datasec, for the
> +   given symtab node. Note that this deliberately returns NULL for objects
> +   which do not go in a section btf_collect_datasec cares about.  */

"Dot, space, space, new sentence."

> +static const char *
> +get_section_name (symtab_node *node)
> +{
> +  const char *section_name = node->get_section ();
> +
> +  if (section_name == NULL)
> +    {
> +      switch (categorize_decl_for_section (node->decl, 0))
> +	{
> +	case SECCAT_BSS:
> +	  section_name = ".bss";
> +	  break;
> +	case SECCAT_DATA:
> +	  section_name = ".data";
> +	  break;
> +	case SECCAT_RODATA:
> +	  section_name = ".rodata";
> +	  break;
> +	default:;
> +	}
> +    }
> +
> +  return section_name;
>   }
>   
>   /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>   static void
>   btf_collect_datasec (ctf_container_ref ctfc)
>   {
> -  /* See cgraph.h struct symtab_node, which varpool_node extends.  */
> +  cgraph_node *func;
> +  FOR_EACH_FUNCTION (func)
> +    {
> +      dw_die_ref die = lookup_decl_die (func->decl);
> +      if (die == NULL)
> +	continue;
> +
> +      ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
> +      if (dtd == NULL)
> +	continue;
> +
> +      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> +	 also a BTF_KIND_FUNC. But the CTF container only allocates one
> +	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> +	 For each such function, also allocate a BTF_KIND_FUNC entry.
> +	 These will be output later.  */

"Dot, space, space, new sentence."

> +      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> +      func_dtd->dtd_data = dtd->dtd_data;
> +      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> +      func_dtd->linkage = dtd->linkage;
> +      func_dtd->dtd_type = num_types_added + num_types_created;
> +
> +      /* Only the BTF_KIND_FUNC type actually references the name. The
> +	 BTF_KIND_FUNC_PROTO is always anonymous.  */
> +      dtd->dtd_data.ctti_name = 0;
> +
> +      vec_safe_push (funcs, func_dtd);
> +      num_types_created++;
> +
> +      /* Mark any 'extern' funcs and add DATASEC entries for them.  */
> +      if (DECL_EXTERNAL (func->decl))
> +	{
> +	  func_dtd->linkage = BTF_LINKAGE_EXTERN;
> +

What is the expected BTF when both decl and definition are present:

extern int extfunc(int x);
int extfunc (int x) {
   int y = foo ();
   return y;
}

> +	  const char *section_name = get_section_name (func);
> +	  /* Note: get_section_name () returns NULL for functions in text
> +	     section. This is intentional, since we do not want to generate
> +	     DATASEC entries for them.  */

"Dot, space, space, new sentence."

> +	  if (section_name == NULL)
> +	    continue;
> +
> +	  struct btf_var_secinfo info;
> +
> +	  /* +1 for the sentinel type not in the types map.  */
> +	  info.type = func_dtd->dtd_type + 1;
> +
> +	  /* Both zero at compile time.  */
> +	  info.size = 0;
> +	  info.offset = 0;
> +
> +	  btf_datasec_push_entry (ctfc, section_name, info);
> +	}
> +    }
> +
>     varpool_node *node;
>     FOR_EACH_VARIABLE (node)
>       {
> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>         if (dvd == NULL)
>   	continue;
>   
> -      const char *section_name = node->get_section ();
>         /* Mark extern variables.  */
>         if (DECL_EXTERNAL (node->decl))
>   	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>   
> +      const char *section_name = get_section_name (node);
>         if (section_name == NULL)
> -	{
> -	  switch (categorize_decl_for_section (node->decl, 0))
> -	    {
> -	    case SECCAT_BSS:
> -	      section_name = ".bss";
> -	      break;
> -	    case SECCAT_DATA:
> -	      section_name = ".data";
> -	      break;
> -	    case SECCAT_RODATA:
> -	      section_name = ".rodata";
> -	      break;
> -	    default:
> -	      continue;
> -	    }
> -	}
> +	continue;
>   
>         struct btf_var_secinfo info;
>   
> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>   
>         btf_datasec_push_entry (ctfc, section_name, info);
>       }
> +
> +  num_types_created += datasecs.length ();
>   }
>   
>   /* Return true if the type ID is that of a type which will not be emitted (for
> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>     if (!btf_emit_id_p (dtd->dtd_type))
>       return;
>   
> -  uint32_t btf_kind
> -    = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
> -
> -  if (btf_kind == BTF_KIND_FUNC_PROTO)
> -    {
> -      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> -	 also a BTF_KIND_FUNC. But the CTF container only allocates one
> -	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> -	 For each such function, also allocate a BTF_KIND_FUNC entry.
> -	 These will be output later.  */
> -      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> -      func_dtd->dtd_data = dtd->dtd_data;
> -      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> -      func_dtd->linkage = dtd->linkage;
> -
> -      vec_safe_push (funcs, func_dtd);
> -      num_types_created++;
> -
> -      /* Only the BTF_KIND_FUNC type actually references the name. The
> -	 BTF_KIND_FUNC_PROTO is always anonymous.  */
> -      dtd->dtd_data.ctti_name = 0;
> -    }
> -
>     ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>   }
>   
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> new file mode 100644
> index 00000000000..f4b298cf019
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> @@ -0,0 +1,28 @@
> +/* Test BTF generation of DATASEC records for extern functions.
> +
> +   Only functions declared extern should have entries in DATASEC records.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
> +
> +/* Function entries should have offset and size of 0 at compile time.  */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
> +
> +extern int foo (int a) __attribute__((section(".foo_sec")));
> +
> +
> +extern int bar (int b) __attribute__((section(".bar_sec")));
> +extern void chacha (void) __attribute__((section(".bar_sec")));
> +
> +__attribute__((section(".foo_sec")))
> +void baz (int *x)
> +{
> +  chacha ();
> +
> +  *x = foo (bar (*x));
> +}
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> new file mode 100644
> index 00000000000..48a946ab14b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> @@ -0,0 +1,19 @@
> +/* Test BTF extern linkage for functions.
> +
> +   We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
> +   one BTF_KIND_FUNC type with extern linkage (extfunc).  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
> +
> +extern int extfunc(int a, int b);
> +
> +int foo (int x) {
> +
> +  int y = extfunc (x, x+1);
> +
> +  return y;
> +}
> diff --git a/include/btf.h b/include/btf.h
> index 9a757ce5bc9..f41ea94b75f 100644
> --- a/include/btf.h
> +++ b/include/btf.h
> @@ -186,12 +186,13 @@ struct btf_var
>   };
>   
>   /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
> -   which describe all BTF_KIND_VAR types contained in the section.  */
> +   which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
> +   in the section.  */
>   struct btf_var_secinfo
>   {
> -  uint32_t type;	/* Type of variable.  */
> -  uint32_t offset;	/* In-section offset of variable (in bytes).  */
> -  uint32_t size;	/* Size (in bytes) of variable.  */
> +  uint32_t type;	/* Type of BTF_KIND_VAR or BTF_KIND_FUNC item.  */
> +  uint32_t offset;	/* In-section offset (in bytes) of item.  */
> +  uint32_t size;	/* Size (in bytes) of item.  */
>   };
>   
>   /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,