During compilation on MIPS, GCC issues a lot of "cast-align" and other types of warnings. Reproducible: Always Steps to Reproduce: 1. Regular build Actual Results: GCC issues a lot of warnings. Expected Results: Reasonable number of warnings.
Created attachment 101429 [details] Patch 1/4
Created attachment 101430 [details] Patch 2/4
Created attachment 101431 [details] Patch 3/4
Created attachment 101432 [details] Patch 4/4
I propose these four independent patches that reduce number of warnings by ~50%. Applying patches: patch -p1 < warn0x.diff
Most of these patches are not MIPS-specific, touch common code and probably remove warnings on different platforms too. Can others take a look at these as well? @Aleksandar Can you write which patch removes what warning?
Patch 1/4 (warn01.diff) removes 98 "cast-align" warnings caused by including vki-linux.h from various sources. PTR_CAST macro from this patch can be used in future, to avoid "cast-align" warnings. Patch 2/4 (warn02.diff) removes few "cast-align" warnings from coregrind/launcher-linux.c. Patch 3/4 (warn03.diff) removes "cast-align" warning from coregrind/m_syswrap/syswrap-generic.c. Patch 4/4 (warn04.diff) is MIPS specific and removes 32 "cast-equal" warnings from coregrind/m_coredump/coredump-elf.c. These numbers are relevant for MIPS build.
gcc on sparcv9 platform (which is currently under development) also warns about unaligned access. Therefore I'd gratefully utilize newly introduced macro in places which these patches do no cover.
This feels to me like hiding misalignment problems. I'd prefer to remove misaligned accesses where possible. Building with --enable-usban at least makes it possible to see, on any platform, where the run-time misaligned accesses are.
Created attachment 102817 [details] remove coredump-elf cast warning (In reply to Aleksandar Rikalo from comment #4) > Created attachment 101432 [details] > Patch 4/4 Can we simplify the patch #4 just to # define DO(n) (*fpu)[n] = arch->vex.guest_f##n ? Would that work? (Check the attached patch.)
> Can we simplify the patch #4 just to > # define DO(n) (*fpu)[n] = arch->vex.guest_f##n > ? > Would that work? (Check the attached patch.) Hi Petar, It wouldn't work - (*fpu)[n] are doubles, but arch->vex.guest_f##n are ULongs. In this case we need raw memory move, without long2double conversions.
(In reply to Aleksandar Rikalo from comment #11) > It wouldn't work - (*fpu)[n] are doubles, but arch->vex.guest_f##n are > ULongs. In this case we need raw memory move, without long2double > conversions. Yes, you are right, the conversion will change the values of FP registers and incorrect values would be stored in the core dump. Original patch has been committed as r16190. Thanks.
(In reply to Julian Seward from comment #9) > This feels to me like hiding misalignment problems. I'd prefer to remove > misaligned > accesses where possible. Building with --enable-usban at least makes it > possible > to see, on any platform, where the run-time misaligned accesses are. Hi Julian, My idea is not to hide all warnings, but to hide "false" ones. There are situations (eg. vki-linux.h:677) where we know (for sure) that address is aligned. Macro introduced in Patch 1/4 should be used to silence compiler in these cases. With -O2, there are no differences in object codes with or without using PTR_CAST macro.
Ivo, Julian, Can we do something about this? It would be nice if we could define the strategy and start to eliminate false warnings. Please take a look on Patch 1/4, if proposed concept is acceptable to you, I will continue to use PTR_CAST.
I am quite happy with the first patch (1/4). That particular expression in vki-linux.h has been extensively studied and found to be benign with regards to potential misaligned access. I'd reword the comment for PTR_CAST slightly: // Some architectures (eg. mips, arm) do not support unaligned memory access // by hardware, so GCC warn about suspicious situations. This macro could // be used to avoid these warnings but only after careful examination. Also why did you chose name 'PTR_CAST'? Perhaps ALIGN_CAST or ASSUME_ALIGNED would be more self explanatory?
Thank You for review. I agree with changes in comment and also You are right about name. I vote for ASSUME_ALIGNED, in that case, it is clear that there is a assumption. (In reply to Ivo Raisr from comment #15) > I am quite happy with the first patch (1/4). > That particular expression in vki-linux.h has been extensively studied and > found to be benign with regards to potential misaligned access. > > I'd reword the comment for PTR_CAST slightly: > // Some architectures (eg. mips, arm) do not support unaligned memory access > // by hardware, so GCC warn about suspicious situations. This macro could > // be used to avoid these warnings but only after careful examination. > > Also why did you chose name 'PTR_CAST'? Perhaps ALIGN_CAST or ASSUME_ALIGNED > would be more self explanatory?
I was also wondering of the impact on other architectures. Please could you conduct a quick test to see how the compiler resolves ASSUME_ALIGNED on amd64/ppc, for example? And mips/arm for comparison? Perhaps we will see that the assembly both cases (with and without ASSUME_ALIGNED) is the same, perhaps not. If not, then the implementation of ASSUME_ALIGNED should be guarded with the corresponding arch-specific #ifdefs. Please supply the fixed first patch afterwards.
Created attachment 104189 [details] Patch 1/4 Fixed patch, after comment 17.
(In reply to Ivo Raisr from comment #17) > I was also wondering of the impact on other architectures. > Please could you conduct a quick test to see how the compiler resolves > ASSUME_ALIGNED on amd64/ppc, for example? And mips/arm for comparison? I have used following code to see how the GCC resolves ASSUME_ALIGNED on MIPS32/64, ARM32, S390, PPC32/64 and AMD64: char *a; int f1() { return *ASSUME_ALIGNED(int*, a); } int f2() { return *((int*)a); } With -O1 (or higher) generated code for f1() and f2() are identical. Without optimizations (-O0), f1() has few instructions more than f2(), more precisely: - AMD64: 2 additional mov instructions - PPC32: additional stw/lwz pair - PPC64: additional std/ld pair - S390: additional st/l pair - ARM32: additional str/ldr pair + two instructions for stack arrangement - MIPS32: additional sw/lw pair - MIPS64: additional sd/ld pair
Created attachment 104201 [details] slightly modified patch 1/4 Patch 1/4 slightly modified to fix formatting and with added NEWS entry.
Patch 1/4 committed in SVN r16256.
Remark by Mark Wielaard: After the number of compiler warnings caused by -Wcast-align is greatly reduced, the following configure check for arm32 should be removed from configure.ac: if test "X$VGCONF_ARCH_PRI" = "Xarm"; then AC_SUBST([FLAG_W_CAST_ALIGN], [""]) else AC_SUBST([FLAG_W_CAST_ALIGN], [-Wcast-align]) fi
What's the status of this now? Can it be closed?
Patch 1/4 landed in the repo. I expect that Aleksandar prepares another patch for review.
(In reply to Julian Seward from comment #23) > What's the status of this now? Can it be closed? No, it shouldn't. This is just the beginning. I'll prepare next few patches soon. Also, there are patches 2 and 3 which are still uncommitted.
Aleksandar, do you have another patch ready?
Created attachment 105392 [details] m_libcbase_warnings.diff: Suppress warnings from coregrind/m_libcbase.c (In reply to Ivo Raisr from comment #26) > Aleksandar, do you have another patch ready? Hi Ivo, Acctualy, I have. m_libcbase_warnings.diff: Suppress warnings from coregrind/m_libcbase.c. Function VG_(memset)() - after "while ((!VG_IS_4_ALIGNED(d)))" pointer d is 4-byte aligned, so it is safe to use ASSUME_ALIGNED for UInt* casting. QSORT - According to swaptype variable initialization (m_libcbase.c:817), condition (swaptype <= 1) implies "elements are Word-aligned", so we can use ASSUME_ALIGNED in parts under the above condition ( in BM_SWAP(), BM_PVINIT() and bm_swapfunc() ). This is tested on mips32/64-linux and amd64-linux. Aleksandar
Created attachment 105406 [details] slightly modified patch for m_libcbase.c
Thank you for another patch. I've modified it slightly in VG_(memset)() and tested successfully on sparcv9/Linux.
Created attachment 105418 [details] Patch for mc_main.c This patch solves "cast-align" warnings in mecheck/mc_main.c In a lot of places in this file after checking for alignment UChar array pointer vabits8 in structure SecMap is casted in UShort pointer. To avoid this cast SecMap can be transformed to an union of UChar vabits8 array and UShort vabits16 array with same byte count. In two other places ASSUME_ALIGNED can be used.
Another patch committed in SVN r16348.
(In reply to Ivo Raisr from comment #31) > Another patch committed in SVN r16348. Thank You! Please take a look at Tamara's patch for mc_main.c.
Created attachment 105429 [details] Slightly modified patch for syswrap-generic.c Nothing special here, just use UInt* instead UChar*. Tested on mips32/64-linux and amd64-linux.
I am afraid I don't see how the latest patch for syswrap-generic.c improves the situation. On sparc64/Linux, I see the following warnings (line numbers are approximate but you get the idea): m_syswrap/syswrap-generic.c: In function ‘inet6_format’: m_syswrap/syswrap-generic.c:702:11: warning: cast increases required alignment of target type [-Wcast-align] (const struct vki_in_addr *)(ip + 12); ^ m_syswrap/syswrap-generic.c: In function ‘pre_mem_read_sockaddr’: m_syswrap/syswrap-generic.c:1119:36: warning: cast increases required alignment of target type [-Wcast-align] struct vki_sockaddr_in* sin = (struct vki_sockaddr_in *)sa; ^ m_syswrap/syswrap-generic.c:1120:36: warning: cast increases required alignment of target type [-Wcast-align] struct vki_sockaddr_in6* sin6 = (struct vki_sockaddr_in6 *)sa; ^ m_syswrap/syswrap-generic.c:1125:36: warning: cast increases required alignment of target type [-Wcast-align] struct vki_sockaddr_nl* nl = (struct vki_sockaddr_nl *)sa; If my understanding is correct then your patch deals just with the first occurrence. And why is ipv6 now an array of 4 x UInt? This does not make much sense - IPv6 is just 16 bytes, usually displayed per hextets [1]. Could you: - Deal with all 4 occurrences - Use for example ASSUME_ALIGNED for the casts? [1] https://en.wikipedia.org/wiki/IPv6_address
Created attachment 105439 [details] Patch 2/4 updated
(In reply to Tamara Vlahovic from comment #35) Committed in Valgrind SVN as r16365. Nice approach, thank you for the patch.
Created attachment 105467 [details] Patch for m_mallocfree.c This patch fixes "cast-align" warnings in coregind/m_mallocfree.c, using ASSUME_ALIGNED macro. Struct Block in file coregind/m_mallocfree.c does not contain any fields, because their variable length. This creates a need for a lot of casts that cause "cast-align" warnings. In line 124 is noted that all Block payloads must be VG_MIN_MALLOC_SZB-aligned. VG_MIN_MALLOC_SZB has value of 8 in 32-bit architectures and 16 on 64-bit architectures. Because of this we may assume that all the types which have size of VG_MIN_MALLOC_SZB or less are correctly aligned.
Created attachment 105474 [details] Patch for warnings related to x87 This patch fixes "cast-align" warnings related to getting and putting x87 guest state and its registers. In files: VEX/priv/guest_x86_helpers.c VEX/priv/guest_amd64_helpers.c functions do_put_x87 and do_get_x87 have parameter UChar* x87_state which is later casted to Fpu_State*. After investigation of the uses of these functions it is determined that most of the calls to the function have a cast to UChar* of argument that is passed as parameter x87_state. Lot of times it is done for argument of Fpu_State* type. Changing the parameter UChar* x87_state with Fpu_State* x87, reduces need for casting and removes "cast-align" warnings. Some "cast-align" warnings in files VEX/priv/guest_x86_helpers.c and VEX/priv/guest_amd64_helpers.c are cause by upcasting pointer to the part of reg field of Fpu_State* structure from UChar* to UShort*. Field reg represent FPU registers, each with a size of 10 bytes. Because of that type of reg field may be changed from UChar* to UShort* without losing correctness. This removes need for upcasting and with that "cast-align" warnings. For removing all "cast-align" warnings in these two files it is needed to remove warnings caused by casting in V128 type. This was not done in this patch.
(In reply to Tamara Vlahovic from comment #37) Committed in Valgrind SVN as r16368. Thank you for the patch.
(In reply to Tamara Vlahovic from comment #38) I've had a look at patch for x87 stuff. While passing Fpu_State* instead of UChar* is a good idea, on the other hand changing UChar[80] to UShort[40] is not a very good idead, though. As you are aware, the real x87 layout contains 8 registers per 80 bits (10 bytes). So it is hard to express 80 bits as a natural C type; and that is the reason why UChar[80] was chosen. In addition to that, Fpu_State_16 still contains UChar[80], leading to confusion. I don't know if there is a good/straightforward way how to mitigate warnings for reg[80]. Perhaps some kind of union? If you are not sure, then split the patch and I will take in Fpu_State* stuff while you can work on reg[80] part.
Created attachment 105554 [details] Patch 2/4 fix I noticed today that my update of a Patch 2/4 have a mistake that causes build to crash on 64BE architectures. This patch fixes it.
(In reply to Tamara Vlahovic from comment #41) Indeed, thank you for spotting it. I think it was caused by a mismerge from TileGX port removal. Commited as SVN r16377.
Created attachment 105566 [details] Patch for warnings related to x87 caused by cast in Fpu_State* I split the patch. I'll look for some other way to fix warnings caused by cast to Short*.
(In reply to Tamara Vlahovic from comment #43) Thank you for the patch. Committed in VEX SVN as r3372.
We are very close to a new Valgrind release. So at this point I will close this particular bug. Please feel free to create a new one and we will work there.