[PATCH] Fix up strtod_l.c for -O0

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

I know that glibc currently refuses to compile with -O0, but e.g.
libquadmath that copies code from glibc doesn't enforce that.

As reported in http://gcc.gnu.org/PR56379 , when strtod_l.c is compiled with
-O0, it misbehaves.  The problem is that __mpn_lshift_1's
__builtin_constant_p (count) && count == BITS_PER_MP_LIMB
isn't just an optimization as the comment says.  __mpn_lshift_1
is normally called either with count equal to constant BITS_PER_MP_LIMB,
and then it expects to do the inline loop, or with variable count
that is assumed to be > 0 and < BITS_PER_MP_LIMB.
__mpn_lshift_1 is always_inline, but __builtin_constant_p is just an
optimization, so at -O0 it will evaluate to 0 when the argument isn't a
constant already during the parsing (and only inlining in that case
changes it into a constant).
The reason why the else part doesn't handle count == BITS_PER_MP_LIMB
properly is
1) mpn_lshift itself claims:
   Argument constraints:
   1. 0 < CNT < BITS_PER_MP_LIMB
   2. If the result is to be written over the input, WP must be >= UP.
2) even ignoring that, by most of the assembly implementations it might
   do the right thing, still ptr[0] will be usually unmodified original
   value and ptr[0] |= limb >> (BITS_PER_MP_LIMB - count);
   performs ptr[0] |= limb; rather than ptr[0] = limb; that would be
   desirable for that count.

The reason for __builtin_constant_p is I think that Ulrich didn't want to
bloat the code, while the compiler could sometimes with VRP figure out
that for variable count it will be > 0 and < BITS_PER_MP_LIMB, it doesn't
have to figure that out always and __mpn_lshift_1 code would then be bloated
by handling both cases, even when we know that only one of them is ever
needed at the particular call site.

So, to fix this, either we can change __mpn_lshift_1 into a macro, either
partially using helper inlines as done in the following patch, or fully,
with no inlines at all, or we could keep the always_inline __mpn_lshift_1,
just use
  if (
#ifdef __OPTIMIZE__
      __builtin_constant_p (count)
      &&
#endif
         count == BITS_PER_MP_LIMB)
    {
...
    }
  else
    {
...
    }
so that for -O1+ builds we'd never have extra code in there, but for
-O0 we would.

So, any preferences on this?  Or perhaps nothing at all, and we'll fix this
in libquadmath independently.

2013-02-19  Jakub Jelinek  <jakub@redhat.com>

	* stdlib/strtod_l.c (__mpn_lshift_1_word, __mpn_lshift_1_var): New
	inline functions.
	(__mpn_lshift_1): Change into macro using the above functions.

diff --git a/stdlib/strtod_l.c b/stdlib/strtod_l.c
index 5959354..58d3743 100644
--- a/stdlib/strtod_l.c
+++ b/stdlib/strtod_l.c
@@ -441,32 +441,46 @@ str_to_mpn (const STRING_TYPE *str, int digcnt, mp_limb_t *n, mp_size_t *nsize,
 }
 
 
-/* Shift {PTR, SIZE} COUNT bits to the left, and fill the vacated bits
-   with the COUNT most significant bits of LIMB.
+/* Helper function for __mpn_lshift_1.  Handle the case of shifting by exactly
+   a word: just copy words, with no actual bit-shifting.  This isn't just
+   an optimization, __mpn_lshift_1_var doesn't handle count == BITS_PER_MP_LIMB
+   properly.  */
+static inline void
+__attribute ((always_inline))
+__mpn_lshift_1_word (mp_limb_t *ptr, mp_size_t size, mp_limb_t limb)
+{
+  mp_size_t i;
+  for (i = size - 1; i > 0; --i)
+    ptr[i] = ptr[i - 1];
+  ptr[0] = limb;
+}
 
-   Tege doesn't like this function so I have to write it here myself. :)
-   --drepper */
+/* Helper function for __mpn_lshift_1.  COUNT must be > 0 and
+   < BITS_PER_MP_LIMB.  */
 static inline void
 __attribute ((always_inline))
-__mpn_lshift_1 (mp_limb_t *ptr, mp_size_t size, unsigned int count,
-		mp_limb_t limb)
+__mpn_lshift_1_var (mp_limb_t *ptr, mp_size_t size, unsigned int count,
+		    mp_limb_t limb)
 {
-  if (__builtin_constant_p (count) && count == BITS_PER_MP_LIMB)
-    {
-      /* Optimize the case of shifting by exactly a word:
-	 just copy words, with no actual bit-shifting.  */
-      mp_size_t i;
-      for (i = size - 1; i > 0; --i)
-	ptr[i] = ptr[i - 1];
-      ptr[0] = limb;
-    }
-  else
-    {
-      (void) __mpn_lshift (ptr, ptr, size, count);
-      ptr[0] |= limb >> (BITS_PER_MP_LIMB - count);
-    }
+  (void) __mpn_lshift (ptr, ptr, size, count);
+  ptr[0] |= limb >> (BITS_PER_MP_LIMB - count);
 }
 
+/* Shift {PTR, SIZE} COUNT bits to the left, and fill the vacated bits
+   with the COUNT most significant bits of LIMB.
+
+   Tege doesn't like this function so I have to write it here myself. :)
+   --drepper */
+#define __mpn_lshift_1(ptr, size, count, limb) \
+  do									\
+    {									\
+      if (__builtin_constant_p (count) && count == BITS_PER_MP_LIMB)	\
+	__mpn_lshift_1_word (ptr, size, limb);				\
+      else								\
+	__mpn_lshift_1_var (ptr, size, count, limb);			\
+    }									\
+  while (0)
+
 
 #define INTERNAL(x) INTERNAL1(x)
 #define INTERNAL1(x) __##x##_internal

	Jakub