Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]

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

Looks OK to me overall. Minor comments below.

Thanks

On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find BTF_KIND_VAR records for references to
> extern const void symbols. We were mistakenly identifing these as
> unsupported types, and as a result skipping emitting VAR records for
> them.
> 
> In addition, the internal DWARF representation from which BTF is
> produced does not generate 'const' modifier DIEs for the void type,
> which meant in BTF the 'const' qualifier was dropped for 'extern const
> void' variables. This patch also adds support for generating a const
> void type in BTF to correct emission for these variables.
> 
> 	PR target/106773
> 
> gcc/
> 
> 	* btfout.cc (btf_collect_datasec): Correct size of void entries.
> 	(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
> 	refer to void types.
> 	(btf_init_postprocess): Create 'const void' type record if needed and
> 	adjust variables to refer to it as appropriate.
> 
> gcc/testsuite/
> 
> 	* gcc.dg/debug/btf/btf-pr106773.c: New test.
> ---
>   gcc/btfout.cc                                 | 44 +++++++++++++++++--
>   gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>   2 files changed, 65 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index a1c6266a7db..05f3a3f9b6e 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>         tree size = DECL_SIZE_UNIT (node->decl);
>         if (tree_fits_uhwi_p (size))
>   	info.size = tree_to_uhwi (size);
> +      else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
> +	info.size = 1;
>   
>         /* Offset is left as 0 at compile time, to be filled in by loaders such
>   	 as libbpf.  */
> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>     ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>   
>     /* Do not add variables which refer to unsupported types.  */
> -  if (btf_removed_type_p (var->dvd_type))
> +  if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>       return 1;
>   
>     arg_ctfc->ctfc_vars_list[num_vars_added] = var;
> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>   {
>     ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>   
> -  size_t i;
> -  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> -
>     holes.create (0);
>     voids.create (0);
>   
>     num_types_added = 0;
>     num_types_created = 0;
>   
> +  /* Workaround for 'const void' variables. These variables are sometimes used
> +     in eBPF programs to address kernel symbols. DWARF does not generate const
> +     qualifier on void type, so we would incorrectly emit these variables
> +     without the const qualifier.
> +     Unfortunately we need the TREE node to know it was const, and we need
> +     to create the const modifier type (if needed) now, before making the types
> +     list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
> +     again when creating the DATASEC entries.  */

"Dot, space, space, new sentence." in 3 places.


> +  ctf_id_t constvoid_id = CTF_NULL_TYPEID;
> +  varpool_node *var;
> +  FOR_EACH_VARIABLE (var)
> +    {
> +      if (!var->decl)
> +	continue;
> +
> +      tree type = TREE_TYPE (var->decl);
> +      if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
> +	{
> +	  dw_die_ref die = lookup_decl_die (var->decl);
> +	  if (die == NULL)
> +	    continue;
> +
> +	  ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
> +	  if (dvd == NULL)
> +	    continue;
> +
> +	  /* Create the 'const' modifier type for void.  */
> +	  if (constvoid_id == CTF_NULL_TYPEID)
> +	    constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
> +					    dvd->dvd_type, CTF_K_CONST, NULL);

No de-duplication of the const void type.  I assume libbpf will take 
care of this eventually.

> +	  dvd->dvd_type = constvoid_id;
> +	}
> +    }
> +
> +  size_t i;
> +  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> +
>     if (num_ctf_types)
>       {
>         init_btf_id_map (num_ctf_types + 1);
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> new file mode 100644
> index 00000000000..f90fa773a4b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> @@ -0,0 +1,25 @@
> +/* Test BTF generation for extern const void symbols.
> +   BTF_KIND_VAR records should be emitted for such symbols if they are used,
> +   as well as a corresponding entry in the appropriate DATASEC record.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage.  */
> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
> +
> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
> +
> +unsigned long func () {
> +  unsigned long x = (unsigned long) &foo;
> +
> +  return x;
> +}
> +