[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
a few proposed patches
a few proposed patches
Mon, 21 May 2012 01:45:50 -0400
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….
- a few proposed patches,
Ken Raeburn <=