bdw-gc includes in libguile.h

bdw-gc includes in libguile.h

[Andy Wingo]

Hello,

I think we made a mistake in exposing bdw-gc.h to libguile.h users.
gc.h is quite scrupulous to not include it, but smob.h, inline.h
(sometimes), and pthread-threads.h pull it in.

Besides the modularity concerns that lead us to need to add bdw-gc libs
and cflags to Guile's libs and cflags, there is an acute problem, and
that is that we enable pthread redirects -- so users of libguile get
pthread_create et al re-#defined.

I think that in 2.2 we should not expose libgc interfaces in libguile,
and that in 2.0 we should disable pthread redirects.

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Andy Wingo]

Hi,

To follow up on this,

On Fri 25 Mar 2011 10:38, Andy Wingo <wingo@pobox.com> writes:

> that is that we enable pthread redirects -- so users of libguile get
> pthread_create et al re-#defined.

This was causing http://thread.gmane.org/gmane.lisp.guile.bugs/5340. 

> I think that in 2.2 we should not expose libgc interfaces in libguile,
> and that in 2.0 we should disable pthread redirects.

I have disabled pthread redirects in stable-2.0.

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Ludovic Courtès]

Hello,

Andy Wingo <wingo@pobox.com> writes:

> I think we made a mistake in exposing bdw-gc.h to libguile.h users.
> gc.h is quite scrupulous to not include it, but smob.h, inline.h
> (sometimes), and pthread-threads.h pull it in.

<libguile/bdw-gc.h> is intentionally pulled because our public headers
use macros and inlines from <gc/gc.h>.

> Besides the modularity concerns that lead us to need to add bdw-gc libs
> and cflags to Guile's libs and cflags, there is an acute problem, and
> that is that we enable pthread redirects -- so users of libguile get
> pthread_create et al re-#defined.

Yes, that’s intended.

> I think that in 2.2 we should not expose libgc interfaces in libguile,

That would be great, but then ‘scm_cell’, ‘SCM_NEWSMOB’, etc. would
need to do a function call, which we don’t want.  Even if we did want
it, the change would break the ABI.

> and that in 2.0 we should disable pthread redirects.

Why?

We rely on pthread redirects internally and if users use pthread, then
they need it too, I suppose.

A meta-comment: can we agree to take more time to discuss this sort of
things?  I’ll try to be responsive, and the earth won’t stop spinning if
the fix waits a couple of days.  ;-)

Thanks,
Ludo’.

Re: bdw-gc includes in libguile.h

[Andy Wingo]

Hi,

On Fri 25 Mar 2011 19:06, ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> I think we made a mistake in exposing bdw-gc.h to libguile.h users.
>> gc.h is quite scrupulous to not include it, but smob.h, inline.h
>> (sometimes), and pthread-threads.h pull it in.
>
> <libguile/bdw-gc.h> is intentionally pulled because our public headers
> use macros and inlines from <gc/gc.h>.

Right; I think this is a mistake, because it couples our external
interface too tightly to libgc's interface.

>> I think that in 2.2 we should not expose libgc interfaces in libguile,
>
> That would be great, but then ‘scm_cell’, ‘SCM_NEWSMOB’, etc. would
> need to do a function call, which we don’t want.  Even if we did want
> it, the change would break the ABI.

I realize this :)  That's why I am proposing it for 2.2, which will
(presumably) be ABI-incompatible.  I don't think inlining NEWSMOB et al
actually buys us anything worth buying, so to speak.

>> and that in 2.0 we should disable pthread redirects.
>
> Why?

I think I addressed this in my other mail; perhaps we can continue the
discussion there.

> A meta-comment: can we agree to take more time to discuss this sort of
> things?  I’ll try to be responsive, and the earth won’t stop spinning if
> the fix waits a couple of days.  ;-)

Sure.  Sorry for the precipitous action.  That said, this bug has been
open since September:  https://savannah.gnu.org/bugs/?32436

Regards,

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Ludovic Courtès]

Hello!

Andy Wingo <wingo@pobox.com> writes:

> On Fri 25 Mar 2011 19:06, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Andy Wingo <wingo@pobox.com> writes:

[...]

>>> I think that in 2.2 we should not expose libgc interfaces in libguile,
>>
>> That would be great, but then ‘scm_cell’, ‘SCM_NEWSMOB’, etc. would
>> need to do a function call, which we don’t want.  Even if we did want
>> it, the change would break the ABI.
>
> I realize this :)  That's why I am proposing it for 2.2, which will
> (presumably) be ABI-incompatible.

