From patchwork Mon Jul 11 21:51:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 13739 Received: (qmail 70225 invoked by alias); 11 Jul 2016 21:51:59 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 70209 invoked by uid 89); 11 Jul 2016 21:51:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=luckily, movslq, shr, jne X-HELO: mail-qk0-f194.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=xPD7/Jdmo6QOtTKF9zRaySe/KgXIXLHTCq1kxwsxKTc=; b=P8xsJorVeLHKi9pjFJb6o8DrWLwg2FmMzhadTk/nS8f/E+wCpwtmP2DRy0vL/taQab xe2MnowD8/Zu3Y2S1TMhKJ0wCiKA/yIeHoFceGtHn/hX38TFobgFC+PqVha65+DoQdGU Ib/Z8eCgcVgDPNWtbbDX3Na+wGRM6u8ZeO/24nMHoXNcq8bPv7t5K1dVIl1GzNs5UvDN 8piJ0VE0nT63r4CilJYjlFblEb2+YzYenXPPM+ONg2Sc5INfXUJkwnptGMkusb+MoE0B LBQq7/dNRHZ3YdjxFLwfVjPcfibT7E2LqPqtytyzsJsF4TsxpT77dQP3UqBRR/PsvS8K jr1A== X-Gm-Message-State: ALyK8tLVcLulTthaPEkxxIbn7CdEvtfQ7odqQNIS6k595XjrdEJNqBhuBel9rlfbO0J+7qdAzfwNloW2LQ0jbw== X-Received: by 10.55.129.135 with SMTP id c129mr28534954qkd.174.1468273905482; Mon, 11 Jul 2016 14:51:45 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1467834756-21898-1-git-send-email-adhemerval.zanella@linaro.org> From: "H.J. Lu" Date: Mon, 11 Jul 2016 14:51:44 -0700 Message-ID: Subject: Re: [PATCH v2] Fix LO_HI_LONG definition To: Chris Metcalf Cc: Adhemerval Zanella , GNU C Library On Mon, Jul 11, 2016 at 2:28 PM, Chris Metcalf wrote: > On 7/11/2016 5:09 PM, H.J. Lu wrote: >> >> On Wed, Jul 6, 2016 at 12:52 PM, Adhemerval Zanella >> wrote: >>> >>> The p{read,write}v{64} consolidation patch [1] added a wrong guard >>> for LO_HI_LONG definition. It currently uses both >>> '__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set >>> the value to be passed in one argument, otherwise it will be split >>> in two. Since kernel commit 601cc11d054 the non-compat >>> preadv/pwritev in same order, so the LO_HI_LONG is the same regardless >>> off/off64_t size or wordsize. >>> >>> The tests were passing on 64-bit because files were small enough so >>> the high part of offset is 0 regardless. This patch also changes the >>> tst-preadvwritev64 test to check reads and writes on a sparse file >>> larger than 4G. >>> >>> Checked on x86_64, i686, x32, aarch64, armhf, and s390. >>> >>> * sysdeps/unix/sysv/linux/sysdep.h >>> [__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): >>> Remove >>> guards. >>> * misc/tst-preadvwritev-common.c: New file. >>> * misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c. >>> * misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and >>> add >>> a check for files larger than 2GB. >>> >>> diff --git a/sysdeps/unix/sysv/linux/sysdep.h >>> b/sysdeps/unix/sysv/linux/sysdep.h >>> index 8c9e62e..a469f57 100644 >>> --- a/sysdeps/unix/sysv/linux/sysdep.h >>> +++ b/sysdeps/unix/sysv/linux/sysdep.h >>> @@ -49,10 +49,6 @@ >>> #endif >>> >>> /* Provide a macro to pass the off{64}_t argument on >>> p{readv,writev}{64}. */ >>> -#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32 >>> -# define LO_HI_LONG(val) (val) >>> -#else >>> -# define LO_HI_LONG(val) \ >>> - (long) (val), \ >>> - (long) (((uint64_t) (val)) >> 32) >>> -#endif >>> +#define LO_HI_LONG(val) \ >>> + (long) (val), \ >>> + (long) (((uint64_t) (val)) >> 32) >>> -- >> >> This change is incorrect for x32. __ASSUME_WORDSIZE64_ILP32 seems >> to mean different things for mips and x86-64. X86-64 always passes 64-bit >> value in a 64-bit register. > > > Also, I'm not convinced the old code was right either. At least, the old one is correct for x86. > In the 64-bit case (or HJ's 32-bit with 64-bit syscall args), shouldn't we > have "#define LO_HI_LONG(val) (val), 0"? The trailing zero will ensure that > the kernel's pos_h argument gets a zero value so we don't end up > or'ing in garbage with the pos_from_hilo() call. > Yes, we have garbage in pos_h (%r8): 0000000000000000 : 0: 8b 05 00 00 00 00 mov 0x0(%rip),%eax # 6 2: R_X86_64_PC32 __libc_multiple_threads-0x4 6: 49 89 ca mov %rcx,%r10 9: 85 c0 test %eax,%eax b: 75 3b jne 48 d: 49 89 c8 mov %rcx,%r8 10: 48 63 d2 movslq %edx,%rdx 13: 48 63 ff movslq %edi,%rdi 16: 49 c1 e8 20 shr $0x20,%r8 1a: b8 28 01 00 00 mov $0x128,%eax 1f: 0f 05 syscall Luckily, kernel removes the garbage: /* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}. */ #define LO_HI_LONG(val) \ (long) (val), \ (long) (((uint64_t) (val)) >> 32) kernel has static inline loff_t pos_from_hilo(unsigned long high, unsigned long low) { #define HALF_LONG_BITS (BITS_PER_LONG / 2) return (((loff_t)high << HALF_LONG_BITS) << HALF_LONG_BITS) | low; } SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec, unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h) { loff_t pos = pos_from_hilo(pos_h, pos_l); But we don't pass garbage nor zero in pos_h. I am testing this patch: diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_ 64/sysdep.h index d023d68..1a671e1 100644 --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h @@ -385,4 +385,8 @@ # endif #endif +/* How to pass the off{64}_t argument on p{readv,writev}{64}. */ +#undef LO_HI_LONG +#define LO_HI_LONG(val) (val) + #endif /* linux/x86_64/sysdep.h */