This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 <joseph@codesourcery.com> wrote:
> 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.

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]