Re: [PATCH] target/ppc: fix unreachable code in do_ldst_quad()

[Richard Henderson]

On 7/20/22 19:27, Daniel Henrique Barboza wrote:

Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
check privilege level") turned the following code unreachable:

if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
 /* lq and stq were privileged prior to V. 2.07 */
 REQUIRE_SV(ctx);


 CID 1490757:  Control flow issues  (UNREACHABLE)
 This code cannot be reached: "if (ctx->le_mode) {

 if (ctx->le_mode) {
 gen_align_no_le(ctx);
 return true;
 }
}

This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
always result in a 'return true' statement.


I think adding ifdefs isn't fantastic.  This isn't actually fix a bug, so we *could* just 
mark this as ignore in Coverity.


If you wanted to clean this up, remove the implicit control flow from REQUIRE_* and turn 
the macros into pure predicates, so that you get


if (REQUIRE_SV(ctx)) {
return true;
}
if (ctx->le_mode) {
...
}


r~

Re: [PATCH] target/ppc: fix unreachable code in do_ldst_quad()

[Víctor Colombo]

On 20/07/2022 10:57, Daniel Henrique Barboza wrote:

Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
check privilege level") turned the following code unreachable:

if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
 /* lq and stq were privileged prior to V. 2.07 */
 REQUIRE_SV(ctx);


 CID 1490757:  Control flow issues  (UNREACHABLE)
 This code cannot be reached: "if (ctx->le_mode) {

 if (ctx->le_mode) {
 gen_align_no_le(ctx);
 return true;
 }
}

This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
always result in a 'return true' statement.

Fix it by using "#if !defined(CONFIG_USER_ONLY)" to fold the code that
shouldn't be there if we're running in a non-privileged state. This is
also how the REQUIRE_SV() macro is being used in
storage-ctrl-impl.c.inc.

Fixes: Coverity CID 1490757
Fixes: fc34e81acd51 ("target/ppc: add macros to check privilege level")
Cc: Matheus Ferst 
Signed-off-by: Daniel Henrique Barboza 
---
  target/ppc/translate/fixedpoint-impl.c.inc | 4 
  1 file changed, 4 insertions(+)

diff --git a/target/ppc/translate/fixedpoint-impl.c.inc 
b/target/ppc/translate/fixedpoint-impl.c.inc
index db14d3bebc..4a32fac4f3 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -82,10 +82,14 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool 
store, bool prefixed)
  /* lq and stq were privileged prior to V. 2.07 */
  REQUIRE_SV(ctx);

+#if !defined(CONFIG_USER_ONLY)
  if (ctx->le_mode) {
  gen_align_no_le(ctx);
  return true;
  }
+#else
+qemu_build_not_reached();


nit: I think the indentation here is off by 1 level (missing 4 spaces)?


+#endif
  }

  if (!store && unlikely(a->ra == a->rt)) {
--
2.36.1



Reviewed-by: Víctor Colombo 

--
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer 

[PATCH] target/ppc: fix unreachable code in do_ldst_quad()

[Daniel Henrique Barboza]

Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
check privilege level") turned the following code unreachable:

if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
/* lq and stq were privileged prior to V. 2.07 */
REQUIRE_SV(ctx);

>>> CID 1490757:  Control flow issues  (UNREACHABLE)
>>> This code cannot be reached: "if (ctx->le_mode) {
if (ctx->le_mode) {
gen_align_no_le(ctx);
return true;
}
}

This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
always result in a 'return true' statement.

Fix it by using "#if !defined(CONFIG_USER_ONLY)" to fold the code that
shouldn't be there if we're running in a non-privileged state. This is
also how the REQUIRE_SV() macro is being used in
storage-ctrl-impl.c.inc.

Fixes: Coverity CID 1490757
Fixes: fc34e81acd51 ("target/ppc: add macros to check privilege level")
Cc: Matheus Ferst 
Signed-off-by: Daniel Henrique Barboza 
---
 target/ppc/translate/fixedpoint-impl.c.inc | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/ppc/translate/fixedpoint-impl.c.inc 
b/target/ppc/translate/fixedpoint-impl.c.inc
index db14d3bebc..4a32fac4f3 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -82,10 +82,14 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool 
store, bool prefixed)
 /* lq and stq were privileged prior to V. 2.07 */
 REQUIRE_SV(ctx);
 
+#if !defined(CONFIG_USER_ONLY)
 if (ctx->le_mode) {
 gen_align_no_le(ctx);
 return true;
 }
+#else
+qemu_build_not_reached();
+#endif
 }
 
 if (!store && unlikely(a->ra == a->rt)) {
-- 
2.36.1