| Message ID | 20220725202122.1810709-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: fix unreachable code in do_ldst_quad() | expand |
On 25/07/2022 17:21, 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. In fact, all REQUIRE_* > macros for target/ppc/translate.c behave the same way: if a condition > isn't met, an exception is generated and a 'return' statement is issued. > > The difference is that all other callers are using it in insns that are > not implemented in user mode. do_ldst_quad(), on the other hand, is user > mode compatible. > > Fixes include wrapping these lines in "if !defined(CONFIG_USER_MODE)", > making it explicit that these lines are not user mode anymore. Another > fix would be, for example, to change REQUIRE_SV() to not issue a > 'return' and check if we're running in privileged mode or not by hand, > but this would change all other callers of the macro that are using it > in an adequate manner. > > The code that was in place before fc34e81acd51 was good enough, so let's > go back to that: open code the ctx->pr condition and fire the exception > if we're not privileged. The difference from the code back then to what > we're doing now is an 'unlikely' compiler hint to ctx->pr and the use of > gen_priv_opc() instead of gen_priv_exception(). > > Fixes: Coverity CID 1490757 > Cc: Matheus Ferst <matheus.ferst@eldorado.org.br> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > target/ppc/translate/fixedpoint-impl.c.inc | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc > index db14d3bebc..a3ade4fe2b 100644 > --- a/target/ppc/translate/fixedpoint-impl.c.inc > +++ b/target/ppc/translate/fixedpoint-impl.c.inc > @@ -79,8 +79,11 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed) > REQUIRE_INSNS_FLAGS(ctx, 64BX); > > if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) { > - /* lq and stq were privileged prior to V. 2.07 */ > - REQUIRE_SV(ctx); > + if (unlikely(ctx->pr)) { > + /* lq and stq were privileged prior to V. 2.07 */ > + gen_priv_opc(ctx); > + return true; > + } > > if (ctx->le_mode) { > gen_align_no_le(ctx); > -- > 2.36.1 > Since the remaining code in this branch is dead code in user-mode, I'd personally prefer the v1 approach, but the difference is unlikely to have any meaningful impact, so either way is good. Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br> 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>
On Mon, 25 Jul 2022 at 21:24, Daniel Henrique Barboza <danielhb413@gmail.com> 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. In fact, all REQUIRE_* > macros for target/ppc/translate.c behave the same way: if a condition > isn't met, an exception is generated and a 'return' statement is issued. > > The difference is that all other callers are using it in insns that are > not implemented in user mode. do_ldst_quad(), on the other hand, is user > mode compatible. This is a Coverity false positive, and I'd already marked it that way in the Coverity UI back on the 20th. Coverity gets confused sometimes by ifdeffery. So you don't need this patch, unless you think the code is genuinely better (more readable to humans, etc) this way. thanks -- PMM
On 8/4/22 15:05, Peter Maydell wrote: > On Mon, 25 Jul 2022 at 21:24, Daniel Henrique Barboza > <danielhb413@gmail.com> 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. In fact, all REQUIRE_* >> macros for target/ppc/translate.c behave the same way: if a condition >> isn't met, an exception is generated and a 'return' statement is issued. >> >> The difference is that all other callers are using it in insns that are >> not implemented in user mode. do_ldst_quad(), on the other hand, is user >> mode compatible. > > This is a Coverity false positive, and I'd already marked it that way > in the Coverity UI back on the 20th. Coverity gets confused sometimes > by ifdeffery. > > So you don't need this patch, unless you think the code is genuinely > better (more readable to humans, etc) this way. The idea was to make Coverity happy. If Coverity is already happy then let's drop this patch - there's no particular improvement made here that justifies it. Thanks, Daniel > > thanks > -- PMM
diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc index db14d3bebc..a3ade4fe2b 100644 --- a/target/ppc/translate/fixedpoint-impl.c.inc +++ b/target/ppc/translate/fixedpoint-impl.c.inc @@ -79,8 +79,11 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed) REQUIRE_INSNS_FLAGS(ctx, 64BX); if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) { - /* lq and stq were privileged prior to V. 2.07 */ - REQUIRE_SV(ctx); + if (unlikely(ctx->pr)) { + /* lq and stq were privileged prior to V. 2.07 */ + gen_priv_opc(ctx); + return true; + } if (ctx->le_mode) { gen_align_no_le(ctx);
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. In fact, all REQUIRE_* macros for target/ppc/translate.c behave the same way: if a condition isn't met, an exception is generated and a 'return' statement is issued. The difference is that all other callers are using it in insns that are not implemented in user mode. do_ldst_quad(), on the other hand, is user mode compatible. Fixes include wrapping these lines in "if !defined(CONFIG_USER_MODE)", making it explicit that these lines are not user mode anymore. Another fix would be, for example, to change REQUIRE_SV() to not issue a 'return' and check if we're running in privileged mode or not by hand, but this would change all other callers of the macro that are using it in an adequate manner. The code that was in place before fc34e81acd51 was good enough, so let's go back to that: open code the ctx->pr condition and fire the exception if we're not privileged. The difference from the code back then to what we're doing now is an 'unlikely' compiler hint to ctx->pr and the use of gen_priv_opc() instead of gen_priv_exception(). Fixes: Coverity CID 1490757 Cc: Matheus Ferst <matheus.ferst@eldorado.org.br> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- target/ppc/translate/fixedpoint-impl.c.inc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)