Floating point #define in ksh

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Floating point #define in ksh

Michael McConville-2
FP has been undefined for at least ten years, and probably forever. It's
used to conditionally add two small sections and one large section of
shf.c.

My initial reaction is that FP should be removed. Is there any reason to
keep it? Should the associated code stay or go?

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Michael McConville-2
Michael McConville wrote:
> FP has been undefined for at least ten years, and probably forever.
> It's used to conditionally add two small sections and one large
> section of shf.c.
>
> My initial reaction is that FP should be removed. Is there any reason
> to keep it? Should the associated code stay or go?

To be (far) more specific, it's used to enable the 'e', 'g', and 'f'
fields of ksh's vfprintf() clone. On one hand, this can't be too
important if it was disabled for >10 years. On the other, the code at
least compiles...

It seems that there's a lot of good cleanup to be done in ksh. There are
many hacks and reimplementations that seem to be remnants of a time when
you couldn't rely on standards much.

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Nicholas Marriott-2
I would kill it. FPBUF_SIZE and DMAXEXP can go too.


On Fri, Sep 11, 2015 at 11:41:40PM -0400, Michael McConville wrote:

> Michael McConville wrote:
> > FP has been undefined for at least ten years, and probably forever.
> > It's used to conditionally add two small sections and one large
> > section of shf.c.
> >
> > My initial reaction is that FP should be removed. Is there any reason
> > to keep it? Should the associated code stay or go?
>
> To be (far) more specific, it's used to enable the 'e', 'g', and 'f'
> fields of ksh's vfprintf() clone. On one hand, this can't be too
> important if it was disabled for >10 years. On the other, the code at
> least compiles...
>
> It seems that there's a lot of good cleanup to be done in ksh. There are
> many hacks and reimplementations that seem to be remnants of a time when
> you couldn't rely on standards much.
>

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Todd C. Miller
In reply to this post by Michael McConville-2
On Fri, 11 Sep 2015 23:41:40 -0400, Michael McConville wrote:

> To be (far) more specific, it's used to enable the 'e', 'g', and 'f'
> fields of ksh's vfprintf() clone. On one hand, this can't be too
> important if it was disabled for >10 years. On the other, the code at
> least compiles...

When I last checked the FP code for ksh's builtin printf was not
correct/complete.  Once upon a I took a stab at replacing ksh's
mini-stdio with the libc one but never got it working 100%.

> It seems that there's a lot of good cleanup to be done in ksh. There are
> many hacks and reimplementations that seem to be remnants of a time when
> you couldn't rely on standards much.

Indeed.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Michael McConville-2
Todd C. Miller wrote:
> On Michael McConville wrote:
> > To be (far) more specific, it's used to enable the 'e', 'g', and 'f'
> > fields of ksh's vfprintf() clone. On one hand, this can't be too
> > important if it was disabled for >10 years. On the other, the code
> > at least compiles...
>
> When I last checked the FP code for ksh's builtin printf was not
> correct/complete. Once upon a I took a stab at replacing ksh's
> mini-stdio with the libc one but never got it working 100%.

I'm planning on trying it. If you want to work with me, let me know.
Either way, I'd be interested to see your diffs if you still have them.

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Michael McConville-2
In reply to this post by Nicholas Marriott-2
Nicholas Marriott wrote:
> I would kill it. FPBUF_SIZE and DMAXEXP can go too.

Here's the diff:


Index: ksh_limval.h
===================================================================
RCS file: /cvs/src/bin/ksh/ksh_limval.h,v
retrieving revision 1.2
diff -u -p -r1.2 ksh_limval.h
--- ksh_limval.h 18 Dec 2004 20:55:52 -0000 1.2
+++ ksh_limval.h 12 Sep 2015 18:59:00 -0000
@@ -4,10 +4,6 @@
 
 /* limits.h is included in sh.h */
 
-#ifndef DMAXEXP
-# define DMAXEXP 128 /* should be big enough */
-#endif
-
 #ifndef BITS
 # define BITS(t) (CHAR_BIT * sizeof(t))
 #endif
Index: shf.c
===================================================================
RCS file: /cvs/src/bin/ksh/shf.c,v
retrieving revision 1.16
diff -u -p -r1.16 shf.c
--- shf.c 19 Apr 2013 17:36:09 -0000 1.16
+++ shf.c 12 Sep 2015 18:59:00 -0000
@@ -705,15 +705,7 @@ shf_smprintf(const char *fmt, ...)
  return shf_sclose(&shf); /* null terminates */
 }
 
-#undef FP /* if you want floating point stuff */
-
 #define BUF_SIZE 128
-#define FPBUF_SIZE (DMAXEXP+16)/* this must be >
- * MAX(DMAXEXP, log10(pow(2, DSIGNIF)))
- *    + ceil(log10(DMAXEXP)) + 8 (I think).
- * Since this is hard to express as a
- * constant, just use a large buffer.
- */
 
 /*
  * What kinda of machine we on?  Hopefully the C compiler will optimize
@@ -746,18 +738,6 @@ shf_smprintf(const char *fmt, ...)
 #define FL_NUMBER 0x400 /* a number was formated %[douxefg] */
 
 
