a few proposed patches

a few proposed patches

[Ken Raeburn]

After reading the "dynamic ffi and C struct" thread this weekend, I started thinking, "I wonder if that's really done so as to handle X and Y and Z, and if we're actually testing it well enough", and got the urge to do another Mac build, which I hadn't done in a while.  After installing libgc 7.2 to get around flaky failures, and fighting with the build system a bit (I have gmp installed via Macports, and that tree also has libgc 7.1…), and waiting hours for builds to finish and looking into why, I have a few patches to propose.  I've uploaded them to the branch wip-raeburn-misc for review, since I'm not sure you'll want to address the issues the same way.

* Eliminate use of GC_PTR.  Looks like it's a holdover from earlier versions of libgc.  Some versions don't define it, so we do.  Apparently the 7.2 release defines it, too, which resulted in bug #11500.  It turns out, too, that some of the casts weren't quite right (casting to GC_PTR when GC_PTR* was needed), and only worked because GC_PTR is void* and thus can be converted to the correct type; I've tried to fix up those cases.  The change discards some minor abstraction of the pointer interface to libgc, but I don't think we really need an abstract name for void* anyways.

* Don't use addresses of code labels with LLVM, even if the compiler supports them.  At least with the version of LLVM GCC on my Mac ("gcc version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)"), the performance seems to be quite poor; "guild compile" was showing about a 4x penalty in CPU time.  (For psyntax-pp.go, it was 10 minutes of CPU time vs 46 minutes.)  Later/future versions may do better, so we can update it with version-number checks, unless we want to build performance tests into the configure script, which doesn't sound like a great idea to me.  Related to this, I made two more changes: Always define statement labels in the VM code anyway, because vm-i-scheme.c uses some of them unconditionally (which makes me wonder if any non-GCC configs are getting tested); and report the time taken for each "guild compile" command during the build.

* Require libgc 7.2 or better.  Too often the fix to flaky problems seems to be "try updating to the latest libgc and see if that fixes it", so let's just require it.  Or is 7.1 really *that* consistently reliable for our use cases on some platforms?  I decided to go with a test in the C code because I was having problems with include directory ordering for a while on my system, with both 7.1 and 7.2 installed.  A configure-time check would work fine in addition, but the C check takes effect after the various include directories for gmp, ffi, etc., are added, and using the order as actually determined in the makefile for compilation, which the configure script may or may not be consistent with.  It would be nice to catch a version error sooner, though.

* Test FFI function calls with signed narrow arguments better -- both positive and negative values.  Currently the Mac port (with libffi 3.0.10) fails these tests, and I'm not sure where the bug lies.  This just adds more, related failures to the ones we've already got.

Let me know if (some of?) these look good and I'll merge them to master, unless someone else wants to cherry-pick some of the changes first.

I've also checked in on master a couple pretty straightforward-looking fixes.  I don't know if either would be applicable to the current release.

One thing still concerns me about the FFI struct size/alignment handling code.  It's written based on some assumptions from the SYSV ABI, and thus may or may not be correct for other systems not based on that ABI.  But the tests are written in Scheme using the same assumptions; they should be written to test against the actual C compiler.  Otherwise we may wind up with FFI tests passing but FFI code not actually working.  After the other stuff I've described above, I haven't had time to tackle this yet….

Ken

Re: a few proposed patches

[Neil Jerram]

Ken Raeburn <raeburn@raeburn.org> writes:

> * Require libgc 7.2 or better.  Too often the fix to flaky problems
> seems to be "try updating to the latest libgc and see if that fixes
> it", so let's just require it.  Or is 7.1 really *that* consistently
> reliable for our use cases on some platforms?

7.2 is required for ARM.  (At least, there are fixes between 7.1 and 7.2
that are definitely required for ARM.  I haven't yet tried the 7.2
release exactly.)

Regards,
        Neil

Re: a few proposed patches

[Andy Wingo]

On Mon 21 May 2012 07:45, Ken Raeburn <raeburn@raeburn.org> writes:

> I've also checked in on master a couple pretty straightforward-looking
> fixes.  I don't know if either would be applicable to the current
> release.

Thank you very much for these, especially the locking one!  How did you
catch the locking bug, just by code inspection?  It could be at the root
of a tricky bug that has been following me around since that code was
introduced...

Thankfully those bugs do not apply to the stable branch.

Regards,

Andy
-- 
http://wingolog.org/

Re: a few proposed patches

[Andy Wingo]

Hi Ken,