OK.

> I don't think inlining NEWSMOB et al actually buys us anything worth
> buying, so to speak.

Yes, I agree.

Internally we should still inline scm_cell & co., though; that can be
done without exposing <gc/gc.h> in public headers I guess.  (Actually,
we should inline scm_cons, too, internally.)

>> A meta-comment: can we agree to take more time to discuss this sort of
>> things?  I’ll try to be responsive, and the earth won’t stop spinning if
>> the fix waits a couple of days.  ;-)
>
> Sure.  Sorry for the precipitous action.  That said, this bug has been
> open since September:  https://savannah.gnu.org/bugs/?32436

Oh indeed, I hadn’t realized there’s a connection; still...

Thanks,
Ludo’.

Re: bdw-gc includes in libguile.h

[Andy Wingo]

Greets :)

On Sun 27 Mar 2011 17:11, ludo@gnu.org (Ludovic Courtès) writes:

> Internally we should still inline scm_cell & co., though; that can be
> done without exposing <gc/gc.h> in public headers I guess.  (Actually,
> we should inline scm_cons, too, internally.)

Agreed.

>>> A meta-comment: can we agree to take more time to discuss this sort of
>>> things?  I’ll try to be responsive, and the earth won’t stop spinning if
>>> the fix waits a couple of days.  ;-)
>>
>> Sure.  Sorry for the precipitous action.  That said, this bug has been
>> open since September:  https://savannah.gnu.org/bugs/?32436
>
> Oh indeed, I hadn’t realized there’s a connection; still...

Do you have any thoughts on that bug, or the recent threads.c patches?

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Ludovic Courtès]

Hello!

Andy Wingo <wingo@pobox.com> writes:

>>> Sure.  Sorry for the precipitous action.  That said, this bug has been
>>> open since September:  https://savannah.gnu.org/bugs/?32436
>>
>> Oh indeed, I hadn’t realized there’s a connection; still...
>
> Do you have any thoughts on that bug,

The problem is that libgc ends up being initialized behind our back upon
the first libgc-redirected ‘pthread_create’ call.

Hans Boehm suggested [0] two solutions:

  1. Disable pthread redirects and instead register threads explicitly
     (in ‘scm_with_guile’).

  2. Initialize libgc in a constructor.

I was leaning towards (2), because this way we’d be in control, and in
particular we’d have GC_all_interior_pointers = 0.  It would only work
on GCC/ELF platforms, and only if libgc wasn’t already initialized (for
instance if Guile is used in an application that already uses libgc on
its own)—but that really covers 90% of our use cases.

I understand you’re in favor of (1).  This would give the same behavior
as in 1.8[*] while being less hackish than (2).  However, it’s only
applicable to 2.1.

Thus, I think we could go with (2) in 2.0 (on platforms where
constructors are available), and with (1) in 2.2.

[0] http://thread.gmane.org/gmane.lisp.guile.bugs/5340

[*] The behavior in 1.8 is that the non-guile-mode stack isn’t scanned.
    I would find it less error-prone if all the stacks were scanned by
    default, but it’s not that important either.

> or the recent threads.c patches?

Yes, see <87vczdqtdx.fsf@gnu.org>.  :-)

(There are a couple of build failures on Hydra since these patches, but
we’ll see that after.)

Thanks!

Ludo’.

Re: bdw-gc includes in libguile.h

[Andy Wingo]

Greets :)

On Mon 28 Mar 2011 21:22, ludo@gnu.org (Ludovic Courtès) writes:

> The problem is that libgc ends up being initialized behind our back upon
> the first libgc-redirected ‘pthread_create’ call.

Indeed.

> Hans Boehm suggested [0] two solutions:
>
>   1. Disable pthread redirects and instead register threads explicitly
>      (in ‘scm_with_guile’).
>
>   2. Initialize libgc in a constructor.
>
> I was leaning towards (2), because this way we’d be in control, and in
> particular we’d have GC_all_interior_pointers = 0.  It would only work
> on GCC/ELF platforms, and only if libgc wasn’t already initialized (for
> instance if Guile is used in an application that already uses libgc on
> its own)—but that really covers 90% of our use cases.
>
> I understand you’re in favor of (1).  This would give the same behavior
> as in 1.8[*] while being less hackish than (2).  However, it’s only
> applicable to 2.1.

