change function of get_addr and set_addr

[coke]

I would expect the following code to print out 'a' twice, once for each type. This patch solves the ticket. But I'm not sure how "kosher" is it.

.sub main
  sortme('ResizablePMCArray')

  $P1 = get_class 'ResizablePMCArray'
  $P0 = subclass $P1, 'RPA'

  sortme('RPA')

.end

.sub sortme
  .param string type
  $P0 = new type
  $P0[0]='z'
  $P0[1]='a'
  .local pmc comparator
  comparator = get_global 'ascii'
  $P0.'sort'(comparator)
  $S0 = $P0[0]
  say $S0
.end

.sub ascii
  .param string a
  .param string b
  $I0 = cmp_str a,b
  .return($I0)
.end

Instead, it generates:

a
Method 'sort' not found for invocant of class 'RPA'
current instr.: 'sortme' pc 53 (foo.pir:21)
called from Sub 'main' pc 25 (foo.pir:10)

sort is a method on FixedPMCArray; it's inherited by esizablePMCArray. It is apparently then NOT inherited by my PIR level subclass.

[pmichaud]

This is a duplicate of RT #54520 -- so look there for further examples.

Pm

[whiteknight]

I'm looking at the RT ticket which seems to indicate that this is a problem with class instantiation. I'm looking at the relevant code in src/pmc/class.pmc and src/pmc/object.pmc, but don't see any obvious errors that would prevent methods from being properly inherited by subclasses. I'm going to run some tests today, and when I get home tonight I'll run it through GDB to try and figure out where the problem lies.

[whiteknight]

This isn't exactly a duplicate of RT #54520. That ticket involves changing the MRO of a class that's already been instantiated. That is not the case in this ticket where the MRO is not being modified at all after instantiating the ResizablePMCArray and the RPA classes.

On cursory inspection, the relevant code in object.pmc and class.pmc looks to be doing The Right Thing. I'll try to look at this issue later tonight in the debugger to see what's going on here.

[whiteknight]

As a quick note, as of r38214 on Ubuntu 9.04 x86_64 the test code above causes a segfault/coredump instead of printing out the error message. This is obviously a change from how it was failing, and it's a change in the wrong direction.

[whiteknight]

In r38217 I made a small change to the sort method. Previously, it was using the macro PMC_size() to determine the number of elements in the array, which only worked with FixedPMCArray PMCs, and not subclasses. So I changed that to a call to VTABLE_elements, which solves that one problem.

Of course, that didn't solve the ticket entirely (or I would have closed it). It gets past the part where it was failing previously, and now segfaults somewhere in the call to the comparison PIR function. I don't have time to dig into it tonight. Here's the backtrace in case anybody wants to play with it:

Program received signal SIGSEGV, Segmentation fault.
0x00007f1f053c5382 in Parrot_convert_arg (interp=0x6fd080, st=0x7fff0db07860)
    at src/call/pcc.c:1126
1126	            UVal_str(st->val) = VTABLE_get_string(interp, UVal_pmc(st->val));
(gdb) bt
#0  0x00007f1f053c5382 in Parrot_convert_arg (interp=0x6fd080, 
    st=0x7fff0db07860) at src/call/pcc.c:1126
#1  0x00007f1f053c5ad2 in Parrot_process_args (interp=0x6fd080, 
    st=0x7fff0db07860, param_or_result=<value optimized out>)
    at src/call/pcc.c:1705
#2  0x00007f1f053c6942 in parrot_pass_args_fromc (interp=0x6fd080, 
    sig=0x7f1f0556b42c "PP", dest=0x85fef8, old_ctxp=0x85bee0, 
    ap=0x7fff0db07a00) at src/call/pcc.c:1927
#3  0x00007f1f053c9c39 in runops_args (interp=0x6fd080, sub=0x7f9480, 
    obj=0x7862c0, meth_unused=<value optimized out>, sig=0x7f1f0556b42b "IPP", 
    ap=0x7fff0db07a00) at src/call/ops.c:241
#4  0x00007f1f053ca377 in Parrot_runops_fromc_args_reti (interp=0x6fd080, 
    sub=0x0, sig=0x7f1f0556b42b "IPP") at src/call/ops.c:403
