Re: [PATCH] btf: improve -dA comments for testsuite

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

On 5/30/23 09:08, David Faust wrote:
>>> @@ -793,7 +917,8 @@ btf_asm_enum_const (unsigned int size, ctf_dmdef_t * dmd)
>>>    /* Asm'out a function parameter description following a BTF_KIND_FUNC_PROTO.  */
>>>    
>>>    static void
>>> -btf_asm_func_arg (ctf_func_arg_t * farg, size_t stroffset)
>>> +btf_asm_func_arg (ctf_container_ref ctfc, ctf_func_arg_t * farg,
>>> +		  size_t stroffset)
>>>    {
>>>      /* If the function arg does not have a name, refer to the null string at
>>>         the start of the string table. This ensures correct encoding for varargs
>>> @@ -803,31 +928,33 @@ btf_asm_func_arg (ctf_func_arg_t * farg, size_t stroffset)
>>>      else
>>>        dw2_asm_output_data (4, 0, "farg_name");
>>>    
>>> -  dw2_asm_output_data (4, (btf_removed_type_p (farg->farg_type)
>>> -			   ? BTF_VOID_TYPEID
>>> -			   : get_btf_id (farg->farg_type)),
>>> -		       "farg_type");
>>> +  btf_asm_type_ref ("farg_type", ctfc, (btf_removed_type_p (farg->farg_type)
>>> +					? BTF_VOID_TYPEID
>>> +					: get_btf_id (farg->farg_type)));
>>>    }
>>>    
>>>    /* Asm'out a BTF_KIND_FUNC type.  */
>>>    
>> Lets keep the function level comments updated.  Apart from
>> btf_asm_func_type, this comment applies to other functions touched in
>> this commit, like btf_asm_datasec_type.
> I don't follow. All those functions are still doing the same thing.
> What needs to be updated exactly?
> 

I meant updating the function-level comments for the additional args 
that are added.

I saw that in this patch, the new functions added follow the style of 
explaining the args, like,

+/* Return the relative index of the variable with final BTF ID ABS.  */
+
+static ctf_id_t
+btf_relative_var_id (ctf_id_t abs)

Hence, my comment.  But on second look, however, I see that the file 
keeps a mix of different styles. This one is up to you then. It makes 
sense to leave out the self-explanatory args.

>>>    static void
>>> -btf_asm_func_type (ctf_dtdef_ref dtd)
>>> +btf_asm_func_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd, size_t i)
>>>    {
>>> -  dw2_asm_output_data (4, dtd->dtd_data.ctti_name, "btt_name");
>>> -  dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_FUNC, 0,
>>> -                                         dtd->linkage),
>>> -                       "btt_info: kind=%u, kflag=%u, linkage=%u",
>>> -                       BTF_KIND_FUNC, 0, dtd->linkage);
>>> -  dw2_asm_output_data (4, get_btf_id (dtd->dtd_data.ctti_type), "btt_type");
>>> +  ctf_id_t ref_id = dtd->dtd_data.ctti_type;
>>> +  dw2_asm_output_data (4, dtd->dtd_data.ctti_name,
>>> +		       "TYPE %lu BTF_KIND_FUNC '%s'",
>>> +		       num_types_added + num_vars_added + 1 + i,
>>> +		       dtd->dtd_name);
>>> +  dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_FUNC, 0, dtd->linkage),
>>> +		       "btt_info: kind=%u, kflag=%u, linkage=%u",
>>> +		       BTF_KIND_FUNC, 0, dtd->linkage);
>>> +  btf_asm_type_ref ("btt_type", ctfc, get_btf_id (ref_id));
>>>    }
>>>    
>>>    /* Asm'out a variable entry following a BTF_KIND_DATASEC.  */
>>>    
>>>    static void
>>> -btf_asm_datasec_entry (struct btf_var_secinfo info)
>>> +btf_asm_datasec_entry (ctf_container_ref ctfc, struct btf_var_secinfo info)
>>>    {
>>> -  dw2_asm_output_data (4, info.type, "bts_type");
>>> +  btf_asm_type_ref ("bts_type", ctfc, info.type);
>>>      dw2_asm_output_data (4, info.offset, "bts_offset");
>>>      dw2_asm_output_data (4, info.size, "bts_size");
>>>    }
>>> @@ -835,9 +962,12 @@ btf_asm_datasec_entry (struct btf_var_secinfo info)
>>>    /* Asm'out a whole BTF_KIND_DATASEC, including its variable entries.  */
>>>    
>>>    static void
>>> -btf_asm_datasec_type (btf_datasec_t ds, size_t stroffset)
>>> +btf_asm_datasec_type (ctf_container_ref ctfc, btf_datasec_t ds, ctf_id_t id,
>>> +		      size_t stroffset)
>>>    {
>>> -  dw2_asm_output_data (4, ds.name_offset + stroffset, "btt_name");
>>> +  dw2_asm_output_data (4, ds.name_offset + stroffset,
>>> +		       "TYPE %lu BTF_KIND_DATASEC '%s'",
>>> +		       btf_absolute_datasec_id (id), ds.name);
>>>      dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_DATASEC, 0,
>>>    					 ds.entries.length ()),
>>>    		       "btt_info");
>>> @@ -845,7 +975,7 @@ btf_asm_datasec_type (btf_datasec_t ds, size_t stroffset)
>>>         loaders such as libbpf.  */
>>>      dw2_asm_output_data (4, 0, "btt_size");
>>>      for (size_t i = 0; i < ds.entries.length (); i++)
>>> -    btf_asm_datasec_entry (ds.entries[i]);
>>> +    btf_asm_datasec_entry (ctfc, ds.entries[i]);
>>>    }
>>>