Re: [PATCH] Fix type errors in win32.

[Eli Zaretskii]

> From: Paul Smith 
> Cc: bug-make@gnu.org
> Date: Sun, 28 Nov 2021 15:19:15 -0500
> 
> On Mon, 2021-10-25 at 15:32 +0300, Eli Zaretskii wrote:
> > the default C runtime used by Make doesn't support %llu, at least not
> > on Windows versions older than Windows 10.
> 
> Hrm, I just added a new use of sprintf() with %lld into GNU make, to
> format a long long variable.  It worked for me in my trivial Windows
> tests but I am using Windows 10 with MSVC 2019 so pretty new.  I don't
> have the facilities to test older systems.

If you used MSVC 2019, you are not using the default MSVCRT.DLL
runtime.

> Should I be using %I64d instead on Windows, to allow support for older
> systems?

Yes, IMO.  You can also use PRIdMAX from inttypes.h for portability.

[bug #55242] Included Makefile not found, no rule to build it but make does not fail

[Dmitry Goncharov]

Follow-up Comment #4, bug #55242 (project make):

i agree that the makefile in question is incorrect.
i don't think the patch should be applied. i should not have provided the
patch to begin with.

i'd just gave the 'a.mk: b' rule a proper recipe which builds 'a.mk', rather
than an empty recipe.
With an empty recipe when 'b' is present, but 'a.mk' is missing, 'a.mk' won't
be created.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

[Dmitry Goncharov]

Follow-up Comment #22, bug #48643 (project make):

> If I add a note when we start the compatibility check, it shows up here
(obviously we'd pick a more informative note if we added this):
...
> I guess it's appropriate here but it seemed odd to me.

i also think, it is appropriate here.
Does the message seem better if it says e.g.


"No implicit rule found to build 'hello.o'.
Trying compatibility search due to explicit prerequisite 'hello.tsk'."

?

> I wonder how many existing build systems would trigger this note?

When this behavior was modified in 2009 there were 5 bug reports listed in
https://savannah.gnu.org/bugs/?17752.

i am less concerned that existing makefiles, which rely on compatibility
search, trigger this new message.

My main concern is the makefiles which never relied on the old definition of
ought-to-exist. Those makefile (or brand new ones) can accidentally fall
victims of compatibility search, because compatibility search is enabled by
default and it is silent.

When i maintain a makefile, i definitely do not want to depend on
compatibility search and would appreciate if make gave me a hint whenever such
dependency is introduced.

Such a message could be enabled by default.

Alternatively, we can introduce a flag e.g. '--warn-compatibility-search' or
'--disable-compatibility-search'.
i like 'disable' better than 'warn', because 'disable' would cause make to
exit with an error on the 1st failed prerequisite. 'Warn', on the other hand,
would cause make to print the same message multiple times (once for every
prerequisite).
i also suspect, that most users won't enable such flag, should we add one.


___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Liviu Ionescu]

Follow-up Comment #6, bug #61409 (project make):

Now I have a slightly better understanding of the problem, although I had no
time to experiment anything yet.

The real problem is in Microsoft UCRT, which has a bug in the code managing
the environment.

Unfortunately, even if Microsoft will fix the bug tomorrow, we still have to
deal with millions of Windows instances which exhibit the bug. And in this
respect, all programs that spawn children processes should consider
workarounds, including GNU make.

If my understanding is correct (based on the explanations I received from
someone much more knowledgeable than myself), the problem is related to the
way the environment is understood by Microsoft. 

---
Internally, the "environment" in a Windows process consists of a little more
than what you see in the standard C envp array. If you run
GetEnvironmentStringsA/W and iterate over it, you'll see the normal
environment variables, but also a few other ones, like these:

=::=::\
=C:=C:\code\ms-spawn-issue\sources\spawn
=ExitCode=

These are special vars that start with an '=' - apparently the include
information about things like which directory is the "current directory" on
each drive letter.

When UCRT's spawn() tries to create a process, it wants to include these extra
variables from the current environment (from GetEnvironmentStringsA/W) on top
of the variables you provide in the 'envp' parameter.

When make starts its subprocesses, it does that by calling CreateProcess() and
passing in a string with the desired environment, but it doesn't know/care
about this kind of extra variables. So processes launched by make are missing
these extra variables in GetEnvironmentStringsA/W.

When launching a process with CreateProcess() and passing NULL to the
environment parameter, or passing NULL to the envp parameter to spawn, there's
no issue (the current exact environment is passed on). But if UCRT's spawn is
called in an environment that is missing these extra variables, it crashes.
---

So, if make creates sub-processes with a custom environment (i.e. does not
pass NULL to inherit the parent), those processes cannot use UCRT to create
sub-processes.

The same applies to sh.exe which spawns, for example, the compiler, to the
compiler who spawns the internal steps, and so on.

The workaround is relatively simple. The only known way that is guaranteed to
work is to pass a NULL environment, which means to inherit the environment
from the parent.

This means that instead of creating a custom environment, the parent must
temporarily adjust its own environment to match the desired one, spawn the
process, and restore the initial environment.

I did not check the code used by make, but if it passes a custom environment
and not a NULL, its children are affected by this Windows bug.

Regarding the question related to UCRT vs MSVCRT, the use of MSVCRT was
deprecated about 10 years ago when UCRT was introduced. mingw-w64 already
implements UCRT for some time, and it is very likely to become the default in
future mingw-w64 releases, since the MSVCRT code has many limitations and is
no longer maintained. All recent binary toolchain distributions (both clang
and gcc) produce binaries the use UCRT.

Thus the suggestion to stick to MSVCRT is not realistic.

Sure, you can close the issue, but this will not solve the problem.



___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61042] Enhance implicit rule search logging

