Created attachment 125715 [details] proposed solution The problem in question has been discovered on a mips64 BE machine. Proposed solution attached. It consists of using the u32 member variable of the union inside the vbits_t structure instead of the u1 one since the least significant part of u32 does not seem to overlap with u1. Tested on x86 and mips64 BE.
I tested the proposed fix on a Power 8 machine running Red Hat 7.6 (Maipo) in BE mode. Works great.
(In reply to Stefan Maksimovic from comment #0) > [...] > Tested on x86 and mips64 BE. Thanks, this also helps on s390x, once And1 and Or1 are actually supported. However, I wonder why the non-overlapping is a problem, and why it doesn't matter for the other union members. Probably because the logic exploits the overlapping of the least significant bit of u32 with u1 somewhere? Where in the logic does that occur? Also, even if not necessary, I wonder whether the logic could be simplified by eliminating u1 and u16 (and possibly u32) as well.
In fact, in most cases of the Ity_I1 operations, the u32 field is used (e.g., vbits.c: 51). The u1 field is used only in few places (removed by the patch). That isn't a problem on LE since unit8_t overlaps with LSB of uint32_t, but it is a problem on BE. Ity_I8, ..., Ity_I64 operations don't require any overlaps. The other possible way to fix the test is to keep u1 field and replace all usages of u32 for Ity_I1 purpose into u1. It seems like it would be more complicated, especially because valgrind_[set|get]_vbits() functions (see valgrind.c) have to be modified. I'm looking at vbits.c and I'm not sure how much logic would be simplified by eliminating the u8, ..., u32 fields (e.g., concat_vbits() function). I would put that option aside.
(In reply to Aleksandar Rikalo from comment #3) [...] > The other possible way to fix [...] seems like it would be more > complicated, especially because valgrind_[set|get]_vbits() functions (see > valgrind.c) have to be modified. > > [...] I'm not sure how much logic would be simplified > by eliminating the u8, ..., u32 fields (e.g., concat_vbits() function). I > would put that option aside. FWIW, I'd be fine with that. Julian, what do you think? Can we move this forward?
> Julian, what do you think? Can we move this forward? Yeah, the patch looks reasonable to me. Pls do land. Thank you all for the analysis and the fix.
Committed at https://sourceware.org/git/?p=valgrind.git;a=commit;h=93bb2da218a691532cd709b195b687e02ac0f1d2