Hi there,

2008/9/12 Ludovic Courtès <ludo@gnu.org>:
>
> That's the second important thing that should go in 1.8.6 IMO.

Cool...

>> +  /* x1 and x2 are the stack depth values that we get on a Debian
>> +     GNU/Linux ia32 system - which we take as our canonical system.
>> +     y1 and y2 are the values measured on the system where Guile is
>> +     currently running. */
>> +  int x1 = 170, x2 = 690, y1, y2;
>
> These results are dependent on what the loop in `boot-9.scm' does.
> Thus, it'd be nicer if they weren't that far away from it.

Agreed, and I think I've addressed this in my latest version (below).

> It might be worth mentioning the GCC version and optimization level that
> led to this result.

Good idea, will do.  (This isn't yet in the attached patch.)

> Also, `x1' and `x2' can be made "static const" or some such.

In the new version, they are #defines.

>> +  SCM_VALIDATE_INT_COPY (1, d1, y1);
>> +  SCM_VALIDATE_INT_COPY (2, d2, y2);
>> +
>> +  calibrated_m = ((double) (y2 - y1)) / (x2 - x1);
>> +  calibrated_c = ((double) y2) - calibrated_m * x2;
>
> Shouldn't it be:
>
>  calibrated_c = y1 - x1;

No, don't think so!  My model equation is y = mx + c, so c = y - mx.

> Also, the computation of `calibrated_m' needs more casts to `double' I
> think.

I don't think so, and this hasn't changed in the new version.  Where
and why do you think more casts are needed?

> Could it be moved to a `%print-stack-calibration' function that we'd use
> for troubleshooting?

Yes.  In the attached patch, I've left this out completely, but I
think we should add it back in as %get-stack-calibration.

> How about entirely removing the startup overhead by computing the
> calibration factors only once, at installation time?
>
> This would mean:
>
>  1. Compile all of Guile with the default calibration factors (m = 1
>     and c = 0).

Agreed.

>  2. Run a Scheme script that computes `m' and `c' and produces, say,
>     `calibration.h', which is included by `stackchk.c'.  Both the
>     computation and the reference stack depths (`x1' and `x2' above)
>     would live in this script, which is clearer IMO.

Agreed, see new libguile/calibrate.scm file.

>  3. Invoke `make' recursively, which should rebuild libguile with the
>     appropriate calibration factor (typically, only `stackchk.lo' would
>     need to be recompiled).

I've done this part a bit differently - see the libguile/Makefile.am
changes - because I couldn't see exactly how the recursive make
approach would work.  If you think recursive make would be
significantly better, can you describe or propose the detailed changes
that would be needed?

> Also, the on-line and Texi documentation of `eval-options' must be
> updated to specify that the stack depth unit is "abstract" and
> (hopefully) portable across platforms.

I will do this (and also NEWS), but let's agree all the code first.

Many thanks for your detailed review, BTW!

Regards,
      Neil