Re: [PATCH v3 6/6] backends: Add register_info, return_value_location, core_note function on mips

[Mark Wielaard <mark@klomp.org>]

Hi,

On Tue, Mar 05, 2024 at 05:51:22PM +0800, Ying Huang wrote:
> From: Ying Huang <ying.huang@oss.cipunited.com>
> 
> Signed-off-by: Ying Huang <ying.huang@oss.cipunited.com>
> ---
>  backends/Makefile.am     |   3 +-
>  backends/mips_corenote.c |  85 +++++++++++++++++
>  backends/mips_init.c     |   3 +
>  backends/mips_regs.c     | 135 +++++++++++++++++++++++++++
>  backends/mips_retval.c   | 196 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 421 insertions(+), 1 deletion(-)
>  create mode 100644 backends/mips_corenote.c
>  create mode 100644 backends/mips_regs.c
>  create mode 100644 backends/mips_retval.c

Similar to the previous one. Added a ChangeLog entry to the commit
message:

        * backends/Makefile.am (mips_SRCS): Add mips_regs.c, 
        mips_retval.c and mips_corenote.c.
        * backends/mips_init.c (mips_init): HOOK register_info,
        return_value_location and core_note.
        * backends/mips_corenote.c: New file.
        * backends/mips_regs.c: Likewise.
        * backends/mips_retval.c: Likewise.

The implementations of core_note, register_info and
return_value_location do look plausible. Pushed.

Although there is one small issue/question:

> diff --git a/backends/mips_corenote.c b/backends/mips_corenote.c
> new file mode 100644
> index 00000000..aeadeb17
> --- /dev/null
> +++ b/backends/mips_corenote.c
> @@ -0,0 +1,85 @@
> +/* MIPS specific core note handling.
> +   Copyright (C) 2024 CIP United Inc.
> +   This file is part of elfutils.
> [...]
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <elf.h>
> +#include <inttypes.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <sys/time.h>
> +
> +#define BACKEND	mips_
> +#include "libebl_CPU.h"
> +
> +#define BITS 64
> +#ifndef BITS
> +# define BITS 		32
> +#else
> +# define BITS 		64
> +#endif

So this only handles 64 BITS. I left it this way.  But it is a
question about the whole series. MIPS has multiple abis, some 32, some
64 bits, some big and some little endian.  If I understand correctly
this backend only handles the 64 bit little endian one?

Again it would be nice if you could come up with non-native variants
of the tests so the implementation can also be tested on non-MIPS
arches. Specifically for tests/run-allregs.sh, tests/run-funcretval.sh
and/or tests/run-readelf-mixed-corenote.sh.

Thanks,

Mark