| Message ID | 1359293537-8251-17-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Am 27.01.2013 15:14, schrieb Anthony Liguori: > Andreas Färber <afaerber@suse.de> writes: >> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >> index 0a1ac86..54722c4 100644 >> --- a/target-ppc/excp_helper.c >> +++ b/target-ppc/excp_helper.c >> @@ -21,14 +21,14 @@ >> >> #include "helper_regs.h" >> >> -//#define DEBUG_OP >> -//#define DEBUG_EXCEPTIONS >> +#define DEBUG_OP 0 >> +#define DEBUG_EXCEPTIONS 0 >> >> -#ifdef DEBUG_EXCEPTIONS >> -# define LOG_EXCP(...) qemu_log(__VA_ARGS__) >> -#else >> -# define LOG_EXCP(...) do { } while (0) >> -#endif >> +#define LOG_EXCP(...) G_STMT_START \ >> + if (DEBUG_EXCEPTIONS) { \ >> + qemu_log(__VA_ARGS__); \ >> + } \ >> + G_STMT_END > > Just thinking out loud a bit.. This form becomes pretty common and it's > ashame to use a macro here if we don't have to. > > I think: > > static inline void LOG_EXCP(const char *fmt, ...) > { > if (debug_exceptions) { > va_list ap; > va_start(ap, fmt); > qemu_logv(fmt, ap); > va_end(ap); > } > } > > Probably would have equivalent performance. debug_exception would be > read-mostly and ought to be very predictable as a result. I strongly > expect that the compiler would actually inline LOG_EXCP too. Thanks for your early feedback. I merely tried to stay close to the original code. I wouldn't mind inline functions either. Or even more harmonization for that matter. > I see LOG_EXCP and LOG_DIS in this series. Perhaps we could just > introduce these functions and then make these flags run-time > controllable? I was feeling conservative during that series in light of compile-time decided conditional; if we want to go down that route we should probably sprinkle quite some unlikely()s for optimization. I think the if (0) { ... } approach would already catch a few things. As a next step, some mechanism as proposed by Peter C. (?) to enable things at configure-time could be built on top. Run-time would need some stabilization phase to avoid command line compatibility issues. > BTW, one advantage of this over your original proposal back to your > point is that you still won't catch linker errors with your proposal. > Dead code eliminate will kill off those branches before the linker ever > sees them. Linker errors would be limited to renamed/dropped/#ifdef'ed functions, wouldn't they? In the past I caught that using existing --enable-debug. My recurring issue is overlooking env->something after removing fields from CPU_COMMON/CPUArchState. I was hoping that to be caught inside if (0) { ... } during my 3x KVM + BSD + MinGW builds rather than patching individual files. Regards, Andreas
On 27.01.2013, at 15:35, Andreas Färber wrote: > Am 27.01.2013 15:14, schrieb Anthony Liguori: >> Andreas Färber <afaerber@suse.de> writes: >>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>> index 0a1ac86..54722c4 100644 >>> --- a/target-ppc/excp_helper.c >>> +++ b/target-ppc/excp_helper.c >>> @@ -21,14 +21,14 @@ >>> >>> #include "helper_regs.h" >>> >>> -//#define DEBUG_OP >>> -//#define DEBUG_EXCEPTIONS >>> +#define DEBUG_OP 0 >>> +#define DEBUG_EXCEPTIONS 0 >>> >>> -#ifdef DEBUG_EXCEPTIONS >>> -# define LOG_EXCP(...) qemu_log(__VA_ARGS__) >>> -#else >>> -# define LOG_EXCP(...) do { } while (0) >>> -#endif >>> +#define LOG_EXCP(...) G_STMT_START \ >>> + if (DEBUG_EXCEPTIONS) { \ >>> + qemu_log(__VA_ARGS__); \ >>> + } \ >>> + G_STMT_END >> >> Just thinking out loud a bit.. This form becomes pretty common and it's >> ashame to use a macro here if we don't have to. >> >> I think: >> >> static inline void LOG_EXCP(const char *fmt, ...) >> { >> if (debug_exceptions) { >> va_list ap; >> va_start(ap, fmt); >> qemu_logv(fmt, ap); >> va_end(ap); >> } >> } >> >> Probably would have equivalent performance. debug_exception would be >> read-mostly and ought to be very predictable as a result. I strongly >> expect that the compiler would actually inline LOG_EXCP too. > > Thanks for your early feedback. I merely tried to stay close to the > original code. I wouldn't mind inline functions either. Or even more > harmonization for that matter. I fully agree. Just recently Scott revamped the openpic debug print code: //#define DEBUG_OPENPIC #ifdef DEBUG_OPENPIC static const int debug_openpic = 1; #else static const int debug_openpic = 0; #endif #define DPRINTF(fmt, ...) do { \ if (debug_openpic) { \ printf(fmt , ## __VA_ARGS__); \ } \ } while (0) I like that approach. It keeps all users identical. The #define stays identical. The callers stay identical. But we do get proper compile time checks. Of course Anthony's approach works too, but the thing I'd definitely like to see is that the #defines don't become numerical, but rather stay of an #ifdef basis. Alex
Am 27.01.2013 15:46, schrieb Alexander Graf: > > On 27.01.2013, at 15:35, Andreas Färber wrote: > >> Am 27.01.2013 15:14, schrieb Anthony Liguori: >>> Andreas Färber <afaerber@suse.de> writes: >>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>>> index 0a1ac86..54722c4 100644 >>>> --- a/target-ppc/excp_helper.c >>>> +++ b/target-ppc/excp_helper.c >>>> @@ -21,14 +21,14 @@ >>>> >>>> #include "helper_regs.h" >>>> >>>> -//#define DEBUG_OP >>>> -//#define DEBUG_EXCEPTIONS >>>> +#define DEBUG_OP 0 >>>> +#define DEBUG_EXCEPTIONS 0 >>>> >>>> -#ifdef DEBUG_EXCEPTIONS >>>> -# define LOG_EXCP(...) qemu_log(__VA_ARGS__) >>>> -#else >>>> -# define LOG_EXCP(...) do { } while (0) >>>> -#endif >>>> +#define LOG_EXCP(...) G_STMT_START \ >>>> + if (DEBUG_EXCEPTIONS) { \ >>>> + qemu_log(__VA_ARGS__); \ >>>> + } \ >>>> + G_STMT_END >>> >>> Just thinking out loud a bit.. This form becomes pretty common and it's >>> ashame to use a macro here if we don't have to. >>> >>> I think: >>> >>> static inline void LOG_EXCP(const char *fmt, ...) >>> { >>> if (debug_exceptions) { >>> va_list ap; >>> va_start(ap, fmt); >>> qemu_logv(fmt, ap); >>> va_end(ap); >>> } >>> } >>> >>> Probably would have equivalent performance. debug_exception would be >>> read-mostly and ought to be very predictable as a result. I strongly >>> expect that the compiler would actually inline LOG_EXCP too. >> >> Thanks for your early feedback. I merely tried to stay close to the >> original code. I wouldn't mind inline functions either. Or even more >> harmonization for that matter. > > I fully agree. Just recently Scott revamped the openpic debug print code: > > > //#define DEBUG_OPENPIC > > #ifdef DEBUG_OPENPIC > static const int debug_openpic = 1; > #else > static const int debug_openpic = 0; > #endif > > #define DPRINTF(fmt, ...) do { \ > if (debug_openpic) { \ > printf(fmt , ## __VA_ARGS__); \ > } \ > } while (0) Like :) I'll wait for more feedback from affected maintainers though before I redo or widen this series. Or were you proposing to do a ppc-only refactoring à la Scott for 1.4? Andreas > I like that approach. It keeps all users identical. The #define stays identical. The callers stay identical. But we do get proper compile time checks. Of course Anthony's approach works too, but the thing I'd definitely like to see is that the #defines don't become numerical, but rather stay of an #ifdef basis. > > > Alex
Am 27.01.2013 um 15:54 schrieb Andreas Färber <afaerber@suse.de>: > Am 27.01.2013 15:46, schrieb Alexander Graf: >> >> On 27.01.2013, at 15:35, Andreas Färber wrote: >> >>> Am 27.01.2013 15:14, schrieb Anthony Liguori: >>>> Andreas Färber <afaerber@suse.de> writes: >>>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c >>>>> index 0a1ac86..54722c4 100644 >>>>> --- a/target-ppc/excp_helper.c >>>>> +++ b/target-ppc/excp_helper.c >>>>> @@ -21,14 +21,14 @@ >>>>> >>>>> #include "helper_regs.h" >>>>> >>>>> -//#define DEBUG_OP >>>>> -//#define DEBUG_EXCEPTIONS >>>>> +#define DEBUG_OP 0 >>>>> +#define DEBUG_EXCEPTIONS 0 >>>>> >>>>> -#ifdef DEBUG_EXCEPTIONS >>>>> -# define LOG_EXCP(...) qemu_log(__VA_ARGS__) >>>>> -#else >>>>> -# define LOG_EXCP(...) do { } while (0) >>>>> -#endif >>>>> +#define LOG_EXCP(...) G_STMT_START \ >>>>> + if (DEBUG_EXCEPTIONS) { \ >>>>> + qemu_log(__VA_ARGS__); \ >>>>> + } \ >>>>> + G_STMT_END >>>> >>>> Just thinking out loud a bit.. This form becomes pretty common and it's >>>> ashame to use a macro here if we don't have to. >>>> >>>> I think: >>>> >>>> static inline void LOG_EXCP(const char *fmt, ...) >>>> { >>>> if (debug_exceptions) { >>>> va_list ap; >>>> va_start(ap, fmt); >>>> qemu_logv(fmt, ap); >>>> va_end(ap); >>>> } >>>> } >>>> >>>> Probably would have equivalent performance. debug_exception would be >>>> read-mostly and ought to be very predictable as a result. I strongly >>>> expect that the compiler would actually inline LOG_EXCP too. >>> >>> Thanks for your early feedback. I merely tried to stay close to the >>> original code. I wouldn't mind inline functions either. Or even more >>> harmonization for that matter. >> >> I fully agree. Just recently Scott revamped the openpic debug print code: >> >> >> //#define DEBUG_OPENPIC >> >> #ifdef DEBUG_OPENPIC >> static const int debug_openpic = 1; >> #else >> static const int debug_openpic = 0; >> #endif >> >> #define DPRINTF(fmt, ...) do { \ >> if (debug_openpic) { \ >> printf(fmt , ## __VA_ARGS__); \ >> } \ >> } while (0) > > Like :) > > I'll wait for more feedback from affected maintainers though before I > redo or widen this series. > Or were you proposing to do a ppc-only refactoring à la Scott for 1.4? No, I think it makes a lot more sense to do this tree-wide after 1.4 :). Alex > > Andreas > >> I like that approach. It keeps all users identical. The #define stays identical. The callers stay identical. But we do get proper compile time checks. Of course Anthony's approach works too, but the thing I'd definitely like to see is that the #defines don't become numerical, but rather stay of an #ifdef basis. >> >> >> Alex > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c index 0a1ac86..54722c4 100644 --- a/target-ppc/excp_helper.c +++ b/target-ppc/excp_helper.c @@ -21,14 +21,14 @@ #include "helper_regs.h" -//#define DEBUG_OP -//#define DEBUG_EXCEPTIONS +#define DEBUG_OP 0 +#define DEBUG_EXCEPTIONS 0 -#ifdef DEBUG_EXCEPTIONS -# define LOG_EXCP(...) qemu_log(__VA_ARGS__) -#else -# define LOG_EXCP(...) do { } while (0) -#endif +#define LOG_EXCP(...) G_STMT_START \ + if (DEBUG_EXCEPTIONS) { \ + qemu_log(__VA_ARGS__); \ + } \ + G_STMT_END /*****************************************************************************/ /* PowerPC Hypercall emulation */ @@ -777,7 +777,7 @@ void ppc_hw_interrupt(CPUPPCState *env) } #endif /* !CONFIG_USER_ONLY */ -#if defined(DEBUG_OP) +#ifndef CONFIG_USER_ONLY static void cpu_dump_rfi(target_ulong RA, target_ulong msr) { qemu_log("Return from exception at " TARGET_FMT_lx " with flags " @@ -835,9 +835,9 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr, /* XXX: beware: this is false if VLE is supported */ env->nip = nip & ~((target_ulong)0x00000003); hreg_store_msr(env, msr, 1); -#if defined(DEBUG_OP) - cpu_dump_rfi(env->nip, env->msr); -#endif + if (DEBUG_OP) { + cpu_dump_rfi(env->nip, env->msr); + } /* No need to raise an exception here, * as rfi is always the last insn of a TB */ diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 2f4f068..0dc6657 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -37,15 +37,10 @@ #include "hw/spapr.h" #include "hw/spapr_vio.h" -//#define DEBUG_KVM +#define DEBUG_KVM 0 -#ifdef DEBUG_KVM #define dprintf(fmt, ...) \ - do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) -#else -#define dprintf(fmt, ...) \ - do { } while (0) -#endif + do { if (DEBUG_KVM) { fprintf(stderr, fmt, ## __VA_ARGS__); } } while (0) #define PROC_DEVTREE_CPU "/proc/device-tree/cpus/" diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c index 902b1cd..5c7a5ce 100644 --- a/target-ppc/mem_helper.c +++ b/target-ppc/mem_helper.c @@ -26,8 +26,6 @@ #include "exec/softmmu_exec.h" #endif /* !defined(CONFIG_USER_ONLY) */ -//#define DEBUG_OP - /*****************************************************************************/ /* Memory load and stores */ diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c index ee168f1..9340fbb 100644 --- a/target-ppc/mmu_helper.c +++ b/target-ppc/mmu_helper.c @@ -21,39 +21,36 @@ #include "sysemu/kvm.h" #include "kvm_ppc.h" -//#define DEBUG_MMU -//#define DEBUG_BATS -//#define DEBUG_SLB -//#define DEBUG_SOFTWARE_TLB +#define DEBUG_MMU 0 +#define DEBUG_BATS 0 +#define DEBUG_SLB 0 +#define DEBUG_SOFTWARE_TLB 0 //#define DUMP_PAGE_TABLES -//#define DEBUG_SOFTWARE_TLB //#define FLUSH_ALL_TLBS -#ifdef DEBUG_MMU -# define LOG_MMU(...) qemu_log(__VA_ARGS__) -# define LOG_MMU_STATE(env) log_cpu_state((env), 0) -#else -# define LOG_MMU(...) do { } while (0) -# define LOG_MMU_STATE(...) do { } while (0) -#endif - -#ifdef DEBUG_SOFTWARE_TLB -# define LOG_SWTLB(...) qemu_log(__VA_ARGS__) -#else -# define LOG_SWTLB(...) do { } while (0) -#endif - -#ifdef DEBUG_BATS -# define LOG_BATS(...) qemu_log(__VA_ARGS__) -#else -# define LOG_BATS(...) do { } while (0) -#endif - -#ifdef DEBUG_SLB -# define LOG_SLB(...) qemu_log(__VA_ARGS__) -#else -# define LOG_SLB(...) do { } while (0) -#endif +#define LOG_MMU(...) G_STMT_START \ + if (DEBUG_MMU) { \ + qemu_log(__VA_ARGS__); \ + } \ + G_STMT_END + +#define LOG_SWTLB(...) G_STMT_START \ + if (DEBUG_SOFTWARE_TLB) { \ + qemu_log(__VA_ARGS__); \ + } \ + G_STMT_END + +#define LOG_BATS(...) G_STMT_START \ + if (DEBUG_BATS) { \ + qemu_log(__VA_ARGS__); \ + } \ + G_STMT_END + +#define LOG_SLB(...) G_STMT_START \ + if (DEBUG_SLB) { \ + qemu_log(__VA_ARGS__); \ + } \ + G_STMT_END /*****************************************************************************/ /* PowerPC MMU emulation */ @@ -534,7 +531,7 @@ static inline int get_bat(CPUPPCState *env, mmu_ctx_t *ctx, } } if (ret < 0) { -#if defined(DEBUG_BATS) +#if DEBUG_BATS if (qemu_log_enabled()) { LOG_BATS("no BAT match for " TARGET_FMT_lx ":\n", virtual); for (i = 0; i < 4; i++) { @@ -1860,7 +1857,9 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, mmu_idx, TARGET_PAGE_SIZE); ret = 0; } else if (ret < 0) { - LOG_MMU_STATE(env); + if (DEBUG_MMU) { + log_cpu_state(env, 0); + } if (access_type == ACCESS_CODE) { switch (ret) { case -1: diff --git a/target-ppc/translate.c b/target-ppc/translate.c index 37ce55f..c599fa7 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -32,14 +32,14 @@ #define GDBSTUB_SINGLE_STEP 0x4 /* Include definitions for instructions classes and implementations flags */ -//#define PPC_DEBUG_DISAS +#define PPC_DEBUG_DISAS 0 //#define DO_PPC_STATISTICS -#ifdef PPC_DEBUG_DISAS -# define LOG_DISAS(...) qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__) -#else -# define LOG_DISAS(...) do { } while (0) -#endif +#define LOG_DISAS(...) G_STMT_START \ + if (PPC_DEBUG_DISAS) { \ + qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__); \ + } \ + G_STMT_END /*****************************************************************************/ /* Code translation helpers */ diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 4f767c9..7ab4d61 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -31,7 +31,7 @@ #include "sysemu/cpus.h" //#define PPC_DUMP_CPU -//#define PPC_DEBUG_SPR +#define PPC_DEBUG_SPR 0 //#define PPC_DUMP_SPR_ACCESSES #if defined(CONFIG_USER_ONLY) #define TODO_USER_ONLY 1 @@ -611,10 +611,10 @@ static inline void spr_register (CPUPPCState *env, int num, printf("Error: Trying to register SPR %d (%03x) twice !\n", num, num); exit(1); } -#if defined(PPC_DEBUG_SPR) - printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", num, num, - name, initial_value); -#endif + if (PPC_DEBUG_SPR) { + printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", + num, num, name, initial_value); + } spr->name = name; spr->uea_read = uea_read; spr->uea_write = uea_write;
Make debug output compile-testable even if disabled. Inline DEBUG_OP check in excp_helper.c. Inline LOG_MMU_STATE() in mmu_helper.c. Inline PPC_DEBUG_SPR check in translate_init.c. Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-ppc/excp_helper.c | 22 +++++++-------- target-ppc/kvm.c | 9 ++----- target-ppc/mem_helper.c | 2 -- target-ppc/mmu_helper.c | 63 +++++++++++++++++++++---------------------- target-ppc/translate.c | 12 ++++----- target-ppc/translate_init.c | 10 +++---- 6 Dateien geändert, 55 Zeilen hinzugefügt(+), 63 Zeilen entfernt(-)