->strstart cleanup

[jkeenan]

From our wiki page PreDecemberReleaseHackathon:

Remove uses of ->strstart outside of src/string. Either use Parrot_str_to_cstring/Parrot_str_free_cstring bufstart. See r40983 for examples of both and ask on #parrot if you're not sure.

There's a chart on that wiki page where you can check off instances as they are edited. We can use this TT for discussion and for more permanent tracking.

Thank you very much.
kid51

[jkeenan]

In r43009 and r43010, I edited 4 instances, 1 in src/utils.c and 3 in src/oo.c. Both these changes passed make test.

But since I'm just blindly following the editing instructions, please review. (I did not commit any changes that generated warnings or caused test failures.)

Thank you very much.
kid51

[jkeenan]

Replying to jkeenan:

In r43009 and r43010, I edited 4 instances, 1 in src/utils.c and 3 in src/oo.c. Both these changes passed make test. But since I'm just blindly following the editing instructions, please review. (I did not commit any changes that generated warnings or caused test failures.)

I'm finding that using the procedures described, t/pmc/eval.t is a crucial test. For example, making changes in ./src/packfile/pf_items.c, then running make corevm and make coretest, I got this:

t/pmc/eval.t ......................... 3/17 
#   Failed test 'eval.thaw'
#   at t/pmc/eval.t line 397.
# Exited with error code: 1
# Received:
# PackFile_unpack: This is not a valid Parrot bytecode file
# couldn't unpack packfile
# current instr.: 'main' pc 17 (/home/jimk/work/parrot/t/pmc/eval_12.pir:12)
# 
# Expected:
# hello from foo_1
# hello from foo_1
# 
# Looks like you failed 1 test of 17.
t/pmc/eval.t ......................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/17 subtests 

... at which point I bailed out of the tests and reverted the file.

Thank you very much.
kid51

[jkeenan]

On #parrot this morning, pmichaud casts a skeptical eye on 'blind replacement of strstart':

Is it noted somewhere that one cannot simply do a blind 
replacement of strstart with Buffer_bufstart ? At least, 
not until we refactor the substr opcode

iiuc, one can replace uses of ->strstart with Buffer_bufstart 
when it's known that the given string isn't a substring.  I'm 
not sure one can always know when that's the case.  Otherwise 
we get to issues noted in the thread surrounding r42961.

just be careful that the solution doesn't end up being "create 
a new string buffer".  That will likely kill PGE and NQP 
performance (and everything that depends on them)

I don't know how many operations are being tested using 
substring operands as opposed to whole strings.  That's 
how I ended up with the failure case in r42961 -- the 
original change was assuming that substrings would 
never be used as an argument to that function.  It's 
not common to test every function for use with every 
possible type of string, at least not yet.  Anyway, 
I wouldn't expect most of the changes made thus far 
to seriously impact PGE or NQP, but since (by necessity 
and design) they make heavy use of substrings, and they 
tend to produce a lot of substring results, it's entirely 
possible that some of the changes would cause things to 
break that we aren't testing.

[jkeenan]

So, speaking of 'blind replacement of strstart', here's what I got when I covered my eyes and worked on src/pmc/eval.c. The replacements occur in two groups.

Let's take the second group first:

Index: src/pmc/eval.pmc
===================================================================
--- src/pmc/eval.pmc    (revision 43016)
+++ src/pmc/eval.pmc    (working copy)
...
@@ -351,7 +351,7 @@
         SUPER(info);
         pf = PackFile_new(INTERP, 0);
 
