* Re: [PATCH 0/2] AArch64 glibc port [not found] <CABXK9ncT6kQQ_YR1kyBMRfg6uAu5LryX33soiAtURN6x-mnkWQ@mail.gmail.com> @ 2012-10-01 15:00 ` Carlos O'Donell 2012-10-03 12:04 ` Joseph S. Myers 0 siblings, 1 reply; 3+ messages in thread From: Carlos O'Donell @ 2012-10-01 15:00 UTC (permalink / raw) To: Marcus Shawcroft; +Cc: libc-alpha, Joseph S. Myers, libc-ports 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 0/2] AArch64 glibc port 2012-10-01 15:00 ` [PATCH 0/2] AArch64 glibc port Carlos O'Donell @ 2012-10-03 12:04 ` Joseph S. Myers 2012-10-03 12:27 ` Carlos O'Donell 0 siblings, 1 reply; 3+ messages in thread From: Joseph S. Myers @ 2012-10-03 12:04 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Marcus Shawcroft, libc-alpha, libc-ports 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 0/2] AArch64 glibc port 2012-10-03 12:04 ` Joseph S. Myers @ 2012-10-03 12:27 ` Carlos O'Donell 0 siblings, 0 replies; 3+ messages in thread From: Carlos O'Donell @ 2012-10-03 12:27 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Marcus Shawcroft, libc-alpha, libc-ports 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-10-03 12:27 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CABXK9ncT6kQQ_YR1kyBMRfg6uAu5LryX33soiAtURN6x-mnkWQ@mail.gmail.com> 2012-10-01 15:00 ` [PATCH 0/2] AArch64 glibc port Carlos O'Donell 2012-10-03 12:04 ` Joseph S. Myers 2012-10-03 12:27 ` Carlos O'Donell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).