Re: [PATCH 07/10] tcg: implement bulletproof JIT

[BALATON Zoltan]

On Wed, 14 Oct 2020, Joelle van Dyne wrote:
> Much of the code that uses the macro is like the following (from
> aarch64/tcg-include.inc.c)
>
>        *TCG_CODE_PTR_RW(s, code_ptr) =
>            deposit32(*TCG_CODE_PTR_RW(s, code_ptr), 0, 26, offset);
>
> Before the change, it was just *code_ptr. I'm saying the alternative
> was to have to write "tcg_insn_unit *rw_code_ptr = (tcg_insn_unit
> *)TCG_CODE_PTR_RW(s, code_ptr)" everywhere or else inline cast it.
> Whereas making it return tcg_insn_unit * means only three instances of
> casting to uint8_t *. Using void * means casting at every instance.

It's not C++ so void * does not need to be cast when assigned to some 
other pointer.

> Not opposed to using an inline function over a macro but "inline" is
> not ANSI C. Not sure what this project thinks about that considering
> the style checker prohibits C99 style comments. Personally I don't
> care much.

QEMU has some compiler dependencies, I think only some recent versions of 
gcc and clang are supported and static inline is used elsewhere in the 
code. Richard is an expert on TCG so you can take his advice.

Regards,
BALATON Zoltan

> -j
>
> On Wed, Oct 14, 2020 at 1:35 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > On 10/14/20 9:03 AM, Joelle van Dyne wrote:
> > > > > static int encode_search(TranslationBlock *tb, uint8_t *block)
> > > > > {
> > > > > -    uint8_t *highwater = tcg_ctx->code_gen_highwater;
> > > > > -    uint8_t *p = block;
> > > > > +    uint8_t *highwater;
> > > > > +    uint8_t *p;
> > > > >     int i, j, n;
> > > > >
> > > > > +    highwater = (uint8_t *)TCG_CODE_PTR_RW(tcg_ctx,
> > > > > +                                           tcg_ctx->code_gen_highwater);
> > > > > +    p = (uint8_t *)TCG_CODE_PTR_RW(tcg_ctx, block);
> > > >
> > > > Why do you need explicit casts here? Can this be avoided by using
> > > > appropriate type or within the macro (I haven't checked this at all just
> > > > dislike casts as they can hide problems otherwise caught by the compiler).
> > > There's the choice between tcg_insn_unit * and uint8_t *. Since it's
> > > used much more widely in tcg-target.inc.c, it seemed like
> > > tcg_insn_unit * was a better choice.
> >
> > False choice.  This is why tcg_ctx->code_gen_highwater is void*.
> >
> > > > > +#if defined(CONFIG_IOS_JIT)
> > > > > +# define TCG_CODE_PTR_RW(s, code_ptr) \
> > > > > +    (tcg_insn_unit *)((uintptr_t)(code_ptr) + (s)->code_rw_mirror_diff)
> >
> > Better as
> >
> > static inline void *tcg_code_ptr_rw(TCGContext *s, void *rx)
> > {
> > #ifdef CONFIG_IOS_JIT
> >     return rx + s->code_rw_mirror_diff;
> > #else
> >     return rx;
> > #endif
> > }
> >
> >
> > r~