| Message ID | CAGQ9bdzqT1EyXYMwACrHpPU=vPjM_b72LJRjb7BW_OzJRXG3bw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 20, 2014 at 04:56:27PM +0400, Konstantin Serebryany wrote: > Hi, > > This patch is the first in the series of patches that remove nested > functions from glibc. > Rationale: nested functions is a non-standard language feature; > removing nested functions > will allow to compile glibc with compilers other than GCC and thus > benefit from other compilers > and code analysis tools. > Discussed previously here: > https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html > No functionality change intended. > > (If approved, this will be my first commit to glibc, so some more > actions may be required) I believe initially you'll need one of us to commit for you, which we will do once the patch is found suitable. You may want to review the contribution checklist as well: http://sourceware.org/glibc/wiki/Contribution%20checklist > > 2014-05-20 Kostya Serebryany <konstantin.s.serebryany@gmail.com> Two spaces between the date and name, and name and email. An extra newline between the ChangeLog content and header. > * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function. > (b64_from_24bit): New function. > > > diff --git a/crypt/md5-crypt.c b/crypt/md5-crypt.c > index d1b92d7..29f66ce 100644 > --- a/crypt/md5-crypt.c > +++ b/crypt/md5-crypt.c > @@ -88,6 +88,19 @@ extern char *__md5_crypt_r (const char *key, const > char *salt, > char *buffer, int buflen); > extern char *__md5_crypt (const char *key, const char *salt); > > +static void b64_from_24bit (char **cp, int *buflen, 'static void' should be on a separate line on its own. > + unsigned int b2, unsigned int b1, unsigned int b0, > + int n) > +{ > + unsigned int w = (b2 << 16) | (b1 << 8) | b0; > + while (n-- > 0 && *buflen > 0) > + { > + *(*cp)++ = b64t[w & 0x3f]; > + --*buflen; > + w >>= 6; > + } > +} > + > > /* This entry point is equivalent to the `crypt' function in Unix > libcs. */ > @@ -268,24 +281,18 @@ __md5_crypt_r (key, salt, buffer, buflen) > --buflen; > } > > - void b64_from_24bit (unsigned int b2, unsigned int b1, unsigned int b0, > - int n) > - { > - unsigned int w = (b2 << 16) | (b1 << 8) | b0; > - while (n-- > 0 && buflen > 0) > - { > - *cp++ = b64t[w & 0x3f]; > - --buflen; > - w >>= 6; > - } > - } > - > - b64_from_24bit (alt_result[0], alt_result[6], alt_result[12], 4); > - b64_from_24bit (alt_result[1], alt_result[7], alt_result[13], 4); > - b64_from_24bit (alt_result[2], alt_result[8], alt_result[14], 4); > - b64_from_24bit (alt_result[3], alt_result[9], alt_result[15], 4); > - b64_from_24bit (alt_result[4], alt_result[10], alt_result[5], 4); > - b64_from_24bit (0, 0, alt_result[11], 2); > + b64_from_24bit (&cp, &buflen, > + alt_result[0], alt_result[6], alt_result[12], 4); > + b64_from_24bit (&cp, &buflen, > + alt_result[1], alt_result[7], alt_result[13], 4); > + b64_from_24bit (&cp, &buflen, > + alt_result[2], alt_result[8], alt_result[14], 4); > + b64_from_24bit (&cp, &buflen, > + alt_result[3], alt_result[9], alt_result[15], 4); > + b64_from_24bit (&cp, &buflen, > + alt_result[4], alt_result[10], alt_result[5], 4); > + b64_from_24bit (&cp, &buflen, > + 0, 0, alt_result[11], 2); > if (buflen <= 0) > { > __set_errno (ERANGE); It would be useful if you could post some kind of performance analysis of the crypt or crypt_r to show that making this change does not cause a significant drop in performance. In fact, I'd be really interested in having a microbenchmark added for this function in benchtests. Instructions in benchtests/README should be useful but if not, please feel free to reach out to me for help. Siddhesh
On Tue, 20 May 2014, Konstantin Serebryany wrote: > (If approved, this will be my first commit to glibc, so some more > actions may be required) I don't see you in copyright.list - are you covered by a corporate copyright assignment? Even if individual patches in this series don't have much copyrightable creativity, the series as a whole still may. (Also: anyone working on this should *not* look at Qualcomm's patches for building glibc for Hexagon with Clang, as Qualcomm does not have a copyright assignment.)
On Tue, May 20, 2014 at 6:32 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Tue, 20 May 2014, Konstantin Serebryany wrote: > >> (If approved, this will be my first commit to glibc, so some more >> actions may be required) > > I don't see you in copyright.list - are you covered by a corporate > copyright assignment? Err. Most likely. (My other e-mail is kcc@google.com) > Even if individual patches in this series don't > have much copyrightable creativity, the series as a whole still may. > > (Also: anyone working on this should *not* look at Qualcomm's patches for > building glibc for Hexagon with Clang, as Qualcomm does not have a > copyright assignment.) Never heard of such :) > > -- > Joseph S. Myers > joseph@codesourcery.com
On Tue, 20 May 2014, Konstantin Serebryany wrote: > On Tue, May 20, 2014 at 6:32 PM, Joseph S. Myers > <joseph@codesourcery.com> wrote: > > On Tue, 20 May 2014, Konstantin Serebryany wrote: > > > >> (If approved, this will be my first commit to glibc, so some more > >> actions may be required) > > > > I don't see you in copyright.list - are you covered by a corporate > > copyright assignment? > Err. Most likely. (My other e-mail is kcc@google.com) Yes, there's a corporate assignment from Google for all GNU packages, dated 2007-03-15.
diff --git a/crypt/md5-crypt.c b/crypt/md5-crypt.c index d1b92d7..29f66ce 100644 --- a/crypt/md5-crypt.c +++ b/crypt/md5-crypt.c @@ -88,6 +88,19 @@ extern char *__md5_crypt_r (const char *key, const char *salt, char *buffer, int buflen); extern char *__md5_crypt (const char *key, const char *salt); +static void b64_from_24bit (char **cp, int *buflen, + unsigned int b2, unsigned int b1, unsigned int b0, + int n) +{ + unsigned int w = (b2 << 16) | (b1 << 8) | b0; + while (n-- > 0 && *buflen > 0) + { + *(*cp)++ = b64t[w & 0x3f]; + --*buflen; + w >>= 6; + } +} + /* This entry point is equivalent to the `crypt' function in Unix
Hi, This patch is the first in the series of patches that remove nested functions from glibc. Rationale: nested functions is a non-standard language feature; removing nested functions will allow to compile glibc with compilers other than GCC and thus benefit from other compilers and code analysis tools. Discussed previously here: https://sourceware.org/ml/libc-alpha/2014-05/msg00400.html No functionality change intended. (If approved, this will be my first commit to glibc, so some more actions may be required) 2014-05-20 Kostya Serebryany <konstantin.s.serebryany@gmail.com> * crypt/md5-crypt.c (__md5_crypt_r): Remove a nested function. (b64_from_24bit): New function. libcs. */ @@ -268,24 +281,18 @@ __md5_crypt_r (key, salt, buffer, buflen) --buflen; } - void b64_from_24bit (unsigned int b2, unsigned int b1, unsigned int b0, - int n) - { - unsigned int w = (b2 << 16) | (b1 << 8) | b0; - while (n-- > 0 && buflen > 0) - { - *cp++ = b64t[w & 0x3f]; - --buflen; - w >>= 6; - } - } - - b64_from_24bit (alt_result[0], alt_result[6], alt_result[12], 4); - b64_from_24bit (alt_result[1], alt_result[7], alt_result[13], 4); - b64_from_24bit (alt_result[2], alt_result[8], alt_result[14], 4); - b64_from_24bit (alt_result[3], alt_result[9], alt_result[15], 4); - b64_from_24bit (alt_result[4], alt_result[10], alt_result[5], 4); - b64_from_24bit (0, 0, alt_result[11], 2); + b64_from_24bit (&cp, &buflen, + alt_result[0], alt_result[6], alt_result[12], 4); + b64_from_24bit (&cp, &buflen, + alt_result[1], alt_result[7], alt_result[13], 4); + b64_from_24bit (&cp, &buflen, + alt_result[2], alt_result[8], alt_result[14], 4); + b64_from_24bit (&cp, &buflen, + alt_result[3], alt_result[9], alt_result[15], 4); + b64_from_24bit (&cp, &buflen, + alt_result[4], alt_result[10], alt_result[5], 4); + b64_from_24bit (&cp, &buflen, + 0, 0, alt_result[11], 2); if (buflen <= 0) { __set_errno (ERANGE);