From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 6AE5C3858D39 for ; Mon, 27 Mar 2023 14:04:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6AE5C3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679925874; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jUbp9WkFA/rh2UgXcNk8nk0SdadchV+xXZcAQrN1l6A=; b=YdA6DKequ4hq5YoLyIKWxG8fS+d8z3ek0qDVHKs8XPBTJRr54MlOlN7HD63g/AfXEP+EFz DBqm/pu3QYA+xs/U5kksCIXCLrbnQZRlcy7w1do2hltc2ZNqYz6t9KXja2St1rDHnCV79M P/MiP4G/svFaIGSmHg9SRvm5HGRCx2c= Received: from mail-yw1-f198.google.com (mail-yw1-f198.google.com [209.85.128.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-127--b8Wy0bCN06BUHhrpm1Wgg-1; Mon, 27 Mar 2023 10:04:26 -0400 X-MC-Unique: -b8Wy0bCN06BUHhrpm1Wgg-1 Received: by mail-yw1-f198.google.com with SMTP id 00721157ae682-536cb268ab8so89139997b3.17 for ; Mon, 27 Mar 2023 07:04:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679925855; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jUbp9WkFA/rh2UgXcNk8nk0SdadchV+xXZcAQrN1l6A=; b=sNsvNRFSYArz9C+k0FYO/viwAnk5Qt86TigJs09f+t/p4sSy2UjWAnoMaCaXz4McjQ slB0KoHISx5nmv+2dl883/uD0qVNP31nd/YbgyMqHUIkyXfckdnUws5g8DULNiPz/hWn GDm3R2zlunb0feKmaWTzXkQJ+yiIOxB2uXP6OKqEQGTjvqV2sia4a8k8rnqdPwSqQb4x WaUmsSEvDuvuTQo+f6O4gQzxNXht0BW3td3RurM86DxHgN+uEaEPs6kCRrDqG4DSaSTE l2+PuD6Vv+X885BmLm/sStkw6jNivsIsl13Dfuf34NncSnhxAovnLuFbOvDS48b+QQuF 4T5Q== X-Gm-Message-State: AAQBX9f78aP+E7ndYTVSNZpx8dntGIsJh8Ioo0YVfB9D8ukuIugNJAQR Dg+5Rid0P0LXXSGMHxD1o/YRaDag3gjku3lEOLVhMhvszHw0gqtppgLNa/6uuikefImsFpZfUD/ S8KUkQcHEt7Va7XhNgPio X-Received: by 2002:a05:7500:3d84:b0:fe:709f:c5c8 with SMTP id le4-20020a0575003d8400b000fe709fc5c8mr327352gab.53.1679925854970; Mon, 27 Mar 2023 07:04:14 -0700 (PDT) X-Google-Smtp-Source: AKy350bQOWZP6Zz1usax7dnjEe+5kjYfuE17hC/EKjBPaa4K2gSDuOQbrwxOVjZWcfkF7m3lSPQ4lw== X-Received: by 2002:a05:7500:3d84:b0:fe:709f:c5c8 with SMTP id le4-20020a0575003d8400b000fe709fc5c8mr327316gab.53.1679925854510; Mon, 27 Mar 2023 07:04:14 -0700 (PDT) Received: from [192.168.0.241] ([198.48.244.52]) by smtp.gmail.com with ESMTPSA id n20-20020a81bd54000000b00545a08184e5sm1961915ywk.117.2023.03.27.07.04.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 27 Mar 2023 07:04:13 -0700 (PDT) Message-ID: <98295b42-c079-c32c-31d6-bb45013342ee@redhat.com> Date: Mon, 27 Mar 2023 10:04:11 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v2 0/5] linux: Avoid va_list for generic syscall wrappers if possible To: Xi Ruoyao , libc-alpha@sourceware.org Cc: caiyinyu , Wang Xuerui , Adhemerval Zanella Netto , Andreas Schwab , Florian Weimer References: <20230325140815.4170296-1-xry111@xry111.site> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <20230325140815.4170296-1-xry111@xry111.site> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/25/23 10:08, Xi Ruoyao via Libc-alpha wrote: > Currently GCC generates highly sub-optimal code on architectures where > the calling convention prefers registers for arugment passing. This is > GCC PR100955. While it's technically a missed-optimization in GCC, it > seems not trivial to fix (I've not seen any compiler which can optimize > this properly yet). I'm glad to see we have a gcc PR open for this. We should be working to improve the compiler instead of working around the issue in glibc. > As the generic Linux syscall wrappers often uses a fixed number of > arguments, and "overreading" the variable arguments is usually safe > (fcntl etc. is already doing this), we can pretend these wrappers were > declared with named arguments and make the compiler do right thing. > Add a macro __ASSUME_SYSCALL_NAMED_WORKS to control this, which should > be defined if we can safely replace "..." with several named arguments. > Use an internal function prototype with named arguments if it's defined, > for fcntl64, fcntl_nocancel, ioctl, mremap, open64, open64_nocancel, > openat64, openat64_nocancel, prctl, ptrace, and generic syscall() > wrapper. I've not changed open* without "64" because I don't have a > test platform. shm_ctl is also not changed because it contains > aggregate variable arugment which is more tricky than integers or > pointers. > Define this macro for LoongArch, x86-64, and AArch64. This should be > also suitable for some other architectures (I think it will be fine on > RISC-V) but again I don't have a test platform. > > This is the first time I make such a large change in Glibc so it's > likely I've done something wrong. Please correct me :). There are few things that I see are wrong, and I'll list them out here at the top-level: (1) Error prone macro usage. #ifdef foo /* Some Stuff. */ #else /* Other stuff. */ #endif (a) Always define foo, either 0 or 1. (b) Always use #if foo / #else (2) Public declarations must match internal declarations. The biggest problem I see here is that the public declaration of the functions you are changing are all variadic, and so my strong opinion is that we should not change the internal definition of these functions to be non-variadic. I expect there are going to be knock-on effects with developer tooling that expects the implementation not to over-read e.g. valgrind looking at reading of registers that have undefined values. --- In summary, I think this is a compiler problem, and that working around this in glibc is going to result in: - Odd corner case ABI issues between public declarations of variadic functions and internal non-variadic definitions. - Poorer testing of #else code that uses variadic arguments, as the public interface requires. I don't support going in this direction. Is there an alternative that could generate better code that doesn't go this way? > Xi Ruoyao (5): > linux: Add __ASSUME_SYSCALL_NAMED_WORKS to allow avoiding va_list for > generic syscall > linux: [__ASSUME_SYSCALL_NAMED_WORKS] Avoid using va_list for various > syscall wrappers > LoongArch: Define __ASSUME_SYSCALL_NAMED_WORKS for Linux > x86_64: Define __ASSUME_SYSCALL_NAMED_WORKS for Linux > aarch64: Define __ASSUME_SYSCALL_NAMED_WORKS for Linux > > include/fcntl.h | 32 ++++++++----- > .../unix/sysv/linux/aarch64/kernel-features.h | 9 ++++ > sysdeps/unix/sysv/linux/fcntl64.c | 40 +++++++++++++---- > sysdeps/unix/sysv/linux/fcntl_nocancel.c | 30 ++++++++++--- > sysdeps/unix/sysv/linux/ioctl.c | 38 ++++++++++++---- > .../sysv/linux/loongarch/kernel-features.h | 29 ++++++++++++ > sysdeps/unix/sysv/linux/mremap.c | 36 +++++++++++++-- > sysdeps/unix/sysv/linux/not-cancel.h | 8 ++-- > sysdeps/unix/sysv/linux/open64.c | 35 ++++++++++++--- > sysdeps/unix/sysv/linux/open64_nocancel.c | 29 ++++++++++-- > sysdeps/unix/sysv/linux/openat64.c | 30 +++++++++++-- > sysdeps/unix/sysv/linux/openat64_nocancel.c | 29 ++++++++++-- > sysdeps/unix/sysv/linux/prctl.c | 23 +++++++++- > sysdeps/unix/sysv/linux/ptrace.c | 45 ++++++++++++++----- > sysdeps/unix/sysv/linux/syscall.c | 35 +++++++++++---- > .../unix/sysv/linux/x86_64/kernel-features.h | 9 ++++ > 16 files changed, 382 insertions(+), 75 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/loongarch/kernel-features.h > -- Cheers, Carlos.