#5  0x00007f1f0541386e in Parrot_quicksort (interp=0x6fd080, data=0x7f6db0, 
    n=2, cmp=0x7f9480) at src/utils.c:941
#6  0x00007f1f054ede2f in Parrot_FixedPMCArray_nci_sort (interp=0x6fd080, 
    pmc=<value optimized out>) at ./src/pmc/fixedpmcarray.pmc:50
#7  0x00007f1f054991a2 in Parrot_NCI_invoke (interp=0x6fd080, pmc=0x0, 
    next=0x85feb0) at ./src/pmc/nci.pmc:329
#8  0x00007f1f0540bafd in runops_slow_core (interp=0x6fd080, pc=0x85fe98)
    at src/runops_cores.c:461
#9  0x00007f1f053c8ef2 in runops_int (interp=0x6fd080, offset=0)
    at src/interpreter.c:982
#10 0x00007f1f053c9a5b in runops (interp=0x6fd080, offs=<value optimized out>)
    at src/call/ops.c:107
#11 0x00007f1f053c9bd0 in runops_args (interp=0x6fd080, sub=0x7f94e0, 
    obj=<value optimized out>, meth_unused=<value optimized out>, 
    sig=0x7f1f0555d9c5 "vP", ap=0x7fff0db07cf0) at src/call/ops.c:255
#12 0x00007f1f053ca5c7 in Parrot_runops_fromc_args (interp=0x6fd080, sub=0x0, 
    sig=0x7f1f0555d9c5 "vP") at src/call/ops.c:324
#13 0x00007f1f0554c7a3 in imcc_run (interp=0x6fd080, 
    sourcefile=0x7fff0db086e1 "subclasstest.pir", argc=1, argv=0x7fff0db07f40)
    at compilers/imcc/main.c:806
#14 0x0000000000400bb2 in main (argc=1, argv=0x7fff0db07f40) at src/main.c:61

[bacek]

Replying to whiteknight:

In r38217 I made a small change to the sort method. Previously, it was using the macro PMC_size() to determine the number of elements in the array, which only worked with FixedPMCArray PMCs, and not subclasses. So I changed that to a call to VTABLE_elements, which solves that one problem.

This patch solves ticket but I'm not sure how "kosher" is it.

diff --git a/src/pmc/fixedpmcarray.pmc b/src/pmc/fixedpmcarray.pmc
index 6eb6e9f..4146ca4 100644
--- a/src/pmc/fixedpmcarray.pmc
+++ b/src/pmc/fixedpmcarray.pmc
@@ -47,7 +47,7 @@ Sort this array, optionally using the provided cmp_func
         const INTVAL n = SELF.elements();
 
         if (n > 1)
