Re: [bug #33138] .PARLLELSYNC enhancement with patch

[Frank Heckenbach]

Paul Smith wrote:

> On Thu, 2013-09-19 at 14:47 +0200, Frank Heckenbach wrote:
> > Paul Smith wrote:
> > 
> > > I didn't fix it this way.  Instead I used the existing MAKE_RESTART
> > > environment variable to communicate from the current make to the
> > > restarted make that the enter message was already printed (if it was) so
> > > it wouldn't be printed again.  This ensures we don't get a stream of
> > > enter/leave statements as we re-exec.
> > > 
> > > This works now (in my repo).

It works for me too. However, since MAKE_RESTARTS is a documented
variable, couldn't this change confuse user Makefiles?

> > 5.
> >
> > ISTM when output_dump() is called, output_context always ought to
> > be NULL (the call is either outside of any OUTPUT_SET/OUTPUT_UNSET
> > section, or (job.c:1274) child->output.syncout is NULL), is that
> > right?

I now see that's not the case ...

> > If so, the use of output_context might be slightly irritating
> > (though not wrong) -- at first I wondered where the
> > log_working_directory() output after the pump_from_tmp() calls was
> > going to and whether it didn't need pumping too if it was going to
> > the temp file, but apparently this never happens.

... which apparently does lead to a problem here (non-deterministic
like many "-j" problems):

% cat Makefile
all: a c

a: b
        $(shell true)

b:
        true

c:
        echo c

% make -j -Oline -sw
c
make: Entering directory 'foo'
make: Entering directory 'foo'
make: Leaving directory 'foo'
make: Leaving directory 'foo'

AIUI, it dumps out to stdout/stderr, but prints "Enter/Leave" to
output_context (which might be dumped much later), so out's contents
are not properly enclosed. Since we dump out to stdout/stderr (and
we got the semaphore for writing to those), it seems logical to me
to print "Enter/Leave" there as well, so this seems to fix it for me
(and again would obviate the need for the first parameter to
log_working_directory()):

--- output.c.orig       2013-09-24 19:45:35.000000000 +0200
+++ output.c    2013-09-24 19:45:50.000000000 +0200
@@ -387,7 +387,7 @@
 
       /* Log the working directory for this dump.  */
       if (print_directory_flag && output_sync != OUTPUT_SYNC_RECURSE)
-        traced = log_working_directory (output_context, 1);
+        traced = log_working_directory (NULL, 1);
 
       if (outfd_not_empty)
         pump_from_tmp (out->out, stdout);
@@ -395,7 +395,7 @@
         pump_from_tmp (out->err, stderr);
 
       if (traced)
-        log_working_directory (output_context, 0);
+        log_working_directory (NULL, 0);
 
       /* Exit the critical section.  */
       if (sem)

I'm afraid I found two new issues (continuing my numbering):

8.

Like job.c, I think function.c should check "output_context->err >= 0",
to avoid redirecting to -1 when no temp file for stderr was set up.

However, I wasn't able to really test it, because when starting make
with stderr closed, the first open() call (here, reading the
Makefile) would open it as fd 2 so "stderr" wasn't closed anymore by
the time setup_tmpfile() was called. This is, of course, perfectly
expected POSIX behaviour, and I don't think there's anything make
can or should do about it. So the whole STREAM_OK checking is
probably overkill since starting make (or just about any non-daemon)
with stdfoo closed is just a stupid thing to do. But now that we
have it, I think we should just keep it.

--- function.c.orig     2013-09-24 16:24:54.000000000 +0200
+++ function.c  2013-09-24 17:55:54.000000000 +0200
@@ -1725,7 +1725,7 @@
        setrlimit (RLIMIT_STACK, &stack_limit);
 #  endif
       child_execute_job (FD_STDIN, pipedes[1],
-                         output_context ? output_context->err : FD_STDERR,
+                         output_context && output_context->err >= 0 ? 
output_context->err : FD_STDERR,
                          command_argv, envp);
     }
   else

9.

In a complex case, I still get mismatched "Enter/Leave" messages.
I haven't been able to produce a small test case so far, but I did
find that the following statement (and the corresponding "Leave"
one) was executed in "-Oline" mode:

        stdio_traced = log_working_directory (NULL, 1);

That seems wrong per se, since it writes to stdout unsynchronized.
This must be due to the first part of this condition:

  if (! output_context || output_sync == OUTPUT_SYNC_RECURSE)

So I'd suggest to change this as follows. This would make sure no
unsynchronized "Enter/Leave" messages could be produced in
-Oline/-Otarget. Hopefully, after your last changes, there should be
no other unsynchronized output in theses modes anymore at all, but
even there is (which would be a bug), it might be better to avoid
those messages. Otherwise, in a parallel, recursive,
target/line-synched build, those unsynchronized messages may appear
in far away places, intermixed with messages from other directories
which is what seemed to happen in my case. After applying this
patch, I haven't seen this problem anymore.

--- output.c.orig       2013-09-24 20:25:14.000000000 +0200
+++ output.c    2013-09-24 20:25:21.000000000 +0200
@@ -583,7 +583,7 @@
     setup_tmpfile (output_context);
 #endif
 
-  if (! output_context || output_sync == OUTPUT_SYNC_RECURSE)
+  if (! output_sync || output_sync == OUTPUT_SYNC_RECURSE)
     {
       if (! stdio_traced && print_directory_flag)
         stdio_traced = log_working_directory (NULL, 1);