[Dmitry Goncharov]

Follow-up Comment #12, bug #61042 (project make):

> I used it because I wanted to mark that data member as being "private" in
the sense that it shouldn't be used directly by other parts of the code;
instead the get_rule_defn() function should always be called to retrieve that
value.

Good to know. i'll follow this convention.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #55242] Included Makefile not found, no rule to build it but make does not fail

[Paul D. Smith]

Follow-up Comment #3, bug #55242 (project make):

I tried these patches (some updates were needed as remake.c has changed
slightly).  They do fix the original issue but I thought of a new test to
add:

# Allow included files to be updated as side-effects of prereqs.  Here a.mk
# already exists, but b (which is rebuilt) updates it so we should re-exec.

touch('a.mk');
unlink('b');

run_make_test(q!
all:; @echo hello=$(hello)
include a.mk
a.mk: b
b: ; @echo hello=world >a.mk
!, '', "touch b\nhello=world");


This does not pass; even though a.mk was rebuilt we don't re-exec because a.mk
was pre-existing, we don't look to see if it's timestamp has changed.

We could continue on to fix this too, but I'm not convinced we should do
that.

In general, make doesn't actually check timestamps of targets unless make
believes that they've been updated.  I'm not sure that it's correct to break
this general rule and do something unique and special for rebuilt makefiles in
particular (or, I guess, this patch does something special for goal targets).

The way to "correct" the original makefile provided in the bug is the same as
when you run into this situation in any other target which is not an included
makefile: it is to add a recipe to the *a.mk* rule.  The recipe can be a "do
nothing" empty recipe: as long as make sees there is SOME recipe then it will
consider the target updated.

So for example, instead of this:

all:; @echo hello=$(hello)
include a.mk
a.mk: b
b: ; @echo hello=world >a.mk

if you added a semicolon (empty recipe) to *a.mk* like this:

all:; @echo hello=$(hello)
include a.mk
a.mk: b ;
b: ; @echo hello=world >a.mk

then it would work fine.

My inclination is to close this as "Not a Bug".

Comments welcome.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

[Paul D. Smith]

Follow-up Comment #21, bug #48643 (project make):

I thought about this but adding such a note caused it to turn up in somewhat
unusual places.  For example in this test:

all: hello.tsk
%.tsk: %.o; $(info hello.tsk)
%.o: %.c; $(info hello.c)
%.o: %.f %.tsk; $(info hello.f)

we expect to get this output:

  make: Circular hello.o <- hello.tsk dependency dropped.
  hello.f
  hello.tsk


If I add a note when we start the compatibility check, it shows up here
(obviously we'd pick a more informative note if we added this):

  Starting compatibility search
  make: Circular hello.o <- hello.tsk dependency dropped.
  hello.f
  hello.tsk


I guess it's appropriate here but it seemed odd to me.

We should consider it.  I wonder how many existing build systems would trigger
this note?  That would be an interesting statistic.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61042] Enhance implicit rule search logging

