Re: [PATCH dwarves 5/5] btf_encoder: skip BTF encoding of static functions with inconsistent prototypes

[Alan Maguire <alan.maguire@oracle.com>]

On 25/01/2023 16:53, Jiri Olsa wrote:
> On Tue, Jan 24, 2023 at 01:45:31PM +0000, Alan Maguire wrote:
> 
> SNIP
> 
>>  	} else {
>>  		struct btf_encoder_state *state = zalloc(sizeof(*state));
>>  
>> @@ -898,10 +954,12 @@ static void btf_encoder__add_saved_func(const void *nodep, const VISIT which,
>>  	/* we can safely free encoder state since we visit each node once */
>>  	free(fn->priv);
>>  	fn->priv = NULL;
>> -	if (fn->proto.optimized_parms) {
>> +	if (fn->proto.optimized_parms || fn->proto.inconsistent_proto) {
>>  		if (encoder->verbose)
>> -			printf("skipping addition of '%s' due to optimized-out parameters\n",
>> -			       function__name(fn));
>> +			printf("skipping addition of '%s' due to %s\n",
>> +			       function__name(fn),
>> +			       fn->proto.optimized_parms ? "optimized-out parameters" :
>> +							   "multiple inconsistent function prototypes");
>>  	} else {
>>  		btf_encoder__add_func(encoder, fn);
>>  	}
>> @@ -1775,6 +1833,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>>  		 */
>>  		if (fn->declaration)
>>  			continue;
>> +		if (!fn->external)
>> +			save = true;
> 
> how about conflicts between static and global functions,
> I can see still few cases like:
> 
>   void __init msg_init(void)
>   static void msg_init(struct spi_message *m, struct spi_transfer *x,
>                        u8 *addr, size_t count, char *tx, char *rx)
> 
>   static inline void free_pgtable_page(u64 *pt)
>   void free_pgtable_page(void *vaddr)
> 
>   STATIC inline long INIT parse_header(u8 *input, long *skip, long in_len)
>   static struct tb_cfg_result parse_header(const struct ctl_pkg *pkg, u32 len,
>                                          enum tb_cfg_pkg_type type, u64 route)
>   static void __init parse_header(char *s)
> 
>

great catch; I hadn't thought of this at all. Looks like it is often
the case that the first function found will actually end up in BTF
currently, so sometimes we get the static, sometimes the non-static.
I'm seeing the same list as above.

> could we enable the check/save globaly?
>

I think we could, though at the additional cost of a larger tree
of functions. I can't see another way to handle it though right
now.

Alan

 
> jirka
> 
>>  		if (!ftype__has_arg_names(&fn->proto))
>>  			continue;
>>  		if (encoder->functions.cnt) {
>> @@ -1790,7 +1850,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>>  			if (func) {
>>  				if (func->generated)
>>  					continue;
>> -				func->generated = true;
>> +				if (!save)
>> +					func->generated = true;
>>  			} else if (encoder->functions.suffix_cnt) {
>>  				/* falling back to name.isra.0 match if no exact
>>  				 * match is found; only bother if we found any
>> diff --git a/dwarves.h b/dwarves.h
>> index 1ad1b3b..9b80262 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -830,6 +830,7 @@ struct ftype {
>>  	uint16_t	 nr_parms;
>>  	uint8_t		 unspec_parms:1; /* just one bit is needed */
>>  	uint8_t		 optimized_parms:1;
>> +	uint8_t		 inconsistent_proto:1;
>>  };
>>  
>>  static inline struct ftype *tag__ftype(const struct tag *tag)
>> -- 
>> 1.8.3.1
>>