Why is this only applicable to 2.1 ?

Regards,

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Ludovic Courtès]

Hello,

Andy Wingo <wingo@pobox.com> writes:

> On Mon 28 Mar 2011 21:22, ludo@gnu.org (Ludovic Courtès) writes:
>
>> The problem is that libgc ends up being initialized behind our back upon
>> the first libgc-redirected ‘pthread_create’ call.
>
> Indeed.
>
>> Hans Boehm suggested [0] two solutions:
>>
>>   1. Disable pthread redirects and instead register threads explicitly
>>      (in ‘scm_with_guile’).
>>
>>   2. Initialize libgc in a constructor.
>>
>> I was leaning towards (2), because this way we’d be in control, and in
>> particular we’d have GC_all_interior_pointers = 0.  It would only work
>> on GCC/ELF platforms, and only if libgc wasn’t already initialized (for
>> instance if Guile is used in an application that already uses libgc on
>> its own)—but that really covers 90% of our use cases.
>>
>> I understand you’re in favor of (1).  This would give the same behavior
>> as in 1.8[*] while being less hackish than (2).  However, it’s only
>> applicable to 2.1.
>
> Why is this only applicable to 2.1 ?

I was thinking it’d break the ABI, but maybe not?

Ludo’.

Re: bdw-gc includes in libguile.h

[Andy Wingo]

On Mon 28 Mar 2011 22:40, ludo@gnu.org (Ludovic Courtès) writes:

>>>   1. Disable pthread redirects and instead register threads explicitly
>>>      (in ‘scm_with_guile’).
>>
>> Why is this only applicable to 2.1 ?
>
> I was thinking it’d break the ABI, but maybe not?

I don't think it does break ABI.  We are always free to redefine C
macros, provided that binaries compiled against the old macros still
work as well as they did.  Given that we require threads to enter Guile
with a scm_with_guile, there is no problem here, I don't think.

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Ludovic Courtès]

Hello!

Andy Wingo <wingo@pobox.com> writes:

> On Mon 28 Mar 2011 22:40, ludo@gnu.org (Ludovic Courtès) writes:
>
>>>>   1. Disable pthread redirects and instead register threads explicitly
>>>>      (in ‘scm_with_guile’).
>>>
>>> Why is this only applicable to 2.1 ?
>>
>> I was thinking it’d break the ABI, but maybe not?
>
> I don't think it does break ABI.  We are always free to redefine C
> macros, provided that binaries compiled against the old macros still
> work as well as they did.  Given that we require threads to enter Guile
> with a scm_with_guile, there is no problem here, I don't think.

OK, then fine with me.

Thanks for persevering!  ;-)

Ludo’.

Re: bdw-gc includes in libguile.h

[Andy Wingo]

Hi!

On Wed 30 Mar 2011 18:15, ludo@gnu.org (Ludovic Courtès) writes:

> Hello!
>
> Andy Wingo <wingo@pobox.com> writes:
>
>> On Mon 28 Mar 2011 22:40, ludo@gnu.org (Ludovic Courtès) writes:
>>
>>>>>   1. Disable pthread redirects and instead register threads explicitly
>>>>>      (in ‘scm_with_guile’).
>>>>
> OK, then fine with me.
>
> Thanks for persevering!  ;-)

Cool.  Thanks for the review :)

Andy
-- 
http://wingolog.org/

Re: bdw-gc includes in libguile.h

[Andy Wingo]

On Fri 25 Mar 2011 19:44, Andy Wingo <wingo@pobox.com> writes:

>>> I think that in 2.2 we should not expose libgc interfaces in libguile,
>>
>> That would be great, but then ‘scm_cell’, ‘SCM_NEWSMOB’, etc. would
>> need to do a function call, which we don’t want.  Even if we did want
>> it, the change would break the ABI.
>
> I realize this :)  That's why I am proposing it for 2.2, which will
> (presumably) be ABI-incompatible.  I don't think inlining NEWSMOB et al
> actually buys us anything worth buying, so to speak.

I pushed some fixes for this to stable-2.0 and to master.  (The more
invasive ones are in master).

Please comment if the mood strikes you; we can revert things if they
turn out to be wrong.

Cheers,

Andy
-- 
http://wingolog.org/