public inbox for bpf@vger.kernel.org
 help / color / mirror / Atom feed
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: yhs@fb.com, ast@kernel.org, olsajiri@gmail.com,
	eddyz87@gmail.com, sinquersw@gmail.com, timo@incline.eu,
	daniel@iogearbox.net, andrii@kernel.org, songliubraving@fb.com,
	john.fastabend@gmail.com, kpsingh@chromium.org, sdf@google.com,
	haoluo@google.com, martin.lau@kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v2 dwarves 1/5] dwarves: help dwarf loader spot functions with optimized-out parameters
Date: Mon, 30 Jan 2023 15:36:09 -0300	[thread overview]
Message-ID: <Y9gOGZ20aSgsYtPP@kernel.org> (raw)
In-Reply-To: <1675088985-20300-2-git-send-email-alan.maguire@oracle.com>

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 <alan.maguire@oracle.com>
> ---
>  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

  reply	other threads:[~2023-01-30 18:36 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-30 14:29 [PATCH v2 dwarves 0/5] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions Alan Maguire
2023-01-30 14:29 ` [PATCH v2 dwarves 1/5] dwarves: help dwarf loader spot functions with optimized-out parameters Alan Maguire
2023-01-30 18:36   ` Arnaldo Carvalho de Melo [this message]
2023-01-30 20:10     ` Arnaldo Carvalho de Melo
2023-01-30 20:23       ` Arnaldo Carvalho de Melo
2023-01-30 22:37         ` Alan Maguire
2023-01-31  0:25           ` Arnaldo Carvalho de Melo
2023-01-31  1:04             ` Arnaldo Carvalho de Melo
2023-01-31 12:14               ` Alan Maguire
2023-01-31 12:33                 ` Arnaldo Carvalho de Melo
2023-01-31 13:35                   ` Jiri Olsa
2023-01-31 17:43                 ` Alexei Starovoitov
2023-01-31 18:16                   ` Alexei Starovoitov
2023-01-31 23:45                     ` Alan Maguire
2023-01-31 23:58                       ` David Vernet
2023-02-01  0:14                         ` Alexei Starovoitov
2023-02-01  3:02                           ` David Vernet
2023-02-01 13:59                             ` Alan Maguire
2023-02-01 15:02                               ` Arnaldo Carvalho de Melo
2023-02-01 15:13                                 ` Alan Maguire
2023-02-01 15:19                                 ` David Vernet
2023-02-01 16:49                                   ` Alexei Starovoitov
2023-02-01 17:01                                     ` Arnaldo Carvalho de Melo
2023-02-01 17:18                                       ` Alan Maguire
2023-02-01 18:54                                         ` Arnaldo Carvalho de Melo
2023-02-01 22:33                                         ` Alan Maguire
2023-02-01 22:32                                       ` Arnaldo Carvalho de Melo
2023-02-02  1:09                                         ` Arnaldo Carvalho de Melo
2023-02-03  1:09                             ` Yonghong Song
2023-01-30 14:29 ` [PATCH v2 dwarves 2/5] btf_encoder: refactor function addition into dedicated btf_encoder__add_func Alan Maguire
2023-02-01 17:19   ` Arnaldo Carvalho de Melo
2023-02-01 17:50     ` Alan Maguire
2023-02-01 18:59       ` Arnaldo Carvalho de Melo
2023-01-30 14:29 ` [PATCH v2 dwarves 3/5] btf_encoder: rework btf_encoders__*() API to allow traversal of encoders Alan Maguire
2023-01-30 22:04   ` Jiri Olsa
2023-01-31  0:24     ` Arnaldo Carvalho de Melo
2023-01-30 14:29 ` [PATCH v2 dwarves 4/5] btf_encoder: represent "."-suffixed functions (".isra.0") in BTF Alan Maguire
2023-01-30 14:29 ` [PATCH v2 dwarves 5/5] btf_encoder: delay function addition to check for function prototype inconsistencies Alan Maguire
2023-01-30 17:20   ` Alexei Starovoitov
2023-01-30 18:08     ` Arnaldo Carvalho de Melo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y9gOGZ20aSgsYtPP@kernel.org \
    --to=acme@kernel.org \
    --cc=alan.maguire@oracle.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=eddyz87@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=kpsingh@chromium.org \
    --cc=martin.lau@kernel.org \
    --cc=olsajiri@gmail.com \
    --cc=sdf@google.com \
    --cc=sinquersw@gmail.com \
    --cc=songliubraving@fb.com \
    --cc=timo@incline.eu \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox