Re: [PATCH 0/2] AArch64 glibc port

Re: [PATCH 0/2] AArch64 glibc port

[Carlos O'Donell]

On 10/1/2012 5:50 AM, Marcus Shawcroft wrote:
> This series of patches provides glibc support for ARM AArch64.
> 
> The patches are split into two, the first provides the glibc/ports
> component, the second provides
> support in elf/elf.h.  The port requires that scripts/config.guess and
> scritps/config.sub are updated
> from GNU config.
> 
> This port has a couple of limitations that I would like to highlight now:
> 
> * There is no IFUNC support yet.
> 
> * There is no support for profiling yet.
> 
> * The kernel ptrace() interface for  hardware break point and watch
> point registers has
> recently changed and the necessary adjustment has not yet been made to glibc.

Marcus,

Thank you for your contribution and welcome to the community!

You don't say that you'd like the patches to be included in glibc, but
I assume that you would like this to be the end result of the review
process.

If you are interested in having the patches applied to glibc trunk, and
in turn become part of the 2.17 release in December, then we'll need your
help to make the review quick and painless.

I've included libc-ports@sourceware.org since ARM is a ports machine and
discussions should be sent there.

(1) Copyright assignment.

I see that you've submitted these patches from your @linaro.org address.

Linaro has a blanket copyright assignment for glibc.

ARM has *no* blanket copyright assignment for glibc.

Were you employed by ARM or Linaro when you created this work?

My worry here is that you actually work for ARM, are on "loan" to Linaro,
and therefore *don't* have a copyright assignment in place for any of
this work.

Any help clarifying the copyright status of this work would be very
helpful.

(2) Split up your patches.

One huge patch is impossible to review.

I can see about 25-30 groupings of files that relate and your patch should
be split up along those lines.

For example:
- the ABI base-lines should be a patch.
- the fe* functions can be a patch.
- the fpu/* functions can be a patch.
- configury bits can be a patch.
- Makefile hunks should be split up into the patches that need them.
etc. etc.

Please also include a description of each patch following the
contribution checklist, speaking of which...

(3) Review the Contribution Checklist.

Could you also go through the contribution checklist, and fix up all the small
mistakes?

For example you should be merging all of the copyright years following the
rules outlined in the checklist.

http://sourceware.org/glibc/wiki/Contribution%20checklist

(4) Testing?

You'll need tell us how you tested this and what if any changes this has
on the non-AArch64 support for ARM.

(5) If you need any help don't hesitate to ask.

A good reference for a new port submission would be Tilera, which was
just recently added to glibc. Go back and read through the mailing list
to see how that was handled by Chris Metcalf. Chris did a great job and
the integration was exceptionally smooth.

Don't hesitate to ask for help, just mail the list.

Cheers,
Carlos.
-- 
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026

Re: [PATCH 0/2] AArch64 glibc port

[Joseph S. Myers]

On Mon, 1 Oct 2012, Carlos O'Donell wrote:

> (2) Split up your patches.
> 
> One huge patch is impossible to review.
> 
> I can see about 25-30 groupings of files that relate and your patch should
> be split up along those lines.

No, absolutely do not split it up artificially.  Patch submissions should 
be self-contained.  The port (the new sysdeps files) should be submitted 
as a single patch, to libc-ports only (not libc-alpha), with appropriate 
rationale for any design choices it seems appropriate to draw attention 
to.  One patch is much easier to review, and spot whether anything is 
missing in it, than 25-30.

The only things to split up are any libc changes required (generally, 
anything outside of ports/sysdeps/.../aarch64/), where each logical change 
should be sent in its own self-contained patch submission with its own 
self-contained rationale.

(If any bits of the port are *not needed at all* for a functional port - 
if they are purely optimized functions - they can be omitted from the 
submission and dealt with later afer the port is in.  But still don't send 
multiple ports patches in the initial submission.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 0/2] AArch64 glibc port

[Carlos O'Donell]

On 10/3/2012 8:04 AM, Joseph S. Myers wrote:
> On Mon, 1 Oct 2012, Carlos O'Donell wrote:
> 
>> (2) Split up your patches.
>>
>> One huge patch is impossible to review.
>>
>> I can see about 25-30 groupings of files that relate and your patch should
>> be split up along those lines.
> 
> No, absolutely do not split it up artificially.  Patch submissions should 
> be self-contained.  The port (the new sysdeps files) should be submitted 
> as a single patch, to libc-ports only (not libc-alpha), with appropriate 
> rationale for any design choices it seems appropriate to draw attention 
> to.  One patch is much easier to review, and spot whether anything is 
> missing in it, than 25-30.
> 
> The only things to split up are any libc changes required (generally, 
> anything outside of ports/sysdeps/.../aarch64/), where each logical change 
> should be sent in its own self-contained patch submission with its own 
> self-contained rationale.
> 
> (If any bits of the port are *not needed at all* for a functional port - 
> if they are purely optimized functions - they can be omitted from the 
> submission and dealt with later afer the port is in.  But still don't send 
> multiple ports patches in the initial submission.)

I agree with your comments about splitting out libc changes, logically
orthogonal changes, and optimization changes not needed for the initial port.

While I disagree about splitting up patches, given that you are the ARM
machine maintainer and have stated your preference, please feel free to
review the current patch.

The single long patch can be just as easily reconstructed by applying all
the patches and reviewing the diff yourself while still providing a logically
split up set of emails with rationale for each patch.

I find it much easier to do a piece-meal review of each mini-patch this way
*followed* by a larger overview review afterwards based on a test build
after having applied the patches.

Personally I see no benefit to having the whole patch a single entity versus 
having it split up with individual rationale listed separately.

Cheers,
Carlos.
-- 
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026