This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598]
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