adesutherland
array loses its contents
I'm stumped at the moment because when I comment out procedure transformCoordinates, the points array contains valid coordinates in writeSVG. When transformCoordinates is called, the points array only contains comma's.
/* rexx */
/* plot from stdin to svg - with a graphics capable shell like kitty */
/* freely inspired on Guff and Kate's awk version of it */
/* rvj 2023-07-21 v0 */
options levelb dashcomments
import rxfnsb
namespace rexxplot expose height width points
main: procedure
points=.string[]
i=0;
maxx=0
maxy=0
do while lines('testseq.txt')
i=i+1;
points[i]=linein('testseq.txt')
end
say 'read in' points[0]-1 'coordinates'
height= 450
width = 600
call transformCoordinates
call writeSVG
transformCoordinates: procedure
do i=1 to points[0]-1
say '<<<<<<' points[i]
if pos(',',points[i])>0 then commapos=pos(',',points[i])
say commapos '>>>' points[i]
x=left(points[i],commapos-1)
say 'x:' height-x
y=substr(points[i],commapos+1)
say 'y:' width-y
say '>>>>>>' points[i]
end
writeSVG: procedure
say "<?xml version='1.0'?>"
say "<svg xmlns='http://www.w3.org/2000/svg' width='"width"'",
"height='"height"' version='1.1'>"
call grid
say "<polyline stroke='#0072B2ff' stroke-width='1.5' fill='none' points='"
do j=1 to points[0]-1
say points[j]
end
say "'/>"
say "</svg>"
grid: procedure
say "<defs>"
say " <pattern id='smallGrid' width='8' height='8' patternUnits='userSpaceOnUse'>"
say " <path d='M 8 0 L 0 0 0 8' fill='none' stroke='gray' stroke-width='0.5'/>"
say " </pattern>"
say " <pattern id='grid' width='80' height='80' patternUnits='userSpaceOnUse'>"
say " <rect width='80' height='80' fill='url(#smallGrid)'/>"
say " <path d='M 80 0 L 0 0 0 80' fill='none' stroke='gray' stroke-width='1'/>"
say " </pattern>"
say "</defs>"
say "<rect width='1000' height='1000' fill='url(#grid)' />"
the input file testseq.txt contains (abbreviated but same symptoms):
1,1
2,2
3,3
4,4
5,5
6,6
7,7
8,8
9,9
10,10
I made a testcase which I think illustrates the problem, and I could use an extra pair of eyes on this:
options levelb dashcomments
import rxfnsb
namespace testcase expose points
points=.string[]
points[1]='1,1'
say 'before' points[1]
if pos(',',points[1])>0 then commapos=pos(',',points[1])
say commapos
say 'after' points[1]
say left(points[1],commapos-1)
say '-----'
if pos(',',points[1])>0 then commapos=pos(',',points[1])
say commapos '>>>' points[1]
x=left(points[1],commapos-1)
say 'x:' x
y=substr(points[1],commapos+1)
say 'y:' y
re-tested this (because some time has passed) with yesterday's revision of crexx:
before 1,1
2
after 1,1
1
-----
2 >>> 1,1
x: 1
y: 1
which I do not quite understand.
I can't see any malfunction. The rexx does exactly what it is supposed to do. Am I overseeing something?
well here is the whole thing:
/* rexx */
/* plot from stdin to svg - with a graphics capable shell like kitty */
/* freely inspired on Guff and Kate's awk version of it */
/* rvj 2023-07-21 v0 */
options levelb dashcomments
import rxfnsb
namespace rexxplot expose height width
points=.string[]
newpoints=.string[]
i=0;
maxx=0
maxy=0
do while lines('testseq.txt')
i=i+1;
points[i]=linein('testseq.txt')
/* say 'i='i points[i] */
end
height= 450
width = 600
newpoints=transformCoordinates(points)
say 'newpoints ===>' newpoints[0]
do i=1 to newpoints[0]-1
say '&&&^^^&&&' newpoints[i]
end
call writeSVG newpoints
transformCoordinates: procedure = .string[]
arg points=.string[]
do i=1 to points[0]-1
say 'in xform input:' points[i]
if pos(',',points[i])>0 then commapos=pos(',',points[i])
x=left(points[i],commapos-1)
y=substr(points[i],commapos+1)
x=height-x
y=width-y
points[i]=x','y
say 'in xform output:' points[i]
end
return points
writeSVG: procedure
arg coordinates=.string[]
say "<?xml version='1.0'?>"
say "<svg xmlns='http://www.w3.org/2000/svg' width='"width"'",
"height='"height"' version='1.1'>"
call grid
say "<polyline stroke='#0072B2ff' stroke-width='1.5' fill='none' points='"
do j=1 to coordinates[0]
say coordinates[j]
end
say "'/>"
say "</svg>"
grid: procedure
say "<defs>"
say " <pattern id='smallGrid' width='8' height='8' patternUnits='userSpaceOnUse'>"
say " <path d='M 8 0 L 0 0 0 8' fill='none' stroke='gray' stroke-width='0.5'/>"
say " </pattern>"
say " <pattern id='grid' width='80' height='80' patternUnits='userSpaceOnUse'>"
say " <rect width='80' height='80' fill='url(#smallGrid)'/>"
say " <path d='M 80 0 L 0 0 0 80' fill='none' stroke='gray' stroke-width='1'/>"
say " </pattern>"
say "</defs>"
say "<rect width='1000' height='1000' fill='url(#grid)' />"
It seems there is a impact on an array if used with some string functions, here is a minimal rexx code where it can be observed:
options levelb dashcomments
import rxfnsb
points=.string[]
points[1]='item1,col1'
points[2]='item2,col1'
newpoints=transformCoordinates(points)
say 1 'old ' points[1]
say 2 'old ' points[2]
exit
transformCoordinates: procedure = .string
arg points=.string[]
do i=1 to points[0]
if pos(',',points[i])>0 then commapos=pos(',',points[i])
x=left(points[i],1)
end
return 'OK'
running it, the passed array loses (parts of) content
116: 1 old ,
116: 2 old item2,col1
I suspect it is the POS function which strips the preceding characters, or sets a pointer to the variable wrongly.
It gets even creepier, the problem arises when the "if POS" is followed by the same POS in the THEN clause:
if pos(',',points[i])>0 then commapos=pos(',',points[i])
if you replace it by:
if pos(',',points[i])>0 then say pos(',',points[i])
it works.
I compared the assembly of both statements, I couldn't see a fundamental difference. If any of you have a flash of inspiration, that would be very helpful.
if pos(',',points[i])>0 then commapos=pos(',',points[i])
.src 17:3="if pos(',',points[i])>0"
load r4,3
load r5,","
settp r5,2
minattrs a1,r1,0
linkattr1 r2,a1,r1
settp r2,0
swap r6,r2
settp r7,0
call r8,pos(),r4
unlink r2
swap r2,r6
igt r8,r8,0
brf l75iffalse,r8
.src 17:28="then"
.src 17:33="abc=pos(',',points[i"
.meta "fpool_test_2.transformcoordinates.abc"="b" ".int" r7
load r9,3
load r10,","
settp r10,2
minattrs a1,r1,0
linkattr1 r6,a1,r1
settp r6,0
swap r11,r6
settp r12,0
call r7,pos(),r9
unlink r6
swap r6,r11
if pos(',',points[i])>0 then say pos(',',points[i])
src 17:3="if pos(',',points[i])>0"
load r4,3
load r5,","
settp r5,2
minattrs a1,r1,0
linkattr1 r2,a1,r1
settp r2,0
swap r6,r2
settp r7,0
call r8,pos(),r4
unlink r2
swap r2,r6
igt r8,r8,0
brf l74iffalse,r8
.src 17:28="then"
.src 17:33="say pos(',',points[i"
load r4,3
load r5,","
settp r5,2
minattrs a1,r1,0
linkattr1 r9,a1,r1
settp r9,0
swap r6,r9
settp r7,0
call r10,pos(),r4
unlink r9
swap r9,r6
Thanks for the simplified example - I can reproduce it. Am investigating.
Interesting, these commands seem to be the wrong way round (but manually changing it does not help)
unlink r2 swap r2,r6
Anyway, what we have is a linked attribute from an array being passed in as a parameter and then passed to another procedure. That kind of scenario.
A
I've pushed a potential compiler fix for this issue - @rvjansen @Peter-Jacob can you retest it?
I just saw the changes you made to left() to swap back registers:
swap r4,a1 /* Swap back - pedantic */
swap r6,a2 /* Swap back - pedantic */
swap r7,a3 /* Swap back - pedantic */
Do we have to check all our string functions to do this? What about functions written in rexx rather than plain asm?
I just saw the changes you made to left() to swap back registers:
swap r4,a1 /* Swap back - pedantic */ swap r6,a2 /* Swap back - pedantic */ swap r7,a3 /* Swap back - pedantic */Do we have to check all our string functions to do this? What about functions written in rexx rather than plain asm?
No. This was my initial change to see if it helped. In fact when you return from a procedure the registers are cleared down anyway.
But because of the swap I am wondering if the argument value will have changed in the calling program. I will test this. So I will give an definitive answer on this later.
In any case, for human readability I think good practice should be to swap them back although there is a performance penalty ...
For rexx programs the compiler swaps back.
In any case, for human readability I think good practice should be to swap them back although there is a performance penalty This is actually the reason I ask. Although it is "only" 3 swap instructions, they add up and degrade performance. We could add an instruction STM store multiple registers simultaneously and LM load multiple to restore them.
- just an idea
In any case, for human readability I think good practice should be to swap them back although there is a performance penalty This is actually the reason I ask. Although it is "only" 3 swap instructions, they add up and degrade performance. We could add an instruction STM store multiple registers simultaneously and LM load multiple to restore them.
- just an idea
Let me investigate. First I am going out to order a new car!
Well, the answer is both simple and complex.
The easy answer is that the 3 swaps at the end are not necessary - and are not necessary in general.
However, there is a clear risk that we need to consider if writing procedures in assembler. The VM allows a procedure to alter an argument, and that change is visible to the caller, and "swap" behaves as you would expect. For example:
.globals=0
main() .locals=5
load r1,"value"
sconcat r3,"y is:",r1
say r3
load r2,1
swap r3,r1
call r4,func(),r2
swap r1,r3
sconcat r3,"y is:",r1
say r3
ret
func() .locals=2
swap r1,a1
sconcat r1,r1,"(leaked)"
ret
Prints
y is: value
y is: value (leaked)
Despite the swaps taking place inside 'func' (between r1 and a1) and around the call to 'func' (between r3 and r1), the change to the argument cascades through to 'main'. This is as designed—I am rather pleased it works!
The compiler handles this by adding prelude code etc.
In sum, we don't need to do the swaps to "put things back" but we should be aware of how changes might affect the caller because the crexx calling convention places the responsibility on the called procedure to handle argument visibility rather than on the caller.
I know this is a long and rather pedantic answer :-) but I wanted to make it clear for myself!
It works as a call-by-reference instead of a call-by-value, which is perfectly fine for me - performance! At least in level B we should keep it that way.
- The example you used passes a simple string, so it's not an array problem, is it?
- When you say that the compiler handles it through prelude code, do you mean it does the appropriate swaps to keep the initial value of the passed parameter?
- When writing the basic string functions, I never considered that the passed parameters might have changed (think of looping over it). Do we need to rethink this?
- The example you used passes a simple string, so it's not an array problem, is it?
The specific issue was with linking and swapping registers and not "unwinding" in the right order.
What I have double-checked is this: both swaps and links to argument registers automatically unwind before returning from the procedure. Which is what we want.
- When you say that the compiler handles it through prelude code, do you mean it does the appropriate swaps to keep the initial value of the passed parameter?
This is an example of the code (it is either copying or swapping depending on if the caller cares about keeping its original value as indicated in the attribute flag)
brtpandt l29c,a1,2
scopy r1,a1
br l29d
l29c:
swap r1,a1
l29d:
- When writing the basic string functions, I never considered that the passed parameters might have changed (think of looping over it). Do we need to rethink this?
Any function you call should take care of that. You 'just' need to make sure your function does not mess up the caller's parameters. In practice, I would be surprised if any code does.
You can remove the swaps I added to left()
Also I think if we make left a standalone procedure, not calling substr(), and written in REXX it might be faster ...