diff mbox series

[v1] Replace {u}int_fast{16|32} with {u}int32_t

Message ID 20220411165835.4028009-1-goldstein.w.n@gmail.com
State New
Headers show
Series [v1] Replace {u}int_fast{16|32} with {u}int32_t | expand

Commit Message

Noah Goldstein April 11, 2022, 4:58 p.m. UTC 
On 32-bit machines this has no affect. On 64-bit machines
{u}int_fast{16|32} are set as {u}int64_t which is often not
ideal. Particularly x86_64 this change both saves code size and
may save instruction cost.

Full xcheck passes on x86_64.
---
 elf/dl-load.c                         |  2 +-
 elf/dl-lookup.c                       | 10 +++++-----
 elf/dl-machine-reject-phdr.h          |  2 +-
 elf/dl-profile.c                      |  2 +-
 elf/setup-vdso.h                      |  2 +-
 hurd/hurdselect.c                     |  2 +-
 iconv/gconv_simple.c                  |  4 ++--
 iconv/gconv_trans.c                   | 10 +++++-----
 iconvdata/cp932.c                     |  2 +-
 iconvdata/johab.c                     |  6 +++---
 iconvdata/sjis.c                      |  2 +-
 locale/elem-hash.h                    |  2 +-
 locale/weight.h                       |  2 +-
 posix/regex_internal.h                |  2 +-
 resolv/nss_dns/dns-canon.c            |  2 +-
 string/strcoll_l.c                    |  2 +-
 string/strxfrm_l.c                    |  8 ++++----
 sysdeps/mips/dl-machine-reject-phdr.h |  2 +-
 sysdeps/unix/sysv/linux/dl-sysdep.c   |  2 +-
 timezone/zic.c                        |  4 ++--
 20 files changed, 35 insertions(+), 35 deletions(-)

Comments

Paul Eggert April 11, 2022, 6:07 p.m. UTC | #1 
On 4/11/22 09:58, Noah Goldstein via Libc-alpha wrote:
> -   uint_fast16_t stack_flags = DEFAULT_STACK_PERMS;
> +   uint32_t stack_flags = DEFAULT_STACK_PERMS;

Doesn't this lose width info - or worse, add misleading info? For this 
particular case it looks like we need 32 bits, when we don't.

If we want to lose info, it'd be simpler to use plain 'int' and 
'unsigned int' for situations like these. That's less misleading.

If we don't want to lose info, it'd be better to define private types 
like glibc_uint_fast16_t and use these private types to address 
efficiency concerns.

I prefer losing the width info here, as this will cause less 
bikeshedding in the future. But if we want to keep width info, surely we 
should not make it more misleading.
Noah Goldstein April 11, 2022, 9:24 p.m. UTC | #2 
On Mon, Apr 11, 2022 at 1:07 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 4/11/22 09:58, Noah Goldstein via Libc-alpha wrote:
> > -   uint_fast16_t stack_flags = DEFAULT_STACK_PERMS;
> > +   uint32_t stack_flags = DEFAULT_STACK_PERMS;
>
> Doesn't this lose width info - or worse, add misleading info? For this
> particular case it looks like we need 32 bits, when we don't.
>
> If we want to lose info, it'd be simpler to use plain 'int' and
> 'unsigned int' for situations like these. That's less misleading.

Assume you only meant replacing the {u}int_fast16_t with unsigned int
if so fixed in v2.
>
> If we don't want to lose info, it'd be better to define private types
> like glibc_uint_fast16_t and use these private types to address
> efficiency concerns.
>
> I prefer losing the width info here, as this will cause less
> bikeshedding in the future. But if we want to keep width info, surely we
> should not make it more misleading.
Paul Eggert April 13, 2022, 7:11 p.m. UTC | #3 
On 4/13/22 08:23, Noah Goldstein wrote:

>> Both, I expect. We can assume int is at least 32 bits.
> 
> Disagree with that a bit. fast32 is saving pretty specifically at least 32 bits
> which int doesn't say but int32_t does.

I thought the idea was to not worry about losing the 16-bit vs 32-bit vs 
whatever info, since it doesn't really matter and keeping the info 
around just encourages bikeshedding later.

