Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unpack 'a3#a*' misbehaving #389

Closed
p5pRT opened this issue Aug 17, 1999 · 8 comments
Closed

unpack 'a3#a*' misbehaving #389

p5pRT opened this issue Aug 17, 1999 · 8 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 17, 1999

Migrated from rt.perl.org#1224 (status was 'resolved')

Searchable as RT1224$

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 1999

From ilya@math.ohio-state.edu

  DB<1> x unpack 'Z3#a*', pack 'Z3#a*', '123456789012345'
  0 123456789012345

OK.

  DB<2> x pack 'a3#a*', '123456789012345'
  0 "15\c@​123456789012345"

OK.

  DB<3> x unpack 'a3#a*', pack 'a3#a*', '123456789012345'
  Argument "15" isn't numeric in unpack at (eval 7) line 2, <IN> line 3.
  eval '($@​, $!, $^E, $,, $/, $\\, $^W) = @​saved;package main; $^D = $^D | $DB​::db_stop;
  unpack \'a3#a*\', pack \'a3#a*\', \'123456789012345\';

  ;' called at lib/perl5db.pl line 1246
  DB​::eval called at lib/perl5db.pl line 1140
  DB​::DB called at -e line 1
  0 ''

The warning is understandable, but it is only a warning! As you see,
the result is empty, not 123456789012345 as expected.

Ilya


Site configuration information for perl 5.00560​:

Configured by ilya at Wed Aug 4 22​:44​:30 EDT 1999.

Summary of my perl5 (revision 5.0 version 5 subversion 60) configuration​:
  Platform​:
  osname=solaris, osvers=2.7, archname=sun4-solaris
  uname='sunos monk 5.7 generic_106541-05 sun4u sparc '
  config_args='-des'
  hint=recommended, useposix=true, d_sigaction=define
  usethreads=undef useperlio=undef d_sfio=undef
  use64bits=undef usemultiplicity=undef
  Compiler​:
  cc='cc', optimize='-O', gccversion=
  cppflags='-I/opt/local/include -I/opt/gnu/include'
  ccflags ='-I/opt/local/include -I/opt/gnu/include'
  stdchar='char', d_stdstdio=define, usevfork=false
  intsize=4, longsize=4, ptrsize=4, doublesize=8
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
  alignbytes=8, usemymalloc=y, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -L/opt/local/lib -L/opt/gnu/lib'
  libpth=/opt/local/lib /opt/gnu/lib /lib /usr/lib /usr/ccs/lib
  libs=-lsocket -lnsl -lgdbm -ldl -lm -lrt -lc -lcrypt -lsec
  libc=/lib/libc.so, so=so, useshrplib=false, libperl=libperl.a
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' '
  cccdlflags='-KPIC', lddlflags='-G -L/opt/local/lib -L/opt/gnu/lib'

Locally applied patches​:
 


@​INC for perl 5.00560​:
  lib
  /home/ilya/perl
  /home/ilya/perl/lib/perl5
  /home/ilya/perl/lib/perl5/site_perl
  /home/ilya/perl/lib/perl5/site_perl/sun4-solaris
  /home/ilya/perl/lib/perl5/site_perl/5.005/sun4-solaris
  /home/ilya/perl/lib/perl5/5.00503/
  /opt/lib/perl5/5.00560/sun4-solaris
  /opt/lib/perl5/5.00560
  /opt/lib/site_perl/5.00560/sun4-solaris
  /opt/lib/site_perl
  .


Environment for perl 5.00560​:
  HOME=/home/ilya
  LANG (unset)
  LANGUAGE (unset)
  LD_LIBRARY_PATH=/opt/local/lib​:/usr/openwin/lib​:/opt/local/lib/rvplayer5.0​:/opt/X11/lib
  LOGDIR (unset)
  PATH=/home/ilya/bin​:/opt/SUNWspro/bin​:/usr/dt/bin​:/usr/ccs/bin​:/opt/java/bin​:/opt/netscape​:/opt/microsoft/bin​:/opt/gnu/bin​:/usr/openwin/bin​:/opt/X11/bin​:/opt/unsup/beta/bin​:/home/tex/bin​:/opt/local/bin​:/usr/bin​:/opt/local/games​:/opt/unsup/gnu/bin​:/opt/unsup/ilya/bin​:.
  PERLLIB=/home/ilya/perl​:/home/ilya/perl/lib/perl5​:/home/ilya/perl/lib/perl5/site_perl​:/home/ilya/perl/lib/perl5/site_perl/sun4-solaris​:/home/ilya/perl/lib/perl5/site_perl/5.005/sun4-solaris​:/home/ilya/perl/lib/perl5/5.00503/
  PERL_BADLANG (unset)
  SHELL=/usr/bin/tcsh

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 1999

From [Unknown Contact. See original ticket]

On Tue, 17 Aug 1999 at 03​:37​:30 -0400, Ilya Zakharevich wrote​:

  DB\<3> x unpack 'a3\#a\*'\, pack 'a3\#a\*'\, '123456789012345'
