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

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

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?

>  /*
>   * 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!

>  	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;
>