This is the mail archive of the
mailing list for the glibc project.
Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598]
On Wed, Jul 18, 2018 at 1:35 PM, Joseph Myers <email@example.com> wrote:
> On Tue, 17 Jul 2018, H.J. Lu wrote:
>> On Tue, Jul 17, 2018 at 5:22 PM, Carlos O'Donell <firstname.lastname@example.org> 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
> 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:
> 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
> 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.