unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
From: Ken Raeburn <raeburn@raeburn.org>
To: guile-devel <guile-devel@gnu.org>
Subject: a few proposed patches
Date: Mon, 21 May 2012 01:45:50 -0400	[thread overview]
Message-ID: <ACF34E1C-B9FA-47F1-B178-0797F4BA7E65@raeburn.org> (raw)

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


             reply	other threads:[~2012-05-21  5:45 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-05-21  5:45 Ken Raeburn [this message]
2012-05-21  6:41 ` a few proposed patches Neil Jerram
2012-05-22  7:54 ` Andy Wingo
2012-05-22 16:51   ` Ken Raeburn
2012-05-22  8:17 ` Andy Wingo
2012-05-22  8:21   ` Andy Wingo
2012-05-22 16:54   ` Ken Raeburn
2012-05-25 14:56     ` Removing ‘GC_PTR’ Ludovic Courtès
2012-05-25 15:31 ` Using labels-as-values on MacOS X Ludovic Courtès
2012-05-25 16:32   ` Hans Aberg
2012-05-26 12:44     ` Ludovic Courtès
2012-05-26 12:58       ` Hans Aberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ACF34E1C-B9FA-47F1-B178-0797F4BA7E65@raeburn.org \
    --to=raeburn@raeburn.org \
    --cc=guile-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).