Re: request review: branch "wingo"

[Andy Wingo]

Hi Neil,

Thanks for the review.

On Mon 30 Mar 2009 14:39, Neil Jerram <address@hidden> writes:

> - add getrlimit and setrlimit wrappers
>
> Should scm_getrlimit code be surrounded by #ifdef HAVE_GETRLIMIT ?

I think it is -- the same block includes the RLIM_*, the helper
function, and the setrlimit definition. Not very clear though, I admit.

> What's the idea of the numerical args?  Should RLIMIT_XXX be defined
> somewhere as Scheme-level integers?

Uf, dunno. Now that I look at this again I'm not sure. We should
probably just have RLIM_* and not the symbols, right? Easier for tab
completion in any case...

> Does it make sense to want to set the soft limit only?  I thought some
> hard limits were only settable by root, so I would suspect yes.  If
> yes, does the setrlimit implementation support this?

You actually have to set both at the same time, in the system call
interface. So if you want to just modify the soft limit, you have to
first getrlimit to find the hard limit.

Hard limits can only be decreased, not increased, and can be done so by
the user process. This is good for e.g. before forking too.

> - getrlimit-based stack limits
> - rely on getrlimit to DTRT, don't make stack calibration file
>
> My inclination is that we should revert these; but I'm not sure, so
> could be persuaded that I'm wrong.
>
> My feeling is that what we had before is a stronger statement (than
> using getrlimit) about the expected behaviour of typical Scheme
> programs (and in particular those used during the build), and is
> therefore worth keeping.  The getrlimit implementation just says "if a
> program is running away, we'll generate a Scheme exception a little
> bit before you'd otherwise get a core".

I think that with the getrlimit code, we keep these guarantees, as much
as we had them before.

Before, you still didn't know that your program was not going to dump
core, because when you were at 35000 words (say) and a C program
recursed up to the limit without reentering Guile, you'd still dump
core. (A contrived example.)

What we have now is similar -- Guile's stack is set to 80% of the
rlimit, or 1 MB, whichever is less. So in the worst case we have 20% of
runway in which to prevent a segfault, and in the normal case (on my
x86-32 laptop) we have 9 MB. It's actually a stronger guarantee, for
example in the case in which your rlimit was set at 41000 words.

> - fix "linking" of guile-config
>
> I don't understand the problem here.  In what way was @bindir@ not
> fully expanded?

Because it was a make variable and not a shell variable, so it expanded
to ${exec_prefix}/bin. (There was code in the Makefile.am before to sed
in the variables at make-time instead of configure-time, but I had
removed it to simplify things.)

> - bugfix: don't dynamic link if we found a registered extension
>
> Would be great to have a test for this, if feasible.

Done.

>> I think the stack calibration stuff is correct, but perhaps more jarring
>> in this commit is a move from ./pre-inst-guile to ./meta/guile, and
>> ./pre-inst-guile-env to ./meta/uninstalled-env. I describe the rationale
>> in 0b6d8fdc28ed8af56e93157179c305fef037e0a0. But then again, given that
>> Neil invested so much time into the stack calibration stuff, that might
>> be jarring too.
>
> As above - the meta/ stuff looks fine to me, but I'm not sure about
> the stack limit change.

I think that it gives us stronger guarantees and less hassle. Did my
reasons sway your opinion at all?

Andy
-- 
http://wingolog.org/