-           Parrot_quicksort(interp, (void **)PMC_array(SELF), n, cmp_func);
+           Parrot_quicksort(interp, (void **)SELF.get_pointer(), n, cmp_func);
     }
 
 /*
@@ -144,6 +144,19 @@ fixed sized array).
 
 /*
 
+=item C<void *get_pointer()>
+
+Get pointer to stored data. Used in sort method.
+
+=cut
+
+*/
+
+    VTABLE void * get_pointer() {
+        return PMC_array(SELF);
+    }
+/*
+
 =item C<INTVAL get_integer()>
 
 Returns the number of elements in the array.

[whiteknight]

Good job bacek++! I actually had that same idea as soon as I logged off for the night. I can't believe that I saw the problem with PMC_size, but didn't recognize the same exact problem with PMC_array().

The patch looks perfectly kosher. I'll sanity check it later but we should be able to apply it directly. Thanks!

[bacek]

I actually tested it and applied in r38227 :)

[jonathan]

'fraid the patch applied as r38227 is wrong, so reverted it in r38233 (reason stated in the commit message). Pm is about to commit a test case to Parrot that this patch would have broken (bug discovered because it broke a Rakudo test).

Thanks,

Jonathan

[pmichaud]

Tests added in r38234.

Pm

[whiteknight]

I don't think the fix was "wrong wrong wrong", but it certainly appears now to have been incomplete. The fix that bacek added only added a get_pointer VTABLE to the FixedPMCArray, when it likely should have also added it to the ResizablePMCArray. It really can't be inherited as-is.

If nobody beats me to it, I'll test this idea out later tonight. It might be worth mentioning somewhere that we expect the get_pointer VTABLE of all array-based PMCs to contain the actual C data array, so everybody can find them.

[whiteknight]

Okay, I've looked back into a few of these issues, and I think that the tests added in r38234 are not good tests.

ResizablePMCArray and FixedPMCArray do not supply get_pointer VTABLEs, and fall back to Default.get_pointer(). This, of course, simply returns SELF which is the behavior that these tests are probing for. However, returning a pointer to the PMC itself is far from the most common use of VTABLE_get_pointer. Hash PMCs for instance use get_pointer to return a pointer to the underlying Hash* structure. Sub PMCs use get_pointer to return the opcode_t* address of the PIR subroutine. So, it doesn't make sense to force the *Array PMC get_pointer VTABLEs to return the address of the PMC itself and not a pointer to it's underlying data structure, when other PMCs aren't held to this rule (and since it makes more sense to use get_pointer here in a different way).

Now, that brings me to the issue of the get_addr opcode, which has problems. get_addr is used in two distinct ways from PIR (it is used in a variety of other ways internally):

1) To retrieve opcode_t* addresses in things like Sub and Continuation PMCs 2) To retrieve the actual location in memory of array PMCs (which is how pmichaud's tests use it)

I would suggest that use #2 is not the right use of get_addr, and if people are looking to get the actual memory location of a PMC in a consistent way a new opcode for that purpose should be created.

I suggest the creation of a get_memaddr opcode to return the actual location in memory of the PMC structure, and let get_addr be a wrapper around the get_pointer VTABLE (with the knowledge that it will return inconsistent results based on PMC type). Thoughts?

[whiteknight]

Okay, I talked with pmichaud++ and jonathan++ tonight, and I've come up with a bit of a game plan that we can use to resolve this issue (although it will take at least one deprecation cycle):

1) Change the Sub, Continuation, EventHandler, and other PMCs to not rely on get_addr and set_addr anymore. This will help to remove the very-volatile get_pointer and set_pointer VTABLEs from being exposed to PIR code (which can easily create segfaults if misused with other types of PMCs that don't expect it). 2) Remove the set_addr opcode entirely. 3) Change the get_addr opcode to return the memory address of the PMC passed into it. This is needed by Rakudo at least, and probably by other HLLs as well eventually. 4) Change FixedPMCArray (and maybe other array PMCs) to use get_pointer to return a pointer to the underlying C array. This will improve encapsulation and allow subclassed types to inherit the sort() method properly.

Does this plan sound decent?

[pmichaud]

On Wed, Apr 22, 2009 at 11:07:38PM -0000, Parrot wrote:

2) Remove the set_addr opcode entirely. 3) Change the get_addr opcode to return the memory address of the PMC passed into it. This is needed by Rakudo at least, and probably by other HLLs as well eventually.

Rakudo doesn't specifically have to have the memory address -- it just needs something it can return from .WHICH() to uniquely identify a PMC that is being used as a container. (The memory address is an obvious choice, however.)

4) Change FixedPMCArray (and maybe other array PMCs) to use get_pointer to return a pointer to the underlying C array. This will improve encapsulation and allow subclassed types to inherit the sort() method properly.

This last part sounds wrong to me. Why would a subclassed type need to get to the underlying C array to be able to use sort in the first place? The sort() method in the base class should know to use that (private) array from the base class, regardless of any subclassing taking place.

Pm

[whiteknight]

Basic implementation of new ops that can be used to get/set label values in a PMC that needs it

[whiteknight]

I've added a quick patch here to demonstrate one possible way forward. This creates two new opcodes, get_label and set_label. Uses:

*get_addr: Used to get a unique pointer value of the PMC. Probably something like $1 = PTR2OPCODE_T(SELF) *set_addr: I'm not sure I see a use for this anymore (doesn't make sense to allow a user to arbitrarily set the memory address of a PMC) *get_label: Calls VTABLE_get_pointer() to get the pointer value of a PMC, does some rudimentary error checking to ensure the returned value is, indeed, a valid pointer into the current packfile, and returns that. My implementation doesn't currently do this error checking because I don't know squat about packfiles. *set_label: Calls VTABLE_set_pointer(interp, $1, (CUR_OPCODE) + $2), after doing some rudimentary error checking to ensure the passed pointer value is indeed a valid offset into bytecode. Again, I don't do any such error checking in my patch.

With this change, we can migrate Sub, ExceptionHandler, and Continuation PMCs to use get_label/set_label instead of get_addr/set_addr to set their label values. Then we can change get_addr to return something like $1 = (INTVAL)$2; , but with proper casting. Any objections if I start editing DEPRECATED now to get this plan into motion?

[whiteknight]

I've updated DEPRECATED.pod in r38728. I'm pushing back the milestone to 1.5 since that's the earliest we're going to be able to handle this.

[allison]

We don't really want to provide a way to get at the raw memory address of a PMC from PIR, because we want a copying/compacting garbage collector, which means the memory address can change. PIR code that relies on a consistent memory address for a PMC is broken.

I'm fine with changing the name of the get_addr and set_addr opcodes to get_pointer and set_pointer to match the vtable functions. (Not get_label/set_label, because that's just as bad about imposing some set of semantics on the pointer, which can really be anything.)

The call to Parrot_quicksort should be using a GET_ATTR accessor.

[NotFound]

Replying to allison:

The call to Parrot_quicksort should be using a GET_ATTR accessor.

If you do that what you get is:

Attributes of type 'PMC    **' cannot be subclassed from a high-level PMC.

[NotFound]

The following attachment is a quick hack that works with the examples. Not a general solution, but may be helpful if the problem is blocking someone.

[NotFound]

Quick hack for FixedPMCArray sort method

[coke]

The original sample code now segfaults instead of saying we can't sort.

[NotFound]

Workaround committed in r41183

[coke]

This ticket has a deprecation notice with it that says:

These opcodes are being repurposed. They will always return a unique memory
address of the PMC. Uses of get_addr and set_addr that would set label values
for Sub, Exception, and related PMC types will instead be handled by
get_label and set_label.

But allison states earlier in the thread

We don't really want to provide a way to get at the raw memory address of a PMC from PIR, because we want a copying/compacting garbage collector, which means the memory address can change. PIR code that relies on a consistent memory address for a PMC is broken.

So, it sounds like this is a rejection of the task listed in deprecated.pod. Can we get a ruling on that? (And if yes, open a new ticket and point to that one for the deprecation notice instead. The original problem in this ticket is resolved.)

[whiteknight]

The Rakudo folks had specifically asked for a way to access the raw memory address of an object, which was why I added that part to the plan. If we don't want that in Parrot it would be trivial for the Rakudo guys to add a new dynop for this purpose.

[pmichaud]

Replying to whiteknight:

The Rakudo folks had specifically asked for a way to access the raw memory address of an object, which was why I added that part to the plan.

This is not precisely correct. We really don't care about the "raw memory address" -- we just want a way to uniquely identify a PMC throughout its lifetime. Currently get_addr appears to be the best mechanism we have to do that.

Pm

[whiteknight]

Replying to pmichaud:

This is not precisely correct. We really don't care about the "raw memory address" -- we just want a way to uniquely identify a PMC throughout its lifetime. Currently get_addr appears to be the best mechanism we have to do that.

Sorry for the misrepresentation. If I may ask, what is the use of this identifier? Is it used to lookup a variable later, or is it used to do equality comparisons?

[NotFound]

Replying to pmichaud:

This is not precisely correct. We really don't care about the "raw memory address" -- we just want a way to uniquely identify a PMC throughout its lifetime. Currently get_addr appears to be the best mechanism we have to do that.

Simple solution: name the opcode get_pmc_id or something like that and document it accordingly. This way we can use the address casted to INTVAL right now and later change it if needed. We can just offset it with some arbitrary value to avoid accidental wrong usages.

[NotFound]

The deprecated usages of set_addr are still present. I've create TT #1857 to track this issue. Please check it before doing further actions in this one.