Re: [PATCH dwarves 3/4] btf_encoder: cache all ELF section info

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

Alan Maguire <alan.maguire@oracle.com> writes:
> On 12/09/2024 20:08, Stephen Brennan wrote:
>> To handle outputting all variables generally, we'll need to store more
>> section data. Create a table of ELF sections so we can refer to all the
>> cached data, not just the percpu section.
>> 
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  btf_encoder.c | 50 ++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 36 insertions(+), 14 deletions(-)
>> 
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 8a2d92e..e3384e5 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -65,12 +65,20 @@ struct elf_function {
>>  	struct btf_encoder_state state;
>>  };
>>  
>> +#define MAX_ELF_SEC_CNT    128
>> +
>>  struct var_info {
>>  	uint64_t    addr;
>>  	const char *name;
>>  	uint32_t    sz;
>>  };
>>  
>> +struct elf_secinfo {
>> +	uint64_t    addr;
>> +	const char *name;
>> +	uint64_t    sz;
>> +};
>> +
>
> One thing we've run into before is hardcoded limits get exceeded on
> systems, and while it seems unlikely for 128 ELF sections, I think it'd
> be better to make this dynamic just in case. Also for parallel mode we
> have N encoders so it might same some memory (which we're pretty
> profligate with in other areas). I'd suggest using reallocarray_grow()
> as we do for functions. One thing we may want to tweak is that
> reallocarray_grow() will grow by 1000 or 1.5 x the current size;
> starting with 1000 ELF sections is a bit much.  We could perhaps
> tweak reallocarray_grow() to have a min_growth parameter to control this
> and set it to something smaller for ELF sections, what do you think?

I agree that the static allocation doesn't make a ton of sense. To be
quite honest, I was lazy during the initial implementation and intended
to come back around to make this dynamically allocated.

I think the answer is to simply use elf_getshdrnum and directly allocate
the array to the size we need. I'll definitely do that in the next
version.

>>  /*
>>   * cu: cu being processed.
>>   */
>> @@ -95,13 +103,13 @@ struct btf_encoder {
>>  			  is_rel,
>>  			  gen_distilled_base;
>>  	uint32_t	  array_index_id;
>> +	struct elf_secinfo secinfo[MAX_ELF_SEC_CNT];
>
> ...so here we'd have a substructure like this
>
> 	struct {
> 		int	allocated;
> 		int	sec_cnt;
> 		struct elf_secinfo *sections;
> 	} elf_sections;
>
>> +	size_t             seccnt;
>>  	struct {
>>  		struct var_info *vars;
>>  		int		var_cnt;
>>  		int		allocated;
>>  		uint32_t	shndx;
>> -		uint64_t	base_addr;
>> -		uint64_t	sec_sz;
>>  	} percpu;
>>  	struct {
>>  		struct elf_function *entries;
>> @@ -1849,7 +1857,7 @@ static int btf_encoder__collect_percpu_var(struct btf_encoder *encoder, GElf_Sym
>>  	 * ET_EXEC file) we need to subtract the section address.
>>  	 */
>>  	if (!encoder->is_rel)
>> -		addr -= encoder->percpu.base_addr;
>> +		addr -= encoder->secinfo[encoder->percpu.shndx].addr;
>>
> We will need some of these mechanics later when we encode function
> addresses; great to have this!

Ironically enough, this code (and especially, the is_rel field referred
to here) gets deleted in patch 4. Maybe I should have left is_rel if you
think we're going to need function addresses from the symbol table.

But patch 4 fully embraces simply using DWARF to get the variable
addresses, and it stops using the ELF symbol table for variables. The
DWARF addresses are always absolute. I couldn't really find a good
reason for using the symbol table, except maybe as a decent way to
filter out bogus variables that didn't actually become real data. For
example, the x86_64 percpu region starts at address 0, and many bogus
DWARF variables have address 0, so  making sure the variable matched the
symbol at address 0 was a good way to filter bogus variables.

I didn't think it was a big deal that I removed this behavior, because
with improved variable filtering, this code continues to produce nearly
identical percpu DATASECs, with the only difference being that we now
(correctly) include the fixed_percpu_data variable for x86_64, which was
omitted before. So there's no regression in functionality.

Stephen

>>  	if (encoder->percpu.var_cnt == encoder->percpu.allocated) {
>>  		struct var_info *new;
>> @@ -1923,6 +1931,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  	uint32_t core_id;
>>  	struct tag *pos;
>>  	int err = -1;
>> +	struct elf_secinfo *pcpu_scn = &encoder->secinfo[encoder->percpu.shndx];
>>  
>>  	if (encoder->percpu.shndx == 0 || !encoder->symtab)
>>  		return 0;
>> @@ -1954,9 +1963,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>  		 * always contains virtual symbol addresses, so subtract
>>  		 * the section address unconditionally.
>>  		 */
>> -		if (addr < encoder->percpu.base_addr || addr >= encoder->percpu.base_addr + encoder->percpu.sec_sz)
>> +		if (addr < pcpu_scn->addr || addr >= pcpu_scn->addr + pcpu_scn->sz)
>>  			continue;
>> -		addr -= encoder->percpu.base_addr;
>> +		addr -= pcpu_scn->addr;
>>  
>>  		if (!btf_encoder__percpu_var_exists(encoder, addr, &size, &name))
>>  			continue; /* not a per-CPU variable */
>> @@ -2099,20 +2108,33 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>>  			goto out;
>>  		}
>>  
>> -		/* find percpu section's shndx */
>> +		/* index the ELF sections for later lookup */
>>  
>>  		GElf_Shdr shdr;
>> -		Elf_Scn *sec = elf_section_by_name(cu->elf, &shdr, PERCPU_SECTION, NULL);
>> +		size_t shndx;
>> +		if (elf_getshdrnum(cu->elf, &encoder->seccnt))
>> +			goto out_delete;
>> +		if (encoder->seccnt >= MAX_ELF_SEC_CNT) {
>> +			fprintf(stderr, "%s: reached limit of ELF sections\n", __func__);
>> +			goto out_delete;
>> +		}
>>  
> as above we should just reallocarray_grow() more space.
>
>> -		if (!sec) {
>> -			if (encoder->verbose)
>> -				printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>> -		} else {
>> -			encoder->percpu.shndx	  = elf_ndxscn(sec);
>> -			encoder->percpu.base_addr = shdr.sh_addr;
>> -			encoder->percpu.sec_sz	  = shdr.sh_size;
>> +		for (shndx = 0; shndx < encoder->seccnt; shndx++) {
>> +			const char *secname = NULL;
>> +			Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
>> +			if (!sec)
>> +				goto out_delete;
>> +			encoder->secinfo[shndx].addr = shdr.sh_addr;
>> +			encoder->secinfo[shndx].sz = shdr.sh_size;
>> +			encoder->secinfo[shndx].name = secname;
>> +
>> +			if (strcmp(secname, PERCPU_SECTION) == 0)
>> +				encoder->percpu.shndx = shndx;
>>  		}
>>  
>> +		if (!encoder->percpu.shndx && encoder->verbose)
>> +			printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>> +
>>  		if (btf_encoder__collect_symbols(encoder, !encoder->skip_encoding_vars))
>>  			goto out_delete;
>>