[PATCH] libdwfl: Remove asserts from library code
Aaron Merey
amerey@redhat.com
Sun Jan 28 20:51:09 GMT 2024
Hi Di,
Thanks for the patch.
On Sun, Dec 24, 2023 at 2:35 AM Di Chen <dichen@redhat.com> wrote:
>
> From 33d436aefb6b63a159943bd24eb432b8cfb8b369 Mon Sep 17 00:00:00 2001
> From: Di Chen <dichen@redhat.com>
> Date: Sun, 24 Dec 2023 11:44:48 +0800
> Subject: [PATCH] libdwfl: Remove asserts from library code
>
> It would be better for elfutils library functions to return an error
> code instead of aborting because of a failed assert.
>
> This commit works on removing the asserts under libdwfl. There are two
> ways handling the removing:
>
> 1) Replace the asserts with if statements.
>
> 2) Replace the asserts with eu_static_assert.
>
> Asserts are heavily used across all elfutils libraries, and it's
> impossibe to implement the removing in one commit. So let's gradually
> remove the asserts in the later coming commits.
Agreed.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31027
>
> Signed-off-by: Di Chen <dichen@redhat.com>
> ---
> libdwfl/dwfl_frame.c | 14 +++++++++-----
> libdwfl/dwfl_frame_pc.c | 3 ++-
> libdwfl/dwfl_frame_regs.c | 7 +++++--
> libdwfl/dwfl_module_build_id.c | 3 ++-
> libdwfl/dwfl_module_getdwarf.c | 3 ++-
> libdwfl/dwfl_module_getsrc_file.c | 3 ++-
> libdwfl/dwfl_module_register_names.c | 3 ++-
> libdwfl/dwfl_segment_report_module.c | 2 +-
> 8 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
> index 5ee71dd4..18150e2a 100644
> --- a/libdwfl/dwfl_frame.c
> +++ b/libdwfl/dwfl_frame.c
> @@ -85,12 +85,14 @@ free_states (Dwfl_Frame *state)
> static Dwfl_Frame *
> state_alloc (Dwfl_Thread *thread)
> {
> - assert (thread->unwound == NULL);
> + if (thread->unwound != NULL)
> + return NULL;
> Ebl *ebl = thread->process->ebl;
> size_t nregs = ebl_frame_nregs (ebl);
> if (nregs == 0)
> return NULL;
> - assert (nregs < sizeof (((Dwfl_Frame *) NULL)->regs_set) * 8);
> + if (nregs >= sizeof (((Dwfl_Frame *) NULL)->regs_set) * 8)
> + return NULL;
All of these assert removals look good, but it may be helpful if we also
set an error code with __libdwfl_seterrno, at least in the dwfl_frame*.c
functions being modified in this patch.
I think the INVALID_REGISTER error can be used when nregs is too big.
However for 'thread->unwound != NULL' it's not clear to me exactly
which DWFL_ERROR we should use. The NO_UNWIND error seems applicable
but its description reads "Unwinding not supported for this architecture".
Arch support isn't necessarily the problem here.
Maybe we should add another DWFL_ERROR for these cases, something like
"BAD_FRAME"? Then we can use this error code when a frame is missing
or its pc_state is corrupt.
> Dwfl_Frame *state = malloc (sizeof (*state) + sizeof (*state->regs) * nregs);
> if (state == NULL)
> return NULL;
> @@ -283,8 +285,9 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
> }
> int err = callback (&thread, arg);
> if (err != DWARF_CB_OK)
> - return err;
> - assert (thread.unwound == NULL);
> + return err;
> + if (thread.unwound != NULL)
> + return -1;
> }
> /* NOTREACHED */
> }
> @@ -450,7 +453,8 @@ dwfl_thread_getframes (Dwfl_Thread *thread,
> __libdwfl_seterrno (err);
> return -1;
> }
> - assert (state->pc_state == DWFL_FRAME_STATE_PC_UNDEFINED);
> + if (state->pc_state != DWFL_FRAME_STATE_PC_UNDEFINED)
> + return -1;
> free_states (state);
> return 0;
> }
> diff --git a/libdwfl/dwfl_frame_pc.c b/libdwfl/dwfl_frame_pc.c
> index 296c815b..b9991e9a 100644
> --- a/libdwfl/dwfl_frame_pc.c
> +++ b/libdwfl/dwfl_frame_pc.c
> @@ -35,7 +35,8 @@
> bool
> dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
> {
> - assert (state->pc_state == DWFL_FRAME_STATE_PC_SET);
> + if (state->pc_state != DWFL_FRAME_STATE_PC_SET)
> + return false;
> *pc = state->pc;
> ebl_normalize_pc (state->thread->process->ebl, pc);
> if (isactivation)
> diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
> index a4bd3884..064f5e8a 100644
> --- a/libdwfl/dwfl_frame_regs.c
> +++ b/libdwfl/dwfl_frame_regs.c
> @@ -37,8 +37,11 @@ dwfl_thread_state_registers (Dwfl_Thread *thread, int firstreg,
> unsigned nregs, const Dwarf_Word *regs)
> {
> Dwfl_Frame *state = thread->unwound;
> - assert (state && state->unwound == NULL);
> - assert (state->initial_frame);
> + if (state == NULL || state->unwound != NULL || !state->initial_frame)
> + {
> + __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
> + return false;
Need two more spaces of indentation here.
> + }
> for (unsigned regno = firstreg; regno < firstreg + nregs; regno++)
> if (! __libdwfl_frame_reg_set (state, regno, regs[regno - firstreg]))
> {
> diff --git a/libdwfl/dwfl_module_build_id.c b/libdwfl/dwfl_module_build_id.c
> index 0c198f23..4dcc046e 100644
> --- a/libdwfl/dwfl_module_build_id.c
> +++ b/libdwfl/dwfl_module_build_id.c
> @@ -65,7 +65,8 @@ __libdwfl_find_build_id (Dwfl_Module *mod, bool set, Elf *elf)
> int build_id_len;
>
> /* For mod == NULL use dwelf_elf_gnu_build_id directly. */
> - assert (mod != NULL);
> + if (mod == NULL)
> + return -1;
>
> int result = __libdwfl_find_elf_build_id (mod, elf, &build_id_bits,
> &build_id_elfaddr, &build_id_len);
> diff --git a/libdwfl/dwfl_module_getdwarf.c b/libdwfl/dwfl_module_getdwarf.c
> index 6f98c02b..596de51e 100644
> --- a/libdwfl/dwfl_module_getdwarf.c
> +++ b/libdwfl/dwfl_module_getdwarf.c
> @@ -161,7 +161,8 @@ open_elf (Dwfl_Module *mod, struct dwfl_file *file)
> mod->e_type = ET_DYN;
> }
> else
> - assert (mod->main.elf != NULL);
> + if (mod->main.elf == NULL)
> + return DWFL_E (LIBELF, elf_errno ());
>
> return DWFL_E_NOERROR;
> }
> diff --git a/libdwfl/dwfl_module_getsrc_file.c b/libdwfl/dwfl_module_getsrc_file.c
> index fc144225..4456dc58 100644
> --- a/libdwfl/dwfl_module_getsrc_file.c
> +++ b/libdwfl/dwfl_module_getsrc_file.c
> @@ -165,7 +165,8 @@ dwfl_module_getsrc_file (Dwfl_Module *mod,
>
> if (cur_match > 0)
> {
> - assert (*nsrcs == 0 || *srcsp == match);
> + if (*nsrcs != 0 && *srcsp != match)
> + return -1;
>
> *nsrcs = cur_match;
> *srcsp = match;
> diff --git a/libdwfl/dwfl_module_register_names.c b/libdwfl/dwfl_module_register_names.c
> index 9ea09371..6fb4195b 100644
> --- a/libdwfl/dwfl_module_register_names.c
> +++ b/libdwfl/dwfl_module_register_names.c
> @@ -73,7 +73,8 @@ dwfl_module_register_names (Dwfl_Module *mod,
> }
> if (likely (len > 0))
> {
> - assert (len > 1); /* Backend should never yield "". */
> + if (len <= 1) /* Backend should never yield "". */
> + return -1;
The return statement needs to be indented more.
> result = (*func) (arg, regno, setname, prefix, name, bits, type);
> }
> }
> diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
> index dc34e0ae..39e27e22 100644
> --- a/libdwfl/dwfl_segment_report_module.c
> +++ b/libdwfl/dwfl_segment_report_module.c
> @@ -530,7 +530,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
> if (filesz > SIZE_MAX / sizeof (Elf32_Nhdr))
> continue;
>
> - assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
> + eu_static_assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
>
> void *notes;
> if (ei_data == MY_ELFDATA
> --
> 2.41.0
>
Aaron
More information about the Elfutils-devel
mailing list