[perl #42407] [PATCH] refactor vtable overriding, delegate.c generation

1 view
Skip to first unread message

Alek Storm

unread,
Apr 9, 2007, 10:35:23 PM4/9/07
to bugs-bi...@rt.perl.org
# New Ticket Created by "Alek Storm"
# Please include the string: [perl #42407]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=42407 >


This patch refactors and optimizes methods involved in vtable
overriding, eliminating several of them. It's now easier to deprecate
and then remove the old double-underscore method of overriding, which
I'll submit in a later patch. Also:
- make generated delegate.c smaller, refactor
- type cleanups
- bugfix - child class' "init" vtable method gets called twice when
both parent and child class override it
- add test for it

Files affected:
src/objects.c
src/pmc/delegate.pmc
src/pmc/parrotobject.pmc
lib/Parrot/Pmc2c/delegate.pm
include/parrot/objects.h
compilers/imcc/pbc.c
t/pmc/object-meths.t

--
Alek Storm

Alek Storm

unread,
Apr 9, 2007, 10:37:57 PM4/9/07
to bugs-bi...@rt.perl.org
# New Ticket Created by "Alek Storm"
# Please include the string: [perl #42408]

# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=42408 >


And... oops. Here's the patch, in all its glory.

--
Alek Storm

vtable_refactor.patch

Alek Storm

unread,
Apr 21, 2007, 9:20:46 PM4/21/07
to perl6-i...@perl.org, bugs-bi...@rt.perl.org
This patch has been sitting for over a week. Is there a problem? I
can't continue work in this area until this is committed.

--
Alek Storm

Chromatic

unread,
Apr 22, 2007, 1:00:15 AM4/22/07
to perl6-i...@perl.org, Alek Storm, bugs-bi...@rt.perl.org
On Saturday 21 April 2007 18:20, Alek Storm wrote:

> This patch has been sitting for over a week. Is there a problem? I
> can't continue work in this area until this is committed.

It came in just before the release and it touched a lot of files, so I
(speaking only for myself) let it sit for a couple of days. Unfortunately,
it also came in after Steve Peters's "No C++ Keywords" patch, so it didn't
apply cleanly.

I massaged it a little bit to get it to apply. Then I cleaned up a few
stylistic nits.

+ const char *meth_c, *name_c = string_to_cstring(interp, name);
+ for (i = 0; (meth_c = Parrot_vtable_slot_names[i]); ++i) {
+ if (!*meth_c)
continue;
/* XXX slot_names still have __ in front */
- if (strcmp(name, meth + 2) == 0)
+ if (strcmp(name_c, meth_c + 2) == 0)
return i;
}
return -1;
}

The char * returned from string_to_cstring() needs explicit
string_cstring_free(). This is annoying, but I'm not sure anyone has a good
fix for it at the moment. I have been performing the free even after
real_exception calls, though I bet we could mark the lowest bit of the
pointer in the string so that real_exception could free the strings directly
and not leak memory.

+ STRING *ns_key = parrot_hash_get_idx(interp, PMC_struct_val(ns),
key);
+ PMC *res = VTABLE_get_pmc_keyed_str(interp, ns, ns_key);

PDD 07 says:

Use vertical alignment for clarity of parallelism.

... so when I make a change to a function, I try to align assignments and
declarations as far as possible. It does improve clarity and scannability.

Here's patch as I have it now. I do notice one failure that I can't explain,
in t/pmc/smop_attribute.t:

1..9
ok 1 - The object isa SMOP_Attribute
ok 2 - test the SMOP_Attribute name method
ok 3 - test the SMOP_Attribute name method
ok 4 - test the SMOP_Attribute name method
Segmentation fault (core dumped)

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1212630848 (LWP 3539)]
0xb7dd5d05 in pobject_lives (interp=0x804e008, obj=0x11) at src/gc/dod.c:141
141 if (PObj_is_live_or_free_TESTALL(obj)) {
(gdb) bt
#0 0xb7dd5d05 in pobject_lives (interp=0x804e008, obj=0x11)
at src/gc/dod.c:141
#1 0xb7dd6293 in Parrot_dod_trace_children (interp=0x804e008,
how_many=4294967010) at src/gc/dod.c:391
#2 0xb7dd607d in trace_active_PMCs (interp=0x804e008, trace_stack=1)
at src/gc/dod.c:320
#3 0xb7dd6c93 in Parrot_dod_ms_run (interp=0x804e008, flags=1)
at src/gc/dod.c:956
#4 0xb7dd6d87 in Parrot_do_dod_run (interp=0x804e008, flags=1)
at src/gc/dod.c:995
#5 0xb7dd5382 in more_traceable_objects (interp=0x804e008, pool=0x806eb00)
at src/gc/smallobject.c:107
#6 0xb7dd5411 in gc_ms_get_free_object (interp=0x804e008, pool=0x806eb00)
at src/gc/smallobject.c:152
#7 0xb7dd3558 in get_free_buffer (interp=0x804e008, pool=0x806eb00)
at src/headers.c:51
#8 0xb7dd3b1a in new_string_header (interp=0x804e008, flags=0)
at src/headers.c:351
#9 0xb7d7afd2 in string_make_direct (interp=0x804e008, buffer=0xb7ee83fb,
len=11, encoding=0x806ec20, charset=0x80711d0, flags=0) at
src/string.c:601
#10 0xb7d7adc5 in string_from_cstring (interp=0x804e008, buffer=0xb7ee83fb,
len=0) at src/string.c:482
#11 0xb7d70f83 in find_vtable_meth_ns (interp=0x804e008, ns=0x820adb4,
vtable_index=139) at src/objects.c:70
#12 0xb7d71122 in Parrot_find_vtable_meth (interp=0x804e008, pmc=0x820a684,
meth=0x806f6f8) at src/objects.c:116
#13 0xb7e6b366 in Parrot_ParrotObject_get_class (interp=0x804e008,
pmc=0x820a684) at src/pmc/parrotobject.pmc:210