On Mon 21 May 2012 07:45, Ken Raeburn <raeburn@raeburn.org> writes:

> * Eliminate use of GC_PTR.  Looks like it's a holdover from earlier
> versions of libgc.  Some versions don't define it, so we do.  Apparently
> the 7.2 release defines it, too, which resulted in bug #11500.  It turns
> out, too, that some of the casts weren't quite right (casting to GC_PTR
> when GC_PTR* was needed), and only worked because GC_PTR is void* and
> thus can be converted to the correct type; I've tried to fix up those
> cases.  The change discards some minor abstraction of the pointer
> interface to libgc, but I don't think we really need an abstract name
> for void* anyways.
>
> * Require libgc 7.2 or better.  Too often the fix to flaky problems
> seems to be "try updating to the latest libgc and see if that fixes it",
> so let's just require it.  Or is 7.1 really *that* consistently reliable
> for our use cases on some platforms?  I decided to go with a test in the
> C code because I was having problems with include directory ordering for
> a while on my system, with both 7.1 and 7.2 installed.  A configure-time
> check would work fine in addition, but the C check takes effect after
> the various include directories for gmp, ffi, etc., are added, and using
> the order as actually determined in the makefile for compilation, which
> the configure script may or may not be consistent with.  It would be
> nice to catch a version error sooner, though.

These are related.  Until recently, the intention was that 7.1 was the
minimum version, though we supported compilation against 6.8, which is
the version in Debian stable.  As it is, the final 7.2 release was only
made a couple weeks ago, which is too new, at least for stable-2.0.

On the other hand, requiring 7.2 in master would probably be
acceptable.  Input from others is appreciated.

I think at least for stable-2.0, some more targeted fix can be
appropriate.

> * Don't use addresses of code labels with LLVM, even if the compiler
> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)"),

This is a very old and buggy compiler, AFAIK.  Your system might also
contain clang, which is probably better, if it works.

> the performance seems to be quite poor; "guild compile" was showing
> about a 4x penalty in CPU time.

Well, in this case it is worth making a change.  But can you try with
newer clang to see what it does?  I'd hate to turn it off for new
compilers as well.

> * Test FFI function calls with signed narrow arguments better -- both
> positive and negative values.  Currently the Mac port (with libffi
> 3.0.10) fails these tests, and I'm not sure where the bug lies.  This
> just adds more, related failures to the ones we've already got.

I don't recall what the end result was, but it could have been a bug in
a libffi compiled by that libgc.

Related:

  http://news.gmane.org/find-root.php?group=gmane.lisp.guile.bugs&article=5908
  http://news.gmane.org/find-root.php?group=gmane.lisp.guile.bugs&article=6142

> One thing still concerns me about the FFI struct size/alignment handling
> code.  It's written based on some assumptions from the SYSV ABI, and
> thus may or may not be correct for other systems not based on that ABI.
> But the tests are written in Scheme using the same assumptions; they
> should be written to test against the actual C compiler.  Otherwise we
> may wind up with FFI tests passing but FFI code not actually working.
> After the other stuff I've described above, I haven't had time to tackle
> this yet….

Good questions, these...

Andy
-- 
http://wingolog.org/

Re: a few proposed patches

[Andy Wingo]

On Tue 22 May 2012 10:17, Andy Wingo <wingo@pobox.com> writes:

> I don't recall what the end result was, but it could have been a bug in
> a libffi compiled by that libgc.

By that GCC, I meant; which is really an old fork Apple made of GNU GCC.

Andy
-- 
http://wingolog.org/

Re: a few proposed patches

[Ken Raeburn]

On May 22, 2012, at 03:54, Andy Wingo wrote:
> On Mon 21 May 2012 07:45, Ken Raeburn <raeburn@raeburn.org> writes:
> 
>> I've also checked in on master a couple pretty straightforward-looking
>> fixes.  I don't know if either would be applicable to the current
>> release.
> 
> Thank you very much for these, especially the locking one!  How did you
> catch the locking bug, just by code inspection?  It could be at the root
> of a tricky bug that has been following me around since that code was
> introduced…

No, I found the struct-size one by inspection, and ran into the locking one trying to test it. :-)

The build hung with consistent stack traces while running Guile, always trying to lock that mutex.  I suspected a double-lock attempt or a missing unlock, so I instrumented the lock functions to report what they were doing, with line numbers, and noticed the excess unlocks.

> Thankfully those bugs do not apply to the stable branch.

That's good news.

Ken

Re: a few proposed patches

[Ken Raeburn]

