Goops & Valgrind

Goops & Valgrind

[Han-Wen Nienhuys]

Running the test suite through valgrind, I get some fishy errors.

Can someone shed a light on this?  The culprit seems to be 

#define SCM_NUMBER_OF_SLOTS(x) \
 ((SCM_STRUCT_DATA (x)[scm_struct_i_n_words]) - scm_struct_n_extra_words)

where scm_struct_i_n_words is -2


==18569== Invalid read of size 4
==18569==    at 0x4055F93: scm_sys_fast_slot_ref (goops.c:1217)
==18569==    by 0x404073C: ceval (eval.i.c:1337)
==18569==    by 0x40404A6: ceval (eval.i.c:609)
==18569==    by 0x4054EA5: get_slot_value (goops.c:1291)
==18569==    by 0x405684E: scm_sys_initialize_object (goops.c:580)
==18569==    by 0x404073C: ceval (eval.i.c:1337)
==18569==    by 0x40407D5: ceval (eval.i.c:358)
==18569==    by 0x4048C31: scm_m_define (eval.c:1215)
==18569==    by 0x40464CC: scm_apply (eval.i.c:1728)
==18569==    by 0x403F026: ceval (eval.i.c:1037)
==18569==    by 0x404010A: ceval (eval.i.c:329)
==18569==    by 0x40479CA: scm_primitive_eval (eval.c:3992)
==18569==  Address 0x42fda98 is 8 bytes before a block of size 4 alloc'd
==18569==    at 0x4006AEE: malloc (vg_replace_malloc.c:207)
==18569==    by 0x4006C6F: realloc (vg_replace_malloc.c:429)
==18569==    by 0x404F3C7: scm_realloc (gc-malloc.c:109)
==18569==    by 0x404F634: scm_malloc (gc-malloc.c:144)
==18569==    by 0x404F66A: scm_gc_malloc (gc-malloc.c:326)
==18569==    by 0x4054D8E: scm_sys_allocate_instance (goops.c:1541)
==18569==    by 0x404073C: ceval (eval.i.c:1337)
==18569==    by 0x40404A6: ceval (eval.i.c:609)
==18569==    by 0x4048C31: scm_m_define (eval.c:1215)
==18569==    by 0x40464CC: scm_apply (eval.i.c:1728)
==18569==    by 0x403F026: ceval (eval.i.c:1037)
==18569==    by 0x404010A: ceval (eval.i.c:329)



==18569== Invalid read of size 4
==18569==    at 0x4055EF3: scm_sys_fast_slot_set_x (goops.c:1231)
==18569==    by 0x403F673: ceval (eval.i.c:1548)
==18569==    by 0x40407D5: ceval (eval.i.c:358)
==18569==    by 0x4055C24: set_slot_value (goops.c:1336)
==18569==    by 0x405680D: scm_sys_initialize_object (goops.c:584)
==18569==    by 0x404073C: ceval (eval.i.c:1337)
==18569==    by 0x40407D5: ceval (eval.i.c:358)
==18569==    by 0x4048C31: scm_m_define (eval.c:1215)
==18569==    by 0x40464CC: scm_apply (eval.i.c:1728)
==18569==    by 0x403F026: ceval (eval.i.c:1037)
==18569==    by 0x404010A: ceval (eval.i.c:329)
==18569==    by 0x40479CA: scm_primitive_eval (eval.c:3992)
==18569==  Address 0x42fda98 is 8 bytes before a block of size 4 alloc'd
==18569==    at 0x4006AEE: malloc (vg_replace_malloc.c:207)
==18569==    by 0x4006C6F: realloc (vg_replace_malloc.c:429)
==18569==    by 0x404F3C7: scm_realloc (gc-malloc.c:109)
==18569==    by 0x404F634: scm_malloc (gc-malloc.c:144)
==18569==    by 0x404F66A: scm_gc_malloc (gc-malloc.c:326)
==18569==    by 0x4054D8E: scm_sys_allocate_instance (goops.c:1541)
==18569==    by 0x404073C: ceval (eval.i.c:1337)
==18569==    by 0x40404A6: ceval (eval.i.c:609)
==18569==    by 0x4048C31: scm_m_define (eval.c:1215)
==18569==    by 0x40464CC: scm_apply (eval.i.c:1728)
==18569==    by 0x403F026: ceval (eval.i.c:1037)
==18569==    by 0x404010A: ceval (eval.i.c:329)


