Re: request review: branch wingo
Andy Wingo wi...@pobox.com writes: I went ahead and pushed an 80%-only patch, as you suggest. All UNIX systems that I know of have getrlimit. Windows does not of course, but it seems that their stack limit is hard-coded: http://www.mapleprimes.com/forum/stack_limit In any case, it seems that when Windows people bump up against this limit they'll let us know and we can send them looking for the right #define. OK, that sounds like a reasonable position. Thanks, Neil
Re: request review: branch wingo
Howdy, On Wed 01 Apr 2009 15:23, Neil Jerram n...@ossau.uklinux.net writes: l...@gnu.org (Ludovic Courtès) writes: I have no objection to that. We still want to support existing scripts, of course - but I assume that's why you said mark as deprecated and not remove. :-) I agree :) We can make guile-config get its information from pkg-config instead. Cheers, Andy -- http://wingolog.org/
Re: request review: branch wingo
Hi Neil, Neil Jerram n...@ossau.uklinux.net writes: 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. Speaking of which, should we mark `guile-config' as deprecated in favor of `pkg-config' (in 1.9)? Thanks, Ludo'.
Re: request review: branch wingo
Hello! Andy Wingo wi...@pobox.com writes: On Tue 31 Mar 2009 14:31, l...@gnu.org (Ludovic Courtès) writes: Besides, there's the thread about cross-compilation where we mention building the compiler with an already installed Guile that may have an inappropriate stack limit. I don't think that is relevant. Since the Guile that is running would choose a stack size appropriate for it, based on the host getrlimit, there would be no problem. The already-installed Guile wouldn't use getrlimit(2) since that would be an old 1.8. You probably saw the other response already, but I don't think that the compiler will work with 1.8 as a host. Yes, I saw it right after, so forget my remark. You'd have to install a 1.9 on the host, and somehow tell it that it should compile .go files with the endianness of the target. OK. Thanks, Ludo'.
Re: request review: branch wingo
Andy Wingo wi...@pobox.com writes: So, I really intended to wait for review, but it's irritating having `master' broken, so I went ahead and merged this in. You waited for 31 hours and I still don't know how it was broken, which I find irritating as well. I think the stack calibration stuff is correct, Again, this all boils down to an arbitrary choice: 1 MiB instead of 40 KiB. Surely someday this won't be enough. Besides, there's the thread about cross-compilation where we mention building the compiler with an already installed Guile that may have an inappropriate stack limit. 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. I think the rationale (The proximate cause of its creation is that I want to be able to build external packages against uninstalled Guile, and to do that I need guile-tools in the PATH, but I don't want $top_builddir/libtool in the path.) is questionable. Things are not meant to work this way (Libtool's `.la' and executable scripts, `.pc' files, etc.), so it looks quite hackish to me. I'm probably biased because I've got used to installing Guile when I want to test apps against it (I have 1.8 and HEAD under a different prefix so I can test against both). Thanks, Ludo'.
Re: request review: branch wingo
Hello, Andy Wingo wi...@pobox.com writes: .scm.go: $(MKDIR_P) `dirname $...@` $(top_builddir)/pre-inst-guile \ -l $(top_builddir)/libguile/stack-limit-calibration.scm \ $(top_srcdir)/scripts/compile -o $@ $ This will run the compile script, but since there is no entry point (-e), you just load the compile script then exit. No compilation happens. Ouch, indeed. Mea culpa! Besides, there's the thread about cross-compilation where we mention building the compiler with an already installed Guile that may have an inappropriate stack limit. I don't think that is relevant. Since the Guile that is running would choose a stack size appropriate for it, based on the host getrlimit, there would be no problem. The already-installed Guile wouldn't use getrlimit(2) since that would be an old 1.8. Linking against uninstalled libtool libraries works fine, as long as you don't install. That's right, but that seems awkward to me, except for tests. Pkg-config is designed for uninstalled operation, from pkg-config(1): OK, I didn't know that. Sorry for talking too fast. I have that too, but it adds a step to the debugging cycle. I don't think there's any harm in supporting this additional mode of hacking, which is only for hackers in any case. Well, yes. Thanks, Ludo'.
Re: request review: branch wingo
Howdy howdy, On Tue 31 Mar 2009 14:31, l...@gnu.org (Ludovic Courtès) writes: Besides, there's the thread about cross-compilation where we mention building the compiler with an already installed Guile that may have an inappropriate stack limit. I don't think that is relevant. Since the Guile that is running would choose a stack size appropriate for it, based on the host getrlimit, there would be no problem. The already-installed Guile wouldn't use getrlimit(2) since that would be an old 1.8. You probably saw the other response already, but I don't think that the compiler will work with 1.8 as a host. You'd have to install a 1.9 on the host, and somehow tell it that it should compile .go files with the endianness of the target. Linking against uninstalled libtool libraries works fine, as long as you don't install. That's right, but that seems awkward to me, except for tests. I've used it in GStreamer for years now -- not a proof of correctness to be sure, but it works well enough to hack. Cheers, Andy -- http://wingolog.org/
Re: request review: branch wingo
Andy Wingo wi...@pobox.com 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 n...@ossau.uklinux.net 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/guile-devel@gnu.org/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
Re: request review: branch wingo
Andy Wingo wi...@pobox.com writes: Hey all, On Fri 27 Mar 2009 16:29, Andy Wingo wi...@pobox.com writes: On the wingo branch in the main repository, you will find the following patches: So, I really intended to wait for review, but it's irritating having `master' broken, so I went ahead and merged this in. Here are my comments on the merged commits. - allow building against uninstalled guile; move some things to meta/ Looks like a good idea. Could be some gotchas lurking, but I don't immediately see any. - add getrlimit and setrlimit wrappers Should scm_getrlimit code be surrounded by #ifdef HAVE_GETRLIMIT ? What's the idea of the numerical args? Should RLIMIT_XXX be defined somewhere as Scheme-level integers? 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? - 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. But maybe this isn't important enough to justify the hassle around stack-limit calibration. What do others think? - fix check for guile-tools running uninstalled - fix distcheck hopefully, by cleaning the vm-i-*.i files Fine. - fix linking of guile-config I don't understand the problem here. In what way was @bindir@ not fully expanded? - bugfix: don't dynamic link if we found a registered extension Would be great to have a test for this, if feasible. 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. Regards, Neil
Re: request review: branch wingo
Hey all, On Fri 27 Mar 2009 16:29, Andy Wingo wi...@pobox.com writes: On the wingo branch in the main repository, you will find the following patches: So, I really intended to wait for review, but it's irritating having `master' broken, so I went ahead and merged this in. 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. Please let me know if yall have a concern with this merge -- we can fix it tomorrow or Monday. Cheers, Andy -- http://wingolog.org/
Re: request review: branch wingo
Andy Wingo wi...@pobox.com writes: Please let me know if yall have a concern with this merge -- we can fix it tomorrow or Monday. I'll aim to take a look tomorrow evening. Neil
