Re: [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2025-02-11 at 16:49 -0800, Stephen Brennan wrote:
> In order to handle duplicate variables in a data section, we'll need to
> compare variables against the others in their data section. To do this,
> we'll need to postpone adding the variables until they have all been
> identified and collected.  Store all the necessary data to encode the
> VARs (and their DECL_TAGs) and then encode them just before the DATASEC
> containing them. No meaningful change in the output is intended, though
> the ordering of type IDs generated will likely differ.
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---

I think this patch looks good,
but annotation index handling needs a small a fix,
see below.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -109,6 +109,20 @@ struct elf_functions {
>  	int suffix_cnt; /* number of .isra, .part etc */
>  };
>  
> +struct saved_annot {
> +	const char *value;
> +	int component_idx;
> +};
> +
> +struct saved_var {
> +	const char *name;
> +	uint32_t type;

Nit: maybe name this 'id' or 'var_id'?

> +	struct saved_annot *annots;
> +	uint32_t linkage;
> +	uint32_t offset;
> +	uint32_t size;
> +};
> +

[...]

> @@ -2359,30 +2428,34 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>  			       name, cu->name, addr);
>  		}
>  
> -		/* add a BTF_KIND_VAR in encoder->types */
> -		id = btf_encoder__add_var(encoder, type, name, linkage);
> -		if (id < 0) {
> -			fprintf(stderr, "error: failed to encode variable '%s' at addr 0x%" PRIx64 "\n",
> -			        name, addr);
> -			goto out;
> +		/* Save the annotations */
> +		annot_count = 0;
> +		annots = NULL;
> +		list_for_each_entry(annot, &var->annots, node) {
> +			annot_count += 1;
>  		}
> +		if (annot_count) {
> +			int idx = 0;
>  
> -		list_for_each_entry(annot, &var->annots, node) {
> -			int tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, id, annot->component_idx);
> -			if (tag_type_id < 0) {
> -				fprintf(stderr, "error: failed to encode tag '%s' to variable '%s' with component_idx %d\n",
> -					annot->value, name, annot->component_idx);
> +			annots = calloc(annot_count + 1, sizeof(*annots));
> +			if (!annots) {
> +				fprintf(stderr, "error: allocation failure\n");
> +				err = -ENOMEM;
>  				goto out;
>  			}
> +			list_for_each_entry(annot, &var->annots, node) {
> +				annots[idx].value = annot->value;
> +				annots[idx].component_idx = annot->component_idx;
> +			}

I think 'idx++' is missing here.

>  		}
>  
>  		/*
> -		 * Add the variable to the secinfo for the section it appears in.
> -		 * Later we will generate a BTF_VAR_DATASEC for all any section with
> -		 * an encoded variable.
> +		 * Store all the collected information for the variable. Later,
> +		 * we will deduplicate and output all.
>  		 */
> -		id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
> -		if (id < 0) {
> +		err = btf_encoder__store_var(encoder, shndx, name, type, linkage,
> +					    (uint32_t)addr, (uint32_t)size, annots);
> +		if (err < 0) {
>  			fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
>  				name, addr);

Free 'annots' in case of an error?

>  			goto out;

[...]