If we want to keep that info then we should create private types like 
glibc_int_fast32_t as I mentioned earlier. If not, let's just use 'int'.


> Can you confirm all the tz changes are okay?

Oh, sorry, I didn't notice that you were changing zic.c.

Unfortunately the zic.c changes are not OK, as zic.c is supposed to 
portable to platforms that lack int32_t (the C standard allows such 
platforms).
Noah Goldstein April 13, 2022, 11:28 p.m. UTC | #4 
On Wed, Apr 13, 2022 at 2:11 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 4/13/22 08:23, Noah Goldstein wrote:
>
> >> Both, I expect. We can assume int is at least 32 bits.
> >
> > Disagree with that a bit. fast32 is saving pretty specifically at least 32 bits
> > which int doesn't say but int32_t does.
>
> I thought the idea was to not worry about losing the 16-bit vs 32-bit vs
> whatever info, since it doesn't really matter and keeping the info
> around just encourages bikeshedding later.

The reason not replacing fast32 with int is int can be 2-bytes so it can
be incorrect. uint32_t will always work where fast32 worked.
>
> If we want to keep that info then we should create private types like
> glibc_int_fast32_t as I mentioned earlier. If not, let's just use 'int'.
>
glibc_int_fast32_t would work fine as well, just seems like a bigger
change than is needed.

>
> > Can you confirm all the tz changes are okay?
>
> Oh, sorry, I didn't notice that you were changing zic.c.
>
> Unfortunately the zic.c changes are not OK, as zic.c is supposed to
> portable to platforms that lack int32_t (the C standard allows such
> platforms).

Fixed in v4.
Paul Eggert April 13, 2022, 11:38 p.m. UTC | #5 
On 4/13/22 16:28, Noah Goldstein wrote:
> he reason not replacing fast32 with int is int can be 2-bytes so it can
> be incorrect.

No, int cannot be 2 8-bit bytes in GNU code. GNU code can assume that 
int is at least 32 bits. See the first paragraph of:

https://www.gnu.org/prep/standards/html_node/CPU-Portability.html

Similarly, POSIX code can assume that int is at least 32 bits; look for 
"INT_MIN" and "INT_MAX" in:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
Noah Goldstein April 14, 2022, midnight UTC | #6 
On Wed, Apr 13, 2022 at 6:38 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 4/13/22 16:28, Noah Goldstein wrote:
> > he reason not replacing fast32 with int is int can be 2-bytes so it can
> > be incorrect.
>
> No, int cannot be 2 8-bit bytes in GNU code. GNU code can assume that
> int is at least 32 bits. See the first paragraph of:
>
> https://www.gnu.org/prep/standards/html_node/CPU-Portability.html
>
> Similarly, POSIX code can assume that int is at least 32 bits; look for
> "INT_MIN" and "INT_MAX" in:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html

Okay, fixed in v5 to use int/unsigned int unless it very obviously
went against the
style of the function.
diff mbox series

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 892e8ef2f6..b528005182 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1093,7 +1093,7 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
    /* On most platforms presume that PT_GNU_STACK is absent and the stack is
     * executable.  Other platforms default to a nonexecutable stack and don't
     * need PT_GNU_STACK to do so.  */
