* [Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state
@ 2012-05-14 14:46 Fabien Chouteau
2012-05-14 15:39 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-14 14:46 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, agraf
Quit if no log file is defined.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
target-ppc/translate.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index cf59765..f17bd91 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -9319,6 +9319,10 @@ void cpu_dump_state (CPUPPCState *env, FILE *f, fprintf_function cpu_fprintf,
int i;
+ if (f == NULL) {
+ return;
+ }
+
cpu_synchronize_state(env);
cpu_fprintf(f, "NIP " TARGET_FMT_lx " LR " TARGET_FMT_lx " CTR "
--
1.7.9.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state
2012-05-14 14:46 [Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state Fabien Chouteau
@ 2012-05-14 15:39 ` Peter Maydell
2012-05-15 9:39 ` [Qemu-devel] [PATCH] " Fabien Chouteau
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2012-05-14 15:39 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-ppc, qemu-devel, agraf
On 14 May 2012 15:46, Fabien Chouteau <chouteau@adacore.com> wrote:
> Quit if no log file is defined.
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
> target-ppc/translate.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index cf59765..f17bd91 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -9319,6 +9319,10 @@ void cpu_dump_state (CPUPPCState *env, FILE *f, fprintf_function cpu_fprintf,
>
> int i;
>
> + if (f == NULL) {
> + return;
> + }
> +
> cpu_synchronize_state(env);
>
> cpu_fprintf(f, "NIP " TARGET_FMT_lx " LR " TARGET_FMT_lx " CTR "
target-ppc isn't the only one that doesn't check for a NULL f:
perhaps it would be better to say "you can't call this with a
NULL FILE*" and fix whatever is calling it in that way?
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-14 15:39 ` Peter Maydell
@ 2012-05-15 9:39 ` Fabien Chouteau
2012-05-15 13:31 ` Andreas Färber
0 siblings, 1 reply; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-15 9:39 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, agraf
Do not call cpu_dump_state if logfile is NULL.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
qemu-log.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/qemu-log.h b/qemu-log.h
index fccfb110..2cd5ffa 100644
--- a/qemu-log.h
+++ b/qemu-log.h
@@ -51,7 +51,12 @@ extern int loglevel;
/* Special cases: */
/* cpu_dump_state() logging functions: */
-#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
+#define log_cpu_state(env, f) \
+do { \
+ if (logfile != NULL) { \
+ cpu_dump_state((env), logfile, fprintf, (f)); \
+ } \
+ } while (0)
#define log_cpu_state_mask(b, env, f) do { \
if (loglevel & (b)) log_cpu_state((env), (f)); \
} while (0)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 9:39 ` [Qemu-devel] [PATCH] " Fabien Chouteau
@ 2012-05-15 13:31 ` Andreas Färber
2012-05-15 16:08 ` Fabien Chouteau
0 siblings, 1 reply; 12+ messages in thread
From: Andreas Färber @ 2012-05-15 13:31 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: peter.maydell, qemu-devel, agraf
Am 15.05.2012 11:39, schrieb Fabien Chouteau:
> Do not call cpu_dump_state if logfile is NULL.
And where is log_cpu_state() being called from? Its caller is passing
NULL already then.
Andreas
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
> qemu-log.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-log.h b/qemu-log.h
> index fccfb110..2cd5ffa 100644
> --- a/qemu-log.h
> +++ b/qemu-log.h
> @@ -51,7 +51,12 @@ extern int loglevel;
> /* Special cases: */
>
> /* cpu_dump_state() logging functions: */
> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
> +#define log_cpu_state(env, f) \
> +do { \
> + if (logfile != NULL) { \
> + cpu_dump_state((env), logfile, fprintf, (f)); \
> + } \
> + } while (0)
> #define log_cpu_state_mask(b, env, f) do { \
> if (loglevel & (b)) log_cpu_state((env), (f)); \
> } while (0)
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 13:31 ` Andreas Färber
@ 2012-05-15 16:08 ` Fabien Chouteau
2012-05-15 16:20 ` Peter Maydell
2012-05-16 3:50 ` Andreas Färber
0 siblings, 2 replies; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-15 16:08 UTC (permalink / raw)
To: Andreas Färber; +Cc: peter.maydell, qemu-devel, agraf
On 05/15/2012 03:31 PM, Andreas Färber wrote:
> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>> Do not call cpu_dump_state if logfile is NULL.
>
> And where is log_cpu_state() being called from? Its caller is passing
> NULL already then.
>
No, logfile is a global variable. log_cpu_state() takes only CPUState
and flags parameters.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>> qemu-log.h | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-log.h b/qemu-log.h
>> index fccfb110..2cd5ffa 100644
>> --- a/qemu-log.h
>> +++ b/qemu-log.h
>> @@ -51,7 +51,12 @@ extern int loglevel;
>> /* Special cases: */
>>
>> /* cpu_dump_state() logging functions: */
>> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
>> +#define log_cpu_state(env, f) \
>> +do { \
>> + if (logfile != NULL) { \
>> + cpu_dump_state((env), logfile, fprintf, (f)); \
>> + } \
>> + } while (0)
>> #define log_cpu_state_mask(b, env, f) do { \
>> if (loglevel & (b)) log_cpu_state((env), (f)); \
>> } while (0)
>
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 16:08 ` Fabien Chouteau
@ 2012-05-15 16:20 ` Peter Maydell
2012-05-15 16:45 ` Fabien Chouteau
2012-05-16 3:50 ` Andreas Färber
1 sibling, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2012-05-15 16:20 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: agraf, Andreas Färber, qemu-devel
On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>> Do not call cpu_dump_state if logfile is NULL.
>>
>> And where is log_cpu_state() being called from? Its caller is passing
>> NULL already then.
> No, logfile is a global variable. log_cpu_state() takes only CPUState
> and flags parameters.
The question is which of the following two options we want:
(1) callers should be guarding the calls to log_cpu_state() with
checks for qemu_log_enabled() or qemu_loglevel_mask()
(2) log_cpu_state() does its own check for whether logging is enabled
in the same way that qemu_log() and qemu_log_vprintf() do
At the moment most callers of log_cpu_state() do their own checks
as per (1), but you could make an argument that we should switch
to (2) instead.
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 16:20 ` Peter Maydell
@ 2012-05-15 16:45 ` Fabien Chouteau
2012-05-15 18:04 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-15 16:45 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, agraf, Andreas Färber
On 05/15/2012 06:20 PM, Peter Maydell wrote:
> On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>> Do not call cpu_dump_state if logfile is NULL.
>>>
>>> And where is log_cpu_state() being called from? Its caller is passing
>>> NULL already then.
>
>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>> and flags parameters.
>
> The question is which of the following two options we want:
> (1) callers should be guarding the calls to log_cpu_state() with
> checks for qemu_log_enabled() or qemu_loglevel_mask()
> (2) log_cpu_state() does its own check for whether logging is enabled
> in the same way that qemu_log() and qemu_log_vprintf() do
>
> At the moment most callers of log_cpu_state() do their own checks
> as per (1), but you could make an argument that we should switch
> to (2) instead.
>
I think (2) is better, we do the check in one place and that's it. All
call to log_cpu_state are safe. And as you said, it's already the way
qemu_log and qemu_log_vprintf work.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 16:45 ` Fabien Chouteau
@ 2012-05-15 18:04 ` Peter Maydell
0 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2012-05-15 18:04 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel, agraf, Andreas Färber
On 15 May 2012 17:45, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 05/15/2012 06:20 PM, Peter Maydell wrote:
>> The question is which of the following two options we want:
>> (1) callers should be guarding the calls to log_cpu_state() with
>> checks for qemu_log_enabled() or qemu_loglevel_mask()
>> (2) log_cpu_state() does its own check for whether logging is enabled
>> in the same way that qemu_log() and qemu_log_vprintf() do
>>
>> At the moment most callers of log_cpu_state() do their own checks
>> as per (1), but you could make an argument that we should switch
>> to (2) instead.
>
> I think (2) is better, we do the check in one place and that's it. All
> call to log_cpu_state are safe. And as you said, it's already the way
> qemu_log and qemu_log_vprintf work.
Yeah, it seems reasonable to me. I think your commit message could
be better though, since you're actually kind of changing an API
here, not just fixing a segfault bug.
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-15 16:08 ` Fabien Chouteau
2012-05-15 16:20 ` Peter Maydell
@ 2012-05-16 3:50 ` Andreas Färber
2012-05-16 8:29 ` Fabien Chouteau
1 sibling, 1 reply; 12+ messages in thread
From: Andreas Färber @ 2012-05-16 3:50 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: peter.maydell, qemu-devel, agraf
Am 15.05.2012 18:08, schrieb Fabien Chouteau:
> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>> Do not call cpu_dump_state if logfile is NULL.
>>
>> And where is log_cpu_state() being called from? Its caller is passing
>> NULL already then.
>>
>
> No, logfile is a global variable. log_cpu_state() takes only CPUState
> and flags parameters.
Ah, I see now that f is a different f here, logfile becomes
log_cpu_state()'s f. Unfortunate naming.
Your fix looks OK then but I would recommend turning it into a static
inline function to avoid the line breaks.
Andreas
>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>> ---
>>> qemu-log.h | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-log.h b/qemu-log.h
>>> index fccfb110..2cd5ffa 100644
>>> --- a/qemu-log.h
>>> +++ b/qemu-log.h
>>> @@ -51,7 +51,12 @@ extern int loglevel;
>>> /* Special cases: */
>>>
>>> /* cpu_dump_state() logging functions: */
>>> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
>>> +#define log_cpu_state(env, f) \
>>> +do { \
>>> + if (logfile != NULL) { \
>>> + cpu_dump_state((env), logfile, fprintf, (f)); \
>>> + } \
>>> + } while (0)
>>> #define log_cpu_state_mask(b, env, f) do { \
>>> if (loglevel & (b)) log_cpu_state((env), (f)); \
>>> } while (0)
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-16 3:50 ` Andreas Färber
@ 2012-05-16 8:29 ` Fabien Chouteau
2012-05-16 13:39 ` Fabien Chouteau
0 siblings, 1 reply; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-16 8:29 UTC (permalink / raw)
To: Andreas Färber; +Cc: peter.maydell, qemu-devel, agraf
On 05/16/2012 05:50 AM, Andreas Färber wrote:
> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>> Do not call cpu_dump_state if logfile is NULL.
>>>
>>> And where is log_cpu_state() being called from? Its caller is passing
>>> NULL already then.
>>>
>>
>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>> and flags parameters.
>
> Ah, I see now that f is a different f here, logfile becomes
> log_cpu_state()'s f. Unfortunate naming.
>
> Your fix looks OK then but I would recommend turning it into a static
> inline function to avoid the line breaks.
>
In this case I can rewrite all the macros in qemu-log.h to static inline.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-16 8:29 ` Fabien Chouteau
@ 2012-05-16 13:39 ` Fabien Chouteau
2012-05-23 15:43 ` Fabien Chouteau
0 siblings, 1 reply; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-16 13:39 UTC (permalink / raw)
To: Andreas Färber; +Cc: peter.maydell, qemu-devel, agraf
On 05/16/2012 10:29 AM, Fabien Chouteau wrote:
> On 05/16/2012 05:50 AM, Andreas Färber wrote:
>> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>>> Do not call cpu_dump_state if logfile is NULL.
>>>>
>>>> And where is log_cpu_state() being called from? Its caller is passing
>>>> NULL already then.
>>>>
>>>
>>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>>> and flags parameters.
>>
>> Ah, I see now that f is a different f here, logfile becomes
>> log_cpu_state()'s f. Unfortunate naming.
>>
>> Your fix looks OK then but I would recommend turning it into a static
>> inline function to avoid the line breaks.
>>
>
> In this case I can rewrite all the macros in qemu-log.h to static inline.
>
This is more complex than expected...
1 - GCC rejects inlined variadic functions
2 - Moving from macro to inline implies use of types defined in cpu.h
(target_ulong, CPUArchState...), which I cannot include because
qemu-log.h is used in tools (i.e. without cpu.h).
Conclusion: unless someone volunteer for a massive restructuring of
qemu-log we have to keep the marcro for log_cpu_state.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state
2012-05-16 13:39 ` Fabien Chouteau
@ 2012-05-23 15:43 ` Fabien Chouteau
0 siblings, 0 replies; 12+ messages in thread
From: Fabien Chouteau @ 2012-05-23 15:43 UTC (permalink / raw)
To: Andreas Färber; +Cc: peter.maydell, qemu-devel, agraf
On 05/16/2012 03:39 PM, Fabien Chouteau wrote:
> On 05/16/2012 10:29 AM, Fabien Chouteau wrote:
>> On 05/16/2012 05:50 AM, Andreas Färber wrote:
>>> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>>>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>>>> Do not call cpu_dump_state if logfile is NULL.
>>>>>
>>>>> And where is log_cpu_state() being called from? Its caller is passing
>>>>> NULL already then.
>>>>>
>>>>
>>>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>>>> and flags parameters.
>>>
>>> Ah, I see now that f is a different f here, logfile becomes
>>> log_cpu_state()'s f. Unfortunate naming.
>>>
>>> Your fix looks OK then but I would recommend turning it into a static
>>> inline function to avoid the line breaks.
>>>
>>
>> In this case I can rewrite all the macros in qemu-log.h to static inline.
>>
>
> This is more complex than expected...
>
> 1 - GCC rejects inlined variadic functions
>
> 2 - Moving from macro to inline implies use of types defined in cpu.h
> (target_ulong, CPUArchState...), which I cannot include because
> qemu-log.h is used in tools (i.e. without cpu.h).
>
> Conclusion: unless someone volunteer for a massive restructuring of
> qemu-log we have to keep the marcro for log_cpu_state.
>
So, are we good with the second patch?
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-05-23 15:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-14 14:46 [Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state Fabien Chouteau
2012-05-14 15:39 ` Peter Maydell
2012-05-15 9:39 ` [Qemu-devel] [PATCH] " Fabien Chouteau
2012-05-15 13:31 ` Andreas Färber
2012-05-15 16:08 ` Fabien Chouteau
2012-05-15 16:20 ` Peter Maydell
2012-05-15 16:45 ` Fabien Chouteau
2012-05-15 18:04 ` Peter Maydell
2012-05-16 3:50 ` Andreas Färber
2012-05-16 8:29 ` Fabien Chouteau
2012-05-16 13:39 ` Fabien Chouteau
2012-05-23 15:43 ` Fabien Chouteau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.