-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Re: Goops & Valgrind

[Ludovic Courtès]

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:

> Running the test suite through valgrind, I get some fishy errors.

So was that fixed by this commit?

  commit 51ef99f7fa9fb766fbb48619fc5863ab9914591d
  Author: Han-Wen Nienhuys <hanwen@lilypond.org>
  Date:   Sat Aug 16 02:18:51 2008 -0300

      Fix memory corruption issue with hell[] array: realloc/calloc need to
      factor in sizeof(scm_t_bits)

Thanks,
Ludovic.

Re: Goops & Valgrind

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:
> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
> 
>> Running the test suite through valgrind, I get some fishy errors.
> 
> So was that fixed by this commit?
> 
>   commit 51ef99f7fa9fb766fbb48619fc5863ab9914591d
>   Author: Han-Wen Nienhuys <hanwen@lilypond.org>
>   Date:   Sat Aug 16 02:18:51 2008 -0300
> 
>       Fix memory corruption issue with hell[] array: realloc/calloc need to
>       factor in sizeof(scm_t_bits)

No, this error is still there.  I lack knowledge of goops to investigate this.

-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Re: Goops & Valgrind

[Andy Wingo]

Hi Han-Wen,

On Fri 15 Aug 2008 22:15, Han-Wen Nienhuys <hanwen@xs4all.nl> writes:

> Running the test suite through valgrind, I get some fishy errors.
>
> Can someone shed a light on this?  The culprit seems to be 
>
> #define SCM_NUMBER_OF_SLOTS(x) \
>  ((SCM_STRUCT_DATA (x)[scm_struct_i_n_words]) - scm_struct_n_extra_words)
>
> where scm_struct_i_n_words is -2

Classes that are not metaclasses allocate their instances using "light
structs". So the object layout goes like this:

                     the vtable word               the data word
                +-------------------------------+---------------------+
SCM of object = |SCM of class | scm_tc3_struct  | SCM* array of slots |
                +-------------------------------|---------------------+

For classes, the SCM* points to the middle of a SCM array, which has
some number of words before 0; 4 words normally, or 6 if the object is
an "entity", like a generic function. But for objects there are no words
before 0, hence the valid valgrind error.

  i = scm_to_unsigned_integer (index, 0, SCM_NUMBER_OF_SLOTS(obj)-1);

There could be two fixes. One would be to assume that the Scheme code
that calls %fast-slot-ref et al is well-formed, and thus we need no
bounds checking. It's all in goops.scm, so this would be a decent
assumption. The other would be to use a different definition of
SCM_NUMBER_OF SLOTS, which would probably have a different purpose:

#define SCM_NUMBER_OF_FIELDS(x) (SCM_STRUCT_VTABLE (x)[scm_si_nfields])

Cheers,

Andy
-- 
http://wingolog.org/

Re: Goops & Valgrind

[Ludovic Courtès]

Hello,

Andy Wingo <wingo@pobox.com> writes:

> There could be two fixes. One would be to assume that the Scheme code
> that calls %fast-slot-ref et al is well-formed, and thus we need no
> bounds checking. It's all in goops.scm, so this would be a decent
> assumption. The other would be to use a different definition of
> SCM_NUMBER_OF SLOTS, which would probably have a different purpose:
>
> #define SCM_NUMBER_OF_FIELDS(x) (SCM_STRUCT_VTABLE (x)[scm_si_nfields])

I'm not sure I understand all the details right now, but I would welcome
patches.  :-)

That said, AFAIK, primitives always do as much run-time checking as
needed, making no assumption about the correctness of Scheme code.  I
think it would be nice to follow that philosophy, unless it yields a
performance hit.

Thanks,
Ludo'.

Re: Goops & Valgrind

[Neil Jerram]

2008/8/18 Andy Wingo <wingo@pobox.com>:
> Hi Han-Wen,
>
> On Fri 15 Aug 2008 22:15, Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>
>> Running the test suite through valgrind, I get some fishy errors.
>>
>> Can someone shed a light on this?  The culprit seems to be
>>
>> #define SCM_NUMBER_OF_SLOTS(x) \
>>  ((SCM_STRUCT_DATA (x)[scm_struct_i_n_words]) - scm_struct_n_extra_words)
>>
>> where scm_struct_i_n_words is -2

Reading somewhat belatedly through this trail...