On May 22, 2012, at 04:17, Andy Wingo wrote:
> These are related.  Until recently, the intention was that 7.1 was the
> minimum version, though we supported compilation against 6.8, which is
> the version in Debian stable.  As it is, the final 7.2 release was only
> made a couple weeks ago, which is too new, at least for stable-2.0.

Hm.  Okay.  Most of the advice I remembered seeing was to get the latest alpha of 7.something (didn't remember if that was 7.1 or 7.2).  But I thought I also ran across bug fixes mentioned as having been added since the alphas, that fixed bugs found running Guile.  Maybe it was since 7.1 alphas, I don't recall how old the report was, nor whether it was about stable-2.0 or master.  But supporting 6.8 seems kind of pointless, if we need 7.1 to function properly.  And it looks to me like 7.2alpha2, at least, should pass the version test I used, and its timestamp is back in 2009.

> On the other hand, requiring 7.2 in master would probably be
> acceptable.  Input from others is appreciated.

It is on master where I was seeing sporadic failures during the build while using 7.1, which disappeared around the time I switched to 7.2.

> I think at least for stable-2.0, some more targeted fix can be
> appropriate.

I thought about looking to see if there was some additional version identifier (GC_ALPHA_VERSION?) that would distinguish versions that did define GC_PTR from those that don't, but since GC_PTR just looked like a holdover in need of deletion, it didn't seem worth it.

Is GC_PTR defined as void* in 6.8?  If so, the patch to remove GC_PTR would still work.  Though a configure test could probably be written to test whether the libgc header defines GC_PTR.

> 
>> * Don't use addresses of code labels with LLVM, even if the compiler
>> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
>> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)"),
> 
> This is a very old and buggy compiler, AFAIK.  Your system might also
> contain clang, which is probably better, if it works.
> 
>> the performance seems to be quite poor; "guild compile" was showing
>> about a 4x penalty in CPU time.
> 
> Well, in this case it is worth making a change.  But can you try with
> newer clang to see what it does?  I'd hate to turn it off for new
> compilers as well.

Hm, it's not what configure picks up by default, but yes, it's there.  I'll give it a try when I get a chance.

Ken

Removing ‘GC_PTR’

[Ludovic Courtès]

Hi,

Ken Raeburn <raeburn@raeburn.org> skribis:

> Is GC_PTR defined as void* in 6.8?  If so, the patch to remove GC_PTR
> would still work.  Though a configure test could probably be written
> to test whether the libgc header defines GC_PTR.

I agree we can remove it: in stable-2.0 it’s no longer used since commit
0f6dd25023da59bcfefb080c66a2d2650d955ffa.

Thanks,
Ludo’.

Using labels-as-values on MacOS X

[Ludovic Courtès]

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

Hi,

Ken Raeburn <raeburn@raeburn.org> skribis:

> * Don't use addresses of code labels with LLVM, even if the compiler
> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build
> 2336.1.00)"),

Damn, what compiler is this?  It’s not the old GCC 4.2 fork?  Is it
Clang?  GCC with DragonEgg?

> the performance seems to be quite poor; "guild compile" was showing
> about a 4x penalty in CPU time.  (For psyntax-pp.go, it was 10 minutes
> of CPU time vs 46 minutes.)

Weird, would be good to see what happens.

The two options for VM dispatching are:


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

#include <stdio.h>
int
vm_jt (const unsigned int *code)
{
  static const void *jump_table[] = { &&op0, &&op1, &&op2 };
  register const unsigned int *ip = code;

#define NEXT goto *jump_table[*ip++]

  NEXT;

 op0:
  puts ("op0");
  return 0;
 op1:
  puts ("op1");
  NEXT;
 op2:
  puts ("op2");
  NEXT;
}

int
vm_switch (const unsigned int *code)
{
  register const unsigned int *ip = code;

  while (1)
    switch (*ip++)
      {
      case 0:
	puts ("op0");
	return 0;
      case 1:
	puts ("op1");
	break;
      case 2:
	puts ("op2");
	break;
      }
}

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


Could you look at the code generated by that compiler for this file?

With GCC 4.6.2 at -O2, the dispatch with labels-as-values looks like this:

	mov	-4(%rbx), %eax
	movq	jump_table.2080(,%rax,8), %rax
	addq	$4, %rbx
	jmp	*%rax

In the other case it’s a series of comparisons like this:

	cmpl	$2, %eax
	jne	.L14
	movl	$.LC2, %edi
	call	puts
	movl	(%rbx), %eax
	addq	$4, %rbx
	cmpl	$1, %eax
	jne	.L16
	movl	$.LC1, %edi
	call	puts
	jmp	.L14

