#869 is inpgstr.c INPgetStr faulty?

[Jason Pyeron]

(feel free to assign patch creation to me)

I ran into a situation where my quoted parameters (IF_STRING) were being truncated.

I traced it down to the now find all good characters logic.

Instinctively I made a patch, but then I realized this could introduce regressions.

Should I introduce a:

#define IF_STRING_PROPER (IF_STRING | 0x100)

or fix INPgetStr knowing that some esoteric code somewhere was relying bad behavior e.g.:

• x=" " to become the empty string
• both x="fast" and x="fast mode" to become "fast"
• foo="fast,debug" to become foo=fast and debug

The scope of this is much wider than my experience... For now and locally I am using the IF_STRING_PROPER approach, but I am suspecting this is a rip the band-aid off moment.

[Jason Pyeron]

Note: the test suite does not seem to have coverage for this and both patches do not change the test outcomes.

[Jason Pyeron]

the current behavior seems to conflict the examples in the manual. But each of those examples seems to bypass INPgetStr.

But CIDER rootfile and IC.FILE examples would go through INPgetStr and filenames with offending characters will cause issues.

[Giles Atkinson]

A valid bug report contains the description of a bug. Steps for reproduction and any needed inputs should be included.

[Jason Pyeron]

My local patch is how I found it. I do not yet have test cases for rootfile and IC.FILE.

but the test cases are articulated above....

x=" " to become the empty string
both x="fast" and x="fast mode" to become "fast"
foo="fast,debug" to become foo=fast and debug

Most importantly, as I tried to say above - I dont have the historical knowledge to know if INPgetStr was designed on purpose to act so weirdly! If someone, with historical knowlege could answer that, then I can write test cases, and submit patch.

But if this is "as designed" then so be it.

[Jason Pyeron]

to clarify the initial report:

c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 40)     /* now find all good characters */
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 41)     for (point = *line; *point != '\0'; point++) {
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 42)  if ((*point == ' ') ||
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 43)      (*point == '\t') ||
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 44)      (*point == '=') ||
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 45)      (*point == '(') ||
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 46)      (*point == ')') || (*point == ',') || (*point == separator))
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 47)      break;
c4e2e82d42 src/circuit/inpgstr.c         (arno   2000-05-25 20:25:40 +0000 48)     }

[Jason Pyeron]

Create 2 files, the first is from the manual section 14.7.2.1 page 499:

****** jfet1.cir ******
*Two-dimensional Junction Field-Effect Transistor (JFET)
VDD 1 0 0.5V
VGG 2 0 -1.0v AC 1V
VSS 3 0 0.0V
QJ1 1 2 3 M_NJF AREA=1 SAVE
.MODEL M_NJF NBJT LEVEL=2

+ options jfet defw=10.0um
+ output rootfile="./j1root/" psi n.conc p.conc phin phip equ.psi vac.psi
+ x.mesh w=0.2 h.e=0.001 r=1.8
+ x.mesh w=0.8 h.s=0.001 h.m=0.1 r=2.0
+ x.mesh w=0.8 h.e=0.001 h.m=0.1 r=2.0
+ x.mesh w=0.2 h.s=0.001 r=1.8
+ y.mesh w=0.2 h.e=0.01 r=1.8
+ y.mesh w=0.8 h.s=0.01 h.m=0.1 r=1.8
+
+ domain num=1 mat=1
+ material num=1 silicon
+
+ elec num=1 x.l=0.0 x.h=0.0 y.l=0.0 y.h=1.0
+ elec num=2 x.l=0.5 x.h=1.5 y.l=0.0 y.h=0.0
+ elec num=3 x.l=2.0 x.h=2.0 y.l=0.0 y.h=1.0
+
+ doping unif n.type conc=3.0e15
+ doping unif p.type conc=2.0e17 x.l=0.2 x.h=1.8 y.h=0.2
+
+ models bgn srh auger conctau concmob fieldmob ^aval
.option bypass=1 temp=27
.control
dc vgg 0.0 -2.0001 -0.1
print i(vss)
load ./j1root/DC.12.qj1
shell 'sleep 1'
gnuplot gplot1 xycontour phin
shell 'sleep 1'
gnuplot gplot2 xycontour sqrt((ex * ex) + (ey * ey))
shell 'sleep 1'
set gnuplot_terminal=png/quit
gnuplot gplot3 xycontour phin
shell 'sleep 1'
gnuplot gplot4 xycontour sqrt((ex * ex) + (ey * ey))
shell 'sleep 1'
quit
.endc
.end

and then the same file, where rootfile is modified to use () in the name

****** jfet1-869.cir ******
*Two-dimensional Junction Field-Effect Transistor (JFET)
VDD 1 0 0.5V
VGG 2 0 -1.0v AC 1V
VSS 3 0 0.0V
QJ1 1 2 3 M_NJF AREA=1 SAVE
.MODEL M_NJF NBJT LEVEL=2

+ options jfet defw=10.0um
+ output rootfile="./j1root-bug(869)/" psi n.conc p.conc phin phip equ.psi vac.psi
+ x.mesh w=0.2 h.e=0.001 r=1.8
+ x.mesh w=0.8 h.s=0.001 h.m=0.1 r=2.0
+ x.mesh w=0.8 h.e=0.001 h.m=0.1 r=2.0
+ x.mesh w=0.2 h.s=0.001 r=1.8
+ y.mesh w=0.2 h.e=0.01 r=1.8
+ y.mesh w=0.8 h.s=0.01 h.m=0.1 r=1.8
+
+ domain num=1 mat=1
+ material num=1 silicon
+
+ elec num=1 x.l=0.0 x.h=0.0 y.l=0.0 y.h=1.0
+ elec num=2 x.l=0.5 x.h=1.5 y.l=0.0 y.h=0.0
+ elec num=3 x.l=2.0 x.h=2.0 y.l=0.0 y.h=1.0
+
+ doping unif n.type conc=3.0e15
+ doping unif p.type conc=2.0e17 x.l=0.2 x.h=1.8 y.h=0.2
+
+ models bgn srh auger conctau concmob fieldmob ^aval
.option bypass=1 temp=27
.control
dc vgg 0.0 -2.0001 -0.1
print i(vss)
load ./j1root-bug(869)/DC.12.qj1
shell 'sleep 1'
gnuplot gplot1 xycontour phin
shell 'sleep 1'
gnuplot gplot2 xycontour sqrt((ex * ex) + (ey * ey))
shell 'sleep 1'
set gnuplot_terminal=png/quit
gnuplot gplot3 xycontour phin
shell 'sleep 1'
gnuplot gplot4 xycontour sqrt((ex * ex) + (ey * ey))
shell 'sleep 1'
quit
.endc
.end

then:

mkdir -v 'j1root' 'j1root-bug(869)'
./src/ngspice jfet.cir
./src/ngspice jfet-869.cir

So I ask, is INPgetStr working as designed? That is was there a historical reason ' ', '\t', '(', ')', and ',' are terminators for strings? It seems too deliberate to be an accident.

I dont want to make a fix that breaks backwards compatability....

[Giles Atkinson]

Thanks, that makes it clear. My guess is that it really is working as designed, and has been for 41 years. But there were likely no filenames in model specifications then. Now it's terrible!

Stopping on those "garbage" characters seems reasonable, as in some contexts they are punctuation. But once an opening quote is seen, everything up to a matching quote or the terminating null should be accepted, as should an escaped matching quote (\"). Otherwise, why parse quotes at all?