Re: [PATCH 2/5] readelf: Adapt src/readelf -h/-S/-r/-w/-l/-d/-a on mips

2023-05-11 Thread Mark Wielaard
Hi,

On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
> From: Ying Huang 
> 
> -h: support show Flags name
> -S: support show mips related section type
> -r: support show type and relocation info value of Relocation section
> -w: can work and can show correct "strp" contents
> -l: support show mips related program header entry type
> -d: can show mips related Dynamic type name
> -a: support show complete Object attribute section ".gnu.attributes"
> ---
>  backends/Makefile.am   |   2 +-
>  backends/mips_attrs.c  | 107 +++
>  backends/mips_init.c   |   7 +
>  backends/mips_symbol.c | 572 +
>  libebl/eblreloctypecheck.c |   8 +-
>  libebl/eblreloctypename.c  |   8 +-
>  libelf/elf.h   |  93 +-
>  src/readelf.c  | 190 +---
>  8 files changed, 932 insertions(+), 55 deletions(-)
>  create mode 100644 backends/mips_attrs.c

This is generally OK, but there are a few issues and questions.

> diff --git a/backends/Makefile.am b/backends/Makefile.am
> index bda1b604..428a1a03 100644
> --- a/backends/Makefile.am
> +++ b/backends/Makefile.am
> @@ -100,7 +100,7 @@ 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_SRCS = mips_init.c mips_symbol.c mips_attrs.c
>  
>  libebl_backends_a_SOURCES = $(i386_SRCS) $(sh_SRCS) $(x86_64_SRCS) \
>   $(ia64_SRCS) $(alpha_SRCS) $(arm_SRCS) \

OK.

> diff --git a/backends/mips_attrs.c b/backends/mips_attrs.c
> new file mode 100644
> index ..1419814e
> --- /dev/null
> +++ b/backends/mips_attrs.c
> @@ -0,0 +1,107 @@
> +/* Object attribute tags for MIPS.
> +   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 .  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +#include 
> +
> +#define BACKEND arm_

You mean mips_

> +#include "libebl_CPU.h"
> +
> +#define KNOWN_VALUES(...) do \
> +  {  \
> +static const char *table[] = { __VA_ARGS__ };\
> +if (value < sizeof table / sizeof table[0])  \
> +  *value_name = table[value];\
> +  } while (0)
> +
> +/* copy binutils-2.34/binutils/readelf.c display_mips_gnu_attribute */

In this and the few other cases this is OK because it is just taking
the constants/names. For snippets like that where they are adapted to
the elfutils code that is fine. But note that for larger parts the
licenses are slightly different so in general you should write your own
code instead of copy/pasting from binutils.

