Ticket #218 (reopened deprecation)

Opened 6 years ago

Last modified 4 years ago

change function of get_addr and set_addr

Reported by: coke Owned by: whiteknight
Priority: normal Milestone:
Component: core Version:
Severity: fatal Keywords:
Cc: Language:
Patch status: Platform: all

Description (last modified by bacek) (diff)

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.

Attachments

new_label_ops.patch Download (0.9 KB) - added by whiteknight 5 years ago.
Basic implementation of new ops that can be used to get/set label values in a PMC that needs it
TT_218.patch Download (0.8 KB) - added by NotFound 5 years ago.
Quick hack for FixedPMCArray sort method

Change History

  Changed 6 years ago by pmichaud

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

Pm

  Changed 6 years ago by whiteknight

  • status changed from new to assigned
  • owner set to whiteknight
  • component changed from none to core
  • platform set to all
  • milestone set to 1.0

  Changed 6 years ago by 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.

  Changed 6 years ago by 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.

  Changed 5 years ago by 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.

follow-up: ↓ 9   Changed 5 years ago by 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

  Changed 5 years ago by bacek

  • description modified (diff)

  Changed 5 years ago by bacek

  • description modified (diff)

in reply to: ↑ 6   Changed 5 years ago by 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.

  Changed 5 years ago by 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!

  Changed 5 years ago by bacek

  • status changed from assigned to closed
  • resolution set to fixed

I actually tested it and applied in r38227 :)

  Changed 5 years ago by jonathan

  • status changed from closed to reopened
  • resolution fixed deleted

'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

  Changed 5 years ago by pmichaud

Tests added in r38234.

Pm

  Changed 5 years ago by 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.

  Changed 5 years ago by 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?

  Changed 5 years ago by 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?

  Changed 5 years ago by 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

Changed 5 years ago by whiteknight

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

  Changed 5 years ago by 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?

  Changed 5 years ago by whiteknight

  • milestone changed from 1.2 to 1.5

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.

  Changed 5 years ago by coke

  • lang set to tcl

follow-up: ↓ 22   Changed 5 years ago by 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.

in reply to: ↑ 21   Changed 5 years ago by 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.

  Changed 5 years ago by 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.

Changed 5 years ago by NotFound

Quick hack for FixedPMCArray sort method

  Changed 5 years ago by whiteknight

  • milestone 1.5 deleted

  Changed 5 years ago by coke

  • severity changed from high to fatal

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

  Changed 5 years ago by coke

  • keywords tcl blocker removed
  • lang tcl deleted

  Changed 5 years ago by NotFound

Workaround committed in r41183

  Changed 5 years ago by 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.)

follow-up: ↓ 31   Changed 5 years ago by 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.

  Changed 5 years ago by coke

  • type changed from bug to deprecated
  • summary changed from can't sort a PIR subclass of an RPA. to change function of get_addr and set_addr

in reply to: ↑ 29 ; follow-ups: ↓ 32 ↓ 33   Changed 5 years ago by 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

in reply to: ↑ 31   Changed 5 years ago by 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?

in reply to: ↑ 31   Changed 5 years ago by 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.

  Changed 4 years ago by 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.

Note: See TracTickets for help on using tickets.