Argument "15" isn't numeric in unpack at \(eval 7\) line 2\, \<IN> line 3\.
    eval '\($@&#8203;\, $\!\, $^E\, $\,\, $/\, $\\\\\, $^W\) = @&#8203;saved;package main; $^D = $^D | $DB&#8203;::db\_stop;
  unpack \\'a3\#a\*\\'\, pack \\'a3\#a\*\\'\, \\'123456789012345\\';

[and the unpack gets a zero figure for the length because of the
embedded null]

I just used POPi to pick up the number, and this calls Perl_sv_2iv. This
seems to be less forgiving than other conversion routines. Anyhow,
looking at Perl_sv_2iv, there's only one place I can see that makes it
emit the warning, and, sure enough, it sets the returned value to zero.
Most other circumstances (I've not traced them) seem to lead to the
warning being emitted, but nevertheless a "best guess" being returned.

So​: Am I misusing POPi [pp.c line 3399 or so], or is there a bug in
Perl_sv_2iv?

And, I can't help feeling that /\d+\0+/ looks like a bona-fide number to
me. Is the warning justified at all? There's no warning for /\d+\s+/.

Ian

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 1999

From [Unknown Contact. See original ticket]

Ian Phillipps <ian@​dial.pipex.com> writes​:

And, I can't help feeling that /\d+\0+/ looks like a bona-fide number to
me. Is the warning justified at all? There's no warning for /\d+\s+/.

\d+\0 looks _more_ like a number to me than \d+\s !

--
Nick Ing-Simmons <nik@​tiuk.ti.com>
Via, but not speaking for​: Texas Instruments Ltd.

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 1999

From [Unknown Contact. See original ticket]

On Wed, Aug 18, 1999 at 12​:47​:09PM +0100, Ian Phillipps wrote​:

On Tue, 17 Aug 1999 at 03​:37​:30 -0400, Ilya Zakharevich wrote​:

  DB\<3> x unpack 'a3\#a\*'\, pack 'a3\#a\*'\, '123456789012345'
Argument "15" isn't numeric in unpack at \(eval 7\) line 2\, \<IN> line 3\.
    eval '\($@&#8203;\, $\!\, $^E\, $\,\, $/\, $\\\\\, $^W\) = @&#8203;saved;package main; $^D = $^D | $DB&#8203;::db\_stop;
  unpack \\'a3\#a\*\\'\, pack \\'a3\#a\*\\'\, \\'123456789012345\\';

[and the unpack gets a zero figure for the length because of the
embedded null]

I just used POPi to pick up the number, and this calls Perl_sv_2iv. This
seems to be less forgiving than other conversion routines.

What is/was strange to me is that POPi is used in pp_add as well, but
it gives a correct result​:

perl -wle "$two = qq(2\0); print 2+$two"
Argument "2" isn't numeric in add at -e line 1.
4

Aha! It is not POPi, it is POPn!

  > perl -Minteger -wle "$two = qq(2\0); print 2+$two"
  Argument "2" isn't numeric in i_add at -e line 1.
  2

So it is *definitely* a bug in sv_2iv.

Ilya

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 1999

From [Unknown Contact. See original ticket]

On Wed, 18 Aug 1999 at 17​:50​:59 +0100, Nick Ing-Simmons wrote​:

Ian Phillipps <ian@​dial.pipex.com> writes​:

And, I can't help feeling that /\d+\0+/ looks like a bona-fide number to
me. Is the warning justified at all? There's no warning for /\d+\s+/.

\d+\0 looks _more_ like a number to me than \d+\s !

I propose this, which will make Ilya's case work OK, but doesn't fix the
inconsistency with sv_2iv. This returns 0 if there's a warning, unlike the
other sv_2xv routines, which run the conversion anyway.

My patch will stop the looks_like_number scan at the first "\0", which
is after all what atof() does. Hence, "123\0foo" will now be converted
to a number without any warnings.

None of the 1440 op/numconvert tests is affected by this patch :-)

(My apologies if this doesn't go cleanly on to 60...)

Inline Patch
--- sv.c.orig	Mon Aug  2 22:29:12 1999
+++ sv.c	Thu Aug 19 12:31:18 1999
@@ -1635,5 +1635,5 @@
     while (isSPACE(*s))
 	s++;
-    if (s >= send)
+    if (s >= send || *s == 0)
 	return numtype;
     if (len == 10 && memEQ(sbegin, "0 but true", 10))

Ian

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 1999

From [Unknown Contact. See original ticket]

On Thu, Aug 19, 1999 at 12​:56​:25PM +0100, Ian Phillipps wrote​:

Ian Phillipps <ian@​dial.pipex.com> writes​:

And, I can't help feeling that /\d+\0+/ looks like a bona-fide number to
me. Is the warning justified at all? There's no warning for /\d+\s+/.

\d+\0 looks _more_ like a number to me than \d+\s !

I propose this, which will make Ilya's case work OK, but doesn't fix the
inconsistency with sv_2iv. This returns 0 if there's a warning, unlike the
other sv_2xv routines, which run the conversion anyway.

My patch will stop the looks_like_number scan at the first "\0", which
is after all what atof() does. Hence, "123\0foo" will now be converted
to a number without any warnings.

I would prefer sv_2iv() call conversion routine after emitting a
warning. (I see no value in the testcase I sent - one can always use
'A' instead of 'a' in the unpack specifier.)

None of the 1440 op/numconvert tests is affected by this patch :-)

