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

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

On 12/12/22 12:59, David Faust wrote:
> 
> 
> On 12/8/22 23:34, Indu Bhagat wrote:
>> 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.
> 
> Hm, not sure I follow. Where is the duplication? The const void type is
> only created once here, for the first such variable which needs it, and
> reused for subsequent variables. And it does not already exist in the
> CTF which we are translating into BTF.
> 

You're right - you are reusing the const void type once generated for a 
CU for each usage. My bad - I didnt follow the code properly :)

> In any case, yes libbpf can handle duplicated types. Though it would
> still be good to minimize that where we can to not bloat the BTF info.
> 
>>
>>> +	  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;
>>> +}
>>> +
>>