* [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
@ 2024-03-26 22:36 Peter Damianov
2024-03-26 22:43 ` Peter0x44
2024-03-27 7:51 ` Richard Biener
0 siblings, 2 replies; 9+ messages in thread
From: Peter Damianov @ 2024-03-26 22:36 UTC (permalink / raw)
To: gcc-patches; +Cc: Peter Damianov
lto-wrapper generates Makefiles that use the following:
touch -r file file.tmp && mv file.tmp file
to truncate files.
If there is no suitable "touch" or "mv" available, then this errors with
"The system cannot find the file specified".
The solution here is to check if sh -c true works, before trying to use it in
the Makefile. If it doesn't, then fall back to "copy /y nul file" instead.
The fallback doesn't work exactly the same (the modified time of the file gets
updated), but this doesn't seem to matter in practice.
I tested this both in environments both with and without sh present, and
observed no issues.
Signed-off-by: Peter Damianov <peter0x44@disroot.org>
---
gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
index 5186d040ce0..8dee0aaa2d8 100644
--- a/gcc/lto-wrapper.cc
+++ b/gcc/lto-wrapper.cc
@@ -1389,6 +1389,27 @@ make_exists (void)
return errmsg == NULL && exit_status == 0 && err == 0;
}
+/* Test that an sh command is present and working, return true if so.
+ This is only relevant for windows hosts, where a /bin/sh shell cannot
+ be assumed to exist. */
+
+static bool
+sh_exists (void)
+{
+ const char *sh = "sh";
+ const char *sh_args[] = {sh, "-c", "true", NULL};
+#ifdef _WIN32
+ int exit_status = 0;
+ int err = 0;
+ const char *errmsg
+ = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
+ "sh", NULL, NULL, &exit_status, &err);
+ return errmsg == NULL && exit_status == 0 && err == 0;
+#else
+ return true;
+#endif
+}
+
/* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */
static void
@@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
const char *collect_gcc;
char *collect_gcc_options;
int parallel = 0;
+ bool have_sh = sh_exists ();
int jobserver = 0;
bool jobserver_requested = false;
int auto_parallel = 0;
@@ -2016,6 +2038,7 @@ cont:
argv_ptr[5] = NULL;
if (parallel)
{
+ fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
fprintf (mstream, "%s:\n\t@%s ", output_name, new_argv[0]);
for (j = 1; new_argv[j] != NULL; ++j)
fprintf (mstream, " '%s'", new_argv[j]);
@@ -2024,9 +2047,15 @@ cont:
truncate them as soon as we have processed it. This
reduces temporary disk-space usage. */
if (! save_temps)
- fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
- "2>&1 && mv \"%s.tem\" \"%s\"\n",
- input_name, input_name, input_name, input_name);
+ {
+ fprintf (mstream,
+ have_sh
+ ? "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
+ "2>&1 && mv \"%s.tem\" \"%s\"\n"
+ : "\t@-copy /y nul \"%s\" > NUL "
+ "2>&1\n",
+ input_name, input_name, input_name, input_name);
+ }
}
else
{
--
2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-26 22:36 [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710] Peter Damianov
@ 2024-03-26 22:43 ` Peter0x44
2024-03-27 7:51 ` Richard Biener
1 sibling, 0 replies; 9+ messages in thread
From: Peter0x44 @ 2024-03-26 22:43 UTC (permalink / raw)
To: gcc-patches
26 Mar 2024 10:36:45 pm Peter Damianov <peter0x44@disroot.org>:
> lto-wrapper generates Makefiles that use the following:
> touch -r file file.tmp && mv file.tmp file
> to truncate files.
> If there is no suitable "touch" or "mv" available, then this errors
> with
> "The system cannot find the file specified".
>
> The solution here is to check if sh -c true works, before trying to use
> it in
> the Makefile. If it doesn't, then fall back to "copy /y nul file"
> instead.
> The fallback doesn't work exactly the same (the modified time of the
> file gets
> updated), but this doesn't seem to matter in practice.
>
> I tested this both in environments both with and without sh present,
> and
> observed no issues.
>
> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> ---
> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
> index 5186d040ce0..8dee0aaa2d8 100644
> --- a/gcc/lto-wrapper.cc
> +++ b/gcc/lto-wrapper.cc
> @@ -1389,6 +1389,27 @@ make_exists (void)
> return errmsg == NULL && exit_status == 0 && err == 0;
> }
>
> +/* Test that an sh command is present and working, return true if so.
> + This is only relevant for windows hosts, where a /bin/sh shell
> cannot
> + be assumed to exist. */
> +
> +static bool
> +sh_exists (void)
> +{
> + const char *sh = "sh";
> + const char *sh_args[] = {sh, "-c", "true", NULL};
> +#ifdef _WIN32
> + int exit_status = 0;
> + int err = 0;
> + const char *errmsg
> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
> + "sh", NULL, NULL, &exit_status, &err);
> + return errmsg == NULL && exit_status == 0 && err == 0;
> +#else
> + return true;
> +#endif
> +}
> +
> /* Execute gcc. ARGC is the number of arguments. ARGV contains the
> arguments. */
>
> static void
> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
> const char *collect_gcc;
> char *collect_gcc_options;
> int parallel = 0;
> + bool have_sh = sh_exists ();
> int jobserver = 0;
> bool jobserver_requested = false;
> int auto_parallel = 0;
> @@ -2016,6 +2038,7 @@ cont:
> argv_ptr[5] = NULL;
> if (parallel)
> {
> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
> fprintf (mstream, "%s:\n\t@%s ", output_name, new_argv[0]);
> for (j = 1; new_argv[j] != NULL; ++j)
> fprintf (mstream, " '%s'", new_argv[j]);
> @@ -2024,9 +2047,15 @@ cont:
> truncate them as soon as we have processed it. This
> reduces temporary disk-space usage. */
> if (! save_temps)
> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
> - input_name, input_name, input_name, input_name);
> + {
> + fprintf (mstream,
> + have_sh
> + ? "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
> + : "\t@-copy /y nul \"%s\" > NUL "
> + "2>&1\n",
> + input_name, input_name, input_name, input_name);
> + }
> }
> else
> {
> --
> 2.39.2
I made this patch against gcc 13.2, because I couldn't get gcc 14 to
build.
I got the following errors:
In file included from ../../../../gcc/libgcc/config/i386/cpuinfo.c:30:
../../../../gcc/libgcc/../gcc/common/config/i386/cpuinfo.h: In function
'get_available_features':
../../../../gcc/libgcc/../gcc/common/config/i386/cpuinfo.h:938:21: error:
'bit_USER_MSR' undeclared (first use in this function)
938 | if (edx & bit_USER_MSR)
| ^~~~~~~~~~~~
../../../../gcc/libgcc/../gcc/common/config/i386/cpuinfo.h:938:21: note:
each undeclared identifier is reported only once for each function it
appears in
../../../../gcc/libgcc/../gcc/common/config/i386/cpuinfo.h:950:25: error:
'bit_AVXVNNIINT16' undeclared (first use in this function); did you mean
'bit_AVXVNNIINT8'?
950 | if (edx & bit_AVXVNNIINT16)
| ^~~~~~~~~~~~~~~~
| bit_AVXVNNIINT8
Hopefully it's still okay.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-26 22:36 [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710] Peter Damianov
2024-03-26 22:43 ` Peter0x44
@ 2024-03-27 7:51 ` Richard Biener
1 sibling, 0 replies; 9+ messages in thread
From: Richard Biener @ 2024-03-27 7:51 UTC (permalink / raw)
To: Peter Damianov; +Cc: gcc-patches
On Tue, Mar 26, 2024 at 11:37 PM Peter Damianov <peter0x44@disroot.org> wrote:
>
> lto-wrapper generates Makefiles that use the following:
> touch -r file file.tmp && mv file.tmp file
> to truncate files.
> If there is no suitable "touch" or "mv" available, then this errors with
> "The system cannot find the file specified".
>
> The solution here is to check if sh -c true works, before trying to use it in
> the Makefile. If it doesn't, then fall back to "copy /y nul file" instead.
> The fallback doesn't work exactly the same (the modified time of the file gets
> updated), but this doesn't seem to matter in practice.
I suppose it doesn't matter as we (no longer?) have the input as dependency
on the rule so make doesn't get confused to re-build it. I guess we
only truncate
the file because it's going to be deleted by another process.
Instead of doing sth like sh_exists I would suggest to simply use #ifdef __WIN
or something like that? Not sure if we have suitable macros to identify the
host operating system though and whether mingw, cygwin, etc. behave the same
here.
As a stop-gap solution doing
( @-touch -r ... ) || true
might also work? Or another way to note to make the command can fail
without causing a problem.
Another way would be to have a portable solution to truncate a file
(maybe even removing it would work). I don't think we should override
SHELL.
Richard.
> I tested this both in environments both with and without sh present, and
> observed no issues.
>
> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> ---
> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
> index 5186d040ce0..8dee0aaa2d8 100644
> --- a/gcc/lto-wrapper.cc
> +++ b/gcc/lto-wrapper.cc
> @@ -1389,6 +1389,27 @@ make_exists (void)
> return errmsg == NULL && exit_status == 0 && err == 0;
> }
>
> +/* Test that an sh command is present and working, return true if so.
> + This is only relevant for windows hosts, where a /bin/sh shell cannot
> + be assumed to exist. */
> +
> +static bool
> +sh_exists (void)
> +{
> + const char *sh = "sh";
> + const char *sh_args[] = {sh, "-c", "true", NULL};
> +#ifdef _WIN32
> + int exit_status = 0;
> + int err = 0;
> + const char *errmsg
> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
> + "sh", NULL, NULL, &exit_status, &err);
> + return errmsg == NULL && exit_status == 0 && err == 0;
> +#else
> + return true;
> +#endif
> +}
> +
> /* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */
>
> static void
> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
> const char *collect_gcc;
> char *collect_gcc_options;
> int parallel = 0;
> + bool have_sh = sh_exists ();
> int jobserver = 0;
> bool jobserver_requested = false;
> int auto_parallel = 0;
> @@ -2016,6 +2038,7 @@ cont:
> argv_ptr[5] = NULL;
> if (parallel)
> {
> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
> fprintf (mstream, "%s:\n\t@%s ", output_name, new_argv[0]);
> for (j = 1; new_argv[j] != NULL; ++j)
> fprintf (mstream, " '%s'", new_argv[j]);
> @@ -2024,9 +2047,15 @@ cont:
> truncate them as soon as we have processed it. This
> reduces temporary disk-space usage. */
> if (! save_temps)
> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
> - input_name, input_name, input_name, input_name);
> + {
> + fprintf (mstream,
> + have_sh
> + ? "\t@-touch -r \"%s\" \"%s.tem\" > /dev/null "
> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
> + : "\t@-copy /y nul \"%s\" > NUL "
> + "2>&1\n",
> + input_name, input_name, input_name, input_name);
> + }
> }
> else
> {
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-27 17:37 ` Peter0x44
@ 2024-03-27 17:50 ` Richard Biener
0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2024-03-27 17:50 UTC (permalink / raw)
To: Peter0x44; +Cc: gcc-patches
> Am 27.03.2024 um 18:37 schrieb Peter0x44 <peter0x44@disroot.org>:
>
>
>>
>>> >> > Another way would be to have a portable solution to truncate a file
>>> >> > (maybe even removing it would work). I don't think we should override
>>> >> > SHELL.
> I've been thinking harder about this, these files get unlinked at the end if they are temporary.
> Is there no way to get make to communicate back this info so lto-wrapper can just unlink the file on its own? It feels easier and less invasive than introducing a whole new option to the driver for only that purpose. If there is some way, I'm not aware of it.
The point is we want to do this earlier than when control gets back to LTO-wrapper to reduce the amount of disk space required.
Richard.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-27 11:27 ` Richard Biener
@ 2024-03-27 17:37 ` Peter0x44
2024-03-27 17:50 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Peter0x44 @ 2024-03-27 17:37 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
>> >> > Another way would be to have a portable solution to truncate a file
>> >> > (maybe even removing it would work). I don't think we should override
>> >> > SHELL.
I've been thinking harder about this, these files get unlinked at the
end if they are temporary.
Is there no way to get make to communicate back this info so lto-wrapper
can just unlink the file on its own? It feels easier and less invasive
than introducing a whole new option to the driver for only that purpose.
If there is some way, I'm not aware of it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-27 9:13 ` peter0x44
@ 2024-03-27 11:27 ` Richard Biener
2024-03-27 17:37 ` Peter0x44
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2024-03-27 11:27 UTC (permalink / raw)
To: peter0x44; +Cc: gcc-patches
On Wed, Mar 27, 2024 at 10:13 AM peter0x44 <peter0x44@disroot.org> wrote:
>
> On 2024-03-27 01:58, Richard Biener wrote:
> > On Wed, Mar 27, 2024 at 9:13 AM Peter0x44 <peter0x44@disroot.org>
> > wrote:
> >>
> >> I accidentally replied off-list. Sorry.
> >>
> >> 27 Mar 2024 8:09:30 am Peter0x44 <peter0x44@disroot.org>:
> >>
> >>
> >> 27 Mar 2024 7:51:26 am Richard Biener <richard.guenther@gmail.com>:
> >>
> >> > On Tue, Mar 26, 2024 at 11:37 PM Peter Damianov <peter0x44@disroot.org>
> >> > wrote:
> >> >>
> >> >> lto-wrapper generates Makefiles that use the following:
> >> >> touch -r file file.tmp && mv file.tmp file
> >> >> to truncate files.
> >> >> If there is no suitable "touch" or "mv" available, then this errors
> >> >> with
> >> >> "The system cannot find the file specified".
> >> >>
> >> >> The solution here is to check if sh -c true works, before trying to
> >> >> use it in
> >> >> the Makefile. If it doesn't, then fall back to "copy /y nul file"
> >> >> instead.
> >> >> The fallback doesn't work exactly the same (the modified time of the
> >> >> file gets
> >> >> updated), but this doesn't seem to matter in practice.
> >> >
> >> > I suppose it doesn't matter as we (no longer?) have the input as
> >> > dependency
> >> > on the rule so make doesn't get confused to re-build it. I guess we
> >> > only truncate
> >> > the file because it's going to be deleted by another process.
> >> >
> >> > Instead of doing sth like sh_exists I would suggest to simply use
> >> > #ifdef __WIN
> >> > or something like that? Not sure if we have suitable macros to
> >> > identify the
> >> > host operating system though and whether mingw, cygwin, etc. behave the
> >> > same
> >> > here.
> >>
> >> They do, I tested. Using sh_exists is deliberate, I've had to program
> >> on
> >> school computers that had cmd.exe disabled, but had busybox-w32
> >> working,
> >> so it might be more flexible in that way. I would prefer a solution
> >> which
> >> didn't require invoking cmd.exe if there is a working shell present.
> >
> > Hmm, but then I'd expect SHELL to be appropriately set in such
> > situation? So shouldn't sh_exists at least try to look at $SHELL?
> I'm not sure it would . On windows, make searches the PATH for an sh.exe
> if
> present, and then uses it by default. The relevant code is here:
> https://git.savannah.gnu.org/cgit/make.git/tree/src/variable.c#n1628
> >
> >> I figured doing the "sh_exists" is okay because there is a basically
> >> identical check for make.
> >>
> >> >
> >> > As a stop-gap solution doing
> >> >
> >> > ( @-touch -r ... ) || true
> >> >
> >> > might also work? Or another way to note to make the command can fail
> >> > without causing a problem.
> >>
> >> I don't think it would work. cmd.exe can't run subshells like this.
> >
> > Hmm, OK. So this is all for the case where 'make' is available (as you
> > say we check for that) but no POSIX command environment is
> > (IIRC both touch and mv are part of POSIX). Testing for 'touch' and
> > 'mv' would then be another option?
> I think it would work, but keep in mind they could be placed on the PATH
> but
> still invoked from cmd.exe, so you might have to be careful of the
> redirection
> syntax and no /dev/null. It's a bit more complexity that doesn't seem
> necessary, to me.
> >
> >> >
> >> > Another way would be to have a portable solution to truncate a file
> >> > (maybe even removing it would work). I don't think we should override
> >> > SHELL.
> >>
> >> Do you mean that perhaps an special command line argument could be
> >> added
> >> to lto-wrapper to do it, and then the makefile could invoke
> >> lto-wrapper
> >> to remove or truncate files instead of a shell? I'm not sure I get the
> >> proposed suggestion.
> >
> > The point is to truncate the file at the earliest point to reduce the
> > peak disk space required for a LTO link with many LTRANS units.
> > But yes, I guess it would be possible to add a flag to gcc itself
> > so the LTRANS compile would truncate the file. Currently we
> > emit sth like
> >
> > ./libquantum.ltrans0.ltrans.o:
> > @/space/rguenther/install/trunk-r14-8925/usr/local/bin/gcc
> > '-xlto' '-c' '-fno-openmp' '-fno-openacc' '-fno-pie'
> > '-fcf-protection=none' '-g' '-mtune=generic' '-march=x86-64' '-O2'
> > '-O2' '-g' '-v' '-save-temps' '-mtune=generic' '-march=x86-64'
> > '-dumpdir' 'libquantum.' '-dumpbase' './libquantum.ltrans0.ltrans'
> > '-fltrans' '-o' './libquantum.ltrans0.ltrans.o'
> > './libquantum.ltrans0.o'
> >
> > so adding a '-truncate-input' flag for the driver or even more
> > explicit '-truncate ./libquantum.ltrans0.o'
> > might be a more elegant solution then?
>
> This is much more elegant. I like it, I can take a look at implementing
> it.
> Would I be looking at gcc.cc for this?
Yes, you'd add the Driver option to gcc/common.opt and handle it from gcc/gcc.cc
Richard.
> Best wishes,
> Peter.
>
> >
> > Richard.
> >
> >> >
> >> > Richard.
> >> >
> >> >> I tested this both in environments both with and without sh present,
> >> >> and
> >> >> observed no issues.
> >> >>
> >> >> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> >> >> ---
> >> >> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
> >> >> 1 file changed, 32 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
> >> >> index 5186d040ce0..8dee0aaa2d8 100644
> >> >> --- a/gcc/lto-wrapper.cc
> >> >> +++ b/gcc/lto-wrapper.cc
> >> >> @@ -1389,6 +1389,27 @@ make_exists (void)
> >> >> return errmsg == NULL && exit_status == 0 && err == 0;
> >> >> }
> >> >>
> >> >> +/* Test that an sh command is present and working, return true if so.
> >> >> + This is only relevant for windows hosts, where a /bin/sh shell
> >> >> cannot
> >> >> + be assumed to exist. */
> >> >> +
> >> >> +static bool
> >> >> +sh_exists (void)
> >> >> +{
> >> >> + const char *sh = "sh";
> >> >> + const char *sh_args[] = {sh, "-c", "true", NULL};
> >> >> +#ifdef _WIN32
> >> >> + int exit_status = 0;
> >> >> + int err = 0;
> >> >> + const char *errmsg
> >> >> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
> >> >> + "sh", NULL, NULL, &exit_status, &err);
> >> >> + return errmsg == NULL && exit_status == 0 && err == 0;
> >> >> +#else
> >> >> + return true;
> >> >> +#endif
> >> >> +}
> >> >> +
> >> >> /* Execute gcc. ARGC is the number of arguments. ARGV contains the
> >> >> arguments. */
> >> >>
> >> >> static void
> >> >> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
> >> >> const char *collect_gcc;
> >> >> char *collect_gcc_options;
> >> >> int parallel = 0;
> >> >> + bool have_sh = sh_exists ();
> >> >> int jobserver = 0;
> >> >> bool jobserver_requested = false;
> >> >> int auto_parallel = 0;
> >> >> @@ -2016,6 +2038,7 @@ cont:
> >> >> argv_ptr[5] = NULL;
> >> >> if (parallel)
> >> >> {
> >> >> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
> >> >> fprintf (mstream, "%s:\n\t@%s ", output_name,
> >> >> new_argv[0]);
> >> >> for (j = 1; new_argv[j] != NULL; ++j)
> >> >> fprintf (mstream, " '%s'", new_argv[j]);
> >> >> @@ -2024,9 +2047,15 @@ cont:
> >> >> truncate them as soon as we have processed it. This
> >> >> reduces temporary disk-space usage. */
> >> >> if (! save_temps)
> >> >> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" >
> >> >> /dev/null "
> >> >> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
> >> >> - input_name, input_name, input_name,
> >> >> input_name);
> >> >> + {
> >> >> + fprintf (mstream,
> >> >> + have_sh
> >> >> + ? "\t@-touch -r \"%s\" \"%s.tem\" >
> >> >> /dev/null "
> >> >> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
> >> >> + : "\t@-copy /y nul \"%s\" > NUL "
> >> >> + "2>&1\n",
> >> >> + input_name, input_name, input_name,
> >> >> input_name);
> >> >> + }
> >> >> }
> >> >> else
> >> >> {
> >> >> --
> >> >> 2.39.2
> >> >>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-27 8:58 ` Richard Biener
@ 2024-03-27 9:13 ` peter0x44
2024-03-27 11:27 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: peter0x44 @ 2024-03-27 9:13 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On 2024-03-27 01:58, Richard Biener wrote:
> On Wed, Mar 27, 2024 at 9:13 AM Peter0x44 <peter0x44@disroot.org>
> wrote:
>>
>> I accidentally replied off-list. Sorry.
>>
>> 27 Mar 2024 8:09:30 am Peter0x44 <peter0x44@disroot.org>:
>>
>>
>> 27 Mar 2024 7:51:26 am Richard Biener <richard.guenther@gmail.com>:
>>
>> > On Tue, Mar 26, 2024 at 11:37 PM Peter Damianov <peter0x44@disroot.org>
>> > wrote:
>> >>
>> >> lto-wrapper generates Makefiles that use the following:
>> >> touch -r file file.tmp && mv file.tmp file
>> >> to truncate files.
>> >> If there is no suitable "touch" or "mv" available, then this errors
>> >> with
>> >> "The system cannot find the file specified".
>> >>
>> >> The solution here is to check if sh -c true works, before trying to
>> >> use it in
>> >> the Makefile. If it doesn't, then fall back to "copy /y nul file"
>> >> instead.
>> >> The fallback doesn't work exactly the same (the modified time of the
>> >> file gets
>> >> updated), but this doesn't seem to matter in practice.
>> >
>> > I suppose it doesn't matter as we (no longer?) have the input as
>> > dependency
>> > on the rule so make doesn't get confused to re-build it. I guess we
>> > only truncate
>> > the file because it's going to be deleted by another process.
>> >
>> > Instead of doing sth like sh_exists I would suggest to simply use
>> > #ifdef __WIN
>> > or something like that? Not sure if we have suitable macros to
>> > identify the
>> > host operating system though and whether mingw, cygwin, etc. behave the
>> > same
>> > here.
>>
>> They do, I tested. Using sh_exists is deliberate, I've had to program
>> on
>> school computers that had cmd.exe disabled, but had busybox-w32
>> working,
>> so it might be more flexible in that way. I would prefer a solution
>> which
>> didn't require invoking cmd.exe if there is a working shell present.
>
> Hmm, but then I'd expect SHELL to be appropriately set in such
> situation? So shouldn't sh_exists at least try to look at $SHELL?
I'm not sure it would . On windows, make searches the PATH for an sh.exe
if
present, and then uses it by default. The relevant code is here:
https://git.savannah.gnu.org/cgit/make.git/tree/src/variable.c#n1628
>
>> I figured doing the "sh_exists" is okay because there is a basically
>> identical check for make.
>>
>> >
>> > As a stop-gap solution doing
>> >
>> > ( @-touch -r ... ) || true
>> >
>> > might also work? Or another way to note to make the command can fail
>> > without causing a problem.
>>
>> I don't think it would work. cmd.exe can't run subshells like this.
>
> Hmm, OK. So this is all for the case where 'make' is available (as you
> say we check for that) but no POSIX command environment is
> (IIRC both touch and mv are part of POSIX). Testing for 'touch' and
> 'mv' would then be another option?
I think it would work, but keep in mind they could be placed on the PATH
but
still invoked from cmd.exe, so you might have to be careful of the
redirection
syntax and no /dev/null. It's a bit more complexity that doesn't seem
necessary, to me.
>
>> >
>> > Another way would be to have a portable solution to truncate a file
>> > (maybe even removing it would work). I don't think we should override
>> > SHELL.
>>
>> Do you mean that perhaps an special command line argument could be
>> added
>> to lto-wrapper to do it, and then the makefile could invoke
>> lto-wrapper
>> to remove or truncate files instead of a shell? I'm not sure I get the
>> proposed suggestion.
>
> The point is to truncate the file at the earliest point to reduce the
> peak disk space required for a LTO link with many LTRANS units.
> But yes, I guess it would be possible to add a flag to gcc itself
> so the LTRANS compile would truncate the file. Currently we
> emit sth like
>
> ./libquantum.ltrans0.ltrans.o:
> @/space/rguenther/install/trunk-r14-8925/usr/local/bin/gcc
> '-xlto' '-c' '-fno-openmp' '-fno-openacc' '-fno-pie'
> '-fcf-protection=none' '-g' '-mtune=generic' '-march=x86-64' '-O2'
> '-O2' '-g' '-v' '-save-temps' '-mtune=generic' '-march=x86-64'
> '-dumpdir' 'libquantum.' '-dumpbase' './libquantum.ltrans0.ltrans'
> '-fltrans' '-o' './libquantum.ltrans0.ltrans.o'
> './libquantum.ltrans0.o'
>
> so adding a '-truncate-input' flag for the driver or even more
> explicit '-truncate ./libquantum.ltrans0.o'
> might be a more elegant solution then?
This is much more elegant. I like it, I can take a look at implementing
it.
Would I be looking at gcc.cc for this?
Best wishes,
Peter.
>
> Richard.
>
>> >
>> > Richard.
>> >
>> >> I tested this both in environments both with and without sh present,
>> >> and
>> >> observed no issues.
>> >>
>> >> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
>> >> ---
>> >> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
>> >> 1 file changed, 32 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
>> >> index 5186d040ce0..8dee0aaa2d8 100644
>> >> --- a/gcc/lto-wrapper.cc
>> >> +++ b/gcc/lto-wrapper.cc
>> >> @@ -1389,6 +1389,27 @@ make_exists (void)
>> >> return errmsg == NULL && exit_status == 0 && err == 0;
>> >> }
>> >>
>> >> +/* Test that an sh command is present and working, return true if so.
>> >> + This is only relevant for windows hosts, where a /bin/sh shell
>> >> cannot
>> >> + be assumed to exist. */
>> >> +
>> >> +static bool
>> >> +sh_exists (void)
>> >> +{
>> >> + const char *sh = "sh";
>> >> + const char *sh_args[] = {sh, "-c", "true", NULL};
>> >> +#ifdef _WIN32
>> >> + int exit_status = 0;
>> >> + int err = 0;
>> >> + const char *errmsg
>> >> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
>> >> + "sh", NULL, NULL, &exit_status, &err);
>> >> + return errmsg == NULL && exit_status == 0 && err == 0;
>> >> +#else
>> >> + return true;
>> >> +#endif
>> >> +}
>> >> +
>> >> /* Execute gcc. ARGC is the number of arguments. ARGV contains the
>> >> arguments. */
>> >>
>> >> static void
>> >> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
>> >> const char *collect_gcc;
>> >> char *collect_gcc_options;
>> >> int parallel = 0;
>> >> + bool have_sh = sh_exists ();
>> >> int jobserver = 0;
>> >> bool jobserver_requested = false;
>> >> int auto_parallel = 0;
>> >> @@ -2016,6 +2038,7 @@ cont:
>> >> argv_ptr[5] = NULL;
>> >> if (parallel)
>> >> {
>> >> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
>> >> fprintf (mstream, "%s:\n\t@%s ", output_name,
>> >> new_argv[0]);
>> >> for (j = 1; new_argv[j] != NULL; ++j)
>> >> fprintf (mstream, " '%s'", new_argv[j]);
>> >> @@ -2024,9 +2047,15 @@ cont:
>> >> truncate them as soon as we have processed it. This
>> >> reduces temporary disk-space usage. */
>> >> if (! save_temps)
>> >> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" >
>> >> /dev/null "
>> >> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
>> >> - input_name, input_name, input_name,
>> >> input_name);
>> >> + {
>> >> + fprintf (mstream,
>> >> + have_sh
>> >> + ? "\t@-touch -r \"%s\" \"%s.tem\" >
>> >> /dev/null "
>> >> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
>> >> + : "\t@-copy /y nul \"%s\" > NUL "
>> >> + "2>&1\n",
>> >> + input_name, input_name, input_name,
>> >> input_name);
>> >> + }
>> >> }
>> >> else
>> >> {
>> >> --
>> >> 2.39.2
>> >>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
2024-03-27 8:12 Peter0x44
@ 2024-03-27 8:58 ` Richard Biener
2024-03-27 9:13 ` peter0x44
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2024-03-27 8:58 UTC (permalink / raw)
To: Peter0x44; +Cc: gcc-patches
On Wed, Mar 27, 2024 at 9:13 AM Peter0x44 <peter0x44@disroot.org> wrote:
>
> I accidentally replied off-list. Sorry.
>
> 27 Mar 2024 8:09:30 am Peter0x44 <peter0x44@disroot.org>:
>
>
> 27 Mar 2024 7:51:26 am Richard Biener <richard.guenther@gmail.com>:
>
> > On Tue, Mar 26, 2024 at 11:37 PM Peter Damianov <peter0x44@disroot.org>
> > wrote:
> >>
> >> lto-wrapper generates Makefiles that use the following:
> >> touch -r file file.tmp && mv file.tmp file
> >> to truncate files.
> >> If there is no suitable "touch" or "mv" available, then this errors
> >> with
> >> "The system cannot find the file specified".
> >>
> >> The solution here is to check if sh -c true works, before trying to
> >> use it in
> >> the Makefile. If it doesn't, then fall back to "copy /y nul file"
> >> instead.
> >> The fallback doesn't work exactly the same (the modified time of the
> >> file gets
> >> updated), but this doesn't seem to matter in practice.
> >
> > I suppose it doesn't matter as we (no longer?) have the input as
> > dependency
> > on the rule so make doesn't get confused to re-build it. I guess we
> > only truncate
> > the file because it's going to be deleted by another process.
> >
> > Instead of doing sth like sh_exists I would suggest to simply use
> > #ifdef __WIN
> > or something like that? Not sure if we have suitable macros to
> > identify the
> > host operating system though and whether mingw, cygwin, etc. behave the
> > same
> > here.
>
> They do, I tested. Using sh_exists is deliberate, I've had to program on
> school computers that had cmd.exe disabled, but had busybox-w32 working,
> so it might be more flexible in that way. I would prefer a solution which
> didn't require invoking cmd.exe if there is a working shell present.
Hmm, but then I'd expect SHELL to be appropriately set in such
situation? So shouldn't sh_exists at least try to look at $SHELL?
> I figured doing the "sh_exists" is okay because there is a basically
> identical check for make.
>
> >
> > As a stop-gap solution doing
> >
> > ( @-touch -r ... ) || true
> >
> > might also work? Or another way to note to make the command can fail
> > without causing a problem.
>
> I don't think it would work. cmd.exe can't run subshells like this.
Hmm, OK. So this is all for the case where 'make' is available (as you
say we check for that) but no POSIX command environment is
(IIRC both touch and mv are part of POSIX). Testing for 'touch' and
'mv' would then be another option?
> >
> > Another way would be to have a portable solution to truncate a file
> > (maybe even removing it would work). I don't think we should override
> > SHELL.
>
> Do you mean that perhaps an special command line argument could be added
> to lto-wrapper to do it, and then the makefile could invoke lto-wrapper
> to remove or truncate files instead of a shell? I'm not sure I get the
> proposed suggestion.
The point is to truncate the file at the earliest point to reduce the
peak disk space required for a LTO link with many LTRANS units.
But yes, I guess it would be possible to add a flag to gcc itself
so the LTRANS compile would truncate the file. Currently we
emit sth like
./libquantum.ltrans0.ltrans.o:
@/space/rguenther/install/trunk-r14-8925/usr/local/bin/gcc
'-xlto' '-c' '-fno-openmp' '-fno-openacc' '-fno-pie'
'-fcf-protection=none' '-g' '-mtune=generic' '-march=x86-64' '-O2'
'-O2' '-g' '-v' '-save-temps' '-mtune=generic' '-march=x86-64'
'-dumpdir' 'libquantum.' '-dumpbase' './libquantum.ltrans0.ltrans'
'-fltrans' '-o' './libquantum.ltrans0.ltrans.o'
'./libquantum.ltrans0.o'
so adding a '-truncate-input' flag for the driver or even more
explicit '-truncate ./libquantum.ltrans0.o'
might be a more elegant solution then?
Richard.
> >
> > Richard.
> >
> >> I tested this both in environments both with and without sh present,
> >> and
> >> observed no issues.
> >>
> >> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> >> ---
> >> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
> >> 1 file changed, 32 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
> >> index 5186d040ce0..8dee0aaa2d8 100644
> >> --- a/gcc/lto-wrapper.cc
> >> +++ b/gcc/lto-wrapper.cc
> >> @@ -1389,6 +1389,27 @@ make_exists (void)
> >> return errmsg == NULL && exit_status == 0 && err == 0;
> >> }
> >>
> >> +/* Test that an sh command is present and working, return true if so.
> >> + This is only relevant for windows hosts, where a /bin/sh shell
> >> cannot
> >> + be assumed to exist. */
> >> +
> >> +static bool
> >> +sh_exists (void)
> >> +{
> >> + const char *sh = "sh";
> >> + const char *sh_args[] = {sh, "-c", "true", NULL};
> >> +#ifdef _WIN32
> >> + int exit_status = 0;
> >> + int err = 0;
> >> + const char *errmsg
> >> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
> >> + "sh", NULL, NULL, &exit_status, &err);
> >> + return errmsg == NULL && exit_status == 0 && err == 0;
> >> +#else
> >> + return true;
> >> +#endif
> >> +}
> >> +
> >> /* Execute gcc. ARGC is the number of arguments. ARGV contains the
> >> arguments. */
> >>
> >> static void
> >> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
> >> const char *collect_gcc;
> >> char *collect_gcc_options;
> >> int parallel = 0;
> >> + bool have_sh = sh_exists ();
> >> int jobserver = 0;
> >> bool jobserver_requested = false;
> >> int auto_parallel = 0;
> >> @@ -2016,6 +2038,7 @@ cont:
> >> argv_ptr[5] = NULL;
> >> if (parallel)
> >> {
> >> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
> >> fprintf (mstream, "%s:\n\t@%s ", output_name,
> >> new_argv[0]);
> >> for (j = 1; new_argv[j] != NULL; ++j)
> >> fprintf (mstream, " '%s'", new_argv[j]);
> >> @@ -2024,9 +2047,15 @@ cont:
> >> truncate them as soon as we have processed it. This
> >> reduces temporary disk-space usage. */
> >> if (! save_temps)
> >> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" >
> >> /dev/null "
> >> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
> >> - input_name, input_name, input_name,
> >> input_name);
> >> + {
> >> + fprintf (mstream,
> >> + have_sh
> >> + ? "\t@-touch -r \"%s\" \"%s.tem\" >
> >> /dev/null "
> >> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
> >> + : "\t@-copy /y nul \"%s\" > NUL "
> >> + "2>&1\n",
> >> + input_name, input_name, input_name,
> >> input_name);
> >> + }
> >> }
> >> else
> >> {
> >> --
> >> 2.39.2
> >>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710]
@ 2024-03-27 8:12 Peter0x44
2024-03-27 8:58 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Peter0x44 @ 2024-03-27 8:12 UTC (permalink / raw)
To: gcc-patches
I accidentally replied off-list. Sorry.
27 Mar 2024 8:09:30 am Peter0x44 <peter0x44@disroot.org>:
27 Mar 2024 7:51:26 am Richard Biener <richard.guenther@gmail.com>:
> On Tue, Mar 26, 2024 at 11:37 PM Peter Damianov <peter0x44@disroot.org>
> wrote:
>>
>> lto-wrapper generates Makefiles that use the following:
>> touch -r file file.tmp && mv file.tmp file
>> to truncate files.
>> If there is no suitable "touch" or "mv" available, then this errors
>> with
>> "The system cannot find the file specified".
>>
>> The solution here is to check if sh -c true works, before trying to
>> use it in
>> the Makefile. If it doesn't, then fall back to "copy /y nul file"
>> instead.
>> The fallback doesn't work exactly the same (the modified time of the
>> file gets
>> updated), but this doesn't seem to matter in practice.
>
> I suppose it doesn't matter as we (no longer?) have the input as
> dependency
> on the rule so make doesn't get confused to re-build it. I guess we
> only truncate
> the file because it's going to be deleted by another process.
>
> Instead of doing sth like sh_exists I would suggest to simply use
> #ifdef __WIN
> or something like that? Not sure if we have suitable macros to
> identify the
> host operating system though and whether mingw, cygwin, etc. behave the
> same
> here.
They do, I tested. Using sh_exists is deliberate, I've had to program on
school computers that had cmd.exe disabled, but had busybox-w32 working,
so it might be more flexible in that way. I would prefer a solution which
didn't require invoking cmd.exe if there is a working shell present.
I figured doing the "sh_exists" is okay because there is a basically
identical check for make.
>
> As a stop-gap solution doing
>
> ( @-touch -r ... ) || true
>
> might also work? Or another way to note to make the command can fail
> without causing a problem.
I don't think it would work. cmd.exe can't run subshells like this.
>
> Another way would be to have a portable solution to truncate a file
> (maybe even removing it would work). I don't think we should override
> SHELL.
Do you mean that perhaps an special command line argument could be added
to lto-wrapper to do it, and then the makefile could invoke lto-wrapper
to remove or truncate files instead of a shell? I'm not sure I get the
proposed suggestion.
>
> Richard.
>
>> I tested this both in environments both with and without sh present,
>> and
>> observed no issues.
>>
>> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
>> ---
>> gcc/lto-wrapper.cc | 35 ++++++++++++++++++++++++++++++++---
>> 1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
>> index 5186d040ce0..8dee0aaa2d8 100644
>> --- a/gcc/lto-wrapper.cc
>> +++ b/gcc/lto-wrapper.cc
>> @@ -1389,6 +1389,27 @@ make_exists (void)
>> return errmsg == NULL && exit_status == 0 && err == 0;
>> }
>>
>> +/* Test that an sh command is present and working, return true if so.
>> + This is only relevant for windows hosts, where a /bin/sh shell
>> cannot
>> + be assumed to exist. */
>> +
>> +static bool
>> +sh_exists (void)
>> +{
>> + const char *sh = "sh";
>> + const char *sh_args[] = {sh, "-c", "true", NULL};
>> +#ifdef _WIN32
>> + int exit_status = 0;
>> + int err = 0;
>> + const char *errmsg
>> + = pex_one (PEX_SEARCH, sh_args[0], CONST_CAST (char **, sh_args),
>> + "sh", NULL, NULL, &exit_status, &err);
>> + return errmsg == NULL && exit_status == 0 && err == 0;
>> +#else
>> + return true;
>> +#endif
>> +}
>> +
>> /* Execute gcc. ARGC is the number of arguments. ARGV contains the
>> arguments. */
>>
>> static void
>> @@ -1402,6 +1423,7 @@ run_gcc (unsigned argc, char *argv[])
>> const char *collect_gcc;
>> char *collect_gcc_options;
>> int parallel = 0;
>> + bool have_sh = sh_exists ();
>> int jobserver = 0;
>> bool jobserver_requested = false;
>> int auto_parallel = 0;
>> @@ -2016,6 +2038,7 @@ cont:
>> argv_ptr[5] = NULL;
>> if (parallel)
>> {
>> + fprintf (mstream, "SHELL=%s\n", have_sh ? "sh" : "cmd");
>> fprintf (mstream, "%s:\n\t@%s ", output_name,
>> new_argv[0]);
>> for (j = 1; new_argv[j] != NULL; ++j)
>> fprintf (mstream, " '%s'", new_argv[j]);
>> @@ -2024,9 +2047,15 @@ cont:
>> truncate them as soon as we have processed it. This
>> reduces temporary disk-space usage. */
>> if (! save_temps)
>> - fprintf (mstream, "\t@-touch -r \"%s\" \"%s.tem\" >
>> /dev/null "
>> - "2>&1 && mv \"%s.tem\" \"%s\"\n",
>> - input_name, input_name, input_name,
>> input_name);
>> + {
>> + fprintf (mstream,
>> + have_sh
>> + ? "\t@-touch -r \"%s\" \"%s.tem\" >
>> /dev/null "
>> + "2>&1 && mv \"%s.tem\" \"%s\"\n"
>> + : "\t@-copy /y nul \"%s\" > NUL "
>> + "2>&1\n",
>> + input_name, input_name, input_name,
>> input_name);
>> + }
>> }
>> else
>> {
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-27 17:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-26 22:36 [PATCH] lto: Don't assume a posix shell is usable on windows [PR110710] Peter Damianov
2024-03-26 22:43 ` Peter0x44
2024-03-27 7:51 ` Richard Biener
2024-03-27 8:12 Peter0x44
2024-03-27 8:58 ` Richard Biener
2024-03-27 9:13 ` peter0x44
2024-03-27 11:27 ` Richard Biener
2024-03-27 17:37 ` Peter0x44
2024-03-27 17:50 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).