That obj=0x11 looks really suspicious to me.

(gdb) frame 12
#12 0xb7d71122 in Parrot_find_vtable_meth (interp=0x804e008, pmc=0x820a684,
meth=0x806f6f8) at src/objects.c:116
116 PMC *res = find_vtable_meth_ns(interp, ns, vtable_index);
(gdb) p meth
$5 = (STRING *) 0x806f6f8
(gdb) p meth->strstart
$6 = 0xb7eea156 "get_class"

That's as far as I've been able to trace however. The tests do pass if I
revert the patch. Any ideas?

-- c

42408_modified.patch

Alek Storm

unread,
Apr 22, 2007, 8:57:33 PM4/22/07
to chromatic, perl6-i...@perl.org, bugs-bi...@rt.perl.org
On 4/22/07, chromatic <chro...@wgz.org> wrote:
> It came in just before the release and it touched a lot of files, so I
> (speaking only for myself) let it sit for a couple of days. Unfortunately,
> it also came in after Steve Peters's "No C++ Keywords" patch, so it didn't
> apply cleanly.

Thanks. I should've checked that.

> That's as far as I've been able to trace however. The tests do pass if I
> revert the patch. Any ideas?

I think the patch exposed either a GC or SMOP bug. Here's the smallest I could get the test case and still have it segfault without gdb's help:

.sub _main :main
load_bytecode 'library/Test/More.pir'

# import test routines
.local pmc exports, curr_namespace, test_namespace
curr_namespace = get_namespace
test_namespace = get_namespace [ "Test::More" ]
exports = split " ", "plan ok is isa_ok"
test_namespace.export_to(curr_namespace, exports)

plan( 9 )

$P0 = new 'SMOP_Attribute'
isa_ok ($P0, 'SMOP_Attribute')

$S1 = $P0.'name'()
is ($S1, 'TestClass1', 'test the SMOP_Attribute name method')

$P0 = new 'SMOP_Attribute'
$S0 = $P0.'type'("TestTypeClass1")
is ($S0, 'TestTypeClass1', 'test the SMOP_Attribute name method')

$S1 = $P0.'type'()
is ($S1, 'TestTypeClass1', 'test the SMOP_Attribute name method')
.end

However, we can make it segfault earlier using gdb, because the problem only shows up when a DOD run is triggered. We can test whether the memory has been corrupted yet from anywhere in Parrot by issuing this:

call (*interp->arena_base->pmc_pool->more_objects)(interp, interp->arena_base->pmc_pool)

If it runs and exits, no problem. If it segfaults, problem. I was able to track the cause down to smop_init() in src/pmc/smop_attribute.pmc. Running the aforementioned command before the call to mem_sys_allocate_zeroed() exits cleanly, but running it afterwards causes a segfault, so mem_sys_allocate_zeroed() (and the calloc() inside it) corrupts something. That's as far as I can get for now - looking at the code immediately preceding the segfault doesn't help any.

The exact same thing happens without the patch, but for some reason the test case above doesn't trigger a DOD run on an unpatched parrot, so it doesn't show up unless you use gdb. At least it's Not My Fault(TM), but this one looks like a doozy to fix. Someone more familiar with the garbage collector than I needs to sort this out. Should we start a new ticket?

--
Alek Storm

Chromatic

unread,
Apr 23, 2007, 2:32:13 AM4/23/07
to Alek Storm, Jonathan Worthington, perl6-i...@perl.org, bugs-bi...@rt.perl.org

Nice analysis. I added the run_gc() method to the interpreter a few weeks ago
to help with this sort of thing. Here's a patch to the tests to demonstrate
the bug and a patch to SMOP_Attribute to fix the thing.

Jonathan, can you help us figure out why deleting these lines out of init()
fixes the problem? Are they vestigial?

/* turn on marking of the class_data array */
PObj_data_is_PMC_array_SET(self);

-- c

smop_attr_fix.patch

Jonathan Worthington