Up until your email, I was thinking that the problem here is the "-
scm_struct_n_extra_words" - i.e. Mikael's error #2.  It seems clear
from the code in scm_alloc_struct() that SCM_STRUCT_DATA
(x)[scm_struct_i_n_words] does not include the extra words, and hence
that scm_struct_n_extra_words should not be subtracted.

But then you wrote....

> Classes that are not metaclasses allocate their instances using "light
> structs". So the object layout goes like this:
>
>                     the vtable word               the data word
>                +-------------------------------+---------------------+
> SCM of object = |SCM of class | scm_tc3_struct  | SCM* array of slots |
>                +-------------------------------|---------------------+
>
> For classes, the SCM* points to the middle of a SCM array, which has
> some number of words before 0; 4 words normally, or 6 if the object is
> an "entity", like a generic function. But for objects there are no words
> before 0, hence the valid valgrind error.

Are you sure?  Surely that would require a call somewhere to
scm_alloc_struct() with n_extra = 0, and I can't see any of those.

Also, is Mikael right with his error #1?  I'm thinking not, because I
believe that instances are structs too, so surely it's OK to call
SCM_STRUCT_DATA (x)[...] on them?

Regards,
      Neil

Re: Goops & Valgrind

[Andy Wingo]

On Thu 11 Sep 2008 23:06, "Neil Jerram" <neiljerram@googlemail.com> writes:

>> Classes that are not metaclasses allocate their instances using "light
>> structs". So the object layout goes like this:
>>
>>                     the vtable word               the data word
>>                +-------------------------------+---------------------+
>> SCM of object = |SCM of class | scm_tc3_struct  | SCM* array of slots |
>>                +-------------------------------|---------------------+
>>
>> For classes, the SCM* points to the middle of a SCM array, which has
>> some number of words before 0; 4 words normally, or 6 if the object is
>> an "entity", like a generic function. But for objects there are no words
>> before 0, hence the valid valgrind error.
>
> Are you sure?  Surely that would require a call somewhere to
> scm_alloc_struct() with n_extra = 0, and I can't see any of those.

I'm sure -- goops.c:1541 in master. Doesn't go through scm_alloc_struct
at all.

> Also, is Mikael right with his error #1?  I'm thinking not, because I
> believe that instances are structs too, so surely it's OK to call
> SCM_STRUCT_DATA (x)[...] on them?

I can't recall the mail at the moment. Please reply if you want me to
dig through this -- I'm happy to do so. But instances are structs, yes.
Calling SCM_STRUCT_DATA (x)[] does work. You have to know how many
fields there are, though -- you get that from the vtable.

Andy
-- 
http://wingolog.org/

Re: Goops & Valgrind

[Neil Jerram]

2008/9/12 Andy Wingo <wingo@pobox.com>:
> On Thu 11 Sep 2008 23:06, "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> Are you sure?  Surely that would require a call somewhere to
>> scm_alloc_struct() with n_extra = 0, and I can't see any of those.
>
> I'm sure -- goops.c:1541 in master. Doesn't go through scm_alloc_struct
> at all.

Thanks, I see now.

>> Also, is Mikael right with his error #1?  I'm thinking not, because I
>> believe that instances are structs too, so surely it's OK to call
>> SCM_STRUCT_DATA (x)[...] on them?
>
> I can't recall the mail at the moment. Please reply if you want me to
> dig through this -- I'm happy to do so. But instances are structs, yes.
> Calling SCM_STRUCT_DATA (x)[] does work. You have to know how many
> fields there are, though -- you get that from the vtable.

Agreed.  So I think the right fix here is along the lines of your
second suggestion:

> #define SCM_NUMBER_OF_FIELDS(x) (SCM_STRUCT_VTABLE (x)[scm_si_nfields])

I propose specifically that we:

- remove the SCM_NUMBER_OF_SLOTS macro - because it's never been
right, so there can't be external code relying on it

- change scm_sys_fast_slot_ref and scm_sys_fast_slot_set_x to say

  i = scm_to_unsigned_integer (index, 0, SCM_SLOT (SCM_CLASS_OF (obj),
scm_si_nfields) - 1);

OK?  (There are way too many goops/struct macros already, so let's not
introduce another one!)

One last concern, though: I didn't understand what you meant by "would
probably have a different purpose".  (In:

> assumption. The other would be to use a different definition of
> SCM_NUMBER_OF SLOTS, which would probably have a different purpose:

)

Regards,
     Neil

Re: Goops & Valgrind

[Neil Jerram]