> +bool
> +mips_check_object_attribute (Ebl *ebl __attribute__ ((unused)),
> + const char *vendor, int tag, uint64_t value,
> + const char **tag_name, const char **value_name)
> +{
> +  if (!strcmp (vendor, "gnu"))
> +switch (tag)
> +  {
> +  case Tag_GNU_MIPS_ABI_FP:
> + *tag_name = "Tag_GNU_MIPS_ABI_FP";
> + switch (value)
> + {
> +   case Val_GNU_MIPS_ABI_FP_ANY:
> + *value_name = "Hard or soft float";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_DOUBLE:
> + *value_name = "Hard float (double precision)";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_SINGLE:
> + *value_name = "Hard float (single precision)";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_SOFT:
> + *value_name = "Soft float";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_OLD_64:
> + *value_name = "Hard float (MIPS32r2 64-bit FPU 12 callee-saved)";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_XX:
> + *value_name = "Hard float (32-bit CPU, Any FPU)";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_64:
> + *value_name = "Hard float (32-bit CPU, 64-bit FPU)";
> + return true;
> +   case Val_GNU_MIPS_ABI_FP_64A:
> +   

Re: [PATCH 3/5] elflint: Fix invalid type of relocation info and other issues on mips

2023-05-11 Thread Mark Wielaard
Hi,

On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
> From: Ying Huang 
> 
> add some check related functions
> ---
>  backends/mips_init.c  |  3 +++
>  backends/mips_symbol.c| 33 +
>  libebl/eblrelocvaliduse.c |  8 ++--
>  src/elflint.c | 23 ---
>  4 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/backends/mips_init.c b/backends/mips_init.c
> index 5bba822b..4c2f21b9 100644
> --- a/backends/mips_init.c
> +++ b/backends/mips_init.c
> @@ -51,6 +51,9 @@ mips_init (Elf *elf __attribute__ ((unused)),
>HOOK (eh, segment_type_name);
>HOOK (eh, dynamic_tag_check);
>HOOK (eh, dynamic_tag_name);
> +  HOOK (eh, machine_section_flag_check);
>HOOK (eh, check_object_attribute);
> +  HOOK (eh, check_special_symbol);
> +  HOOK (eh, check_reloc_target_type);
>return eh;
>  }

OK. But see below for hooking reloc_valid_use

> diff --git a/backends/mips_symbol.c b/backends/mips_symbol.c
> index e760d58d..8787fcee 100644
> --- a/backends/mips_symbol.c
> +++ b/backends/mips_symbol.c
> @@ -158,6 +158,39 @@ mips_section_type_name (int type,
>return NULL;
>  }
>  
> +bool
> +mips_check_reloc_target_type (Ebl *ebl __attribute__ ((unused)), Elf64_Word 
> sh_type)
> +{
> +  return (sh_type == SHT_MIPS_DWARF);
> +}

OK

> +/* Check whether given symbol's st_value and st_size are OK despite failing
> +   normal checks.  */
> +bool
> +mips_check_special_symbol (Elf *elf,
> + const GElf_Sym *sym __attribute__ ((unused)),
> + const char *name __attribute__ ((unused)),
> + const GElf_Shdr *destshdr)
> +{
> +  size_t shstrndx;
> +  if (elf_getshdrstrndx (elf, &shstrndx) != 0)
> +return false;
> +  const char *sname = elf_strptr (elf, shstrndx, destshdr->sh_name);
> +  if (sname == NULL)
> +return false;
> +  return (strcmp (sname, ".got") == 0 || strcmp (sname, ".bss") == 0);
> +}

Could you add a comment why .got and .bss are special in this case?

> +/* Check whether SHF_MASKPROC flags are valid.  */
> +bool
> +mips_machine_section_flag_check (GElf_Xword sh_flags)
> +{
> +  return ((sh_flags &~ (SHF_MIPS_GPREL |
> + SHF_MIPS_MERGE |
> + SHF_MIPS_ADDR |
> + SHF_MIPS_STRINGS)) == 0);
> +}

OK. But see below for checking other SHF_MIPS flags.

>  /* Check whether machine flags are valid.  */
>  bool
>  mips_machine_flag_check (GElf_Word flags)
> diff --git a/libebl/eblrelocvaliduse.c b/libebl/eblrelocvaliduse.c
> index f0bed345..44b8d300 100644
> --- a/libebl/eblrelocvaliduse.c
> +++ b/libebl/eblrelocvaliduse.c
> @@ -32,10 +32,14 @@
>  #endif
>  
>  #include 
> -
> +#include 
>  
>  bool
>  ebl_reloc_valid_use (Ebl *ebl, int reloc)
>  {
> -  return ebl != NULL ? ebl->reloc_valid_use (ebl->elf, reloc) : false;
> +  int relocNew = reloc;
> +  GElf_Ehdr ehdr;
> +  if(ebl->elf->class == ELFCLASS64 && gelf_getehdr(ebl->elf, &ehdr) != NULL 
> && ehdr.e_machine == EM_MIPS)
> +relocNew = ELF64_MIPS_R_TYPE(reloc);
> +  return ebl != NULL ? ebl->reloc_valid_use (ebl->elf, relocNew) : false;
>  }

This should be a mips reloc_valid_use hook.

> diff --git a/src/elflint.c b/src/elflint.c
> index dd42dcb4..04f1ee92 100644
> --- a/src/elflint.c
> +++ b/src/elflint.c
> @@ -935,7 +935,10 @@ section [%2d] '%s': symbol %zu (%s): non-local symbol 
> outside range described in
>   }
>  
>if (GELF_ST_TYPE (sym->st_info) == STT_SECTION
> -   && GELF_ST_BIND (sym->st_info) != STB_LOCAL)
> +   && GELF_ST_BIND (sym->st_info) != STB_LOCAL
> +   && ehdr->e_machine != EM_MIPS
> +   && strcmp (name, "_DYNAMIC_LINK") != 0
> +   && strcmp (name, "_DYNAMIC_LINKING") != 0)

Could you add a comment here about both symbols not being local on
MIPS?

>   ERROR (_("\
>  section [%2d] '%s': symbol %zu (%s): non-local section symbol\n"),
>  idx, section_name (ebl, idx), cnt, name);
> @@ -3789,6 +3792,12 @@ cannot get section header for section [%2zu] '%s': 
> %s\n"),
>   && ebl_bss_plt_p (ebl))
> good_type = SHT_NOBITS;
>  
> + if (ehdr->e_machine == EM_MIPS
> + && (strstr(special_sections[s].name, ".debug") != NULL))
> +   {
> + good_type = SHT_MIPS_DWARF;
> +   }

OK. You don't need explicit brackets here

