qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DEC


From: Daniel Henrique Barboza
Subject: Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR
Date: Thu, 14 Jul 2022 13:30:47 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0



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.ferst@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>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]