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

[ayartsev]

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.

[ayartsev]

+ added cfe-commits to the subscribers list.

[ayartsev]

Ping.

[jordan_rose]

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

[ayartsev]

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!

[sylvestre.ledru]

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 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 the patch (after changes introduced by r236533).

[sylvestre.ledru]

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 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!

In D8774#187749, @sylvestre.ledru wrote:

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

Fixed issues, please test it once again.

[jordan_rose]

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

160–168

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 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!

[jordan_rose]

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

[ayartsev]

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]

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

[zaks.anna]

Sylvestre,

Would you be interested in testing this before it lands?

[sylvestre.ledru]

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

[zaks.anna]

Anton has commit access.

[sylvestre.ledru]

OK, Good!

Works for me. Thanks for your patience Anton!

[ayartsev]

Committed as r241201

tools/scan-build/ccc-analyzer

elsif ($ErrorType eq $OtherError) {
     $prefix = "clang_other_error";
   }
 
   # Generate the preprocessed file with Clang.
   my ($PPH, $PPFile) = tempfile( $prefix . "_XXXXXX",
                                  SUFFIX => GetPPExt($Lang),
                                  DIR => $Dir);
   close ($PPH);
-  system $Clang, @$Args, "-E", "-o", $PPFile;
+  my $ExecLine = join(' ', qq/"$Clang"/, @{PrepareArgs($Args)}, "-E", "-o", $PPFile, "2>&1", "|");
+  open(PS, $ExecLine);
+  while ( <PS> ) {}
+  close PS;
 
   # Create the info file.
   open (OUT, ">", "$PPFile.info.txt") or die "Cannot open $PPFile.info.txt\n";
   print OUT abs_path($file), "\n";
   print OUT "$ErrorType\n";
   print OUT "@$Args\n";
   close OUT;
   `uname -a >> $PPFile.info.txt 2>&1`;
-  `$Compiler -v >> $PPFile.info.txt 2>&1`;
+  `"$Compiler" -v >> $PPFile.info.txt 2>&1`;
   rename($ofile, "$PPFile.stderr.txt");
   return (basename $PPFile);
 }
 
 ##----------------------------------------------------------------------------##
 #  Running the analyzer.
 ##----------------------------------------------------------------------------##
 
