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

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Jan 24, 2023 at 01:45:31PM +0000, Alan Maguire wrote:

SNIP

>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn)
>  {
> @@ -819,13 +837,51 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
>  	}
>  	/* If we find an existing entry, we want to merge observations
>  	 * across both functions, checking that the "seen optimized-out
> -	 * parameters" status is reflected in our tree entry.
> +	 * parameters"/inconsistent proto status is reflected in tree entry.
>  	 * If the entry is new, record encoder state required
>  	 * to add the local function later (encoder + type_id_off)
> -	 * such that we can add the function later.
> +	 * such that we can add the function later.  Parameter names are
> +	 * also stored in state to speed up multiple static function
> +	 * comparisons.
>  	 */
>  	if (*nodep != fn) {
> -		(*nodep)->proto.optimized_parms |= fn->proto.optimized_parms;
> +		struct function *ofn = *nodep;
> +
> +		ofn->proto.optimized_parms |= fn->proto.optimized_parms;
> +		/* compare parameters to see if signatures match */
> +
> +		if (ofn->proto.inconsistent_proto)
> +			goto out;
> +
> +		if (ofn->proto.nr_parms != fn->proto.nr_parms) {
> +			ofn->proto.inconsistent_proto = 1;
> +			goto out;
> +		}
> +		if (ofn->proto.nr_parms > 0) {
> +			struct btf_encoder_state *state = ofn->priv;
> +			const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
> +			int i;
> +
> +			if (!state->got_parameter_names) {
> +				parameter_names__get(&ofn->proto, BTF_ENCODER_MAX_PARAMETERS,
> +						     state->parameter_names);
> +				state->got_parameter_names = true;
> +			}
> +			parameter_names__get(&fn->proto, BTF_ENCODER_MAX_PARAMETERS,
> +					     parameter_names);
> +			for (i = 0; i < ofn->proto.nr_parms; i++) {
> +				if (!state->parameter_names[i]) {
> +					if (!parameter_names[i])
> +						continue;
> +				} else if (parameter_names[i]) {
> +					if (strcmp(state->parameter_names[i],
> +						   parameter_names[i]) == 0)
> +						continue;

I guess we can't check type easily? tag has type field,
but I'm not sure if we can get reasonable type info from that

jirka

> +				}
> +				ofn->proto.inconsistent_proto = 1;
> +				goto out;
> +			}
> +		}
>  	} 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;
>  		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
>