2008/9/14 Neil Jerram <neiljerram@googlemail.com>:

>  i = scm_to_unsigned_integer (index, 0, SCM_SLOT (SCM_CLASS_OF (obj),
> scm_si_nfields) - 1);

There should be a SCM_I_INUM in there too.

    Neil

Re: Goops & Valgrind

[Andy Wingo]

Hi,

Sorry for the spam, but I'm going through some backlog that I didn't
have the resources to deal with. Has this issue been addressed?

Andy

On Sun 14 Sep 2008 14:06, "Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/9/12 Andy Wingo <wingo@pobox.com>:
>> On Thu 11 Sep 2008 23:06, "Neil Jerram" <neiljerram@googlemail.com> writes:
>>
>>> Are you sure?  Surely that would require a call somewhere to
>>> scm_alloc_struct() with n_extra = 0, and I can't see any of those.
>>
>> I'm sure -- goops.c:1541 in master. Doesn't go through scm_alloc_struct
>> at all.
>
> Thanks, I see now.
>
>>> Also, is Mikael right with his error #1?  I'm thinking not, because I
>>> believe that instances are structs too, so surely it's OK to call
>>> SCM_STRUCT_DATA (x)[...] on them?
>>
>> I can't recall the mail at the moment. Please reply if you want me to
>> dig through this -- I'm happy to do so. But instances are structs, yes.
>> Calling SCM_STRUCT_DATA (x)[] does work. You have to know how many
>> fields there are, though -- you get that from the vtable.
>
> Agreed.  So I think the right fix here is along the lines of your
> second suggestion:
>
>> #define SCM_NUMBER_OF_FIELDS(x) (SCM_STRUCT_VTABLE (x)[scm_si_nfields])
>
> I propose specifically that we:
>
> - remove the SCM_NUMBER_OF_SLOTS macro - because it's never been
> right, so there can't be external code relying on it
>
> - change scm_sys_fast_slot_ref and scm_sys_fast_slot_set_x to say
>
>   i = scm_to_unsigned_integer (index, 0, SCM_SLOT (SCM_CLASS_OF (obj),
> scm_si_nfields) - 1);
>
> OK?  (There are way too many goops/struct macros already, so let's not
> introduce another one!)
>
> One last concern, though: I didn't understand what you meant by "would
> probably have a different purpose".  (In:
>
>> assumption. The other would be to use a different definition of
>> SCM_NUMBER_OF SLOTS, which would probably have a different purpose:
>
> )
>
> Regards,
>      Neil

-- 
http://wingolog.org/

Re: Goops & Valgrind

[Neil Jerram]

2009/1/4 Andy Wingo <wingo@pobox.com>:
> Hi,
>
> Sorry for the spam, but I'm going through some backlog that I didn't
> have the resources to deal with. Has this issue been addressed?
>
> Andy
>
> On Sun 14 Sep 2008 14:06, "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> 2008/9/12 Andy Wingo <wingo@pobox.com>:
>>> On Thu 11 Sep 2008 23:06, "Neil Jerram" <neiljerram@googlemail.com> writes:
>>>
>>>> Are you sure?  Surely that would require a call somewhere to
>>>> scm_alloc_struct() with n_extra = 0, and I can't see any of those.
>>>
>>> I'm sure -- goops.c:1541 in master. Doesn't go through scm_alloc_struct
>>> at all.
>>
>> Thanks, I see now.
>>
>>>> Also, is Mikael right with his error #1?  I'm thinking not, because I
>>>> believe that instances are structs too, so surely it's OK to call
>>>> SCM_STRUCT_DATA (x)[...] on them?
>>>
>>> I can't recall the mail at the moment. Please reply if you want me to
>>> dig through this -- I'm happy to do so. But instances are structs, yes.
>>> Calling SCM_STRUCT_DATA (x)[] does work. You have to know how many
>>> fields there are, though -- you get that from the vtable.
>>
>> Agreed.  So I think the right fix here is along the lines of your
>> second suggestion:
>>
>>> #define SCM_NUMBER_OF_FIELDS(x) (SCM_STRUCT_VTABLE (x)[scm_si_nfields])
>>
>> I propose specifically that we:
>>
>> - remove the SCM_NUMBER_OF_SLOTS macro - because it's never been
>> right, so there can't be external code relying on it
>>
>> - change scm_sys_fast_slot_ref and scm_sys_fast_slot_set_x to say
>>
>>   i = scm_to_unsigned_integer (index, 0, SCM_SLOT (SCM_CLASS_OF (obj),
>> scm_si_nfields) - 1);
>>
>> OK?  (There are way too many goops/struct macros already, so let's not
>> introduce another one!)
>>
>> One last concern, though: I didn't understand what you meant by "would
>> probably have a different purpose".  (In:
>>
>>> assumption. The other would be to use a different definition of
>>> SCM_NUMBER_OF SLOTS, which would probably have a different purpose:
>>
>> )
>>
>> Regards,
>>      Neil
>
> --
> http://wingolog.org/
>

