libdwfl: Remove asserts from library code
Commit Message
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.
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(-)
void *notes;
if (ei_data == MY_ELFDATA
Comments
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
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.
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(-)
@@ -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;
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;
}
@@ -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)
@@ -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;
+ }
for (unsigned regno = firstreg; regno < firstreg + nregs; regno++)
if (! __libdwfl_frame_reg_set (state, regno, regs[regno - firstreg]))
{
@@ -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);
@@ -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;
}
@@ -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;
@@ -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;
result = (*func) (arg, regno, setname, prefix, name, bits, type);
}
}
@@ -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