Merging ‘bdw-gc-static-alloc’

Merging ‘bdw-gc-static-alloc’

[Ludovic Courtès]

Hello!

Following the inlining of stringbuf storage
(ba54a2026beaadb4e7566d4b9e2c9e4c7cd793e6), the ‘bdw-gc-static-alloc'
branch introduces a small source-level incompatibility.  Namely, the
following no longer works:

  const char foo[] = "foo";
  SCM_SYMBOL (s_foo, foo);

Because of the macrology that’s used to statically allocate stringbufs [0],
the string has to be a literal:

  SCM_SYMBOL (s_foo, "foo");

The use of non-literal strings occurred only in ‘eval.c’ in Guile proper [1].

The question is: should we merge ‘bdw-gc-static-alloc’ despite this
incompatibility (it can be argued that this won’t hurt many people and
can be easily worked around)?

If the answer is “no”, we still have the option of merging other aspects
of the ‘bdw-gc-static-alloc’ branch, such as static allocation of subrs.

What do you think?

Thanks,
Ludo’.

[0] http://git.sv.gnu.org/gitweb/?p=guile.git;a=blobdiff;f=libguile/snarf.h;h=9eaccf60c14d825e657718e6f75257e293649f84;hp=c3113e1a7a977f95eb922e924855244dfe4d6cc2;hb=5f236208d0d864546e59afa0f5a11c9b3ba14b10;hpb=f0eb5ae6c173aed35965b0561897fda1d8ff0db1
[1] http://git.sv.gnu.org/gitweb/?p=guile.git;a=blobdiff;f=libguile/eval.c;h=e58c05410c2831d4061eb92b4ee2883c9203c919;hp=59db42976242e12fd41995e5e1fa5d169bd3b9bf;hb=5f236208d0d864546e59afa0f5a11c9b3ba14b10;hpb=d7e7a02a6251c8ed4f76933d9d30baeee3f599c0

Re: Merging ‘bdw-gc-static-alloc’

[Andy Wingo]

On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes:

> Because of the macrology that’s used to statically allocate stringbufs [0],
> the string has to be a literal:
>
>   SCM_SYMBOL (s_foo, "foo");
>
> What do you think?

Need more information :) How widespread is the non-literal usage? Could
you grep the source packages of the Guile users that you have identified
for occurrences of this idiom?

Andy
-- 
http://wingolog.org/

Re: Merging ‘bdw-gc-static-alloc’

[Ludovic Courtès]

Hi,

Andy Wingo <wingo@pobox.com> writes:

> On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Because of the macrology that’s used to statically allocate stringbufs [0],
>> the string has to be a literal:
>>
>>   SCM_SYMBOL (s_foo, "foo");
>>
>> What do you think?
>
> Need more information :) How widespread is the non-literal usage? Could
> you grep the source packages of the Guile users that you have identified
> for occurrences of this idiom?

Apparently this is going to be rare.  I’ve found no occurrences here:

  http://www.google.fr/codesearch?q=SCM_%28GLOBAL_%29%3FSYMBOL&hl=fr&btnG=Rechercher+du+code

“grep -r -E \
  'SCM_(GLOBAL_)?SYMBOL[[:blank:]]*\([^,]+,[[:blank:]]*[^"]+\)' \
  gnucash-2.2.9 guile-avahi guile-cairo-1.4.0 \
  guile-gnome-platform-2.16.1 gnutls mailutils-2.1 snd-9.4”
doesn’t show any occurrence either.

So I’d be tempted to merge this in.

What do you think?

Ludo’.

Re: Merging ‘bdw-gc-static-alloc’

[Andy Wingo]

EHLO

On Fri 23 Oct 2009 11:16, ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> Because of the macrology that’s used to statically allocate stringbufs [0],
>>> the string has to be a literal:
>>>
>>>   SCM_SYMBOL (s_foo, "foo");
>>>
>>> What do you think?
>>
>> Need more information :) How widespread is the non-literal usage? Could
>> you grep the source packages of the Guile users that you have identified
>> for occurrences of this idiom?
>
> Apparently this is going to be rare.  I’ve found no occurrences here:
>
> So I’d be tempted to merge this in.
>
> What do you think?

Roll on, merge train, roll on!

I have finished case-lambda, but am rebasing and cleaning up patches I
haven't pushed yet. So it's a race. Gentlemen, start your git rebase &
push!

A
-- 
http://wingolog.org/

Re: Merging ‘bdw-gc-static-alloc’

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

Hello!

I merged that branch in ‘master’.

... and soon after I noticed this glitch with ‘SCM_DEFINE’: when
‘SCM_SUPPORT_STATIC_ALLOCATION’ is defined, it requires a previous C
declaration of the subr being defined.  For instance:

  SCM_DEFINE (foo, "foo", 1, 0, 0, (SCM x), "") { ... }

must now be preceded by:

  extern SCM foo (SCM);

This is not a problem in Guile core where we compile with
‘-Wmissing-prototypes’ and provide public declarations for all
‘SCM_DEFINE’d functions, but it may be a problem in projects that don’t.

This is easily remedied this way:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 624 bytes --]

--- a/libguile/snarf.h
+++ b/libguile/snarf.h
@@ -101,14 +101,15 @@ SCM_SNARF_DOCS(primitive, FNAME, PRIMNAME, ARGLIST, REQ, OPT, VAR, DOCSTRING)
 /* Static subr allocation.  */
 #define SCM_DEFINE(FNAME, PRIMNAME, REQ, OPT, VAR, ARGLIST, DOCSTRING)	\
 SCM_SYMBOL (scm_i_paste (FNAME, __name), PRIMNAME);			\
-SCM_SNARF_HERE(								\
+SCM_SNARF_HERE(							\
   static const char scm_i_paste (s_, FNAME) [] = PRIMNAME;		\
+  extern SCM FNAME ARGLIST;						\
   SCM_IMMUTABLE_SUBR (scm_i_paste (FNAME, __subr),			\
 		      scm_i_paste (FNAME, __name),			\
 		      REQ, OPT, VAR, &FNAME);				\
   SCM FNAME ARGLIST							\

[-- Attachment #3: Type: text/plain, Size: 679 bytes --]


However, there are potentially 2 problems with this:

  1. The declaration could conflict with a previous, slightly different
     one.

     Apparently, a given function declaration and the same one decorated
     with GCC function attributes are considered the same, so this
     should be OK.  However, I’m slightly concerned about MSVC’s
     __declspec: does it work if it sees:

       __declspec(dllexport) extern SCM foo (SCM);
       extern SCM foo (SCM);

     If it works, that probably means this point is moot.

  2. The automatically added ‘extern’ declaration makes
     ‘-Wmissing-prototypes’ useless, which is annoying.

Thoughts?

Thanks,
Ludo’.

(no subject)

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

>      should be OK.  However, I’m slightly concerned about MSVC’s
>      __declspec: does it work if it sees:
>
>        __declspec(dllexport) extern SCM foo (SCM);
>        extern SCM foo (SCM);

I don't know for sure, but I know I've seen "function redeclared with
different linkage" warnings in the past, and this could well be one of
the situations that generates such warnings.

But what about using SCM_API instead of extern?  Wouldn't that always
generate the right code?  (At least for building guile itself.)

>      If it works, that probably means this point is moot.
>
>   2. The automatically added ‘extern’ declaration makes
>      ‘-Wmissing-prototypes’ useless, which is annoying.

I guess it depends if the primitives in question are designed to be used
only from Scheme, or also from other C files.  If they're only for
Scheme, it's a nice feature that SCM_DEFINE does everything needed and
the developer doesn't need a matching prototype.  If they're for other C
files, the missing prototype warning could be helpful.

Then again, the developer will get some kind of warning anyway when
trying to use the function from another file.  So I think the balance is
in favour of SCM_DEFINE including the prototype.

   Neil

Re: Merging ‘bdw-gc-static-alloc’

[Ludovic Courtès]

Hi Neil,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>>      should be OK.  However, I’m slightly concerned about MSVC’s
>>      __declspec: does it work if it sees:
>>
>>        __declspec(dllexport) extern SCM foo (SCM);
>>        extern SCM foo (SCM);
>
> I don't know for sure, but I know I've seen "function redeclared with
> different linkage" warnings in the past, and this could well be one of
> the situations that generates such warnings.

Right.

> But what about using SCM_API instead of extern?  Wouldn't that always
> generate the right code?  (At least for building guile itself.)

You’re right.  It’ll work for building Guile itself, but most probably
fail when building external libraries, because it will have
SCM_API==dllimport whereas the symbols of the lib being built want
dllexport.

I have no idea how to fix this since third party libraries typically
have their own ‘SCM_API’-like macro for that purpose.

Well, we could just disable ‘SCM_SUPPORT_STATIC_ALLOCATION’ altogether
for “Woe32” builds.

What do you think?

In the meantime, I’ve pushed this:
http://git.savannah.gnu.org/cgit/guile.git/commit/?id=f4767979987530bb9d88dde6ebe61cdac6f756b9 .

> I guess it depends if the primitives in question are designed to be used
> only from Scheme, or also from other C files.  If they're only for
> Scheme, it's a nice feature that SCM_DEFINE does everything needed and
> the developer doesn't need a matching prototype.  If they're for other C
> files, the missing prototype warning could be helpful.

Yes, although ideally functions only for Scheme would be either static
or with internal linkage, which calls for an ‘SCM_DEFINE_LOCAL’ macro or
similar.

> Then again, the developer will get some kind of warning anyway when
> trying to use the function from another file.  So I think the balance is
> in favour of SCM_DEFINE including the prototype.

Yes.

Thanks,
Ludo’.

(no subject)

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

> Hi Neil,
>
> Neil Jerram <neil@ossau.uklinux.net> writes:
>
>> But what about using SCM_API instead of extern?  Wouldn't that always
>> generate the right code?  (At least for building guile itself.)
>
> You’re right.  It’ll work for building Guile itself, but most probably
> fail when building external libraries, because it will have
> SCM_API==dllimport whereas the symbols of the lib being built want
> dllexport.
>
> I have no idea how to fix this since third party libraries typically
> have their own ‘SCM_API’-like macro for that purpose.
>
> Well, we could just disable ‘SCM_SUPPORT_STATIC_ALLOCATION’ altogether
> for “Woe32” builds.
>
> What do you think?

We're talking here about macros that are always used in .c files, aren't
we?  (As opposed to headers that an application using the library would
include.)

So on Woe32, we actually always want dllexport.

i.e. if the header has
SCM_API SCM scm_frobate (SCM arg);

and the SCM_DEFINE in .c expands to something that includes
__declspec(dllexport) SCM scm_frobate (SCM arg);

that should be good, because SCM_API will be __declspec(dllexport) when
building the library.

In the Scheme-only case, that does mean exporting something that doesn't
need exporting - but either we can just live with that, or later add
your suggestion of SCM_DEFINE_LOCAL.

Regards,
        Neil