Sorry, no.  I have the code change ready to go now; do you by any
chance have a convenient test for this?

       Neil

Re: Goops & Valgrind

[Neil Jerram]

2009/1/4 Neil Jerram <neiljerram@googlemail.com>:
>
> Sorry, no.  I have the code change ready to go now; do you by any
> chance have a convenient test for this?

Well I worked out a test, and have now committed that + the fix to
branch_release-1-8 and master.

    Neil

Re: Goops & Valgrind

[Mikael Djurfeldt]

2008/9/11 Neil Jerram <neiljerram@googlemail.com>:
> Also, is Mikael right with his error #1?  I'm thinking not, because I
> believe that instances are structs too, so surely it's OK to call
> SCM_STRUCT_DATA (x)[...] on them?

It is good that you are sceptical about what I say because it was a
long time ago that I was well oriented in this code.  However, the
problem here is not whether SCM_STRUCT_DATA is operating on the right
type of object.  The real problem is that it is looking after
information which is not stored in an instance.  Furthermore, it is
stored in the class object at a location with a negative offset.
Instances don't have any slots with negative offset.

Re: Goops & Valgrind

[Neil Jerram]

2008/9/16 Mikael Djurfeldt <mikael@djurfeldt.com>:
> 2008/9/11 Neil Jerram <neiljerram@googlemail.com>:
>> Also, is Mikael right with his error #1?  I'm thinking not, because I
>> believe that instances are structs too, so surely it's OK to call
>> SCM_STRUCT_DATA (x)[...] on them?
>
> It is good that you are sceptical about what I say because it was a
> long time ago that I was well oriented in this code.  However, the
> problem here is not whether SCM_STRUCT_DATA is operating on the right
> type of object.  The real problem is that it is looking after
> information which is not stored in an instance.  Furthermore, it is
> stored in the class object at a location with a negative offset.
> Instances don't have any slots with negative offset.

Agreed, I understand that now, and I think my latest code suggestion
already takes it into account.

Regards,
      Neil

Re: Goops & Valgrind

[Mikael Djurfeldt]

2008/8/16 Han-Wen Nienhuys <hanwen@xs4all.nl>:
> Running the test suite through valgrind, I get some fishy errors.
>
> Can someone shed a light on this?  The culprit seems to be
>
> #define SCM_NUMBER_OF_SLOTS(x) \
>  ((SCM_STRUCT_DATA (x)[scm_struct_i_n_words]) - scm_struct_n_extra_words)
>
> where scm_struct_i_n_words is -2

This is severely bitrotted code.  There are at least three errors
associated with this:

1. scm_struct_i_n_words is an offset in a vtable (a GOOPS class is a
vtable), but %fast-slot-ref passes an instance

2. The value at this location apparently now is the number of slots
with nonnegative index (contrary to what is said by some comment in
the code), so scm_struct_n_extra_words should not be subtracted

3. It's not sensible to access slots this way (as in
%fast-slot-ref/set!) since different kinds of slots have different
representations of their data.  However, when working with the MOP (as
active-slot.scm does) some kind of more direct or raw  access to the
slots may be required.

Unfortunately, I don't have time to fix this.  I suggest that some
Guile developer removes %fast-slot-ref/set! and supplies some other
(more clean) way of supporting the code in active-slot.scm.  Also,
make sure to check that these primitives are not used anywhere else.

I apologize if I'm the reason for parts or all of this mess.

Best regards,
Mikael D.

Re: Goops & Valgrind

[Han-Wen Nienhuys]

Mikael Djurfeldt escreveu:

> Unfortunately, I don't have time to fix this.  I suggest that some
> Guile developer removes %fast-slot-ref/set! and supplies some other
> (more clean) way of supporting the code in active-slot.scm.  Also,
> make sure to check that these primitives are not used anywhere else.

Do we have a slow-slot-ref that we could simply substitute for this code? 


-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen