Re: [PATCH 4/5] stack: Fix stack unwind failure on mips

[Mark Wielaard <mark@klomp.org>]

Hi,

On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
> From: Ying Huang <ying.huang@oss.cipunited.com>
> 
> add abi_cfi, set_initial_registers_tid, unwind on mips.
> "./src/stack -p PID" can show stack information
> ---
>  backends/Makefile.am    |  3 +-
>  backends/mips_cfi.c     | 68 +++++++++++++++++++++++++++++++++
>  backends/mips_init.c    |  4 ++
>  backends/mips_initreg.c | 70 ++++++++++++++++++++++++++++++++++
>  backends/mips_unwind.c  | 84 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 228 insertions(+), 1 deletion(-)
>  create mode 100644 backends/mips_cfi.c
>  create mode 100644 backends/mips_initreg.c
>  create mode 100644 backends/mips_unwind.c

This looks good. Just two questions below.

> diff --git a/backends/Makefile.am b/backends/Makefile.am
> index 428a1a03..ddc31c9d 100644
> --- a/backends/Makefile.am
> +++ b/backends/Makefile.am
> @@ -100,7 +100,8 @@ loongarch_SRCS = loongarch_init.c loongarch_symbol.c
>  
>  arc_SRCS = arc_init.c arc_symbol.c
>  
> -mips_SRCS = mips_init.c mips_symbol.c mips_attrs.c
> +mips_SRCS = mips_init.c mips_symbol.c mips_attrs.c mips_initreg.c \
> +	    mips_cfi.c mips_unwind.c

OK
 
>  libebl_backends_a_SOURCES = $(i386_SRCS) $(sh_SRCS) $(x86_64_SRCS) \
>  			    $(ia64_SRCS) $(alpha_SRCS) $(arm_SRCS) \
> diff --git a/backends/mips_cfi.c b/backends/mips_cfi.c
> new file mode 100644
> index 00000000..77132cc1
> --- /dev/null
> +++ b/backends/mips_cfi.c
> @@ -0,0 +1,68 @@
> +/* MIPS ABI-specified defaults for DWARF CFI.
> +   Copyright (C) 2009 Red Hat, Inc.
> +   Copyright (C) 2023 CIP United Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of either
> +
> +     * the GNU Lesser General Public License as published by the Free
> +       Software Foundation; either version 3 of the License, or (at
> +       your option) any later version
> +
> +   or
> +
> +     * the GNU General Public License as published by the Free
> +       Software Foundation; either version 2 of the License, or (at
> +       your option) any later version
> +
> +   or both in parallel, as here.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received copies of the GNU General Public License and
> +   the GNU Lesser General Public License along with this program.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <dwarf.h>
> +
> +#define BACKEND mips_
> +#include "libebl_CPU.h"
> +
> +int
> +mips_abi_cfi (Ebl *ebl __attribute__ ((unused)), Dwarf_CIE *abi_info)
> +{
> +  static const uint8_t abi_cfi[] =
> +    {
> +      DW_CFA_def_cfa, ULEB128_7 (31), ULEB128_7 (0),
> +      /* Callee-saved regs.  */
> +      DW_CFA_same_value, ULEB128_7 (16), /* s0 */
> +      DW_CFA_same_value, ULEB128_7 (17), /* s1 */
> +      DW_CFA_same_value, ULEB128_7 (18), /* s2 */
> +      DW_CFA_same_value, ULEB128_7 (19), /* s3 */
> +      DW_CFA_same_value, ULEB128_7 (20), /* s4 */
> +      DW_CFA_same_value, ULEB128_7 (21), /* s5 */
> +      DW_CFA_same_value, ULEB128_7 (22), /* s6 */
> +      DW_CFA_same_value, ULEB128_7 (23), /* s7 */
> +      DW_CFA_same_value, ULEB128_7 (28), /* gp */
> +      DW_CFA_same_value, ULEB128_7 (29), /* sp */
> +      DW_CFA_same_value, ULEB128_7 (30), /* fp */
> +
> +      DW_CFA_val_offset, ULEB128_7 (29), ULEB128_7 (0),
> +    };
> +
> +  abi_info->initial_instructions = abi_cfi;
> +  abi_info->initial_instructions_end = &abi_cfi[sizeof abi_cfi];
> +  abi_info->data_alignment_factor = -4;
> +
> +  abi_info->return_address_register = 31; /* %ra */
> +
> +  return 0;
> +}

Looks good, but do you have a reference to the ABI docs would be nice
to add an URL as comment for people to double check.

> diff --git a/backends/mips_init.c b/backends/mips_init.c
> index 4c2f21b9..3caa9fee 100644
> --- a/backends/mips_init.c
> +++ b/backends/mips_init.c
> @@ -55,5 +55,9 @@ mips_init (Elf *elf __attribute__ ((unused)),
>    HOOK (eh, check_object_attribute);
>    HOOK (eh, check_special_symbol);
>    HOOK (eh, check_reloc_target_type);
> +  HOOK (eh, set_initial_registers_tid);
> +  HOOK (eh, abi_cfi);
> +  HOOK (eh, unwind);
> +  eh->frame_nregs = 32;
>    return eh;
>  }

