[v2,dwarves,1/5] dwarves: help dwarf loader spot functions with optimized-out parameters

Commit Message

Alan Maguire Jan. 30, 2023, 2:29 p.m. UTC

Compilation generates DWARF at several stages, and often the
later DWARF representations more accurately represent optimizations
that have occurred during compilation.

In particular, parameter representations can be spotted by their
abstract origin references to the original parameter, but they
often have more accurate location information.  In most cases,
the parameter locations will match calling conventions, and be
registers for the first 6 parameters on x86_64, first 8 on ARM64
etc.  If the parameter is not a register when it should be however,
it is likely passed via the stack or the compiler has used a
constant representation instead.  The latter can often be
spotted by checking for a DW_AT_const_value attribute,
as noted by Eduard.

In addition, absence of a location tag (either across
the abstract origin reference and the original parameter,
or in the standalone parameter description) is evidence of
an optimized-out parameter.  Presence of a location tag
is stored in the parameter description and shared between
abstract tags and their original referents.

This change adds a field to parameters and their associated
ftype to note if a parameter has been optimized out.  Having
this information allows us to skip such functions, as their
presence in CUs makes BTF encoding impossible.

Signed-off-by: Alan Maguire <[e-mail]>
---
 dwarf_loader.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 dwarves.h      |   5 ++-
 2 files changed, 122 insertions(+), 8 deletions(-)

Comments

Arnaldo Carvalho de Melo Jan. 30, 2023, 6:36 p.m. UTC | #1

Em Mon, Jan 30, 2023 at 02:29:41PM +0000, Alan Maguire escreveu:
> Compilation generates DWARF at several stages, and often the
> later DWARF representations more accurately represent optimizations
> that have occurred during compilation.
> 
> In particular, parameter representations can be spotted by their
> abstract origin references to the original parameter, but they
> often have more accurate location information.  In most cases,
> the parameter locations will match calling conventions, and be
> registers for the first 6 parameters on x86_64, first 8 on ARM64
> etc.  If the parameter is not a register when it should be however,
> it is likely passed via the stack or the compiler has used a
> constant representation instead.  The latter can often be
> spotted by checking for a DW_AT_const_value attribute,
> as noted by Eduard.
> 
> In addition, absence of a location tag (either across
> the abstract origin reference and the original parameter,
> or in the standalone parameter description) is evidence of
> an optimized-out parameter.  Presence of a location tag
> is stored in the parameter description and shared between
> abstract tags and their original referents.
> 
> This change adds a field to parameters and their associated
> ftype to note if a parameter has been optimized out.  Having
> this information allows us to skip such functions, as their
> presence in CUs makes BTF encoding impossible.
> 
> Signed-off-by: Alan Maguire <[e-mail]>
> ---
>  dwarf_loader.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  dwarves.h      |   5 ++-
>  2 files changed, 122 insertions(+), 8 deletions(-)
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 5a74035..93c2307 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -992,13 +992,98 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
>  	return member;
>  }
>  
> -static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
> +/* How many function parameters are passed via registers?  Used below in
> + * determining if an argument has been optimized out or if it is simply
> + * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
> + * allows unsupported architectures to skip tagging optimized-out
> + * values.
> + */
> +#if defined(__x86_64__)
> +#define NR_REGISTER_PARAMS      6
> +#elif defined(__s390__)
> +#define NR_REGISTER_PARAMS	5
> +#elif defined(__aarch64__)
> +#define NR_REGISTER_PARAMS      8
> +#elif defined(__mips__)
> +#define NR_REGISTER_PARAMS	8
> +#elif defined(__powerpc__)
> +#define NR_REGISTER_PARAMS	8
> +#elif defined(__sparc__)
> +#define NR_REGISTER_PARAMS	6
> +#elif defined(__riscv) && __riscv_xlen == 64
> +#define NR_REGISTER_PARAMS	8
> +#elif defined(__arc__)
> +#define NR_REGISTER_PARAMS	8
> +#else
> +#define NR_REGISTER_PARAMS      0
> +#endif

This should be done as a function, something like:

