Eli, it looks like the problem is triggered as soon
as you have some output generated during the
`read_all_makefiles ()` phase, as it causes a call
to `prepare_mutex_handle_string ()`.

And after reading the Makefiles, `main ()` calls
`decode_env_switches ("MAKEFLAGS")` which in turn
calls `decode_switches ()` and the added flag
`--sync-mutex=0x44` is now present.

So in my example at the top of comment #3, the trigger
was the `perror_with_name ()`  call for the missing
no-Makefile file.

The problem indeed doesn't happen with simple
Makefiles where make doesn't generate any output
while reading the Makefiles.

Examples:

$ cat Makefile.bug
$(info Hello)
all:
        @echo All done


$ WinDebug/make.exe -f Makefile.bug -Otarget
Hello
make: *** internal error: multiple --sync-mutex options.  Stop.

$ cat Makefile.ok
all:
        $(info Hello)
        @echo All done
$ WinDebug/make.exe -f Makefile.ok -Otarget
Hello
All done

Of course, you have to be "lucky" and have that particular
memory layout I described in comment #3, i.e. the initial
xstrdup in `prepare_mutex_handle_string ()` must allocate
the "0x44" string just after the list itself. Otherwise
the memory overwrite that happens in `decode_switches ()`
is likely to remain asymptomatic. For example, when I launch
make from the debugger, the allocation pattern is different
and all seems fine, I could only figure this out by
attaching the debugger to the process...

Also, I just noticed that the problem is only reproducible
with 64-bits builds. I figure it might be difficult to
reproduce with other builds of make (msys, cygwin).

So the bug is likely to remain unnoticed most of the time,
which doesn't mean it's not there ;-)

Christian: Thanks for the extended explanations.

For the record: I don't see this on my system, at least not with the simple recipe you provided.  Moreover, the (second) call to `decode_switches`, after `prepare_mutex_handle_string`, never examines the --sync-mutex switch here.  And that is actually what I would expect, since the --sync-mutex switch is prepared for sub-Make, which is never launched in this case.

Moreover, the same pattern of code is used for the --jobserver-fds switch, so if there is a problem, it should show up on Unix as well.

But I trust Paul will fix this anyway.

Thanks.

Paul: If you already see and understand the problem, by all means take it.  I didn't yet have time to even build the release ;-).

Thanks.

Well, the symptom can even be reproduced without a Makefile...

$ /C/Workspace/src/git/make/WinDebug/make -Otarget -f no-Makefile
make: no-Makefile: No such file or directory
make: *** internal error: multiple --sync-mutex options.  Stop.

The reason for this "multiple --sync-mutex options" problem
can be traced to the memory handling bug I presented earlier.
I'll try to explain better what I saw while I was debugging
this issue.

The initial allocation of the sync_mutex stringlist which
happens in `prepare_mutex_handle_string ()` (main.c) leads
to the following memory layout at the address pointed to
by `sync_mutex->list`:

   +--------+
 0 |    x--------.
   +--------+    |
 1 |        |    |
   +--------+    |
 2 | "0xf4" |<---'
   +--------+

i.e. the blocks returned by the xmalloc and xstrdup
calls happen to be contiguous... So far, so good.

For reasons I didn't bother to check, the program then
enters `decode_switches ()` and one of these
switches is now "--sync-mutex 0xf4" (for example).
When handling this parameter in the `case string:`,
the code first tries to check if the stringlist
is not already allocated (it is), or if the list needs
to be expanded (that check is wrong). Here we have
idx == max == 1 for that list, so the expansion check
will wrongly decide all is fine and proceed with the
update code, which will make slot 1 point to "0xf4"
and null-terminate the list in the slot 2.

   +--------+
 0 |    x--------.
   +--------+    |
 1 |    x-----------.
   +--------+    |  |
 2 |      0 |<---'  |
   +--------+       v
                  "0xf4" (somewhere else in memory)

Now we have the slot 0 pointing to an empty value
(`(char*)0) and slot 1 pointing to "0xf4", and this
is what `decode_output_sync_flags ()` detects and
reports as an error.

Hope my previous comments now make better sense ;-)

(please ignore the first patch file #29339 which was
wrong; I believe the second patch file #29345 is correct,
as well as the alternative solution presented in comment #2
of changing the test operator from == to >=, in the expansion
check).

Second thought... I see now where the bug is coming from.

Some use cases for the stringlist imply it's handled as a null-terminated array:

  for (pp=output_sync_option->list; *pp; ++pp)

Some others iterate up to idx:

      for (idx = 1; idx < sync_mutex->idx; idx++)

For the second example, the list is initialized like this:

      sync_mutex->list = xmalloc (sizeof (char *));
      sync_mutex->list[0] = xstrdup (hdl_string);
      sync_mutex->idx = 1;
      sync_mutex->max = 1;

(copy/paste from the jobserver_fds code?)

The problem is that decode_switches() assumes the first use case, but when sync_mutex ends up there, the fact that idx == max prevents the list to expand as the (sl->idx == sl->max - 1)
condition is never met.

So how to fix this best? For consistency, perhaps always ensure
such a "list" null-terminated, and fix the initialization.
That would also be in accordance with the struct stringlist
comments. But there's also a simpler fix which would be to
change the expansion check from (sl->idx == sl->max - 1) to
(sl->idx >= sl->max - 1) so that it would also work for the
case when they're both set to 1.

Both solutions tested with success.


(file #29345)