On Sun, Jul 26, 2020 at 04:59:16PM +0000, Matthieu Bucchianeri wrote:
> Hello Balaton,
>
> Thank you for your thorough review! See my response below.
>
> > > static inline void gen_evmwsmiaa(DisasContext *ctx) {
> > > - TCGv_i64 acc = tcg_temp_new_i64();
> > > - TCGv_i64 tmp = tcg_temp_new_i64();
> > > + TCGv_i64 acc;
> > > + TCGv_i64 tmp;
> > > +
> > > + if (unlikely(!ctx->spe_enabled)) {
> > > + gen_exception(ctx, POWERPC_EXCP_SPEU);
> > > + return;
> > > + }
> >
> > Isn't this missing here:
> >
> > acc = tcg_temp_new_i64();
> > tmp = tcg_temp_new_i64();
> >
> > You've removed allocating temps but this hunk does not add it back after the
> > exception unlike others in this patch.
>
> I should have probably mentioned somewhere that this patch also
> fixes a resource leak in that function. It is not very obvious
> when looking at it as a patch, but if you take a look at the
> original code, you will see that I removed these allocations on
> purpose:
>
>
>https://github.com/qemu/qemu/blob/d577dbaac7553767232faabb6a3e291aebd348ae/target/ppc/translate/spe-impl.inc.c#L532
Ok, can you please split the memory leak fix into a separate patch to
make this easier to review.
>
> > static inline void gen_evmwsmiaa(DisasContext *ctx)
> > {
> > TCGv_i64 acc = tcg_temp_new_i64();
> > TCGv_i64 tmp = tcg_temp_new_i64();
> >
> > gen_evmwsmi(ctx); /* rD := rA * rB */
> >
> > acc = tcg_temp_new_i64();
> > tmp = tcg_temp_new_i64();
>
> I apologize for not making this any more clear in my description.
>
> Let me know if this looks correct now, with the full context in mind.
>
> Thanks.
>
> LeoStella, LLC
> A joint venture of Thales Alenia Space and Spaceflight Industries
>
> 12501 E Marginal Way S • Tukwila, WA 98168
>
> Proprietary Document: This document may contain trade secrets or otherwise
> proprietary and confidential information owned by LeoStella LLC. It is
> intended only for the individual addressee and may not be copied,
> distributed, or otherwise disclosed without LeoStella LLC express prior
> written authorization.
> Export Controlled: This document may contain technical data whose export is
> restricted by the Arms Export Control Act (Title 22, U.S.C., Sec 2751 et
> seq.) or the Export Administration Act of 1979, as amended, Title 50,U.S.C.,
> app 2401 et seq. Violation of these export laws are subject to severe
> criminal penalties. Recipient shall not export, re-export, or otherwise
> transfer or share this document to any foreign person (as defined by U.S.
> export laws) without advance written authorization from LeoStella LLC.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