OK

> diff --git a/backends/mips_initreg.c b/backends/mips_initreg.c
> new file mode 100644
> index 00000000..31b8de13
> --- /dev/null
> +++ b/backends/mips_initreg.c
> @@ -0,0 +1,70 @@
> +/* Fetch live process registers from TID.
> +   Copyright (C) 2023 CIP United Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of either
> +
> +     * the GNU Lesser General Public License as published by the Free
> +       Software Foundation; either version 3 of the License, or (at
> +       your option) any later version
> +
> +   or
> +
> +     * the GNU General Public License as published by the Free
> +       Software Foundation; either version 2 of the License, or (at
> +       your option) any later version
> +
> +   or both in parallel, as here.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received copies of the GNU General Public License and
> +   the GNU Lesser General Public License along with this program.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <stdlib.h>
> +#if (defined(mips) || defined(__mips) || defined(__mips__) || defined(MIPS) || defined(__MIPS__)) && defined(__linux__)
> +# include <sys/user.h>
> +# include <sys/ptrace.h>
> +#endif
> +
> +#define BACKEND mips_
> +#include "libebl_CPU.h"
> +#include <stdio.h>
> +
> +
> +bool
> +mips_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
> +			  ebl_tid_registers_t *setfunc __attribute__ ((unused)),
> +				  void *arg __attribute__ ((unused)))
> +{
> +#if (!defined(mips) && !defined(__mips) && !defined(__mips__) && !defined(MIPS) && !defined(__MIPS__)) || !defined(__linux__)
> +  return false;
> +#else /* __mips__ */
> +/* For PTRACE_GETREGS */
> +struct pt_regs {
> +    uint64_t regs[32];
> +    uint64_t lo;
> +    uint64_t hi;
> +    uint64_t pc;
> +    uint64_t badvaddr;
> +    uint64_t cause;
> +    uint64_t status;
> +};

Isn't this defined in some standard (or glibc/linux specific) header?

> +  struct pt_regs gregs;
> +  if (ptrace (PTRACE_GETREGS, tid, 0, &gregs) != 0)
> +    return false;
> +  if (! setfunc (-1, 1, (Dwarf_Word *) &gregs.pc, arg))
> +    return false;
> +  return setfunc (0, 32, (Dwarf_Word *) &gregs.regs[0], arg);
> +#endif /* __mips__ */
> +}

OK

> diff --git a/backends/mips_unwind.c b/backends/mips_unwind.c
> new file mode 100644
> index 00000000..d09db3a9
> --- /dev/null
> +++ b/backends/mips_unwind.c
> @@ -0,0 +1,84 @@
> +/* Get previous frame state for an existing frame state.
> +   Copyright (C) 2016 The Qt Company Ltd.
> +   Copyright (C) 2023 CIP United Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of either
> +
> +     * the GNU Lesser General Public License as published by the Free
> +       Software Foundation; either version 3 of the License, or (at
> +       your option) any later version
> +
> +   or
> +
> +     * the GNU General Public License as published by the Free
> +       Software Foundation; either version 2 of the License, or (at
> +       your option) any later version
> +
> +   or both in parallel, as here.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received copies of the GNU General Public License and
> +   the GNU Lesser General Public License along with this program.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#define BACKEND mips_
> +#define SP_REG 29
> +#define FP_REG 30
> +#define LR_REG 31
> +#define FP_OFFSET 0
> +#define LR_OFFSET 8
> +#define SP_OFFSET 16
> +
> +#include "libebl_CPU.h"
> +
> +/* There was no CFI. Maybe we happen to have a frame pointer and can unwind from that?  */
> +
> +bool
> +EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)),
> +                 ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc,
> +                 ebl_pid_memory_read_t *readfunc, void *arg,
> +                 bool *signal_framep __attribute__ ((unused)))
> +{
> +  Dwarf_Word fp, lr, sp;
> +
> +  if (!getfunc(LR_REG, 1, &lr, arg))
> +    return false;
> +
> +  if (lr == 0 || !setfunc(-1, 1, &lr, arg))
> +    return false;
> +
> +  if (!getfunc(FP_REG, 1, &fp, arg))
> +    fp = 0;
> +
> +  if (!getfunc(SP_REG, 1, &sp, arg))
> +    sp = 0;
> +
> +  Dwarf_Word newLr, newFp, newSp;
> +
> +  if (!readfunc(fp + LR_OFFSET, &newLr, arg))
> +    newLr = 0;
> +
> +  if (!readfunc(fp + FP_OFFSET, &newFp, arg))
> +    newFp = 0;
> +
> +  newSp = fp + SP_OFFSET;
> +
> +  // These are not fatal if they don't work. They will just prevent unwinding at the next frame.
> +  setfunc(LR_REG, 1, &newLr, arg);
> +  setfunc(FP_REG, 1, &newFp, arg);
> +  setfunc(SP_REG, 1, &newSp, arg);
> +
> +  // If the fp is invalid, we might still have a valid lr.
> +  // But if the fp is valid, then the stack should be moving in the right direction.
> +  return fp == 0 || newSp > sp;
> +}

Looks good.

Thanks,

Mark