From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id 74C37385B53E for ; Mon, 28 Nov 2022 21:17:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 74C37385B53E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.96,201,1665475200"; d="scan'208";a="87881716" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 28 Nov 2022 13:17:24 -0800 IronPort-SDR: 2z774t5jOBoArDDYR6B0NrsRFw3g9xKZRxgTYNgMLDG9YgLvtm76cfAZo+W4UBkXFJl6V8MKm0 +hTcdRZWdKCa1FB9wZZQB97ttIkUGBZKLvgWqoOtShNIen9+QwoNXGV2wD51BnwFmqfmmbrr9y 1FZzJKtT9GSBvqlcO1iM7ID/UlJATDO0GWOCBil+zAMOsUfrOSZekStdmcyiuBdcH2ZyrrYKu0 WqKYEwnQ/yFrQv6EBocYesUYnYSL4sWROEYH6hlFlqKN0t2Fxttjo8jDrhXMzK09kCxUXIUJga ZNQ= Date: Mon, 28 Nov 2022 21:17:19 +0000 From: Joseph Myers To: Seija K. CC: Subject: Re: Add restrict annotations to all functions that require it In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-14.mgc.mentorg.com (139.181.222.14) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-3115.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sat, 26 Nov 2022, Seija K. via Libc-alpha wrote: > The format value has to be a string literal, every time. Otherwise, you are > not using these functions correctly. To reinforce this fact, I put > __restrict over every example of this I could find. There are several other changes in this patch that aren't adding __restrict. Maybe they are a good idea, maybe they aren't, but they aren't described by the proposed commit message, and a patch should do one thing and do it well (so if those other changes are desirable, they should go in a separate patch with its own commit message explaining them). For both patches, it would also be helpful to know whether they result in any changes to installed stripped shared libraries. > diff --git a/string/memcpy.c b/string/memcpy.c > index 9cf64530e73..920683e3d8c 100644 > --- a/string/memcpy.c > +++ b/string/memcpy.c > @@ -25,10 +25,10 @@ > #endif > > void * > -MEMCPY (void *dstpp, const void *srcpp, size_t len) > +MEMCPY (void *__restrict dstpp, const void *__restrict srcpp, size_t len) > { > - unsigned long int dstp = (long int) dstpp; > - unsigned long int srcp = (long int) srcpp; > + unsigned long int dstp = (unsigned long int) dstpp; > + unsigned long int srcp = (unsigned long int) srcpp; This is an example of the other changes. > diff --git a/string/memfrob.c b/string/memfrob.c > index 9707cb9fda1..025929f90cb 100644 > --- a/string/memfrob.c > +++ b/string/memfrob.c > @@ -22,7 +22,7 @@ memfrob (void *s, size_t n) > { > char *p = (char *) s; > > - while (n-- > 0) > + for (; n; n--) > *p++ ^= 42; Likewise. > diff --git a/string/memrchr.c b/string/memrchr.c > index 8eb6829e458..7fd7f666c84 100644 > --- a/string/memrchr.c > +++ b/string/memrchr.c > @@ -63,9 +63,8 @@ MEMRCHR > const unsigned char *char_ptr; > const unsigned long int *longword_ptr; > unsigned long int longword, magic_bits, charmask; > - unsigned char c; > > - c = (unsigned char) c_in; > + const unsigned char c = (const unsigned char) c_in; > > /* Handle the last few characters by reading one character at a time. > Do this until CHAR_PTR is aligned on a longword boundary. */ > @@ -182,7 +181,7 @@ MEMRCHR > > char_ptr = (const unsigned char *) longword_ptr; > > - while (n-- > 0) > + for (; n > 0; --n) Likewise. > diff --git a/string/memset.c b/string/memset.c > index 1303dd7ad36..273bd5d6ff0 100644 > --- a/string/memset.c > +++ b/string/memset.c > @@ -26,7 +26,7 @@ void * > inhibit_loop_to_libcall > MEMSET (void *dstpp, int c, size_t len) > { > - long int dstp = (long int) dstpp; > + unsigned long int dstp = (unsigned long int) dstpp; Likewise. > diff --git a/string/stpncpy.c b/string/stpncpy.c > index 61f0134fb20..7346d790a1a 100644 > --- a/string/stpncpy.c > +++ b/string/stpncpy.c > @@ -37,9 +37,9 @@ weak_alias (__stpncpy, stpncpy) > /* Copy no more than N characters of SRC to DEST, returning the address of > the terminating '\0' in DEST, if any, or else DEST + N. */ > char * > -STPNCPY (char *dest, const char *src, size_t n) > +STPNCPY (char * __restrict dest, const char * __restrict src, size_t n) > { > - size_t size = __strnlen (src, n); > + const size_t size = __strnlen (src, n); Likewise. > diff --git a/string/strlen.c b/string/strlen.c > index 54f3fb8167a..f443fd16d86 100644 > --- a/string/strlen.c > +++ b/string/strlen.c > @@ -33,6 +33,9 @@ STRLEN (const char *str) > const unsigned long int *longword_ptr; > unsigned long int longword, himagic, lomagic; > > + if (sizeof (longword) > 8) > + abort (); > + > /* Handle the first few characters by reading one character at a time. > Do this until CHAR_PTR is aligned on a longword boundary. */ > for (char_ptr = str; ((unsigned long int) char_ptr > @@ -59,8 +62,6 @@ STRLEN (const char *str) > himagic = ((himagic << 16) << 16) | himagic; > lomagic = ((lomagic << 16) << 16) | lomagic; > } > - if (sizeof (longword) > 8) > - abort (); Likewise (and if you change anything here, surely using a static assertion is better than this "if"). > diff --git a/string/strxfrm_l.c b/string/strxfrm_l.c > index 188a3d826a6..79596a9c6de 100644 > --- a/string/strxfrm_l.c > +++ b/string/strxfrm_l.c > @@ -441,7 +441,7 @@ do_xfrm_cached (STRING_TYPE *dest, size_t n, const > locale_data_t *l_data, > len = weights[idxarr[backw]++]; > > if (needed + len < n) > - while (len-- > 0) > + for (; len; --len) > dest[needed++] = weights[idxarr[backw]++]; > else > { > @@ -457,7 +457,7 @@ do_xfrm_cached (STRING_TYPE *dest, size_t n, const > locale_data_t *l_data, > /* Now handle the forward element. */ > len = weights[idxarr[idxcnt]++]; > if (needed + len < n) > - while (len-- > 0) > + for (; len; --len) > dest[needed++] = weights[idxarr[idxcnt]++]; > else > { > @@ -488,7 +488,7 @@ do_xfrm_cached (STRING_TYPE *dest, size_t n, const > locale_data_t *l_data, > size_t len = weights[idxarr[--backw]++]; > > if (needed + len < n) > - while (len-- > 0) > + for (; len; --len) > dest[needed++] = weights[idxarr[backw]++]; > else Likewise. > diff --git a/string/swab.c b/string/swab.c > index 083a80091f7..6aecb93c5eb 100644 > --- a/string/swab.c > +++ b/string/swab.c > @@ -18,15 +18,15 @@ > #include > > void > -swab (const void *bfrom, void *bto, ssize_t n) > +swab (const void *__restrict bfrom, void *__restrict bto, ssize_t n) > { > - const char *from = (const char *) bfrom; > - char *to = (char *) bto; > + const unsigned char *from = (const unsigned char *) bfrom; > + unsigned char *to = (unsigned char *) bto; > > n &= ~((ssize_t) 1); > while (n > 1) > { > - const char b0 = from[--n], b1 = from[--n]; > + const unsigned char b0 = from[--n], b1 = from[--n]; Likewise. > diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h > index 005d089501f..922eed6deae 100644 > --- a/sysdeps/x86_64/dl-machine.h > +++ b/sysdeps/x86_64/dl-machine.h > @@ -416,17 +416,14 @@ and creates an unsatisfiable circular dependency.\n", > value += reloc->r_addend; > *(unsigned int *) reloc_addr = value; > > - const char *fmt; Likewise; lots of changes here with nothing apparently to do with restrict. -- Joseph S. Myers joseph@codesourcery.com