From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27690 invoked by alias); 18 Jul 2018 20:36:03 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 27672 invoked by uid 89); 18 Jul 2018 20:36:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=exceptionally, relating, extent X-HELO: relay1.mentorg.com Date: Wed, 18 Jul 2018 20:36:00 -0000 From: Joseph Myers To: "H.J. Lu" CC: Carlos O'Donell , GNU C Library Subject: Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598] In-Reply-To: Message-ID: References: <21dc4ff1-4729-d448-d2d8-0a58f51546e7@redhat.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2018-07/txt/msg00580.txt.bz2 On Tue, 17 Jul 2018, H.J. Lu wrote: > On Tue, Jul 17, 2018 at 5:22 PM, Carlos O'Donell wrote: > > On 07/17/2018 06:53 PM, Joseph Myers wrote: > >> On Mon, 16 Jul 2018, Carlos O'Donell wrote: > >> > >>> This version is OK. > >> > >> The documentation omissions need to be fixed for the release. I see no > >> install.texi update / INSTALL regeneration for the new configure option > >> and no NEWS update for the new feature and associated configure option. > > Hi Joseph, can you review > > https://sourceware.org/ml/libc-alpha/2018-06/msg00328.html > > Thanks. I'd like to be clear on a general principle relating to patch series: Sometimes patches in a series can go in independently, as and when approved. But sometimes the splits of patches into a series are purely for the convenience of review, and the patches are not intended to be committed independently. That's obviously the case if earlier patches break the build on some architectures and later ones fix it, for example - in that case, to keep bisectability, it's required to squash the patches into a single commit. But even if bisectability isn't broken and so separate commits pushed at the same time are OK, in my view documentation is a fundamental part of a new feature patch. If it's split out for review (which is something I'd discourage, I prefer to see the documentation and tests included in the main patch together with the implementation code), that does *not* mean an approval of the main patch makes it OK to commit without the documentation - just as an approval of a patch known to break the build doesn't make it OK to commit until the later patches fixing the build are also approved, and the whole set squashed together for commit. Rather, I'd consider any approval of a patch whose documentation was split out to be implicitly conditional on the documentation also being approved and checked in at the same time (even if not in the same commit), to ensure we keep the invariant of features in glibc being documented in glibc. For an approval to be valid without the documentation patch I think it would need to say so explicitly, and explain why - and take extra care to ensure there really is consensus for such a deviation from the normal rules on documenting new features. (Just as, exceptionally, a patch known to break some architecture might be approved for commit, if there is no active architecture maintainer for the problem architecture and that architecture is likely to be obsoleted.) > Here is a patch to test mixing CET executables and non-CET libraries: > > https://sourceware.org/ml/libc-alpha/2018-07/msg00526.html Could you please give a detailed description of the testing you have run for the CET changes in general? The *minimum* for any nontrivial patch submission to glibc code should be a statement of the platform for which the glibc testsuite was run, or description of other testing done (and if we receive patch submissions that don't state even that, we should ask for it). In the case of CET, a complicated feature with multiple configurations involved, and dependencies on hardware and kernel support, I'd expect a rather more extended discussion, maybe a few paragraphs, of the different cases tested and what those tests actually cover. I'd guess those cases include various combinations of: * i686, x86_64 and x32 glibc builds. * glibc builds with and without --enable-cet. * Tests run on kernels both with and without CET support. * Tests run on both CET and non-CET hardware (or simulators for such hardware). But that needs a proper explanation, posted to the libc-alpha list, of what you did actually test (and each subsequent patch submission should have such an explanation, of what was tested for that patch) - and, as I said, of to what extent that really covers the functionality of the code. -- Joseph S. Myers joseph@codesourcery.com