unread,
Apr 23, 2007, 12:07:34 PM4/23/07
to chromatic, Alek Storm, perl6-i...@perl.org, bugs-bi...@rt.perl.org
chromatic wrote:
> Jonathan, can you help us figure out why deleting these lines out of init() fixes the problem? Are they vestigial?
>
> /* turn on marking of the class_data array */
> PObj_data_is_PMC_array_SET(self);
>
I saw those before and thought they were very suspect; I only gave SMOP
a cursory glance for inspiration before digging into PDD15
implementation though. When you set this flag, I believe the GC assumes
the data pointer of the PMC points to a chunk of memory containing an
array of pointers. It then looks at the int in the pmc_ext structure to
say how many pointers there are. That way you don't have to write your
own mark routine for some aggregate types. I may have the specific
details wrong, but it's something like that.

I think in the case of SMOP, the usage of it is bogus/wrong. I don't
remember the int in pmc_ext being set, and assuming that everything in a
struct is and always will be contiguous non-NULL PMCs or STRING pointers
is probably a fast way to segfaults when you change something about the
struct in the future, or if it's uninitialized.

I expect custom mark and destroy are the flags that should be set, and
the mark and destroy vtable methods implemented. As for the more general
future of SMOP, I'm don't know.

Hope this helps,

Jonathan

Chromatic

unread,
Apr 23, 2007, 12:46:59 PM4/23/07
to Jonathan Worthington, Alek Storm, perl6-i...@perl.org, bugs-bi...@rt.perl.org
On Monday 23 April 2007 09:07, Jonathan Worthington wrote:

> chromatic wrote:
> > Jonathan, can you help us figure out why deleting these lines out of
> > init() fixes the problem? Are they vestigial?
> >
> > /* turn on marking of the class_data array */
> > PObj_data_is_PMC_array_SET(self);
>
> I saw those before and thought they were very suspect; I only gave SMOP
> a cursory glance for inspiration before digging into PDD15
> implementation though. When you set this flag, I believe the GC assumes
> the data pointer of the PMC points to a chunk of memory containing an
> array of pointers. It then looks at the int in the pmc_ext structure to
> say how many pointers there are. That way you don't have to write your
> own mark routine for some aggregate types. I may have the specific
> details wrong, but it's something like that.
>
> I think in the case of SMOP, the usage of it is bogus/wrong. I don't
> remember the int in pmc_ext being set, and assuming that everything in a
> struct is and always will be contiguous non-NULL PMCs or STRING pointers
> is probably a fast way to segfaults when you change something about the
> struct in the future, or if it's uninitialized.

Thanks, that matches my reading of the code, too.

I removed this dubious code in r18305 and applied Alek's original patch as
r18306.

Thanks, everyone!

-- c

Leopold Toetsch

unread,
Apr 23, 2007, 3:17:24 PM4/23/07
to perl6-i...@perl.org, Jonathan Worthington, chromatic
Am Montag, 23. April 2007 18:07 schrieb Jonathan Worthington:
> >           /* turn on marking of the class_data array */
> >           PObj_data_is_PMC_array_SET(self);
> >  
>
> I saw those before and thought they were very suspect;

It's not per se suspect. The (data*) points to an array of objects contained
in 'SMOP_Attribute' that needs be delt with by the GC (I presume).

So the flag above is ok. But this doesn't work at it's own, it addtionally
needs the size of the array being present in PMC_int_val(self).

See also: src/gc/dod.c:384 ff

leo

Alek Storm

unread,
Apr 23, 2007, 4:39:08 PM4/23/07
to chromatic, Jonathan Worthington, perl6-i...@perl.org, bugs-bi...@rt.perl.org
On 4/23/07, chromatic <chro...@wgz.org> wrote:
> On Monday 23 April 2007 09:07, Jonathan Worthington wrote:
>
> > chromatic wrote:
> > > Jonathan, can you help us figure out why deleting these lines out of
> > > init() fixes the problem? Are they vestigial?
> > >
> > > /* turn on marking of the class_data array */
> > > PObj_data_is_PMC_array_SET(self);
> >
> > I saw those before and thought they were very suspect; I only gave SMOP
> > a cursory glance for inspiration before digging into PDD15
> > implementation though. When you set this flag, I believe the GC assumes
> > the data pointer of the PMC points to a chunk of memory containing an
> > array of pointers. It then looks at the int in the pmc_ext structure to
> > say how many pointers there are. That way you don't have to write your
> > own mark routine for some aggregate types. I may have the specific
> > details wrong, but it's something like that.

Wow, that's actually how I guessed it would work - didn't want to
touch it without the okay from someone who knows GC, though. Plus, it
was really late it my timezone :)

> I removed this dubious code in r18305 and applied Alek's original patch as
> r18306.
>
> Thanks, everyone!

Gracias. I attached one more small patch that gets rid of two
seemingly unnecessary lines in smop_init() - they're easy to miss when
one's looking at the big picture.

--
Alek Storm

smop.patch

Matt Diephouse via RT

unread,
Apr 24, 2007, 12:50:53 PM4/24/07
to perl6-i...@perl.org
On Mon Apr 23 13:39:40 2007, alek....@gmail.com wrote:
> Gracias. I attached one more small patch that gets rid of two
> seemingly unnecessary lines in smop_init() - they're easy to miss when
> one's looking at the big picture.

Applied in r18321. Thanks!

--
Matt Diephouse


Reply all
Reply to author
Forward
0 new messages