[Paul D. Smith]

Follow-up Comment #11, bug #61042 (project make):

I used it because I wanted to mark that data member as being "private" in the
sense that it shouldn't be used directly by other parts of the code; instead
the get_rule_defn() function should always be called to retrieve that value.

If we were using C++ many things would be a lot cleaner :).

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61042] Enhance implicit rule search logging

[Dmitry Goncharov]

Follow-up Comment #10, bug #61042 (project make):

Thank you, Paul.

Does the leading underscore in rule->_defn has a special meaning, follow gnu
make convention or similar?

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Paul D. Smith]

Follow-up Comment #5, bug #61409 (project make):

Reading the busybox issue thread here
https://github.com/rmyorston/busybox-w32/issues/234 it _appears_ that the
problem really is not related to GNU make, although I can't understand why the
failure is only seen if the program is invoked from GNU make and not from
other tools or directly from the shell.

In any event, I'm tempted to close this issue.  @ilg please comment.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

[Dmitry Goncharov]

Follow-up Comment #20, bug #48643 (project make):

Thank you, Paul.

The user may not realize that their makefile relies on compatibility search
and the related performance penalty.
Do you think it is appropriate to print a message when make begins
compatibility search to let the user know?

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

Re: [PATCH] Fix type errors in win32.

[Paul Smith]

On Mon, 2021-10-25 at 15:32 +0300, Eli Zaretskii wrote:
> the default C runtime used by Make doesn't support %llu, at least not
> on Windows versions older than Windows 10.

Hrm, I just added a new use of sprintf() with %lld into GNU make, to
format a long long variable.  It worked for me in my trivial Windows
tests but I am using Windows 10 with MSVC 2019 so pretty new.  I don't
have the facilities to test older systems.

Should I be using %I64d instead on Windows, to allow support for older
systems?

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Eli Zaretskii]

Follow-up Comment #4, bug #61409 (project make):

If Make invokes other programs, and those programs have problems because the
use UCRT, how is that a problem of Make?

What I meant by my comment about MinGW was that as long as Make doesn't use
the 'spawnve' implementation from UCRT, Make itself is not directly affected
by that problem.


___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Paul D. Smith]

Follow-up Comment #3, bug #61409 (project make):

I agree that GNU make shouldn't / can't fix a problem in the MS runtime.  The
question is, whether there's a way GNU make could work around or avoid the
problem (and how annoying that would be).

I'm not sure I understand the comment that as long as MinGW doesn't use UCRT,
it's a theoretical problem; surely GNU make on Windows is used to invoke other
kinds of programs, that aren't built with MinGW?  Or did I misunderstand what
you meant?

Liviu: you didn't provide any information on how you compiled GNU make; can
you specify?  Did you use MSVC with the default build_w32.bat file?  Did you
use GCC with build_w32.bat?  Something else?  Maybe that doesn't matter WRT
this issue.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Eli Zaretskii]

Follow-up Comment #2, bug #61409 (project make):

AFAIU, this problem only happens when using the UCRT runtime, which is not
what MinGW uses.  So, as long as MinGW doesn't use UCRT, this is largely a
theoretical problem for GNU Make on Windows.  If and when MinGW64 will start
using UCRT as its runtime, the problem should be reported to them, so that
they provide some workaround.

IOW, I don't think it should be the GNU Make's business to fix a problem in
the MS runtime.


___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #61409] The code used to create sub-processes on Windows triggers the spawnve() issue

[Paul D. Smith]

Follow-up Comment #1, bug #61409 (project make):

Unfortunately I have no experience in Windows.  Someone who has such
experience will need to track this down and figure out what's going wrong.  I
don't know if Eli has time or energy for this or not.

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

[Paul D. Smith]

Update of bug #48643 (project make):

  Status:None => Fixed  
 Open/Closed:Open => Closed 

___

Follow-up Comment #19:

I pushed Dmitry's fixes and new tests, plus added changes to the GNU make
manual to describe the new algorithm.

Thanks!

___

Reply to this item at:

  

___
  Message sent via Savannah
  https://savannah.gnu.org/

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Paul Smith]

