On Thu, May 06, 2021 at 11:45:09PM -0700, Eric Biggers wrote: > Just to be clear (looking at the latest patches on the list which are copying > whole structs), by "the memcpy() approach does get optimized properly", I meant > that it gets optimized properly in implementations of get_unaligned_le16(), > get_unaligned_le32(), put_unaligned_le32(), etc., where a single word (or less > than a word) is loaded or stored. I don't know how reliably the compilers will > optimize out the copy if you memcpy() a whole struct instead of a single word. > > Even if they don't optimize it out, I don't expect that it would be a > performance problem in this context, so it's probably still fine to solve the > problem. But I just wanted to clarify what I meant here. For the most recent patch that sent out, we really needed to copy out the whole structure since we're then passing it to ext2fs library functions. I agree that it's not likely going to be a performance problem, and at this point, I'm more concerned about code clarity and correctness. Especially since apparently the problems which Harshad's change and my most recent commit addressed were not picked up by UBSAN (either using gcc or clang), --- and IMHO they really should have. So we can't count on UBSAN to find all possible alignment problems. Lesson learned, before I do future releases, I should do a build and "make check" on a armhf chroot running on a arm-64 machine, as well as on a sparc64 machine, since these seem to be the most sensitive to alignment issues. And if I miss anything, fortunately Debian's autobuilders on a large cross-section of architectures will catch them since we run the regression test suite as part of the package build. - Ted P.S. Harshad, could you prepare patches to kernel files in ext4 and jbd2 to make similar alignment portability fixes? Thanks!!