-        if (!PackFile_unpack(INTERP, pf, (opcode_t *)packed->strstart,
+        if (!PackFile_unpack(INTERP, pf, (opcode_t *)Buffer_bufstart(packed),
                 packed->strlen))
             Parrot_ex_throw_from_c_args(INTERP, NULL, EXCEPTION_EXTERNAL_ERROR,
                 "couldn't unpack packfile");

This replacement 'worked' in the sense that it passed make corevm and make coretest.

However, the first group, while it passed make corevm, failed make coretest -- specifically, t/pmc/eval.t.

Index: src/pmc/eval.pmc
===================================================================
--- src/pmc/eval.pmc    (revision 43016)
+++ src/pmc/eval.pmc    (working copy)
@@ -281,10 +281,10 @@
                                          aligned_size);
         res->strlen  = res->bufused = size;
 
-        if ((size_t)(res->strstart) & 0xf) {
-            char *adr     = res->strstart;
+        if ((size_t)(Buffer_bufstart(res)) & 0xf) {
+            char *adr     = Buffer_bufstart(res);
             adr          += 16 - ((size_t)adr & 0xf);
-            res->strstart = adr;
+            Buffer_bufstart(res) = adr;
         }
 
         /* We block GC while doing the packing, since GC run during a pack

The output of prove -v t/pmc/eval.t is large. See attachment.

kid51

[jkeenan]

Impact of naive replacment of strstart in src/pmc/eval.pmc at some locations

[jkeenan]

This patch to src/io/socket_unix.c, considered by itself, passes make coretest and make test.

Index: src/io/socket_unix.c
===================================================================
--- src/io/socket_unix.c	(revision 43018)
+++ src/io/socket_unix.c	(working copy)
@@ -301,7 +301,7 @@
     /*
      * Ignore encoding issues for now.
      */
-    if ((error = send(io->os_handle, (char *)s->strstart + byteswrote,
+    if ((error = send(io->os_handle, (char *)Buffer_bufstart(s) + byteswrote,
                     bytes, 0)) >= 0) {
         byteswrote += error;
         bytes -= error;

Whether it satisfies the concerns Patrick raises I cannot say.

kid51

[jkeenan]

Replying to jkeenan: chromatic noted on #parrot:

kid51, if the string used in IO is the result of a substr() operation, the logical string will not start at the start of the buffer.

So I don't think this would meet Patrick's concerns.

[doughera]

>  just be careful that the solution doesn't end up being "create
>  a new string buffer".  That will likely kill PGE and NQP
>  performance (and everything that depends on them)

It's unclear to me what the point of such blind replacement would be 
anyway.  Replacing peeking at the ->strstart entry by peeking at the 
->bufstart entry (even if encapsulated in a macro) is still peeking at the 
internals of a STRING representation to find the underlying C string.

-- 
    Andy Dougherty		doughera@lafayette.edu

[jkeenan]

Replying to jkeenan:

Reviewing this nine-month-old discussion, it appears that quite a few knowledgeable contributors disagreed with the wisdom of this goal:

Remove uses of ->strstart outside of src/string. Either use Parrot_str_to_cstring/Parrot_str_free_cstring bufstart. See r40983 for examples of both and ask on #parrot if you're not sure.

If the goal is fundamentally unwise, then we should close this ticket. I will do so in seven days if there are no objections.

Thank you very much.

kid51

[jkeenan]

See also TT #630.

[jkeenan]

Sounding the final call for comments on the wisdom of keeping this effort going!

[jkeenan]

To summarize how we've gotten to this point ...

One objective set prior to a weekend pre-release hackathon in Dec 2009 was:

Remove uses of '->strstart' outside of src/string.
Either use Parrot_str_to_cstring/Parrot_str_free_cstring bufstart.

Some progress was made toward this objective and is documented on the wiki at [wiki PreDecemberReleaseHackathon]. However, as a review of earlier posts in this ticket will indicate, the objective itself was subsequently called into question and there has been no forward momentum in the last nine months. Moreover, no one has responded to my recent posts asking for a rationale for continued efforts to that objective.

Hence, I am closing this ticket. If at any point in the future you can identify locations where ->strstart could be further eliminated, please open a new ticket and post a patch.

Thank you very much.

kid51