On Sun, 2021-11-28 at 15:49 +0100, Jouke Witteveen wrote:
> I fully agree, but was not aware of the robustness of INTSTR_LENGTH.
> It felt a bit fragile when I spotted its definition in makeint.h.

The C standard defines the largest unsigned long long value
as 18446744073709551615, the largest signed long long value
as 9223372036854775807, and the smallest signed long long value as -
9223372036854775808.  So, the definition cannot be wrong in any
standards-conforming implementation of C.

> File sizes are an interesting application indeed. If you want me to
> change the patches to use strtoll, I would need some help since I am
> not sure how to set things up so that we get a fallback
> implementation on platforms where it is missing.

I already did it locally, thanks for the offer.  I enhanced the tests
to check that the above (signed) values are parsed correctly.

I just changed the gnulib import of "strtol" to "strtoll".  strtoll()
is required by the C99 standard (which we were forced to require in GNU
make as well, since gnulib requires it) so it's pretty much everywhere
these days.  There are a few older platforms where it has incorrect
implementations that gnulib will take care of for us.

Cheers!

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Jouke Witteveen]

On Sun, Nov 28, 2021 at 3:20 PM Paul Smith  wrote:
>
> On Sun, 2021-11-28 at 14:57 +0100, Jouke Witteveen wrote:
> > > Since the two arguments are equal, it doesn't matter which of LHS
> > > or RHS we return.
> >
> > They could differ for instance when one of them contains a '+'-sign.
> > My reason for using LHS is that we already have a string for it.
>
> I don't think that it's necessary return the exact string.  If the user
> wanted a string match they can do that other ways.  Returning the
> "absolute value" (stripping leading +/-, leading 0's, etc.) seems more
> useful to me.

I fully agree, but was not aware of the robustness of INTSTR_LENGTH.
It felt a bit fragile when I spotted its definition in makeint.h.

> > > However, now that I think about it I need to change the code more: we
> > > need to be using "long long" here not just "long".  While on Linux etc.
> > > a "long" is 8 bytes, on Windows "long" is only 4 bytes.
> >
> > I was hoping this would not be necessary, and I cannot think of a
> > typical use case where make is a good fit for dealing with large
> > integers. The benefit of "long" is that strtol is more widely
> > available than strtoll.
>
> I see what you mean, but I _really_ don't like the idea of GNU make
> working differently on different platforms, even if such use cases are
> rare.  I can imagine a situation where, for example, someone wants to
> compare the sizes of files and if one of the files is >4G then it will
> work on Linux and fail on Windows.

File sizes are an interesting application indeed. If you want me to
change the patches to use strtoll, I would need some help since I am
not sure how to set things up so that we get a fallback implementation
on platforms where it is missing.

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Jouke Witteveen]

On Sun, Nov 28, 2021 at 3:33 PM Paul Smith  wrote:
>
> On Sun, 2021-11-28 at 08:24 +0100, Jouke Witteveen wrote:
> > On the user side, strcmp could now probably be defined something like
> > $(and $(intcmp $(words $1),$(words $2)),$(findstring x $1 x,x $2 x))
>
> I don't think this is equivalent since a putative strcmp would also do
> greater / less than comparison (like intcmp does).  Of course that
> leads into all sorts of i18n / locale issues, but likely we'd just punt
> (like C's strcmp does) and use ASCII ordering; we've already taken that
> route elsewhere.

This was only intended as a demonstration of how the two-argument
strcmp function could be implemented already (I initially forgot the
spaces around the 'x's). This would serve as a test for exact
string-wise equality.

If anything, this could mean that there is no reason to spend time on
adding strcmp now. It's all a digression and I agree with everything
you said about it.

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Paul Smith]

On Sun, 2021-11-28 at 08:24 +0100, Jouke Witteveen wrote:
> On the user side, strcmp could now probably be defined something like
> $(and $(intcmp $(words $1),$(words $2)),$(findstring x$1x,x$2x))

I don't think this is equivalent since a putative strcmp would also do
greater / less than comparison (like intcmp does).  Of course that
leads into all sorts of i18n / locale issues, but likely we'd just punt
(like C's strcmp does) and use ASCII ordering; we've already taken that
route elsewhere.

You can probably gin up something that works using $(sort ...) but it
seems tricky.  Anyway I'm not suggesting this be added right now but
just keeping options open for the future.

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Paul Smith]

