| Message ID | CAMe9rOojrkHtkWrDgw6Y6O7U5iZLjQph-qX1aH4iqPyupHXjYQ@mail.gmail.com |
|---|---|
| State | Committed |
| Headers |
Return-Path: <hjl.tools@gmail.com> X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-il1-x131.google.com (mail-il1-x131.google.com [IPv6:2607:f8b0:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 9119E3887000 for <libc-alpha@sourceware.org>; Mon, 13 Apr 2020 16:33:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9119E3887000 Received: by mail-il1-x131.google.com with SMTP id i14so8981250ilr.11 for <libc-alpha@sourceware.org>; Mon, 13 Apr 2020 09:33:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jTXqLprPjvBHruJgPAMjT6G6o3sQpjPZ6Q/+lDhno7g=; b=Lnu2zX6IFKlHUC2dWTxIeA+vC6b2WUNcnqzqZGlGSchMZnLv6UoZ5wLUOaeDTZnZL7 kSSGjC45JjON6VddeH1GKnYVz1drMudgaQQeIq3r/OEqJNhcS6W1cXIdbOChMeMUpaG3 irXR/DmSzGeWJJf23Vb19p5u2mZm317ibmGYkCDLINP41f+IxJit98wXOqgGp50RGrxK Z9KkBvgroVw1LtGmTgMRi+THuh8Uco+Yx0waCZfr2gXtEqgAmXm1jCHQyhhgsW4zbVra +1Bvw6IIWrgqtWsBNsSDcIasHjfduRRmebqA5Zxy0llBQRdcu3DPdxEJTP6O247SFlpY +Jiw== X-Gm-Message-State: AGi0PubaYOCy7QYxZzLdGETrFqGO+JBvQX7mDT7/8jFqK4ItNKmtHpvu lYJxb3b++9Z3MskjVm8vzqCbOQRG2Zp6fjy6uMLuag== X-Google-Smtp-Source: APiQypL/CYbXZT+29RFG1nwLy0bic4V78lFKIOZW+dlS/Jd1rjQGe7D7iG88pbOPT+40WrJDSPHzqstFjmQTiJdudt8= X-Received: by 2002:a92:cc11:: with SMTP id s17mr16293208ilp.13.1586795621871; Mon, 13 Apr 2020 09:33:41 -0700 (PDT) MIME-Version: 1.0 References: <20200413142515.16619-1-hjl.tools@gmail.com> <20200413142515.16619-3-hjl.tools@gmail.com> <8736974fag.fsf@mid.deneb.enyo.de> <CAMe9rOoNg8Cq8rHmLaMf6gAUdyGEpmnsJ5VOyGj2OO1AUFbLMw@mail.gmail.com> <87y2qz2z5l.fsf@mid.deneb.enyo.de> In-Reply-To: <87y2qz2z5l.fsf@mid.deneb.enyo.de> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Mon, 13 Apr 2020 09:33:06 -0700 Message-ID: <CAMe9rOojrkHtkWrDgw6Y6O7U5iZLjQph-qX1aH4iqPyupHXjYQ@mail.gmail.com> Subject: V2 [PATCH 2/3] x32: Properly pass long to syscall [BZ #25810] To: Florian Weimer <fw@deneb.enyo.de> Cc: "H.J. Lu via Libc-alpha" <libc-alpha@sourceware.org> Content-Type: multipart/mixed; boundary="0000000000005fa9e005a32ea584" X-Spam-Status: No, score=-19.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> X-List-Received-Date: Mon, 13 Apr 2020 16:33:44 -0000 |
| Series |
V2 [PATCH 2/3] x32: Properly pass long to syscall [BZ #25810]
|
|
Commit Message
H.J. Lu
April 13, 2020, 4:33 p.m. UTC
On Mon, Apr 13, 2020 at 8:17 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * H. J. Lu: > > > On Mon, Apr 13, 2020 at 7:43 AM Florian Weimer <fw@deneb.enyo.de> wrote: > >> > >> * H. J. Lu via Libc-alpha: > >> > >> > diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >> > index 06e39212ee..93dad97c8f 100644 > >> > --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >> > +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >> > @@ -219,12 +219,14 @@ > >> > /* Registers clobbered by syscall. */ > >> > # define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx" > >> > > >> > -/* Create a variable 'name' based on type 'X' to avoid explicit types. > >> > - This is mainly used set use 64-bits arguments in x32. */ > >> > -#define TYPEFY(X, name) __typeof__ ((X) - (X)) name > >> > +/* This also works when X is an array. */ > >> > +#define TYPEFY1(X) __typeof__ ((X) - (X)) > >> > >> I think the comment is now misleading. For an array X, (X) - (X) is > >> of type ptrdiff_t, which is signed, so it triggers sign extension, not > > > > I will update the comments. > > Thanks. > > >> zero extension, in the x32 case. I think this is the reason why my > >> proposed construct went astray in your experiments. > >> > >> Is it really necessary to support arrays for system call arguments? > > > > int > > fexecve (int fd, char *const argv[], char *const envp[]) > > { > > if (fd < 0 || argv == NULL || envp == NULL) > > { > > __set_errno (EINVAL); > > return -1; > > } > > > > #ifdef __NR_execveat > > /* Avoid implicit array coercion in syscall macros. */ > > INLINE_SYSCALL (execveat, 5, fd, "", &argv[0], &envp[0], AT_EMPTY_PATH); > > > > "" is an array. > > If that were the only reason for enforcing pointer decay, I'd say we'd > change the fexecve implementation. But see below. > > >> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > index 2b5cf0c0ea..f3bc7e89df 100644 > >> > --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > @@ -44,6 +44,24 @@ > >> > # define ZERO_EXTEND_5 movl %r8d, %r8d; > >> > # undef ZERO_EXTEND_6 > >> > # define ZERO_EXTEND_6 movl %r9d, %r9d; > >> > +#else /*! __ASSEMBLER__ */ > >> > +# undef ARGIFY > >> > +/* For x32, cast > >> > + 1. pointer to unsigned long (32 bit). > >> > + 2. 32-bit unsigned integer to unsigned long long (64 bit). > >> > + 3. 32-bit integer to long long (64 bit). > >> > + */ > >> > +# define ARGIFY(X) \ > >> > + ({ \ > >> > + _Pragma ("GCC diagnostic push"); \ > >> > + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > >> > + (__builtin_classify_type (X) == 5 \ > >> > + ? (unsigned long) (X) \ > >> > + : ((sizeof (X) <= 4 && (TYPEFY1 (X)) 0 < (TYPEFY1 (X)) -1) \ > >> > + ? (unsigned long long) (X) \ > >> > + : (long long) (X))); \ > >> > + _Pragma ("GCC diagnostic pop"); \ > >> > + }) > >> > #endif /* __ASSEMBLER__ */ > >> > >> This should use long int instead long everywhere. > > > > I will update. > > > >> __builtin_classify_type is undocumented. I'm not sure if its behavior > >> and the constant 5 is actually stable across GCC versions, but the > >> powerpc uses it as well, for a slightly difference purpose (to > >> recognize array arguments, it seems). > > > > include/libc-pointer-arith.h has > > > > /* 1 if 'type' is a pointer type, 0 otherwise. */ > > # define __pointer_type(type) (__builtin_classify_type ((type) 0) == 5) > > > > If __builtin_classify_type ever changes, glibc won't build correctly. > > I see. In this case, can we solely rely on __builtin_classify_type? > I dont't think the inner ternary operator is necessary. > > /* Enforce zero-extension for pointers and array system call > arguments. For integer types, extend to long long int (the full > register) using a regular cast, resulting in zero or sign extension > based on the signed-ness of the original type. */ > #define ARGIFY(X) \ > ({ \ > _Pragma ("GCC diagnostic push"); \ > _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > (__builtin_classify_type (X) == 5 \ > ? (long long int) (uintptr_t) (X) \ > : (long long int) (X))); \ > _Pragma ("GCC diagnostic pop"); \ > }) > > (Untested, sorry.) Here is the updated patch.
Comments
* H. J. Lu via Libc-alpha: > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Mon, 13 Apr 2020 07:02:46 -0700 > Subject: V2 [PATCH 2/3] x32: Properly pass long to syscall [BZ #25810] > > X32 has 32-bit long and pointer with 64-bit off_t. Since x32 psABI > requires that pointers passed in registers must be zero-extended to > 64bit, x32 can share many syscall interfaces with LP64. When a LP64 > syscall with long and unsigned long arguments is used for x32, these > arguments must be properly extended to 64-bit. Otherwise if the upper > 32 bits of the register have undefined value, such a syscall will be > rejected by kernel. This version looks okay to me, thanks.
On 13/04/2020 13:33, H.J. Lu via Libc-alpha wrote: > On Mon, Apr 13, 2020 at 8:17 AM Florian Weimer <fw@deneb.enyo.de> wrote: >> >> * H. J. Lu: >> >>> On Mon, Apr 13, 2020 at 7:43 AM Florian Weimer <fw@deneb.enyo.de> wrote: >>>> >>>> * H. J. Lu via Libc-alpha: >>>> >>>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h >>>>> index 06e39212ee..93dad97c8f 100644 >>>>> --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h >>>>> +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h >>>>> @@ -219,12 +219,14 @@ >>>>> /* Registers clobbered by syscall. */ >>>>> # define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx" >>>>> >>>>> -/* Create a variable 'name' based on type 'X' to avoid explicit types. >>>>> - This is mainly used set use 64-bits arguments in x32. */ >>>>> -#define TYPEFY(X, name) __typeof__ ((X) - (X)) name >>>>> +/* This also works when X is an array. */ >>>>> +#define TYPEFY1(X) __typeof__ ((X) - (X)) >>>> >>>> I think the comment is now misleading. For an array X, (X) - (X) is >>>> of type ptrdiff_t, which is signed, so it triggers sign extension, not >>> >>> I will update the comments. >> >> Thanks. >> >>>> zero extension, in the x32 case. I think this is the reason why my >>>> proposed construct went astray in your experiments. >>>> >>>> Is it really necessary to support arrays for system call arguments? >>> >>> int >>> fexecve (int fd, char *const argv[], char *const envp[]) >>> { >>> if (fd < 0 || argv == NULL || envp == NULL) >>> { >>> __set_errno (EINVAL); >>> return -1; >>> } >>> >>> #ifdef __NR_execveat >>> /* Avoid implicit array coercion in syscall macros. */ >>> INLINE_SYSCALL (execveat, 5, fd, "", &argv[0], &envp[0], AT_EMPTY_PATH); >>> >>> "" is an array. >> >> If that were the only reason for enforcing pointer decay, I'd say we'd >> change the fexecve implementation. But see below. >> >>>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >>>>> index 2b5cf0c0ea..f3bc7e89df 100644 >>>>> --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >>>>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >>>>> @@ -44,6 +44,24 @@ >>>>> # define ZERO_EXTEND_5 movl %r8d, %r8d; >>>>> # undef ZERO_EXTEND_6 >>>>> # define ZERO_EXTEND_6 movl %r9d, %r9d; >>>>> +#else /*! __ASSEMBLER__ */ >>>>> +# undef ARGIFY >>>>> +/* For x32, cast >>>>> + 1. pointer to unsigned long (32 bit). >>>>> + 2. 32-bit unsigned integer to unsigned long long (64 bit). >>>>> + 3. 32-bit integer to long long (64 bit). >>>>> + */ >>>>> +# define ARGIFY(X) \ >>>>> + ({ \ >>>>> + _Pragma ("GCC diagnostic push"); \ >>>>> + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ >>>>> + (__builtin_classify_type (X) == 5 \ >>>>> + ? (unsigned long) (X) \ >>>>> + : ((sizeof (X) <= 4 && (TYPEFY1 (X)) 0 < (TYPEFY1 (X)) -1) \ >>>>> + ? (unsigned long long) (X) \ >>>>> + : (long long) (X))); \ >>>>> + _Pragma ("GCC diagnostic pop"); \ >>>>> + }) >>>>> #endif /* __ASSEMBLER__ */ >>>> >>>> This should use long int instead long everywhere. >>> >>> I will update. >>> >>>> __builtin_classify_type is undocumented. I'm not sure if its behavior >>>> and the constant 5 is actually stable across GCC versions, but the >>>> powerpc uses it as well, for a slightly difference purpose (to >>>> recognize array arguments, it seems). >>> >>> include/libc-pointer-arith.h has >>> >>> /* 1 if 'type' is a pointer type, 0 otherwise. */ >>> # define __pointer_type(type) (__builtin_classify_type ((type) 0) == 5) >>> >>> If __builtin_classify_type ever changes, glibc won't build correctly. >> >> I see. In this case, can we solely rely on __builtin_classify_type? >> I dont't think the inner ternary operator is necessary. >> >> /* Enforce zero-extension for pointers and array system call >> arguments. For integer types, extend to long long int (the full >> register) using a regular cast, resulting in zero or sign extension >> based on the signed-ness of the original type. */ >> #define ARGIFY(X) \ >> ({ \ >> _Pragma ("GCC diagnostic push"); \ >> _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ >> (__builtin_classify_type (X) == 5 \ >> ? (long long int) (uintptr_t) (X) \ >> : (long long int) (X))); \ >> _Pragma ("GCC diagnostic pop"); \ >> }) >> >> (Untested, sorry.) > > Here is the updated patch. > > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > index 2b5cf0c0ea..67335c2a7f 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > @@ -44,6 +44,20 @@ > # define ZERO_EXTEND_5 movl %r8d, %r8d; > # undef ZERO_EXTEND_6 > # define ZERO_EXTEND_6 movl %r9d, %r9d; > +#else /*! __ASSEMBLER__ */ > +# undef ARGIFY > +/* Enforce zero-extension for pointers and array system call arguments. > + For integer types, extend to int64_t (the full register) using a > + regular cast, resulting in zero or sign extension based on the > + signedness of the original type. */ > +# define ARGIFY(X) \ > + ({ \ > + _Pragma ("GCC diagnostic push"); \ > + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > + (__builtin_classify_type (X) == 5 \ > + ? (uintptr_t) (X) : (int64_t) (X)); \ > + _Pragma ("GCC diagnostic pop"); \ > + }) > #endif /* __ASSEMBLER__ */ > > #endif /* linux/x86_64/x32/sysdep.h */ I face a similar issue when fixing BZ#12683 for x32, and one possible solution I found was: #define __SSC(__x) \ ({ \ __syscall_arg_t __arg = sizeof (1 ? (__x) : 0ULL) < 8 \ ? (unsigned long int) (uintptr_t)(__x) \ : (__syscall_arg_t) (__typeof__ ((__x) - (__x))) (__x); \ __arg; \ }) Which was for argument passing in the internal cancellable bridge. Later you suggested a solution I have also devised sometime ago [1]. Couldn't we use any of them instead of explicitly disable/enable compile warnings? [1] https://sourceware.org/legacy-ml/libc-alpha/2017-12/msg00315.html
On Mon, Apr 13, 2020 at 11:35 AM Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > > On 13/04/2020 13:33, H.J. Lu via Libc-alpha wrote: > > On Mon, Apr 13, 2020 at 8:17 AM Florian Weimer <fw@deneb.enyo.de> wrote: > >> > >> * H. J. Lu: > >> > >>> On Mon, Apr 13, 2020 at 7:43 AM Florian Weimer <fw@deneb.enyo.de> wrote: > >>>> > >>>> * H. J. Lu via Libc-alpha: > >>>> > >>>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >>>>> index 06e39212ee..93dad97c8f 100644 > >>>>> --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >>>>> +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h > >>>>> @@ -219,12 +219,14 @@ > >>>>> /* Registers clobbered by syscall. */ > >>>>> # define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx" > >>>>> > >>>>> -/* Create a variable 'name' based on type 'X' to avoid explicit types. > >>>>> - This is mainly used set use 64-bits arguments in x32. */ > >>>>> -#define TYPEFY(X, name) __typeof__ ((X) - (X)) name > >>>>> +/* This also works when X is an array. */ > >>>>> +#define TYPEFY1(X) __typeof__ ((X) - (X)) > >>>> > >>>> I think the comment is now misleading. For an array X, (X) - (X) is > >>>> of type ptrdiff_t, which is signed, so it triggers sign extension, not > >>> > >>> I will update the comments. > >> > >> Thanks. > >> > >>>> zero extension, in the x32 case. I think this is the reason why my > >>>> proposed construct went astray in your experiments. > >>>> > >>>> Is it really necessary to support arrays for system call arguments? > >>> > >>> int > >>> fexecve (int fd, char *const argv[], char *const envp[]) > >>> { > >>> if (fd < 0 || argv == NULL || envp == NULL) > >>> { > >>> __set_errno (EINVAL); > >>> return -1; > >>> } > >>> > >>> #ifdef __NR_execveat > >>> /* Avoid implicit array coercion in syscall macros. */ > >>> INLINE_SYSCALL (execveat, 5, fd, "", &argv[0], &envp[0], AT_EMPTY_PATH); > >>> > >>> "" is an array. > >> > >> If that were the only reason for enforcing pointer decay, I'd say we'd > >> change the fexecve implementation. But see below. > >> > >>>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >>>>> index 2b5cf0c0ea..f3bc7e89df 100644 > >>>>> --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >>>>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >>>>> @@ -44,6 +44,24 @@ > >>>>> # define ZERO_EXTEND_5 movl %r8d, %r8d; > >>>>> # undef ZERO_EXTEND_6 > >>>>> # define ZERO_EXTEND_6 movl %r9d, %r9d; > >>>>> +#else /*! __ASSEMBLER__ */ > >>>>> +# undef ARGIFY > >>>>> +/* For x32, cast > >>>>> + 1. pointer to unsigned long (32 bit). > >>>>> + 2. 32-bit unsigned integer to unsigned long long (64 bit). > >>>>> + 3. 32-bit integer to long long (64 bit). > >>>>> + */ > >>>>> +# define ARGIFY(X) \ > >>>>> + ({ \ > >>>>> + _Pragma ("GCC diagnostic push"); \ > >>>>> + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > >>>>> + (__builtin_classify_type (X) == 5 \ > >>>>> + ? (unsigned long) (X) \ > >>>>> + : ((sizeof (X) <= 4 && (TYPEFY1 (X)) 0 < (TYPEFY1 (X)) -1) \ > >>>>> + ? (unsigned long long) (X) \ > >>>>> + : (long long) (X))); \ > >>>>> + _Pragma ("GCC diagnostic pop"); \ > >>>>> + }) > >>>>> #endif /* __ASSEMBLER__ */ > >>>> > >>>> This should use long int instead long everywhere. > >>> > >>> I will update. > >>> > >>>> __builtin_classify_type is undocumented. I'm not sure if its behavior > >>>> and the constant 5 is actually stable across GCC versions, but the > >>>> powerpc uses it as well, for a slightly difference purpose (to > >>>> recognize array arguments, it seems). > >>> > >>> include/libc-pointer-arith.h has > >>> > >>> /* 1 if 'type' is a pointer type, 0 otherwise. */ > >>> # define __pointer_type(type) (__builtin_classify_type ((type) 0) == 5) > >>> > >>> If __builtin_classify_type ever changes, glibc won't build correctly. > >> > >> I see. In this case, can we solely rely on __builtin_classify_type? > >> I dont't think the inner ternary operator is necessary. > >> > >> /* Enforce zero-extension for pointers and array system call > >> arguments. For integer types, extend to long long int (the full > >> register) using a regular cast, resulting in zero or sign extension > >> based on the signed-ness of the original type. */ > >> #define ARGIFY(X) \ > >> ({ \ > >> _Pragma ("GCC diagnostic push"); \ > >> _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > >> (__builtin_classify_type (X) == 5 \ > >> ? (long long int) (uintptr_t) (X) \ > >> : (long long int) (X))); \ > >> _Pragma ("GCC diagnostic pop"); \ > >> }) > >> > >> (Untested, sorry.) > > > > Here is the updated patch. > > > > > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > > index 2b5cf0c0ea..67335c2a7f 100644 > > --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > > @@ -44,6 +44,20 @@ > > # define ZERO_EXTEND_5 movl %r8d, %r8d; > > # undef ZERO_EXTEND_6 > > # define ZERO_EXTEND_6 movl %r9d, %r9d; > > +#else /*! __ASSEMBLER__ */ > > +# undef ARGIFY > > +/* Enforce zero-extension for pointers and array system call arguments. > > + For integer types, extend to int64_t (the full register) using a > > + regular cast, resulting in zero or sign extension based on the > > + signedness of the original type. */ > > +# define ARGIFY(X) \ > > + ({ \ > > + _Pragma ("GCC diagnostic push"); \ > > + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > > + (__builtin_classify_type (X) == 5 \ > > + ? (uintptr_t) (X) : (int64_t) (X)); \ > > + _Pragma ("GCC diagnostic pop"); \ > > + }) > > #endif /* __ASSEMBLER__ */ > > > > #endif /* linux/x86_64/x32/sysdep.h */ > > I face a similar issue when fixing BZ#12683 for x32, and one possible > solution I found was: > > #define __SSC(__x) \ > ({ \ > __syscall_arg_t __arg = sizeof (1 ? (__x) : 0ULL) < 8 \ > ? (unsigned long int) (uintptr_t)(__x) \ > : (__syscall_arg_t) (__typeof__ ((__x) - (__x))) (__x); \ > __arg; \ > }) This is similar to what I have checked in. > Which was for argument passing in the internal cancellable bridge. > Later you suggested a solution I have also devised sometime ago [1]. > > Couldn't we use any of them instead of explicitly disable/enable > compile warnings? > > [1] https://sourceware.org/legacy-ml/libc-alpha/2017-12/msg00315.html This doesn't work on array, like "".
* H. J. Lu via Libc-alpha: >> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >> > index 2b5cf0c0ea..67335c2a7f 100644 >> > --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h >> > @@ -44,6 +44,20 @@ >> > # define ZERO_EXTEND_5 movl %r8d, %r8d; >> > # undef ZERO_EXTEND_6 >> > # define ZERO_EXTEND_6 movl %r9d, %r9d; >> > +#else /*! __ASSEMBLER__ */ >> > +# undef ARGIFY >> > +/* Enforce zero-extension for pointers and array system call arguments. >> > + For integer types, extend to int64_t (the full register) using a >> > + regular cast, resulting in zero or sign extension based on the >> > + signedness of the original type. */ >> > +# define ARGIFY(X) \ >> > + ({ \ >> > + _Pragma ("GCC diagnostic push"); \ >> > + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ >> > + (__builtin_classify_type (X) == 5 \ >> > + ? (uintptr_t) (X) : (int64_t) (X)); \ >> > + _Pragma ("GCC diagnostic pop"); \ >> > + }) >> > #endif /* __ASSEMBLER__ */ >> > >> > #endif /* linux/x86_64/x32/sysdep.h */ >> >> I face a similar issue when fixing BZ#12683 for x32, and one possible >> solution I found was: >> >> #define __SSC(__x) \ >> ({ \ >> __syscall_arg_t __arg = sizeof (1 ? (__x) : 0ULL) < 8 \ >> ? (unsigned long int) (uintptr_t)(__x) \ >> : (__syscall_arg_t) (__typeof__ ((__x) - (__x))) (__x); \ >> __arg; \ >> }) > > This is similar to what I have checked in. I think Adhemerval specifically meant the additional cast using __typeof__ to suppress the warning (which I did not think of). The _Pragma construct as written is a Clang porting hazard because it causes the statement expression to return void (which is a Clang/GCC discrepancy). Rather than introducing a temporary, using the __typeof__ hack is probably the better way of fixing it.
On Mon, Apr 13, 2020 at 2:13 PM Florian Weimer <fw@deneb.enyo.de> wrote: > > * H. J. Lu via Libc-alpha: > > >> > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > index 2b5cf0c0ea..67335c2a7f 100644 > >> > --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h > >> > @@ -44,6 +44,20 @@ > >> > # define ZERO_EXTEND_5 movl %r8d, %r8d; > >> > # undef ZERO_EXTEND_6 > >> > # define ZERO_EXTEND_6 movl %r9d, %r9d; > >> > +#else /*! __ASSEMBLER__ */ > >> > +# undef ARGIFY > >> > +/* Enforce zero-extension for pointers and array system call arguments. > >> > + For integer types, extend to int64_t (the full register) using a > >> > + regular cast, resulting in zero or sign extension based on the > >> > + signedness of the original type. */ > >> > +# define ARGIFY(X) \ > >> > + ({ \ > >> > + _Pragma ("GCC diagnostic push"); \ > >> > + _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \ > >> > + (__builtin_classify_type (X) == 5 \ > >> > + ? (uintptr_t) (X) : (int64_t) (X)); \ > >> > + _Pragma ("GCC diagnostic pop"); \ > >> > + }) > >> > #endif /* __ASSEMBLER__ */ > >> > > >> > #endif /* linux/x86_64/x32/sysdep.h */ > >> > >> I face a similar issue when fixing BZ#12683 for x32, and one possible > >> solution I found was: > >> > >> #define __SSC(__x) \ > >> ({ \ > >> __syscall_arg_t __arg = sizeof (1 ? (__x) : 0ULL) < 8 \ > >> ? (unsigned long int) (uintptr_t)(__x) \ > >> : (__syscall_arg_t) (__typeof__ ((__x) - (__x))) (__x); \ > >> __arg; \ > >> }) > > > > This is similar to what I have checked in. > > I think Adhemerval specifically meant the additional cast using > __typeof__ to suppress the warning (which I did not think of). His patch: https://sourceware.org/pipermail/libc-alpha/2020-April/112520.html also needs to suppress -Wpointer-to-int-cast > The _Pragma construct as written is a Clang porting hazard because it > causes the statement expression to return void (which is a Clang/GCC > discrepancy). Rather than introducing a temporary, using the > __typeof__ hack is probably the better way of fixing it. I don't think it works on array.
From fd210e7ada784986f5f5bff3e4892478fc998fea Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 13 Apr 2020 07:02:46 -0700
Subject: V2 [PATCH 2/3] x32: Properly pass long to syscall [BZ #25810]
X32 has 32-bit long and pointer with 64-bit off_t. Since x32 psABI
requires that pointers passed in registers must be zero-extended to
64bit, x32 can share many syscall interfaces with LP64. When a LP64
syscall with long and unsigned long arguments is used for x32, these
arguments must be properly extended to 64-bit. Otherwise if the upper
32 bits of the register have undefined value, such a syscall will be
rejected by kernel.
Enforce zero-extension for pointers and array system call arguments.
For integer types, extend to int64_t (the full register) using a
regular cast, resulting in zero or sign extension based on the
signedness of the original type.
For
void *mmap(void *addr, size_t length, int prot, int flags,
int fd, off_t offset);
we now generate
0: 41 f7 c1 ff 0f 00 00 test $0xfff,%r9d
7: 75 1f jne 28 <__mmap64+0x28>
9: 48 63 d2 movslq %edx,%rdx
c: 89 f6 mov %esi,%esi
e: 4d 63 c0 movslq %r8d,%r8
11: 4c 63 d1 movslq %ecx,%r10
14: b8 09 00 00 40 mov $0x40000009,%eax
19: 0f 05 syscall
That is
1. addr is unchanged.
2. length is zero-extend to 64 bits.
3. prot is sign-extend to 64 bits.
4. flags is sign-extend to 64 bits.
5. fd is sign-extend to 64 bits.
6. offset is unchanged.
For int arguments, since kernel uses only the lower 32 bits and ignores
the upper 32 bits in 64-bit registers, these work correctly.
Tested on x86-64 and x32. There are no code changes on x86-64.
---
sysdeps/unix/sysv/linux/x86_64/sysdep.h | 15 +++++++++------
sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h | 14 ++++++++++++++
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
index 06e39212ee..3596273c94 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
@@ -219,12 +219,15 @@
/* Registers clobbered by syscall. */
# define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx"
-/* Create a variable 'name' based on type 'X' to avoid explicit types.
- This is mainly used set use 64-bits arguments in x32. */
-#define TYPEFY(X, name) __typeof__ ((X) - (X)) name
-/* Explicit cast the argument to avoid integer from pointer warning on
- x32. */
-#define ARGIFY(X) ((__typeof__ ((X) - (X))) (X))
+/* NB: This also works when X is an array. For an array X, type of
+ (X) - (X) is ptrdiff_t, which is signed, since size of ptrdiff_t
+ == size of pointer, cast is a NOP. */
+#define TYPEFY1(X) __typeof__ ((X) - (X))
+/* Explicit cast the argument. */
+#define ARGIFY(X) ((TYPEFY1 (X)) (X))
+/* Create a variable 'name' based on type of variable 'X' to avoid
+ explicit types. */
+#define TYPEFY(X, name) __typeof__ (ARGIFY (X)) name
#undef INTERNAL_SYSCALL
#define INTERNAL_SYSCALL(name, nr, args...) \
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
index 2b5cf0c0ea..67335c2a7f 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
@@ -44,6 +44,20 @@
# define ZERO_EXTEND_5 movl %r8d, %r8d;
# undef ZERO_EXTEND_6
# define ZERO_EXTEND_6 movl %r9d, %r9d;
+#else /*! __ASSEMBLER__ */
+# undef ARGIFY
+/* Enforce zero-extension for pointers and array system call arguments.
+ For integer types, extend to int64_t (the full register) using a
+ regular cast, resulting in zero or sign extension based on the
+ signedness of the original type. */
+# define ARGIFY(X) \
+ ({ \
+ _Pragma ("GCC diagnostic push"); \
+ _Pragma ("GCC diagnostic ignored \"-Wpointer-to-int-cast\""); \
+ (__builtin_classify_type (X) == 5 \
+ ? (uintptr_t) (X) : (int64_t) (X)); \
+ _Pragma ("GCC diagnostic pop"); \
+ })
#endif /* __ASSEMBLER__ */
#endif /* linux/x86_64/x32/sysdep.h */
--
2.25.2