| Message ID | f43fc5581003130656j408f64bfp7fe4bf486c625624@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Mar 13, 2010 at 04:56:03PM +0200, Blue Swirl wrote: > When building with -DNDEBUG, assert(0) will not stop execution > so it must not be used for abnormal termination. > > Use cpu_abort() when in CPU context, abort() otherwise. > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> The CRIS parts look good, thanks. Cheers > --- > block/vvfat.c | 20 ++++++++++---------- > hw/sh7750.c | 30 +++++++++++++++--------------- > hw/sh_intc.c | 6 +++--- > hw/sh_serial.c | 4 ++-- > hw/sm501.c | 12 ++++++------ > hw/tc58128.c | 8 ++++---- > linux-user/signal.c | 1 - > qdict.c | 3 +-- > target-cris/helper.c | 2 +- > target-cris/translate_v10.c | 16 +++++++++------- > target-sh4/README.sh4 | 2 +- > target-sh4/helper.c | 6 +++--- > target-sh4/op_helper.c | 2 +- > 13 files changed, 56 insertions(+), 56 deletions(-) > > diff --git a/block/vvfat.c b/block/vvfat.c > index aaa8593..36f6ab4 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1638,7 +1638,7 @@ static uint32_t > get_cluster_count_for_direntry(BDRVVVFATState* s, > /* new file */ > schedule_new_file(s, qemu_strdup(path), cluster_num); > else { > - assert(0); > + abort(); > return 0; > } > } > @@ -1659,7 +1659,7 @@ static uint32_t > get_cluster_count_for_direntry(BDRVVVFATState* s, > if (offset != mapping->info.file.offset + s->cluster_size > * (cluster_num - mapping->begin)) { > /* offset of this cluster in file chain has changed */ > - assert(0); > + abort(); > copy_it = 1; > } else if (offset == 0) { > const char* basename = get_basename(mapping->path); > @@ -1671,7 +1671,7 @@ static uint32_t > get_cluster_count_for_direntry(BDRVVVFATState* s, > > if (mapping->first_mapping_index != first_mapping_index > && mapping->info.file.offset > 0) { > - assert(0); > + abort(); > copy_it = 1; > } > > @@ -1837,7 +1837,7 @@ DLOG(fprintf(stderr, "check direntry %d: \n", > i); print_direntry(direntries + i) > goto fail; > } > } else > - assert(0); /* cluster_count = 0; */ > + abort(); /* cluster_count = 0; */ > > ret += cluster_count; > } > @@ -2458,7 +2458,7 @@ static int handle_commits(BDRVVVFATState* s) > commit_t* commit = array_get(&(s->commits), i); > switch(commit->action) { > case ACTION_RENAME: case ACTION_MKDIR: > - assert(0); > + abort(); > fail = -2; > break; > case ACTION_WRITEOUT: { > @@ -2519,7 +2519,7 @@ static int handle_commits(BDRVVVFATState* s) > break; > } > default: > - assert(0); > + abort(); > } > } > if (i > 0 && array_remove_slice(&(s->commits), 0, i)) > @@ -2607,7 +2607,7 @@ static int do_commit(BDRVVVFATState* s) > ret = handle_renames_and_mkdirs(s); > if (ret) { > fprintf(stderr, "Error handling renames (%d)\n", ret); > - assert(0); > + abort(); > return ret; > } > > @@ -2618,21 +2618,21 @@ static int do_commit(BDRVVVFATState* s) > ret = commit_direntries(s, 0, -1); > if (ret) { > fprintf(stderr, "Fatal: error while committing (%d)\n", ret); > - assert(0); > + abort(); > return ret; > } > > ret = handle_commits(s); > if (ret) { > fprintf(stderr, "Error handling commits (%d)\n", ret); > - assert(0); > + abort(); > return ret; > } > > ret = handle_deletes(s); > if (ret) { > fprintf(stderr, "Error deleting\n"); > - assert(0); > + abort(); > return ret; > } > > diff --git a/hw/sh7750.c b/hw/sh7750.c > index 9c39f4b..0291d5f 100644 > --- a/hw/sh7750.c > +++ b/hw/sh7750.c > @@ -206,7 +206,7 @@ static uint32_t sh7750_mem_readb(void *opaque, > target_phys_addr_t addr) > switch (addr) { > default: > error_access("byte read", addr); > - assert(0); > + abort(); > } > } > > @@ -240,7 +240,7 @@ static uint32_t sh7750_mem_readw(void *opaque, > target_phys_addr_t addr) > return 0; > default: > error_access("word read", addr); > - assert(0); > + abort(); > } > } > > @@ -287,7 +287,7 @@ static uint32_t sh7750_mem_readl(void *opaque, > target_phys_addr_t addr) > return s->cpu->prr; > default: > error_access("long read", addr); > - assert(0); > + abort(); > } > } > > @@ -303,7 +303,7 @@ static void sh7750_mem_writeb(void *opaque, > target_phys_addr_t addr, > } > > error_access("byte write", addr); > - assert(0); > + abort(); > } > > static void sh7750_mem_writew(void *opaque, target_phys_addr_t addr, > @@ -349,12 +349,12 @@ static void sh7750_mem_writew(void *opaque, > target_phys_addr_t addr, > s->gpioic = mem_value; > if (mem_value != 0) { > fprintf(stderr, "I/O interrupts not implemented\n"); > - assert(0); > + abort(); > } > return; > default: > error_access("word write", addr); > - assert(0); > + abort(); > } > } > > @@ -433,7 +433,7 @@ static void sh7750_mem_writel(void *opaque, > target_phys_addr_t addr, > return; > default: > error_access("long write", addr); > - assert(0); > + abort(); > } > } > > @@ -618,7 +618,7 @@ static struct intc_group groups_irl[] = { > > static uint32_t invalid_read(void *opaque, target_phys_addr_t addr) > { > - assert(0); > + abort(); > > return 0; > } > @@ -635,7 +635,7 @@ static uint32_t sh7750_mmct_readl(void *opaque, > target_phys_addr_t addr) > case MM_ITLB_ADDR: > case MM_ITLB_DATA: > /* XXXXX */ > - assert(0); > + abort(); > break; > case MM_OCACHE_ADDR: > case MM_OCACHE_DATA: > @@ -644,10 +644,10 @@ static uint32_t sh7750_mmct_readl(void *opaque, > target_phys_addr_t addr) > case MM_UTLB_ADDR: > case MM_UTLB_DATA: > /* XXXXX */ > - assert(0); > + abort(); > break; > default: > - assert(0); > + abort(); > } > > return ret; > @@ -656,7 +656,7 @@ static uint32_t sh7750_mmct_readl(void *opaque, > target_phys_addr_t addr) > static void invalid_write(void *opaque, target_phys_addr_t addr, > uint32_t mem_value) > { > - assert(0); > + abort(); > } > > static void sh7750_mmct_writel(void *opaque, target_phys_addr_t addr, > @@ -672,7 +672,7 @@ static void sh7750_mmct_writel(void *opaque, > target_phys_addr_t addr, > case MM_ITLB_ADDR: > case MM_ITLB_DATA: > /* XXXXX */ > - assert(0); > + abort(); > break; > case MM_OCACHE_ADDR: > case MM_OCACHE_DATA: > @@ -683,10 +683,10 @@ static void sh7750_mmct_writel(void *opaque, > target_phys_addr_t addr, > break; > case MM_UTLB_DATA: > /* XXXXX */ > - assert(0); > + abort(); > break; > default: > - assert(0); > + abort(); > break; > } > } > diff --git a/hw/sh_intc.c b/hw/sh_intc.c > index b6f45f0..da36d32 100644 > --- a/hw/sh_intc.c > +++ b/hw/sh_intc.c > @@ -105,7 +105,7 @@ int sh_intc_get_pending_vector(struct intc_desc > *desc, int imask) > } > } > > - assert(0); > + abort(); > } > > #define INTC_MODE_NONE 0 > @@ -181,7 +181,7 @@ static void sh_intc_locate(struct intc_desc *desc, > } > } > > - assert(0); > + abort(); > } > > static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id, > @@ -260,7 +260,7 @@ static void sh_intc_write(void *opaque, > target_phys_addr_t offset, > case INTC_MODE_ENABLE_REG | INTC_MODE_IS_PRIO: break; > case INTC_MODE_DUAL_SET: value |= *valuep; break; > case INTC_MODE_DUAL_CLR: value = *valuep & ~value; break; > - default: assert(0); > + default: abort(); > } > > for (k = 0; k <= first; k++) { > diff --git a/hw/sh_serial.c b/hw/sh_serial.c > index 2447b91..93dc144 100644 > --- a/hw/sh_serial.c > +++ b/hw/sh_serial.c > @@ -182,7 +182,7 @@ static void sh_serial_ioport_write(void *opaque, > uint32_t offs, uint32_t val) > } > > fprintf(stderr, "sh_serial: unsupported write to 0x%02x\n", offs); > - assert(0); > + abort(); > } > > static uint32_t sh_serial_ioport_read(void *opaque, uint32_t offs) > @@ -282,7 +282,7 @@ static uint32_t sh_serial_ioport_read(void > *opaque, uint32_t offs) > > if (ret & ~((1 << 16) - 1)) { > fprintf(stderr, "sh_serial: unsupported read from 0x%02x\n", offs); > - assert(0); > + abort(); > } > > return ret; > diff --git a/hw/sm501.c b/hw/sm501.c > index cd1f595..8018586 100644 > --- a/hw/sm501.c > +++ b/hw/sm501.c > @@ -596,7 +596,7 @@ static inline uint16_t get_hwc_color(SM501State > *state, int crt, int index) > break; > default: > printf("invalid hw cursor color.\n"); > - assert(0); > + abort(); > } > > switch (index) { > @@ -663,7 +663,7 @@ static uint32_t sm501_system_config_read(void > *opaque, target_phys_addr_t addr) > default: > printf("sm501 system config : not implemented register read." > " addr=%x\n", (int)addr); > - assert(0); > + abort(); > } > > return ret; > @@ -713,7 +713,7 @@ static void sm501_system_config_write(void *opaque, > default: > printf("sm501 system config : not implemented register write." > " addr=%x, val=%x\n", (int)addr, value); > - assert(0); > + abort(); > } > } > > @@ -843,7 +843,7 @@ static uint32_t sm501_disp_ctrl_read(void *opaque, > target_phys_addr_t addr) > default: > printf("sm501 disp ctrl : not implemented register read." > " addr=%x\n", (int)addr); > - assert(0); > + abort(); > } > > return ret; > @@ -951,7 +951,7 @@ static void sm501_disp_ctrl_write(void *opaque, > default: > printf("sm501 disp ctrl : not implemented register write." > " addr=%x, val=%x\n", (int)addr, value); > - assert(0); > + abort(); > } > } > > @@ -1097,7 +1097,7 @@ static void sm501_draw_crt(SM501State * s) > default: > printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n", > s->dc_crt_control); > - assert(0); > + abort(); > break; > } > > diff --git a/hw/tc58128.c b/hw/tc58128.c > index 264aa02..672a01c 100644 > --- a/hw/tc58128.c > +++ b/hw/tc58128.c > @@ -82,7 +82,7 @@ static void handle_command(tc58128_dev * dev, uint8_t command) > break; > default: > fprintf(stderr, "unknown flash command 0x%02x\n", command); > - assert(0); > + abort(); > } > } > > @@ -110,12 +110,12 @@ static void handle_address(tc58128_dev * dev, > uint8_t data) > break; > default: > /* Invalid data */ > - assert(0); > + abort(); > } > dev->address_cycle++; > break; > default: > - assert(0); > + abort(); > } > } > > @@ -164,7 +164,7 @@ static int tc58128_cb(uint16_t porta, uint16_t portb, > *periph_pdtra &= 0xff00; > *periph_pdtra |= handle_read(&tc58128_devs[dev]); > } else { > - assert(0); > + abort(); > } > return 1; > } > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 07616e3..e327c3d 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -411,7 +411,6 @@ static void QEMU_NORETURN force_sig(int target_sig) > sigsuspend(&act.sa_mask); > > /* unreachable */ > - assert(0); > abort(); > } > > diff --git a/qdict.c b/qdict.c > index 7fb425a..aae57bf 100644 > --- a/qdict.c > +++ b/qdict.c > @@ -194,8 +194,7 @@ double qdict_get_double(const QDict *qdict, const char *key) > case QTYPE_QINT: > return qint_get_int(qobject_to_qint(obj)); > default: > - assert(0); > - return 0.0; > + abort(); > } > } > > diff --git a/target-cris/helper.c b/target-cris/helper.c > index b101dc5..f0d60c1 100644 > --- a/target-cris/helper.c > +++ b/target-cris/helper.c > @@ -137,7 +137,7 @@ static void do_interruptv10(CPUState *env) > break; > > case EXCP_BUSFAULT: > - assert(0); > + cpu_abort(env, "Unhandled busfault"); > break; > > default: > diff --git a/target-cris/translate_v10.c b/target-cris/translate_v10.c > index 564cdb0..14e590d 100644 > --- a/target-cris/translate_v10.c > +++ b/target-cris/translate_v10.c > @@ -285,7 +285,7 @@ static unsigned int dec10_quick_imm(DisasContext *dc) > default: > LOG_DIS("pc=%x mode=%x quickimm %d r%d r%d\n", > dc->pc, dc->mode, dc->opcode, dc->src, dc->dst); > - assert(0); > + cpu_abort(dc->env, "Unhandled quickimm\n"); > break; > } > return 2; > @@ -594,7 +594,9 @@ static unsigned int dec10_reg(DisasContext *dc) > case 4: tmp = 2; break; > case 2: tmp = 1; break; > case 1: tmp = 0; break; > - default: assert(0); break; > + default: > + cpu_abort(dc->env, "Unhandled BIAP"); > + break; > } > > t = tcg_temp_new(); > @@ -611,7 +613,7 @@ static unsigned int dec10_reg(DisasContext *dc) > default: > LOG_DIS("pc=%x reg %d r%d r%d\n", dc->pc, > dc->opcode, dc->src, dc->dst); > - assert(0); > + cpu_abort(dc->env, "Unhandled opcode"); > break; > } > } else { > @@ -687,7 +689,7 @@ static unsigned int dec10_reg(DisasContext *dc) > default: > LOG_DIS("pc=%x reg %d r%d r%d\n", dc->pc, > dc->opcode, dc->src, dc->dst); > - assert(0); > + cpu_abort(dc->env, "Unhandled opcode"); > break; > } > } > @@ -945,7 +947,7 @@ static int dec10_bdap_m(DisasContext *dc, int size) > if (!dc->postinc && (dc->ir & (1 << 11))) { > int simm = dc->ir & 0xff; > > - // assert(0); > + /* cpu_abort(dc->env, "Unhandled opcode"); */ > /* sign extended. */ > simm = (int8_t)simm; > > @@ -1044,7 +1046,7 @@ static unsigned int dec10_ind(DisasContext *dc) > default: > LOG_DIS("pc=%x var-ind.%d %d r%d r%d\n", > dc->pc, size, dc->opcode, dc->src, dc->dst); > - assert(0); > + cpu_abort(dc->env, "Unhandled opcode"); > break; > } > return insn_len; > @@ -1136,7 +1138,7 @@ static unsigned int dec10_ind(DisasContext *dc) > break; > default: > LOG_DIS("ERROR pc=%x opcode=%d\n", dc->pc, dc->opcode); > - assert(0); > + cpu_abort(dc->env, "Unhandled opcode"); > break; > } > > diff --git a/target-sh4/README.sh4 b/target-sh4/README.sh4 > index a92b6f3..e578830 100644 > --- a/target-sh4/README.sh4 > +++ b/target-sh4/README.sh4 > @@ -6,7 +6,7 @@ The sh4 target is not ready at all yet for integration > in qemu. This > file describes the current state of implementation. > > Most places requiring attention and/or modification can be detected by > -looking for "XXXXX" or "assert (0)". > +looking for "XXXXX" or "abort()". > > The sh4 core is located in target-sh4/*, while the 7750 peripheral > features (IO ports for example) are located in hw/sh7750.[ch]. The > diff --git a/target-sh4/helper.c b/target-sh4/helper.c > index 486be5d..48a1e28 100644 > --- a/target-sh4/helper.c > +++ b/target-sh4/helper.c > @@ -235,7 +235,7 @@ static int itlb_replacement(CPUState * env) > return 2; > if ((env->mmucr & 0x2c000000) == 0x00000000) > return 3; > - assert(0); > + cpu_abort(env, "Unhandled itlb_replacement"); > } > > /* Find the corresponding entry in the right TLB > @@ -462,7 +462,7 @@ int cpu_sh4_handle_mmu_fault(CPUState * env, > target_ulong address, int rw, > env->exception_index = 0x100; > break; > default: > - assert(0); > + cpu_abort(env, "Unhandled MMU fault"); > } > return 1; > } > @@ -513,7 +513,7 @@ void cpu_load_tlb(CPUSH4State * env) > entry->size = 1024 * 1024; /* 1M */ > break; > default: > - assert(0); > + cpu_abort(env, "Unhandled load_tlb"); > break; > } > entry->sh = (uint8_t)cpu_ptel_sh(env->ptel); > diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c > index 529df0c..2e5f555 100644 > --- a/target-sh4/op_helper.c > +++ b/target-sh4/op_helper.c > @@ -71,7 +71,7 @@ void helper_ldtlb(void) > { > #ifdef CONFIG_USER_ONLY > /* XXXXX */ > - assert(0); > + cpu_abort(env, "Unhandled ldtlb"); > #else > cpu_load_tlb(env); > #endif > -- > 1.6.2.4 > >
Blue Swirl <blauwirbel@gmail.com> writes: > When building with -DNDEBUG, assert(0) will not stop execution > so it must not be used for abnormal termination. For each case: are you sure the code does not recover after assert(0)? Not saying it does, just asking whether you checked. > Use cpu_abort() when in CPU context, abort() otherwise. I sympathize with the general idea, but I don't like dead code after abort(). What about cleaning that up?
On 3/15/10, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > > > When building with -DNDEBUG, assert(0) will not stop execution > > so it must not be used for abnormal termination. > > > For each case: are you sure the code does not recover after assert(0)? > Not saying it does, just asking whether you checked. I had not checked but did just now, QEMU system emulators do not handle SIGABRT. The situation is different for user emulator, but then assert(0) and abort() would be equally correct or incorrect. > > Use cpu_abort() when in CPU context, abort() otherwise. > > > I sympathize with the general idea, but I don't like dead code after > abort(). What about cleaning that up? Good idea, but it should be a separate patch. This patch is "safe", whereas the cleanup patch could cause problems if it's not done carefully.
>>> I sympathize with the general idea, but I don't like dead code >> after abort(). What about cleaning that up? >> > Good idea, but it should be a separate patch. This patch is "safe", > whereas the cleanup patch could cause problems if it's not done > carefully. This patch is "safe", however I'd consider not changing assert(0)->abort() if there is code after the assert that looks like an attempt at recovering. Example: if (!p) { printf ("the impossible has happened!"); assert (0); } return p->q; should be changed to abort, while if (!p) { printf ("the impossible has happened!"); assert (0); return 0; } return p->q; should not. Paolo
On 3/15/10, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > > I sympathize with the general idea, but I don't like dead code > > > > > > > after abort(). What about cleaning that up? > > > > > > > > Good idea, but it should be a separate patch. This patch is "safe", > > whereas the cleanup patch could cause problems if it's not done > > carefully. > > > > This patch is "safe", however I'd consider not changing assert(0)->abort() > if there is code after the assert that looks like an attempt at recovering. > Example: > > if (!p) { > printf ("the impossible has happened!"); > assert (0); > } > > return p->q; > > should be changed to abort, while > > if (!p) { > printf ("the impossible has happened!"); > assert (0); > return 0; > } > > return p->q; > > should not. Why not? According to manual page, assert(x) is equal to if (!x) abort(). As I mentioned earlier, system emulators don't handle SIGABRT and for user emulators the level of bugginess remains constant (or rather, it no longer depends on NDEBUG). Therefore the recovery code will never be executed.
Paolo Bonzini <pbonzini@redhat.com> writes: >>>> I sympathize with the general idea, but I don't like dead code >>> after abort(). What about cleaning that up? >>> >> Good idea, but it should be a separate patch. This patch is "safe", >> whereas the cleanup patch could cause problems if it's not done >> carefully. > > This patch is "safe", however I'd consider not changing > assert(0)->abort() if there is code after the assert that looks like > an attempt at recovering. Example: > > if (!p) { > printf ("the impossible has happened!"); > assert (0); > } > > return p->q; > > should be changed to abort, while > > if (!p) { > printf ("the impossible has happened!"); > assert (0); > return 0; > } > > return p->q; > > should not. Except when you find that the recovery attempt is insufficient, of course. Requires closer inspection.
>> I'd consider not changing assert(0)->abort() >> if there is code after the assert that looks like an attempt at recovering. >> Example: >> >> if (!p) { >> printf ("the impossible has happened!"); >> assert (0); >> } >> >> return p->q; >> >> should be changed to abort, while >> >> if (!p) { >> printf ("the impossible has happened!"); >> assert (0); >> return 0; >> } >> >> return p->q; >> >> should not. > > Why not? According to manual page, assert(x) is equal to if (!x) abort(). > As I mentioned earlier, system emulators don't handle SIGABRT ... which won't be generated if !NDEBUG. Only if the recovery code makes sense, of course. However, my point was that those cases where there is recovery code are not no-brainers. Paolo
Blue Swirl <blauwirbel@gmail.com> writes: > On 3/15/10, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@gmail.com> writes: >> >> > When building with -DNDEBUG, assert(0) will not stop execution >> > so it must not be used for abnormal termination. >> >> >> For each case: are you sure the code does not recover after assert(0)? >> Not saying it does, just asking whether you checked. > > I had not checked but did just now, > > QEMU system emulators do not handle SIGABRT. The situation is > different for user emulator, but then assert(0) and abort() would be > equally correct or incorrect. Please don't tell me that user emulators make abort() return. abort() is declared __noreturn__, and the optimizer may well rely on that. >> > Use cpu_abort() when in CPU context, abort() otherwise. >> >> >> I sympathize with the general idea, but I don't like dead code after >> abort(). What about cleaning that up? > > Good idea, but it should be a separate patch. This patch is "safe", > whereas the cleanup patch could cause problems if it's not done > carefully. I support keeping separate things separate. However, separating something like if (mapping->first_mapping_index != first_mapping_index && mapping->info.file.offset > 0) { - assert(0); + abort(); copy_it = 1; } from if (mapping->first_mapping_index != first_mapping_index && mapping->info.file.offset > 0) { abort(); - copy_it = 1; } doesn't seem worth it.
On 3/15/10, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > I'd consider not changing assert(0)->abort() > > > if there is code after the assert that looks like an attempt at > recovering. > > > Example: > > > > > > if (!p) { > > > printf ("the impossible has happened!"); > > > assert (0); > > > } > > > > > > return p->q; > > > > > > should be changed to abort, while > > > > > > if (!p) { > > > printf ("the impossible has happened!"); > > > assert (0); > > > return 0; > > > } > > > > > > return p->q; > > > > > > should not. > > > > > > > Why not? According to manual page, assert(x) is equal to if (!x) abort(). > > As I mentioned earlier, system emulators don't handle SIGABRT > > > > ... which won't be generated if !NDEBUG. Only if the recovery code makes > sense, of course. However, my point was that those cases where there is > recovery code are not no-brainers. Except that compiling with -DNDEBUG was broken and fixed only recently with a6c6f76ceb95a0986fd1a36cc30f8241734d20c3. Thus I suspect nobody uses -DNDEBUG for production builds and the code paths after assert(0) are untested.
On 03/15/2010 07:36 PM, Markus Armbruster wrote: > Please don't tell me that user emulators make abort() return. abort() > is declared __noreturn__, and the optimizer may well rely on that. If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I suppose abort() will return. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 03/15/2010 07:36 PM, Markus Armbruster wrote: >> Please don't tell me that user emulators make abort() return. abort() >> is declared __noreturn__, and the optimizer may well rely on that. > > If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I > suppose abort() will return. I program doing that gets what it asks for, and richly deserves.
Paolo Bonzini wrote: > On 03/15/2010 07:36 PM, Markus Armbruster wrote: > >Please don't tell me that user emulators make abort() return. abort() > >is declared __noreturn__, and the optimizer may well rely on that. > > If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I suppose > abort() will return. On Linux, man abort says: If the SIGABRT signal is ignored, or caught by a handler that returns, the abort() function will still terminate the process. It does this by restoring the default disposition for SIGABRT and then raising the sig‐ nal for a second time. However I have a suspicious that I've seen abort() return on some other OS in the distant past, maybe SunOS 4. I wouldn't rely on abort() always terminating the process on all OSes. -- Jamie
Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 03/15/2010 07:36 PM, Markus Armbruster wrote: > >> Please don't tell me that user emulators make abort() return. abort() > >> is declared __noreturn__, and the optimizer may well rely on that. > > > > If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I > > suppose abort() will return. > > I program doing that gets what it asks for, and richly deserves. A guest program is also allowed to trap SIGABRT with a signal handler, and that does have some uses. E.g. cleaning up temporary files and shmem segments following a crash when calling 3rd party code. Whatever the guest does with SIGABRT, it should not result in _QEMU_ crashing - whether due to abort() returning, or QEMU's control flow jumping to the guest's signal handler from an unexpected location. -- Jamie
Jamie Lokier <jamie@shareable.org> writes: > Paolo Bonzini wrote: >> On 03/15/2010 07:36 PM, Markus Armbruster wrote: >> >Please don't tell me that user emulators make abort() return. abort() >> >is declared __noreturn__, and the optimizer may well rely on that. >> >> If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I suppose >> abort() will return. > > On Linux, man abort says: > > If the SIGABRT signal is ignored, or caught by a handler that returns, > the abort() function will still terminate the process. It does this by > restoring the default disposition for SIGABRT and then raising the sig‐ > nal for a second time. > > However I have a suspicious that I've seen abort() return on some > other OS in the distant past, maybe SunOS 4. Dark age. > I wouldn't rely on abort() always terminating the process on all OSes. Such behavior is not permitted by ISO C: 7.20.4.1 The abort function [...] The abort function causes abnormal program termination to occur, unless the signal SIGABRT is being caught and the signal handler does not return. [...] The abort function does not return to its caller.
On 03/16/2010 06:55 PM, Jamie Lokier wrote: > A guest program is also allowed to trap SIGABRT with a signal handler, > and that does have some uses. E.g. cleaning up temporary files and > shmem segments following a crash when calling 3rd party code. > > Whatever the guest does with SIGABRT, it should not result in_QEMU_ > crashing - whether due to abort() returning, or QEMU's control flow > jumping to the guest's signal handler from an unexpected location. That's very hard to ensure however if QEMU was already in unstable state, as witnessed by its call to abort(). Things can only go downhill. Maybe there should be a qemu_abort wrapper (and a QEMU_ASSERT companion) that does simply abort/assert under system emulation, but under user-mode emulation does signal (SIGABRT, SIG_DFL); abort (); Paolo
On 3/17/10, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 03/16/2010 06:55 PM, Jamie Lokier wrote: > > > A guest program is also allowed to trap SIGABRT with a signal handler, > > and that does have some uses. E.g. cleaning up temporary files and > > shmem segments following a crash when calling 3rd party code. > > > > Whatever the guest does with SIGABRT, it should not result in_QEMU_ > > crashing - whether due to abort() returning, or QEMU's control flow > > jumping to the guest's signal handler from an unexpected location. > > > > That's very hard to ensure however if QEMU was already in unstable state, > as witnessed by its call to abort(). Things can only go downhill. > > Maybe there should be a qemu_abort wrapper (and a QEMU_ASSERT companion) > that does simply abort/assert under system emulation, but under user-mode > emulation does > > signal (SIGABRT, SIG_DFL); > abort (); Fine, we cover this obscure corner case. But what if in addition to this, the user process forked and the other process ptraced the QEMU process, and when QEMU attempts to install the default signal handler, the call is instead diverted to somewhere else with ptrace?
Jamie Lokier <jamie@shareable.org> writes: > Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > On 03/15/2010 07:36 PM, Markus Armbruster wrote: >> >> Please don't tell me that user emulators make abort() return. abort() >> >> is declared __noreturn__, and the optimizer may well rely on that. >> > >> > If the user programs make a "signal (SIGABRT, SIG_IGN)" call, I >> > suppose abort() will return. >> >> I program doing that gets what it asks for, and richly deserves. > > A guest program is also allowed to trap SIGABRT with a signal handler, > and that does have some uses. E.g. cleaning up temporary files and > shmem segments following a crash when calling 3rd party code. I'd expect such handler not to return, but SIG_DFL, clean up, then re-raise the signal. Regardless, a guest program can do whatever it pleases, and that includes the stupid as well as the clever. But the guest's doings should never unduly interfere with QEMU's use of signals. > Whatever the guest does with SIGABRT, it should not result in _QEMU_ > crashing - whether due to abort() returning, or QEMU's control flow > jumping to the guest's signal handler from an unexpected location. Agreed. The guest replacing one of QEMU's handlers with SIG_IGN or SIG_DFL would be "fun", too. No idea whether that's actually possible; I know next to nothing about user mode emulation.
Blue Swirl wrote: > But what if in addition to > this, the user process forked and the other process ptraced the QEMU > process, How good is the cross-arch ptrace() emulation, anyway? :-) -- Jamie
On 3/19/10, Jamie Lokier <jamie@shareable.org> wrote: > Blue Swirl wrote: > > But what if in addition to > > this, the user process forked and the other process ptraced the QEMU > > process, > > > How good is the cross-arch ptrace() emulation, anyway? :-) Probably as good as SIGABRT abuse support. But the case could be native as well, not cross-arch. Actually cross-arch execution of strace could be interesting. GDB not so much. UML?
diff --git a/block/vvfat.c b/block/vvfat.c index aaa8593..36f6ab4 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1638,7 +1638,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s, /* new file */ schedule_new_file(s, qemu_strdup(path), cluster_num); else { - assert(0); + abort(); return 0; } } @@ -1659,7 +1659,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s, if (offset != mapping->info.file.offset + s->cluster_size * (cluster_num - mapping->begin)) { /* offset of this cluster in file chain has changed */ - assert(0); + abort(); copy_it = 1; } else if (offset == 0) { const char* basename = get_basename(mapping->path); @@ -1671,7 +1671,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s, if (mapping->first_mapping_index != first_mapping_index && mapping->info.file.offset > 0) { - assert(0); + abort(); copy_it = 1; } @@ -1837,7 +1837,7 @@ DLOG(fprintf(stderr, "check direntry %d: \n", i); print_direntry(direntries + i) goto fail; } } else - assert(0); /* cluster_count = 0; */ + abort(); /* cluster_count = 0; */ ret += cluster_count; } @@ -2458,7 +2458,7 @@ static int handle_commits(BDRVVVFATState* s) commit_t* commit = array_get(&(s->commits), i); switch(commit->action) { case ACTION_RENAME: case ACTION_MKDIR: - assert(0); + abort(); fail = -2; break; case ACTION_WRITEOUT: { @@ -2519,7 +2519,7 @@ static int handle_commits(BDRVVVFATState* s) break; } default: - assert(0); + abort(); } } if (i > 0 && array_remove_slice(&(s->commits), 0, i)) @@ -2607,7 +2607,7 @@ static int do_commit(BDRVVVFATState* s) ret = handle_renames_and_mkdirs(s); if (ret) { fprintf(stderr, "Error handling renames (%d)\n", ret); - assert(0); + abort(); return ret; } @@ -2618,21 +2618,21 @@ static int do_commit(BDRVVVFATState* s) ret = commit_direntries(s, 0, -1); if (ret) { fprintf(stderr, "Fatal: error while committing (%d)\n", ret); - assert(0); + abort(); return ret; } ret = handle_commits(s); if (ret) { fprintf(stderr, "Error handling commits (%d)\n", ret); - assert(0); + abort(); return ret; } ret = handle_deletes(s); if (ret) { fprintf(stderr, "Error deleting\n"); - assert(0); + abort(); return ret; } diff --git a/hw/sh7750.c b/hw/sh7750.c index 9c39f4b..0291d5f 100644 --- a/hw/sh7750.c +++ b/hw/sh7750.c @@ -206,7 +206,7 @@ static uint32_t sh7750_mem_readb(void *opaque, target_phys_addr_t addr) switch (addr) { default: error_access("byte read", addr); - assert(0); + abort(); } } @@ -240,7 +240,7 @@ static uint32_t sh7750_mem_readw(void *opaque, target_phys_addr_t addr) return 0; default: error_access("word read", addr); - assert(0); + abort(); } } @@ -287,7 +287,7 @@ static uint32_t sh7750_mem_readl(void *opaque, target_phys_addr_t addr) return s->cpu->prr; default: error_access("long read", addr); - assert(0); + abort(); } } @@ -303,7 +303,7 @@ static void sh7750_mem_writeb(void *opaque, target_phys_addr_t addr, } error_access("byte write", addr); - assert(0); + abort(); } static void sh7750_mem_writew(void *opaque, target_phys_addr_t addr, @@ -349,12 +349,12 @@ static void sh7750_mem_writew(void *opaque, target_phys_addr_t addr, s->gpioic = mem_value; if (mem_value != 0) { fprintf(stderr, "I/O interrupts not implemented\n"); - assert(0); + abort(); } return; default: error_access("word write", addr); - assert(0); + abort(); } } @@ -433,7 +433,7 @@ static void sh7750_mem_writel(void *opaque, target_phys_addr_t addr, return; default: error_access("long write", addr); - assert(0); + abort(); } } @@ -618,7 +618,7 @@ static struct intc_group groups_irl[] = { static uint32_t invalid_read(void *opaque, target_phys_addr_t addr) { - assert(0); + abort(); return 0; } @@ -635,7 +635,7 @@ static uint32_t sh7750_mmct_readl(void *opaque, target_phys_addr_t addr) case MM_ITLB_ADDR: case MM_ITLB_DATA: /* XXXXX */ - assert(0); + abort(); break; case MM_OCACHE_ADDR: case MM_OCACHE_DATA: @@ -644,10 +644,10 @@ static uint32_t sh7750_mmct_readl(void *opaque, target_phys_addr_t addr) case MM_UTLB_ADDR: case MM_UTLB_DATA: /* XXXXX */ - assert(0); + abort(); break; default: - assert(0); + abort(); } return ret; @@ -656,7 +656,7 @@ static uint32_t sh7750_mmct_readl(void *opaque, target_phys_addr_t addr) static void invalid_write(void *opaque, target_phys_addr_t addr, uint32_t mem_value) { - assert(0); + abort(); } static void sh7750_mmct_writel(void *opaque, target_phys_addr_t addr, @@ -672,7 +672,7 @@ static void sh7750_mmct_writel(void *opaque, target_phys_addr_t addr, case MM_ITLB_ADDR: case MM_ITLB_DATA: /* XXXXX */ - assert(0); + abort(); break; case MM_OCACHE_ADDR: case MM_OCACHE_DATA: @@ -683,10 +683,10 @@ static void sh7750_mmct_writel(void *opaque, target_phys_addr_t addr, break; case MM_UTLB_DATA: /* XXXXX */ - assert(0); + abort(); break; default: - assert(0); + abort(); break; } } diff --git a/hw/sh_intc.c b/hw/sh_intc.c index b6f45f0..da36d32 100644 --- a/hw/sh_intc.c +++ b/hw/sh_intc.c @@ -105,7 +105,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask) } } - assert(0); + abort(); } #define INTC_MODE_NONE 0 @@ -181,7 +181,7 @@ static void sh_intc_locate(struct intc_desc *desc, } } - assert(0); + abort(); } static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id, @@ -260,7 +260,7 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset, case INTC_MODE_ENABLE_REG | INTC_MODE_IS_PRIO: break; case INTC_MODE_DUAL_SET: value |= *valuep; break; case INTC_MODE_DUAL_CLR: value = *valuep & ~value; break; - default: assert(0); + default: abort(); } for (k = 0; k <= first; k++) { diff --git a/hw/sh_serial.c b/hw/sh_serial.c index 2447b91..93dc144 100644 --- a/hw/sh_serial.c +++ b/hw/sh_serial.c @@ -182,7 +182,7 @@ static void sh_serial_ioport_write(void *opaque, uint32_t offs, uint32_t val) } fprintf(stderr, "sh_serial: unsupported write to 0x%02x\n", offs); - assert(0); + abort(); } static uint32_t sh_serial_ioport_read(void *opaque, uint32_t offs) @@ -282,7 +282,7 @@ static uint32_t sh_serial_ioport_read(void *opaque, uint32_t offs) if (ret & ~((1 << 16) - 1)) { fprintf(stderr, "sh_serial: unsupported read from 0x%02x\n", offs); - assert(0); + abort(); } return ret; diff --git a/hw/sm501.c b/hw/sm501.c index cd1f595..8018586 100644 --- a/hw/sm501.c +++ b/hw/sm501.c @@ -596,7 +596,7 @@ static inline uint16_t get_hwc_color(SM501State *state, int crt, int index) break; default: printf("invalid hw cursor color.\n"); - assert(0); + abort(); } switch (index) { @@ -663,7 +663,7 @@ static uint32_t sm501_system_config_read(void *opaque, target_phys_addr_t addr) default: printf("sm501 system config : not implemented register read." " addr=%x\n", (int)addr); - assert(0); + abort(); } return ret; @@ -713,7 +713,7 @@ static void sm501_system_config_write(void *opaque, default: printf("sm501 system config : not implemented register write." " addr=%x, val=%x\n", (int)addr, value); - assert(0); + abort(); } } @@ -843,7 +843,7 @@ static uint32_t sm501_disp_ctrl_read(void *opaque, target_phys_addr_t addr) default: printf("sm501 disp ctrl : not implemented register read." " addr=%x\n", (int)addr); - assert(0); + abort(); } return ret; @@ -951,7 +951,7 @@ static void sm501_disp_ctrl_write(void *opaque, default: printf("sm501 disp ctrl : not implemented register write." " addr=%x, val=%x\n", (int)addr, value); - assert(0); + abort(); } } @@ -1097,7 +1097,7 @@ static void sm501_draw_crt(SM501State * s) default: printf("sm501 draw crt : invalid DC_CRT_CONTROL=%x.\n", s->dc_crt_control); - assert(0); + abort(); break; } diff --git a/hw/tc58128.c b/hw/tc58128.c index 264aa02..672a01c 100644 --- a/hw/tc58128.c +++ b/hw/tc58128.c @@ -82,7 +82,7 @@ static void handle_command(tc58128_dev * dev, uint8_t command) break; default: fprintf(stderr, "unknown flash command 0x%02x\n", command); - assert(0); + abort(); } } @@ -110,12 +110,12 @@ static void handle_address(tc58128_dev * dev, uint8_t data) break; default: /* Invalid data */ - assert(0); + abort(); } dev->address_cycle++; break; default: - assert(0); + abort(); } } @@ -164,7 +164,7 @@ static int tc58128_cb(uint16_t porta, uint16_t portb, *periph_pdtra &= 0xff00; *periph_pdtra |= handle_read(&tc58128_devs[dev]); } else { - assert(0); + abort(); } return 1; } diff --git a/linux-user/signal.c b/linux-user/signal.c index 07616e3..e327c3d 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -411,7 +411,6 @@ static void QEMU_NORETURN force_sig(int target_sig) sigsuspend(&act.sa_mask); /* unreachable */ - assert(0); abort(); } diff --git a/qdict.c b/qdict.c index 7fb425a..aae57bf 100644 --- a/qdict.c +++ b/qdict.c @@ -194,8 +194,7 @@ double qdict_get_double(const QDict *qdict, const char *key) case QTYPE_QINT: return qint_get_int(qobject_to_qint(obj)); default: - assert(0); - return 0.0; + abort(); } } diff --git a/target-cris/helper.c b/target-cris/helper.c index b101dc5..f0d60c1 100644 --- a/target-cris/helper.c +++ b/target-cris/helper.c @@ -137,7 +137,7 @@ static void do_interruptv10(CPUState *env) break; case EXCP_BUSFAULT: - assert(0); + cpu_abort(env, "Unhandled busfault"); break; default: diff --git a/target-cris/translate_v10.c b/target-cris/translate_v10.c index 564cdb0..14e590d 100644 --- a/target-cris/translate_v10.c +++ b/target-cris/translate_v10.c @@ -285,7 +285,7 @@ static unsigned int dec10_quick_imm(DisasContext *dc) default: LOG_DIS("pc=%x mode=%x quickimm %d r%d r%d\n", dc->pc, dc->mode, dc->opcode, dc->src, dc->dst); - assert(0); + cpu_abort(dc->env, "Unhandled quickimm\n"); break; } return 2; @@ -594,7 +594,9 @@ static unsigned int dec10_reg(DisasContext *dc) case 4: tmp = 2; break; case 2: tmp = 1; break; case 1: tmp = 0; break; - default: assert(0); break; + default: + cpu_abort(dc->env, "Unhandled BIAP"); + break; } t = tcg_temp_new(); @@ -611,7 +613,7 @@ static unsigned int dec10_reg(DisasContext *dc) default: LOG_DIS("pc=%x reg %d r%d r%d\n", dc->pc, dc->opcode, dc->src, dc->dst); - assert(0); + cpu_abort(dc->env, "Unhandled opcode"); break; } } else { @@ -687,7 +689,7 @@ static unsigned int dec10_reg(DisasContext *dc) default: LOG_DIS("pc=%x reg %d r%d r%d\n", dc->pc, dc->opcode, dc->src, dc->dst); - assert(0); + cpu_abort(dc->env, "Unhandled opcode"); break; } } @@ -945,7 +947,7 @@ static int dec10_bdap_m(DisasContext *dc, int size) if (!dc->postinc && (dc->ir & (1 << 11))) { int simm = dc->ir & 0xff; - // assert(0); + /* cpu_abort(dc->env, "Unhandled opcode"); */ /* sign extended. */ simm = (int8_t)simm; @@ -1044,7 +1046,7 @@ static unsigned int dec10_ind(DisasContext *dc) default: LOG_DIS("pc=%x var-ind.%d %d r%d r%d\n", dc->pc, size, dc->opcode, dc->src, dc->dst); - assert(0); + cpu_abort(dc->env, "Unhandled opcode"); break; } return insn_len; @@ -1136,7 +1138,7 @@ static unsigned int dec10_ind(DisasContext *dc) break; default: LOG_DIS("ERROR pc=%x opcode=%d\n", dc->pc, dc->opcode); - assert(0); + cpu_abort(dc->env, "Unhandled opcode"); break; } diff --git a/target-sh4/README.sh4 b/target-sh4/README.sh4 index a92b6f3..e578830 100644 --- a/target-sh4/README.sh4 +++ b/target-sh4/README.sh4 @@ -6,7 +6,7 @@ The sh4 target is not ready at all yet for integration in qemu. This file describes the current state of implementation. Most places requiring attention and/or modification can be detected by -looking for "XXXXX" or "assert (0)". +looking for "XXXXX" or "abort()". The sh4 core is located in target-sh4/*, while the 7750 peripheral features (IO ports for example) are located in hw/sh7750.[ch]. The diff --git a/target-sh4/helper.c b/target-sh4/helper.c index 486be5d..48a1e28 100644 --- a/target-sh4/helper.c +++ b/target-sh4/helper.c @@ -235,7 +235,7 @@ static int itlb_replacement(CPUState * env) return 2; if ((env->mmucr & 0x2c000000) == 0x00000000) return 3; - assert(0); + cpu_abort(env, "Unhandled itlb_replacement"); } /* Find the corresponding entry in the right TLB @@ -462,7 +462,7 @@ int cpu_sh4_handle_mmu_fault(CPUState * env, target_ulong address, int rw, env->exception_index = 0x100; break; default: - assert(0); + cpu_abort(env, "Unhandled MMU fault"); } return 1; } @@ -513,7 +513,7 @@ void cpu_load_tlb(CPUSH4State * env) entry->size = 1024 * 1024; /* 1M */ break; default: - assert(0); + cpu_abort(env, "Unhandled load_tlb"); break; } entry->sh = (uint8_t)cpu_ptel_sh(env->ptel); diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c index 529df0c..2e5f555 100644 --- a/target-sh4/op_helper.c +++ b/target-sh4/op_helper.c @@ -71,7 +71,7 @@ void helper_ldtlb(void) { #ifdef CONFIG_USER_ONLY /* XXXXX */ - assert(0); + cpu_abort(env, "Unhandled ldtlb"); #else cpu_load_tlb(env); #endif
When building with -DNDEBUG, assert(0) will not stop execution so it must not be used for abnormal termination. Use cpu_abort() when in CPU context, abort() otherwise. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- block/vvfat.c | 20 ++++++++++---------- hw/sh7750.c | 30 +++++++++++++++--------------- hw/sh_intc.c | 6 +++--- hw/sh_serial.c | 4 ++-- hw/sm501.c | 12 ++++++------ hw/tc58128.c | 8 ++++---- linux-user/signal.c | 1 - qdict.c | 3 +-- target-cris/helper.c | 2 +- target-cris/translate_v10.c | 16 +++++++++------- target-sh4/README.sh4 | 2 +- target-sh4/helper.c | 6 +++--- target-sh4/op_helper.c | 2 +- 13 files changed, 56 insertions(+), 56 deletions(-)