Well, they fence a different can of worms...

Ilya

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 1999

From [Unknown Contact. See original ticket]

On Thu, 19 Aug 1999 at 14​:32​:08 -0400, Ilya Zakharevich wrote​:

On Thu, Aug 19, 1999 at 12​:56​:25PM +0100, Ian Phillipps wrote​:

Ian Phillipps <ian@​dial.pipex.com> writes​:

Ian 1> I can't help feeling that /\d+\0+/ looks like a bona-fide number to
Ian 1> me. Is the warning justified at all? There's no warning for /\d+\s+/.

Nick > \d+\0 looks _more_ like a number to me than \d+\s !

Ian 2 > I propose this, which will make Ilya's case work OK, but doesn't fix the
Ian 2 > inconsistency with sv_2iv. This returns 0 if there's a warning, unlike the
Ian 2 > other sv_2xv routines, which run the conversion anyway.

Ian 2 > My patch will stop the looks_like_number scan at the first "\0", which
Ian 2 > is after all what atof() does. Hence, "123\0foo" will now be converted
Ian 2 > to a number without any warnings.

Ilya > I would prefer sv_2iv() call conversion routine after emitting a
Ilya > warning. (I see no value in the testcase I sent - one can always use
Ilya > 'A' instead of 'a' in the unpack specifier.)

This patchlet was in support of Nick's agreement with my suggestion that
"123\0" should be treated as a fully legitimate number. I still think
it's Good Thing and crave the attention of the pumpkings for it,
independent of the sv_2iv issue.

Fix for sv_2iv​:

Unfortunately, looks_like_number has two functions​:
  (a) is it a number? and
  (b) what sort of number is it (integer/rational)?
looks_like_number has no concept of 'it looks most like an integer',
and reports 0 whatever's wrong. Hence this patch assumes the worst case.
It includes my previous patch at the end, which can be snipped out if
not to taste. I assume the dTHR is needed for the warning.

Inline Patch
--- sv.c.orig	Mon Aug  2 22:29:12 1999
+++ sv.c	Thu Aug 19 23:26:25 1999
@@ -1156,4 +1156,11 @@
 	I32 numtype = looks_like_number(sv);
 
+	if (!numtype) {
+	    dTHR;
+	    if (ckWARN(WARN_NUMERIC))
+		not_a_number(sv);
+	    numtype = IS_NUMBER_NOT_IV;   /* Assume the worst case */
+	}
+
 	/* We want to avoid a possible problem when we cache an IV which
 	   may be later translated to an NV, and the resulting NV is not
@@ -1191,5 +1198,5 @@
 	    }
 	}
-	else if (numtype) {
+	else {
 	    /* The NV may be reconstructed from IV - safe to cache IV,
 	       which may be calculated by atol(). */
@@ -1199,14 +1206,4 @@
 	    SvIVX(sv) = Atol(SvPVX(sv));
 	}
-	else {				/* Not a number.  Cache 0. */
-	    dTHR;
-
-	    if (SvTYPE(sv) < SVt_PVIV)
-		sv_upgrade(sv, SVt_PVIV);
-	    SvIVX(sv) = 0;
-	    (void)SvIOK_on(sv);
-	    if (ckWARN(WARN_NUMERIC))
-		not_a_number(sv);
-	}
     }
     else  {
@@ -1635,5 +1632,5 @@
     while (isSPACE(*s))
 	s++;
-    if (s >= send)
+    if (s >= send || *s == 0)
 	return numtype;
     if (len == 10 && memEQ(sbegin, "0 but true", 10))

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 1999

From [Unknown Contact. See original ticket]

On Thu, Aug 19, 1999 at 11​:34​:32PM +0100, Ian Phillipps wrote​:

Ilya > I would prefer sv_2iv() call conversion routine after emitting a
Ilya > warning. (I see no value in the testcase I sent - one can always use
Ilya > 'A' instead of 'a' in the unpack specifier.)

This patchlet was in support of Nick's agreement with my suggestion that
"123\0" should be treated as a fully legitimate number. I still think
it's Good Thing and crave the attention of the pumpkings for it,
independent of the sv_2iv issue.

I would prefer things to remain as they are. I saw no real-line
complaints that looks_like_number() is too picky, and I can imagine a
lot of situations when "123\0" is a result of a bug in a user code.

Thus IMO there is no value in removing this strictness, while there is
a value in preserving it.

Unfortunately, looks_like_number has two functions​:
(a) is it a number? and
(b) what sort of number is it (integer/rational)?
looks_like_number has no concept of 'it looks most like an integer',
and reports 0 whatever's wrong.

But why this is an issue for sv_2iv() but not for sv_2nv()?

I find your argument not very convincing.

Ilya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant