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':
