Re: [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Alan Maguire <alan.maguire@oracle.com> writes:
> On 03/10/2024 00:52, Stephen Brennan wrote:
>> Currently we maintain one buffer of DATASEC entries that describe the
>> offsets for variables in the percpu ELF section. In order to make it
>> possible to output all global variables, we'll need to output a DATASEC
>> for each ELF section containing variables, and we'll need to control
>> whether or not to encode variables on a per-section basis.
>> 
>> With this change, the ability to emit VARs from multiple sections is
>> technically present, but not enabled, so pahole still only emits percpu
>> variables. A subsequent change will enable emitting all global
>> variables.
>> 
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>
> Some questions about shndx handling, but
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
>> ---
>>  btf_encoder.c | 90 ++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 56 insertions(+), 34 deletions(-)
>> 
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 1872e00..2fd1648 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -98,6 +98,8 @@ struct elf_secinfo {
>>  	const char *name;
>>  	uint64_t    sz;
>>  	uint32_t    type;
>> +	bool        include;
>> +	struct gobuffer secinfo;
>>  };
>>  
>>  /*
>> @@ -107,7 +109,6 @@ struct btf_encoder {
>>  	struct list_head  node;
>>  	struct btf        *btf;
>>  	struct cu         *cu;
>> -	struct gobuffer   percpu_secinfo;
>>  	const char	  *source_filename;
>>  	const char	  *filename;
>>  	struct elf_symtab *symtab;
>> @@ -124,7 +125,6 @@ struct btf_encoder {
>>  	uint32_t	  array_index_id;
>>  	struct elf_secinfo *secinfo;
>>  	size_t             seccnt;
>> -	size_t             percpu_shndx;
>
> heh, ignore my earlier gripes about the type here since it's being
> removed now!
>
>>  	int                encode_vars;
>>  	struct {
>>  		struct elf_function *entries;
>> @@ -784,46 +784,56 @@ static int32_t btf_encoder__add_var(struct btf_encoder *encoder, uint32_t type,
>>  	return id;
>>  }
>>  
>> -static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, uint32_t type,
>> -				     uint32_t offset, uint32_t size)
>> +static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
>> +					    uint32_t type, uint32_t offset, uint32_t size)
>>  {
>>  	struct btf_var_secinfo si = {
>>  		.type = type,
>>  		.offset = offset,
>>  		.size = size,
>>  	};
>> -	return gobuffer__add(&encoder->percpu_secinfo, &si, sizeof(si));
>> +	return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
>>  }
>>  
>>  int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
>>  {
>> -	struct gobuffer *var_secinfo_buf = &other->percpu_secinfo;
>> -	size_t sz = gobuffer__size(var_secinfo_buf);
>> -	uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> -	uint32_t type_id;
>> -	uint32_t next_type_id = btf__type_cnt(encoder->btf);
>> -	int32_t i, id;
>> -	struct btf_var_secinfo *vsi;
>> -
>> +	size_t shndx;
>>  	if (encoder == other)
>>  		return 0;
>>  
>>  	btf_encoder__add_saved_funcs(other);
>>  
>> -	for (i = 0; i < nr_var_secinfo; i++) {
>> -		vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
>> -		type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
>> -		id = btf_encoder__add_var_secinfo(encoder, type_id, vsi->offset, vsi->size);
>> -		if (id < 0)
>> -			return id;
>> +	for (shndx = 0; shndx < other->seccnt; shndx++) {
>
> can't we start from 1 here since the first section is SHT_NULL?

Yeah, we're starting from 1 elsewhere, so I think all the other loops
should do so. Otherwise it's inconsistent. Nice catch, thanks!

Stephen

>> +		struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
>> +		size_t sz = gobuffer__size(var_secinfo_buf);
>> +		uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> +		uint32_t type_id;
>> +		uint32_t next_type_id = btf__type_cnt(encoder->btf);
>> +		int32_t i, id;
>> +		struct btf_var_secinfo *vsi;
>> +
>> +		if (strcmp(encoder->secinfo[shndx].name, other->secinfo[shndx].name)) {
>> +			fprintf(stderr, "mismatched ELF sections at index %zu: \"%s\", \"%s\"\n",
>> +				shndx, encoder->secinfo[shndx].name, other->secinfo[shndx].name);
>> +			return -1;
>> +		}
>> +
>> +		for (i = 0; i < nr_var_secinfo; i++) {
>> +			vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
>> +			type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
>> +			id = btf_encoder__add_var_secinfo(encoder, shndx, type_id, vsi->offset, vsi->size);
>> +			if (id < 0)
>> +				return id;
>> +		}
>>  	}
>>  
>>  	return btf__add_btf(encoder->btf, other->btf);
>>  }
>>  
>> -static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, const char *section_name)
>> +static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
>>  {
>> -	struct gobuffer *var_secinfo_buf = &encoder->percpu_secinfo;
>> +	struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
>> +	const char *section_name = encoder->secinfo[shndx].name;
>>  	struct btf *btf = encoder->btf;
>>  	size_t sz = gobuffer__size(var_secinfo_buf);
>>  	uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> @@ -2032,12 +2042,14 @@ int btf_encoder__encode(struct btf_encoder *encoder)
>>  {
>>  	bool should_tag_kfuncs;
>>  	int err;
>> +	size_t shndx;
>>  
>>  	/* for single-threaded case, saved funcs are added here */
>>  	btf_encoder__add_saved_funcs(encoder);
>>  
>> -	if (gobuffer__size(&encoder->percpu_secinfo) != 0)
>> -		btf_encoder__add_datasec(encoder, PERCPU_SECTION);
>> +	for (shndx = 0; shndx < encoder->seccnt; shndx++)
>
> same question here for 0 shndx
>
>
>> +		if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
>> +			btf_encoder__add_datasec(encoder, shndx);
>>  
>>  	/* Empty file, nothing to do, so... done! */
>>  	if (btf__type_cnt(encoder->btf) == 1)
>> @@ -2167,7 +2179,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  	struct tag *pos;
>>  	int err = -1;
>>  
>> -	if (encoder->percpu_shndx == 0 || !encoder->symtab)
>> +	if (!encoder->symtab)
>>  		return 0;
>>  
>>  	if (encoder->verbose)
>> @@ -2180,6 +2192,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  		struct llvm_annotation *annot;
>>  		const struct tag *tag;
>>  		size_t shndx, size;
>> +		struct elf_secinfo *sec = NULL;
>>  		uint64_t addr;
>>  		int id;
>>  
>> @@ -2211,7 +2224,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  
>>  		/* Get the ELF section info for the variable */
>>  		shndx = get_elf_section(encoder, addr);
>> -		if (shndx != encoder->percpu_shndx)
>> +		if (shndx >= 0 && shndx < encoder->seccnt)
>> +			sec = &encoder->secinfo[shndx];
>> +		if (!sec || !sec->include)
>>  			continue;
>>  
>>  		/* Convert addr to section relative */
>> @@ -2252,7 +2267,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  		size = tag__size(tag, cu);
>>  		if (size == 0 || size > UINT32_MAX) {
>>  			if (encoder->verbose)
>> -				fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
>> +				fprintf(stderr, "Ignoring %s-sized variable '%s'...\n",
>>  					size == 0 ? "zero" : "over", name);
>>  			continue;
>>  		}
>> @@ -2289,13 +2304,14 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  		}
>>  
>>  		/*
>> -		 * add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
>> -		 * encoder->types later when we add BTF_VAR_DATASEC.
>> +		 * 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.
>>  		 */
>> -		id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
>> +		id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
>>  		if (id < 0) {
>>  			fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
>> -			        name, addr);
>> +				name, addr);
>>  			goto out;
>>  		}
>>  	}
>> @@ -2373,6 +2389,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>>  			goto out_delete;
>>  		}
>>  
>> +		bool found_percpu = false;
>>  		for (shndx = 0; shndx < encoder->seccnt; shndx++) {
>>  			const char *secname = NULL;
>>  			Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
>> @@ -2383,11 +2400,14 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>>  			encoder->secinfo[shndx].name = secname;
>>  			encoder->secinfo[shndx].type = shdr.sh_type;
>>  
>> -			if (strcmp(secname, PERCPU_SECTION) == 0)
>> -				encoder->percpu_shndx = shndx;
>> +			if (strcmp(secname, PERCPU_SECTION) == 0) {
>> +				found_percpu = true;
>> +				if (encoder->encode_vars & BTF_VAR_PERCPU)
>> +					encoder->secinfo[shndx].include = true;
>> +			}
>>  		}
>>  
>> -		if (!encoder->percpu_shndx && encoder->verbose)
>> +		if (!found_percpu && encoder->verbose)
>>  			printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>>  
>>  		if (btf_encoder__collect_symbols(encoder))
>> @@ -2415,12 +2435,14 @@ void btf_encoder__delete_func(struct elf_function *func)
>>  void btf_encoder__delete(struct btf_encoder *encoder)
>>  {
>>  	int i;
>> +	size_t shndx;
>>  
>>  	if (encoder == NULL)
>>  		return;
>>  
>>  	btf_encoders__delete(encoder);
>> -	__gobuffer__delete(&encoder->percpu_secinfo);
>> +	for (shndx = 0; shndx < encoder->seccnt; shndx++)
>> +		__gobuffer__delete(&encoder->secinfo[shndx].secinfo);
>>  	zfree(&encoder->filename);
>>  	zfree(&encoder->source_filename);
>>  	btf__free(encoder->btf);