Ping regarding this UB due to an invalid shift -- I think this is now the only remaining sanitizer error in a 'make check-functional', so it would be nice to get it fixed...
thanks -- PMM On Thu, 7 Nov 2024 at 17:33, Peter Maydell <[email protected]> wrote: > > On Tue, 7 Jun 2022 at 00:31, Richard Henderson > <[email protected]> wrote: > > > > From: Xiaojuan Yang <[email protected]> > > > > This includes: > > - TLBSRCH > > - TLBRD > > - TLBWR > > - TLBFILL > > - TLBCLR > > - TLBFLUSH > > - INVTLB > > Hi; running the loongarch functional tests on a build with > the clang undefined-behaviour sanitizer enabled reveals an > attempt to shift by an out-of-range amount in > helper_invtlb_page_asid_or_g(): > > ../../target/loongarch/tcg/tlb_helper.c:470:31: runtime error: shift > exponent 244 is too large for 64-bit type 'uint64_t' (aka 'unsigned > long') > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior > ../../target/loongarch/tcg/tlb_helper.c:470:31 in > > > +void helper_invtlb_page_asid_or_g(CPULoongArchState *env, > > + target_ulong info, target_ulong addr) > > +{ > > + uint16_t asid = info & 0x3ff; > > + > > + for (int i = 0; i < LOONGARCH_TLB_MAX; i++) { > > + LoongArchTLB *tlb = &env->tlb[i]; > > + uint8_t tlb_g = FIELD_EX64(tlb->tlb_entry0, TLBENTRY, G); > > + uint16_t tlb_asid = FIELD_EX64(tlb->tlb_misc, TLB_MISC, ASID); > > + uint64_t vpn, tlb_vppn; > > + uint8_t tlb_ps, compare_shift; > > + > > + if (i >= LOONGARCH_STLB) { > > + tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS); > > + } else { > > + tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS); > > + } > > We read here a field from the guest, which can be 0. > > > + tlb_vppn = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN); > > + vpn = (addr & TARGET_VIRT_MASK) >> (tlb_ps + 1); > > + compare_shift = tlb_ps + 1 - R_TLB_MISC_VPPN_SHIFT; > > If tlb_ps is 0, then "tlb_ps + 1 - R_TLB_MISC_VPPN_SHIFT" > is tlb_ps + 1 - 13 == tlb_ps - 12. When converted back to > uint8_t this is 244. > > > + > > + if ((tlb_g || (tlb_asid == asid)) && > > + (vpn == (tlb_vppn >> compare_shift))) { > > Here we shift tlb_vppn by 244, which is undefined behaviour > and triggers the sanitizer. > > > + tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, E, 0); > > + } > > + } > > + tlb_flush(env_cpu(env)); > > +} > > What's the intended behaviour here? > > This likely applies also to other similar functions; this > is just the one that I found. > > thanks > -- PMM
