From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109449 invoked by alias); 18 Jul 2018 21:06:34 -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 109425 invoked by uid 89); 18 Jul 2018 21:06:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=Sometimes X-HELO: mail-oi0-f66.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KsTnCxOp+xHCjQAnh2sgnkbRw8W3P1URW+deSR3O+FI=; b=cGDvMOMldPOp91s+NXzsT8tVuNw5awaEnuNdoO8NkQ6tj7XU1cQzClLSfGSCLVMbhO eUNuE27GI5df5Np/4WRjgEcr9j5KqJ9efUViUr+R9TC/t89vsDJ9K83oVVEdm6LyaMRf 1er2RrLAB+xoDBry5KB08HIw1IS8/vhYUGQG//HdIcqDCRBp4k04M+KPSno0dHrmhAEb yfT1nEeijnCm2Edb3o1gsKDBWqjDcFgjrSWBAzE2T0mnjg5hBUq8FOvJc0zEbhs+5yX1 IhZmQobxvqw+FLSaMEE3pHQz/aO6Vofw0Q/hyT8Ix4WjprcebKUcts4BhdztrAKjWhCM r9dg== MIME-Version: 1.0 In-Reply-To: References: <21dc4ff1-4729-d448-d2d8-0a58f51546e7@redhat.com> From: "H.J. Lu" Date: Wed, 18 Jul 2018 21:06:00 -0000 Message-ID: Subject: Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598] To: Joseph Myers Cc: "Carlos O'Donell" , GNU C Library Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-07/txt/msg00586.txt.bz2 On Wed, Jul 18, 2018 at 1:35 PM, Joseph Myers wrote: > 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. The second part of the CET patches depends on the the first couple CET patches. But here are no dependencies within the second part. > 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. I prefer a separate small documentation patch since not everyone has time to review a big patch. But people may have time to review a small documentation patch. Still my small documentation patch got no review in a month. > 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. > All my CET changes have been tested with glibc "make check" and "make xcheck": 1. On non-CET processors under non-CET kernel: a. With and without --enable-cet b. For i686, x86_64 and x32. 2. On 64-bit CET SDV under CET kernel with --enable-cet a. Only x86_64 and x32 tested. b. i686 isn't tested since 64-bit CET SDV doesn't support i686. 3. scripts/build-many-glibcs.py on x86-64. H.J.