From: Mikael Pettersson Subject: Re: [PATCH] e2fsprogs: m68k bitops fixes Date: Sun, 16 Dec 2012 22:39:32 +0100 Message-ID: <20686.16276.932405.296864@pilspetsen.it.uu.se> References: <20685.53075.403265.997210@pilspetsen.it.uu.se> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: linux-ext4@vger.kernel.org, Andreas Schwab To: Mikael Pettersson Return-path: Received: from smtp1.uu.se ([130.238.7.54]:38598 "EHLO smtp1.uu.se" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751365Ab2LPVj7 (ORCPT ); Sun, 16 Dec 2012 16:39:59 -0500 In-Reply-To: <20685.53075.403265.997210@pilspetsen.it.uu.se> Sender: linux-ext4-owner@vger.kernel.org List-ID: Mikael Pettersson writes: > This patch fixes two problems with the m68k-specific bitops > in e2fsprogs. > > The first problem is that tst_bitops SEGVs: > > LD_LIBRARY_PATH=../../lib DYLD_LIBRARY_PATH=../../lib ./tst_bitops > make[1]: *** [check] Segmentation fault > > Post-mortem debugging shows that this occurs in the m68k-specific > ext2fs_set_bit() when called with BIG_TEST_BIT as bit number: > > Core was generated by `./tst_bitops'. > Program terminated with signal 11, Segmentation fault. > #0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357 > 357 __asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0" > (gdb) bt > #0 0x80000820 in ext2fs_set_bit (addr=0xc01ba008, nr=2147483690) at ../../lib/ext2fs/bitops.h:357 > #1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112 > (gdb) up > #1 main (argc=1, argv=0xefc31174) at tst_bitops.c:112 > 112 ext2fs_set_bit(BIG_TEST_BIT, bigarray); > > The SEGV occurs because bfset et al interpret the bit number as a > _signed_ value, so 2147483690 is actually -2147483606, causing an > access to a location about 1/4 GB before 'bigarray'. > > To fix this I adjust the base address to point to the correct byte, > and limit the bit number to be within that byte, before performing > the bfset (and likewise in the clear_bit and test_bit functions). > This is similar to what the x86-specific bitops do. > > With this in place the SEGV is gone and the tests pass, but there > is still some strange output from tst_bitops: > > big bit number (2147483690) test: 0, expected 4 > big bit number (2147483690) test: 4, expected 0 > ext2fs_set_bit big_test successful > > The test and expected values differ, yet the test case doesn't fail. > > A second look at the m68k bitop asm() statements makes the reason clear: > there are _no_ dependencies whatsoever between the asm()s and the > memory locations being accessed (a dependency on the address isn't > enough to create a dependency on memory locations reachable from it, > and __volatile__ doesn't create memory dependencies either). The > simplest fix is to add "memory" clobbers (the kernel's bitops also > have memory clobbers), and with those in place the tests still pass > and the debug output looks sane: > > big bit number (2147483690) test: 4, expected 4 > big bit number (2147483690) test: 0, expected 0 > ext2fs_set_bit big_test successful > > Tested on m68k-linux with e2fsprogs-1.42.6 and gcc-4.6.3. Andreas Schwab suggested that there was no longer an advantage to using inline asm for these operations, so this new patch deletes the m68k-specific code from lib/ext2fs/bitops.h (leaving only 32-bit x86 to still use inline asm there). Tested on m68k-linux with e2fsprogs-1.42.6 and gcc-4.6.3 as before. All tests pass and the debug output looks sane. I compared the e2fsck binaries from the previous build with this one. They had identical .text sizes, and almost the same number of bit field instructions (obviously compiler-generated), so this change should have no serious performance implications. Signed-off-by: Mikael Pettersson Cc: Andreas Schwab --- lib/ext2fs/bitops.h | 39 +-------------------------------------- 1 files changed, 1 insertion(+), 38 deletions(-) --- e2fsprogs-1.42.6/lib/ext2fs/bitops.h.~1~ 2012-09-10 04:53:00.000000000 +0200 +++ e2fsprogs-1.42.6/lib/ext2fs/bitops.h 2012-12-16 17:41:09.000000000 +0100 @@ -200,7 +200,7 @@ extern errcode_t ext2fs_find_first_zero_ */ #ifdef NO_INLINE_FUNCS #if (defined(__GNUC__) && (defined(__i386__) || defined(__i486__) || \ - defined(__i586__) || defined(__mc68000__))) + defined(__i586__))) /* This prevents bitops.c from trying to include the C */ /* function version of these functions */ #define _EXT2_HAVE_ASM_BITOPS_ @@ -345,43 +345,6 @@ _INLINE_ __u16 ext2fs_swab16(__u16 val) #endif /* i386 */ -#if ((defined __GNUC__) && !defined(_EXT2_USE_C_VERSIONS_) && \ - (defined(__mc68000__))) - -#define _EXT2_HAVE_ASM_BITOPS_ - -_INLINE_ int ext2fs_set_bit(unsigned int nr,void * addr) -{ - char retval; - - __asm__ __volatile__ ("bfset %2@{%1:#1}; sne %0" - : "=d" (retval) : "d" (nr^7), "a" (addr)); - - return retval; -} - -_INLINE_ int ext2fs_clear_bit(unsigned int nr, void * addr) -{ - char retval; - - __asm__ __volatile__ ("bfclr %2@{%1:#1}; sne %0" - : "=d" (retval) : "d" (nr^7), "a" (addr)); - - return retval; -} - -_INLINE_ int ext2fs_test_bit(unsigned int nr, const void * addr) -{ - char retval; - - __asm__ __volatile__ ("bftst %2@{%1:#1}; sne %0" - : "=d" (retval) : "d" (nr^7), "a" (addr)); - - return retval; -} - -#endif /* __mc68000__ */