On Fri, Feb 17, 2023 at 11:10:30PM +0000, Alan Maguire wrote:
> Calling conventions dictate which registers are used for
> function parameters.
>
> When a function is optimized however, we need to ensure that
> the non-optimized parameters do not violate expectations about
> register use as this would violate expectations for tracing.
> At CU initialization, create a mapping from parameter index
> to expected DW_OP_reg, and use it to validate parameters
> match with expectations. A parameter which is passed via
> the stack, as a constant, or uses an unexpected register,
> violates these expectations and it (and the associated
> function) are marked as having unexpected register mapping.
>
> Note though that there is as exception here that needs to
> be handled; when a (typedef) struct is passed as a parameter,
> it can use multiple registers so will throw off later register
> expectations. Exempt functions that have unexpected
> register usage _and_ struct parameters (examples are found
> in the "tracing_struct" test).
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
> dwarves.h | 5 +++
> 2 files changed, 109 insertions(+), 5 deletions(-)
>
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index acdb68d..014e130 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
> return 0;
> }
>
> +/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
> + * This will allow us to identify cases where optimized-out parameters
> + * interfere with expectations about register contents on function
> + * entry.
> + */
> +static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
> +{
> + memset(cu->register_params, -1, sizeof(cu->register_params));
> +
> + switch (ehdr->e_machine) {
> + case EM_S390:
> + /* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
> + cu->register_params[0] = DW_OP_reg2; // %r2
> + cu->register_params[1] = DW_OP_reg3; // %r3
> + cu->register_params[2] = DW_OP_reg4; // %r4
> + cu->register_params[3] = DW_OP_reg5; // %r5
> + cu->register_params[4] = DW_OP_reg6; // %r6
> + return;
> + case EM_X86_64:
> + /* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
> + cu->register_params[0] = DW_OP_reg5; // %rdi
> + cu->register_params[1] = DW_OP_reg4; // %rsi
> + cu->register_params[2] = DW_OP_reg1; // %rdx
> + cu->register_params[3] = DW_OP_reg2; // %rcx
> + cu->register_params[4] = DW_OP_reg8; // %r8
> + cu->register_params[5] = DW_OP_reg9; // %r9
> + return;
> + case EM_ARM:
> + /* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
> + case EM_AARCH64:
> + /* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
> + cu->register_params[0] = DW_OP_reg0;
> + cu->register_params[1] = DW_OP_reg1;
> + cu->register_params[2] = DW_OP_reg2;
> + cu->register_params[3] = DW_OP_reg3;
> + cu->register_params[4] = DW_OP_reg4;
> + cu->register_params[5] = DW_OP_reg5;
> + cu->register_params[6] = DW_OP_reg6;
> + cu->register_params[7] = DW_OP_reg7;
> + return;
> + default:
> + return;
> + }
> +}
> +
> static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> struct conf_load *conf, int param_idx)
> {
> @@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> if (parm->has_loc &&
> attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
> loc.exprlen != 0) {
> + int expected_reg = cu->register_params[param_idx];
> Dwarf_Op *expr = loc.expr;
>
> switch (expr->atom) {
> case DW_OP_reg0 ... DW_OP_reg31:
> + /* mark parameters that use an unexpected
> + * register to hold a parameter; these will
> + * be problematic for users of BTF as they
> + * violate expectations about register
> + * contents.
> + */
> + if (expected_reg >= 0 && expected_reg != expr->atom)
> + parm->unexpected_reg = 1;
> + break;
Overall I guess it's a step forward, since it addresses the immediate issue,
but probably too fragile long term.
Your earlier example:
__bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)
had
0x0891dabe: DW_TAG_formal_parameter
DW_AT_location (indexed (0x7a) loclist = 0x00f50eb1:
[0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
[0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
[0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
[0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
[0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
0x0891dad4: DW_TAG_formal_parameter
DW_AT_location (indexed (0x7b) loclist = 0x00f50eda:
[0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
[0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
[0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
DW_AT_name ("acked")
Both args will fail above check. If I'm reading above code correctly.
It checks that every reg in DW_AT_location matches ?
Or just first ?
> case DW_OP_breg0 ... DW_OP_breg31:
> break;
> default:
> - parm->optimized = 1;
> + parm->unexpected_reg = 1;
> break;
> }
> } else if (has_const_value) {
> - parm->optimized = 1;
> + parm->unexpected_reg = 1;
Is this part too restrictive as well?
Just because one arg is constant it doesn't mean that the calling convention
is not correct for this and other args.