> Later/future versions may do better, so we can update it with
> version-number checks, unless we want to build performance tests into
> the configure script, which doesn't sound like a great idea to me.

Agreed, but OTOH #ifdef __LLVM__ isn’t future-proof.

Thanks,
Ludo’.

Re: Using labels-as-values on MacOS X

[Hans Aberg]

On 25 May 2012, at 17:31, Ludovic Courtès wrote:

> Ken Raeburn <raeburn@raeburn.org> skribis:
> 
>> * Don't use addresses of code labels with LLVM, even if the compiler
>> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
>> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build
>> 2336.1.00)"),
> 
> Damn, what compiler is this?  It’s not the old GCC 4.2 fork?  Is it
> Clang?  GCC with DragonEgg?

$ /usr/bin/gcc --version
i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.9.00)
$ ls -l /usr/bin/gcc
lrwxr-xr-x  1 root  wheel  12 Mar 23 21:20 /usr/bin/gcc -> llvm-gcc-4.2

It comes with Xcode 4.3.2 (on Mac OS X 10.7.4). LLVM was originally intended as a backend for GCC, and this a variation of that compiler.

There is also
$ ls -l /usr/bin/cc
lrwxr-xr-x  1 root  wheel  5 Mar 23 21:19 /usr/bin/cc -> clang
$ clang --version
Apple clang version 3.1 (tags/Apple/clang-318.0.58) (based on LLVM 3.1svn)

In addition, I have
$ which gcc
/usr/local/bin/gcc
$ gcc --version
gcc (GCC) 4.7.0

It was compiled using the LLVM-GCC above.

Hans

Re: Using labels-as-values on MacOS X

[Ludovic Courtès]

Hi,

Hans Aberg <haberg-1@telia.com> skribis:

> On 25 May 2012, at 17:31, Ludovic Courtès wrote:
>
>> Ken Raeburn <raeburn@raeburn.org> skribis:
>> 
>>> * Don't use addresses of code labels with LLVM, even if the compiler
>>> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
>>> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build
>>> 2336.1.00)"),
>> 
>> Damn, what compiler is this?  It’s not the old GCC 4.2 fork?  Is it
>> Clang?  GCC with DragonEgg?
>
> $ /usr/bin/gcc --version
> i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.9.00)
> $ ls -l /usr/bin/gcc
> lrwxr-xr-x  1 root  wheel  12 Mar 23 21:20 /usr/bin/gcc -> llvm-gcc-4.2

They put so much effort into making a compiler indistinguishable from
GCC that all we can hope for is that it generates code that performs as
well as GCC’s.  I wouldn’t want to #ifdef around these shameless hacks.

Thanks for the info!

Ludo’.

Re: Using labels-as-values on MacOS X

[Hans Aberg]

On 26 May 2012, at 14:44, Ludovic Courtès wrote:

> Hans Aberg <haberg-1@telia.com> skribis:
> 
>> On 25 May 2012, at 17:31, Ludovic Courtès wrote:
>> 
>>> Ken Raeburn <raeburn@raeburn.org> skribis:
>>> 
>>>> * Don't use addresses of code labels with LLVM, even if the compiler
>>>> supports them.  At least with the version of LLVM GCC on my Mac ("gcc
>>>> version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build
>>>> 2336.1.00)"),
>>> 
>>> Damn, what compiler is this?  It’s not the old GCC 4.2 fork?  Is it
>>> Clang?  GCC with DragonEgg?
>> 
>> $ /usr/bin/gcc --version
>> i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.9.00)
>> $ ls -l /usr/bin/gcc
>> lrwxr-xr-x  1 root  wheel  12 Mar 23 21:20 /usr/bin/gcc -> llvm-gcc-4.2
> 
> They put so much effort into making a compiler indistinguishable from
> GCC that all we can hope for is that it generates code that performs as
> well as GCC’s.  I wouldn’t want to #ifdef around these shameless hacks.

When compiling packages, I normally use /usr/bin/gcc, which is llvm-gcc then, because I think it was what package writer normally would expect. So 'configure' normally chooses gcc, which then becomes llvm-gcc unless one is doing something, like setting CC or installing ones own GCC.

Otherwise, the system compiler is now actually clang:
  $ ls -l /usr/bin/cc
  /usr/bin/cc -> clang
Before it was LLVM-GCC which still has a link from /usr/bin/gcc.

> Thanks for the info!

You are welcome.

Hans