+sub PrepareArgs {
+  my $Args = shift;
+  my @TempArgs = @$Args;
+  foreach (@TempArgs) {
+    $_ =~ s/"/\\"/g;
+    $_ = qq/"$_"/;
+  }
+  return \@TempArgs;
+}
+
 sub GetCCArgs {
   my $mode = shift;
   my $Args = shift;
 
-  pipe (FROM_CHILD, TO_PARENT);
-  my $pid = fork();
-  if ($pid == 0) {
-    close FROM_CHILD;
-    open(STDOUT,">&", \*TO_PARENT);
-    open(STDERR,">&", \*TO_PARENT);
-    exec $Clang, "-###", $mode, @$Args;
-  }
-  close(TO_PARENT);
   my $line;
-  while (<FROM_CHILD>) {
+  my $ExecLine = join(' ', qq/"$Clang"/, "-###", $mode, @{PrepareArgs($Args)}, "2>&1", "|");
+  open(PS, $ExecLine);
+  while ( <PS> ) {
     next if (!/\s"?-cc1"?\s/);
     $line = $_;
   }
-
-  waitpid($pid,0);
-  close(FROM_CHILD);
+  close PS;
 
   die "could not find clang line\n" if (!defined $line);
   # Strip leading and trailing whitespace characters.
   $line =~ s/^\s+|\s+$//g;
   my @items = quotewords('\s+', 0, $line);
   my $cmd = shift @items;
   die "cannot find 'clang' in 'clang' command\n" if (!($cmd =~ /clang/));
   return \@items;
if ($Verbose == 1) {
     # gcc for various purposes.
     print STDERR join(' ', @PrintArgs);
     print STDERR "\n";
   }
   elsif ($Verbose == 2) {
     print STDERR "#SHELL (cd '$dir' && @PrintArgs)\n";
   }
 
-  # Capture the STDERR of clang and send it to a temporary file.
-  # Capture the STDOUT of clang and reroute it to ccc-analyzer's STDERR.
+  # Save STDOUT and STDERR of clang to a temporary file and reroute
+  # all clang output to ccc-analyzer's STDERR.
   # We save the output file in the 'crashes' directory if clang encounters
   # any problems with the file.
-  pipe (FROM_CHILD, TO_PARENT);
-  my $pid = fork();
-  if ($pid == 0) {
-    close FROM_CHILD;
-    open(STDOUT,">&", \*TO_PARENT);
-    open(STDERR,">&", \*TO_PARENT);
-    exec $Cmd, @CmdArgs;
-  }
-
-  close TO_PARENT;
   my ($ofh, $ofile) = tempfile("clang_output_XXXXXX", DIR => $HtmlDir);
 
-  while (<FROM_CHILD>) {
+  my $ExecLine = join(' ', qq/"$Cmd"/, @{PrepareArgs(\@CmdArgs)}, "2>&1", "|");
+  open(PS, $ExecLine);
+  while ( <PS> ) {
     print $ofh $_;
     print STDERR $_;
   }
-  close $ofh;
-
-  waitpid($pid,0);
-  close(FROM_CHILD);
+  close PS;
   my $Result = $?;
+  close $ofh;
 
   # Did the command die because of a signal?
   if ($ReportFailures) {
     if ($Result & 127 and $Cmd eq $Clang and defined $HtmlDir) {
       ProcessClangFailure($Clang, $Lang, $file, \@CmdArgsSansAnalyses,
                           $HtmlDir, "Crash", $ofile);
     }
     elsif ($Result) {

 my @CompileOpts;
 my @LinkOpts;
 my @Files;
 my $Lang;
 my $Output;
 my %Uniqued;
 
 # Forward arguments to gcc.
-my $Status = system($Compiler,@ARGV);
+{
+  my $PreparedARGV = PrepareArgs(\@ARGV);
+  my $ExecLine = join(' ', qq/"$Compiler"/, @{$PreparedARGV}, "|");
+  open(PS, $ExecLine);
+  while ( <PS> ) {
+    print $_;
+  }
+  close PS;
+  my $Status = $?;
+
   if (defined $ENV{'CCC_ANALYZER_LOG'}) {
-  print STDERR "$Compiler @ARGV\n";
+    print STDERR "$Compiler @{$PreparedARGV}\n";
   }
   if ($Status) { exit($Status >> 8); }
+}
 
 # Get the analysis options.
 my $Analyses = $ENV{'CCC_ANALYZER_ANALYSIS'};
 
 # Get the plugins to load.
 my $Plugins = $ENV{'CCC_ANALYZER_PLUGINS'};
 
 # Get the store model.

tools/scan-build/scan-build

ENDTEXT
   # argument
   my @PluginLoadCommandline_xclang;
   foreach my $param ( @PluginsToLoad ) {
     push ( @PluginLoadCommandline_xclang, "-Xclang" );
     push ( @PluginLoadCommandline_xclang, $param );
   }
   my %EnabledCheckers;
   foreach my $lang ("c", "objective-c", "objective-c++", "c++") {
-    pipe(FROM_CHILD, TO_PARENT);
-    my $pid = fork();
-    if ($pid == 0) {
-      close FROM_CHILD;
-      open(STDOUT,">&", \*TO_PARENT);
-      open(STDERR,">&", \*TO_PARENT);
-      exec $Clang, ( @PluginLoadCommandline_xclang, '--analyze', '-x', $lang, '-', '-###');
-    }
-    close(TO_PARENT);
-    while(<FROM_CHILD>) {
+    my $ExecLine = join(' ', qq/"$Clang"/, @PluginLoadCommandline_xclang, "--analyze", "-x", $lang, "-", "-###", "2>&1", "|");
+    open(PS, $ExecLine);
+    while (<PS>) {
       foreach my $val (split /\s+/) {
         $val =~ s/\"//g;
         if ($val =~ /-analyzer-checker\=([^\s]+)/) {
           $EnabledCheckers{$1} = 1;
         }
       }
     }
-    waitpid($pid,0);
-    close(FROM_CHILD);
   }
 
   # Query clang for complete list of checkers.
   if (defined $Clang && -x $Clang) {
-    pipe(FROM_CHILD, TO_PARENT);
-    my $pid = fork();
-    if ($pid == 0) {
-      close FROM_CHILD;
-      open(STDOUT,">&", \*TO_PARENT);
-      open(STDERR,">&", \*TO_PARENT);
-      exec $Clang, ('-cc1', @PluginsToLoad , '-analyzer-checker-help');
-    }
-    close(TO_PARENT);
+    my $ExecLine = join(' ', qq/"$Clang"/, "-cc1", @PluginsToLoad, "-analyzer-checker-help", "2>&1", "|");
+    open(PS, $ExecLine);
     my $foundCheckers = 0;
-    while(<FROM_CHILD>) {
+    while (<PS>) {
       if (/CHECKERS:/) {
         $foundCheckers = 1;
         last;
       }
     }
     if (!$foundCheckers) {
       print "  *** Could not query Clang for the list of available checkers.";
     }
     else {
       print("\nAVAILABLE CHECKERS:\n\n");
       my $skip = 0;
-      while(<FROM_CHILD>) {
+       while(<PS>) {
         if (/experimental/) {
           $skip = 1;
           next;
         }
         if ($skip) {
           next if (!/^\s\s[^\s]/);
           $skip = 0;
         }
else {
             print "   ";
           }
         }
         else {
           print "   ";
         }
         print $_;
       }
-      print "\nNOTE: \"+\" indicates that an analysis is enabled by default.\n"
+      print "\nNOTE: \"+\" indicates that an analysis is enabled by default.\n";
     }
-    waitpid($pid,0);
-    close(FROM_CHILD);
+    close PS;
   }
 
 print <<ENDTEXT
 
 BUILD OPTIONS
 
  You can specify any build option acceptable to the build command.