tests: handle "funny" exit code 127 produced by MSVC-compiled exes

Commit Message

Johannes Schindelin Oct. 30, 2023, 3:45 p.m. UTC

From: Johannes Schindelin <[e-mail]>

The exit code 127 is well-documented to mean: command not found.

Unfortunately, it is also used as fall-back in Cygwin's
`pinfo::status_exit()` method (which maps things like Windows'
`STATUS_ACCESS_VIOLATION` to `128 | SIGSEGV`).

This is particularly unfortunate because there is no explicit mapping
for `STATUS_STACK_OVERFLOW`. Meaning: when MSVC-compiled executables
produce a stack overflow the exit code in the Cygwin Bash will be 127.
Consequently, the same will be true for the MSYS2 Bash that is used by
Git for Windows.

Now, `jk/tree-name-and-depth-limit` introduces a pair of test cases that
expect a command that produces a stack overflow to fail, which it
typically does with exit code 139 (which means SIGSEGV).

But since MSVC-compiled `git.exe` exits with `STATUS_STACK_OVERFLOW`
which the MSYS2 runtime maps to 127, and since 127 is taken to mean
"command not found" by `test_must_fail`, even though everything works as
planned the two new test cases fail when run in `win+VS test`.

Let's work around this by:

1) recording which C compiler was used, and

2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a
   regular failure.

There is a slight downside of this approach in that a real missing
command could be mistaken for a failure. However, this would be caught
on other platforms, and besides, we use `test_must_fail` only for `git`
and `scalar` anymore, and we can be pretty certain that both are there.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
    Fix t6700.[45] in win+VS test
    
    These two test cases have been failing for a while in Git for Windows'
    shears/* branches. Took a good while to figure out, too.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1604

 contrib/buildsystems/CMakeLists.txt | 3 ++-
 t/test-lib-functions.sh             | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)


base-commit: 3130c155df9a65ebccf128b4af5a19af49532580

Comments

Jeff King Oct. 30, 2023, 5:56 p.m. UTC | #1

On Mon, Oct 30, 2023 at 03:45:32PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Now, `jk/tree-name-and-depth-limit` introduces a pair of test cases that
> expect a command that produces a stack overflow to fail, which it
> typically does with exit code 139 (which means SIGSEGV).

I think you're misinterpreting the purpose of the tests from that
series; they're not intended to segfault. Quoting from t6700:

  # We'll test against two depths here: a small one that will let us check the
  # behavior of the config setting easily, and a large one that should be
  # forbidden by default. Testing the default depth will let us know whether our
  # default is enough to prevent segfaults on systems that run the tests.

So for the "big tree" tests in that file, we are looking for a
controlled failure rather than a segfault. And indeed, the end of that
series already lowered the default to accommodate the msys windows
build; see the discussion in 4d5693ba05 (lower core.maxTreeDepth default
to 2048, 2023-08-31).

So I think the test is working as designed here: it is showing us that
the default value is not sufficient to protect MSVC builds from running
out of stack space. There are a few options there:

  1. We can lower the default everywhere.

  2. We can lower it just for MSVC builds.

  3. We can accept the situation and skip the tests for that build.

There's a bit more discussion in the commit I referenced above.

> Let's work around this by:
> 
> 1) recording which C compiler was used, and
> 
> 2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a
>    regular failure.
> 
> There is a slight downside of this approach in that a real missing
> command could be mistaken for a failure. However, this would be caught
> on other platforms, and besides, we use `test_must_fail` only for `git`
> and `scalar` anymore, and we can be pretty certain that both are there.

I think there is another much worse downside to your patch: we will stop
noticing when MSVC builds segfault in the tests. The purpose of
test_must_fail is to allow controlled and expected failure returns from
the command, but still report on unexpected situations (signal death,
command not found, and so on).

-Peff

Patch

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 6b819e2fbdf..e164484be98 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1057,7 +1057,8 @@  if(NOT PYTHON_TESTS)
 	set(NO_PYTHON 1)
 endif()
 
-file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SHELL_PATH='${SHELL_PATH}'\n")
+file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "CMAKE_C_COMPILER='${CMAKE_C_COMPILER}'\n")
+file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SHELL_PATH='${SHELL_PATH}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "TEST_SHELL_PATH='${TEST_SHELL_PATH}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PERL_PATH='${PERL_PATH}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "DIFF='${DIFF}'\n")
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..ee19c748973 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1112,6 +1112,9 @@  test_must_fail () {
 		return 1
 	elif test $exit_code -eq 127
 	then
+		# Work-around for MSVC-compiled executables
+		case "$CMAKE_C_COMPILER" in *MSVC*) return 0;; esac
+
 		echo >&4 "test_must_fail: command not found: $*"
 		return 1
 	elif test $exit_code -eq 126