-   uint_fast16_t stack_flags = DEFAULT_STACK_PERMS;
+   uint32_t stack_flags = DEFAULT_STACK_PERMS;
 
   {
     /* Scan the program header table, collecting its load commands.  */
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 7b2a6622be..765670719f 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -208,7 +208,7 @@  is_nodelete (struct link_map *map, int flags)
    in the unique symbol table, creating a new entry if necessary.
    Return the matching symbol in RESULT.  */
 static void
-do_lookup_unique (const char *undef_name, uint_fast32_t new_hash,
+do_lookup_unique (const char *undef_name, uint32_t new_hash,
 		  struct link_map *map, struct sym_val *result,
 		  int type_class, const ElfW(Sym) *sym, const char *strtab,
 		  const ElfW(Sym) *ref, const struct link_map *undef_map,
@@ -339,7 +339,7 @@  marking %s [%lu] as NODELETE due to unique symbol\n",
    something bad happened.  */
 static int
 __attribute_noinline__
-do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
+do_lookup_x (const char *undef_name, uint32_t new_hash,
 	     unsigned long int *old_hash, const ElfW(Sym) *ref,
 	     struct sym_val *result, struct r_scope_elem *scope, size_t i,
 	     const struct r_found_version *const version, int flags,
@@ -558,10 +558,10 @@  skip:
 }
 
 
-static uint_fast32_t
+static uint32_t
 dl_new_hash (const char *s)
 {
-  uint_fast32_t h = 5381;
+  uint32_t h = 5381;
   for (unsigned char c = *s; c != '\0'; c = *++s)
     h = h * 33 + c;
   return h & 0xffffffff;
@@ -816,7 +816,7 @@  _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
 		     const struct r_found_version *version,
 		     int type_class, int flags, struct link_map *skip_map)
 {
-  const uint_fast32_t new_hash = dl_new_hash (undef_name);
+  const uint32_t new_hash = dl_new_hash (undef_name);
   unsigned long int old_hash = 0xffffffff;
   struct sym_val current_value = { NULL, NULL };
   struct r_scope_elem **scope = symbol_scope;
diff --git a/elf/dl-machine-reject-phdr.h b/elf/dl-machine-reject-phdr.h
index ea18289fda..b8baabe3be 100644
--- a/elf/dl-machine-reject-phdr.h
+++ b/elf/dl-machine-reject-phdr.h
@@ -24,7 +24,7 @@ 
 /* Return true iff ELF program headers are incompatible with the running
    host.  */
 static inline bool
-elf_machine_reject_phdr_p (const ElfW(Phdr) *phdr, uint_fast16_t phnum,
+elf_machine_reject_phdr_p (const ElfW(Phdr) *phdr, uint32_t phnum,
 			   const char *buf, size_t len, struct link_map *map,
 			   int fd)
 {
diff --git a/elf/dl-profile.c b/elf/dl-profile.c
index 9359be7c33..3a3b49ca0f 100644
--- a/elf/dl-profile.c
+++ b/elf/dl-profile.c
@@ -558,7 +558,7 @@  _dl_mcount (ElfW(Addr) frompc, ElfW(Addr) selfpc)
 	  /* If we still have no entry stop searching and insert.  */
 	  if (*topcindex == 0)
 	    {
-	      uint_fast32_t newarc = catomic_exchange_and_add (narcsp, 1);
+	      uint32_t newarc = catomic_exchange_and_add (narcsp, 1);
 
 	      /* In rare cases it could happen that all entries in FROMS are
 		 occupied.  So we cannot count this anymore.  */
diff --git a/elf/setup-vdso.h b/elf/setup-vdso.h
index db639b0d4f..e89cc63258 100644
--- a/elf/setup-vdso.h
+++ b/elf/setup-vdso.h
@@ -36,7 +36,7 @@  setup_vdso (struct link_map *main_map __attribute__ ((unused)),
       l->l_phdr = ((const void *) GLRO(dl_sysinfo_dso)
 		   + GLRO(dl_sysinfo_dso)->e_phoff);
       l->l_phnum = GLRO(dl_sysinfo_dso)->e_phnum;
-      for (uint_fast16_t i = 0; i < l->l_phnum; ++i)
+      for (uint32_t i = 0; i < l->l_phnum; ++i)
 	{
 	  const ElfW(Phdr) *const ph = &l->l_phdr[i];
 	  if (ph->p_type == PT_DYNAMIC)
diff --git a/hurd/hurdselect.c b/hurd/hurdselect.c
index e9d5c64bf3..038ac3acf2 100644
--- a/hurd/hurdselect.c
+++ b/hurd/hurdselect.c
@@ -575,7 +575,7 @@  _hurd_select (int nfds,
     for (i = 0; i < nfds; ++i)
       {
 	int type = d[i].type;
-	int_fast16_t revents = 0;
+	int32_t revents = 0;
 
 	if (type & SELECT_ERROR)
 	  switch (d[i].error)
diff --git a/iconv/gconv_simple.c b/iconv/gconv_simple.c
index be8504791b..3f8802f411 100644
--- a/iconv/gconv_simple.c
+++ b/iconv/gconv_simple.c
@@ -959,8 +959,8 @@  ucs4le_internal_loop_single (struct __gconv_step *step,
       }									      \
     else								      \
       {									      \
-	uint_fast32_t cnt;						      \
-	uint_fast32_t i;						      \
+	uint32_t cnt;						      \
+	uint32_t i;						      \
 									      \
 	if (ch >= 0xc2 && ch < 0xe0)					      \
 	  {								      \
diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
index 0101332d61..bc7e58b477 100644
--- a/iconv/gconv_trans.c
+++ b/iconv/gconv_trans.c
@@ -37,15 +37,15 @@  __gconv_transliterate (struct __gconv_step *step,
 		       unsigned char **outbufstart, size_t *irreversible)
 {
   /* Find out about the locale's transliteration.  */
-  uint_fast32_t size;
+  uint32_t size;
   const uint32_t *from_idx;
   const uint32_t *from_tbl;
   const uint32_t *to_idx;
   const uint32_t *to_tbl;
   const uint32_t *winbuf;
   const uint32_t *winbufend;
-  uint_fast32_t low;
-  uint_fast32_t high;
+  uint32_t low;
+  uint32_t high;
 
   /* The input buffer.  There are actually 4-byte values.  */
   winbuf = (const uint32_t *) *inbufp;
@@ -85,7 +85,7 @@  __gconv_transliterate (struct __gconv_step *step,
   high = size;
   while (low < high)
     {
-      uint_fast32_t med = (low + high) / 2;
+      uint32_t med = (low + high) / 2;
       uint32_t idx;
       int cnt;
 
@@ -111,7 +111,7 @@  __gconv_transliterate (struct __gconv_step *step,
 	  do
 	    {
 	      /* Determine length of replacement.  */
-	      uint_fast32_t len = 0;
+	      uint32_t len = 0;
 	      int res;
 	      const unsigned char *toinptr;
 	      unsigned char *outptr;
diff --git a/iconvdata/cp932.c b/iconvdata/cp932.c
index 91487fa34c..f40f8e2a9d 100644
--- a/iconvdata/cp932.c
+++ b/iconvdata/cp932.c
@@ -4572,7 +4572,7 @@  static const char from_ucs4_extra[229][2] =
 	/* Two-byte character.  First test whether the next character	      \
 	   is also available.  */					      \
 	uint32_t ch2;							      \
-	uint_fast32_t idx;						      \
+	uint32_t idx;						      \
 									      \
 	if (__glibc_unlikely (inptr + 1 >= inend))			      \
 	  {								      \
diff --git a/iconvdata/johab.c b/iconvdata/johab.c
index 33ec7b9a62..ae271def96 100644
--- a/iconvdata/johab.c
+++ b/iconvdata/johab.c
@@ -130,7 +130,7 @@  static const uint16_t jamo_from_ucs_table[51] =
 
 
 static uint32_t
-johab_sym_hanja_to_ucs (uint_fast32_t idx, uint_fast32_t c1, uint_fast32_t c2)
+johab_sym_hanja_to_ucs (uint32_t idx, uint32_t c1, uint32_t c2)
 {
   if (idx <= 0xdefe)
     return (uint32_t) __ksc5601_sym_to_ucs[(c1 - 0xd9) * 188 + c2
@@ -189,7 +189,7 @@  johab_sym_hanja_to_ucs (uint_fast32_t idx, uint_fast32_t c1, uint_fast32_t c2)
 	    /* Two-byte character.  First test whether the next		      \
 	       character is also available.  */				      \
 	    uint32_t ch2;						      \
-	    uint_fast32_t idx;						      \
+	    uint32_t idx;						      \
 									      \
 	    if (__glibc_unlikely (inptr + 1 >= inend))			      \
 	      {								      \
@@ -204,7 +204,7 @@  johab_sym_hanja_to_ucs (uint_fast32_t idx, uint_fast32_t c1, uint_fast32_t c2)
 	    if (__glibc_likely (ch <= 0xd3))				      \
 	      {								      \
 		/* Hangul */						      \
-		int_fast32_t i, m, f;					      \
+		int32_t i, m, f;					      \
 									      \
 		i = init[(idx & 0x7c00) >> 10];				      \
 		m = mid[(idx & 0x03e0) >> 5];				      \
diff --git a/iconvdata/sjis.c b/iconvdata/sjis.c
index e1bdb3025c..5aea18b314 100644
--- a/iconvdata/sjis.c
+++ b/iconvdata/sjis.c
@@ -4359,7 +4359,7 @@  static const char from_ucs4_extra[0x100][2] =
 	/* Two-byte character.  First test whether the next byte	      \
 	   is also available.  */					      \
 	uint32_t ch2;							      \
-	uint_fast32_t idx;						      \
+	uint32_t idx;						      \
 									      \
 	if (__glibc_unlikely (inptr + 1 >= inend))			      \
 	  {								      \
diff --git a/locale/elem-hash.h b/locale/elem-hash.h
index 8bccfcb6d0..08dc3da77a 100644
--- a/locale/elem-hash.h
+++ b/locale/elem-hash.h
@@ -18,7 +18,7 @@ 
 
 /* The hashing function used for the table with collation symbols.  */
 static int32_t __attribute__ ((pure, unused))
-elem_hash (const char *str, int_fast32_t n)
+elem_hash (const char *str, int32_t n)
 {
   int32_t result = n;
 
diff --git a/locale/weight.h b/locale/weight.h
index c49f4e6d90..8be2d220f8 100644
--- a/locale/weight.h
+++ b/locale/weight.h
@@ -27,7 +27,7 @@  findidx (const int32_t *table,
 	 const unsigned char *extra,
 	 const unsigned char **cpp, size_t len)
 {
-  int_fast32_t i = table[*(*cpp)++];
+  int32_t i = table[*(*cpp)++];
   const unsigned char *cp;
   const unsigned char *usrc;
 
diff --git a/posix/regex_internal.h b/posix/regex_internal.h
index 5827597d2c..cb079b16f7 100644
--- a/posix/regex_internal.h
+++ b/posix/regex_internal.h
@@ -814,7 +814,7 @@  re_string_elem_size_at (const re_string_t *pstr, Idx idx)
 # ifdef _LIBC
   const unsigned char *p, *extra;
   const int32_t *table, *indirect;
-  uint_fast32_t nrules = _NL_CURRENT_WORD (LC_COLLATE, _NL_COLLATE_NRULES);
+  uint32_t nrules = _NL_CURRENT_WORD (LC_COLLATE, _NL_COLLATE_NRULES);
 
   if (nrules != 0)
     {
diff --git a/resolv/nss_dns/dns-canon.c b/resolv/nss_dns/dns-canon.c
index 3151e50ae1..0110a9c318 100644
--- a/resolv/nss_dns/dns-canon.c
+++ b/resolv/nss_dns/dns-canon.c
@@ -118,7 +118,7 @@  _nss_dns_getcanonname_r (const char *name, char *buffer, size_t buflen,
 		goto unavail;
 
 	      /* Check whether type and class match.  */
-	      uint_fast16_t type;
+	      uint32_t type;
 	      NS_GET16 (type, ptr);
 	      if (type == qtypes[i])
 		{
diff --git a/string/strcoll_l.c b/string/strcoll_l.c
index 1b08f11b99..b366020fc7 100644
--- a/string/strcoll_l.c
+++ b/string/strcoll_l.c
@@ -257,7 +257,7 @@  int
 STRCOLL (const STRING_TYPE *s1, const STRING_TYPE *s2, locale_t l)
 {
   struct __locale_data *current = l->__locales[LC_COLLATE];
-  uint_fast32_t nrules = current->values[_NL_ITEM_INDEX (_NL_COLLATE_NRULES)].word;
+  uint32_t nrules = current->values[_NL_ITEM_INDEX (_NL_COLLATE_NRULES)].word;
   /* We don't assign the following values right away since it might be
      unnecessary in case there are no rules.  */
   const unsigned char *rulesets;
diff --git a/string/strxfrm_l.c b/string/strxfrm_l.c
index 5519c3cf8f..188a3d826a 100644
--- a/string/strxfrm_l.c
+++ b/string/strxfrm_l.c
@@ -49,7 +49,7 @@ 
 /* Group locale data for shorter parameter lists.  */
 typedef struct
 {
-  uint_fast32_t nrules;
+  uint32_t nrules;
   unsigned char *rulesets;
   USTRING_TYPE *weights;
   int32_t *table;
@@ -135,7 +135,7 @@  do_xfrm (const USTRING_TYPE *usrc, STRING_TYPE *dest, size_t n,
 {
   int32_t weight_idx;
   unsigned char rule_idx;
-  uint_fast32_t pass;
+  uint32_t pass;
   size_t needed = 0;
   size_t last_needed;
 
@@ -404,10 +404,10 @@  static size_t
 do_xfrm_cached (STRING_TYPE *dest, size_t n, const locale_data_t *l_data,
 		size_t idxmax, int32_t *idxarr, const unsigned char *rulearr)
 {
-  uint_fast32_t nrules = l_data->nrules;
+  uint32_t nrules = l_data->nrules;
   unsigned char *rulesets = l_data->rulesets;
   USTRING_TYPE *weights = l_data->weights;
-  uint_fast32_t pass;
+  uint32_t pass;
   size_t needed = 0;
   size_t last_needed;
   size_t idxcnt;
diff --git a/sysdeps/mips/dl-machine-reject-phdr.h b/sysdeps/mips/dl-machine-reject-phdr.h
index e784320234..36ed7300d4 100644
--- a/sysdeps/mips/dl-machine-reject-phdr.h
+++ b/sysdeps/mips/dl-machine-reject-phdr.h
@@ -152,7 +152,7 @@  static const struct abi_req none_req = { true, true, true, false, true };
    impact of dlclose.  */
 
 static bool __attribute_used__
-elf_machine_reject_phdr_p (const ElfW(Phdr) *phdr, uint_fast16_t phnum,
+elf_machine_reject_phdr_p (const ElfW(Phdr) *phdr, uint32_t phnum,
 			   const char *buf, size_t len, struct link_map *map,
 			   int fd)
 {
diff --git a/sysdeps/unix/sysv/linux/dl-sysdep.c b/sysdeps/unix/sysv/linux/dl-sysdep.c
index c90f109b11..75cd85c6a1 100644
--- a/sysdeps/unix/sysv/linux/dl-sysdep.c
+++ b/sysdeps/unix/sysv/linux/dl-sysdep.c
@@ -269,7 +269,7 @@  _dl_discover_osversion (void)
       } expected_note = { { sizeof "Linux", sizeof (ElfW(Word)), 0 }, "Linux" };
       const ElfW(Phdr) *const phdr = GLRO(dl_sysinfo_map)->l_phdr;
       const ElfW(Word) phnum = GLRO(dl_sysinfo_map)->l_phnum;
-      for (uint_fast16_t i = 0; i < phnum; ++i)
+      for (uint32_t i = 0; i < phnum; ++i)
 	if (phdr[i].p_type == PT_NOTE)
 	  {
 	    const ElfW(Addr) start = (phdr[i].p_vaddr
diff --git a/timezone/zic.c b/timezone/zic.c
index 2875b5544c..16d0cede65 100644
--- a/timezone/zic.c
+++ b/timezone/zic.c
@@ -1789,7 +1789,7 @@  rulesub(struct rule *rp, const char *loyearp, const char *hiyearp,
 }
 
 static void
-convert(const int_fast32_t val, char *const buf)
+convert(const int32_t val, char *const buf)
 {
 	register int	i;
 	register int	shift;
@@ -1811,7 +1811,7 @@  convert64(const zic_t val, char *const buf)
 }
 
 static void
-puttzcode(const int_fast32_t val, FILE *const fp)
+puttzcode(const int32_t val, FILE *const fp)
 {
 	char	buf[4];