Hi Di, Thanks for the patch.
On Sun, Dec 24, 2023 at 2:35 AM Di Chen <dic...@redhat.com> wrote: > > From 33d436aefb6b63a159943bd24eb432b8cfb8b369 Mon Sep 17 00:00:00 2001 > From: Di Chen <dic...@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 <dic...@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
