Re: Unexpected pass for t6120-describe.sh on cygwin
Hi Michael, On Wed, 13 Sep 2017, Michael J Gruber wrote: > Could you please try and report on the following (cygwin, MinGW): > > ulimit -s > ulimit -s 4096 # anything lower than the value from above > ulimit -s > bash -c "ulimit -s" Git Bash (MINGW, well, not precisely [*1*]): me@work MINGW64 ~ $ ulimit -s 2032 me@work MINGW64 ~ $ ulimit -s 4096 # anything lower than the value from above me@work MINGW64 ~ $ ulimit -s 4096 me@work MINGW64 ~ $ bash -c "ulimit -s" 2032 Judging by your comment, 4096 should be replaced. So here goes again: me@work MINGW64 ~ $ ulimit -s 1024 me@work MINGW64 ~ $ ulimit -s 1024 me@work MINGW64 ~ $ bash -c "ulimit -s" 2032 And here is the same output of my 64-bit Cygwin installation (just updated to the current [*2*] one): me@work ~ $ ulimit -s 2032 me@work ~ $ ulimit -s 1024 me@work ~ $ ulimit -s 1024 me@work ~ $ bash -c "ulimit -s" 2032 Ciao, Dscho Footnote *1*: I know it is confusing for Linux folks, there are two very different classes of executables in Git for Windows: MSYS2 ones and MINGW ones. The former implicitly link against the MSYS2 runtime, and therefore can make use of its POSIX emulation layer, the latter do not, and therefore they can use "only" what the Win32 API provides. For details, see https://github.com/git-for-windows/git/wiki/The-difference-between-MINGW-and-MSYS2
Re: Unexpected pass for t6120-describe.sh on cygwin
Johannes Schindelin venit, vidit, dixit 12.09.2017 15:39: > Hi Ramsay, > > On Sat, 9 Sep 2017, Ramsay Jones wrote: > >> I ran the test-suite on the 'pu' branch last night (simply because that >> was what I had built at the time!), which resulted in a PASS, but t6120 >> was showing a 'TODO passed' for #52. >> >> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack' >> branch, which uses 'ulimit -s' to try and force a stack overflow. >> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created >> a test program (see below) to eat up the stack and tried running it with >> various ulimit values (128, 12, 8), but it always seg-faulted at the >> same stack-frame. (after using approx 2MB stack space). >> >> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled >> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests, but >> haven't looked into it. >> >> Given that 'ulimit' is a bash built-in, this may also be a problem on >> MinGW and Git-For-Windows, but I can't test on those platforms. > > It is. Thanks. I just dug something up from an old cygwin list post. Could you please try and report on the following (cygwin, MinGW): ulimit -s ulimit -s 4096 # anything lower than the value from above ulimit -s bash -c "ulimit -s" My suspicion from that post is that ulimit affects the sub shell only - if yes, running a test inside the sub shell to confirm whether the setting is in effect would be great, of course. /usr/bin/echo $(seq 10) or such does the job on Linux (with stack limit 128), but I don't know whether this is portably reliable. If ulimit on these platforms has no effect but does not lie about the value either it's a simple prerequisite test (test ulimit present, if yes set and get and compare the stack size values). Michael
Re: Unexpected pass for t6120-describe.sh on cygwin
Hi Ramsay, On Sat, 9 Sep 2017, Ramsay Jones wrote: > I ran the test-suite on the 'pu' branch last night (simply because that > was what I had built at the time!), which resulted in a PASS, but t6120 > was showing a 'TODO passed' for #52. > > This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack' > branch, which uses 'ulimit -s' to try and force a stack overflow. > Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created > a test program (see below) to eat up the stack and tried running it with > various ulimit values (128, 12, 8), but it always seg-faulted at the > same stack-frame. (after using approx 2MB stack space). > > So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled > on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests, but > haven't looked into it. > > Given that 'ulimit' is a bash built-in, this may also be a problem on > MinGW and Git-For-Windows, but I can't test on those platforms. It is. Thanks, Dscho
Re: Unexpected pass for t6120-describe.sh on cygwin
On 11/09/17 11:31, Adam Dinwoodie wrote:
> On Sat, Sep 09, 2017 at 02:13:32PM +0100, Ramsay Jones wrote:
>> I ran the test-suite on the 'pu' branch last night (simply because
>> that was what I had built at the time!), which resulted in a PASS,
>> but t6120 was showing a 'TODO passed' for #52.
>
> Confirmed, I also see this unexpected pass.
>
>> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
>> branch, which uses 'ulimit -s' to try and force a stack overflow.
>> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
>> a test program (see below) to eat up the stack and tried running it
>> with various ulimit values (128, 12, 8), but it always seg-faulted
>> at the same stack-frame. (after using approx 2MB stack space).
>
> Yes, Cygwin does not implement the ulimit API from the Single Unix
> Specification, per https://cygwin.com/cygwin-api/std-notimpl.html. I
Isn't that ulimit(3) not ulimit the bash built-in - which would
more than likely be implemented by {get,set}rlimit(). (I haven't
looked to find out, obviously!).
> suspect the Bash builtin still exists because (a) nobody has bothered to
> remove it, (b) including it avoids breaking scripts that assume it
> exists but don't particularly rely on its output being sensical, or
> (c) both.
(d) it seems to provide useful information on some of the limits anyway.
>> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
>> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
>> but haven't looked into it.
>>
>> Given that 'ulimit' is a bash built-in, this may also be a problem
>> on MinGW and Git-For-Windows, but I can't test on those platforms.
>
> I'll leave this to the relevant folks to test; I don't have a useful
> test environment for those either. That said, I'll note the comment in
> t6120 says the ULIMIT_STACK_SIZE prerequisite excludes running the test
> on Windows, so I assume it's not a problem there.
I suspect that was a guess. ;-)
> On Sun, Sep 10, 2017 at 05:58:49PM +0100, Ramsay Jones wrote:
>> On 10/09/17 13:27, Michael J Gruber wrote:
>>> So, I guess, short of testing the effect of "ulimit -s" with another
>>> expensive test, it's best to simply set these prerequisites based on
>>> "uname -s".
>>
>> Yes, I was going to suggest adding !CYGWIN to the test(s) (or perhaps
>> to the 'test_lazy_prereq' expression(s)).
>
> Given the issue is Cygwin not implementing ulimit at all, but Cygwin
> Bash pretending everything's fine, I think handling this as a special
> case for Cygwin seems like the correct approach. Something like the
> below, based on the existing code in test-lib.sh for the PIPE prereq?
>
> -- >8 --
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 83f5d3dd2..376cd91ae 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1166,14 +1166,32 @@ test_lazy_prereq UNZIP '
> test $? -ne 127
> '
>
> +# On Cygwin, ulimit has no effect because the underlying API hasn't been
> +# implemented. Just fail if trying to do something with ulimit.
> run_with_limited_cmdline () {
> - (ulimit -s 128 && "$@")
> + case $(uname -s) in
> + CYGWIN*)
> + false
> + ;;
> + *)
> + (ulimit -s 128 && "$@")
> + ;;
> + esac
> }
>
> test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
>
> +# On Cygwin, ulimit has no effect because the underlying API hasn't been
> +# implemented. Just fail if trying to do something with ulimit.
> run_with_limited_stack () {
> - (ulimit -s 128 && "$@")
> + case $(uname -s) in
> + CYGWIN*)
> + false
> + ;;
> + *)
> + (ulimit -s 128 && "$@")
> + ;;
> + esac
> }
>
> test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
>
What about the other uses of ulimit? (No, I still haven't tried
ulimit with open file descriptors - I will assume it doesn't work).
I was thinking something more like this:
$ git diff
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83f5d3dd2..6c939708a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1009,6 +1009,11 @@ case $uname_s in
GIT_TEST_CMP=mingw_test_cmp
;;
*CYGWIN*)
+ # the ulimit bash built-in is ineffective
+ # but always succeeds, so always fail instead
+ ulimit () {
+ false
+ }
test_set_prereq POSIXPERM
test_set_prereq EXECKEEPSPID
test_set_prereq CYGWIN
$
This assumes that 'ulimit -n 32' fails to limit the number
of open files, of course.
ATB,
Ramsay Jones
Re: Unexpected pass for t6120-describe.sh on cygwin
On Sun, Sep 10, 2017 at 02:27:47PM +0200, Michael J Gruber wrote:
> This apparantly expects "ulimit -s" to fail on platforms that don't
> support it, so set the prereq accordingly. I moved the following to
> t/test-lib.sh:
>
> run_with_limited_stack () {
> (ulimit -s 128 && "$@")
> }
>
> test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
>
> Same things as above. Two things to note:
> - Those requisites could be the same, also they are used in different ways.
> - "ulimit -s" returning success without doing anything means that, all
> along, we ran the existing tests when we didn't mean to (on Win), and
> they succeeded for the wrong reason, which we did not notice.
Right, if ulimit doesn't actually limit, then the test isn't really
accomplishing anything. But we likely have several tests in our suite
that do that. As long as they do something on _some_ platforms, they're
still useful. And as long as their failure mode is a false-pass, they
don't bother people on the other platforms.
So I think one option is to just leave it.
> So, I guess, short of testing the effect of "ulimit -s" with another
> expensive test, it's best to simply set these prerequisites based on
> "uname -s".
You can make a cheap test that uses a lot of stack. E.g., this program
runs in about 3ms on my machine, and you can reliably run "ulimit -s 128
&& ./a.out >/dev/null" to detect it segfaulting:
-- >8 --
#include
/*
* Our goal is to use a lot of stack space relatively cheaply. To do that,
* allocate a big stack buffer and recurse. But we take a few precautions to
* avoid a clever compiler optimizing away our stack:
*
* - we need to use the buffer so that it can't be eliminated
*
* - we recurse after touching the buffer but before printing it,
* which makes it hard to do tail-recursion.
*/
static void foo(unsigned x)
{
size_t i;
unsigned char buf[1024];
if (!x)
return;
for (i = 0; i < sizeof(buf); i++)
buf[x] = x & 0xff;
foo(x - 1);
fwrite(buf, 1, sizeof(buf), stdout);
}
int main(void)
{
foo(128);
return 0;
}
-- 8< --
One downside is that it means git's test suite would (on a successful
run) stimulate segfaults, which are sometimes tracked and logged by the
kernel (or may even generate coredumps outside of the test suite area).
-Peff
Re: Unexpected pass for t6120-describe.sh on cygwin
On Sat, Sep 09, 2017 at 02:13:32PM +0100, Ramsay Jones wrote:
> I ran the test-suite on the 'pu' branch last night (simply because
> that was what I had built at the time!), which resulted in a PASS,
> but t6120 was showing a 'TODO passed' for #52.
Confirmed, I also see this unexpected pass.
> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
> branch, which uses 'ulimit -s' to try and force a stack overflow.
> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
> a test program (see below) to eat up the stack and tried running it
> with various ulimit values (128, 12, 8), but it always seg-faulted
> at the same stack-frame. (after using approx 2MB stack space).
Yes, Cygwin does not implement the ulimit API from the Single Unix
Specification, per https://cygwin.com/cygwin-api/std-notimpl.html. I
suspect the Bash builtin still exists because (a) nobody has bothered to
remove it, (b) including it avoids breaking scripts that assume it
exists but don't particularly rely on its output being sensical, or
(c) both.
> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
> but haven't looked into it.
>
> Given that 'ulimit' is a bash built-in, this may also be a problem
> on MinGW and Git-For-Windows, but I can't test on those platforms.
I'll leave this to the relevant folks to test; I don't have a useful
test environment for those either. That said, I'll note the comment in
t6120 says the ULIMIT_STACK_SIZE prerequisite excludes running the test
on Windows, so I assume it's not a problem there.
On Sun, Sep 10, 2017 at 05:58:49PM +0100, Ramsay Jones wrote:
> On 10/09/17 13:27, Michael J Gruber wrote:
> > So, I guess, short of testing the effect of "ulimit -s" with another
> > expensive test, it's best to simply set these prerequisites based on
> > "uname -s".
>
> Yes, I was going to suggest adding !CYGWIN to the test(s) (or perhaps
> to the 'test_lazy_prereq' expression(s)).
Given the issue is Cygwin not implementing ulimit at all, but Cygwin
Bash pretending everything's fine, I think handling this as a special
case for Cygwin seems like the correct approach. Something like the
below, based on the existing code in test-lib.sh for the PIPE prereq?
-- >8 --
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83f5d3dd2..376cd91ae 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1166,14 +1166,32 @@ test_lazy_prereq UNZIP '
test $? -ne 127
'
+# On Cygwin, ulimit has no effect because the underlying API hasn't been
+# implemented. Just fail if trying to do something with ulimit.
run_with_limited_cmdline () {
- (ulimit -s 128 && "$@")
+ case $(uname -s) in
+ CYGWIN*)
+ false
+ ;;
+ *)
+ (ulimit -s 128 && "$@")
+ ;;
+ esac
}
test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
+# On Cygwin, ulimit has no effect because the underlying API hasn't been
+# implemented. Just fail if trying to do something with ulimit.
run_with_limited_stack () {
- (ulimit -s 128 && "$@")
+ case $(uname -s) in
+ CYGWIN*)
+ false
+ ;;
+ *)
+ (ulimit -s 128 && "$@")
+ ;;
+ esac
}
test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
Re: Unexpected pass for t6120-describe.sh on cygwin
On 10/09/17 13:27, Michael J Gruber wrote:
> Ramsay Jones venit, vidit, dixit 09.09.2017 15:13:
[snip]
>> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
>> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
>> but haven't looked into it.
>>
>> Given that 'ulimit' is a bash built-in, this may also be a problem
>> on MinGW and Git-For-Windows, but I can't test on those platforms.
>>
> Thanks for the note. We have this in t/test-lib.sh:
>
> run_with_limited_cmdline () {
> (ulimit -s 128 && "$@")
> }
>
> test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
>
> This apparantly expects "ulimit -s" to fail on platforms that don't
> support it,
Indeed, but cygwin lies (about the stack, anyway) ... :-D
$ ulimit -a
core file size (blocks, -c) unlimited
data seg size (kbytes, -d) unlimited
file size (blocks, -f) unlimited
open files (-n) 256
pipe size(512 bytes, -p) 8
stack size (kbytes, -s) 2032
cpu time (seconds, -t) unlimited
max user processes (-u) 256
virtual memory (kbytes, -v) unlimited
$
$ ulimit -a -H
core file size (blocks, -c) unlimited
data seg size (kbytes, -d) unlimited
file size (blocks, -f) unlimited
open files (-n) 3200
pipe size(512 bytes, -p) 8
stack size (kbytes, -s) unlimited
cpu time (seconds, -t) unlimited
max user processes (-u) 256
virtual memory (kbytes, -v) unlimited
$
Note that ulimit claims that the soft limit for the stack
is (2MB - 16kb), but the hard limit is unlimited. Using the
test program, it is clear that it always segfaults on the
soft limit, no matter what value is requested. (while not
'failing' the request).
Note, also, that the soft/hard limit on open files is 256/3200.
(but I still haven't investigated if ulimit honours that limit
yet).
so set the prereq accordingly. I moved the following to
> t/test-lib.sh:
>
> run_with_limited_stack () {
> (ulimit -s 128 && "$@")
> }
>
> test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
>
> Same things as above. Two things to note:
> - Those requisites could be the same, also they are used in different ways.
> - "ulimit -s" returning success without doing anything means that, all
> along, we ran the existing tests when we didn't mean to (on Win), and
> they succeeded for the wrong reason, which we did not notice.
Yes.
> So, I guess, short of testing the effect of "ulimit -s" with another
> expensive test, it's best to simply set these prerequisites based on
> "uname -s".
Yes, I was going to suggest adding !CYGWIN to the test(s) (or perhaps
to the 'test_lazy_prereq' expression(s)).
ATB,
Ramsay Jones
Re: Unexpected pass for t6120-describe.sh on cygwin
Ramsay Jones venit, vidit, dixit 09.09.2017 15:13:
> Hi Adam,
>
> I ran the test-suite on the 'pu' branch last night (simply because
> that was what I had built at the time!), which resulted in a PASS,
> but t6120 was showing a 'TODO passed' for #52.
>
> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
> branch, which uses 'ulimit -s' to try and force a stack overflow.
> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
> a test program (see below) to eat up the stack and tried running it
> with various ulimit values (128, 12, 8), but it always seg-faulted
> at the same stack-frame. (after using approx 2MB stack space).
>
> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
> but haven't looked into it.
>
> Given that 'ulimit' is a bash built-in, this may also be a problem
> on MinGW and Git-For-Windows, but I can't test on those platforms.
>
> Unfortunately, I can't spend more time on git today, hence this
> heads up! ;-)
Thanks for the note. We have this in t/test-lib.sh:
run_with_limited_cmdline () {
(ulimit -s 128 && "$@")
}
test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
This apparantly expects "ulimit -s" to fail on platforms that don't
support it, so set the prereq accordingly. I moved the following to
t/test-lib.sh:
run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
Same things as above. Two things to note:
- Those requisites could be the same, also they are used in different ways.
- "ulimit -s" returning success without doing anything means that, all
along, we ran the existing tests when we didn't mean to (on Win), and
they succeeded for the wrong reason, which we did not notice.
So, I guess, short of testing the effect of "ulimit -s" with another
expensive test, it's best to simply set these prerequisites based on
"uname -s".
> ATB,
> Ramsay Jones
>
> -- >8 --
> diff --git a/test.c b/test.c
> new file mode 100644
> index 000..bcbb805
> --- /dev/null
> +++ b/test.c
> @@ -0,0 +1,21 @@
> +#include
> +#include
> +#include
> +
> +void test(uint64_t count)
> +{
> + int i, junk[1024];
> +
> + for (i = 0; i < 1024; i++)
> + junk[i] = count;
> + i = junk[count % 1024];
> + printf("%" PRIuMAX "\n", (uintmax_t)count);
> + fflush(stdout);
> + test(count + 1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + test(0);
> + return 0;
> +}
>
Unexpected pass for t6120-describe.sh on cygwin
Hi Adam,
I ran the test-suite on the 'pu' branch last night (simply because
that was what I had built at the time!), which resulted in a PASS,
but t6120 was showing a 'TODO passed' for #52.
This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack'
branch, which uses 'ulimit -s' to try and force a stack overflow.
Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created
a test program (see below) to eat up the stack and tried running it
with various ulimit values (128, 12, 8), but it always seg-faulted
at the same stack-frame. (after using approx 2MB stack space).
So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled
on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests,
but haven't looked into it.
Given that 'ulimit' is a bash built-in, this may also be a problem
on MinGW and Git-For-Windows, but I can't test on those platforms.
Unfortunately, I can't spend more time on git today, hence this
heads up! ;-)
ATB,
Ramsay Jones
-- >8 --
diff --git a/test.c b/test.c
new file mode 100644
index 000..bcbb805
--- /dev/null
+++ b/test.c
@@ -0,0 +1,21 @@
+#include
+#include
+#include
+
+void test(uint64_t count)
+{
+ int i, junk[1024];
+
+ for (i = 0; i < 1024; i++)
+ junk[i] = count;
+ i = junk[count % 1024];
+ printf("%" PRIuMAX "\n", (uintmax_t)count);
+ fflush(stdout);
+ test(count + 1);
+}
+
+int main(int argc, char *argv[])
+{
+ test(0);
+ return 0;
+}
