On 7/14/22 13:17, Matheus K. Ferst wrote:
On 14/07/2022 10:35, Daniel Henrique Barboza wrote:
On 7/13/22 15:38, Matheus Ferst wrote:
When using "-machine none", env->tb_env is not allocated, causing the
segmentation fault reported in issue #85 (launchpad bug #811683). To
avoid this problem, check if the pointer != NULL before calling the
methods to print TBU/TBL/DECR.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/85
Signed-off-by: Matheus Ferst <matheus.fe...@eldorado.org.br>
---
v2:
- Added checks in monitor_get_decr, monitor_get_tbu, and monitor_get_tbl.
- Link to v1:
https://lists.gnu.org/archive/html/qemu-ppc/2022-07/msg00173.html
---
target/ppc/cpu_init.c | 16 ++++++++--------
target/ppc/monitor.c | 9 +++++++++
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 86ad28466a..7e96baac9f 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7476,18 +7476,18 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int
flags)
"%08x iidx %d didx %d\n",
env->msr, env->spr[SPR_HID0], env->hflags,
cpu_mmu_index(env, true), cpu_mmu_index(env, false));
-#if !defined(NO_TIMER_DUMP)
Why did you remove the NO_TIMER_DUMP check? Is it redundant with the env->tb_env
check?
This is the only reference to this macro since it was added in d9bce9d99f46. I
suppose it was manually defined, but the only discussion[1] I could find around
this patch doesn't mention it. I don't see any other reason to define it other
than avoiding segfaults in machines that don't allocate env_tb, but we can keep
it if you prefer.
Thanks for the explanation. And nah, no need to keep it around since you're
already gating the code by checking env->tb_env.
Daniel
- qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
+ if (env->tb_env) {
+ qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
#if !defined(CONFIG_USER_ONLY)
- " DECR " TARGET_FMT_lu
+ " DECR " TARGET_FMT_lu
#endif
- "\n",
- cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
+ "\n",
+ cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
#if !defined(CONFIG_USER_ONLY)
- , cpu_ppc_load_decr(env)
-#endif
- );
+ , cpu_ppc_load_decr(env)
#endif
+ );
+ }
Not really a problem with your patch, but since you're changing this code, can
you please cleanse it from evil? I mean, look at this:
if (env->tb_env) {
qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
#if !defined(CONFIG_USER_ONLY)
" DECR " TARGET_FMT_lu
#endif
"\n",
cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
#if !defined(CONFIG_USER_ONLY)
, cpu_ppc_load_decr(env)
#endif
);
}
2 ifdef macros in the middle of qemu_fprintf() params? With one line starting
with a ', '? Why are we trading sanity for 3 lines of code repetition?
We can --at least-- do something like this:
if (env->tb_env) {
#if !defined(CONFIG_USER_ONLY)
qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
" DECR " TARGET_FMT_lu
"\n",
cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env),
cpu_ppc_load_decr(env));
#else
qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
"\n",
cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env));
#endif
}
Thanks,
Daniel
Sure, I'll change that in v3.
[1] https://lists.gnu.org/archive/html/qemu-devel/2007-03/msg00239.html
Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
