Re [PATCH v3] replace atoi with strtol in varasm.cc (decode_reg_name_and_count) [PR114540]

[Heiko Eißfeldt]

Thanks for your feedback!

On 11/23/24 11:02 PM, Jakub Jelinek wrote:

On Sat, Nov 23, 2024 at 07:26:55PM +0100, Heiko Eißfeldt wrote:

--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -993,9 +993,12 @@ decode_reg_name_and_count (const char *asmspec, int 
*pnregs)
          break;
        if (asmspec[0] != 0 && i < 0)
        {
-         i = atoi (asmspec);
-         if (i < FIRST_PSEUDO_REGISTER && i >= 0 && reg_names[i][0])
-           return i;
+         char *pend{};

Why the {} strtol will write to pend no matter what, so it doesn't need to
be cleared.

+         errno = 0;
+         long j = strtol (asmspec, &pend, 10);
+         if (errno != ERANGE && j <= INT_MAX
+             && j < FIRST_PSEUDO_REGISTER && j >= 0 && reg_names[j][0])
+           return j;

If all conditions don't fit on a single line, all should be written
on separate lines, so
          if (errno != ERANGE
              && ...
              && ...)
I'm not sure what is the point of the j <= INT_MAX check,
FIRST_PSEUDO_REGISTER will surely be smaller than INT_MAX on all targets.

Furthermore, when strtol is used, I wonder if that
       for (i = strlen (asmspec) - 1; i >= 0; i--)
         if (! ISDIGIT (asmspec[i]))
           break;
isn't just a waste of time, if it wouldn't be enough to test that
ISDIGIT (asmspec[0]) and *pend == '\0' after the strtol call (if not,
it should fallthrough to the name matching rather than just return -2.

I agree, but was hesitant to change more than the offending atoi() part.

So
-      for (i = strlen (asmspec) - 1; i >= 0; i--)
-       if (! ISDIGIT (asmspec[i]))
-         break;
-      if (asmspec[0] != 0 && i < 0)
-       {
-         i = atoi (asmspec);
-         if (i < FIRST_PSEUDO_REGISTER && i >= 0 && reg_names[i][0])
-           return i;
-         else
-           return -2;
-       }
+      if (ISDIGIT (asmspec[0]))
+       {
+         char *pend;
+         errno = 0;
+         unsigned long j = strtoul (asmspec, &pend, 10);
+         if (*pend == '\0')
+           {

I would then add a

+ static_assert ( FIRST_PSEUDO_REGISTER <= INT_MAX );

here to 'document' the underlying assumption. I hope that is ok.

Would it make sense to have a nullptr check for asmspec at the
beginning, too?

+             if (errno != ERANGE
+                 && j < FIRST_PSEUDO_REGISTER
+                 && reg_names[j][0])
+               return j;
+             else
+               return -2;
+           }
+       }

        Jakub

diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index dd67dd441c0..3e3e4bc5b42 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -988,16 +988,21 @@ decode_reg_name_and_count (const char *asmspec, int 
*pnregs)
       asmspec = strip_reg_name (asmspec);
 
       /* Allow a decimal number as a "register name".  */
-      for (i = strlen (asmspec) - 1; i >= 0; i--)
-       if (! ISDIGIT (asmspec[i]))
-         break;
-      if (asmspec[0] != 0 && i < 0)
+      if (ISDIGIT (asmspec[0]))
        {
-         i = atoi (asmspec);
-         if (i < FIRST_PSEUDO_REGISTER && i >= 0 && reg_names[i][0])
-           return i;
-         else
-           return -2;
+         char *pend;
+         errno = 0;
+         unsigned long j = strtoul (asmspec, &pend, 10);
+         if (*pend == '\0')
+           {
+             static_assert( FIRST_PSEUDO_REGISTER <= INT_MAX );
+             if (errno != ERANGE
+                 && j < FIRST_PSEUDO_REGISTER
+                 && reg_names[j][0])
+               return j;
+             else
+               return -2;
+           }
        }
 
       for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)