int cu__nr_register_params(struct cu *cu)
{
	GElf_Ehdr ehdr;

	gelf_getehdr(cu->elf, &ehdr);

	switch (ehdr.machine) {
	...

}

I'm coding that now, will send the diff shortly.

This is to support cross-builds.

- Arnaldo

> +
> +static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> +					struct conf_load *conf, int param_idx)
>  {
>  	struct parameter *parm = tag__alloc(cu, sizeof(*parm));
>  
>  	if (parm != NULL) {
> +		bool has_const_value;
> +		Dwarf_Attribute attr;
> +		struct location loc;
> +
>  		tag__init(&parm->tag, cu, die);
>  		parm->name = attr_string(die, DW_AT_name, conf);
> +
> +		if (param_idx >= NR_REGISTER_PARAMS)
> +			return parm;
> +		/* Parameters which use DW_AT_abstract_origin to point at
> +		 * the original parameter definition (with no name in the DIE)
> +		 * are the result of later DWARF generation during compilation
> +		 * so often better take into account if arguments were
> +		 * optimized out.
> +		 *
> +		 * By checking that locations for parameters that are expected
> +		 * to be passed as registers are actually passed as registers,
> +		 * we can spot optimized-out parameters.
> +		 *
> +		 * It can also be the case that a parameter DIE has
> +		 * a constant value attribute reflecting optimization or
> +		 * has no location attribute.
> +		 *
> +		 * From the DWARF spec:
> +		 *
> +		 * "4.1.10
> +		 *
> +		 * A DW_AT_const_value attribute for an entry describing a
> +		 * variable or formal parameter whose value is constant and not
> +		 * represented by an object in the address space of the program,
> +		 * or an entry describing a named constant. (Note
> +		 * that such an entry does not have a location attribute.)"
> +		 *
> +		 * So we can also use the absence of a location for a parameter
> +		 * as evidence it has been optimized out.  This info will
> +		 * need to be shared between a parameter and any abstract
> +		 * origin references however, since gcc can have location
> +		 * information in the parameter that refers back to the original
> +		 * via abstract origin, so we need to share location presence
> +		 * between these parameter representations.  See
> +		 * ftype__recode_dwarf_types() below for how this is handled.
> +		 */
> +		parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
> +		has_const_value = dwarf_attr(die, DW_AT_const_value, &attr) != NULL;
> +		if (parm->has_loc &&
> +		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
> +			loc.exprlen != 0) {
> +			Dwarf_Op *expr = loc.expr;
> +
> +			switch (expr->atom) {
> +			case DW_OP_reg1 ... DW_OP_reg31:
> +			case DW_OP_breg0 ... DW_OP_breg31:
> +				break;
> +			default:
> +				parm->optimized = 1;
> +				break;
> +			}
> +		} else if (has_const_value) {
> +			parm->optimized = 1;
> +		}
>  	}
>  
>  	return parm;
> @@ -1450,7 +1535,7 @@ static struct tag *die__create_new_parameter(Dwarf_Die *die,
>  					     struct cu *cu, struct conf_load *conf,
>  					     int param_idx)
>  {
> -	struct parameter *parm = parameter__new(die, cu, conf);
> +	struct parameter *parm = parameter__new(die, cu, conf, param_idx);
>  
>  	if (parm == NULL)
>  		return NULL;
> @@ -2194,6 +2279,7 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
>  
>  	ftype__for_each_parameter(type, pos) {
>  		struct dwarf_tag *dpos = pos->tag.priv;
> +		struct parameter *opos;
>  		struct dwarf_tag *dtype;
>  
>  		if (dpos->type.off == 0) {
> @@ -2207,8 +2293,18 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
>  				tag__print_abstract_origin_not_found(&pos->tag);
>  				continue;
>  			}
> -			pos->name = tag__parameter(dtype->tag)->name;
> +			opos = tag__parameter(dtype->tag);
> +			pos->name = opos->name;
>  			pos->tag.type = dtype->tag->type;
> +			/* share location information between parameter and
> +			 * abstract origin; if neither have location, we will
> +			 * mark the parameter as optimized out.
> +			 */
> +			if (pos->has_loc)
> +				opos->has_loc = pos->has_loc;
> +
> +			if (pos->optimized)
> +				opos->optimized = pos->optimized;
>  			continue;
>  		}
>  
> @@ -2478,18 +2574,33 @@ out:
>  	return 0;
>  }
>  
> -static int cu__resolve_func_ret_types(struct cu *cu)
> +static int cu__resolve_func_ret_types_optimized(struct cu *cu)
>  {
>  	struct ptr_table *pt = &cu->functions_table;
>  	uint32_t i;
>  
>  	for (i = 0; i < pt->nr_entries; ++i) {
>  		struct tag *tag = pt->entries[i];
> +		struct parameter *pos;
> +		struct function *fn = tag__function(tag);
> +
> +		/* mark function as optimized if parameter is, or
> +		 * if parameter does not have a location; at this
> +		 * point location presence has been marked in
> +		 * abstract origins for cases where a parameter
> +		 * location is not stored in the original function
> +		 * parameter tag.
> +		 */
> +		ftype__for_each_parameter(&fn->proto, pos) {
> +			if (pos->optimized || !pos->has_loc) {
> +				fn->proto.optimized_parms = 1;
> +				break;
> +			}
> +		}
>  
>  		if (tag == NULL || tag->type != 0)
>  			continue;
>  
> -		struct function *fn = tag__function(tag);
>  		if (!fn->abstract_origin)
>  			continue;
>  
> @@ -2612,7 +2723,7 @@ static int die__process_and_recode(Dwarf_Die *die, struct cu *cu, struct conf_lo
>  	if (ret != 0)
>  		return ret;
>  
> -	return cu__resolve_func_ret_types(cu);
> +	return cu__resolve_func_ret_types_optimized(cu);
>  }
>  
>  static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,
> @@ -3132,7 +3243,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>  	 * encoded in another subprogram through abstract_origin
>  	 * tag. Let us visit all subprograms again to resolve this.
>  	 */
> -	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> +	if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
>  		goto out_abort;
>  
>  	if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
> diff --git a/dwarves.h b/dwarves.h
> index 589588e..2723466 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -808,6 +808,8 @@ size_t lexblock__fprintf(const struct lexblock *lexblock, const struct cu *cu,
>  struct parameter {
>  	struct tag tag;
>  	const char *name;
> +	uint8_t optimized:1;
> +	uint8_t has_loc:1;
>  };
>  
>  static inline struct parameter *tag__parameter(const struct tag *tag)
> @@ -827,7 +829,8 @@ struct ftype {
>  	struct tag	 tag;
>  	struct list_head parms;
>  	uint16_t	 nr_parms;
> -	uint8_t		 unspec_parms; /* just one bit is needed */
> +	uint8_t		 unspec_parms:1; /* just one bit is needed */
> +	uint8_t		 optimized_parms:1;
>  };
>  
>  static inline struct ftype *tag__ftype(const struct tag *tag)
> -- 
> 1.8.3.1
>

Arnaldo Carvalho de Melo Jan. 30, 2023, 8:10 p.m. UTC | #2

Em Mon, Jan 30, 2023 at 03:36:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 30, 2023 at 02:29:41PM +0000, Alan Maguire escreveu:
> > Compilation generates DWARF at several stages, and often the
> > later DWARF representations more accurately represent optimizations
> > that have occurred during compilation.
> > 
> > In particular, parameter representations can be spotted by their
> > abstract origin references to the original parameter, but they
> > often have more accurate location information.  In most cases,
> > the parameter locations will match calling conventions, and be
> > registers for the first 6 parameters on x86_64, first 8 on ARM64
> > etc.  If the parameter is not a register when it should be however,
> > it is likely passed via the stack or the compiler has used a
> > constant representation instead.  The latter can often be
> > spotted by checking for a DW_AT_const_value attribute,
> > as noted by Eduard.
> > 
> > In addition, absence of a location tag (either across
> > the abstract origin reference and the original parameter,
> > or in the standalone parameter description) is evidence of
> > an optimized-out parameter.  Presence of a location tag
> > is stored in the parameter description and shared between
> > abstract tags and their original referents.
> > 
> > This change adds a field to parameters and their associated
> > ftype to note if a parameter has been optimized out.  Having
> > this information allows us to skip such functions, as their
> > presence in CUs makes BTF encoding impossible.
> > 
> > Signed-off-by: Alan Maguire <[e-mail]>
> > ---
> >  dwarf_loader.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  dwarves.h      |   5 ++-
> >  2 files changed, 122 insertions(+), 8 deletions(-)
> > 
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index 5a74035..93c2307 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -992,13 +992,98 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
> >  	return member;
> >  }
> >  
> > -static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
> > +/* How many function parameters are passed via registers?  Used below in
> > + * determining if an argument has been optimized out or if it is simply
> > + * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
> > + * allows unsupported architectures to skip tagging optimized-out
> > + * values.
> > + */
> > +#if defined(__x86_64__)
> > +#define NR_REGISTER_PARAMS      6
> > +#elif defined(__s390__)
> > +#define NR_REGISTER_PARAMS	5
> > +#elif defined(__aarch64__)
> > +#define NR_REGISTER_PARAMS      8
> > +#elif defined(__mips__)
> > +#define NR_REGISTER_PARAMS	8
> > +#elif defined(__powerpc__)
> > +#define NR_REGISTER_PARAMS	8
> > +#elif defined(__sparc__)
> > +#define NR_REGISTER_PARAMS	6
> > +#elif defined(__riscv) && __riscv_xlen == 64
> > +#define NR_REGISTER_PARAMS	8
> > +#elif defined(__arc__)
> > +#define NR_REGISTER_PARAMS	8
> > +#else
> > +#define NR_REGISTER_PARAMS      0
> > +#endif
> 
> This should be done as a function, something like:
> 
> int cu__nr_register_params(struct cu *cu)
> {
> 	GElf_Ehdr ehdr;
> 
> 	gelf_getehdr(cu->elf, &ehdr);
> 
> 	switch (ehdr.machine) {
> 	...
> 
> }
> 
> I'm coding that now, will send the diff shortly.
> 
> This is to support cross-builds.

I made this change to this patch, please check.

Thanks,

- Arnaldo

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 752a3c1afc4494f2..81963e71715c8435 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -994,29 +994,29 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
 
 /* How many function parameters are passed via registers?  Used below in
  * determining if an argument has been optimized out or if it is simply
- * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
- * allows unsupported architectures to skip tagging optimized-out
+ * an argument > cu__nr_register_params().  Making cu__nr_register_params()
+ * return 0 allows unsupported architectures to skip tagging optimized-out
  * values.
  */
-#if defined(__x86_64__)
-#define NR_REGISTER_PARAMS      6
-#elif defined(__s390__)
-#define NR_REGISTER_PARAMS	5
-#elif defined(__aarch64__)
-#define NR_REGISTER_PARAMS      8
-#elif defined(__mips__)
-#define NR_REGISTER_PARAMS	8
-#elif defined(__powerpc__)
-#define NR_REGISTER_PARAMS	8
-#elif defined(__sparc__)
-#define NR_REGISTER_PARAMS	6
-#elif defined(__riscv) && __riscv_xlen == 64
-#define NR_REGISTER_PARAMS	8
-#elif defined(__arc__)
-#define NR_REGISTER_PARAMS	8
-#else
-#define NR_REGISTER_PARAMS      0
-#endif
+static int arch__nr_register_params(const GElf_Ehdr *ehdr)
+{
+	switch (ehdr->e_machine) {
+	case EM_S390:	 return 5;
+	case EM_SPARC:
+	case EM_SPARCV9:
+	case EM_X86_64:	 return 6;
+	case EM_AARCH64:
+	case EM_ARC:
+	case EM_ARM:
+	case EM_MIPS:
+	case EM_PPC:
+	case EM_PPC64:
+	case EM_RISCV:	 return 8;
+	default:	 break;
+	}
+
+	return 0;
+}
 
 static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 					struct conf_load *conf, int param_idx)
@@ -1031,7 +1031,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 		tag__init(&parm->tag, cu, die);
 		parm->name = attr_string(die, DW_AT_name, conf);
 
-		if (param_idx >= NR_REGISTER_PARAMS)
+		if (param_idx >= cu->nr_register_params)
 			return parm;
 		/* Parameters which use DW_AT_abstract_origin to point at
 		 * the original parameter definition (with no name in the DIE)
@@ -2870,6 +2870,7 @@ static int cu__set_common(struct cu *cu, struct conf_load *conf,
 		return DWARF_CB_ABORT;
 
 	cu->little_endian = ehdr.e_ident[EI_DATA] == ELFDATA2LSB;
+	cu->nr_register_params = arch__nr_register_params(&ehdr);
 	return 0;
 }
 
diff --git a/dwarves.h b/dwarves.h
index fd1ca3ae9f4ab531..ddf56f0124e0ec03 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -262,6 +262,7 @@ struct cu {
 	uint8_t		 has_addr_info:1;
 	uint8_t		 uses_global_strings:1;
 	uint8_t		 little_endian:1;
+	uint8_t		 nr_register_params;
 	uint16_t	 language;
 	unsigned long	 nr_inline_expansions;
 	size_t		 size_inline_expansions;

Arnaldo Carvalho de Melo Jan. 30, 2023, 8:23 p.m. UTC | #3

Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 30, 2023 at 03:36:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > +#define NR_REGISTER_PARAMS	8
> > > +#elif defined(__arc__)
> > > +#define NR_REGISTER_PARAMS	8
> > > +#else
> > > +#define NR_REGISTER_PARAMS      0
> > > +#endif
> > 
> > This should be done as a function, something like:
> > 
> > int cu__nr_register_params(struct cu *cu)
> > {
> > 	GElf_Ehdr ehdr;
> > 
> > 	gelf_getehdr(cu->elf, &ehdr);
> > 
> > 	switch (ehdr.machine) {
> > 	...
> > 
> > }
> > 
> > I'm coding that now, will send the diff shortly.
> > 
> > This is to support cross-builds.
> 
> I made this change to this patch, please check.

And added this to that cset:

Committer notes:

Changed the NR_REGISTER_PARAMS definition from a if/elif/endif for the
native architecture into a function that uses the ELF header e_machine
to find the target architecture, to allow for cross builds. 

---

- Arnaldo

> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 752a3c1afc4494f2..81963e71715c8435 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -994,29 +994,29 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
>  
>  /* How many function parameters are passed via registers?  Used below in
>   * determining if an argument has been optimized out or if it is simply
> - * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
> - * allows unsupported architectures to skip tagging optimized-out
> + * an argument > cu__nr_register_params().  Making cu__nr_register_params()
> + * return 0 allows unsupported architectures to skip tagging optimized-out
>   * values.
>   */
> -#if defined(__x86_64__)
> -#define NR_REGISTER_PARAMS      6
> -#elif defined(__s390__)
> -#define NR_REGISTER_PARAMS	5
> -#elif defined(__aarch64__)
> -#define NR_REGISTER_PARAMS      8
> -#elif defined(__mips__)
> -#define NR_REGISTER_PARAMS	8
> -#elif defined(__powerpc__)
> -#define NR_REGISTER_PARAMS	8
> -#elif defined(__sparc__)
> -#define NR_REGISTER_PARAMS	6
> -#elif defined(__riscv) && __riscv_xlen == 64
> -#define NR_REGISTER_PARAMS	8
> -#elif defined(__arc__)
> -#define NR_REGISTER_PARAMS	8
> -#else
> -#define NR_REGISTER_PARAMS      0
> -#endif
> +static int arch__nr_register_params(const GElf_Ehdr *ehdr)
> +{
> +	switch (ehdr->e_machine) {
> +	case EM_S390:	 return 5;
> +	case EM_SPARC:
> +	case EM_SPARCV9:
> +	case EM_X86_64:	 return 6;
> +	case EM_AARCH64:
> +	case EM_ARC:
> +	case EM_ARM:
> +	case EM_MIPS:
> +	case EM_PPC:
> +	case EM_PPC64:
> +	case EM_RISCV:	 return 8;
> +	default:	 break;
> +	}
> +
> +	return 0;
> +}
>  
>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  					struct conf_load *conf, int param_idx)
> @@ -1031,7 +1031,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  		tag__init(&parm->tag, cu, die);
>  		parm->name = attr_string(die, DW_AT_name, conf);
>  
> -		if (param_idx >= NR_REGISTER_PARAMS)
> +		if (param_idx >= cu->nr_register_params)
>  			return parm;
>  		/* Parameters which use DW_AT_abstract_origin to point at
>  		 * the original parameter definition (with no name in the DIE)
> @@ -2870,6 +2870,7 @@ static int cu__set_common(struct cu *cu, struct conf_load *conf,
>  		return DWARF_CB_ABORT;
>  
>  	cu->little_endian = ehdr.e_ident[EI_DATA] == ELFDATA2LSB;
> +	cu->nr_register_params = arch__nr_register_params(&ehdr);
>  	return 0;
>  }
>  
> diff --git a/dwarves.h b/dwarves.h
> index fd1ca3ae9f4ab531..ddf56f0124e0ec03 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -262,6 +262,7 @@ struct cu {
>  	uint8_t		 has_addr_info:1;
>  	uint8_t		 uses_global_strings:1;
>  	uint8_t		 little_endian:1;
> +	uint8_t		 nr_register_params;
>  	uint16_t	 language;
>  	unsigned long	 nr_inline_expansions;
>  	size_t		 size_inline_expansions;

Alan Maguire Jan. 30, 2023, 10:37 p.m. UTC | #4

On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Jan 30, 2023 at 03:36:09PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>> +#define NR_REGISTER_PARAMS	8
>>>> +#elif defined(__arc__)
>>>> +#define NR_REGISTER_PARAMS	8
>>>> +#else
>>>> +#define NR_REGISTER_PARAMS      0
>>>> +#endif
>>>
>>> This should be done as a function, something like:
>>>
>>> int cu__nr_register_params(struct cu *cu)
>>> {
>>> 	GElf_Ehdr ehdr;
>>>
>>> 	gelf_getehdr(cu->elf, &ehdr);
>>>
>>> 	switch (ehdr.machine) {
>>> 	...
>>>
>>> }
>>>
>>> I'm coding that now, will send the diff shortly.
>>>
>>> This is to support cross-builds.
>>
>> I made this change to this patch, please check.
> 
> And added this to that cset:
> 
> Committer notes:
> 
> Changed the NR_REGISTER_PARAMS definition from a if/elif/endif for the
> native architecture into a function that uses the ELF header e_machine
> to find the target architecture, to allow for cross builds. 
> 
> ---
> 
> - Arnaldo
> 
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index 752a3c1afc4494f2..81963e71715c8435 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -994,29 +994,29 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
>>  
>>  /* How many function parameters are passed via registers?  Used below in
>>   * determining if an argument has been optimized out or if it is simply
>> - * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
>> - * allows unsupported architectures to skip tagging optimized-out
>> + * an argument > cu__nr_register_params().  Making cu__nr_register_params()
>> + * return 0 allows unsupported architectures to skip tagging optimized-out
>>   * values.
>>   */
>> -#if defined(__x86_64__)
>> -#define NR_REGISTER_PARAMS      6
>> -#elif defined(__s390__)
>> -#define NR_REGISTER_PARAMS	5
>> -#elif defined(__aarch64__)
>> -#define NR_REGISTER_PARAMS      8
>> -#elif defined(__mips__)
>> -#define NR_REGISTER_PARAMS	8
>> -#elif defined(__powerpc__)
>> -#define NR_REGISTER_PARAMS	8
>> -#elif defined(__sparc__)
>> -#define NR_REGISTER_PARAMS	6
>> -#elif defined(__riscv) && __riscv_xlen == 64
>> -#define NR_REGISTER_PARAMS	8
>> -#elif defined(__arc__)
>> -#define NR_REGISTER_PARAMS	8
>> -#else
>> -#define NR_REGISTER_PARAMS      0
>> -#endif
>> +static int arch__nr_register_params(const GElf_Ehdr *ehdr)
>> +{
>> +	switch (ehdr->e_machine) {
>> +	case EM_S390:	 return 5;
>> +	case EM_SPARC:
>> +	case EM_SPARCV9:
>> +	case EM_X86_64:	 return 6;
>> +	case EM_AARCH64:
>> +	case EM_ARC:
>> +	case EM_ARM:
>> +	case EM_MIPS:
>> +	case EM_PPC:
>> +	case EM_PPC64:
>> +	case EM_RISCV:	 return 8;
>> +	default:	 break;
>> +	}
>> +
>> +	return 0;
>> +}
>>  
>>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>  					struct conf_load *conf, int param_idx)
>> @@ -1031,7 +1031,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>  		tag__init(&parm->tag, cu, die);
>>  		parm->name = attr_string(die, DW_AT_name, conf);
>>  
>> -		if (param_idx >= NR_REGISTER_PARAMS)
>> +		if (param_idx >= cu->nr_register_params)
>>  			return parm;
>>  		/* Parameters which use DW_AT_abstract_origin to point at
>>  		 * the original parameter definition (with no name in the DIE)
>> @@ -2870,6 +2870,7 @@ static int cu__set_common(struct cu *cu, struct conf_load *conf,
>>  		return DWARF_CB_ABORT;
>>  
>>  	cu->little_endian = ehdr.e_ident[EI_DATA] == ELFDATA2LSB;
>> +	cu->nr_register_params = arch__nr_register_params(&ehdr);
>>  	return 0;
>>  }
>>  
>> diff --git a/dwarves.h b/dwarves.h
>> index fd1ca3ae9f4ab531..ddf56f0124e0ec03 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -262,6 +262,7 @@ struct cu {
>>  	uint8_t		 has_addr_info:1;
>>  	uint8_t		 uses_global_strings:1;
>>  	uint8_t		 little_endian:1;
>> +	uint8_t		 nr_register_params;
>>  	uint16_t	 language;
>>  	unsigned long	 nr_inline_expansions;
>>  	size_t		 size_inline_expansions;
> 

Thanks for this, never thought of cross-builds to be honest!
Tested just now on x86_64 and aarch64 at my end, just ran
into one small thing on one system; turns out EM_RISCV isn't
defined if using a very old elf.h; below works around this
(dwarves otherwise builds fine on this system).

diff --git a/dwarf_loader.c b/dwarf_loader.c
index dba2d37..47a3bc2 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -992,6 +992,11 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *c
        return member;
 }
 
+/* for older elf.h */
+#ifndef EM_RISCV
+#define EM_RISCV       243
+#endif
+
 /* How many function parameters are passed via registers?  Used below in
  * determining if an argument has been optimized out or if it is simply
  * an argument > cu__nr_register_params().  Making cu__nr_register_params()

Arnaldo Carvalho de Melo Jan. 31, 2023, 12:25 a.m. UTC | #5

Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> +++ b/dwarves.h
> >> @@ -262,6 +262,7 @@ struct cu {
> >>  	uint8_t		 has_addr_info:1;
> >>  	uint8_t		 uses_global_strings:1;
> >>  	uint8_t		 little_endian:1;
> >> +	uint8_t		 nr_register_params;
> >>  	uint16_t	 language;
> >>  	unsigned long	 nr_inline_expansions;
> >>  	size_t		 size_inline_expansions;
> > 
 
> Thanks for this, never thought of cross-builds to be honest!

> Tested just now on x86_64 and aarch64 at my end, just ran
> into one small thing on one system; turns out EM_RISCV isn't
> defined if using a very old elf.h; below works around this
> (dwarves otherwise builds fine on this system).

Ok, will add it and will test with containers for older distros too.

- Arnaldo
 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index dba2d37..47a3bc2 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -992,6 +992,11 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *c
>         return member;
>  }
>  
> +/* for older elf.h */
> +#ifndef EM_RISCV
> +#define EM_RISCV       243
> +#endif
> +
>  /* How many function parameters are passed via registers?  Used below in
>   * determining if an argument has been optimized out or if it is simply
>   * an argument > cu__nr_register_params().  Making cu__nr_register_params()

Arnaldo Carvalho de Melo Jan. 31, 2023, 1:04 a.m. UTC | #6

Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >> +++ b/dwarves.h
> > >> @@ -262,6 +262,7 @@ struct cu {
> > >>  	uint8_t		 has_addr_info:1;
> > >>  	uint8_t		 uses_global_strings:1;
> > >>  	uint8_t		 little_endian:1;
> > >> +	uint8_t		 nr_register_params;
> > >>  	uint16_t	 language;
> > >>  	unsigned long	 nr_inline_expansions;
> > >>  	size_t		 size_inline_expansions;
> > > 
>  
> > Thanks for this, never thought of cross-builds to be honest!
> 
> > Tested just now on x86_64 and aarch64 at my end, just ran
> > into one small thing on one system; turns out EM_RISCV isn't
> > defined if using a very old elf.h; below works around this
> > (dwarves otherwise builds fine on this system).
> 
> Ok, will add it and will test with containers for older distros too.

Its on the 'next' branch, so that it gets tested in the libbpf github
repo at:

https://github.com/libbpf/libbpf/actions/workflows/pahole.yml

It failed yesterday and today due to problems with the installation of
llvm, probably tomorrow it'll be back working as I saw some
notifications floating by.

I added the conditional EM_RISCV definition as well as removed the dup
iterator that Jiri noticed.

Thanks,

- Arnaldo
 
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index dba2d37..47a3bc2 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -992,6 +992,11 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *c
> >         return member;
> >  }
> >  
> > +/* for older elf.h */
> > +#ifndef EM_RISCV
> > +#define EM_RISCV       243
> > +#endif
> > +
> >  /* How many function parameters are passed via registers?  Used below in
> >   * determining if an argument has been optimized out or if it is simply
> >   * an argument > cu__nr_register_params().  Making cu__nr_register_params()
> 
> -- 
> 
> - Arnaldo

Alan Maguire Jan. 31, 2023, 12:14 p.m. UTC | #7

On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>> +++ b/dwarves.h
>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>  	uint8_t		 has_addr_info:1;
>>>>>  	uint8_t		 uses_global_strings:1;
>>>>>  	uint8_t		 little_endian:1;
>>>>> +	uint8_t		 nr_register_params;
>>>>>  	uint16_t	 language;
>>>>>  	unsigned long	 nr_inline_expansions;
>>>>>  	size_t		 size_inline_expansions;
>>>>
>>  
>>> Thanks for this, never thought of cross-builds to be honest!
>>
>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>> into one small thing on one system; turns out EM_RISCV isn't
>>> defined if using a very old elf.h; below works around this
>>> (dwarves otherwise builds fine on this system).
>>
>> Ok, will add it and will test with containers for older distros too.
> 
> Its on the 'next' branch, so that it gets tested in the libbpf github
> repo at:
> 
> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> 
> It failed yesterday and today due to problems with the installation of
> llvm, probably tomorrow it'll be back working as I saw some
> notifications floating by.
> 
> I added the conditional EM_RISCV definition as well as removed the dup
> iterator that Jiri noticed.
>

Thanks again Arnaldo! I've hit an issue with this series in
BTF encoding of kfuncs; specifically we see some kfuncs missing
from the BTF representation, and as a result:

WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_ct_change_status

Not sure why I didn't notice this previously.

The problem is the DWARF - and therefore BTF - generated for a function like

int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
{
        return -EOPNOTSUPP;
}

looks like this:

   <8af83a2>   DW_AT_external    : 1
    <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
    <8af83a6>   DW_AT_decl_file   : 5
    <8af83a7>   DW_AT_decl_line   : 737
    <8af83a9>   DW_AT_decl_column : 5
    <8af83aa>   DW_AT_prototyped  : 1
    <8af83aa>   DW_AT_type        : <0x8ad8547>
    <8af83ae>   DW_AT_sibling     : <0x8af83cd>
 <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
    <8af83b3>   DW_AT_name        : ctx
    <8af83b7>   DW_AT_decl_file   : 5
    <8af83b8>   DW_AT_decl_line   : 737
    <8af83ba>   DW_AT_decl_column : 51
    <8af83bb>   DW_AT_type        : <0x8af421d>
 <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
    <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
    <8af83c4>   DW_AT_decl_file   : 5
    <8af83c5>   DW_AT_decl_line   : 737
    <8af83c7>   DW_AT_decl_column : 61
    <8af83c8>   DW_AT_type        : <0x8adc424>

...and because there are no further abstract origin references
with location information either, we classify it as lacking 
locations for (some of) the parameters, and as a result
we skip BTF encoding. We can work around that by doing this:

__attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
{
	return -EOPNOTSUPP;
}

Should we #define some kind of "kfunc" prefix equivalent to the
above to handle these cases in include/linux/bpf.h perhaps?
If that makes sense, I'll send bpf-next patches to cover the
set of kfuncs.

The other thing we might want to do is bump the libbpf version
for dwarves 1.25, what do you think? I've tested with libbpf 1.1
and aside from the above issue all looks good (there's a few dedup
improvements that this version will give us). I can send a patch for
the libbpf update if that makes sense.

Thanks!

Alan
 
> Thanks,
> 
> - Arnaldo
>  
>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>> index dba2d37..47a3bc2 100644
>>> --- a/dwarf_loader.c
>>> +++ b/dwarf_loader.c
>>> @@ -992,6 +992,11 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *c
>>>         return member;
>>>  }
>>>  
>>> +/* for older elf.h */
>>> +#ifndef EM_RISCV
>>> +#define EM_RISCV       243
>>> +#endif
>>> +
>>>  /* How many function parameters are passed via registers?  Used below in
>>>   * determining if an argument has been optimized out or if it is simply
>>>   * an argument > cu__nr_register_params().  Making cu__nr_register_params()
>>
>> -- 
>>
>> - Arnaldo
>

Arnaldo Carvalho de Melo Jan. 31, 2023, 12:33 p.m. UTC | #8

On January 31, 2023 9:14:05 AM GMT-03:00, Alan Maguire <[e-mail]> wrote:
>On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>> +++ b/dwarves.h
>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>  	uint8_t		 has_addr_info:1;
>>>>>>  	uint8_t		 uses_global_strings:1;
>>>>>>  	uint8_t		 little_endian:1;
>>>>>> +	uint8_t		 nr_register_params;
>>>>>>  	uint16_t	 language;
>>>>>>  	unsigned long	 nr_inline_expansions;
>>>>>>  	size_t		 size_inline_expansions;
>>>>>
>>>  
>>>> Thanks for this, never thought of cross-builds to be honest!
>>>
>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>> defined if using a very old elf.h; below works around this
>>>> (dwarves otherwise builds fine on this system).
>>>
>>> Ok, will add it and will test with containers for older distros too.
>> 
>> Its on the 'next' branch, so that it gets tested in the libbpf github
>> repo at:
>> 
>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>> 
>> It failed yesterday and today due to problems with the installation of
>> llvm, probably tomorrow it'll be back working as I saw some
>> notifications floating by.
>> 
>> I added the conditional EM_RISCV definition as well as removed the dup
>> iterator that Jiri noticed.
>>
>
>Thanks again Arnaldo! I've hit an issue with this series in
>BTF encoding of kfuncs; specifically we see some kfuncs missing
>from the BTF representation, and as a result:
>
>WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>
>Not sure why I didn't notice this previously.
>
>The problem is the DWARF - and therefore BTF - generated for a function like
>
>int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>{
>        return -EOPNOTSUPP;
>}
>
>looks like this:
>
>   <8af83a2>   DW_AT_external    : 1
>    <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>    <8af83a6>   DW_AT_decl_file   : 5
>    <8af83a7>   DW_AT_decl_line   : 737
>    <8af83a9>   DW_AT_decl_column : 5
>    <8af83aa>   DW_AT_prototyped  : 1
>    <8af83aa>   DW_AT_type        : <0x8ad8547>
>    <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>    <8af83b3>   DW_AT_name        : ctx
>    <8af83b7>   DW_AT_decl_file   : 5
>    <8af83b8>   DW_AT_decl_line   : 737
>    <8af83ba>   DW_AT_decl_column : 51
>    <8af83bb>   DW_AT_type        : <0x8af421d>
> <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>    <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>    <8af83c4>   DW_AT_decl_file   : 5
>    <8af83c5>   DW_AT_decl_line   : 737
>    <8af83c7>   DW_AT_decl_column : 61
>    <8af83c8>   DW_AT_type        : <0x8adc424>
>
>...and because there are no further abstract origin references
>with location information either, we classify it as lacking 
>locations for (some of) the parameters, and as a result
>we skip BTF encoding. We can work around that by doing this:
>
>__attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>{
>	return -EOPNOTSUPP;
>}
>
>Should we #define some kind of "kfunc" prefix equivalent to the
>above to handle these cases in include/linux/bpf.h perhaps?
>If that makes sense, I'll send bpf-next patches to cover the
>set of kfuncs.

Jiri?

>The other thing we might want to do is bump the libbpf version
>for dwarves 1.25, what do you think? I've tested with libbpf 1.1
>and aside from the above issue all looks good (there's a few dedup
>improvements that this version will give us). I can send a patch for
>the libbpf update if that makes sense.


Please send it, then we give it some more days of wider testing,

Yonghong, Andrii, comments on updating libbpf in the pahole submodule?

- Arnaldo

Jiri Olsa Jan. 31, 2023, 1:35 p.m. UTC | #9

On Tue, Jan 31, 2023 at 09:33:49AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> 
> On January 31, 2023 9:14:05 AM GMT-03:00, Alan Maguire <[e-mail]> wrote:
> >On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> >> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> >>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> >>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>>> +++ b/dwarves.h
> >>>>>> @@ -262,6 +262,7 @@ struct cu {
> >>>>>>  	uint8_t		 has_addr_info:1;
> >>>>>>  	uint8_t		 uses_global_strings:1;
> >>>>>>  	uint8_t		 little_endian:1;
> >>>>>> +	uint8_t		 nr_register_params;
> >>>>>>  	uint16_t	 language;
> >>>>>>  	unsigned long	 nr_inline_expansions;
> >>>>>>  	size_t		 size_inline_expansions;
> >>>>>
> >>>  
> >>>> Thanks for this, never thought of cross-builds to be honest!
> >>>
> >>>> Tested just now on x86_64 and aarch64 at my end, just ran
> >>>> into one small thing on one system; turns out EM_RISCV isn't
> >>>> defined if using a very old elf.h; below works around this
> >>>> (dwarves otherwise builds fine on this system).
> >>>
> >>> Ok, will add it and will test with containers for older distros too.
> >> 
> >> Its on the 'next' branch, so that it gets tested in the libbpf github
> >> repo at:
> >> 
> >> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> >> 
> >> It failed yesterday and today due to problems with the installation of
> >> llvm, probably tomorrow it'll be back working as I saw some
> >> notifications floating by.
> >> 
> >> I added the conditional EM_RISCV definition as well as removed the dup
> >> iterator that Jiri noticed.
> >>
> >
> >Thanks again Arnaldo! I've hit an issue with this series in
> >BTF encoding of kfuncs; specifically we see some kfuncs missing
> >from the BTF representation, and as a result:
> >
> >WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> >WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> >WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> >
> >Not sure why I didn't notice this previously.
> >
> >The problem is the DWARF - and therefore BTF - generated for a function like
> >
> >int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >{
> >        return -EOPNOTSUPP;
> >}
> >
> >looks like this:
> >
> >   <8af83a2>   DW_AT_external    : 1
> >    <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> >    <8af83a6>   DW_AT_decl_file   : 5
> >    <8af83a7>   DW_AT_decl_line   : 737
> >    <8af83a9>   DW_AT_decl_column : 5
> >    <8af83aa>   DW_AT_prototyped  : 1
> >    <8af83aa>   DW_AT_type        : <0x8ad8547>
> >    <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> >    <8af83b3>   DW_AT_name        : ctx
> >    <8af83b7>   DW_AT_decl_file   : 5
> >    <8af83b8>   DW_AT_decl_line   : 737
> >    <8af83ba>   DW_AT_decl_column : 51
> >    <8af83bb>   DW_AT_type        : <0x8af421d>
> > <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> >    <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> >    <8af83c4>   DW_AT_decl_file   : 5
> >    <8af83c5>   DW_AT_decl_line   : 737
> >    <8af83c7>   DW_AT_decl_column : 61
> >    <8af83c8>   DW_AT_type        : <0x8adc424>
> >
> >...and because there are no further abstract origin references
> >with location information either, we classify it as lacking 
> >locations for (some of) the parameters, and as a result
> >we skip BTF encoding. We can work around that by doing this:
> >
> >__attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >{
> >	return -EOPNOTSUPP;
> >}
> >
> >Should we #define some kind of "kfunc" prefix equivalent to the
> >above to handle these cases in include/linux/bpf.h perhaps?
> >If that makes sense, I'll send bpf-next patches to cover the
> >set of kfuncs.
> 
> Jiri?

hum I wonder what's the point of the kfunc if it returns -EOPNOTSUPP,
at least I can't see any other version of it.. maybe some temporary
stuff like for bpf_task_kptr_get

but I think it's good idea to make sure it does not get optimized out,
so some kfunc macro seems like good idea.. or maybe we could use
also declaration tag for kfuncs

David already send some patchset for BPF_KFUNC macro, could be part of
that

https://lore.kernel.org/bpf/[e-mail]/

jirka

> 
> >The other thing we might want to do is bump the libbpf version
> >for dwarves 1.25, what do you think? I've tested with libbpf 1.1
> >and aside from the above issue all looks good (there's a few dedup
> >improvements that this version will give us). I can send a patch for
> >the libbpf update if that makes sense.
> 
> 
> Please send it, then we give it some more days of wider testing,
> 
> Yonghong, Andrii, comments on updating libbpf in the pahole submodule?
> 
> - Arnaldo

Alexei Starovoitov Jan. 31, 2023, 5:43 p.m. UTC | #10

On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>
> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> >>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> >>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>> +++ b/dwarves.h
> >>>>> @@ -262,6 +262,7 @@ struct cu {
> >>>>>   uint8_t          has_addr_info:1;
> >>>>>   uint8_t          uses_global_strings:1;
> >>>>>   uint8_t          little_endian:1;
> >>>>> + uint8_t          nr_register_params;
> >>>>>   uint16_t         language;
> >>>>>   unsigned long    nr_inline_expansions;
> >>>>>   size_t           size_inline_expansions;
> >>>>
> >>
> >>> Thanks for this, never thought of cross-builds to be honest!
> >>
> >>> Tested just now on x86_64 and aarch64 at my end, just ran
> >>> into one small thing on one system; turns out EM_RISCV isn't
> >>> defined if using a very old elf.h; below works around this
> >>> (dwarves otherwise builds fine on this system).
> >>
> >> Ok, will add it and will test with containers for older distros too.
> >
> > Its on the 'next' branch, so that it gets tested in the libbpf github
> > repo at:
> >
> > https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> >
> > It failed yesterday and today due to problems with the installation of
> > llvm, probably tomorrow it'll be back working as I saw some
> > notifications floating by.
> >
> > I added the conditional EM_RISCV definition as well as removed the dup
> > iterator that Jiri noticed.
> >
>
> Thanks again Arnaldo! I've hit an issue with this series in
> BTF encoding of kfuncs; specifically we see some kfuncs missing
> from the BTF representation, and as a result:
>
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>
> Not sure why I didn't notice this previously.
>
> The problem is the DWARF - and therefore BTF - generated for a function like
>
> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> {
>         return -EOPNOTSUPP;
> }
>
> looks like this:
>
>    <8af83a2>   DW_AT_external    : 1
>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>     <8af83a6>   DW_AT_decl_file   : 5
>     <8af83a7>   DW_AT_decl_line   : 737
>     <8af83a9>   DW_AT_decl_column : 5
>     <8af83aa>   DW_AT_prototyped  : 1
>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>     <8af83b3>   DW_AT_name        : ctx
>     <8af83b7>   DW_AT_decl_file   : 5
>     <8af83b8>   DW_AT_decl_line   : 737
>     <8af83ba>   DW_AT_decl_column : 51
>     <8af83bb>   DW_AT_type        : <0x8af421d>
>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>     <8af83c4>   DW_AT_decl_file   : 5
>     <8af83c5>   DW_AT_decl_line   : 737
>     <8af83c7>   DW_AT_decl_column : 61
>     <8af83c8>   DW_AT_type        : <0x8adc424>
>
> ...and because there are no further abstract origin references
> with location information either, we classify it as lacking
> locations for (some of) the parameters, and as a result
> we skip BTF encoding. We can work around that by doing this:
>
> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)

replied in the other thread. This attr is broken and discouraged by gcc.

For kfuncs where aregs are unused, please try __used and __may_unused
applied to arguments.
If that won't work, please add barrier_var(arg) to the body of kfunc
the way we do in selftests.

> {
>         return -EOPNOTSUPP;
> }
>
> Should we #define some kind of "kfunc" prefix equivalent to the
> above to handle these cases in include/linux/bpf.h perhaps?
> If that makes sense, I'll send bpf-next patches to cover the
> set of kfuncs.
>
> The other thing we might want to do is bump the libbpf version
> for dwarves 1.25, what do you think? I've tested with libbpf 1.1
> and aside from the above issue all looks good (there's a few dedup
> improvements that this version will give us). I can send a patch for
> the libbpf update if that makes sense.

Alexei Starovoitov Jan. 31, 2023, 6:16 p.m. UTC | #11

On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> >
> > On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > >>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > >>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>>>> +++ b/dwarves.h
> > >>>>> @@ -262,6 +262,7 @@ struct cu {
> > >>>>>   uint8_t          has_addr_info:1;
> > >>>>>   uint8_t          uses_global_strings:1;
> > >>>>>   uint8_t          little_endian:1;
> > >>>>> + uint8_t          nr_register_params;
> > >>>>>   uint16_t         language;
> > >>>>>   unsigned long    nr_inline_expansions;
> > >>>>>   size_t           size_inline_expansions;
> > >>>>
> > >>
> > >>> Thanks for this, never thought of cross-builds to be honest!
> > >>
> > >>> Tested just now on x86_64 and aarch64 at my end, just ran
> > >>> into one small thing on one system; turns out EM_RISCV isn't
> > >>> defined if using a very old elf.h; below works around this
> > >>> (dwarves otherwise builds fine on this system).
> > >>
> > >> Ok, will add it and will test with containers for older distros too.
> > >
> > > Its on the 'next' branch, so that it gets tested in the libbpf github
> > > repo at:
> > >
> > > https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > >
> > > It failed yesterday and today due to problems with the installation of
> > > llvm, probably tomorrow it'll be back working as I saw some
> > > notifications floating by.
> > >
> > > I added the conditional EM_RISCV definition as well as removed the dup
> > > iterator that Jiri noticed.
> > >
> >
> > Thanks again Arnaldo! I've hit an issue with this series in
> > BTF encoding of kfuncs; specifically we see some kfuncs missing
> > from the BTF representation, and as a result:
> >
> > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> >
> > Not sure why I didn't notice this previously.
> >
> > The problem is the DWARF - and therefore BTF - generated for a function like
> >
> > int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > {
> >         return -EOPNOTSUPP;
> > }
> >
> > looks like this:
> >
> >    <8af83a2>   DW_AT_external    : 1
> >     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> >     <8af83a6>   DW_AT_decl_file   : 5
> >     <8af83a7>   DW_AT_decl_line   : 737
> >     <8af83a9>   DW_AT_decl_column : 5
> >     <8af83aa>   DW_AT_prototyped  : 1
> >     <8af83aa>   DW_AT_type        : <0x8ad8547>
> >     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> >  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> >     <8af83b3>   DW_AT_name        : ctx
> >     <8af83b7>   DW_AT_decl_file   : 5
> >     <8af83b8>   DW_AT_decl_line   : 737
> >     <8af83ba>   DW_AT_decl_column : 51
> >     <8af83bb>   DW_AT_type        : <0x8af421d>
> >  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> >     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> >     <8af83c4>   DW_AT_decl_file   : 5
> >     <8af83c5>   DW_AT_decl_line   : 737
> >     <8af83c7>   DW_AT_decl_column : 61
> >     <8af83c8>   DW_AT_type        : <0x8adc424>
> >
> > ...and because there are no further abstract origin references
> > with location information either, we classify it as lacking
> > locations for (some of) the parameters, and as a result
> > we skip BTF encoding. We can work around that by doing this:
> >
> > __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>
> replied in the other thread. This attr is broken and discouraged by gcc.
>
> For kfuncs where aregs are unused, please try __used and __may_unused
> applied to arguments.
> If that won't work, please add barrier_var(arg) to the body of kfunc
> the way we do in selftests.

There is also
# define __visible __attribute__((__externally_visible__))
that probably fits the best here.

Alan Maguire Jan. 31, 2023, 11:45 p.m. UTC | #12

On 31/01/2023 18:16, Alexei Starovoitov wrote:
> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> <[e-mail]> wrote:
>>
>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>
>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>> +++ b/dwarves.h
>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>   uint16_t         language;
>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>
>>>>>
>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>
>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>> defined if using a very old elf.h; below works around this
>>>>>> (dwarves otherwise builds fine on this system).
>>>>>
>>>>> Ok, will add it and will test with containers for older distros too.
>>>>
>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>> repo at:
>>>>
>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>
>>>> It failed yesterday and today due to problems with the installation of
>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>> notifications floating by.
>>>>
>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>> iterator that Jiri noticed.
>>>>
>>>
>>> Thanks again Arnaldo! I've hit an issue with this series in
>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>> from the BTF representation, and as a result:
>>>
>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>
>>> Not sure why I didn't notice this previously.
>>>
>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>
>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>> {
>>>         return -EOPNOTSUPP;
>>> }
>>>
>>> looks like this:
>>>
>>>    <8af83a2>   DW_AT_external    : 1
>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>     <8af83a9>   DW_AT_decl_column : 5
>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>     <8af83b3>   DW_AT_name        : ctx
>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>     <8af83ba>   DW_AT_decl_column : 51
>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>     <8af83c7>   DW_AT_decl_column : 61
>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>
>>> ...and because there are no further abstract origin references
>>> with location information either, we classify it as lacking
>>> locations for (some of) the parameters, and as a result
>>> we skip BTF encoding. We can work around that by doing this:
>>>
>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>
>> replied in the other thread. This attr is broken and discouraged by gcc.
>>
>> For kfuncs where aregs are unused, please try __used and __may_unused
>> applied to arguments.
>> If that won't work, please add barrier_var(arg) to the body of kfunc
>> the way we do in selftests.
> 
> There is also
> # define __visible __attribute__((__externally_visible__))
> that probably fits the best here.
> 

testing thus for seems to show that for x86_64, David's series
(using __used noinline in the BPF_KFUNC() wrapper and extended
to cover recently-arrived kfuncs like cpumask) is sufficient
to avoid resolve_btfids warnings.

We need to update the LSM_HOOK() definition for BPF LSM too,
otherwise they will cause problems with missing btfids also.

With all that done, I'm not seeing resolve_btfids complaints
for x86_64 (tested gcc9,11). I also tried using __visible, but
using that in the kfunc wrapper causes problems for the static tcp 
congestion control functions. We see warnings like these if __visible
is used in BPF_KFUNC():

net/ipv4/tcp_dctcp.c:79:1: warning: ‘externally_visible’ attribute have effect only on public objects [-Wattributes]
   79 | {

However, for aarch64 with the same changes we see a bunch of complaints
from resolve_btfids for BPF_KFUNC()-wrapped kfuncs and LSM hooks:

  BTFIDS  vmlinux
WARN: resolve_btfids: unresolved symbol tcp_cong_avoid_ai
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
WARN: resolve_btfids: unresolved symbol bpf_lsm_xfrm_state_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_xfrm_policy_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_tun_dev_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_task_to_inode
WARN: resolve_btfids: unresolved symbol bpf_lsm_task_getsecid_obj
WARN: resolve_btfids: unresolved symbol bpf_lsm_task_free
WARN: resolve_btfids: unresolved symbol bpf_lsm_sock_graft
WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_getsecid
WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_clone_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_shm_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_sem_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_sctp_sk_clone
WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_free_mnt_opts
WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_delete
WARN: resolve_btfids: unresolved symbol bpf_lsm_req_classify_flow
WARN: resolve_btfids: unresolved symbol bpf_lsm_release_secctx
WARN: resolve_btfids: unresolved symbol bpf_lsm_perf_event_free
WARN: resolve_btfids: unresolved symbol bpf_lsm_msg_queue_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_msg_msg_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_key_free
WARN: resolve_btfids: unresolved symbol bpf_lsm_ipc_getsecid
WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_post_setxattr
WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_invalidate_secctx
WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_getsecid
WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_inet_csk_clone
WARN: resolve_btfids: unresolved symbol bpf_lsm_inet_conn_established
WARN: resolve_btfids: unresolved symbol bpf_lsm_ib_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_file_set_fowner
WARN: resolve_btfids: unresolved symbol bpf_lsm_file_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_d_instantiate
WARN: resolve_btfids: unresolved symbol bpf_lsm_current_getsecid_subj
WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_transfer
WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_getsecid
WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_free
WARN: resolve_btfids: unresolved symbol bpf_lsm_bprm_committing_creds
WARN: resolve_btfids: unresolved symbol bpf_lsm_bprm_committed_creds
WARN: resolve_btfids: unresolved symbol bpf_lsm_bpf_prog_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_bpf_map_free_security
WARN: resolve_btfids: unresolved symbol bpf_lsm_audit_rule_free
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test3
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release
WARN: resolve_btfids: unresolved symbol bpf_cpumask_any
WARN: resolve_btfids: unresolved symbol bpf_cgroup_acquire
WARN: resolve_btfids: unresolved symbol bpf_cast_to_kern_ctx
  NM      System.map

Thanks!

Alan

David Vernet Jan. 31, 2023, 11:58 p.m. UTC | #13

On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > <[e-mail]> wrote:
> >>
> >> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> >>>
> >>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> >>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> >>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> >>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>>>>> +++ b/dwarves.h
> >>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> >>>>>>>>   uint8_t          has_addr_info:1;
> >>>>>>>>   uint8_t          uses_global_strings:1;
> >>>>>>>>   uint8_t          little_endian:1;
> >>>>>>>> + uint8_t          nr_register_params;
> >>>>>>>>   uint16_t         language;
> >>>>>>>>   unsigned long    nr_inline_expansions;
> >>>>>>>>   size_t           size_inline_expansions;
> >>>>>>>
> >>>>>
> >>>>>> Thanks for this, never thought of cross-builds to be honest!
> >>>>>
> >>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> >>>>>> into one small thing on one system; turns out EM_RISCV isn't
> >>>>>> defined if using a very old elf.h; below works around this
> >>>>>> (dwarves otherwise builds fine on this system).
> >>>>>
> >>>>> Ok, will add it and will test with containers for older distros too.
> >>>>
> >>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> >>>> repo at:
> >>>>
> >>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> >>>>
> >>>> It failed yesterday and today due to problems with the installation of
> >>>> llvm, probably tomorrow it'll be back working as I saw some
> >>>> notifications floating by.
> >>>>
> >>>> I added the conditional EM_RISCV definition as well as removed the dup
> >>>> iterator that Jiri noticed.
> >>>>
> >>>
> >>> Thanks again Arnaldo! I've hit an issue with this series in
> >>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> >>> from the BTF representation, and as a result:
> >>>
> >>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> >>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> >>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> >>>
> >>> Not sure why I didn't notice this previously.
> >>>
> >>> The problem is the DWARF - and therefore BTF - generated for a function like
> >>>
> >>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >>> {
> >>>         return -EOPNOTSUPP;
> >>> }
> >>>
> >>> looks like this:
> >>>
> >>>    <8af83a2>   DW_AT_external    : 1
> >>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> >>>     <8af83a6>   DW_AT_decl_file   : 5
> >>>     <8af83a7>   DW_AT_decl_line   : 737
> >>>     <8af83a9>   DW_AT_decl_column : 5
> >>>     <8af83aa>   DW_AT_prototyped  : 1
> >>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> >>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> >>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> >>>     <8af83b3>   DW_AT_name        : ctx
> >>>     <8af83b7>   DW_AT_decl_file   : 5
> >>>     <8af83b8>   DW_AT_decl_line   : 737
> >>>     <8af83ba>   DW_AT_decl_column : 51
> >>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> >>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> >>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> >>>     <8af83c4>   DW_AT_decl_file   : 5
> >>>     <8af83c5>   DW_AT_decl_line   : 737
> >>>     <8af83c7>   DW_AT_decl_column : 61
> >>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> >>>
> >>> ...and because there are no further abstract origin references
> >>> with location information either, we classify it as lacking
> >>> locations for (some of) the parameters, and as a result
> >>> we skip BTF encoding. We can work around that by doing this:
> >>>
> >>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >>
> >> replied in the other thread. This attr is broken and discouraged by gcc.
> >>
> >> For kfuncs where aregs are unused, please try __used and __may_unused
> >> applied to arguments.
> >> If that won't work, please add barrier_var(arg) to the body of kfunc
> >> the way we do in selftests.
> > 
> > There is also
> > # define __visible __attribute__((__externally_visible__))
> > that probably fits the best here.
> > 
> 
> testing thus for seems to show that for x86_64, David's series
> (using __used noinline in the BPF_KFUNC() wrapper and extended
> to cover recently-arrived kfuncs like cpumask) is sufficient
> to avoid resolve_btfids warnings.

Nice. Alexei -- lmk how you want to proceed. I think using the
__bpf_kfunc macro in the short term (with __used and noinline) is
probably the least controversial way to unblock this, but am open to
other suggestions.

> 
> We need to update the LSM_HOOK() definition for BPF LSM too,
> otherwise they will cause problems with missing btfids also.
> 
> With all that done, I'm not seeing resolve_btfids complaints
> for x86_64 (tested gcc9,11). I also tried using __visible, but
> using that in the kfunc wrapper causes problems for the static tcp 
> congestion control functions. We see warnings like these if __visible
> is used in BPF_KFUNC():
> 
> net/ipv4/tcp_dctcp.c:79:1: warning: ‘externally_visible’ attribute have effect only on public objects [-Wattributes]
>    79 | {

Yeah, I tend to think we should try to avoid using hidden / visible
attributes given that (to my knowledge) they're really more meant for
controlling whether a symbol is exported from a shared object rather
than controlling what the compiler is doing when it creates the
compilation unit. One could imagine that in an LTO build, the compiler
would still optimize the function regardless of its visibility for that
reason, though it's possible I don't have the full picture.

> 
> However, for aarch64 with the same changes we see a bunch of complaints
> from resolve_btfids for BPF_KFUNC()-wrapped kfuncs and LSM hooks:
> 
>   BTFIDS  vmlinux
> WARN: resolve_btfids: unresolved symbol tcp_cong_avoid_ai
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
> WARN: resolve_btfids: unresolved symbol bpf_lsm_xfrm_state_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_xfrm_policy_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_tun_dev_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_task_to_inode
> WARN: resolve_btfids: unresolved symbol bpf_lsm_task_getsecid_obj
> WARN: resolve_btfids: unresolved symbol bpf_lsm_task_free
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sock_graft
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_getsecid
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sk_clone_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_shm_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sem_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sctp_sk_clone
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_free_mnt_opts
> WARN: resolve_btfids: unresolved symbol bpf_lsm_sb_delete
> WARN: resolve_btfids: unresolved symbol bpf_lsm_req_classify_flow
> WARN: resolve_btfids: unresolved symbol bpf_lsm_release_secctx
> WARN: resolve_btfids: unresolved symbol bpf_lsm_perf_event_free
> WARN: resolve_btfids: unresolved symbol bpf_lsm_msg_queue_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_msg_msg_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_key_free
> WARN: resolve_btfids: unresolved symbol bpf_lsm_ipc_getsecid
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_post_setxattr
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_invalidate_secctx
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_getsecid
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inode_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inet_csk_clone
> WARN: resolve_btfids: unresolved symbol bpf_lsm_inet_conn_established
> WARN: resolve_btfids: unresolved symbol bpf_lsm_ib_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_file_set_fowner
> WARN: resolve_btfids: unresolved symbol bpf_lsm_file_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_d_instantiate
> WARN: resolve_btfids: unresolved symbol bpf_lsm_current_getsecid_subj
> WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_transfer
> WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_getsecid
> WARN: resolve_btfids: unresolved symbol bpf_lsm_cred_free
> WARN: resolve_btfids: unresolved symbol bpf_lsm_bprm_committing_creds
> WARN: resolve_btfids: unresolved symbol bpf_lsm_bprm_committed_creds
> WARN: resolve_btfids: unresolved symbol bpf_lsm_bpf_prog_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_bpf_map_free_security
> WARN: resolve_btfids: unresolved symbol bpf_lsm_audit_rule_free
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test3
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release
> WARN: resolve_btfids: unresolved symbol bpf_cpumask_any
> WARN: resolve_btfids: unresolved symbol bpf_cgroup_acquire
> WARN: resolve_btfids: unresolved symbol bpf_cast_to_kern_ctx
>   NM      System.map

Is that all of them? That's surprising that we'd only fail to resolve a
random subset of the kfuncs, e.g. bpf_cpumask_any(). There's nothing
whatsoever special about it.

> 
> Thanks!
> 
> Alan

Alexei Starovoitov Feb. 1, 2023, 12:14 a.m. UTC | #14

On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>
> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > > On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > > <[e-mail]> wrote:
> > >>
> > >> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > >>>
> > >>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > >>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > >>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > >>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>>>>>>> +++ b/dwarves.h
> > >>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > >>>>>>>>   uint8_t          has_addr_info:1;
> > >>>>>>>>   uint8_t          uses_global_strings:1;
> > >>>>>>>>   uint8_t          little_endian:1;
> > >>>>>>>> + uint8_t          nr_register_params;
> > >>>>>>>>   uint16_t         language;
> > >>>>>>>>   unsigned long    nr_inline_expansions;
> > >>>>>>>>   size_t           size_inline_expansions;
> > >>>>>>>
> > >>>>>
> > >>>>>> Thanks for this, never thought of cross-builds to be honest!
> > >>>>>
> > >>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > >>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > >>>>>> defined if using a very old elf.h; below works around this
> > >>>>>> (dwarves otherwise builds fine on this system).
> > >>>>>
> > >>>>> Ok, will add it and will test with containers for older distros too.
> > >>>>
> > >>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > >>>> repo at:
> > >>>>
> > >>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > >>>>
> > >>>> It failed yesterday and today due to problems with the installation of
> > >>>> llvm, probably tomorrow it'll be back working as I saw some
> > >>>> notifications floating by.
> > >>>>
> > >>>> I added the conditional EM_RISCV definition as well as removed the dup
> > >>>> iterator that Jiri noticed.
> > >>>>
> > >>>
> > >>> Thanks again Arnaldo! I've hit an issue with this series in
> > >>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > >>> from the BTF representation, and as a result:
> > >>>
> > >>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > >>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > >>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > >>>
> > >>> Not sure why I didn't notice this previously.
> > >>>
> > >>> The problem is the DWARF - and therefore BTF - generated for a function like
> > >>>
> > >>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > >>> {
> > >>>         return -EOPNOTSUPP;
> > >>> }
> > >>>
> > >>> looks like this:
> > >>>
> > >>>    <8af83a2>   DW_AT_external    : 1
> > >>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > >>>     <8af83a6>   DW_AT_decl_file   : 5
> > >>>     <8af83a7>   DW_AT_decl_line   : 737
> > >>>     <8af83a9>   DW_AT_decl_column : 5
> > >>>     <8af83aa>   DW_AT_prototyped  : 1
> > >>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > >>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > >>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > >>>     <8af83b3>   DW_AT_name        : ctx
> > >>>     <8af83b7>   DW_AT_decl_file   : 5
> > >>>     <8af83b8>   DW_AT_decl_line   : 737
> > >>>     <8af83ba>   DW_AT_decl_column : 51
> > >>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > >>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > >>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > >>>     <8af83c4>   DW_AT_decl_file   : 5
> > >>>     <8af83c5>   DW_AT_decl_line   : 737
> > >>>     <8af83c7>   DW_AT_decl_column : 61
> > >>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > >>>
> > >>> ...and because there are no further abstract origin references
> > >>> with location information either, we classify it as lacking
> > >>> locations for (some of) the parameters, and as a result
> > >>> we skip BTF encoding. We can work around that by doing this:
> > >>>
> > >>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > >>
> > >> replied in the other thread. This attr is broken and discouraged by gcc.
> > >>
> > >> For kfuncs where aregs are unused, please try __used and __may_unused
> > >> applied to arguments.
> > >> If that won't work, please add barrier_var(arg) to the body of kfunc
> > >> the way we do in selftests.
> > >
> > > There is also
> > > # define __visible __attribute__((__externally_visible__))
> > > that probably fits the best here.
> > >
> >
> > testing thus for seems to show that for x86_64, David's series
> > (using __used noinline in the BPF_KFUNC() wrapper and extended
> > to cover recently-arrived kfuncs like cpumask) is sufficient
> > to avoid resolve_btfids warnings.
>
> Nice. Alexei -- lmk how you want to proceed. I think using the
> __bpf_kfunc macro in the short term (with __used and noinline) is
> probably the least controversial way to unblock this, but am open to
> other suggestions.

Sounds good to me, but sounds like __used and noinline are not
enough to address the issues on aarch64?

> Yeah, I tend to think we should try to avoid using hidden / visible
> attributes given that (to my knowledge) they're really more meant for
> controlling whether a symbol is exported from a shared object rather
> than controlling what the compiler is doing when it creates the
> compilation unit. One could imagine that in an LTO build, the compiler
> would still optimize the function regardless of its visibility for that
> reason, though it's possible I don't have the full picture.

__visible is specifically done to prevent optimization of
functions that are externally visible. That should address LTO concerns.
We haven't seen LTO messing up anything. Just something to keep in mind.

David Vernet Feb. 1, 2023, 3:02 a.m. UTC | #15

On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> >
> > On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > > On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > > > On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > > > <[e-mail]> wrote:
> > > >>
> > > >> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > > >>>
> > > >>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > > >>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > >>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > > >>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > > >>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > >>>>>>>> +++ b/dwarves.h
> > > >>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > > >>>>>>>>   uint8_t          has_addr_info:1;
> > > >>>>>>>>   uint8_t          uses_global_strings:1;
> > > >>>>>>>>   uint8_t          little_endian:1;
> > > >>>>>>>> + uint8_t          nr_register_params;
> > > >>>>>>>>   uint16_t         language;
> > > >>>>>>>>   unsigned long    nr_inline_expansions;
> > > >>>>>>>>   size_t           size_inline_expansions;
> > > >>>>>>>
> > > >>>>>
> > > >>>>>> Thanks for this, never thought of cross-builds to be honest!
> > > >>>>>
> > > >>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > > >>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > > >>>>>> defined if using a very old elf.h; below works around this
> > > >>>>>> (dwarves otherwise builds fine on this system).
> > > >>>>>
> > > >>>>> Ok, will add it and will test with containers for older distros too.
> > > >>>>
> > > >>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > > >>>> repo at:
> > > >>>>
> > > >>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > > >>>>
> > > >>>> It failed yesterday and today due to problems with the installation of
> > > >>>> llvm, probably tomorrow it'll be back working as I saw some
> > > >>>> notifications floating by.
> > > >>>>
> > > >>>> I added the conditional EM_RISCV definition as well as removed the dup
> > > >>>> iterator that Jiri noticed.
> > > >>>>
> > > >>>
> > > >>> Thanks again Arnaldo! I've hit an issue with this series in
> > > >>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > > >>> from the BTF representation, and as a result:
> > > >>>
> > > >>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > > >>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > > >>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > > >>>
> > > >>> Not sure why I didn't notice this previously.
> > > >>>
> > > >>> The problem is the DWARF - and therefore BTF - generated for a function like
> > > >>>
> > > >>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > >>> {
> > > >>>         return -EOPNOTSUPP;
> > > >>> }
> > > >>>
> > > >>> looks like this:
> > > >>>
> > > >>>    <8af83a2>   DW_AT_external    : 1
> > > >>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > > >>>     <8af83a6>   DW_AT_decl_file   : 5
> > > >>>     <8af83a7>   DW_AT_decl_line   : 737
> > > >>>     <8af83a9>   DW_AT_decl_column : 5
> > > >>>     <8af83aa>   DW_AT_prototyped  : 1
> > > >>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > > >>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > > >>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > > >>>     <8af83b3>   DW_AT_name        : ctx
> > > >>>     <8af83b7>   DW_AT_decl_file   : 5
> > > >>>     <8af83b8>   DW_AT_decl_line   : 737
> > > >>>     <8af83ba>   DW_AT_decl_column : 51
> > > >>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > > >>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > > >>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > > >>>     <8af83c4>   DW_AT_decl_file   : 5
> > > >>>     <8af83c5>   DW_AT_decl_line   : 737
> > > >>>     <8af83c7>   DW_AT_decl_column : 61
> > > >>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > > >>>
> > > >>> ...and because there are no further abstract origin references
> > > >>> with location information either, we classify it as lacking
> > > >>> locations for (some of) the parameters, and as a result
> > > >>> we skip BTF encoding. We can work around that by doing this:
> > > >>>
> > > >>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > >>
> > > >> replied in the other thread. This attr is broken and discouraged by gcc.
> > > >>
> > > >> For kfuncs where aregs are unused, please try __used and __may_unused
> > > >> applied to arguments.
> > > >> If that won't work, please add barrier_var(arg) to the body of kfunc
> > > >> the way we do in selftests.
> > > >
> > > > There is also
> > > > # define __visible __attribute__((__externally_visible__))
> > > > that probably fits the best here.
> > > >
> > >
> > > testing thus for seems to show that for x86_64, David's series
> > > (using __used noinline in the BPF_KFUNC() wrapper and extended
> > > to cover recently-arrived kfuncs like cpumask) is sufficient
> > > to avoid resolve_btfids warnings.
> >
> > Nice. Alexei -- lmk how you want to proceed. I think using the
> > __bpf_kfunc macro in the short term (with __used and noinline) is
> > probably the least controversial way to unblock this, but am open to
> > other suggestions.
> 
> Sounds good to me, but sounds like __used and noinline are not
> enough to address the issues on aarch64?

Indeed, we'll have to make sure that's also addressed. Alan -- did you
try Alexei's suggestion to use __weak? Does that fix the issue for
aarch64? I'm still confused as to why it's only complaining for a small
subset of kfuncs, which include those that have external linkage.

> 
> > Yeah, I tend to think we should try to avoid using hidden / visible
> > attributes given that (to my knowledge) they're really more meant for
> > controlling whether a symbol is exported from a shared object rather
> > than controlling what the compiler is doing when it creates the
> > compilation unit. One could imagine that in an LTO build, the compiler
> > would still optimize the function regardless of its visibility for that
> > reason, though it's possible I don't have the full picture.
> 
> __visible is specifically done to prevent optimization of
> functions that are externally visible. That should address LTO concerns.
> We haven't seen LTO messing up anything. Just something to keep in mind.

Ah, fair enough. I was conflating that with the visibility("...")
attribute. As you pointed out, __visible is something else entirely, and
is meant to avoid possible issues with LTO.

One other option we could consider is enforcing that kfuncs must have
global linkage and can't be static. If we did that, it seems like
__visible would be a viable option. Though we'd have to verify that it
addresses the issue w/ aarch64.

Alan Maguire Feb. 1, 2023, 1:59 p.m. UTC | #16

On 01/02/2023 03:02, David Vernet wrote:
> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>>>
>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>   uint16_t         language;
>>>>>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>
>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>
>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>
>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>> repo at:
>>>>>>>>
>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>
>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>> notifications floating by.
>>>>>>>>
>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>> iterator that Jiri noticed.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>> from the BTF representation, and as a result:
>>>>>>>
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>
>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>
>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>
>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>> {
>>>>>>>         return -EOPNOTSUPP;
>>>>>>> }
>>>>>>>
>>>>>>> looks like this:
>>>>>>>
>>>>>>>    <8af83a2>   DW_AT_external    : 1
>>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>     <8af83a9>   DW_AT_decl_column : 5
>>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>     <8af83b3>   DW_AT_name        : ctx
>>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>     <8af83ba>   DW_AT_decl_column : 51
>>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>     <8af83c7>   DW_AT_decl_column : 61
>>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>
>>>>>>> ...and because there are no further abstract origin references
>>>>>>> with location information either, we classify it as lacking
>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>
>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>
>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>
>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>> applied to arguments.
>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>> the way we do in selftests.
>>>>>
>>>>> There is also
>>>>> # define __visible __attribute__((__externally_visible__))
>>>>> that probably fits the best here.
>>>>>
>>>>
>>>> testing thus for seems to show that for x86_64, David's series
>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>> to avoid resolve_btfids warnings.
>>>
>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>> probably the least controversial way to unblock this, but am open to
>>> other suggestions.
>>
>> Sounds good to me, but sounds like __used and noinline are not
>> enough to address the issues on aarch64?
> 
> Indeed, we'll have to make sure that's also addressed. Alan -- did you
> try Alexei's suggestion to use __weak? Does that fix the issue for
> aarch64? I'm still confused as to why it's only complaining for a small
> subset of kfuncs, which include those that have external linkage.
> 

I finally got to the bottom of the aarch64 issues; there was a 1-line bug
in the changes I made to the DWARF handling code which leads to BTF generation;
it was excluding a bunch of functions incorrectly, marking them as optimized out.
The fix is:

diff --git a/dwarf_loader.c b/dwarf_loader.c
index dba2d37..8364e17 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
                        Dwarf_Op *expr = loc.expr;
 
                        switch (expr->atom) {
-                       case DW_OP_reg1 ... DW_OP_reg31:
+                       case DW_OP_reg0 ... DW_OP_reg31:
                        case DW_OP_breg0 ... DW_OP_breg31:
                                break;
                        default:

..and because reg0 is the first parameter for aarch64, we were
incorrectly landing in the "default:" of the switch statement
and marking a bunch of functions as optimized out
because we thought the first argument was. Sorry about this,
and thanks for all the suggestions!

Arnaldo, will I send a v3 series incorporating the above fix
to patch 1?

With this fix in place, prefixing the kfunc functions with

__used noinline

...did the trick to ensure kfuncs were not excluded on x86_64
and aarch64.

>>
>>> Yeah, I tend to think we should try to avoid using hidden / visible
>>> attributes given that (to my knowledge) they're really more meant for
>>> controlling whether a symbol is exported from a shared object rather
>>> than controlling what the compiler is doing when it creates the
>>> compilation unit. One could imagine that in an LTO build, the compiler
>>> would still optimize the function regardless of its visibility for that
>>> reason, though it's possible I don't have the full picture.
>>
>> __visible is specifically done to prevent optimization of
>> functions that are externally visible. That should address LTO concerns.
>> We haven't seen LTO messing up anything. Just something to keep in mind.
> 
> Ah, fair enough. I was conflating that with the visibility("...")
> attribute. As you pointed out, __visible is something else entirely, and
> is meant to avoid possible issues with LTO.
> 
> One other option we could consider is enforcing that kfuncs must have
> global linkage and can't be static. If we did that, it seems like
> __visible would be a viable option. Though we'd have to verify that it
> addresses the issue w/ aarch64.
>

Arnaldo Carvalho de Melo Feb. 1, 2023, 3:02 p.m. UTC | #17

Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
> On 01/02/2023 03:02, David Vernet wrote:
> > On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> >> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> >>>
> >>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> >>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> >>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> >>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> >>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> >>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>>>>>>>>>> +++ b/dwarves.h
> >>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> >>>>>>>>>>>>   uint8_t          has_addr_info:1;
> >>>>>>>>>>>>   uint8_t          uses_global_strings:1;
> >>>>>>>>>>>>   uint8_t          little_endian:1;
> >>>>>>>>>>>> + uint8_t          nr_register_params;
> >>>>>>>>>>>>   uint16_t         language;
> >>>>>>>>>>>>   unsigned long    nr_inline_expansions;
> >>>>>>>>>>>>   size_t           size_inline_expansions;
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
> >>>>>>>>>
> >>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> >>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
> >>>>>>>>>> defined if using a very old elf.h; below works around this
> >>>>>>>>>> (dwarves otherwise builds fine on this system).
> >>>>>>>>>
> >>>>>>>>> Ok, will add it and will test with containers for older distros too.
> >>>>>>>>
> >>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> >>>>>>>> repo at:
> >>>>>>>>
> >>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> >>>>>>>>
> >>>>>>>> It failed yesterday and today due to problems with the installation of
> >>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
> >>>>>>>> notifications floating by.
> >>>>>>>>
> >>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
> >>>>>>>> iterator that Jiri noticed.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
> >>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> >>>>>>> from the BTF representation, and as a result:
> >>>>>>>
> >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> >>>>>>>
> >>>>>>> Not sure why I didn't notice this previously.
> >>>>>>>
> >>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
> >>>>>>>
> >>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >>>>>>> {
> >>>>>>>         return -EOPNOTSUPP;
> >>>>>>> }
> >>>>>>>
> >>>>>>> looks like this:
> >>>>>>>
> >>>>>>>    <8af83a2>   DW_AT_external    : 1
> >>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> >>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
> >>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
> >>>>>>>     <8af83a9>   DW_AT_decl_column : 5
> >>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
> >>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> >>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> >>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> >>>>>>>     <8af83b3>   DW_AT_name        : ctx
> >>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
> >>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
> >>>>>>>     <8af83ba>   DW_AT_decl_column : 51
> >>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> >>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> >>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> >>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
> >>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
> >>>>>>>     <8af83c7>   DW_AT_decl_column : 61
> >>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> >>>>>>>
> >>>>>>> ...and because there are no further abstract origin references
> >>>>>>> with location information either, we classify it as lacking
> >>>>>>> locations for (some of) the parameters, and as a result
> >>>>>>> we skip BTF encoding. We can work around that by doing this:
> >>>>>>>
> >>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> >>>>>>
> >>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
> >>>>>>
> >>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
> >>>>>> applied to arguments.
> >>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
> >>>>>> the way we do in selftests.
> >>>>>
> >>>>> There is also
> >>>>> # define __visible __attribute__((__externally_visible__))
> >>>>> that probably fits the best here.
> >>>>>
> >>>>
> >>>> testing thus for seems to show that for x86_64, David's series
> >>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
> >>>> to cover recently-arrived kfuncs like cpumask) is sufficient
> >>>> to avoid resolve_btfids warnings.
> >>>
> >>> Nice. Alexei -- lmk how you want to proceed. I think using the
> >>> __bpf_kfunc macro in the short term (with __used and noinline) is
> >>> probably the least controversial way to unblock this, but am open to
> >>> other suggestions.
> >>
> >> Sounds good to me, but sounds like __used and noinline are not
> >> enough to address the issues on aarch64?
> > 
> > Indeed, we'll have to make sure that's also addressed. Alan -- did you
> > try Alexei's suggestion to use __weak? Does that fix the issue for
> > aarch64? I'm still confused as to why it's only complaining for a small
> > subset of kfuncs, which include those that have external linkage.
> > 
> 
> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
> in the changes I made to the DWARF handling code which leads to BTF generation;
> it was excluding a bunch of functions incorrectly, marking them as optimized out.
> The fix is:
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index dba2d37..8364e17 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>                         Dwarf_Op *expr = loc.expr;
>  
>                         switch (expr->atom) {
> -                       case DW_OP_reg1 ... DW_OP_reg31:
> +                       case DW_OP_reg0 ... DW_OP_reg31:
>                         case DW_OP_breg0 ... DW_OP_breg31:
>                                 break;
>                         default:
> 
> ..and because reg0 is the first parameter for aarch64, we were
> incorrectly landing in the "default:" of the switch statement
> and marking a bunch of functions as optimized out
> because we thought the first argument was. Sorry about this,
> and thanks for all the suggestions!
> 
> Arnaldo, will I send a v3 series incorporating the above fix
> to patch 1?

I can fix it here. Done, I;ll force push it to the 'next' branch.

Also I noted the index_idx usage in parameter__new(), it can be -1 when
processing:

 <1><2eb2>: Abbrev Number: 18 (DW_TAG_subroutine_type)
    <2eb3>   DW_AT_prototyped  : 1
    <2eb3>   DW_AT_sibling     : <0x2ec2>
 <2><2eb7>: Abbrev Number: 3 (DW_TAG_formal_parameter)
    <2eb8>   DW_AT_type        : <0x414>
 <2><2ebc>: Abbrev Number: 3 (DW_TAG_formal_parameter)
    <2ebd>   DW_AT_type        : <0x69>
 <2><2ec1>: Abbrev Number: 0

 And in that case we don't have the location expression:

  <1><af36>: Abbrev Number: 77 (DW_TAG_subprogram)
    <af37>   DW_AT_external    : 1
    <af37>   DW_AT_name        : (indirect string, offset: 0x4ff7): startup_64_setup_env
    <af3b>   DW_AT_decl_file   : 1
    <af3b>   DW_AT_decl_line   : 592
    <af3d>   DW_AT_decl_column : 13
    <af3e>   DW_AT_prototyped  : 1
    <af3e>   DW_AT_low_pc      : 0xffffffff81000570
    <af46>   DW_AT_high_pc     : 0x6d
    <af4e>   DW_AT_frame_base  : 1 byte block: 9c       (DW_OP_call_frame_cfa)
    <af50>   DW_AT_call_all_calls: 1
    <af50>   DW_AT_sibling     : <0xb11f>
 <2><af54>: Abbrev Number: 67 (DW_TAG_formal_parameter)
    <af55>   DW_AT_name        : (indirect string, offset: 0x2a50d): physbase
    <af59>   DW_AT_decl_file   : 1
    <af59>   DW_AT_decl_line   : 592
    <af5b>   DW_AT_decl_column : 48
    <af5c>   DW_AT_type        : <0x4c>
    <af60>   DW_AT_location    : 0x10 (location list)
    <af64>   DW_AT_GNU_locviews: 0xc

I.e. its just a function _type_, not an actual function, so I'm applying
this on top of that first patch, ok?

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 7e05fde8a5c3ac26..253c5efaf3b55a93 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -1035,7 +1035,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 		tag__init(&parm->tag, cu, die);
 		parm->name = attr_string(die, DW_AT_name, conf);
 
-		if (param_idx >= cu->nr_register_params)
+		if (param_idx >= cu->nr_register_params || param_idx < 0)
 			return parm;
 		/* Parameters which use DW_AT_abstract_origin to point at
 		 * the original parameter definition (with no name in the DIE)


- Arnaldo
 
> With this fix in place, prefixing the kfunc functions with
> 
> __used noinline
> 
> ...did the trick to ensure kfuncs were not excluded on x86_64
> and aarch64.
> 
> >>
> >>> Yeah, I tend to think we should try to avoid using hidden / visible
> >>> attributes given that (to my knowledge) they're really more meant for
> >>> controlling whether a symbol is exported from a shared object rather
> >>> than controlling what the compiler is doing when it creates the
> >>> compilation unit. One could imagine that in an LTO build, the compiler
> >>> would still optimize the function regardless of its visibility for that
> >>> reason, though it's possible I don't have the full picture.
> >>
> >> __visible is specifically done to prevent optimization of
> >> functions that are externally visible. That should address LTO concerns.
> >> We haven't seen LTO messing up anything. Just something to keep in mind.
> > 
> > Ah, fair enough. I was conflating that with the visibility("...")
> > attribute. As you pointed out, __visible is something else entirely, and
> > is meant to avoid possible issues with LTO.
> > 
> > One other option we could consider is enforcing that kfuncs must have
> > global linkage and can't be static. If we did that, it seems like
> > __visible would be a viable option. Though we'd have to verify that it
> > addresses the issue w/ aarch64.
> >

Alan Maguire Feb. 1, 2023, 3:13 p.m. UTC | #18

On 01/02/2023 15:02, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
>> On 01/02/2023 03:02, David Vernet wrote:
>>> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>>>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>>>>>
>>>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>>>   uint16_t         language;
>>>>>>>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>>>
>>>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>>>
>>>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>>>
>>>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>>>> repo at:
>>>>>>>>>>
>>>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>>>
>>>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>>>> notifications floating by.
>>>>>>>>>>
>>>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>>>> iterator that Jiri noticed.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>>>> from the BTF representation, and as a result:
>>>>>>>>>
>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>>>
>>>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>>>
>>>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>>>
>>>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>> {
>>>>>>>>>         return -EOPNOTSUPP;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> looks like this:
>>>>>>>>>
>>>>>>>>>    <8af83a2>   DW_AT_external    : 1
>>>>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>>>     <8af83a9>   DW_AT_decl_column : 5
>>>>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>>>     <8af83b3>   DW_AT_name        : ctx
>>>>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>>>     <8af83ba>   DW_AT_decl_column : 51
>>>>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>>>     <8af83c7>   DW_AT_decl_column : 61
>>>>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>>>
>>>>>>>>> ...and because there are no further abstract origin references
>>>>>>>>> with location information either, we classify it as lacking
>>>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>>>
>>>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>
>>>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>>>
>>>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>>>> applied to arguments.
>>>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>>>> the way we do in selftests.
>>>>>>>
>>>>>>> There is also
>>>>>>> # define __visible __attribute__((__externally_visible__))
>>>>>>> that probably fits the best here.
>>>>>>>
>>>>>>
>>>>>> testing thus for seems to show that for x86_64, David's series
>>>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>>>> to avoid resolve_btfids warnings.
>>>>>
>>>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>>>> probably the least controversial way to unblock this, but am open to
>>>>> other suggestions.
>>>>
>>>> Sounds good to me, but sounds like __used and noinline are not
>>>> enough to address the issues on aarch64?
>>>
>>> Indeed, we'll have to make sure that's also addressed. Alan -- did you
>>> try Alexei's suggestion to use __weak? Does that fix the issue for
>>> aarch64? I'm still confused as to why it's only complaining for a small
>>> subset of kfuncs, which include those that have external linkage.
>>>
>>
>> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
>> in the changes I made to the DWARF handling code which leads to BTF generation;
>> it was excluding a bunch of functions incorrectly, marking them as optimized out.
>> The fix is:
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index dba2d37..8364e17 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>                         Dwarf_Op *expr = loc.expr;
>>  
>>                         switch (expr->atom) {
>> -                       case DW_OP_reg1 ... DW_OP_reg31:
>> +                       case DW_OP_reg0 ... DW_OP_reg31:
>>                         case DW_OP_breg0 ... DW_OP_breg31:
>>                                 break;
>>                         default:
>>
>> ..and because reg0 is the first parameter for aarch64, we were
>> incorrectly landing in the "default:" of the switch statement
>> and marking a bunch of functions as optimized out
>> because we thought the first argument was. Sorry about this,
>> and thanks for all the suggestions!
>>
>> Arnaldo, will I send a v3 series incorporating the above fix
>> to patch 1?
> 
> I can fix it here. Done, I;ll force push it to the 'next' branch.
> 
> Also I noted the index_idx usage in parameter__new(), it can be -1 when
> processing:
> 
>  <1><2eb2>: Abbrev Number: 18 (DW_TAG_subroutine_type)
>     <2eb3>   DW_AT_prototyped  : 1
>     <2eb3>   DW_AT_sibling     : <0x2ec2>
>  <2><2eb7>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2eb8>   DW_AT_type        : <0x414>
>  <2><2ebc>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2ebd>   DW_AT_type        : <0x69>
>  <2><2ec1>: Abbrev Number: 0
> 
>  And in that case we don't have the location expression:
> 
>   <1><af36>: Abbrev Number: 77 (DW_TAG_subprogram)
>     <af37>   DW_AT_external    : 1
>     <af37>   DW_AT_name        : (indirect string, offset: 0x4ff7): startup_64_setup_env
>     <af3b>   DW_AT_decl_file   : 1
>     <af3b>   DW_AT_decl_line   : 592
>     <af3d>   DW_AT_decl_column : 13
>     <af3e>   DW_AT_prototyped  : 1
>     <af3e>   DW_AT_low_pc      : 0xffffffff81000570
>     <af46>   DW_AT_high_pc     : 0x6d
>     <af4e>   DW_AT_frame_base  : 1 byte block: 9c       (DW_OP_call_frame_cfa)
>     <af50>   DW_AT_call_all_calls: 1
>     <af50>   DW_AT_sibling     : <0xb11f>
>  <2><af54>: Abbrev Number: 67 (DW_TAG_formal_parameter)
>     <af55>   DW_AT_name        : (indirect string, offset: 0x2a50d): physbase
>     <af59>   DW_AT_decl_file   : 1
>     <af59>   DW_AT_decl_line   : 592
>     <af5b>   DW_AT_decl_column : 48
>     <af5c>   DW_AT_type        : <0x4c>
>     <af60>   DW_AT_location    : 0x10 (location list)
>     <af64>   DW_AT_GNU_locviews: 0xc
> 
> I.e. its just a function _type_, not an actual function, so I'm applying
> this on top of that first patch, ok?
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 7e05fde8a5c3ac26..253c5efaf3b55a93 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1035,7 +1035,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  		tag__init(&parm->tag, cu, die);
>  		parm->name = attr_string(die, DW_AT_name, conf);
>  
> -		if (param_idx >= cu->nr_register_params)
> +		if (param_idx >= cu->nr_register_params || param_idx < 0)
>  			return parm;
>  		/* Parameters which use DW_AT_abstract_origin to point at
>  		 * the original parameter definition (with no name in the DIE)
> 
>

ah, great catch. thanks again!

Alan
 
> - Arnaldo
>  
>> With this fix in place, prefixing the kfunc functions with
>>
>> __used noinline
>>
>> ...did the trick to ensure kfuncs were not excluded on x86_64
>> and aarch64.
>>
>>>>
>>>>> Yeah, I tend to think we should try to avoid using hidden / visible
>>>>> attributes given that (to my knowledge) they're really more meant for
>>>>> controlling whether a symbol is exported from a shared object rather
>>>>> than controlling what the compiler is doing when it creates the
>>>>> compilation unit. One could imagine that in an LTO build, the compiler
>>>>> would still optimize the function regardless of its visibility for that
>>>>> reason, though it's possible I don't have the full picture.
>>>>
>>>> __visible is specifically done to prevent optimization of
>>>> functions that are externally visible. That should address LTO concerns.
>>>> We haven't seen LTO messing up anything. Just something to keep in mind.
>>>
>>> Ah, fair enough. I was conflating that with the visibility("...")
>>> attribute. As you pointed out, __visible is something else entirely, and
>>> is meant to avoid possible issues with LTO.
>>>
>>> One other option we could consider is enforcing that kfuncs must have
>>> global linkage and can't be static. If we did that, it seems like
>>> __visible would be a viable option. Though we'd have to verify that it
>>> addresses the issue w/ aarch64.
>>>
>

David Vernet Feb. 1, 2023, 3:19 p.m. UTC | #19

On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
> > On 01/02/2023 03:02, David Vernet wrote:
> > > On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> > >> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> > >>>
> > >>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > >>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > >>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > >>>>> <[e-mail]> wrote:
> > >>>>>>
> > >>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > >>>>>>>
> > >>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > >>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > >>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > >>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>>>>>>>>>>> +++ b/dwarves.h
> > >>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > >>>>>>>>>>>>   uint8_t          has_addr_info:1;
> > >>>>>>>>>>>>   uint8_t          uses_global_strings:1;
> > >>>>>>>>>>>>   uint8_t          little_endian:1;
> > >>>>>>>>>>>> + uint8_t          nr_register_params;
> > >>>>>>>>>>>>   uint16_t         language;
> > >>>>>>>>>>>>   unsigned long    nr_inline_expansions;
> > >>>>>>>>>>>>   size_t           size_inline_expansions;
> > >>>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
> > >>>>>>>>>
> > >>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > >>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > >>>>>>>>>> defined if using a very old elf.h; below works around this
> > >>>>>>>>>> (dwarves otherwise builds fine on this system).
> > >>>>>>>>>
> > >>>>>>>>> Ok, will add it and will test with containers for older distros too.
> > >>>>>>>>
> > >>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > >>>>>>>> repo at:
> > >>>>>>>>
> > >>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > >>>>>>>>
> > >>>>>>>> It failed yesterday and today due to problems with the installation of
> > >>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
> > >>>>>>>> notifications floating by.
> > >>>>>>>>
> > >>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
> > >>>>>>>> iterator that Jiri noticed.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
> > >>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > >>>>>>> from the BTF representation, and as a result:
> > >>>>>>>
> > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > >>>>>>>
> > >>>>>>> Not sure why I didn't notice this previously.
> > >>>>>>>
> > >>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
> > >>>>>>>
> > >>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > >>>>>>> {
> > >>>>>>>         return -EOPNOTSUPP;
> > >>>>>>> }
> > >>>>>>>
> > >>>>>>> looks like this:
> > >>>>>>>
> > >>>>>>>    <8af83a2>   DW_AT_external    : 1
> > >>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > >>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
> > >>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
> > >>>>>>>     <8af83a9>   DW_AT_decl_column : 5
> > >>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
> > >>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > >>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > >>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > >>>>>>>     <8af83b3>   DW_AT_name        : ctx
> > >>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
> > >>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
> > >>>>>>>     <8af83ba>   DW_AT_decl_column : 51
> > >>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > >>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > >>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > >>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
> > >>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
> > >>>>>>>     <8af83c7>   DW_AT_decl_column : 61
> > >>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > >>>>>>>
> > >>>>>>> ...and because there are no further abstract origin references
> > >>>>>>> with location information either, we classify it as lacking
> > >>>>>>> locations for (some of) the parameters, and as a result
> > >>>>>>> we skip BTF encoding. We can work around that by doing this:
> > >>>>>>>
> > >>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > >>>>>>
> > >>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
> > >>>>>>
> > >>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
> > >>>>>> applied to arguments.
> > >>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
> > >>>>>> the way we do in selftests.
> > >>>>>
> > >>>>> There is also
> > >>>>> # define __visible __attribute__((__externally_visible__))
> > >>>>> that probably fits the best here.
> > >>>>>
> > >>>>
> > >>>> testing thus for seems to show that for x86_64, David's series
> > >>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
> > >>>> to cover recently-arrived kfuncs like cpumask) is sufficient
> > >>>> to avoid resolve_btfids warnings.
> > >>>
> > >>> Nice. Alexei -- lmk how you want to proceed. I think using the
> > >>> __bpf_kfunc macro in the short term (with __used and noinline) is
> > >>> probably the least controversial way to unblock this, but am open to
> > >>> other suggestions.
> > >>
> > >> Sounds good to me, but sounds like __used and noinline are not
> > >> enough to address the issues on aarch64?
> > > 
> > > Indeed, we'll have to make sure that's also addressed. Alan -- did you
> > > try Alexei's suggestion to use __weak? Does that fix the issue for
> > > aarch64? I'm still confused as to why it's only complaining for a small
> > > subset of kfuncs, which include those that have external linkage.
> > > 
> > 
> > I finally got to the bottom of the aarch64 issues; there was a 1-line bug
> > in the changes I made to the DWARF handling code which leads to BTF generation;
> > it was excluding a bunch of functions incorrectly, marking them as optimized out.
> > The fix is:
> > 
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index dba2d37..8364e17 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> >                         Dwarf_Op *expr = loc.expr;
> >  
> >                         switch (expr->atom) {
> > -                       case DW_OP_reg1 ... DW_OP_reg31:
> > +                       case DW_OP_reg0 ... DW_OP_reg31:
> >                         case DW_OP_breg0 ... DW_OP_breg31:
> >                                 break;
> >                         default:
> > 
> > ..and because reg0 is the first parameter for aarch64, we were
> > incorrectly landing in the "default:" of the switch statement
> > and marking a bunch of functions as optimized out
> > because we thought the first argument was. Sorry about this,
> > and thanks for all the suggestions!

Great, so inline and __used with __bpf_kfunc sounds like the way forward
in the short term. Arnaldo / Alexei -- how do you want to resolve the
dependency here? Going through bpf-next is probably a good idea so that
we get proper CI coverage, and any kfuncs added to bpf-next after this
can use the macro. Does that work for you?

> > 
> > Arnaldo, will I send a v3 series incorporating the above fix
> > to patch 1?
> 
> I can fix it here. Done, I;ll force push it to the 'next' branch.
> 
> Also I noted the index_idx usage in parameter__new(), it can be -1 when
> processing:
> 
>  <1><2eb2>: Abbrev Number: 18 (DW_TAG_subroutine_type)
>     <2eb3>   DW_AT_prototyped  : 1
>     <2eb3>   DW_AT_sibling     : <0x2ec2>
>  <2><2eb7>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2eb8>   DW_AT_type        : <0x414>
>  <2><2ebc>: Abbrev Number: 3 (DW_TAG_formal_parameter)
>     <2ebd>   DW_AT_type        : <0x69>
>  <2><2ec1>: Abbrev Number: 0
> 
>  And in that case we don't have the location expression:
> 
>   <1><af36>: Abbrev Number: 77 (DW_TAG_subprogram)
>     <af37>   DW_AT_external    : 1
>     <af37>   DW_AT_name        : (indirect string, offset: 0x4ff7): startup_64_setup_env
>     <af3b>   DW_AT_decl_file   : 1
>     <af3b>   DW_AT_decl_line   : 592
>     <af3d>   DW_AT_decl_column : 13
>     <af3e>   DW_AT_prototyped  : 1
>     <af3e>   DW_AT_low_pc      : 0xffffffff81000570
>     <af46>   DW_AT_high_pc     : 0x6d
>     <af4e>   DW_AT_frame_base  : 1 byte block: 9c       (DW_OP_call_frame_cfa)
>     <af50>   DW_AT_call_all_calls: 1
>     <af50>   DW_AT_sibling     : <0xb11f>
>  <2><af54>: Abbrev Number: 67 (DW_TAG_formal_parameter)
>     <af55>   DW_AT_name        : (indirect string, offset: 0x2a50d): physbase
>     <af59>   DW_AT_decl_file   : 1
>     <af59>   DW_AT_decl_line   : 592
>     <af5b>   DW_AT_decl_column : 48
>     <af5c>   DW_AT_type        : <0x4c>
>     <af60>   DW_AT_location    : 0x10 (location list)
>     <af64>   DW_AT_GNU_locviews: 0xc
> 
> I.e. its just a function _type_, not an actual function, so I'm applying
> this on top of that first patch, ok?
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 7e05fde8a5c3ac26..253c5efaf3b55a93 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1035,7 +1035,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  		tag__init(&parm->tag, cu, die);
>  		parm->name = attr_string(die, DW_AT_name, conf);
>  
> -		if (param_idx >= cu->nr_register_params)
> +		if (param_idx >= cu->nr_register_params || param_idx < 0)
>  			return parm;
>  		/* Parameters which use DW_AT_abstract_origin to point at
>  		 * the original parameter definition (with no name in the DIE)
> 
> 
> - Arnaldo
>  
> > With this fix in place, prefixing the kfunc functions with
> > 
> > __used noinline
> > 
> > ...did the trick to ensure kfuncs were not excluded on x86_64
> > and aarch64.

[...]

Thanks,
David

Alexei Starovoitov Feb. 1, 2023, 4:49 p.m. UTC | #20

On Wed, Feb 1, 2023 at 7:19 AM David Vernet <[e-mail]> wrote:
>
> On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
> > > On 01/02/2023 03:02, David Vernet wrote:
> > > > On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> > > >> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> > > >>>
> > > >>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > > >>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > > >>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > > >>>>> <[e-mail]> wrote:
> > > >>>>>>
> > > >>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > > >>>>>>>
> > > >>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > > >>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > >>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > > >>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > > >>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > >>>>>>>>>>>> +++ b/dwarves.h
> > > >>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > > >>>>>>>>>>>>   uint8_t          has_addr_info:1;
> > > >>>>>>>>>>>>   uint8_t          uses_global_strings:1;
> > > >>>>>>>>>>>>   uint8_t          little_endian:1;
> > > >>>>>>>>>>>> + uint8_t          nr_register_params;
> > > >>>>>>>>>>>>   uint16_t         language;
> > > >>>>>>>>>>>>   unsigned long    nr_inline_expansions;
> > > >>>>>>>>>>>>   size_t           size_inline_expansions;
> > > >>>>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
> > > >>>>>>>>>
> > > >>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > > >>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > > >>>>>>>>>> defined if using a very old elf.h; below works around this
> > > >>>>>>>>>> (dwarves otherwise builds fine on this system).
> > > >>>>>>>>>
> > > >>>>>>>>> Ok, will add it and will test with containers for older distros too.
> > > >>>>>>>>
> > > >>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > > >>>>>>>> repo at:
> > > >>>>>>>>
> > > >>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > > >>>>>>>>
> > > >>>>>>>> It failed yesterday and today due to problems with the installation of
> > > >>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
> > > >>>>>>>> notifications floating by.
> > > >>>>>>>>
> > > >>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
> > > >>>>>>>> iterator that Jiri noticed.
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
> > > >>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > > >>>>>>> from the BTF representation, and as a result:
> > > >>>>>>>
> > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > > >>>>>>>
> > > >>>>>>> Not sure why I didn't notice this previously.
> > > >>>>>>>
> > > >>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
> > > >>>>>>>
> > > >>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > >>>>>>> {
> > > >>>>>>>         return -EOPNOTSUPP;
> > > >>>>>>> }
> > > >>>>>>>
> > > >>>>>>> looks like this:
> > > >>>>>>>
> > > >>>>>>>    <8af83a2>   DW_AT_external    : 1
> > > >>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > > >>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
> > > >>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
> > > >>>>>>>     <8af83a9>   DW_AT_decl_column : 5
> > > >>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
> > > >>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > > >>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > > >>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > > >>>>>>>     <8af83b3>   DW_AT_name        : ctx
> > > >>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
> > > >>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
> > > >>>>>>>     <8af83ba>   DW_AT_decl_column : 51
> > > >>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > > >>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > > >>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > > >>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
> > > >>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
> > > >>>>>>>     <8af83c7>   DW_AT_decl_column : 61
> > > >>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > > >>>>>>>
> > > >>>>>>> ...and because there are no further abstract origin references
> > > >>>>>>> with location information either, we classify it as lacking
> > > >>>>>>> locations for (some of) the parameters, and as a result
> > > >>>>>>> we skip BTF encoding. We can work around that by doing this:
> > > >>>>>>>
> > > >>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > >>>>>>
> > > >>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
> > > >>>>>>
> > > >>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
> > > >>>>>> applied to arguments.
> > > >>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
> > > >>>>>> the way we do in selftests.
> > > >>>>>
> > > >>>>> There is also
> > > >>>>> # define __visible __attribute__((__externally_visible__))
> > > >>>>> that probably fits the best here.
> > > >>>>>
> > > >>>>
> > > >>>> testing thus for seems to show that for x86_64, David's series
> > > >>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
> > > >>>> to cover recently-arrived kfuncs like cpumask) is sufficient
> > > >>>> to avoid resolve_btfids warnings.
> > > >>>
> > > >>> Nice. Alexei -- lmk how you want to proceed. I think using the
> > > >>> __bpf_kfunc macro in the short term (with __used and noinline) is
> > > >>> probably the least controversial way to unblock this, but am open to
> > > >>> other suggestions.
> > > >>
> > > >> Sounds good to me, but sounds like __used and noinline are not
> > > >> enough to address the issues on aarch64?
> > > >
> > > > Indeed, we'll have to make sure that's also addressed. Alan -- did you
> > > > try Alexei's suggestion to use __weak? Does that fix the issue for
> > > > aarch64? I'm still confused as to why it's only complaining for a small
> > > > subset of kfuncs, which include those that have external linkage.
> > > >
> > >
> > > I finally got to the bottom of the aarch64 issues; there was a 1-line bug
> > > in the changes I made to the DWARF handling code which leads to BTF generation;
> > > it was excluding a bunch of functions incorrectly, marking them as optimized out.
> > > The fix is:
> > >
> > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > index dba2d37..8364e17 100644
> > > --- a/dwarf_loader.c
> > > +++ b/dwarf_loader.c
> > > @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> > >                         Dwarf_Op *expr = loc.expr;
> > >
> > >                         switch (expr->atom) {
> > > -                       case DW_OP_reg1 ... DW_OP_reg31:
> > > +                       case DW_OP_reg0 ... DW_OP_reg31:
> > >                         case DW_OP_breg0 ... DW_OP_breg31:
> > >                                 break;
> > >                         default:
> > >
> > > ..and because reg0 is the first parameter for aarch64, we were
> > > incorrectly landing in the "default:" of the switch statement
> > > and marking a bunch of functions as optimized out
> > > because we thought the first argument was. Sorry about this,
> > > and thanks for all the suggestions!
>
> Great, so inline and __used with __bpf_kfunc sounds like the way forward
> in the short term. Arnaldo / Alexei -- how do you want to resolve the
> dependency here? Going through bpf-next is probably a good idea so that
> we get proper CI coverage, and any kfuncs added to bpf-next after this
> can use the macro. Does that work for you?

It feels fixed pahole should be done under some flag
otherwise when people update the pahole the existing and older
kernels might stop building with warns:
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
...

Arnaldo, could you check what warns do you see with this fixed pahole
in bpf tree ?
If there are only few warns then we can manually add __used noinline
to these places, push to bpf tree and push to stable.

Then in bpf-next we can clean up everything with __bpf_kfunc.

Arnaldo Carvalho de Melo Feb. 1, 2023, 5:01 p.m. UTC | #21

Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
> On Wed, Feb 1, 2023 at 7:19 AM David Vernet <[e-mail]> wrote:
> >
> > On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
> > > > On 01/02/2023 03:02, David Vernet wrote:
> > > > > On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> > > > >> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> > > > >>>
> > > > >>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > > > >>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > > > >>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > > > >>>>> <[e-mail]> wrote:
> > > > >>>>>>
> > > > >>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > > > >>>>>>>
> > > > >>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > > > >>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > >>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > > > >>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > > > >>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > >>>>>>>>>>>> +++ b/dwarves.h
> > > > >>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > > > >>>>>>>>>>>>   uint8_t          has_addr_info:1;
> > > > >>>>>>>>>>>>   uint8_t          uses_global_strings:1;
> > > > >>>>>>>>>>>>   uint8_t          little_endian:1;
> > > > >>>>>>>>>>>> + uint8_t          nr_register_params;
> > > > >>>>>>>>>>>>   uint16_t         language;
> > > > >>>>>>>>>>>>   unsigned long    nr_inline_expansions;
> > > > >>>>>>>>>>>>   size_t           size_inline_expansions;
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
> > > > >>>>>>>>>
> > > > >>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > > > >>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > > > >>>>>>>>>> defined if using a very old elf.h; below works around this
> > > > >>>>>>>>>> (dwarves otherwise builds fine on this system).
> > > > >>>>>>>>>
> > > > >>>>>>>>> Ok, will add it and will test with containers for older distros too.
> > > > >>>>>>>>
> > > > >>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > > > >>>>>>>> repo at:
> > > > >>>>>>>>
> > > > >>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > > > >>>>>>>>
> > > > >>>>>>>> It failed yesterday and today due to problems with the installation of
> > > > >>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
> > > > >>>>>>>> notifications floating by.
> > > > >>>>>>>>
> > > > >>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
> > > > >>>>>>>> iterator that Jiri noticed.
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
> > > > >>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > > > >>>>>>> from the BTF representation, and as a result:
> > > > >>>>>>>
> > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > > > >>>>>>>
> > > > >>>>>>> Not sure why I didn't notice this previously.
> > > > >>>>>>>
> > > > >>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
> > > > >>>>>>>
> > > > >>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > > >>>>>>> {
> > > > >>>>>>>         return -EOPNOTSUPP;
> > > > >>>>>>> }
> > > > >>>>>>>
> > > > >>>>>>> looks like this:
> > > > >>>>>>>
> > > > >>>>>>>    <8af83a2>   DW_AT_external    : 1
> > > > >>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > > > >>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
> > > > >>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
> > > > >>>>>>>     <8af83a9>   DW_AT_decl_column : 5
> > > > >>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
> > > > >>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > > > >>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > > > >>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > > > >>>>>>>     <8af83b3>   DW_AT_name        : ctx
> > > > >>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
> > > > >>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
> > > > >>>>>>>     <8af83ba>   DW_AT_decl_column : 51
> > > > >>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > > > >>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > > > >>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > > > >>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
> > > > >>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
> > > > >>>>>>>     <8af83c7>   DW_AT_decl_column : 61
> > > > >>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > > > >>>>>>>
> > > > >>>>>>> ...and because there are no further abstract origin references
> > > > >>>>>>> with location information either, we classify it as lacking
> > > > >>>>>>> locations for (some of) the parameters, and as a result
> > > > >>>>>>> we skip BTF encoding. We can work around that by doing this:
> > > > >>>>>>>
> > > > >>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > > >>>>>>
> > > > >>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
> > > > >>>>>>
> > > > >>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
> > > > >>>>>> applied to arguments.
> > > > >>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
> > > > >>>>>> the way we do in selftests.
> > > > >>>>>
> > > > >>>>> There is also
> > > > >>>>> # define __visible __attribute__((__externally_visible__))
> > > > >>>>> that probably fits the best here.
> > > > >>>>>
> > > > >>>>
> > > > >>>> testing thus for seems to show that for x86_64, David's series
> > > > >>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
> > > > >>>> to cover recently-arrived kfuncs like cpumask) is sufficient
> > > > >>>> to avoid resolve_btfids warnings.
> > > > >>>
> > > > >>> Nice. Alexei -- lmk how you want to proceed. I think using the
> > > > >>> __bpf_kfunc macro in the short term (with __used and noinline) is
> > > > >>> probably the least controversial way to unblock this, but am open to
> > > > >>> other suggestions.
> > > > >>
> > > > >> Sounds good to me, but sounds like __used and noinline are not
> > > > >> enough to address the issues on aarch64?
> > > > >
> > > > > Indeed, we'll have to make sure that's also addressed. Alan -- did you
> > > > > try Alexei's suggestion to use __weak? Does that fix the issue for
> > > > > aarch64? I'm still confused as to why it's only complaining for a small
> > > > > subset of kfuncs, which include those that have external linkage.
> > > > >
> > > >
> > > > I finally got to the bottom of the aarch64 issues; there was a 1-line bug
> > > > in the changes I made to the DWARF handling code which leads to BTF generation;
> > > > it was excluding a bunch of functions incorrectly, marking them as optimized out.
> > > > The fix is:
> > > >
> > > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > > index dba2d37..8364e17 100644
> > > > --- a/dwarf_loader.c
> > > > +++ b/dwarf_loader.c
> > > > @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> > > >                         Dwarf_Op *expr = loc.expr;
> > > >
> > > >                         switch (expr->atom) {
> > > > -                       case DW_OP_reg1 ... DW_OP_reg31:
> > > > +                       case DW_OP_reg0 ... DW_OP_reg31:
> > > >                         case DW_OP_breg0 ... DW_OP_breg31:
> > > >                                 break;
> > > >                         default:
> > > >
> > > > ..and because reg0 is the first parameter for aarch64, we were
> > > > incorrectly landing in the "default:" of the switch statement
> > > > and marking a bunch of functions as optimized out
> > > > because we thought the first argument was. Sorry about this,
> > > > and thanks for all the suggestions!
> >
> > Great, so inline and __used with __bpf_kfunc sounds like the way forward
> > in the short term. Arnaldo / Alexei -- how do you want to resolve the
> > dependency here? Going through bpf-next is probably a good idea so that
> > we get proper CI coverage, and any kfuncs added to bpf-next after this
> > can use the macro. Does that work for you?
> 
> It feels fixed pahole should be done under some flag
> otherwise when people update the pahole the existing and older
> kernels might stop building with warns:
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> ...
> 
> Arnaldo, could you check what warns do you see with this fixed pahole
> in bpf tree ?

Sure.

> If there are only few warns then we can manually add __used noinline
> to these places, push to bpf tree and push to stable.
> 
> Then in bpf-next we can clean up everything with __bpf_kfunc.

Alan Maguire Feb. 1, 2023, 5:18 p.m. UTC | #22

On 01/02/2023 17:01, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
>> On Wed, Feb 1, 2023 at 7:19 AM David Vernet <[e-mail]> wrote:
>>>
>>> On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
>>>>> On 01/02/2023 03:02, David Vernet wrote:
>>>>>> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>>>>>>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>>>>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>>>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>>>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>>>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>>>>>>   uint16_t         language;
>>>>>>>>>>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>>>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>>>>>>> repo at:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>>>>>>
>>>>>>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>>>>>>> notifications floating by.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>>>>>>> iterator that Jiri noticed.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>>>>>>> from the BTF representation, and as a result:
>>>>>>>>>>>>
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>>>>>>
>>>>>>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>>>>>>
>>>>>>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>>>>>>
>>>>>>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>> {
>>>>>>>>>>>>         return -EOPNOTSUPP;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> looks like this:
>>>>>>>>>>>>
>>>>>>>>>>>>    <8af83a2>   DW_AT_external    : 1
>>>>>>>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83a9>   DW_AT_decl_column : 5
>>>>>>>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>>>>>>     <8af83b3>   DW_AT_name        : ctx
>>>>>>>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83ba>   DW_AT_decl_column : 51
>>>>>>>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>>>>>>     <8af83c7>   DW_AT_decl_column : 61
>>>>>>>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>>>>>>
>>>>>>>>>>>> ...and because there are no further abstract origin references
>>>>>>>>>>>> with location information either, we classify it as lacking
>>>>>>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>>>>>>
>>>>>>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>
>>>>>>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>>>>>>
>>>>>>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>>>>>>> applied to arguments.
>>>>>>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>>>>>>> the way we do in selftests.
>>>>>>>>>>
>>>>>>>>>> There is also
>>>>>>>>>> # define __visible __attribute__((__externally_visible__))
>>>>>>>>>> that probably fits the best here.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> testing thus for seems to show that for x86_64, David's series
>>>>>>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>>>>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>>>>>>> to avoid resolve_btfids warnings.
>>>>>>>>
>>>>>>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>>>>>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>>>>>>> probably the least controversial way to unblock this, but am open to
>>>>>>>> other suggestions.
>>>>>>>
>>>>>>> Sounds good to me, but sounds like __used and noinline are not
>>>>>>> enough to address the issues on aarch64?
>>>>>>
>>>>>> Indeed, we'll have to make sure that's also addressed. Alan -- did you
>>>>>> try Alexei's suggestion to use __weak? Does that fix the issue for
>>>>>> aarch64? I'm still confused as to why it's only complaining for a small
>>>>>> subset of kfuncs, which include those that have external linkage.
>>>>>>
>>>>>
>>>>> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
>>>>> in the changes I made to the DWARF handling code which leads to BTF generation;
>>>>> it was excluding a bunch of functions incorrectly, marking them as optimized out.
>>>>> The fix is:
>>>>>
>>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>>> index dba2d37..8364e17 100644
>>>>> --- a/dwarf_loader.c
>>>>> +++ b/dwarf_loader.c
>>>>> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>>                         Dwarf_Op *expr = loc.expr;
>>>>>
>>>>>                         switch (expr->atom) {
>>>>> -                       case DW_OP_reg1 ... DW_OP_reg31:
>>>>> +                       case DW_OP_reg0 ... DW_OP_reg31:
>>>>>                         case DW_OP_breg0 ... DW_OP_breg31:
>>>>>                                 break;
>>>>>                         default:
>>>>>
>>>>> ..and because reg0 is the first parameter for aarch64, we were
>>>>> incorrectly landing in the "default:" of the switch statement
>>>>> and marking a bunch of functions as optimized out
>>>>> because we thought the first argument was. Sorry about this,
>>>>> and thanks for all the suggestions!
>>>
>>> Great, so inline and __used with __bpf_kfunc sounds like the way forward
>>> in the short term. Arnaldo / Alexei -- how do you want to resolve the
>>> dependency here? Going through bpf-next is probably a good idea so that
>>> we get proper CI coverage, and any kfuncs added to bpf-next after this
>>> can use the macro. Does that work for you?
>>
>> It feels fixed pahole should be done under some flag
>> otherwise when people update the pahole the existing and older
>> kernels might stop building with warns:
>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>> ...
>>

Good point, something like

--skip_inconsistent_proto	Skip functions that have multiple inconsistent
				function prototypes sharing the same name, or
				have optimized-out parameters.

? Implementation needs a bit of thought though because we're
not really doing the same thing that we were before. Previously we
were adding the first instance of a function in the CU we came across.
Probably safest to resurrect that behaviour for the legacy
non-skip-inconsistent-proto case I think. The final patch handling
inconsistent function prototypes will need to be reworked a bit to 
support this, since we tossed this approach and used saving/merging 
multiple instances in the tree instead.  Once I've built bpf trees I'll
have a go at getting this working.

>> Arnaldo, could you check what warns do you see with this fixed pahole
>> in bpf tree ?
> 
> Sure.
> 

I can collect this for x86_64/aarch64 too; might take a few hours
before I have the results.

>> If there are only few warns then we can manually add __used noinline
>> to these places, push to bpf tree and push to stable.
>>
>> Then in bpf-next we can clean up everything with __bpf_kfunc.
>

Arnaldo Carvalho de Melo Feb. 1, 2023, 6:54 p.m. UTC | #23

Em Wed, Feb 01, 2023 at 05:18:29PM +0000, Alan Maguire escreveu:
> On 01/02/2023 17:01, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
> >> It feels fixed pahole should be done under some flag
> >> otherwise when people update the pahole the existing and older
> >> kernels might stop building with warns:
> >> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> >> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> >> ...
 
> Good point, something like
 
> --skip_inconsistent_proto	Skip functions that have multiple inconsistent
> 				function prototypes sharing the same name, or
> 				have optimized-out parameters.

We have:

⬢[acme@toolbox pahole]$ grep '"skip_encoding.*' pahole.c
		.name = "skip_encoding_btf_vars",
		.name = "skip_encoding_btf_decl_tag",
		.name = "skip_encoding_btf_type_tag",
		.name = "skip_encoding_btf_enum64",
⬢[acme@toolbox pahole]$

Perhaps, even being long, we should be consistent and name it:

	--skip_encoding_btf_inconsistent_proto

?
 
> ? Implementation needs a bit of thought though because we're
> not really doing the same thing that we were before. Previously we
> were adding the first instance of a function in the CU we came across.
> Probably safest to resurrect that behaviour for the legacy
> non-skip-inconsistent-proto case I think. The final patch handling

Consider getting what I have now in my next branch, that has the fixups
I made while reviewing, as discussed in this thread:

⬢[acme@toolbox pahole]$ git log --oneline -6
b1576cf15106efd7 (HEAD -> master) pahole: Sync with libbpf-1.1
e9db5622d97395b7 btf_encoder: Delay function addition to check for function prototype inconsistencies
74675488e8ed5718 btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF
be470fa5757e5915 btf_encoder: Rework btf_encoders__*() API to allow traversal of encoders
d6e0778f6b5912da btf_encoder: Refactor function addition into dedicated btf_encoder__add_func
f77b5ae93844b5c4 dwarf_loader: Help spotting functions with optimized-out parameters
⬢[acme@toolbox pahole]$

And at the point where you change the behaviour you introduce the
option, so that we don't have to remove it and then ressurect.

- Arnaldo

> inconsistent function prototypes will need to be reworked a bit to 
> support this, since we tossed this approach and used saving/merging 
> multiple instances in the tree instead.  Once I've built bpf trees I'll
> have a go at getting this working.
> 
> >> Arnaldo, could you check what warns do you see with this fixed pahole
> >> in bpf tree ?
> > 
> > Sure.
> > 
> 
> I can collect this for x86_64/aarch64 too; might take a few hours
> before I have the results.
> 
> >> If there are only few warns then we can manually add __used noinline
> >> to these places, push to bpf tree and push to stable.
> >>
> >> Then in bpf-next we can clean up everything with __bpf_kfunc.
> >

Arnaldo Carvalho de Melo Feb. 1, 2023, 10:32 p.m. UTC | #24

Em Wed, Feb 01, 2023 at 02:01:47PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
> > On Wed, Feb 1, 2023 at 7:19 AM David Vernet <[e-mail]> wrote:
> > >
> > > On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
> > > > > On 01/02/2023 03:02, David Vernet wrote:
> > > > > > On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
> > > > > >> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
> > > > > >>>
> > > > > >>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
> > > > > >>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
> > > > > >>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
> > > > > >>>>> <[e-mail]> wrote:
> > > > > >>>>>>
> > > > > >>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
> > > > > >>>>>>>
> > > > > >>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
> > > > > >>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > >>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
> > > > > >>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
> > > > > >>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > >>>>>>>>>>>> +++ b/dwarves.h
> > > > > >>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
> > > > > >>>>>>>>>>>>   uint8_t          has_addr_info:1;
> > > > > >>>>>>>>>>>>   uint8_t          uses_global_strings:1;
> > > > > >>>>>>>>>>>>   uint8_t          little_endian:1;
> > > > > >>>>>>>>>>>> + uint8_t          nr_register_params;
> > > > > >>>>>>>>>>>>   uint16_t         language;
> > > > > >>>>>>>>>>>>   unsigned long    nr_inline_expansions;
> > > > > >>>>>>>>>>>>   size_t           size_inline_expansions;
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
> > > > > >>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
> > > > > >>>>>>>>>> defined if using a very old elf.h; below works around this
> > > > > >>>>>>>>>> (dwarves otherwise builds fine on this system).
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Ok, will add it and will test with containers for older distros too.
> > > > > >>>>>>>>
> > > > > >>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
> > > > > >>>>>>>> repo at:
> > > > > >>>>>>>>
> > > > > >>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
> > > > > >>>>>>>>
> > > > > >>>>>>>> It failed yesterday and today due to problems with the installation of
> > > > > >>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
> > > > > >>>>>>>> notifications floating by.
> > > > > >>>>>>>>
> > > > > >>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
> > > > > >>>>>>>> iterator that Jiri noticed.
> > > > > >>>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
> > > > > >>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
> > > > > >>>>>>> from the BTF representation, and as a result:
> > > > > >>>>>>>
> > > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > > > > >>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> > > > > >>>>>>>
> > > > > >>>>>>> Not sure why I didn't notice this previously.
> > > > > >>>>>>>
> > > > > >>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
> > > > > >>>>>>>
> > > > > >>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > > > >>>>>>> {
> > > > > >>>>>>>         return -EOPNOTSUPP;
> > > > > >>>>>>> }
> > > > > >>>>>>>
> > > > > >>>>>>> looks like this:
> > > > > >>>>>>>
> > > > > >>>>>>>    <8af83a2>   DW_AT_external    : 1
> > > > > >>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
> > > > > >>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
> > > > > >>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
> > > > > >>>>>>>     <8af83a9>   DW_AT_decl_column : 5
> > > > > >>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
> > > > > >>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
> > > > > >>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
> > > > > >>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
> > > > > >>>>>>>     <8af83b3>   DW_AT_name        : ctx
> > > > > >>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
> > > > > >>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
> > > > > >>>>>>>     <8af83ba>   DW_AT_decl_column : 51
> > > > > >>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
> > > > > >>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
> > > > > >>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
> > > > > >>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
> > > > > >>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
> > > > > >>>>>>>     <8af83c7>   DW_AT_decl_column : 61
> > > > > >>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
> > > > > >>>>>>>
> > > > > >>>>>>> ...and because there are no further abstract origin references
> > > > > >>>>>>> with location information either, we classify it as lacking
> > > > > >>>>>>> locations for (some of) the parameters, and as a result
> > > > > >>>>>>> we skip BTF encoding. We can work around that by doing this:
> > > > > >>>>>>>
> > > > > >>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
> > > > > >>>>>>
> > > > > >>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
> > > > > >>>>>>
> > > > > >>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
> > > > > >>>>>> applied to arguments.
> > > > > >>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
> > > > > >>>>>> the way we do in selftests.
> > > > > >>>>>
> > > > > >>>>> There is also
> > > > > >>>>> # define __visible __attribute__((__externally_visible__))
> > > > > >>>>> that probably fits the best here.
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>> testing thus for seems to show that for x86_64, David's series
> > > > > >>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
> > > > > >>>> to cover recently-arrived kfuncs like cpumask) is sufficient
> > > > > >>>> to avoid resolve_btfids warnings.
> > > > > >>>
> > > > > >>> Nice. Alexei -- lmk how you want to proceed. I think using the
> > > > > >>> __bpf_kfunc macro in the short term (with __used and noinline) is
> > > > > >>> probably the least controversial way to unblock this, but am open to
> > > > > >>> other suggestions.
> > > > > >>
> > > > > >> Sounds good to me, but sounds like __used and noinline are not
> > > > > >> enough to address the issues on aarch64?
> > > > > >
> > > > > > Indeed, we'll have to make sure that's also addressed. Alan -- did you
> > > > > > try Alexei's suggestion to use __weak? Does that fix the issue for
> > > > > > aarch64? I'm still confused as to why it's only complaining for a small
> > > > > > subset of kfuncs, which include those that have external linkage.
> > > > > >
> > > > >
> > > > > I finally got to the bottom of the aarch64 issues; there was a 1-line bug
> > > > > in the changes I made to the DWARF handling code which leads to BTF generation;
> > > > > it was excluding a bunch of functions incorrectly, marking them as optimized out.
> > > > > The fix is:
> > > > >
> > > > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > > > index dba2d37..8364e17 100644
> > > > > --- a/dwarf_loader.c
> > > > > +++ b/dwarf_loader.c
> > > > > @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> > > > >                         Dwarf_Op *expr = loc.expr;
> > > > >
> > > > >                         switch (expr->atom) {
> > > > > -                       case DW_OP_reg1 ... DW_OP_reg31:
> > > > > +                       case DW_OP_reg0 ... DW_OP_reg31:
> > > > >                         case DW_OP_breg0 ... DW_OP_breg31:
> > > > >                                 break;
> > > > >                         default:
> > > > >
> > > > > ..and because reg0 is the first parameter for aarch64, we were
> > > > > incorrectly landing in the "default:" of the switch statement
> > > > > and marking a bunch of functions as optimized out
> > > > > because we thought the first argument was. Sorry about this,
> > > > > and thanks for all the suggestions!
> > >
> > > Great, so inline and __used with __bpf_kfunc sounds like the way forward
> > > in the short term. Arnaldo / Alexei -- how do you want to resolve the
> > > dependency here? Going through bpf-next is probably a good idea so that
> > > we get proper CI coverage, and any kfuncs added to bpf-next after this
> > > can use the macro. Does that work for you?
> > 
> > It feels fixed pahole should be done under some flag
> > otherwise when people update the pahole the existing and older
> > kernels might stop building with warns:
> > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > ...
> > 
> > Arnaldo, could you check what warns do you see with this fixed pahole
> > in bpf tree ?
> 
> Sure.

These appeared on a distro like .config:

  BTFIDS  vmlinux
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_cpumask_any
WARN: resolve_btfids: unresolved symbol bpf_ct_change_status

I'll do it with allmodconfig
 
> > If there are only few warns then we can manually add __used noinline
> > to these places, push to bpf tree and push to stable.
> > 
> > Then in bpf-next we can clean up everything with __bpf_kfunc.
> 
> -- 
> 
> - Arnaldo

Alan Maguire Feb. 1, 2023, 10:33 p.m. UTC | #25

On 01/02/2023 17:18, Alan Maguire wrote:
> On 01/02/2023 17:01, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
>>> On Wed, Feb 1, 2023 at 7:19 AM David Vernet <[e-mail]> wrote:
>>>>
>>>> On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
>>>>> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
>>>>>> On 01/02/2023 03:02, David Vernet wrote:
>>>>>>> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>>>>>>>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>>>>>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>>>>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>>>>>>>   uint8_t          has_addr_info:1;
>>>>>>>>>>>>>>>>>>   uint8_t          uses_global_strings:1;
>>>>>>>>>>>>>>>>>>   uint8_t          little_endian:1;
>>>>>>>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>>>>>>>   uint16_t         language;
>>>>>>>>>>>>>>>>>>   unsigned long    nr_inline_expansions;
>>>>>>>>>>>>>>>>>>   size_t           size_inline_expansions;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>>>>>>>> repo at:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>>>>>>>> notifications floating by.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>>>>>>>> iterator that Jiri noticed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>>>>>>>> from the BTF representation, and as a result:
>>>>>>>>>>>>>
>>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>>>>>>>
>>>>>>>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>>>>>>>
>>>>>>>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>>> {
>>>>>>>>>>>>>         return -EOPNOTSUPP;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> looks like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>>    <8af83a2>   DW_AT_external    : 1
>>>>>>>>>>>>>     <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>>>>>>>     <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>>>>>>>     <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>>>>>>>     <8af83a9>   DW_AT_decl_column : 5
>>>>>>>>>>>>>     <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>>>>>>>     <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>>>>>>>     <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>>>>>>>  <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>>>>>>>     <8af83b3>   DW_AT_name        : ctx
>>>>>>>>>>>>>     <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>>>>>>>     <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>>>>>>>     <8af83ba>   DW_AT_decl_column : 51
>>>>>>>>>>>>>     <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>>>>>>>  <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>>>>>>>     <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>>>>>>>     <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>>>>>>>     <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>>>>>>>     <8af83c7>   DW_AT_decl_column : 61
>>>>>>>>>>>>>     <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ...and because there are no further abstract origin references
>>>>>>>>>>>>> with location information either, we classify it as lacking
>>>>>>>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>>
>>>>>>>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>>>>>>>
>>>>>>>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>>>>>>>> applied to arguments.
>>>>>>>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>>>>>>>> the way we do in selftests.
>>>>>>>>>>>
>>>>>>>>>>> There is also
>>>>>>>>>>> # define __visible __attribute__((__externally_visible__))
>>>>>>>>>>> that probably fits the best here.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> testing thus for seems to show that for x86_64, David's series
>>>>>>>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>>>>>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>>>>>>>> to avoid resolve_btfids warnings.
>>>>>>>>>
>>>>>>>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>>>>>>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>>>>>>>> probably the least controversial way to unblock this, but am open to
>>>>>>>>> other suggestions.
>>>>>>>>
>>>>>>>> Sounds good to me, but sounds like __used and noinline are not
>>>>>>>> enough to address the issues on aarch64?
>>>>>>>
>>>>>>> Indeed, we'll have to make sure that's also addressed. Alan -- did you
>>>>>>> try Alexei's suggestion to use __weak? Does that fix the issue for
>>>>>>> aarch64? I'm still confused as to why it's only complaining for a small
>>>>>>> subset of kfuncs, which include those that have external linkage.
>>>>>>>
>>>>>>
>>>>>> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
>>>>>> in the changes I made to the DWARF handling code which leads to BTF generation;
>>>>>> it was excluding a bunch of functions incorrectly, marking them as optimized out.
>>>>>> The fix is:
>>>>>>
>>>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>>>> index dba2d37..8364e17 100644
>>>>>> --- a/dwarf_loader.c
>>>>>> +++ b/dwarf_loader.c
>>>>>> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>>>                         Dwarf_Op *expr = loc.expr;
>>>>>>
>>>>>>                         switch (expr->atom) {
>>>>>> -                       case DW_OP_reg1 ... DW_OP_reg31:
>>>>>> +                       case DW_OP_reg0 ... DW_OP_reg31:
>>>>>>                         case DW_OP_breg0 ... DW_OP_breg31:
>>>>>>                                 break;
>>>>>>                         default:
>>>>>>
>>>>>> ..and because reg0 is the first parameter for aarch64, we were
>>>>>> incorrectly landing in the "default:" of the switch statement
>>>>>> and marking a bunch of functions as optimized out
>>>>>> because we thought the first argument was. Sorry about this,
>>>>>> and thanks for all the suggestions!
>>>>
>>>> Great, so inline and __used with __bpf_kfunc sounds like the way forward
>>>> in the short term. Arnaldo / Alexei -- how do you want to resolve the
>>>> dependency here? Going through bpf-next is probably a good idea so that
>>>> we get proper CI coverage, and any kfuncs added to bpf-next after this
>>>> can use the macro. Does that work for you?
>>>
>>> It feels fixed pahole should be done under some flag
>>> otherwise when people update the pahole the existing and older
>>> kernels might stop building with warns:
>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>> ...
>>>
> 
> Good point, something like
> 
> --skip_inconsistent_proto	Skip functions that have multiple inconsistent
> 				function prototypes sharing the same name, or
> 				have optimized-out parameters.
> 
> ? Implementation needs a bit of thought though because we're
> not really doing the same thing that we were before. Previously we
> were adding the first instance of a function in the CU we came across.
> Probably safest to resurrect that behaviour for the legacy
> non-skip-inconsistent-proto case I think. The final patch handling
> inconsistent function prototypes will need to be reworked a bit to 
> support this, since we tossed this approach and used saving/merging 
> multiple instances in the tree instead.  Once I've built bpf trees I'll
> have a go at getting this working.
> 
>>> Arnaldo, could you check what warns do you see with this fixed pahole
>>> in bpf tree ?
>>
>> Sure.
>>
> 
> I can collect this for x86_64/aarch64 too; might take a few hours
> before I have the results.
>

The results I'm seeing with the bpf tree across x86_64 and aarch64 are 
consistent using the updated pahole:

WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_ct_change_status

 
>>> If there are only few warns then we can manually add __used noinline
>>> to these places, push to bpf tree and push to stable.
>>>
>>> Then in bpf-next we can clean up everything with __bpf_kfunc.
>>

If the skipping of inconsistent prototype functions happens under 
a flag and not by default, presumably we'd have something like
a 3-patch series for bpf; one patch with an update to scripts/pahole-flags.sh

if [ "${pahole_ver}" -ge "125" ]; then
	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto"
fi

...so that we can enable building with 1.25, and then two additional patches adding
__used noinline prefixes to bpf_ct_change_status and bpf_task_kptr_get(); splitting 
these out into separate patches would probably make sense as different stable trees
might need one but not the other. I _think_ that's what you have in mind, is that
right?

Arnaldo Carvalho de Melo Feb. 2, 2023, 1:09 a.m. UTC | #26

Em Wed, Feb 01, 2023 at 07:32:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Feb 01, 2023 at 02:01:47PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
> > > > Great, so inline and __used with __bpf_kfunc sounds like the way forward
> > > > in the short term. Arnaldo / Alexei -- how do you want to resolve the
> > > > dependency here? Going through bpf-next is probably a good idea so that
> > > > we get proper CI coverage, and any kfuncs added to bpf-next after this
> > > > can use the macro. Does that work for you?
> > > 
> > > It feels fixed pahole should be done under some flag
> > > otherwise when people update the pahole the existing and older
> > > kernels might stop building with warns:
> > > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> > > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> > > ...
> > > 
> > > Arnaldo, could you check what warns do you see with this fixed pahole
> > > in bpf tree ?
> > 
> > Sure.
> 
> These appeared on a distro like .config:
> 
>   BTFIDS  vmlinux
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> WARN: resolve_btfids: unresolved symbol bpf_cpumask_any
> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
> 
> I'll do it with allmodconfig

^C[1]+  Done                    nohup make -j32 O=../build/allmodconfig

⬢[acme@toolbox bpf-next]$
⬢[acme@toolbox bpf-next]$ grep "^WARN: resolve_btfids: " nohup.out
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_cpumask_any
WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
⬢[acme@toolbox bpf-next]$

Yonghong Song Feb. 3, 2023, 1:09 a.m. UTC | #27

On 1/31/23 7:02 PM, David Vernet wrote:
> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <[e-mail]> wrote:
>>>
>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>    uint8_t          has_addr_info:1;
>>>>>>>>>>>>    uint8_t          uses_global_strings:1;
>>>>>>>>>>>>    uint8_t          little_endian:1;
>>>>>>>>>>>> + uint8_t          nr_register_params;
>>>>>>>>>>>>    uint16_t         language;
>>>>>>>>>>>>    unsigned long    nr_inline_expansions;
>>>>>>>>>>>>    size_t           size_inline_expansions;
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>
>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>
>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>
>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>> repo at:
>>>>>>>>
>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>
>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>> notifications floating by.
>>>>>>>>
>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>> iterator that Jiri noticed.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>> from the BTF representation, and as a result:
>>>>>>>
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>
>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>
>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>
>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>> {
>>>>>>>          return -EOPNOTSUPP;
>>>>>>> }
>>>>>>>
>>>>>>> looks like this:
>>>>>>>
>>>>>>>     <8af83a2>   DW_AT_external    : 1
>>>>>>>      <8af83a2>   DW_AT_name        : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>      <8af83a6>   DW_AT_decl_file   : 5
>>>>>>>      <8af83a7>   DW_AT_decl_line   : 737
>>>>>>>      <8af83a9>   DW_AT_decl_column : 5
>>>>>>>      <8af83aa>   DW_AT_prototyped  : 1
>>>>>>>      <8af83aa>   DW_AT_type        : <0x8ad8547>
>>>>>>>      <8af83ae>   DW_AT_sibling     : <0x8af83cd>
>>>>>>>   <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>      <8af83b3>   DW_AT_name        : ctx
>>>>>>>      <8af83b7>   DW_AT_decl_file   : 5
>>>>>>>      <8af83b8>   DW_AT_decl_line   : 737
>>>>>>>      <8af83ba>   DW_AT_decl_column : 51
>>>>>>>      <8af83bb>   DW_AT_type        : <0x8af421d>
>>>>>>>   <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>      <8af83c0>   DW_AT_name        : (indirect string, offset: 0x27f6a2): hash
>>>>>>>      <8af83c4>   DW_AT_decl_file   : 5
>>>>>>>      <8af83c5>   DW_AT_decl_line   : 737
>>>>>>>      <8af83c7>   DW_AT_decl_column : 61
>>>>>>>      <8af83c8>   DW_AT_type        : <0x8adc424>
>>>>>>>
>>>>>>> ...and because there are no further abstract origin references
>>>>>>> with location information either, we classify it as lacking
>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>
>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>
>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>
>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>> applied to arguments.
>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>> the way we do in selftests.
>>>>>
>>>>> There is also
>>>>> # define __visible __attribute__((__externally_visible__))
>>>>> that probably fits the best here.
>>>>>
>>>>
>>>> testing thus for seems to show that for x86_64, David's series
>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>> to avoid resolve_btfids warnings.
>>>
>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>> probably the least controversial way to unblock this, but am open to
>>> other suggestions.
>>
>> Sounds good to me, but sounds like __used and noinline are not
>> enough to address the issues on aarch64?
> 
> Indeed, we'll have to make sure that's also addressed. Alan -- did you
> try Alexei's suggestion to use __weak? Does that fix the issue for
> aarch64? I'm still confused as to why it's only complaining for a small
> subset of kfuncs, which include those that have external linkage.
> 
>>
>>> Yeah, I tend to think we should try to avoid using hidden / visible
>>> attributes given that (to my knowledge) they're really more meant for
>>> controlling whether a symbol is exported from a shared object rather
>>> than controlling what the compiler is doing when it creates the
>>> compilation unit. One could imagine that in an LTO build, the compiler
>>> would still optimize the function regardless of its visibility for that
>>> reason, though it's possible I don't have the full picture.
>>
>> __visible is specifically done to prevent optimization of
>> functions that are externally visible. That should address LTO concerns.
>> We haven't seen LTO messing up anything. Just something to keep in mind.
> 
> Ah, fair enough. I was conflating that with the visibility("...")
> attribute. As you pointed out, __visible is something else entirely, and
> is meant to avoid possible issues with LTO.
> 
> One other option we could consider is enforcing that kfuncs must have
> global linkage and can't be static. If we did that, it seems like

Do we really want static function to be kfuncs? It may work if we ensure
the same function name is not used in other files. But it sounds weird
since kfunc can be considered as an 'export' function (to be used by
bpf programs) which in general should have global linkage?

> __visible would be a viable option. Though we'd have to verify that it
> addresses the issue w/ aarch64.

Patch

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 5a74035..93c2307 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -992,13 +992,98 @@  static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
 	return member;
 }
 
-static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
+/* How many function parameters are passed via registers?  Used below in
+ * determining if an argument has been optimized out or if it is simply
+ * an argument > NR_REGISTER_PARAMS.  Setting NR_REGISTER_PARAMS to 0
+ * allows unsupported architectures to skip tagging optimized-out
+ * values.
+ */
+#if defined(__x86_64__)
+#define NR_REGISTER_PARAMS      6
+#elif defined(__s390__)
+#define NR_REGISTER_PARAMS	5
+#elif defined(__aarch64__)
+#define NR_REGISTER_PARAMS      8
+#elif defined(__mips__)
+#define NR_REGISTER_PARAMS	8
+#elif defined(__powerpc__)
+#define NR_REGISTER_PARAMS	8
+#elif defined(__sparc__)
+#define NR_REGISTER_PARAMS	6
+#elif defined(__riscv) && __riscv_xlen == 64
+#define NR_REGISTER_PARAMS	8
+#elif defined(__arc__)
+#define NR_REGISTER_PARAMS	8
+#else
+#define NR_REGISTER_PARAMS      0
+#endif
+
+static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
+					struct conf_load *conf, int param_idx)
 {
 	struct parameter *parm = tag__alloc(cu, sizeof(*parm));
 
 	if (parm != NULL) {
+		bool has_const_value;
+		Dwarf_Attribute attr;
+		struct location loc;
+
 		tag__init(&parm->tag, cu, die);
 		parm->name = attr_string(die, DW_AT_name, conf);
+
+		if (param_idx >= NR_REGISTER_PARAMS)
+			return parm;
+		/* Parameters which use DW_AT_abstract_origin to point at
+		 * the original parameter definition (with no name in the DIE)
+		 * are the result of later DWARF generation during compilation
+		 * so often better take into account if arguments were
+		 * optimized out.
+		 *
+		 * By checking that locations for parameters that are expected
+		 * to be passed as registers are actually passed as registers,
+		 * we can spot optimized-out parameters.
+		 *
+		 * It can also be the case that a parameter DIE has
+		 * a constant value attribute reflecting optimization or
+		 * has no location attribute.
+		 *
+		 * From the DWARF spec:
+		 *
+		 * "4.1.10
+		 *
+		 * A DW_AT_const_value attribute for an entry describing a
+		 * variable or formal parameter whose value is constant and not
+		 * represented by an object in the address space of the program,
+		 * or an entry describing a named constant. (Note
+		 * that such an entry does not have a location attribute.)"
+		 *
+		 * So we can also use the absence of a location for a parameter
+		 * as evidence it has been optimized out.  This info will
+		 * need to be shared between a parameter and any abstract
+		 * origin references however, since gcc can have location
+		 * information in the parameter that refers back to the original
+		 * via abstract origin, so we need to share location presence
+		 * between these parameter representations.  See
+		 * ftype__recode_dwarf_types() below for how this is handled.
+		 */
+		parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
+		has_const_value = dwarf_attr(die, DW_AT_const_value, &attr) != NULL;
+		if (parm->has_loc &&
+		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
+			loc.exprlen != 0) {
+			Dwarf_Op *expr = loc.expr;
+
+			switch (expr->atom) {
+			case DW_OP_reg1 ... DW_OP_reg31:
+			case DW_OP_breg0 ... DW_OP_breg31:
+				break;
+			default:
+				parm->optimized = 1;
+				break;
+			}
+		} else if (has_const_value) {
+			parm->optimized = 1;
+		}
 	}
 
 	return parm;
@@ -1450,7 +1535,7 @@  static struct tag *die__create_new_parameter(Dwarf_Die *die,
 					     struct cu *cu, struct conf_load *conf,
 					     int param_idx)
 {
-	struct parameter *parm = parameter__new(die, cu, conf);
+	struct parameter *parm = parameter__new(die, cu, conf, param_idx);
 
 	if (parm == NULL)
 		return NULL;
@@ -2194,6 +2279,7 @@  static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 
 	ftype__for_each_parameter(type, pos) {
 		struct dwarf_tag *dpos = pos->tag.priv;
+		struct parameter *opos;
 		struct dwarf_tag *dtype;
 
 		if (dpos->type.off == 0) {
@@ -2207,8 +2293,18 @@  static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 				tag__print_abstract_origin_not_found(&pos->tag);
 				continue;
 			}
-			pos->name = tag__parameter(dtype->tag)->name;
+			opos = tag__parameter(dtype->tag);
+			pos->name = opos->name;
 			pos->tag.type = dtype->tag->type;
+			/* share location information between parameter and
+			 * abstract origin; if neither have location, we will
+			 * mark the parameter as optimized out.
+			 */
+			if (pos->has_loc)
+				opos->has_loc = pos->has_loc;
+
+			if (pos->optimized)
+				opos->optimized = pos->optimized;
 			continue;
 		}
 
@@ -2478,18 +2574,33 @@  out:
 	return 0;
 }
 
-static int cu__resolve_func_ret_types(struct cu *cu)
+static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 {
 	struct ptr_table *pt = &cu->functions_table;
 	uint32_t i;
 
 	for (i = 0; i < pt->nr_entries; ++i) {
 		struct tag *tag = pt->entries[i];
+		struct parameter *pos;
+		struct function *fn = tag__function(tag);
+
+		/* mark function as optimized if parameter is, or
+		 * if parameter does not have a location; at this
+		 * point location presence has been marked in
+		 * abstract origins for cases where a parameter
+		 * location is not stored in the original function
+		 * parameter tag.
+		 */
+		ftype__for_each_parameter(&fn->proto, pos) {
+			if (pos->optimized || !pos->has_loc) {
+				fn->proto.optimized_parms = 1;
+				break;
+			}
+		}
 
 		if (tag == NULL || tag->type != 0)
 			continue;
 
-		struct function *fn = tag__function(tag);
 		if (!fn->abstract_origin)
 			continue;
 
@@ -2612,7 +2723,7 @@  static int die__process_and_recode(Dwarf_Die *die, struct cu *cu, struct conf_lo
 	if (ret != 0)
 		return ret;
 
-	return cu__resolve_func_ret_types(cu);
+	return cu__resolve_func_ret_types_optimized(cu);
 }
 
 static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,
@@ -3132,7 +3243,7 @@  static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
 	 * encoded in another subprogram through abstract_origin
 	 * tag. Let us visit all subprograms again to resolve this.
 	 */
-	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
+	if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
 		goto out_abort;
 
 	if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
diff --git a/dwarves.h b/dwarves.h
index 589588e..2723466 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -808,6 +808,8 @@  size_t lexblock__fprintf(const struct lexblock *lexblock, const struct cu *cu,
 struct parameter {
 	struct tag tag;
 	const char *name;
+	uint8_t optimized:1;
+	uint8_t has_loc:1;
 };
 
 static inline struct parameter *tag__parameter(const struct tag *tag)
@@ -827,7 +829,8 @@  struct ftype {
 	struct tag	 tag;
 	struct list_head parms;
 	uint16_t	 nr_parms;
-	uint8_t		 unspec_parms; /* just one bit is needed */
+	uint8_t		 unspec_parms:1; /* just one bit is needed */
+	uint8_t		 optimized_parms:1;
 };
 
 static inline struct ftype *tag__ftype(const struct tag *tag)