Re: request review: branch "wingo"

[Neil Jerram]

Andy Wingo <address@hidden> writes:

> Hi Neil,
>
> Thanks for the review.

Pleasure.  By the way, on the general point of pushing
possibly-contentious stuff to git.sv.gnu.org master...

- I certainly don't think it's the end of the world to do this.  We
  have complete history and can back things out if we need to.  It's
  also (IMO) a reasonable way of asking for review.  And if another
  developer ends up pulling something that they don't want, it's easy
  enough in Git to remove those things from a local tree.

- On the other hand, it does feel slightly incourteous, and the
  argument about master being broken sounds like nonsense - because
  everyone has their own local branches, and AFAIK there is never any
  need to push those apart from for publication purposes.  (Is there?)

On balance, I think it would be better to give an opportunity for
review first, either as a patch email or as commits on another branch.

> 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.

No, it's fine as is then; my fault for missing that.

>> 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...

I personally like the symbols, but precedent (e.g. socket, bind etc.)
would favour just the integer constants.  If the constants are also
better for completion, I guess that clinches it. :-)

>> 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.

OK, fine, thanks for explaining that.

>> - 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.

Yes, OK; after more thought I now agree with you.  There was some
discussion when I recently raised the question of stack overflow
(starting here:
http://www.mail-archive.com/address@hidden/msg02098.html), but
the only concern there was about how the stack depth tracking is
implemented - which there is no suggestion (now) of changing.

I'm favouring 80% getrlimit now because I don't think Guile (and I)
have really benefitted from the time spent investigating stack
overflows recently,  OK, so we are familiar in more detail with some
platforms using more stack than others, but that's not that
interesting and the time would probably have been better spent on
something else... so let's try not to have to do any more of this in
future.

So my concern now is are there platforms that don't provide getrlimit
(or equivalent)?  If not, I'm happy to rip out all the stack
calibration stuff; but if there are, don't we still need to keep it as
a fallback option?

>> - 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.)

Should we then put the Makefile.am code back?  Or does that break your
uninstalled usage?  Other things being equal, I think it's more
important for the generated guile-config to be simple, than for our
Makefile.am to be simple.

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

Great, thanks!

Regards,
        Neil