From patchwork Sun Dec 16 21:39:32 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikael Pettersson X-Patchwork-Id: 206742 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 8FDAA2C00A7 for ; Mon, 17 Dec 2012 08:40:02 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751491Ab2LPVkA (ORCPT ); Sun, 16 Dec 2012 16:40:00 -0500 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 X-Virus-Scanned: amavisd-new at uu.se Received: from pilspetsen.it.uu.se (pilspetsen.it.uu.se [130.238.18.39]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by bornea.its.uu.se (Postfix) with ESMTPS id 1028D8844C; Sun, 16 Dec 2012 22:38:41 +0100 (CET) Received: (from mikpe@localhost) by pilspetsen.it.uu.se (8.14.5+Sun/8.14.5) id qBGLdWZ0014383; Sun, 16 Dec 2012 22:39:32 +0100 (MET) X-Authentication-Warning: pilspetsen.it.uu.se: mikpe set sender to mikpe@it.uu.se using -f MIME-Version: 1.0 Message-ID: <20686.16276.932405.296864@pilspetsen.it.uu.se> Date: Sun, 16 Dec 2012 22:39:32 +0100 From: Mikael Pettersson To: Mikael Pettersson Cc: linux-ext4@vger.kernel.org, Andreas Schwab Subject: Re: [PATCH] e2fsprogs: m68k bitops fixes In-Reply-To: <20685.53075.403265.997210@pilspetsen.it.uu.se> References: <20685.53075.403265.997210@pilspetsen.it.uu.se> X-Mailer: VM 7.17 under Emacs 20.7.1 Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org 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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 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__ */ - #if !defined(_EXT2_HAVE_ASM_SWAB_)