>   /* In a debuginfo file, any normal section can be SHT_NOBITS.
>  This is only invalid for DWARF sections and .shstrtab.  */
>   if (shdr->sh_type != good_type
> @@ -3953,8 +3962,16 @@ section [%2zu] '%s': size not multiple of entry 
> size\n"),
> sh_flags &= ~(GElf_Xword) SHF_MASKPROC;
>   }
> if (sh_flags & SHF_MASKOS)
> - if (gnuld)
> -   sh_flags &= ~(GElf_Xword) SHF_GNU_RETAIN;
> + {
> +   if (gnuld)
> + sh_flags &= ~(GElf_Xword) SHF_GNU_RETAIN;
> +  

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

2023-05-11 Thread Mark Wielaard
Hi,

On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
> From: Ying Huang 
> 
> 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 ..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 .  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +
> +#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 ..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;

Re: [PATCH 5/5] backends: Fix run-native-test.sh and run-funcretval++11.sh run fail on mips

2023-05-11 Thread Mark Wielaard
Hi,

On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
> From: Ying Huang 
> 
> add register_info, return_value_location function on mips
> ---
>  backends/Makefile.am   |   2 +-
>  backends/mips_init.c   |   2 +
>  backends/mips_regs.c   | 109 +
>  backends/mips_retval.c | 261 +
>  4 files changed, 373 insertions(+), 1 deletion(-)
>  create mode 100644 backends/mips_regs.c
>  create mode 100644 backends/mips_retval.c
> 
> diff --git a/backends/Makefile.am b/backends/Makefile.am
> index ddc31c9d..5453f787 100644
> --- a/backends/Makefile.am
> +++ b/backends/Makefile.am
> @@ -101,7 +101,7 @@ 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_initreg.c \
> - mips_cfi.c mips_unwind.c
> + mips_cfi.c mips_unwind.c mips_regs.c mips_retval.c

OK.
 
>  libebl_backends_a_SOURCES = $(i386_SRCS) $(sh_SRCS) $(x86_64_SRCS) \
>   $(ia64_SRCS) $(alpha_SRCS) $(arm_SRCS) \
> diff --git a/backends/mips_init.c b/backends/mips_init.c
> index 3caa9fee..7ca93314 100644
> --- a/backends/mips_init.c
> +++ b/backends/mips_init.c
> @@ -58,6 +58,8 @@ mips_init (Elf *elf __attribute__ ((unused)),
>HOOK (eh, set_initial_registers_tid);
>HOOK (eh, abi_cfi);
>HOOK (eh, unwind);
> +  HOOK (eh, register_info);
> +  HOOK (eh, return_value_location);
>eh->frame_nregs = 32;
>return eh;
>  }

OK

> diff --git a/backends/mips_regs.c b/backends/mips_regs.c
> new file mode 100644
> index ..733caeee
> --- /dev/null
> +++ b/backends/mips_regs.c
> @@ -0,0 +1,109 @@
> +/* Register names and numbers for mips DWARF.
> +   Copyright (C) 2006 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 .  */
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +
> +#define BACKEND mips_
> +#include "libebl_CPU.h"
> +
> +ssize_t
> +mips_register_info (Ebl *ebl __attribute__ ((unused)),
> +   int regno, char *name, size_t namelen,
> +   const char **prefix, const char **setname,
> +   int *bits, int *type)
> +{
> +  if (name == NULL)
> +return 66;
> +
> +  if (regno < 0 || regno > 65 || namelen < 4)
> +return -1;
> +
> +  *prefix = "$";
> +  
> +  if (regno < 32)
> +{
> +  *setname = "integer";
> +  *type = DW_ATE_signed;
> +  *bits = 32;
> +  if (regno < 32 + 10)
> +  {
> +name[0] = regno + '0';
> + namelen = 1;
> +  }
> +  else
> +  {
> +name[0] = (regno / 10) + '0';
> + name[1] = (regno % 10) + '0';
> + namelen = 2;
> +  }
> +}
> +  else if (regno < 64)
> +{
> +  *setname = "FPU";
> +  *type = DW_ATE_float;
> +  *bits = 32;
> +  name[0] = 'f';
> +  if (regno < 32 + 10)
> +  {
> +name[1] = (regno - 32) + '0';
> + namelen = 2;
> +  }
> +  else
> +  {
> +name[1] = (regno - 32) / 10 + '0';
> + name[2] = (regno - 32) % 10 + '0';
> + namelen = 3;
> +  }
> +}
> +  else if (regno == 64)
> +{
> +  *type = DW_ATE_signed;
> +  *bits = 32;
> +  name[0] = 'h';
> +  name[1] = 'i';
> +  namelen = 2;
> +}
> +  else
> +{
> +  *type = DW_ATE_signed;
> +  *bits = 32;
> +  name[0] = 'l';
> +  name[1] = 'o';
> +  namelen = 2;
> +}
> +
> +  name[namelen++] = '\0';
> +  return namelen;
> +}

OK, but indentation seems slightly off. space vs tabs?

> diff --git a/backends/mips_retval.c b/backends/mips_retval.c
> new file mode 100644
> index ..fd9aaefa
> --- /dev/null
> +++ b/backends/mips_retval.c
> @@ -0,0 +1,261 @@
> +/* Function return value location for Linux/mips ABI.
> +   Copyright (C) 2005 Red Hat, Inc.
> +   Copyright (C) 2023 CIP United Inc.
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribu

Re: eu-stacktrace: roadmap and discussion thread

2023-05-11 Thread Serhei Makarov
On Mon, May 8, 2023, at 5:50 PM, Christian Hergert wrote:
> First off, this all sounds great!
> ...
>
>  From a consumption standpoint, it would be nice if Sysprof could get a 
> perf stream where the PERF_SAMPLE_STACK are transparently converted to 
> PERF_SAMPLE_CALLCHAIN. I don't think eu-stacktrace necessarily needs to 
> speak Sysprof's capture API.
>...
>
> Sysprof already contains `sysprofd` which can open the Perf event stream 
> for us via d-bus/CAP_SYS_ADMIN/polkit integration. After Sysprof gets 
> the perf FD back from sysprofd we could spawn eu-stacktrace giving it 
> the FD to consume and another FD to write the translated/passthrough events.
The idea with sysprofd in the below quote sounds intriguing; I think we
can experiment with it once we have some perf parser code of our own.

For now, I'm satisfied with the fact that the patch to enable Sysprof to pipe
data to eu-stacktrace is very small, and the parsing code I'm working on
for separating out Sysprof capture frames is also quite small. Both are
easy to adapt to any refactoring or even data format changes you
might happen to do.

> Sysprof can do offline symbolizing of frames which is somewhat important 
> when trying to analyze profiles from an embedded device, a machine that 
> is disk/network constrained, or end-user-system via bug reports. We can 
> fairly trivially teach Sysprof to do symbolizing via debuginfod.
Yep -- and that could be a separate patchset, since the current approach
I'm using changes nothing about the Sysprof symbolizing phase.

> In the case you're describing, is it right that you may not be able to 
> unwind stack frames without debuginfod because there was no way to 
> locate the `.eh_frame` section for the binary?
Yep -- for containers, I considered debuginfod as a possible fallback
if the .eh_frame data isn't accessible on the local filesystem.
Of course that doesn't work for non-packaged container programs,
unless the developers of those programs set up debuginfod.
In general the workflows for debuginfo on containers are
rather under-developed.

Honestly this isn't a scenario to implement as a first priority,
but I do want to keep track of everything that's required
to have sysprof+eu_stacktrace+eh_frame+PERF_SAMPLE_STACK
maintain feature parity with sysprof+-fno-omit-fp+PERF_SAMPLE_CALLCHAIN.
Thus I am making sure to list such consideration.

> The code to do the mount namespace conversion is quite terrible in 
> Sysprof and even now I'm in the midst of cleaning it up. We have to both 
> create a "view" of the namespace as it was to the PID as well as a way 
> to convert that view into something the mount namespace analyzing the 
> capture file might be able to open. Any of these may or may not be in a 
> rootless container (toolbox/podman/flatpak/etc).
>
> Whether or not this is something we can eventually contain inside of 
> bubblewrap is another can of worms.
One possible solution

For this type of situation, we could perhaps give eu-stacktrace an
optional dependency on sysprof-devel.

Right now, I rely on sysprof-capture-types.h but that's just the header
and therefore not a runtime dependency.

When the sysprof libraries are available, we could also use sysprof's
mount namespace conversion code (in whatever eventual form it takes)
to get .eh_frame data.


Re: eu-stacktrace: roadmap and discussion thread

2023-05-11 Thread Serhei Makarov
On Tue, May 9, 2023, at 2:02 PM, Milian Wolff wrote:
> Hey Serhey,
>
> sounds like a fun project. If you want to see some prior art in that area, do 
> have a look at perfparser [1], esp. [2], and [3]. We solved quite a few of 
> the 
> problems you might encounter in this area. Esp. for good performance, you'll 
> need quite a bit of caching on the stacktrace side, such as [4] and [5].
Excellent, I do look forward to finding out what kind of caching I'll need to 
implement,
and if your solutions work for our use case, then I think making them more
widely-available via elfutils would be a good outcome all round.

All the best,
 Serhei