-#ifdef FP
-#include <math.h>
-
-static double
-my_ceil(double d)
-{
- double i;
-
- return d - modf(d, &i) + (d < 0 ? -1 : 1);
-}
-#endif /* FP */
-
 int
 shf_vfprintf(struct shf *shf, const char *fmt, va_list args)
 {
@@ -771,17 +751,6 @@ shf_vfprintf(struct shf *shf, const char
  char numbuf[(BITS(long long) + 2) / 3 + 1];
  /* this stuff for dealing with the buffer */
  int nwritten = 0;
-#ifdef FP
- /* should be in <math.h>
- *  extern double frexp();
- */
- extern char *ecvt();
-
- double fpnum;
- int expo, decpt;
- char style;
- char fpbuf[FPBUF_SIZE];
-#endif /* FP */
 
  if (!fmt)
  return 0;
@@ -952,134 +921,6 @@ shf_vfprintf(struct shf *shf, const char
  precision = len; /* no loss */
  }
  break;
-
-#ifdef FP
- case 'e':
- case 'g':
- case 'f':
-    {
- char *p;
-
- /*
- * This could probably be done better,
- *  but it seems to work.  Note that gcvt()
- *  is not used, as you cannot tell it to
- *  not strip the zeros.
- */
- flags |= FL_NUMBER;
- if (!(flags & FL_DOT))
- precision = 6; /* default */
- /*
- * Assumes doubles are pushed on
- *  the stack.  If this is not so, then
- *  FL_LLONG/FL_LONG/FL_SHORT should be checked.
- */
- fpnum = va_arg(args, double);
- s = fpbuf;
- style = c;
- /*
- *  This is the same as
- * expo = ceil(log10(fpnum))
- *  but doesn't need -lm.  This is an
- *  approximation as expo is rounded up.
- */
- (void) frexp(fpnum, &expo);
- expo = my_ceil(expo / LOG2_10);
-
- if (expo < 0)
- expo = 0;
-
- p = ecvt(fpnum, precision + 1 + expo,
- &decpt, &tmp);
- if (c == 'g') {
- if (decpt < -4 || decpt > precision)
- style = 'e';
- else
- style = 'f';
- if (decpt > 0 && (precision -= decpt) < 0)
- precision = 0;
- }
- if (tmp)
- *s++ = '-';
- else if (flags & FL_PLUS)
- *s++ = '+';
- else if (flags & FL_BLANK)
- *s++ = ' ';
-
- if (style == 'e')
- *s++ = *p++;
- else {
- if (decpt > 0) {
- /* Overflow check - should
- * never have this problem.
- */
- if (decpt > &fpbuf[sizeof(fpbuf)] - s - 8)
- decpt = &fpbuf[sizeof(fpbuf)] - s - 8;
- (void) memcpy(s, p, decpt);
- s += decpt;
- p += decpt;
- } else
- *s++ = '0';
- }
-
- /* print the fraction? */
- if (precision > 0) {
- *s++ = '.';
- /* Overflow check - should
- * never have this problem.
- */
- if (precision > &fpbuf[sizeof(fpbuf)] - s - 7)
- precision = &fpbuf[sizeof(fpbuf)] - s - 7;
- for (tmp = decpt;  tmp++ < 0 &&
-    precision > 0 ; precision--)
- *s++ = '0';
- tmp = strlen(p);
- if (precision > tmp)
- precision = tmp;
- /* Overflow check - should
- * never have this problem.
- */
- if (precision > &fpbuf[sizeof(fpbuf)] - s - 7)
- precision = &fpbuf[sizeof(fpbuf)] - s - 7;
- (void) memcpy(s, p, precision);
- s += precision;
- /*
- * `g' format strips trailing
- *  zeros after the decimal.
- */
- if (c == 'g' && !(flags & FL_HASH)) {
- while (*--s == '0')
- ;
- if (*s != '.')
- s++;
- }
- } else if (flags & FL_HASH)
- *s++ = '.';
-
- if (style == 'e') {
- *s++ = (flags & FL_UPPER) ? 'E' : 'e';
- if (--decpt >= 0)
- *s++ = '+';
- else {
- *s++ = '-';
- decpt = -decpt;
- }
- p = &numbuf[sizeof(numbuf)];
- for (tmp = 0; tmp < 2 || decpt ; tmp++) {
- *--p = '0' + decpt % 10;
- decpt /= 10;
- }
- tmp = &numbuf[sizeof(numbuf)] - p;
- (void) memcpy(s, p, tmp);
- s += tmp;
- }
-
- len = s - fpbuf;
- s = fpbuf;
- precision = len;
- break;
-    }
-#endif /* FP */
 
  case 's':
  if (!(s = va_arg(args, char *)))

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Nicholas Marriott-2
Works for me. ok anyone?

I think ksh_limval.h can go entirely after this, per the note in
PROJECTS.



On Sat, Sep 12, 2015 at 03:00:02PM -0400, Michael McConville wrote:

> Nicholas Marriott wrote:
> > I would kill it. FPBUF_SIZE and DMAXEXP can go too.
>
> Here's the diff:
>
>
> Index: ksh_limval.h
> ===================================================================
> RCS file: /cvs/src/bin/ksh/ksh_limval.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 ksh_limval.h
> --- ksh_limval.h 18 Dec 2004 20:55:52 -0000 1.2
> +++ ksh_limval.h 12 Sep 2015 18:59:00 -0000
> @@ -4,10 +4,6 @@
>  
>  /* limits.h is included in sh.h */
>  
> -#ifndef DMAXEXP
> -# define DMAXEXP 128 /* should be big enough */
> -#endif
> -
>  #ifndef BITS
>  # define BITS(t) (CHAR_BIT * sizeof(t))
>  #endif
> Index: shf.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/shf.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 shf.c
> --- shf.c 19 Apr 2013 17:36:09 -0000 1.16
> +++ shf.c 12 Sep 2015 18:59:00 -0000
> @@ -705,15 +705,7 @@ shf_smprintf(const char *fmt, ...)
>   return shf_sclose(&shf); /* null terminates */
>  }
>  
> -#undef FP /* if you want floating point stuff */
> -
>  #define BUF_SIZE 128
> -#define FPBUF_SIZE (DMAXEXP+16)/* this must be >
> - * MAX(DMAXEXP, log10(pow(2, DSIGNIF)))
> - *    + ceil(log10(DMAXEXP)) + 8 (I think).
> - * Since this is hard to express as a
> - * constant, just use a large buffer.
> - */
>  
>  /*
>   * What kinda of machine we on?  Hopefully the C compiler will optimize
> @@ -746,18 +738,6 @@ shf_smprintf(const char *fmt, ...)
>  #define FL_NUMBER 0x400 /* a number was formated %[douxefg] */
>  
>  
> -#ifdef FP
> -#include <math.h>
> -
> -static double
> -my_ceil(double d)
> -{
> - double i;
> -
> - return d - modf(d, &i) + (d < 0 ? -1 : 1);
> -}
> -#endif /* FP */
> -
>  int
>  shf_vfprintf(struct shf *shf, const char *fmt, va_list args)
>  {
> @@ -771,17 +751,6 @@ shf_vfprintf(struct shf *shf, const char
>   char numbuf[(BITS(long long) + 2) / 3 + 1];
>   /* this stuff for dealing with the buffer */
>   int nwritten = 0;
> -#ifdef FP
> - /* should be in <math.h>
> - *  extern double frexp();
> - */
> - extern char *ecvt();
> -
> - double fpnum;
> - int expo, decpt;
> - char style;
> - char fpbuf[FPBUF_SIZE];
> -#endif /* FP */
>  
>   if (!fmt)
>   return 0;
> @@ -952,134 +921,6 @@ shf_vfprintf(struct shf *shf, const char
>   precision = len; /* no loss */
>   }
>   break;
> -
> -#ifdef FP
> - case 'e':
> - case 'g':
> - case 'f':
> -    {
> - char *p;
> -
> - /*
> - * This could probably be done better,
> - *  but it seems to work.  Note that gcvt()
> - *  is not used, as you cannot tell it to
> - *  not strip the zeros.
> - */
> - flags |= FL_NUMBER;
> - if (!(flags & FL_DOT))
> - precision = 6; /* default */
> - /*
> - * Assumes doubles are pushed on
> - *  the stack.  If this is not so, then
> - *  FL_LLONG/FL_LONG/FL_SHORT should be checked.
> - */
> - fpnum = va_arg(args, double);
> - s = fpbuf;
> - style = c;
> - /*
> - *  This is the same as
> - * expo = ceil(log10(fpnum))
> - *  but doesn't need -lm.  This is an
> - *  approximation as expo is rounded up.
> - */
> - (void) frexp(fpnum, &expo);
> - expo = my_ceil(expo / LOG2_10);
> -
> - if (expo < 0)
> - expo = 0;
> -
> - p = ecvt(fpnum, precision + 1 + expo,
> - &decpt, &tmp);
> - if (c == 'g') {
> - if (decpt < -4 || decpt > precision)
> - style = 'e';
> - else
> - style = 'f';
> - if (decpt > 0 && (precision -= decpt) < 0)
> - precision = 0;
> - }
> - if (tmp)
> - *s++ = '-';
> - else if (flags & FL_PLUS)
> - *s++ = '+';
> - else if (flags & FL_BLANK)
> - *s++ = ' ';
> -
> - if (style == 'e')
> - *s++ = *p++;
> - else {
> - if (decpt > 0) {
> - /* Overflow check - should
> - * never have this problem.
> - */
> - if (decpt > &fpbuf[sizeof(fpbuf)] - s - 8)
> - decpt = &fpbuf[sizeof(fpbuf)] - s - 8;
> - (void) memcpy(s, p, decpt);
> - s += decpt;
> - p += decpt;
> - } else
> - *s++ = '0';
> - }
> -
> - /* print the fraction? */
> - if (precision > 0) {
> - *s++ = '.';
> - /* Overflow check - should
> - * never have this problem.
> - */
> - if (precision > &fpbuf[sizeof(fpbuf)] - s - 7)
> - precision = &fpbuf[sizeof(fpbuf)] - s - 7;
> - for (tmp = decpt;  tmp++ < 0 &&
> -    precision > 0 ; precision--)
> - *s++ = '0';
> - tmp = strlen(p);
> - if (precision > tmp)
> - precision = tmp;
> - /* Overflow check - should
> - * never have this problem.
> - */
> - if (precision > &fpbuf[sizeof(fpbuf)] - s - 7)
> - precision = &fpbuf[sizeof(fpbuf)] - s - 7;
> - (void) memcpy(s, p, precision);
> - s += precision;
> - /*
> - * `g' format strips trailing
> - *  zeros after the decimal.
> - */
> - if (c == 'g' && !(flags & FL_HASH)) {
> - while (*--s == '0')
> - ;
> - if (*s != '.')
> - s++;
> - }
> - } else if (flags & FL_HASH)
> - *s++ = '.';
> -
> - if (style == 'e') {
> - *s++ = (flags & FL_UPPER) ? 'E' : 'e';
> - if (--decpt >= 0)
> - *s++ = '+';
> - else {
> - *s++ = '-';
> - decpt = -decpt;
> - }
> - p = &numbuf[sizeof(numbuf)];
> - for (tmp = 0; tmp < 2 || decpt ; tmp++) {
> - *--p = '0' + decpt % 10;
> - decpt /= 10;
> - }
> - tmp = &numbuf[sizeof(numbuf)] - p;
> - (void) memcpy(s, p, tmp);
> - s += tmp;
> - }
> -
> - len = s - fpbuf;
> - s = fpbuf;
> - precision = len;
> - break;
> -    }
> -#endif /* FP */
>  
>   case 's':
>   if (!(s = va_arg(args, char *)))

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Michael McConville-2
Nicholas Marriott wrote:
> Works for me. ok anyone?
>
> I think ksh_limval.h can go entirely after this, per the note in
> PROJECTS.

I also just found this gem. It only has one use, so it can probably be
replaced. Am I interpreting it correctly?


Index: shf.c
===================================================================
RCS file: /cvs/src/bin/ksh/shf.c,v
retrieving revision 1.16
diff -u -p -r1.16 shf.c
--- shf.c 19 Apr 2013 17:36:09 -0000 1.16
+++ shf.c 13 Sep 2015 00:42:51 -0000
@@ -715,21 +715,6 @@ shf_smprintf(const char *fmt, ...)
  * constant, just use a large buffer.
  */
 
-/*
- * What kinda of machine we on?  Hopefully the C compiler will optimize
- *  this out...
- *
- * For shorts, we want sign extend for %d but not for %[oxu] - on 16 bit
- *  machines it don't matter.  Assumes C compiler has converted shorts to
- *  ints before pushing them.
- */
-#define POP_INT(f, s, a) \
- (((f) & FL_LLONG) ? va_arg((a), unsigned long long) : \
-    ((f) & FL_LONG) ? va_arg((a), unsigned long) : \
-    (sizeof(int) < sizeof(long) ? ((s) ? \
-    (long) va_arg((a), int) : va_arg((a), unsigned)) : \
-    va_arg((a), unsigned)))
-
 #define ABIGNUM 32000 /* big numer that will fit in a short */
 #define LOG2_10 3.321928094887362347870319429 /* log base 2 of 10 */
 
@@ -890,7 +875,7 @@ shf_vfprintf(struct shf *shf, const char
  case 'x':
  flags |= FL_NUMBER;
  s = &numbuf[sizeof(numbuf)];
- llnum = POP_INT(flags, c == 'd', args);
+ llnum = va_arg(args, unsigned long long);
  switch (c) {
  case 'd':
  case 'i':

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Michael McConville-2
In reply to this post by Nicholas Marriott-2
Nicholas Marriott wrote:
> Works for me. ok anyone?
>
> I think ksh_limval.h can go entirely after this, per the note in
> PROJECTS.

If we're in the business of deleting files, bin/ksh/INSTALL describes
itself with the following:

> [This file is the generic GNU autoconf/configure installation
> description, see the README for pdksh specific
> configuration/installation information]

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Nicholas Marriott-2
In reply to this post by Michael McConville-2
On Sat, Sep 12, 2015 at 08:45:43PM -0400, Michael McConville wrote:
> Nicholas Marriott wrote:
> > Works for me. ok anyone?
> >
> > I think ksh_limval.h can go entirely after this, per the note in
> > PROJECTS.
>
> I also just found this gem. It only has one use, so it can probably be
> replaced. Am I interpreting it correctly?

No this is not right. long can still be 32 or 64 bits on different
platforms (such as i386 (ILP32) and amd64 (I32LP64)).

You need to keep the flags checks because long long or a 64-bit long
take up more space on the stack than an int or a 32-bit long - if you
ask va_arg for a long long when there is only an int or a 32-bit long,
it will use the wrong size.

The second check is kind of confusing, because not only was the comment
clearly not updated when changing from short/int to int/long but the
macro itself was not updated when llnum was changed from unsigned long
to unsigned long long.

When llnum was unsigned long, the intent was that -ve numbers were sign
extended for %d but not %[oux]. So where int is 32 bits and long 64
bits, -1 to %d gave (unsigned long)(int)-1 but -1 to %x gave (unsigned
long)(unsigned int)-1. Where int and long are both 32 bits, it didn't
matter.

But llnum is now always 64 bits (unsigned long long) even when long is
32 bits, so the check is now wrong and will not sign extend when it
should (for example on i386).

So the (sizeof (int) < sizeof (long) condition can be removed - now that
llnum is long long, it should actually be sizeof (int) < sizeof (long
long), which is always true on OpenBSD. But we need to keep the true
branch not the false, so that the sign extension happens correctly for
[di] vs [oux].

There is another problem: %d is checked, but not %i - I think they
should be the same.

Also if it is concerned about sign extension for %d vs %x, then it
should be for %ld vs %lx too, because %ld and %d are the same on ILP32
platforms.

So the macro should possibly be expanded to something like:

        if (flags & FL_LLONG)
                llnum = va_arg(args, unsigned long long);
        else if (flags & FL_LONG) {
                if (c == 'd' || c == 'i')
                        llnum = va_arg(args, long);
                else
                        llnum = va_arg(args, unsigned long);
        } else {
                if (c == 'd' || c == 'i')
                        llnum = va_arg(args, int);
                else
                        llnum = va_arg(args, unsigned int);
        }

I hope this makes sense...

>
>
> Index: shf.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/shf.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 shf.c
> --- shf.c 19 Apr 2013 17:36:09 -0000 1.16
> +++ shf.c 13 Sep 2015 00:42:51 -0000
> @@ -715,21 +715,6 @@ shf_smprintf(const char *fmt, ...)
>   * constant, just use a large buffer.
>   */
>  
> -/*
> - * What kinda of machine we on?  Hopefully the C compiler will optimize
> - *  this out...
> - *
> - * For shorts, we want sign extend for %d but not for %[oxu] - on 16 bit
> - *  machines it don't matter.  Assumes C compiler has converted shorts to
> - *  ints before pushing them.
> - */
> -#define POP_INT(f, s, a) \
> - (((f) & FL_LLONG) ? va_arg((a), unsigned long long) : \
> -    ((f) & FL_LONG) ? va_arg((a), unsigned long) : \
> -    (sizeof(int) < sizeof(long) ? ((s) ? \
> -    (long) va_arg((a), int) : va_arg((a), unsigned)) : \
> -    va_arg((a), unsigned)))
> -
>  #define ABIGNUM 32000 /* big numer that will fit in a short */
>  #define LOG2_10 3.321928094887362347870319429 /* log base 2 of 10 */
>  
> @@ -890,7 +875,7 @@ shf_vfprintf(struct shf *shf, const char
>   case 'x':
>   flags |= FL_NUMBER;
>   s = &numbuf[sizeof(numbuf)];
> - llnum = POP_INT(flags, c == 'd', args);
> + llnum = va_arg(args, unsigned long long);
>   switch (c) {
>   case 'd':
>   case 'i':

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Nicholas Marriott-2
Here is a diff, also make %p better

Index: shf.c
===================================================================
RCS file: /cvs/src/bin/ksh/shf.c,v
retrieving revision 1.17
diff -u -p -r1.17 shf.c
--- shf.c 13 Sep 2015 19:43:42 -0000 1.17
+++ shf.c 14 Sep 2015 07:47:02 -0000
@@ -706,23 +706,7 @@ shf_smprintf(const char *fmt, ...)
 }
 
 #define BUF_SIZE 128
-
-/*
- * What kinda of machine we on?  Hopefully the C compiler will optimize
- *  this out...
- *
- * For shorts, we want sign extend for %d but not for %[oxu] - on 16 bit
- *  machines it don't matter.  Assumes C compiler has converted shorts to
- *  ints before pushing them.
- */
-#define POP_INT(f, s, a) \
- (((f) & FL_LLONG) ? va_arg((a), unsigned long long) : \
-    ((f) & FL_LONG) ? va_arg((a), unsigned long) : \
-    (sizeof(int) < sizeof(long) ? ((s) ? \
-    (long) va_arg((a), int) : va_arg((a), unsigned)) : \
-    va_arg((a), unsigned)))
-
-#define ABIGNUM 32000 /* big numer that will fit in a short */
+#define ABIGNUM 32000 /* big number that will fit in a short */
 #define LOG2_10 3.321928094887362347870319429 /* log base 2 of 10 */
 
 #define FL_HASH 0x001 /* `#' seen */
@@ -848,9 +832,8 @@ shf_vfprintf(struct shf *shf, const char
 
  switch (c) {
  case 'p': /* pointer */
- flags &= ~(FL_LLONG | FL_LONG | FL_SHORT);
- if (sizeof(char *) > sizeof(int))
- flags |= FL_LONG; /* hope it fits.. */
+ flags &= ~(FL_LLONG | FL_SHORT);
+ flags |= FL_LONG;
  /* aaahhh... */
  case 'd':
  case 'i':
@@ -859,7 +842,19 @@ shf_vfprintf(struct shf *shf, const char
  case 'x':
  flags |= FL_NUMBER;
  s = &numbuf[sizeof(numbuf)];
- llnum = POP_INT(flags, c == 'd', args);
+ if (flags & FL_LLONG)
+ llnum = va_arg(args, unsigned long long);
+ else if (flags & FL_LONG) {
+ if (c == 'd' || c == 'i')
+ llnum = va_arg(args, long);
+ else
+ llnum = va_arg(args, unsigned long);
+ } else {
+ if (c == 'd' || c == 'i')
+ llnum = va_arg(args, int);
+ else
+ llnum = va_arg(args, unsigned int);
+ }
  switch (c) {
  case 'd':
  case 'i':


On Sun, Sep 13, 2015 at 10:27:37AM +0100, Nicholas Marriott wrote:

> On Sat, Sep 12, 2015 at 08:45:43PM -0400, Michael McConville wrote:
> > Nicholas Marriott wrote:
> > > Works for me. ok anyone?
> > >
> > > I think ksh_limval.h can go entirely after this, per the note in
> > > PROJECTS.
> >
> > I also just found this gem. It only has one use, so it can probably be
> > replaced. Am I interpreting it correctly?
>
> No this is not right. long can still be 32 or 64 bits on different
> platforms (such as i386 (ILP32) and amd64 (I32LP64)).
>
> You need to keep the flags checks because long long or a 64-bit long
> take up more space on the stack than an int or a 32-bit long - if you
> ask va_arg for a long long when there is only an int or a 32-bit long,
> it will use the wrong size.
>
> The second check is kind of confusing, because not only was the comment
> clearly not updated when changing from short/int to int/long but the
> macro itself was not updated when llnum was changed from unsigned long
> to unsigned long long.
>
> When llnum was unsigned long, the intent was that -ve numbers were sign
> extended for %d but not %[oux]. So where int is 32 bits and long 64
> bits, -1 to %d gave (unsigned long)(int)-1 but -1 to %x gave (unsigned
> long)(unsigned int)-1. Where int and long are both 32 bits, it didn't
> matter.
>
> But llnum is now always 64 bits (unsigned long long) even when long is
> 32 bits, so the check is now wrong and will not sign extend when it
> should (for example on i386).
>
> So the (sizeof (int) < sizeof (long) condition can be removed - now that
> llnum is long long, it should actually be sizeof (int) < sizeof (long
> long), which is always true on OpenBSD. But we need to keep the true
> branch not the false, so that the sign extension happens correctly for
> [di] vs [oux].
>
> There is another problem: %d is checked, but not %i - I think they
> should be the same.
>
> Also if it is concerned about sign extension for %d vs %x, then it
> should be for %ld vs %lx too, because %ld and %d are the same on ILP32
> platforms.
>
> So the macro should possibly be expanded to something like:
>
> if (flags & FL_LLONG)
> llnum = va_arg(args, unsigned long long);
> else if (flags & FL_LONG) {
> if (c == 'd' || c == 'i')
> llnum = va_arg(args, long);
> else
> llnum = va_arg(args, unsigned long);
> } else {
> if (c == 'd' || c == 'i')
> llnum = va_arg(args, int);
> else
> llnum = va_arg(args, unsigned int);
> }
>
> I hope this makes sense...
>
> >
> >
> > Index: shf.c
> > ===================================================================
> > RCS file: /cvs/src/bin/ksh/shf.c,v
> > retrieving revision 1.16
> > diff -u -p -r1.16 shf.c
> > --- shf.c 19 Apr 2013 17:36:09 -0000 1.16
> > +++ shf.c 13 Sep 2015 00:42:51 -0000
> > @@ -715,21 +715,6 @@ shf_smprintf(const char *fmt, ...)
> >   * constant, just use a large buffer.
> >   */
> >  
> > -/*
> > - * What kinda of machine we on?  Hopefully the C compiler will optimize
> > - *  this out...
> > - *
> > - * For shorts, we want sign extend for %d but not for %[oxu] - on 16 bit
> > - *  machines it don't matter.  Assumes C compiler has converted shorts to
> > - *  ints before pushing them.
> > - */
> > -#define POP_INT(f, s, a) \
> > - (((f) & FL_LLONG) ? va_arg((a), unsigned long long) : \
> > -    ((f) & FL_LONG) ? va_arg((a), unsigned long) : \
> > -    (sizeof(int) < sizeof(long) ? ((s) ? \
> > -    (long) va_arg((a), int) : va_arg((a), unsigned)) : \
> > -    va_arg((a), unsigned)))
> > -
> >  #define ABIGNUM 32000 /* big numer that will fit in a short */
> >  #define LOG2_10 3.321928094887362347870319429 /* log base 2 of 10 */
> >  
> > @@ -890,7 +875,7 @@ shf_vfprintf(struct shf *shf, const char
> >   case 'x':
> >   flags |= FL_NUMBER;
> >   s = &numbuf[sizeof(numbuf)];
> > - llnum = POP_INT(flags, c == 'd', args);
> > + llnum = va_arg(args, unsigned long long);
> >   switch (c) {
> >   case 'd':
> >   case 'i':

Reply | Threaded
Open this post in threaded view
|

Re: Floating point #define in ksh

Ted Unangst-6
Nicholas Marriott wrote:
> Here is a diff, also make %p better

that looks better. sigh. why does this even exist?