From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x230.google.com (mail-oi1-x230.google.com [IPv6:2607:f8b0:4864:20::230]) by sourceware.org (Postfix) with ESMTPS id F05533858D20 for ; Tue, 30 May 2023 10:46:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F05533858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oi1-x230.google.com with SMTP id 5614622812f47-39831cb47fbso2852728b6e.1 for ; Tue, 30 May 2023 03:46:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685443594; x=1688035594; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=lFpB2EykHQQzuRnIMPvwzfIWPKaZm94NZY5jlQQSsp0=; b=deEslgIR3D/pUc3N84RokUPTnkDKMxVQpP6OTmQmbBMviukymcwyWgBVUEUJm6UsAS uvDsL1S5aNw7NjbIICVo6/B0vSaxnW7yFlQ+cCrXipodv1YkkvfSDscnIUkFn19SB2f5 jYnoiH4cYJssySD4mB70ARGd2SDyO5vY21lnl51AQ1AdRkvJMwHQFmOpE5lRlbaOKIyo aa/OXLO6OTaKhKaYlm2Ttm7qp+DSyTSuj4FQR1aN01U97sSztvuyJ3hGB9fSU7uv1wsO mXk5hG6mcbKYYGaeJFWEae1zSSzASMzzz6Z0vb79Sk585qJTlTbBzs325/QGJ2cUPyBM KUaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685443594; x=1688035594; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=lFpB2EykHQQzuRnIMPvwzfIWPKaZm94NZY5jlQQSsp0=; b=gTb3eLKvtZ6PfcPL90QLv7XfCQhG2AE1bnz0ttNP+a6EweDGy2D0PCH9c8n9uk0k9J gw6HvOYKH55WnAn/JQqxac7Ft+owpLNjh/Bi0pB2cpU9rayM2YJFaADYLlE/TrKpUwBC WShraKOIih9OK0WdP7eWl7olL4IuCn2BpH2WfE1nNwfbGBGWQ5hncurN0F4zZKtb61X0 chcb9nohvOPQvv/a0sWaiABn7ScVrN39xdQ38fZJ4GS7xrDm0aftaW50y43FJ1kiGudL ankVKTDxGu/y735GVON2mlLqidSG+9lVDC+EyRrqJpUxfWf5MVsNAFguuvAZyg103Dy2 1jQQ== X-Gm-Message-State: AC+VfDzdKFb9p6gfo+DVTQ6s+9bZr+966gb8/fHlnAy94fReURvom8CE F525mNhuvAhmI86hrICNqC+HrMXTzOaduBeCU32kK6q+2Rw= X-Google-Smtp-Source: ACHHUZ6BMh8yv2/YjcdHReR14WGCZaM+xijmAGcyWK5NFMVZ08SyoK9Z8Wkhl4JyLdvFTlg395SE92jMLGfnlJloNgg= X-Received: by 2002:aca:1a13:0:b0:399:169:75d8 with SMTP id a19-20020aca1a13000000b00399016975d8mr772654oia.35.1685443594217; Tue, 30 May 2023 03:46:34 -0700 (PDT) MIME-Version: 1.0 References: <20230528172013.73111-1-bugaevc@gmail.com> <87wn0qw88m.fsf@oldenburg.str.redhat.com> In-Reply-To: <87wn0qw88m.fsf@oldenburg.str.redhat.com> From: Sergey Bugaev Date: Tue, 30 May 2023 13:46:23 +0300 Message-ID: Subject: Re: [PATCH v2 0/3] fcntl fortification To: Florian Weimer Cc: Siddhesh Poyarekar , libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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 Tue, May 30, 2023 at 11:09=E2=80=AFAM Florian Weimer wrote: > > 2. There is a __fcntl_types_compatible () macro which is a thin wrapper > > over __builtin_types_compatible_p () in plain C, and uses an > > std::is_same_v-like check (using partial template specialization) in > > C++. Importantly, it uses __typeof () even in C++ (not decltype ()), > > because we don't want the extra references appended to our type. For > > example, we want 'int', not 'const int &' or 'int &&'. > > I think you should avoid using __typeof, otherwise we need to add > another GCC check. If you need to use decltype, you'll have to add a > __cplusplus version check. I don't think I understand your point about the GCC check, could you please expand? __typeof seems to have already been supported in very ancient GCC versions [0], certainly much earlier than __VA_OPT__ support has appeared. Clang supports typeof too. [0]: https://gcc.gnu.org/onlinedocs/gcc-2.95.3/gcc_4.html#SEC68 > > 5. Here's the fcntl () macro in all of its horrible glory: > > > > #define fcntl(fd, cmd, ...) > > (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd) > > I think we should avoid the new __fcntl_2 symbol because it an > unnecessary optimization. And again I don't think I understand your point :| Could you please expand here as well? What's the (unnecessary) optimization here? __fcntl_2 is required to do runtime checking of whether the runtime value of CMD indeed does not require a third argument. __fcntl_2_inline does the check at compile time if possible, and either emits an error or calls the __fcntl_alias, otherwise it calls through to the __fcntl_2. This all could be done inside the fcntl macro as well, I just chose to move it to the inline function because: - this is way more readable, the macros are unreadable enough already; - this way it can be shared between fcntl and fcntl64; - it *is* possible to do this part in a function rather than a macro, unlike everything else, because it doesn't require us to pass through the untyped 'arg'. > It would very nice if we could generate the appropriate warning for C > (-Wincompatible-pointer-types). This is what I tried to do, but it > might actually be impossible. > > Should we generate errors for C++? It requires compatible pointer > types, after all. So the way I think about this, what this is doing is not making fcntl into a proper type-safe overloaded function, but adding some safeguards on top of the existing vararg definition that catch some mistakes. The diagnostics emitted are different, and this is fine. Another instance of this: you get the "error: call to '__fcntl_missing_arg' declared with attribute error" and not the error you would otherwise get from GCC upon forgetting a required function argument ("error: too few arguments to function 'foo'"). I would prefer to keep type mismatch a warning in C++ too (because this is best-effort additional safeguards, not a real type system), but this would be easy to change if you want me to. > > I have only really tested with (modern) GCC. I briefly checked with > > Clang, but the fortification doesn't seem to get enabled at all; perhap= s > > it's failing some other check. > > Which Clang version? $ clang --version clang version 16.0.4 (Fedora 16.0.4-1.fc38) I now checked again, and what's happening is Clang doesn't seem to have __builtin_va_arg_pack -- which is no longer required for the fcntl fortification, so this would be resolved once I move it to a separate fcntl3.h header. Sergey