Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598]

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 17 Jul 2018, H.J. Lu wrote:

> On Tue, Jul 17, 2018 at 5:22 PM, Carlos O'Donell <carlos@redhat.com> 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