On Thu, Jan 22, 2026 at 4:34 PM Marco Liebel <[email protected]> wrote:
> The pkt variable inside DisasContext is of type Packet * and gets > assigned to a local variable in decode_and_translate_packet. Right now > there seems to be no problem with it but future changes to e.g. > hexagon_tr_transalte_packet are potentially dangerous if pkt is accessed > after the local variable goes out of scope. > > Since packets are being translated one at a time, the type of pkt can be > changed to just Packet to avoid risk of having a dangling pointer. > > Signed-off-by: Marco Liebel <[email protected]> > --- > Reviewed-by: Brian Cain <[email protected]> > target/hexagon/gen_tcg.h | 2 +- > target/hexagon/macros.h | 6 +- > target/hexagon/translate.h | 2 +- > target/hexagon/decode.c | 8 +-- > target/hexagon/genptr.c | 14 ++-- > target/hexagon/translate.c | 111 ++++++++++++++------------------ > target/hexagon/gen_tcg_funcs.py | 2 +- > target/hexagon/hex_common.py | 4 +- > 8 files changed, 65 insertions(+), 84 deletions(-) > > diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h > index 7b96dab918..54562e3ef3 100644 > --- a/target/hexagon/gen_tcg.h > +++ b/target/hexagon/gen_tcg.h > @@ -1363,7 +1363,7 @@ > #define fGEN_TCG_J2_trap0(SHORTCODE) \ > do { \ > uiV = uiV; \ > - tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->pkt->pc); \ > + tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->pkt.pc); \ > TCGv excp = tcg_constant_tl(HEX_EVENT_TRAP0); \ > gen_helper_raise_exception(tcg_env, excp); \ > } while (0) > diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h > index 6c2862a232..eebfe1e5ed 100644 > --- a/target/hexagon/macros.h > +++ b/target/hexagon/macros.h > @@ -83,7 +83,7 @@ > */ > #define CHECK_NOSHUF(VA, SIZE) \ > do { \ > - if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \ > + if (insn->slot == 0 && ctx->pkt.pkt_has_scalar_store_s1) { \ > probe_noshuf_load(VA, SIZE, ctx->mem_idx); \ > process_store(ctx, 1); \ > } \ > @@ -94,11 +94,11 @@ > TCGLabel *noshuf_label = gen_new_label(); \ > tcg_gen_brcondi_tl(TCG_COND_EQ, PRED, 0, noshuf_label); \ > GET_EA; \ > - if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \ > + if (insn->slot == 0 && ctx->pkt.pkt_has_scalar_store_s1) { \ > probe_noshuf_load(EA, SIZE, ctx->mem_idx); \ > } \ > gen_set_label(noshuf_label); \ > - if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \ > + if (insn->slot == 0 && ctx->pkt.pkt_has_scalar_store_s1) { \ > process_store(ctx, 1); \ > } \ > } while (0) > diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h > index a0102b6cbd..ffbfc3ac0e 100644 > --- a/target/hexagon/translate.h > +++ b/target/hexagon/translate.h > @@ -28,7 +28,7 @@ > > typedef struct DisasContext { > DisasContextBase base; > - Packet *pkt; > + Packet pkt; > Insn *insn; > uint32_t next_PC; > uint32_t mem_idx; > diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c > index b5ece60450..c2516d927d 100644 > --- a/target/hexagon/decode.c > +++ b/target/hexagon/decode.c > @@ -748,14 +748,12 @@ int disassemble_hexagon(uint32_t *words, int nwords, > bfd_vma pc, > GString *buf) > { > DisasContext ctx; > - Packet pkt; > > memset(&ctx, 0, sizeof(DisasContext)); > - ctx.pkt = &pkt; > > - if (decode_packet(&ctx, nwords, words, &pkt, true) > 0) { > - snprint_a_pkt_disas(buf, &pkt, words, pc); > - return pkt.encod_pkt_size_in_bytes; > + if (decode_packet(&ctx, nwords, words, &ctx.pkt, true) > 0) { > + snprint_a_pkt_disas(buf, &ctx.pkt, words, pc); > + return ctx.pkt.encod_pkt_size_in_bytes; > } else { > g_string_assign(buf, "<invalid>"); > return 0; > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c > index 36968549d5..160edfe756 100644 > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -382,7 +382,7 @@ static inline void gen_store_conditional8(DisasContext > *ctx, > static TCGv gen_slotval(DisasContext *ctx) > { > int slotval = > - (ctx->pkt->pkt_has_scalar_store_s1 & 1) | (ctx->insn->slot << 1); > + (ctx->pkt.pkt_has_scalar_store_s1 & 1) | (ctx->insn->slot << 1); > return tcg_constant_tl(slotval); > } > #endif > @@ -458,7 +458,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, > TCGv addr, > tcg_gen_brcondi_tl(cond, pred, 0, pred_false); > } > > - if (ctx->pkt->pkt_has_multi_cof) { > + if (ctx->pkt.pkt_has_multi_cof) { > /* If there are multiple branches in a packet, ignore the second > one */ > tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC], > ctx->branch_taken, tcg_constant_tl(0), > @@ -476,8 +476,8 @@ static void gen_write_new_pc_addr(DisasContext *ctx, > TCGv addr, > static void gen_write_new_pc_pcrel(DisasContext *ctx, int pc_off, > TCGCond cond, TCGv pred) > { > - target_ulong dest = ctx->pkt->pc + pc_off; > - if (ctx->pkt->pkt_has_multi_cof) { > + target_ulong dest = ctx->pkt.pc + pc_off; > + if (ctx->pkt.pkt_has_multi_cof) { > gen_write_new_pc_addr(ctx, tcg_constant_tl(dest), cond, pred); > } else { > /* Defer this jump to the end of the TB */ > @@ -528,7 +528,7 @@ static inline void gen_loop0r(DisasContext *ctx, TCGv > RsV, int riV) > fIMMEXT(riV); > fPCALIGN(riV); > tcg_gen_mov_tl(get_result_gpr(ctx, HEX_REG_LC0), RsV); > - tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA0), ctx->pkt->pc + riV); > + tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA0), ctx->pkt.pc + riV); > gen_set_usr_fieldi(ctx, USR_LPCFG, 0); > } > > @@ -542,7 +542,7 @@ static inline void gen_loop1r(DisasContext *ctx, TCGv > RsV, int riV) > fIMMEXT(riV); > fPCALIGN(riV); > tcg_gen_mov_tl(get_result_gpr(ctx, HEX_REG_LC1), RsV); > - tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA1), ctx->pkt->pc + riV); > + tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA1), ctx->pkt.pc + riV); > } > > static void gen_loop1i(DisasContext *ctx, int count, int riV) > @@ -555,7 +555,7 @@ static void gen_ploopNsr(DisasContext *ctx, int N, > TCGv RsV, int riV) > fIMMEXT(riV); > fPCALIGN(riV); > tcg_gen_mov_tl(get_result_gpr(ctx, HEX_REG_LC0), RsV); > - tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA0), ctx->pkt->pc + riV); > + tcg_gen_movi_tl(get_result_gpr(ctx, HEX_REG_SA0), ctx->pkt.pc + riV); > gen_set_usr_fieldi(ctx, USR_LPCFG, N); > gen_pred_write(ctx, 3, tcg_constant_tl(0)); > } > diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c > index e88e19cc1a..b06ed154f3 100644 > --- a/target/hexagon/translate.c > +++ b/target/hexagon/translate.c > @@ -156,8 +156,6 @@ static void gen_goto_tb(DisasContext *ctx, unsigned > tb_slot_idx, > > static void gen_end_tb(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - > gen_exec_counters(ctx); > > if (ctx->branch_cond != TCG_COND_NEVER) { > @@ -171,7 +169,7 @@ static void gen_end_tb(DisasContext *ctx) > gen_goto_tb(ctx, 0, ctx->branch_dest, true); > } > } else if (ctx->is_tight_loop && > - pkt->insn[pkt->num_insns - 1].opcode == J2_endloop0) { > + ctx->pkt.insn[ctx->pkt.num_insns - 1].opcode == > J2_endloop0) { > /* > * When we're in a tight loop, we defer the endloop0 processing > * to take advantage of direct block chaining > @@ -252,11 +250,9 @@ static bool need_slot_cancelled(Packet *pkt) > > static bool need_next_PC(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - > /* Check for conditional control flow or HW loop end */ > - for (int i = 0; i < pkt->num_insns; i++) { > - uint16_t opcode = pkt->insn[i].opcode; > + for (int i = 0; i < ctx->pkt.num_insns; i++) { > + uint16_t opcode = ctx->pkt.insn[i].opcode; > if (GET_ATTRIB(opcode, A_CONDEXEC) && GET_ATTRIB(opcode, A_COF)) { > return true; > } > @@ -339,8 +335,6 @@ static bool pkt_raises_exception(Packet *pkt) > > static bool need_commit(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - > /* > * If the short-circuit property is set to false, we'll always do the > commit > */ > @@ -348,7 +342,7 @@ static bool need_commit(DisasContext *ctx) > return true; > } > > - if (pkt_raises_exception(pkt)) { > + if (pkt_raises_exception(&ctx->pkt)) { > return true; > } > > @@ -395,11 +389,10 @@ static void mark_implicit_writes(DisasContext *ctx) > > static void analyze_packet(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > ctx->read_after_write = false; > ctx->has_hvx_overlap = false; > - for (int i = 0; i < pkt->num_insns; i++) { > - Insn *insn = &pkt->insn[i]; > + for (int i = 0; i < ctx->pkt.num_insns; i++) { > + Insn *insn = &ctx->pkt.insn[i]; > ctx->insn = insn; > if (opcode_analyze[insn->opcode]) { > opcode_analyze[insn->opcode](ctx); > @@ -411,8 +404,7 @@ static void analyze_packet(DisasContext *ctx) > > static void gen_start_packet(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - target_ulong next_PC = ctx->base.pc_next + > pkt->encod_pkt_size_in_bytes; > + target_ulong next_PC = ctx->base.pc_next + > ctx->pkt.encod_pkt_size_in_bytes; > int i; > > /* Clear out the disassembly context */ > @@ -454,13 +446,13 @@ static void gen_start_packet(DisasContext *ctx) > bitmap_zero(ctx->pregs_written, NUM_PREGS); > > /* Initialize the runtime state for packet semantics */ > - if (need_slot_cancelled(pkt)) { > + if (need_slot_cancelled(&ctx->pkt)) { > tcg_gen_movi_tl(hex_slot_cancelled, 0); > } > ctx->branch_taken = NULL; > - if (pkt->pkt_has_cof) { > + if (ctx->pkt.pkt_has_cof) { > ctx->branch_taken = tcg_temp_new(); > - if (pkt->pkt_has_multi_cof) { > + if (ctx->pkt.pkt_has_multi_cof) { > tcg_gen_movi_tl(ctx->branch_taken, 0); > } > if (need_next_PC(ctx)) { > @@ -489,7 +481,7 @@ static void gen_start_packet(DisasContext *ctx) > * Preload the predicated pred registers into > ctx->new_pred_value[pred_num] > * Only endloop instructions conditionally write to pred registers > */ > - if (ctx->need_commit && pkt->pkt_has_endloop) { > + if (ctx->need_commit && ctx->pkt.pkt_has_endloop) { > for (i = 0; i < ctx->preg_log_idx; i++) { > int pred_num = ctx->preg_log[i]; > ctx->new_pred_value[pred_num] = tcg_temp_new(); > @@ -528,13 +520,11 @@ static void gen_start_packet(DisasContext *ctx) > > bool is_gather_store_insn(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - Insn *insn = ctx->insn; > - if (GET_ATTRIB(insn->opcode, A_CVI_NEW) && > - insn->new_value_producer_slot == 1) { > + if (GET_ATTRIB(ctx->insn->opcode, A_CVI_NEW) && > + ctx->insn->new_value_producer_slot == 1) { > /* Look for gather instruction */ > - for (int i = 0; i < pkt->num_insns; i++) { > - Insn *in = &pkt->insn[i]; > + for (int i = 0; i < ctx->pkt.num_insns; i++) { > + Insn *in = &ctx->pkt.insn[i]; > if (GET_ATTRIB(in->opcode, A_CVI_GATHER) && in->slot == 1) { > return true; > } > @@ -637,7 +627,7 @@ static bool slot_is_predicated(Packet *pkt, int > slot_num) > > void process_store(DisasContext *ctx, int slot_num) > { > - bool is_predicated = slot_is_predicated(ctx->pkt, slot_num); > + bool is_predicated = slot_is_predicated(&ctx->pkt, slot_num); > TCGLabel *label_end = NULL; > > /* > @@ -714,13 +704,12 @@ static void process_store_log(DisasContext *ctx) > * slot 1 and then slot 0. This will be important when > * the memory accesses overlap. > */ > - Packet *pkt = ctx->pkt; > - if (pkt->pkt_has_scalar_store_s1) { > - g_assert(!pkt->pkt_has_dczeroa); > + if (ctx->pkt.pkt_has_scalar_store_s1) { > + g_assert(!ctx->pkt.pkt_has_dczeroa); > process_store(ctx, 1); > } > - if (pkt->pkt_has_scalar_store_s0) { > - g_assert(!pkt->pkt_has_dczeroa); > + if (ctx->pkt.pkt_has_scalar_store_s0) { > + g_assert(!ctx->pkt.pkt_has_dczeroa); > process_store(ctx, 0); > } > } > @@ -728,7 +717,7 @@ static void process_store_log(DisasContext *ctx) > /* Zero out a 32-bit cache line */ > static void process_dczeroa(DisasContext *ctx) > { > - if (ctx->pkt->pkt_has_dczeroa) { > + if (ctx->pkt.pkt_has_dczeroa) { > /* Store 32 bytes of zero starting at (addr & ~0x1f) */ > TCGv addr = tcg_temp_new(); > TCGv_i64 zero = tcg_constant_i64(0); > @@ -762,7 +751,7 @@ static void gen_commit_hvx(DisasContext *ctx) > > /* Early exit if not needed */ > if (!ctx->need_commit) { > - g_assert(!pkt_has_hvx_store(ctx->pkt)); > + g_assert(!pkt_has_hvx_store(&ctx->pkt)); > return; > } > > @@ -796,25 +785,23 @@ static void gen_commit_hvx(DisasContext *ctx) > tcg_gen_gvec_mov(MO_64, dstoff, srcoff, size, size); > } > > - if (pkt_has_hvx_store(ctx->pkt)) { > + if (pkt_has_hvx_store(&ctx->pkt)) { > gen_helper_commit_hvx_stores(tcg_env); > } > } > > static void update_exec_counters(DisasContext *ctx) > { > - Packet *pkt = ctx->pkt; > - int num_insns = pkt->num_insns; > int num_real_insns = 0; > int num_hvx_insns = 0; > > - for (int i = 0; i < num_insns; i++) { > - if (!pkt->insn[i].is_endloop && > - !pkt->insn[i].part1 && > - !GET_ATTRIB(pkt->insn[i].opcode, A_IT_NOP)) { > + for (int i = 0; i < ctx->pkt.num_insns; i++) { > + if (!ctx->pkt.insn[i].is_endloop && > + !ctx->pkt.insn[i].part1 && > + !GET_ATTRIB(ctx->pkt.insn[i].opcode, A_IT_NOP)) { > num_real_insns++; > } > - if (GET_ATTRIB(pkt->insn[i].opcode, A_CVI)) { > + if (GET_ATTRIB(ctx->pkt.insn[i].opcode, A_CVI)) { > num_hvx_insns++; > } > } > @@ -843,12 +830,11 @@ static void gen_commit_packet(DisasContext *ctx) > * store. Therefore, we call process_store_log before anything else > * involved in committing the packet. > */ > - Packet *pkt = ctx->pkt; > - bool has_store_s0 = pkt->pkt_has_scalar_store_s0; > + bool has_store_s0 = ctx->pkt.pkt_has_scalar_store_s0; > bool has_store_s1 = > - (pkt->pkt_has_scalar_store_s1 && !ctx->s1_store_processed); > - bool has_hvx_store = pkt_has_hvx_store(pkt); > - if (pkt->pkt_has_dczeroa) { > + (ctx->pkt.pkt_has_scalar_store_s1 && !ctx->s1_store_processed); > + bool has_hvx_store = pkt_has_hvx_store(&ctx->pkt); > + if (ctx->pkt.pkt_has_dczeroa) { > /* > * The dczeroa will be the store in slot 0, check that we don't > have > * a store in slot 1 or an HVX store. > @@ -875,12 +861,11 @@ static void gen_commit_packet(DisasContext *ctx) > FIELD_DP32(mask, PROBE_PKT_SCALAR_HVX_STORES, > HAS_HVX_STORES, 1); > } > - if (has_store_s0 && slot_is_predicated(pkt, 0)) { > - mask = > - FIELD_DP32(mask, PROBE_PKT_SCALAR_HVX_STORES, > - S0_IS_PRED, 1); > + if (has_store_s0 && slot_is_predicated(&ctx->pkt, 0)) { > + mask = FIELD_DP32(mask, PROBE_PKT_SCALAR_HVX_STORES, > S0_IS_PRED, > + 1); > } > - if (has_store_s1 && slot_is_predicated(pkt, 1)) { > + if (has_store_s1 && slot_is_predicated(&ctx->pkt, 1)) { > mask = > FIELD_DP32(mask, PROBE_PKT_SCALAR_HVX_STORES, > S1_IS_PRED, 1); > @@ -898,7 +883,7 @@ static void gen_commit_packet(DisasContext *ctx) > int args = 0; > args = > FIELD_DP32(args, PROBE_PKT_SCALAR_STORE_S0, MMU_IDX, > ctx->mem_idx); > - if (slot_is_predicated(pkt, 0)) { > + if (slot_is_predicated(&ctx->pkt, 0)) { > args = > FIELD_DP32(args, PROBE_PKT_SCALAR_STORE_S0, > IS_PREDICATED, 1); > } > @@ -910,18 +895,18 @@ static void gen_commit_packet(DisasContext *ctx) > > gen_reg_writes(ctx); > gen_pred_writes(ctx); > - if (pkt->pkt_has_hvx) { > + if (ctx->pkt.pkt_has_hvx) { > gen_commit_hvx(ctx); > } > update_exec_counters(ctx); > > - if (pkt->vhist_insn != NULL) { > + if (ctx->pkt.vhist_insn != NULL) { > ctx->pre_commit = false; > - ctx->insn = pkt->vhist_insn; > - pkt->vhist_insn->generate(ctx); > + ctx->insn = ctx->pkt.vhist_insn; > + ctx->pkt.vhist_insn->generate(ctx); > } > > - if (pkt->pkt_has_cof) { > + if (ctx->pkt.pkt_has_cof) { > gen_end_tb(ctx); > } > } > @@ -930,7 +915,6 @@ static void > decode_and_translate_packet(CPUHexagonState *env, DisasContext *ctx) > { > uint32_t words[PACKET_WORDS_MAX]; > int nwords; > - Packet pkt; > int i; > > nwords = read_packet_words(env, ctx, words); > @@ -939,16 +923,15 @@ static void > decode_and_translate_packet(CPUHexagonState *env, DisasContext *ctx) > return; > } > > - ctx->pkt = &pkt; > - if (decode_packet(ctx, nwords, words, &pkt, false) > 0) { > - pkt.pc = ctx->base.pc_next; > + if (decode_packet(ctx, nwords, words, &ctx->pkt, false) > 0) { > + ctx->pkt.pc = ctx->base.pc_next; > gen_start_packet(ctx); > - for (i = 0; i < pkt.num_insns; i++) { > - ctx->insn = &pkt.insn[i]; > + for (i = 0; i < ctx->pkt.num_insns; i++) { > + ctx->insn = &ctx->pkt.insn[i]; > gen_insn(ctx); > } > gen_commit_packet(ctx); > - ctx->base.pc_next += pkt.encod_pkt_size_in_bytes; > + ctx->base.pc_next += ctx->pkt.encod_pkt_size_in_bytes; > } else { > gen_exception_end_tb(ctx, HEX_CAUSE_INVALID_PACKET); > } > diff --git a/target/hexagon/gen_tcg_funcs.py > b/target/hexagon/gen_tcg_funcs.py > index 87b7f10d7f..e7f90a0da1 100755 > --- a/target/hexagon/gen_tcg_funcs.py > +++ b/target/hexagon/gen_tcg_funcs.py > @@ -72,7 +72,7 @@ def gen_tcg_func(f, tag, regs, imms): > for immlett, bits, immshift in imms: > declared.append(hex_common.imm_name(immlett)) > > - arguments = ", ".join(["ctx", "ctx->insn", "ctx->pkt"] + declared) > + arguments = ", ".join(["ctx", "ctx->insn", "&ctx->pkt"] + > declared) > f.write(f" emit_{tag}({arguments});\n") > > elif hex_common.skip_qemu_helper(tag): > diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py > index c0e9f26aeb..e37d5a514f 100755 > --- a/target/hexagon/hex_common.py > +++ b/target/hexagon/hex_common.py > @@ -1144,7 +1144,7 @@ def helper_args(tag, regs, imms): > if need_pkt_has_multi_cof(tag): > args.append(HelperArg( > "i32", > - "tcg_constant_tl(ctx->pkt->pkt_has_multi_cof)", > + "tcg_constant_tl(ctx->pkt.pkt_has_multi_cof)", > "uint32_t pkt_has_multi_cof" > )) > if need_pkt_need_commit(tag): > @@ -1156,7 +1156,7 @@ def helper_args(tag, regs, imms): > if need_PC(tag): > args.append(HelperArg( > "i32", > - "tcg_constant_tl(ctx->pkt->pc)", > + "tcg_constant_tl(ctx->pkt.pc)", > "target_ulong PC" > )) > if need_next_PC(tag): > -- > 2.34.1 > >
