Page MenuHomePhabricator

[analyzer] Prevent ccc/c++-analyzer from hanging on Windows.
ClosedPublic

Authored by ayartsev on Apr 1 2015, 9:06 AM.

Details

Summary

Migrated to Phabricator from the mailing list, original description:

Attached is the patch that prevents ccc/c++-analyzer from hang if launched with ActivePerl or Strawberry Perl interpreters.
The patch replaces the code that creates a pipe from child to parent to the more portable explicit writing to parent, this prevent interpreters from hang.
The conditions for hang are: child should open a pipe to the parent and parent should read from the child, otherwise no hang.

The problem is possibly caused by the bug in emulation of 'fork' by Perl interpreters on Windows. From perlfork documentation, BUGS section:
"In certain cases, the OS-level handles created by the pipe(), socket(), and accept() operators are apparently not duplicated accurately in pseudo-processes. This only happens in some situations, but where it does happen, it may result in deadlocks between the read and write ends of pipe handles, or inability to send or receive data across socket handles."

An example from perlfork documentation also hangs:

simulate open(FOO, "-|")

sub pipe_from_fork ($) {

  my $parent = shift;
  pipe $parent, my $child or die;
  my $pid = fork();
  die "fork() failed: $!" unless defined $pid;
  if ($pid) {
    close $child;
  }
  else {
    close $parent;
    open(STDOUT, ">&=" . fileno($child)) or die;
  }
$pid;

}

if (pipe_from_fork('BAR')) {

parent

while (<BAR>) { print; }

close BAR;

}
else {

  1. child print "pipe_from_fork\n"; exit(0);

}

The hang is not reproduced only with the MSYS Perl.

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 23062.Apr 1 2015, 9:06 AM
ayartsev retitled this revision from to [analyzer] Prevent ccc/c++-analyzer from hanging on Windows..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: jordan_rose.
ayartsev added a subscriber: Unknown Object (MLST).

+ added cfe-commits to the subscribers list.

jordan_rose edited edge metadata.Apr 23 2015, 8:52 AM

Doesn't this break if $CLANG has spaces in it?

ayartsev updated this revision to Diff 24666.Apr 29 2015, 4:57 PM
ayartsev edited edge metadata.

Fixed an issue with spaces. However additional changes (support for spaces in compiler arguments) are required to fully support spaces in a compiler path.
Giving --use-analyzer="F:/C LANG/clang.exe" to scan-build ends up with
error: error reading 'LANG\..\lib\clang\3.7.0'
That's because -resource-dir F:\C LANG\..\lib\clang\3.7.0 is given to clang.

http://reviews.llvm.org/D9357 has lacking changes that completely fix an issue.

The updated patch also eliminates the regression in case if clang crashes - now the proper value is rad from $?.

Please review!

I tried to apply it on the svn after applying D6551 but it fails with:

$ patch -p2  < D8774.diff 
patching file ccc-analyzer
patching file scan-build
Hunk #1 FAILED at 1232.
Hunk #2 succeeded at 1309 (offset 32 lines).
Hunk #3 succeeded at 1346 (offset 32 lines).
1 out of 3 hunks FAILED -- saving rejects to file scan-build.rej

Could you fix that? Thanks

ayartsev updated this revision to Diff 24887.May 4 2015, 9:36 AM

Updated the patch, tested that the patch successfully applies at ToT (r236425).
Let us forget about http://reviews.llvm.org/D6551 for a while until the current patch gets in. I recommend to get fresh 'scan-build' and 'ccc-analyzer' files from svn (I've slightly modified 'ccc-analyzer' at r236423) and apply the current patch over. Please test!

ayartsev updated this revision to Diff 24988.May 5 2015, 3:27 PM

Updated the patch (after changes introduced by r236533).

Sorry about the delay. I just tried again to build Firefox with your patch but it is breaking the build.
http://relman-ci.mozilla.org/job/firefox-scan-build/123/consoleFull

In file included from /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/toolkit/crashreporter/google-breakpad/src/common/Unified_cpp_src_common0.cpp:2:
In file included from /var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.cc:60:
/var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common/../common/logging.h:68:10: error: expected "FILENAME" or <FILENAME>
#include BP_LOGGING_INCLUDE
         ^
<command line>:1:28: note: expanded from here
#define BP_LOGGING_INCLUDE BreakpadLogging.h

Command line:

/usr/share/clang/scan-build-3.7/c++-analyzer -o host_arm_ex_reader.o -c -std=gnu++0x -MD -MP -MF .deps/host_arm_ex_reader.o.pp -DHAVE_A_OUT_H -I/var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common -I. -I/var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common/.. -I/var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common/../../../breakpad-logging -I../../../../../dist/include  -I/var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/dist/include/nspr /var/lib/jenkins/workspace/firefox-scan-build/toolkit/crashreporter/google-breakpad/src/common/arm_ex_reader.cc

I am not sure what is going on but a rebuild without your patch works

ayartsev updated this revision to Diff 27906.Jun 17 2015, 6:52 PM

Updated the patch.
The patch now includes the fix for D9357 that happen to be related to this issue. Perl 'system' and 'exec' routines process arguments differently depending on the perl port (tested with MSYS Perl, Dwimperl, Strawberry Perl and ActiveState Perl, got different results in terms of processing quotes and escaped characters) and on the OS (because arguments are passed to the system's command shell for parsing).
The patch replaces all 'exec' and 'system' calls with pipe form of 'open' that behaves consistently in all tested perl ports and operating systems (Windows 7, OS X, Ubuntu).
Tested with arguments like -DMACRO="\"'A=a' 'B=b'\"" and -DMACRO='"include.h"'.
Please review!

I am not sure what is going on but a rebuild without your patch works

Fixed issues, please test it once again.

I like the idea of simplifying all our commands, and of solving two problems at once! But I'm still a bit concerned…

tools/scan-build/ccc-analyzer
184–192

Does this escape all interesting characters? What about dollar signs and such?

Is it possible to use the multi-argument form of open on Windows now, or is that still not available?

ayartsev updated this revision to Diff 28522.Jun 25 2015, 6:33 PM

Updated the patch.
Now used list form of 'system' to invoke compilers. PerlDoc says: "On Windows, only the system PROGRAM LIST syntax will reliably avoid using the shell". Tested the solution on Windows 7, Ubuntu and OS X - all shell metacharacters and spaces are preserved. Please review!

Nice! Any reason why scan-build is still using open instead of system?

In scan-build we know exactly what arguments we pass to clang and piped 'open' is sufficient here. With 'system' we'd have to redirect streams to capture output.

jordan_rose accepted this revision.Jun 26 2015, 8:30 AM
jordan_rose edited edge metadata.

Okay, makes sense to me! Thanks for sticking with it this whole time.

This revision is now accepted and ready to land.Jun 26 2015, 8:30 AM

Sylvestre,

Would you be interested in testing this before it lands?

Yes, I am building it right now.
ayartsev, do you need me to commit it for you if it is green?

Anton has commit access.

sylvestre.ledru accepted this revision.Jun 26 2015, 11:08 AM
sylvestre.ledru added a reviewer: sylvestre.ledru.

OK, Good!

Works for me. Thanks for your patience Anton!

ayartsev closed this revision.Aug 25 2015, 2:22 PM

Committed as r241201