On Sun, 2021-11-28 at 14:57 +0100, Jouke Witteveen wrote:
> > Since the two arguments are equal, it doesn't matter which of LHS
> > or RHS we return.
> 
> They could differ for instance when one of them contains a '+'-sign.
> My reason for using LHS is that we already have a string for it.

I don't think that it's necessary return the exact string.  If the user
wanted a string match they can do that other ways.  Returning the
"absolute value" (stripping leading +/-, leading 0's, etc.) seems more
useful to me.

>  By using sprintf, we need to make the buffer big enough, which in
> turn requires INTSTR_LENGTH to be fitting for whatever width a long
> has on the current platform.

That's already the case; GNU make defines this constant to be large
enough to hold the largest possible (8-byte) integer value, regardless
of platform.  This is used in various places when showing output etc.

> > However, now that I think about it I need to change the code more: we
> > need to be using "long long" here not just "long".  While on Linux etc.
> > a "long" is 8 bytes, on Windows "long" is only 4 bytes.
> 
> I was hoping this would not be necessary, and I cannot think of a
> typical use case where make is a good fit for dealing with large
> integers. The benefit of "long" is that strtol is more widely
> available than strtoll.

I see what you mean, but I _really_ don't like the idea of GNU make
working differently on different platforms, even if such use cases are
rare.  I can imagine a situation where, for example, someone wants to
compare the sizes of files and if one of the files is >4G then it will
work on Linux and fail on Windows.

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Jouke Witteveen]

On Sun, Nov 28, 2021 at 2:45 PM Paul Smith  wrote:
>
> On Sun, 2021-11-28 at 08:24 +0100, Jouke Witteveen wrote:
> > Thanks for sending this message, I would have otherwise prepared and
> > sent an updated patch series today. My plan was to expand to RHS in
> > the two-argument case if both values are equal. I assume you also
> > updated the documentation where needed? If so, there's nothing I have
> > to add and I'm looking forward to the new functionality.
>
> Yes, I updated the docs.
>
> Since the two arguments are equal, it doesn't matter which of LHS or
> RHS we return.

They could differ for instance when one of them contains a '+'-sign.
My reason for using LHS is that we already have a string for it. By
using sprintf, we need to make the buffer big enough, which in turn
requires INTSTR_LENGTH to be fitting for whatever width a long has on
the current platform.

> The change I made was:
>
>   argv += 2;
>
>   if (*argv == NULL)
> {
>   if (lhs == rhs)
> {
>   char buf[INTSTR_LENGTH+1];
>   sprintf (buf, "%ld", lhs);
>   o = variable_buffer_output(o, buf, strlen (buf));
> }
>   return o;
> }
>
> However, now that I think about it I need to change the code more: we
> need to be using "long long" here not just "long".  While on Linux etc.
> a "long" is 8 bytes, on Windows "long" is only 4 bytes.

I was hoping this would not be necessary, and I cannot think of a
typical use case where make is a good fit for dealing with large
integers. The benefit of "long" is that strtol is more widely
available than strtoll.

Regards,
- Jouke

Re: [PATCH 3/3] Introduce $(compare ...) for numerical comparison

[Paul Smith]

On Sun, 2021-11-28 at 08:24 +0100, Jouke Witteveen wrote:
> Thanks for sending this message, I would have otherwise prepared and
> sent an updated patch series today. My plan was to expand to RHS in
> the two-argument case if both values are equal. I assume you also
> updated the documentation where needed? If so, there's nothing I have
> to add and I'm looking forward to the new functionality.

Yes, I updated the docs.

Since the two arguments are equal, it doesn't matter which of LHS or
RHS we return.

The change I made was:

  argv += 2;

  if (*argv == NULL)
{
  if (lhs == rhs)
{
  char buf[INTSTR_LENGTH+1];
  sprintf (buf, "%ld", lhs);
  o = variable_buffer_output(o, buf, strlen (buf));
}
  return o;
}

However, now that I think about it I need to change the code more: we
need to be using "long long" here not just "long".  While on Linux etc.
a "long" is 8 bytes, on Windows "long" is only 4 bytes.