This patch fixes failures of test such as:
// RUN: clang-cc -E %s -fno-caret-diagnostics 2>&1 >/dev/null | grep ‘file successfully included’ | count 3
when run on Windows with the shell emulation.
stderr_redirection.patch (1.87 KB)
Baptiste,
I’m glad you’re helping with the tests.
I’ve applied your patches (after first applying Daniel’s, and using his command line for running the tests), and saw the following results:
After add_python_exe_path.patch, no change: 251 failures
After clang-win32-path.patch and llvm-win32-path.patch: 230 failures
After stderr_redirection.patch: 250 failures
I then reverted the TestRunner.py patches and am back at 230 failures, which still is a great improvement. I’m not sure what the problem was with the other patches.
-John
Oh, also, should the Path.inc patch also apply to the other isAbsolute function there?
-John
Baptiste,
Having found I forgot to fix the paths in the count.bat and not.bat files, I tried your patches again, but still had the following results:
After add_python_exe_path.patch, no change: 64 failures (no change)
After stderr_redirection.patch: 106 failures
…assuming I applied the patch correctly. I had to do it manually because of differences in our versions.
-John
Hi Baptiste,
This patch isn't quite correct, the situation is both more complicated
and simpler. What the script needs to do is just model the file
mappings (which start out as the default), and then apply each
redirection to that model. Then at the end, it needs to verify that we
can call the subprocess module with the right settings to match the
redirection.
For now, a simpler patch which I would be fine with is to just change
those 4 tests to use a temporary file instead of shell redirection
fun, e.g.:
RUN: clang 2> %t
RUN: grep 'file ...' %t | count 3
- Daniel
2009/8/13 Daniel Dunbar <daniel@zuster.org>
Hi Baptiste,
This patch isn’t quite correct, the situation is both more complicated
and simpler. What the script needs to do is just model the file
mappings (which start out as the default), and then apply each
redirection to that model. Then at the end, it needs to verify that we
can call the subprocess module with the right settings to match the
redirection.For now, a simpler patch which I would be fine with is to just change
those 4 tests to use a temporary file instead of shell redirection
fun, e.g.:RUN: clang 2> %t
RUN: grep ‘file …’ %t | count 3
- Daniel
You’re right, this will work just as well. It is a lot simpler and remove the /dev/null issue.
Can you elaborate why the patch is incorrect? From the tests I made the stderr of clang-cc is correctly redirected to stdin of count/grep… What am I missing?
2009/8/12 John Thompson <john.thompson.jtsoftware@gmail.com>
Baptiste,
Having found I forgot to fix the paths in the count.bat and not.bat files, I tried your patches again, but still had the following results:
After add_python_exe_path.patch, no change: 64 failures (no change)
After stderr_redirection.patch: 106 failures…assuming I applied the patch correctly. I had to do it manually because of differences in our versions.
-John
I also needed to create a directory e:\dev to allow /dev/null file to be created.
Have a look at your failure, if you see some error such as header “stdio.h” or “wchar.h” missing, then you need to change InitHeaderSearch::AddDefaultSystemIncludePaths (in tools\clang\lib\Frontend\InitHeaderSearch.cpp).
I’ve pushed my change on https://code.launchpad.net/llvm mirror. Checks the win32 branches.
I currently have the following failures:
Failing Tests (45):
.\Analysis\casts.c
.\Analysis\complex.c
.\Analysis\null-deref-ps.c
.\CodeGenCXX\new.cpp
.\CodeGenCXX\references.cpp
.\CodeGen\address-space-field1.c
.\CodeGen\address-space-field2.c
.\CodeGen\address-space-field3.c
.\CodeGen\address-space-field4.c
.\CodeGen\cast-to-union.c
.\CodeGen\const-init.c
.\CodeGen\mandel.c
.\CodeGen\mmintrin-test.c
.\CodeGen\stack-protector.c
.\Driver\dragonfly.c
.\Driver\freebsd.c
.\Driver\hello.c
.\Driver\openbsd.c
.\Driver\pth.c
.\Frontend\dependency-gen.c
.\Lexer\11-27-2007-FloatLiterals.c
.\Misc\predefines.c
.\PCH\exprs.c
.\PCH\reloc.c
.\Parser\cxx-template-decl.cpp
.\Preprocessor\header_lookup1.c
.\Preprocessor\line-directive.c
.\Preprocessor\macro_paste_bcpl_comment.c
.\SemaCXX\implicit-int.cpp
.\SemaCXX\nested-name-spec.cpp
.\SemaCXX\nullptr.cpp
.\SemaCXX\typedef-redecl.cpp
.\SemaTemplate\example-dynarray.cpp
.\SemaTemplate\nested-name-spec-template.cpp
.\Sema\complex-int.c
.\Sema\complex-promotion.c
.\Sema\const-eval.c
.\Sema\const-ptr-int-ptr-cast.c
.\Sema\exprs.c
.\Sema\format-strings.c
.\Sema\i-c-e.c
.\Sema\init.c
.\Sema\return.c
.\Sema\static-init.c
.\Sema\wchar.c
I’ve checked a few of them and they seems to be real failures. Though, I couldn’t figure out where to find the current list of failure on buildbot…
2009/8/12 John Thompson <john.thompson.jtsoftware@gmail.com>
Oh, also, should the Path.inc patch also apply to the other isAbsolute function there?
Maybe, though I have not found a failing test triggered by this yet. Though, it would be likely better to have the second function calling the “fixed” one.
Baptiste,
I had made an error applying the patch, which I had to do manually,
since our versions didn't match up. I forgot that in Python,
indentation is everything...
I had already resolved the include path issue. Creating a \dev
directory removed a couple of failures.
Comparing my test failures and yours, it seems you have some failures
I don't have, and I have some you don't have. Of special interest to
me is that it seems you don't have some of the grep problem I'm
having. So which grep are you using? This information would be very
helpful to me.
-John
Hi Baptiste,
This patch isn't quite correct, the situation is both more complicated
and simpler. What the script needs to do is just model the file
mappings (which start out as the default), and then apply each
redirection to that model. Then at the end, it needs to verify that we
can call the subprocess module with the right settings to match the
redirection.For now, a simpler patch which I would be fine with is to just change
those 4 tests to use a temporary file instead of shell redirection
fun, e.g.:RUN: clang 2> %t
RUN: grep 'file ...' %t | count 3- Daniel
You're right, this will work just as well. It is a lot simpler and remove
the /dev/null issue.
I'm not sure if you have already done this on your branch, but if you
submit a patch for this issue I will apply it.
Can you elaborate why the patch is incorrect? From the tests I made the
stderr of clang-cc is correctly redirected to stdin of count/grep... What am
I missing?
Well, the patch handles one case but the mechanism (in the shell) is
much more general. One can do all sorts of unreadable things in the
shell, e.g:
echo "hi" 2> a 1>&2
which aren't really modeled by your patch.
- Daniel