[PATCH] driver: Call finalize method from main.
Calling the driver::finalize() method before returning from main seems to be reducing some memory leaks of the driver (PR93019). $ head -n20 before_patch.txt ==385521== Memcheck, a memory error detector ==385521== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==385521== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info ==385521== Command: /home/cargyris/gcc/install/bin/gcc -g -O2 t1.c ==385521== Parent PID: 165153 ==385521== ==385521== ==385521== HEAP SUMMARY: ==385521== in use at exit: 173,136 bytes in 115 blocks ==385521== total heap usage: 497 allocs, 382 frees, 229,696 bytes allocated ==385521== ==385521== 1 bytes in 1 blocks are still reachable in loss record 1 of 99 $ head -n20 after_patch.txt ==397575== Memcheck, a memory error detector ==397575== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==397575== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info ==397575== Command: /home/cargyris/gcc/install/bin/gcc -g -O2 t1.c ==397575== Parent PID: 165153 ==397575== ==397575== ==397575== HEAP SUMMARY: ==397575== in use at exit: 146,573 bytes in 77 blocks ==397575== total heap usage: 497 allocs, 420 frees, 229,696 bytes allocated ==397575== ==397575== 3 bytes in 1 blocks are indirectly lost in loss record 1 of 61 This makes some sense because driver::finalize() performs many memory deallocations, among other things.It was introduced here: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9376dd63e6a2d94823f6faf8212c9f37bef5a656 Is there any reason for not wanting to call it before returning from main? Also, the driver constructor's can_finalize boolean looks more like a can_restore_env boolean, as it only affects the environment manager and nothing else in the driver (no?).By making driver::finalize() call env.restore() conditional on can_restore_env, it is possible to always call driver::finalize() even for can_restore_env == false and still do all the rest that finalize() does, including freeing memory. Is there anything wrong with calling d.finalize() from main without restoring the environment?The next step is returning from main anyway, and the program ends, so I can't think of any reason not to do this, given that it helps with the memory leaks.Is there? 0001-driver-Call-finalize-method-from-main.patch Description: Binary data
[PATCH] driver: Fix memory leak.
Use std::vector instead of malloc'd pointer to get automatic freeing of memory. Result was verified by valgrind, which showed one less loss record. I think Jonathan is/was working on this transition but on a larger scale. 0001-driver-Fix-memory-leak.patch Description: Binary data
Re: [PATCH] driver: Fix memory leak.
Attached a new patch with these changes. On Mon, 4 Dec 2023 at 12:15, Jonathan Wakely wrote: > On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote: > > > > Use std::vector instead of malloc'd pointer > > to get automatic freeing of memory. > > You can't include there. Instead you need to define > INCLUDE_VECTOR before "system.h" > > Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is > undefined. > > 0001-driver-Fix-memory-leak.patch Description: Binary data
Re: [PATCH] driver: Fix memory leak.
Would that be something like this? Although it didn't fix the leak, which was the entire point of this exercise. Maybe because driver::finalize () is not getting called so the call to mdswitches.release () doesn't really happen, which was the reason I went with std::vector in the first place because it takes care of itself. On Wed, 6 Dec 2023 at 14:39, Jakub Jelinek wrote: > On Wed, Dec 06, 2023 at 02:29:25PM +, Costas Argyris wrote: > > Attached a new patch with these changes. > > > > On Mon, 4 Dec 2023 at 12:15, Jonathan Wakely wrote: > > > > > On Sat, 2 Dec 2023 at 21:24, Costas Argyris wrote: > > > > > > > > Use std::vector instead of malloc'd pointer > > > > to get automatic freeing of memory. > > > > > > You can't include there. Instead you need to define > > > INCLUDE_VECTOR before "system.h" > > > > > > Shouldn't you be using resize, not reserve? Otherwise mdswitches[i] is > > > undefined. > > Any reason not to use vec.h instead? > I especially don't like the fact that with a global > std::vector var; > it means runtime __cxa_atexit for the var the destruction, which it really > doesn't need on exit. > > We really don't need to free the memory at exit time, that is just wasted > cycles, all we need is that it is freed before the pointer or vector is > cleared. > > Jakub > > 0001-driver-Fix-memory-leak.patch Description: Binary data
Re: [PATCH] driver: Fix memory leak.
> Still reachable memory at exit e.g. from valgrind is not a bug. Indeed, this is coming from a valgrind report here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93019 where it was noted that the driver memory leaks could be problematic for JIT. So, since using std::vector did reduce the valgrind records by one (I only targeted a single variable to begin with) I took that as a good sign. Regarding adding a call to XDELETE (mdswitches), yes, that would help in the case where driver::finalize () is actually called, which I think is for JIT.I was trying to take care of the case where it doesn't get called as well, but from what you say I take it that this case is not of interest. On Thu, 7 Dec 2023 at 14:42, Jakub Jelinek wrote: > On Thu, Dec 07, 2023 at 02:28:18PM +0000, Costas Argyris wrote: > > Would that be something like this? > > Yes. Or perhaps even easier just change > > --- gcc/gcc.cc.jj 2023-12-07 08:31:59.970849379 +0100 > +++ gcc/gcc.cc 2023-12-07 15:33:46.616886894 +0100 > @@ -11368,6 +11368,7 @@ driver::finalize () >input_from_pipe = 0; >suffix_subst = NULL; > > + XDELETE (mdswitches); >mdswitches = NULL; >n_mdswitches = 0; > > > Although it didn't fix the leak, which was the entire point of this > > exercise. > > > > Maybe because driver::finalize () is not getting called so the call to > > mdswitches.release () doesn't really happen, which was the reason > > I went with std::vector in the first place because it takes care of > itself. > > In that case you are fixing a non-issue. > exit frees all allocated memory, no need to do it explicitly and waste > compile time cycles on it. > > Leak is when some memory is allocated and pointer to it lost, that is not > the case here. > > Still reachable memory at exit e.g. from valgrind is not a bug. > > E.g. glibc in order to make fewer still reachable memory reports exports > a __libc_freeres function which valgrind and other memory allocation > debuggers can call on exit to free extra memory, something that isn't > really > needed to waste time on normally. But I'm not sure if there is some way > for an application to provide such functions as well. > > Jakub > >
Re: [PATCH] driver: Fix memory leak.
Thanks for all the explanations. In that case I restrict this patch to just freeing the buffer from within driver::finalize only (I think it should be XDELETEVEC instead of XDELETE, no?). On Thu, 7 Dec 2023 at 15:42, Jakub Jelinek wrote: > On Thu, Dec 07, 2023 at 03:16:29PM +0000, Costas Argyris wrote: > > > Still reachable memory at exit e.g. from valgrind is not a bug. > > > > Indeed, this is coming from a valgrind report here: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93019 > > > > where it was noted that the driver memory leaks could be > > problematic for JIT. > > In the invoke_embedded_driver JIT case yes, that calls driver::finalize (), > which is why it should be freed before clearing the pointer in there (as > then it is a real leak). > > > So, since using std::vector did reduce the valgrind records > > by one (I only targeted a single variable to begin with) I took > > that as a good sign. > > > > Regarding adding a call to XDELETE (mdswitches), yes, > > that would help in the case where driver::finalize () is actually > > called, which I think is for JIT.I was trying to take care of the > > case where it doesn't get called as well, but from what you say > > I take it that this case is not of interest. > > That is wasted compile time on a non-issue. > > If you see a JIT issue with definitely lost records, that is something > that obviously should be fixed (but even in that area I think we've been a > little bit lazy in the option handling). > The most important is that the actual compiler binaries (cc1, cc1plus, ...) > don't leak memory (in the definitely lost kind) like crazy, we have > --enable-checking=valgrind > for that purpose, where the driver runs cc1/cc1plus etc. under valgrind, > but this is very expensive and slow, so usually it is run once during a > cycle (if at all), on a fast machine could take even in non-bootstrap mode > a weekend to go through the whole testsuite, then one can look at the > leaks. > > Jakub > > 0001-driver-Fix-memory-leak-in-driver-finalize.patch Description: Binary data
Re: [PATCH] driver: Fix memory leak.
Does the simple XDELETEVEC patch need any more work? I think it just fixes a leak for the JIT case where driver::finalize is called. On Thu, 7 Dec 2023 at 16:04, Jakub Jelinek wrote: > On Thu, Dec 07, 2023 at 04:01:11PM +0000, Costas Argyris wrote: > > Thanks for all the explanations. > > > > In that case I restrict this patch to just freeing the buffer from > > within driver::finalize only (I think it should be XDELETEVEC > > instead of XDELETE, no?). > > Both macros are exactly the same, but XDELETEVEC is probably better > counterpart to XNEWVEC. > > Jakub > >
[PATCH] mingw: Exclude utf8 manifest [PR111170, PR108865]
This patch makes the inclusion of the utf8 manifest on the mingw hosts optional by introducing the configure option --disable-win32-utf8-manifest (has no effect on non-mingw hosts). Bootstrapped OK on i686-w64-mingw32 and x86_64-w64-mingw32 with and without --disable-win32-utf8-manifest. Costas Exclude-win32-utf8-manifest.patch Description: Binary data
Re: [PATCH] mingw: Exclude utf8 manifest [PR111170, PR108865]
Attached a new patch.
A couple things to note:
1) I changed your
host_extra_objs=utf8-mingw32.o
to
host_extra_objs_mingw=utf8-mingw32.o
to match the other two, since I believe that's what you meant.
2) This approach has the complication that the variables
in configure.ac need to be set before it sources config.host.
On Wed, 22 Nov 2023 at 01:17, Jonathan Yong <10wa...@gmail.com> wrote:
> On 11/21/23 18:07, Costas Argyris wrote:
> > This patch makes the inclusion of the utf8 manifest on the
> > mingw hosts optional by introducing the configure option
> > --disable-win32-utf8-manifest (has no effect on non-mingw
> > hosts).
> >
> > Bootstrapped OK on i686-w64-mingw32 and x86_64-w64-mingw32
> > with and without --disable-win32-utf8-manifest.
> >
> > Costas
> >
>
> I would prefer a AC_ARG_ENABLE to document the option in configure.ac,
> so it would show with configure --help. It should set new variables to
> i386/x-mingw32-utf8, utf8rc-mingw32.o and utf8-mingw32.o respectively
> unless disabled, like so:
>
> host_xmake_mingw=i386/x-mingw32-utf8
> host_extra_gcc_objs_mingw=utf8rc-mingw32.o
> host_extra_objs=utf8-mingw32.o
>
> And then entries in config.host would be:
>
> > i[34567]86-*-mingw32* | x86_64-*-mingw*)
> > host_xm_file=i386/xm-mingw32.h
> > host_xmake_file="${host_xmake_file} ${host_xmake_mingw}
> i386/x-mingw32"
> > host_extra_gcc_objs="${host_extra_gcc_objs}
> ${host_extra_gcc_objs_mingw} driver-mingw32. >
> host_extra_objs="${host_extra_objs} ${host_extra_objs_mingw}"
>
>
Exclude-win32-utf8-manifest.patch
Description: Binary data
Re: [PATCH] mingw: Exclude utf8 manifest [PR111170, PR108865]
Thanks. Eric, who originally reported the issue with the Ada compiler dropping a feature due to the utf8 manifest, is asking for this to be backported onto the 13 branch here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70#c13 If you agree with this, is there anything you would like me to do patch-wise? Costas On Thu, 23 Nov 2023 at 00:50, Jonathan Yong <10wa...@gmail.com> wrote: > On 11/22/23 12:34, Costas Argyris wrote: > > Attached a new patch. > > > > A couple things to note: > > > > 1) I changed your > > > > host_extra_objs=utf8-mingw32.o > > > > to > > > > host_extra_objs_mingw=utf8-mingw32.o > > > > to match the other two, since I believe that's what you meant. > > > > 2) This approach has the complication that the variables > > in configure.ac need to be set before it sources config.host. > > > > I specifically asked for it to be done that way so users are aware of it > with --help. Thanks, pushed to master. >
Re: [PATCH] libiberty: On Windows pass a >32k cmdline through a response file.
Thanks, here is the follow up patch for a couple typos in the same file. On Mon, 5 Jun 2023 at 09:12, Jonathan Yong <10wa...@gmail.com> wrote: > On 5/23/23 08:21, Jonathan Yong wrote: > > On 5/22/23 13:25, Costas Argyris wrote: > >> Currently on Windows, when CreateProcess is called with a command-line > >> that exceeds the 32k Windows limit, we get a very bad error: > >> > >> "CreateProcess: No such file or directory" > >> > >> This patch detects the case where this would happen and writes the > >> long command-line to a temporary response file and calls CreateProcess > >> with @file instead. > >> > > > > Looks OK to me. > > > > I will commit it around next week if there are no objections. > > > > Done, pushed to master, thanks. > > From 45c18cf113585aa0b03512a459e757c7aaef69ce Mon Sep 17 00:00:00 2001 From: Costas Argyris Date: Mon, 5 Jun 2023 10:03:11 +0100 Subject: [PATCH] libiberty: pex-win32.c: Fix some typos. Signed-off-by: Costas Argyris --- libiberty/pex-win32.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c index 0fd8b38734c..f7fe306036b 100644 --- a/libiberty/pex-win32.c +++ b/libiberty/pex-win32.c @@ -351,7 +351,7 @@ argv_to_cmdline (char *const *argv) prevent wasting 2 chars per argument of the CreateProcess 32k char limit. We need only escape embedded double-quotes and immediately preceeding backslash characters. A sequence of backslach characters - that is not follwed by a double quote character will not be + that is not followed by a double quote character will not be escaped. */ needs_quotes = 0; for (j = 0; argv[i][j]; j++) @@ -366,7 +366,7 @@ argv_to_cmdline (char *const *argv) /* Escape preceeding backslashes. */ for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) cmdline_len++; - /* Escape the qote character. */ + /* Escape the quote character. */ cmdline_len++; } } -- 2.30.2
[PATCH] libiberty: writeargv: Simplify function error mode.
writeargv can be simplified by getting rid of the error exit mode
that was only relevant many years ago when the function used
to open the file descriptor internally.
From 1271552baee5561fa61652f4ca7673c9667e4f8f Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Mon, 5 Jun 2023 15:02:06 +0100
Subject: [PATCH] libiberty: writeargv: Simplify function error mode.
The goto-based error mode was based on a previous version
of the function where it was responsible for opening the
file, so it had to close it upon any exit:
https://inbox.sourceware.org/gcc-patches/20070417200340.gm9...@sparrowhawk.codesourcery.com/
(thanks pinskia)
This is no longer the case though since now the function
takes the file descriptor as input, so the exit mode on
error can be just a simple return 1 statement.
Signed-off-by: Costas Argyris
---
libiberty/argv.c | 29 +
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/libiberty/argv.c b/libiberty/argv.c
index a95a10e14ff..1a18b4d8866 100644
--- a/libiberty/argv.c
+++ b/libiberty/argv.c
@@ -289,8 +289,8 @@ char **buildargv (const char *input)
@deftypefn Extension int writeargv (char * const *@var{argv}, FILE *@var{file})
Write each member of ARGV, handling all necessary quoting, to the file
-named by FILE, separated by whitespace. Return 0 on success, non-zero
-if an error occurred while writing to FILE.
+associated with FILE, separated by whitespace. Return 0 on success,
+non-zero if an error occurred while writing to FILE.
@end deftypefn
@@ -314,36 +314,25 @@ writeargv (char * const *argv, FILE *f)
if (ISSPACE(c) || c == '\\' || c == '\'' || c == '"')
if (EOF == fputc ('\\', f))
- {
-status = 1;
-goto done;
- }
+ return 1;
if (EOF == fputc (c, f))
-{
- status = 1;
- goto done;
-}
+return 1;
+
arg++;
}
/* Write out a pair of quotes for an empty argument. */
if (arg == *argv)
- if (EOF == fputs ("\"\"", f))
- {
- status = 1;
- goto done;
- }
+if (EOF == fputs ("\"\"", f))
+ return 1;
if (EOF == fputc ('\n', f))
-{
- status = 1;
- goto done;
-}
+return 1;
+
argv++;
}
- done:
return status;
}
--
2.30.2
Re: [PATCH] libiberty: writeargv: Simplify function error mode.
You are right, this is also a remnant of the old function design
that I completely missed.Here is the follow-up patch for that.
Thanks for pointing it out.
Costas
On Tue, 6 Jun 2023 at 04:12, Jeff Law wrote:
>
>
> On 6/5/23 08:37, Costas Argyris via Gcc-patches wrote:
> > writeargv can be simplified by getting rid of the error exit mode
> > that was only relevant many years ago when the function used
> > to open the file descriptor internally.
> [ ... ]
> Thanks. I've pushed this to the trunk.
>
> You could (as a follow-up) simplify it even further. There's no need
> for the status variable as far as I can tell. You could just have the
> final return be "return 0;" instead of "return status;".
>
> Jeff
>
From 13fdfea60eeac64e028315392614b955e998487d Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Tue, 6 Jun 2023 09:15:48 +0100
Subject: [PATCH] libiberty: writeargv: Remove unnecessary status variable.
Follow-up from 4d1e4ce986f pointed out by jlaw.
Signed-off-by: Costas Argyris
---
libiberty/argv.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/libiberty/argv.c b/libiberty/argv.c
index 1a18b4d8866..c2823d3e4ba 100644
--- a/libiberty/argv.c
+++ b/libiberty/argv.c
@@ -299,8 +299,6 @@ non-zero if an error occurred while writing to FILE.
int
writeargv (char * const *argv, FILE *f)
{
- int status = 0;
-
if (f == NULL)
return 1;
@@ -333,7 +331,7 @@ writeargv (char * const *argv, FILE *f)
argv++;
}
- return status;
+ return 0;
}
/*
--
2.30.2
[PATCH] libiberty: pex-unix.c: Make pex_unix_cleanup signature always match body.
I saw this while working on something else:
pex_unix_cleanup signature doesn't always match the
body of the function in terms of ATTRIBUTE_UNUSED.
If the conditional code in the body is compiled, then
ATTRIBUTE_UNUSED isn't correct.
This change makes it always match, thereby making it
a bit cleaner IMO.
Costas
From 4c84afd631ad09011b237790599e1c320852f82d Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Wed, 7 Jun 2023 10:34:14 +0100
Subject: [PATCH] libiberty: pex-unix.c: Make pex_unix_cleanup signature always
match body.
Signed-off-by: Costas Argyris
---
libiberty/pex-unix.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libiberty/pex-unix.c b/libiberty/pex-unix.c
index 33b5bce31c2..10f8ddd2feb 100644
--- a/libiberty/pex-unix.c
+++ b/libiberty/pex-unix.c
@@ -814,9 +814,9 @@ pex_unix_fdopenw (struct pex_obj *obj ATTRIBUTE_UNUSED, int fd,
}
static void
-pex_unix_cleanup (struct pex_obj *obj ATTRIBUTE_UNUSED)
-{
#if !defined (HAVE_WAIT4) && !defined (HAVE_WAITPID)
+pex_unix_cleanup (struct pex_obj *obj)
+{
while (obj->sysdep != NULL)
{
struct status_list *this;
@@ -827,5 +827,9 @@ pex_unix_cleanup (struct pex_obj *obj ATTRIBUTE_UNUSED)
free (this);
obj->sysdep = (void *) next;
}
-#endif
}
+#else
+pex_unix_cleanup (struct pex_obj *obj ATTRIBUTE_UNUSED)
+{
+}
+#endif
--
2.30.2
Re: [PATCH] libiberty: pex-unix.c: Make pex_unix_cleanup signature always match body.
Oh OK, thanks for the clarification. Costas On Wed, 7 Jun 2023 at 13:59, Jeff Law wrote: > > > On 6/7/23 04:21, Costas Argyris via Gcc-patches wrote: > > I saw this while working on something else: > > > > pex_unix_cleanup signature doesn't always match the > > body of the function in terms of ATTRIBUTE_UNUSED. > > If the conditional code in the body is compiled, then > > ATTRIBUTE_UNUSED isn't correct. > > > > This change makes it always match, thereby making it > > a bit cleaner IMO. > ATTRIBUTE_UNUSED is meant to be a "maybe unused" decoration. I'd just > leave it as-is. > > jeff >
[PATCH] gcc-ar: Remove code duplication.
Some refactoring I thought would be useful while looking at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576
I think some duplicated code can go away by doing this,
while also saving a bit of memory.
From c3f3b2fd53291805b5d0be19df6d1a348c5889ec Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Thu, 15 Jun 2023 12:37:35 +0100
Subject: [PATCH] gcc-ar: Remove code duplication.
Preparatory refactoring that simplifies by eliminating
some duplicated code, before trying to fix 77576.
I believe this stands on its own regardless of the PR.
It also saves a nargv element when we have a plugin and
three when not.
Signed-off-by: Costas Argyris
---
gcc/gcc-ar.cc | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/gcc/gcc-ar.cc b/gcc/gcc-ar.cc
index 5e5b63e1988..4e4c525927d 100644
--- a/gcc/gcc-ar.cc
+++ b/gcc/gcc-ar.cc
@@ -128,6 +128,9 @@ main (int ac, char **av)
const char *exe_name;
#if HAVE_LTO_PLUGIN > 0
char *plugin;
+ const int j = 2; /* Two extra args, --plugin */
+#else
+ const int j = 0; /* No extra args. */
#endif
int k, status, err;
const char *err_msg;
@@ -206,25 +209,21 @@ main (int ac, char **av)
}
}
+ /* Prepend - if necessary. */
+ if (is_ar && av[1] && av[1][0] != '-')
+av[1] = concat ("-", av[1], NULL);
+
/* Create new command line with plugin - if we have one, otherwise just
copy the command through. */
- nargv = XCNEWVEC (const char *, ac + 4);
+ nargv = XCNEWVEC (const char *, ac + j + 1); /* +j plugin args +1 for NULL. */
nargv[0] = exe_name;
#if HAVE_LTO_PLUGIN > 0
nargv[1] = "--plugin";
nargv[2] = plugin;
- if (is_ar && av[1] && av[1][0] != '-')
-av[1] = concat ("-", av[1], NULL);
- for (k = 1; k < ac; k++)
-nargv[2 + k] = av[k];
- nargv[2 + k] = NULL;
-#else
- if (is_ar && av[1] && av[1][0] != '-')
-av[1] = concat ("-", av[1], NULL);
- for (k = 1; k < ac; k++)
-nargv[k] = av[k];
- nargv[k] = NULL;
#endif
+ for (k = 1; k < ac; k++)
+nargv[j + k] = av[k];
+ nargv[j + k] = NULL;
/* Run utility */
/* ??? the const is misplaced in pex_one's argv? */
--
2.30.2
Re: [PATCH] gcc-ar: Handle response files properly [PR77576]
Pinging to try and get this bug in gcc-ar fixed.
Note that the patch posted as an attachment in
https://gcc.gnu.org/pipermail/gcc-patches/2023-July/623400.html
is exactly the same as the patch embedded in
https://gcc.gnu.org/pipermail/gcc-patches/2023-July/623855.html
and the one posted in the PR itself
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576
On Fri, 7 Jul 2023 at 13:00, Costas Argyris
wrote:
> Bootstrapped successfully on x86_64-pc-linux-gnu
>
> On Fri, 7 Jul 2023 at 11:33, Costas Argyris
> wrote:
>
>> Problem: gcc-ar fails when a @file is passed to it:
>>
>> $ cat rsp
>> --version
>> $ gcc-ar @rsp
>> /usr/bin/ar: invalid option -- '@'
>>
>> This is because a dash '-' is prepended to the first
>> argument if it doesn't start with one, resulting in
>> the wrong call 'ar -@rsp'.
>>
>> Fix: Expand argv to get rid of any @files and if any
>> expansions were made, pass everything through a
>> temporary response file.
>>
>> $ gcc-ar @rsp
>> GNU ar (GNU Binutils for Debian) 2.35.2
>> ...
>>
>>
>> PR gcc-ar/77576
>> * gcc/gcc-ar.cc (main): Expand argv and use
>> temporary response file to call ar if any
>> expansions were made.
>> ---
>> gcc/gcc-ar.cc | 47 +++
>> 1 file changed, 47 insertions(+)
>>
>> diff --git a/gcc/gcc-ar.cc b/gcc/gcc-ar.cc
>> index 4e4c525927d..417c4913793 100644
>> --- a/gcc/gcc-ar.cc
>> +++ b/gcc/gcc-ar.cc
>> @@ -135,6 +135,10 @@ main (int ac, char **av)
>>int k, status, err;
>>const char *err_msg;
>>const char **nargv;
>> + char **old_argv;
>> + const char *rsp_file = NULL;
>> + const char *rsp_arg = NULL;
>> + const char *rsp_argv[3];
>>bool is_ar = !strcmp (PERSONALITY, "ar");
>>int exit_code = FATAL_EXIT_CODE;
>>int i;
>> @@ -209,6 +213,13 @@ main (int ac, char **av)
>> }
>> }
>>
>> + /* Expand any @files before modifying the command line
>> + and use a temporary response file if there were any. */
>> + old_argv = av;
>> + expandargv (&ac, &av);
>> + if (av != old_argv)
>> +rsp_file = make_temp_file ("");
>> +
>>/* Prepend - if necessary. */
>>if (is_ar && av[1] && av[1][0] != '-')
>> av[1] = concat ("-", av[1], NULL);
>> @@ -225,6 +236,39 @@ main (int ac, char **av)
>> nargv[j + k] = av[k];
>>nargv[j + k] = NULL;
>>
>> + /* If @file was passed, put nargv into the temporary response
>> + file and then change it to a single @FILE argument, where
>> + FILE is the temporary filename. */
>> + if (rsp_file)
>> +{
>> + FILE *f;
>> + int status;
>> + f = fopen (rsp_file, "w");
>> + if (f == NULL)
>> +{
>> + fprintf (stderr, "Cannot open temporary file %s\n", rsp_file);
>> + exit (1);
>> +}
>> + status = writeargv (
>> + CONST_CAST2 (char * const *, const char **, nargv) + 1, f);
>> + if (status)
>> +{
>> + fprintf (stderr, "Cannot write to temporary file %s\n",
>> rsp_file);
>> + exit (1);
>> +}
>> + status = fclose (f);
>> + if (EOF == status)
>> +{
>> + fprintf (stderr, "Cannot close temporary file %s\n", rsp_file);
>> + exit (1);
>> +}
>> + rsp_arg = concat ("@", rsp_file, NULL);
>> + rsp_argv[0] = nargv[0];
>> + rsp_argv[1] = rsp_arg;
>> + rsp_argv[2] = NULL;
>> + nargv = rsp_argv;
>> +}
>> +
>>/* Run utility */
>>/* ??? the const is misplaced in pex_one's argv? */
>>err_msg = pex_one (PEX_LAST|PEX_SEARCH,
>> @@ -249,5 +293,8 @@ main (int ac, char **av)
>>else
>> exit_code = SUCCESS_EXIT_CODE;
>>
>> + if (rsp_file)
>> +unlink (rsp_file);
>> +
>>return exit_code;
>> }
>> --
>> 2.30.2
>>
>
Re: [PATCH] gcc-ar: Handle response files properly [PR77576]
ping
On Fri, 14 Jul 2023 at 09:05, Costas Argyris
wrote:
> Pinging to try and get this bug in gcc-ar fixed.
>
> Note that the patch posted as an attachment in
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-July/623400.html
>
> is exactly the same as the patch embedded in
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-July/623855.html
>
> and the one posted in the PR itself
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576
>
> On Fri, 7 Jul 2023 at 13:00, Costas Argyris
> wrote:
>
>> Bootstrapped successfully on x86_64-pc-linux-gnu
>>
>> On Fri, 7 Jul 2023 at 11:33, Costas Argyris
>> wrote:
>>
>>> Problem: gcc-ar fails when a @file is passed to it:
>>>
>>> $ cat rsp
>>> --version
>>> $ gcc-ar @rsp
>>> /usr/bin/ar: invalid option -- '@'
>>>
>>> This is because a dash '-' is prepended to the first
>>> argument if it doesn't start with one, resulting in
>>> the wrong call 'ar -@rsp'.
>>>
>>> Fix: Expand argv to get rid of any @files and if any
>>> expansions were made, pass everything through a
>>> temporary response file.
>>>
>>> $ gcc-ar @rsp
>>> GNU ar (GNU Binutils for Debian) 2.35.2
>>> ...
>>>
>>>
>>> PR gcc-ar/77576
>>> * gcc/gcc-ar.cc (main): Expand argv and use
>>> temporary response file to call ar if any
>>> expansions were made.
>>> ---
>>> gcc/gcc-ar.cc | 47 +++
>>> 1 file changed, 47 insertions(+)
>>>
>>> diff --git a/gcc/gcc-ar.cc b/gcc/gcc-ar.cc
>>> index 4e4c525927d..417c4913793 100644
>>> --- a/gcc/gcc-ar.cc
>>> +++ b/gcc/gcc-ar.cc
>>> @@ -135,6 +135,10 @@ main (int ac, char **av)
>>>int k, status, err;
>>>const char *err_msg;
>>>const char **nargv;
>>> + char **old_argv;
>>> + const char *rsp_file = NULL;
>>> + const char *rsp_arg = NULL;
>>> + const char *rsp_argv[3];
>>>bool is_ar = !strcmp (PERSONALITY, "ar");
>>>int exit_code = FATAL_EXIT_CODE;
>>>int i;
>>> @@ -209,6 +213,13 @@ main (int ac, char **av)
>>> }
>>> }
>>>
>>> + /* Expand any @files before modifying the command line
>>> + and use a temporary response file if there were any. */
>>> + old_argv = av;
>>> + expandargv (&ac, &av);
>>> + if (av != old_argv)
>>> +rsp_file = make_temp_file ("");
>>> +
>>>/* Prepend - if necessary. */
>>>if (is_ar && av[1] && av[1][0] != '-')
>>> av[1] = concat ("-", av[1], NULL);
>>> @@ -225,6 +236,39 @@ main (int ac, char **av)
>>> nargv[j + k] = av[k];
>>>nargv[j + k] = NULL;
>>>
>>> + /* If @file was passed, put nargv into the temporary response
>>> + file and then change it to a single @FILE argument, where
>>> + FILE is the temporary filename. */
>>> + if (rsp_file)
>>> +{
>>> + FILE *f;
>>> + int status;
>>> + f = fopen (rsp_file, "w");
>>> + if (f == NULL)
>>> +{
>>> + fprintf (stderr, "Cannot open temporary file %s\n", rsp_file);
>>> + exit (1);
>>> +}
>>> + status = writeargv (
>>> + CONST_CAST2 (char * const *, const char **, nargv) + 1, f);
>>> + if (status)
>>> +{
>>> + fprintf (stderr, "Cannot write to temporary file %s\n",
>>> rsp_file);
>>> + exit (1);
>>> +}
>>> + status = fclose (f);
>>> + if (EOF == status)
>>> +{
>>> + fprintf (stderr, "Cannot close temporary file %s\n",
>>> rsp_file);
>>> + exit (1);
>>> +}
>>> + rsp_arg = concat ("@", rsp_file, NULL);
>>> + rsp_argv[0] = nargv[0];
>>> + rsp_argv[1] = rsp_arg;
>>> + rsp_argv[2] = NULL;
>>> + nargv = rsp_argv;
>>> +}
>>> +
>>>/* Run utility */
>>>/* ??? the const is misplaced in pex_one's argv? */
>>>err_msg = pex_one (PEX_LAST|PEX_SEARCH,
>>> @@ -249,5 +293,8 @@ main (int ac, char **av)
>>>else
>>> exit_code = SUCCESS_EXIT_CODE;
>>>
>>> + if (rsp_file)
>>> +unlink (rsp_file);
>>> +
>>>return exit_code;
>>> }
>>> --
>>> 2.30.2
>>>
>>
[PATCH] gcc-ar: Handle response files properly [PR77576]
Basically implementing what Andrew said in the PR: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576 If @file has been passed to gcc-ar, do the following: 1) Expand it to get an argv without any @files. 2) Then apply the plugin modifications to argv. 3) Create temporary response file. 4) Put the modified argv in the temporary file. 5) Call ar with @tmp. 6) Delete the temporary response file. 0001-gcc-ar-Handle-response-files-properly-PR77576.patch Description: Binary data
Re: [PATCH] gcc-ar: Handle response files properly [PR77576]
I should also add that for a rsp file that contains just "--version": gcc-ar @rsp fails without the patch (current problem) and successfully prints the version info with it. On Sat, 1 Jul 2023 at 22:45, Costas Argyris wrote: > Basically implementing what Andrew said in the PR: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77576 > > If @file has been passed to gcc-ar, do the following: > > 1) Expand it to get an argv without any @files. > 2) Then apply the plugin modifications to argv. > 3) Create temporary response file. > 4) Put the modified argv in the temporary file. > 5) Call ar with @tmp. > 6) Delete the temporary response file. >
Re: [PATCH] gcc-ar: Handle response files properly [PR77576]
Problem: gcc-ar fails when a @file is passed to it:
$ cat rsp
--version
$ gcc-ar @rsp
/usr/bin/ar: invalid option -- '@'
This is because a dash '-' is prepended to the first
argument if it doesn't start with one, resulting in
the wrong call 'ar -@rsp'.
Fix: Expand argv to get rid of any @files and if any
expansions were made, pass everything through a
temporary response file.
$ gcc-ar @rsp
GNU ar (GNU Binutils for Debian) 2.35.2
...
PR gcc-ar/77576
* gcc/gcc-ar.cc (main): Expand argv and use
temporary response file to call ar if any
expansions were made.
---
gcc/gcc-ar.cc | 47 +++
1 file changed, 47 insertions(+)
diff --git a/gcc/gcc-ar.cc b/gcc/gcc-ar.cc
index 4e4c525927d..417c4913793 100644
--- a/gcc/gcc-ar.cc
+++ b/gcc/gcc-ar.cc
@@ -135,6 +135,10 @@ main (int ac, char **av)
int k, status, err;
const char *err_msg;
const char **nargv;
+ char **old_argv;
+ const char *rsp_file = NULL;
+ const char *rsp_arg = NULL;
+ const char *rsp_argv[3];
bool is_ar = !strcmp (PERSONALITY, "ar");
int exit_code = FATAL_EXIT_CODE;
int i;
@@ -209,6 +213,13 @@ main (int ac, char **av)
}
}
+ /* Expand any @files before modifying the command line
+ and use a temporary response file if there were any. */
+ old_argv = av;
+ expandargv (&ac, &av);
+ if (av != old_argv)
+rsp_file = make_temp_file ("");
+
/* Prepend - if necessary. */
if (is_ar && av[1] && av[1][0] != '-')
av[1] = concat ("-", av[1], NULL);
@@ -225,6 +236,39 @@ main (int ac, char **av)
nargv[j + k] = av[k];
nargv[j + k] = NULL;
+ /* If @file was passed, put nargv into the temporary response
+ file and then change it to a single @FILE argument, where
+ FILE is the temporary filename. */
+ if (rsp_file)
+{
+ FILE *f;
+ int status;
+ f = fopen (rsp_file, "w");
+ if (f == NULL)
+{
+ fprintf (stderr, "Cannot open temporary file %s\n", rsp_file);
+ exit (1);
+}
+ status = writeargv (
+ CONST_CAST2 (char * const *, const char **, nargv) + 1, f);
+ if (status)
+{
+ fprintf (stderr, "Cannot write to temporary file %s\n",
rsp_file);
+ exit (1);
+}
+ status = fclose (f);
+ if (EOF == status)
+{
+ fprintf (stderr, "Cannot close temporary file %s\n", rsp_file);
+ exit (1);
+}
+ rsp_arg = concat ("@", rsp_file, NULL);
+ rsp_argv[0] = nargv[0];
+ rsp_argv[1] = rsp_arg;
+ rsp_argv[2] = NULL;
+ nargv = rsp_argv;
+}
+
/* Run utility */
/* ??? the const is misplaced in pex_one's argv? */
err_msg = pex_one (PEX_LAST|PEX_SEARCH,
@@ -249,5 +293,8 @@ main (int ac, char **av)
else
exit_code = SUCCESS_EXIT_CODE;
+ if (rsp_file)
+unlink (rsp_file);
+
return exit_code;
}
--
2.30.2
Re: [PATCH] gcc-ar: Handle response files properly [PR77576]
Bootstrapped successfully on x86_64-pc-linux-gnu
On Fri, 7 Jul 2023 at 11:33, Costas Argyris
wrote:
> Problem: gcc-ar fails when a @file is passed to it:
>
> $ cat rsp
> --version
> $ gcc-ar @rsp
> /usr/bin/ar: invalid option -- '@'
>
> This is because a dash '-' is prepended to the first
> argument if it doesn't start with one, resulting in
> the wrong call 'ar -@rsp'.
>
> Fix: Expand argv to get rid of any @files and if any
> expansions were made, pass everything through a
> temporary response file.
>
> $ gcc-ar @rsp
> GNU ar (GNU Binutils for Debian) 2.35.2
> ...
>
>
> PR gcc-ar/77576
> * gcc/gcc-ar.cc (main): Expand argv and use
> temporary response file to call ar if any
> expansions were made.
> ---
> gcc/gcc-ar.cc | 47 +++
> 1 file changed, 47 insertions(+)
>
> diff --git a/gcc/gcc-ar.cc b/gcc/gcc-ar.cc
> index 4e4c525927d..417c4913793 100644
> --- a/gcc/gcc-ar.cc
> +++ b/gcc/gcc-ar.cc
> @@ -135,6 +135,10 @@ main (int ac, char **av)
>int k, status, err;
>const char *err_msg;
>const char **nargv;
> + char **old_argv;
> + const char *rsp_file = NULL;
> + const char *rsp_arg = NULL;
> + const char *rsp_argv[3];
>bool is_ar = !strcmp (PERSONALITY, "ar");
>int exit_code = FATAL_EXIT_CODE;
>int i;
> @@ -209,6 +213,13 @@ main (int ac, char **av)
> }
> }
>
> + /* Expand any @files before modifying the command line
> + and use a temporary response file if there were any. */
> + old_argv = av;
> + expandargv (&ac, &av);
> + if (av != old_argv)
> +rsp_file = make_temp_file ("");
> +
>/* Prepend - if necessary. */
>if (is_ar && av[1] && av[1][0] != '-')
> av[1] = concat ("-", av[1], NULL);
> @@ -225,6 +236,39 @@ main (int ac, char **av)
> nargv[j + k] = av[k];
>nargv[j + k] = NULL;
>
> + /* If @file was passed, put nargv into the temporary response
> + file and then change it to a single @FILE argument, where
> + FILE is the temporary filename. */
> + if (rsp_file)
> +{
> + FILE *f;
> + int status;
> + f = fopen (rsp_file, "w");
> + if (f == NULL)
> +{
> + fprintf (stderr, "Cannot open temporary file %s\n", rsp_file);
> + exit (1);
> +}
> + status = writeargv (
> + CONST_CAST2 (char * const *, const char **, nargv) + 1, f);
> + if (status)
> +{
> + fprintf (stderr, "Cannot write to temporary file %s\n",
> rsp_file);
> + exit (1);
> +}
> + status = fclose (f);
> + if (EOF == status)
> +{
> + fprintf (stderr, "Cannot close temporary file %s\n", rsp_file);
> + exit (1);
> +}
> + rsp_arg = concat ("@", rsp_file, NULL);
> + rsp_argv[0] = nargv[0];
> + rsp_argv[1] = rsp_arg;
> + rsp_argv[2] = NULL;
> + nargv = rsp_argv;
> +}
> +
>/* Run utility */
>/* ??? the const is misplaced in pex_one's argv? */
>err_msg = pex_one (PEX_LAST|PEX_SEARCH,
> @@ -249,5 +293,8 @@ main (int ac, char **av)
>else
> exit_code = SUCCESS_EXIT_CODE;
>
> + if (rsp_file)
> +unlink (rsp_file);
> +
>return exit_code;
> }
> --
> 2.30.2
>
[PATCH] libiberty: fix memory leak in pex-win32.c and refactor
Hi
It seems that the win32_spawn function in libiberty/pex-win32.c is leaking
the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).The
problem here is that the cleanup code is written 3 times, one at each exit
scenario.
The proposed attached refactoring has the cleanup code appearing just once
and is executed for all exit scenarios, reducing the likelihood of such
leaks in the future.
Thanks,
Costas
From ca1ff7b48db2e30963bd4c0e08ecd6653214a5db Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Sun, 26 Feb 2023 16:34:11 +
Subject: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor
Fix memory leak of cmdline buffer and refactor to have
cleanup code appear once for all exit cases.
---
libiberty/pex-win32.c | 33 +++--
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index 02d3a3e839b..82303947de4 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -577,14 +577,12 @@ win32_spawn (const char *executable,
LPSTARTUPINFO si,
LPPROCESS_INFORMATION pi)
{
- char *full_executable;
- char *cmdline;
+ char *full_executable = NULL;
+ char *cmdline = NULL;
+ pid_t pid = (pid_t) -1;
char **env_copy;
char *env_block = NULL;
- full_executable = NULL;
- cmdline = NULL;
-
if (env)
{
int env_size;
@@ -622,13 +620,13 @@ win32_spawn (const char *executable,
full_executable = find_executable (executable, search);
if (!full_executable)
-goto error;
+goto exit;
cmdline = argv_to_cmdline (argv);
if (!cmdline)
-goto error;
+goto exit;
/* Create the child process. */
- if (!CreateProcess (full_executable, cmdline,
+ if (CreateProcess (full_executable, cmdline,
/*lpProcessAttributes=*/NULL,
/*lpThreadAttributes=*/NULL,
/*bInheritHandles=*/TRUE,
@@ -638,26 +636,17 @@ win32_spawn (const char *executable,
si,
pi))
{
- free (env_block);
-
- free (full_executable);
-
- return (pid_t) -1;
+ CloseHandle (pi->hThread);
+ pid = (pid_t) pi->hProcess;
}
-
+
+ exit:
/* Clean up. */
- CloseHandle (pi->hThread);
- free (full_executable);
- free (env_block);
-
- return (pid_t) pi->hProcess;
-
- error:
free (env_block);
free (cmdline);
free (full_executable);
- return (pid_t) -1;
+ return pid;
}
/* Spawn a script. This simulates the Unix script execution mechanism.
--
2.30.2
Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor
I forgot to mention that: 1) The CreateProcess documentation https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa doesn't mention anything about taking ownership of this or any other buffer passed to it. 2) The cmdline buffer gets created by the argv_to_cmdline function https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339 which has this comment right above it: /* Return a Windows command-line from ARGV. It is the caller's responsibility to free the string returned. */ Thanks, Costas On Thu, 2 Mar 2023 at 07:32, Richard Biener wrote: > On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches > wrote: > > > > Hi > > > > It seems that the win32_spawn function in libiberty/pex-win32.c is > leaking > > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3). > The > > problem here is that the cleanup code is written 3 times, one at each > exit > > scenario. > > > > The proposed attached refactoring has the cleanup code appearing just > once > > and is executed for all exit scenarios, reducing the likelihood of such > > leaks in the future. > > One could imagine that CreateProcess in case of success takes ownership of > the buffer pointed to by cmdline? If you can confirm it is not then the > patch > looks OK to me. > > Thanks, > Richard. > > > Thanks, > > Costas >
Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor
Thanks for the review. What is the next step please? Thanks, Costas On Thu, 2 Mar 2023 at 10:08, Richard Biener wrote: > On Thu, Mar 2, 2023 at 10:21 AM Costas Argyris > wrote: > > > > I forgot to mention that: > > > > 1) The CreateProcess documentation > > > > > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa > > > > doesn't mention anything about taking ownership of this or any other > buffer passed to it. > > Thanks - thus the patch is OK. > > Thanks, > Richard. > > > 2) The cmdline buffer gets created by the argv_to_cmdline function > > > > https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339 > > > > which has this comment right above it: > > > > /* Return a Windows command-line from ARGV. It is the caller's > >responsibility to free the string returned. */ > > > > Thanks, > > Costas > > > > On Thu, 2 Mar 2023 at 07:32, Richard Biener > wrote: > >> > >> On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches > >> wrote: > >> > > >> > Hi > >> > > >> > It seems that the win32_spawn function in libiberty/pex-win32.c is > leaking > >> > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3). > The > >> > problem here is that the cleanup code is written 3 times, one at each > exit > >> > scenario. > >> > > >> > The proposed attached refactoring has the cleanup code appearing just > once > >> > and is executed for all exit scenarios, reducing the likelihood of > such > >> > leaks in the future. > >> > >> One could imagine that CreateProcess in case of success takes ownership > of > >> the buffer pointed to by cmdline? If you can confirm it is not then > the patch > >> looks OK to me. > >> > >> Thanks, > >> Richard. > >> > >> > Thanks, > >> > Costas >
[PATCH] driver: Treat include path args the same way between cpp_unique_options and asm_options. [PR71850]
This is a proposal to fix PR71850 by applying the existing logic for
passing include paths to cc1 to as.
Thanks,
Costas
From 393aff0d006ee9372cc8b9321c612c2dfb4b0a31 Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Thu, 2 Mar 2023 18:27:22 +
Subject: [PATCH] driver: Treat include path args the same way between
cpp_unique_options and asm_options. [PR71850]
On Windows, when a @file with many include paths is passed to gcc, it forwards those include paths to cc1 through a temporary @file as well, so they don't end up in the command line.This is because cpp_unique_options has %@{I* which passes -I args in a temporary file, if a temporary file was passed to the driver in the first place.
The same logic is not applied in asm_options, and this leads to the include paths being passed as command line arguments to the assembler, which causes the failure on Windows seen in PR71850.
Treating the -I args to the assembler the same way as to the compiler (that is, through a @tempfile if @file was passed to gcc) solves the issue, allowing a large number of include paths to be passed to gcc on Windows through a @file.
---
gcc/gcc.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index becc56051a8..b1fa80cde4f 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -1278,7 +1278,7 @@ static const char *asm_options =
#if HAVE_GNU_AS
/* If GNU AS is used, then convert -w (no warnings), -I, and -v
to the assembler equivalents. */
-"%{v} %{w:-W} %{I*} "
+"%{v} %{w:-W} %@{I*} "
#endif
"%(asm_debug_option)"
ASM_COMPRESS_DEBUG_SPEC
--
2.30.2
Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
Hi This is a proposal for addressing https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108865 by integrating the UTF-8 manifest file into gcc's build process for the 64-bit mingw host. The analysis and discussion leading up to the latest patch are written in the bug report. The patch attached in this email is exactly the same as the one posted last in the bug report. I should also mention that, in case of approval, I would need someone with write access to make the commit for me please. Thanks, Costas 0001-Enable-UTF-8-code-page-on-Windows-64-bit-host-PR1088.patch Description: Binary data
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
Hi Jacek, "Is there a reason to make it specific to x86_64? It seems to me that all mingw hosts could use it." Are you referring to the 32-bit host?My concern here is that this functionality (embedding the UTF-8 manifest file into the executable) is only truly supported in recent versions of Windows.From: https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page It says that Windows Version 1903 (May 2019 Update) enables this, so we are looking at the 64-bit version of Windows. I suppose you are referring to the scenario where one has a 32-bit gcc + mingw running in a 64-bit Windows that is recent enough to support this?It is not clear to me based on the above doc what would happen encoding-wise in that situation, and I haven't tried it either because I assumed that most people would want the 64-bit version of gcc since they are probably running a 64-bit OS. If you think it is useful, I could look into that as a separate task to try and keep this one simple, if that makes sense. "I think that .manifest file should also be a dependency here." Why is that?Windres takes only the .rc file as its input, as per its own doc, and it successfully compiles it into an object file.The .manifest file is only referenced by the .rc file, and it doesn't get passed to windres, so I don't see why it has to be listed as a prerequisite in the make rule. Thanks, Costas On Tue, 7 Mar 2023 at 12:02, Jacek Caban wrote: > Hi Costas, > > On 3/7/23 01:52, Costas Argyris via Gcc-patches wrote: > > This is a proposal for addressing > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108865 > > by integrating the UTF-8 manifest file into gcc's build process for the > 64-bit mingw host. > > > Is there a reason to make it specific to x86_64? It seems to me that all > mingw hosts could use it. > > > > +# The resource .rc file references the utf8 .manifest file. > +# Compile it into an object file using windres. > +# The resulting .o file gets added to host_extra_gcc_objs in > +# config.host for x86_64-*-mingw* host and gets linked into > +# the driver as a .o file, so it's lack of symbols is OK. > +utf8rc-mingw32.o : $(srcdir)/config/i386/utf8-mingw32.rc > + $(WINDRES) $< $@ > > > I think that .manifest file should also be a dependency here. > > > Thanks, > > Jacek >
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
Hi Jacek, "but I think it should work just fine if you didn't explicitly limit the patch to x86_64." I would think so too. Actually, even cygwin might benefit from this, assuming it has the same problem, which I don't know if it's the case. But I'm not experienced with that so I would like to explore these hosts separately and just focus on the most common 64-bit Windows host with this change, if possible. "The point that when winnt-utf8.manifest is modified, utf8-mingw32.o should be rebuilt." Right, makes sense. Just noting that winnt-utf8.manifest is really not meant to be modified, because it is copied straight from: https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page and will probably remain like that, but I do get your point and I am happy to make the change. Thanks, Costas On Tue, 7 Mar 2023 at 14:18, Jacek Caban wrote: > Hi Costas, > > On 3/7/23 15:00, Costas Argyris wrote: > > Hi Jacek, > > > > "Is there a reason to make it specific to x86_64? It seems to me that > > all mingw hosts could use it." > > > > Are you referring to the 32-bit host?My concern here is that this > > functionality (embedding the UTF-8 > > manifest file into the executable) is only truly supported in recent > > versions of Windows.From: > > > > > https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page > > > > It says that Windows Version 1903 (May 2019 Update) enables this, so > > we are looking at the 64-bit > > version of Windows. > > > > I suppose you are referring to the scenario where one has a 32-bit > > gcc + mingw running in a 64-bit > > Windows that is recent enough to support this?It is not clear to > > me based on the above doc what > > would happen encoding-wise in that situation, and I haven't tried it > > either because I assumed that > > most people would want the 64-bit version of gcc since they are > > probably running a 64-bit OS. > > > > If you think it is useful, I could look into that as a separate task > > to try and keep this one simple, if > > that makes sense. > > > Yes, realistically it's mostly about 32-bit gcc on 64-bit Windows > (perhaps aarch64 as well at some point in the future). It's probably > indeed not very popular configuration those days, but I think it should > work just fine if you didn't explicitly limit the patch to x86_64. > > > > "I think that .manifest file should also be a dependency here." > > > > Why is that?Windres takes only the .rc file as its input, as per > > its own doc, and it successfully > > compiles it into an object file.The .manifest file is only > > referenced by the .rc file, and it doesn't > > get passed to windres, so I don't see why it has to be listed as a > > prerequisite in the make rule. > > > The point that when winnt-utf8.manifest is modified, utf8-mingw32.o > should be rebuilt. Anyway, it's probably not a big deal (I should > disclaim that I'm not very familiar with gcc build system; I'm mostly on > this ML due to mingw-w64 contributions). > > > Thanks, > > Jacek > >
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
Added .manifest file to the make rule for utf8rc-mingw32.o, latest patch attached. On Tue, 7 Mar 2023 at 15:27, Costas Argyris wrote: > Hi Jacek, > > "but I think it should work just fine if you didn't explicitly limit the > patch to x86_64." > > I would think so too. > > Actually, even cygwin might benefit from this, assuming it has the same > problem, which I don't know if it's the case. > > But I'm not experienced with that so I would like to explore these hosts > separately and just focus on the most common 64-bit Windows host with this > change, if possible. > > "The point that when winnt-utf8.manifest is modified, utf8-mingw32.o > should be rebuilt." > > Right, makes sense. > > Just noting that winnt-utf8.manifest is really not meant to be modified, > because it is copied straight from: > > > https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page > > and will probably remain like that, but I do get your point and I am happy > to make the change. > > Thanks, > Costas > > On Tue, 7 Mar 2023 at 14:18, Jacek Caban wrote: > >> Hi Costas, >> >> On 3/7/23 15:00, Costas Argyris wrote: >> > Hi Jacek, >> > >> > "Is there a reason to make it specific to x86_64? It seems to me that >> > all mingw hosts could use it." >> > >> > Are you referring to the 32-bit host?My concern here is that this >> > functionality (embedding the UTF-8 >> > manifest file into the executable) is only truly supported in recent >> > versions of Windows.From: >> > >> > >> https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page >> > >> > It says that Windows Version 1903 (May 2019 Update) enables this, so >> > we are looking at the 64-bit >> > version of Windows. >> > >> > I suppose you are referring to the scenario where one has a 32-bit >> > gcc + mingw running in a 64-bit >> > Windows that is recent enough to support this?It is not clear to >> > me based on the above doc what >> > would happen encoding-wise in that situation, and I haven't tried it >> > either because I assumed that >> > most people would want the 64-bit version of gcc since they are >> > probably running a 64-bit OS. >> > >> > If you think it is useful, I could look into that as a separate task >> > to try and keep this one simple, if >> > that makes sense. >> >> >> Yes, realistically it's mostly about 32-bit gcc on 64-bit Windows >> (perhaps aarch64 as well at some point in the future). It's probably >> indeed not very popular configuration those days, but I think it should >> work just fine if you didn't explicitly limit the patch to x86_64. >> >> >> > "I think that .manifest file should also be a dependency here." >> > >> > Why is that?Windres takes only the .rc file as its input, as per >> > its own doc, and it successfully >> > compiles it into an object file.The .manifest file is only >> > referenced by the .rc file, and it doesn't >> > get passed to windres, so I don't see why it has to be listed as a >> > prerequisite in the make rule. >> >> >> The point that when winnt-utf8.manifest is modified, utf8-mingw32.o >> should be rebuilt. Anyway, it's probably not a big deal (I should >> disclaim that I'm not very familiar with gcc build system; I'm mostly on >> this ML due to mingw-w64 contributions). >> >> >> Thanks, >> >> Jacek >> >> From 694d6f4860a08f690070df411f3f72d66a48a981 Mon Sep 17 00:00:00 2001 From: Costas Argyris Date: Tue, 28 Feb 2023 17:10:18 + Subject: [PATCH] Enable UTF-8 code page on Windows 64-bit host [PR108865] Compile a resource object that contains the utf8 manifest. Then link that object into the driver and compiler proper. For compiler proper the link has to be forced because the resource object file gets into a static library (libbackend.a) and gets eventually dropped because it has no symbols of its own and nothing is referencing it inside the library. Therefore, an artificial symbol is planted to force the link. --- gcc/config.host | 5 ++- gcc/config/i386/sym-mingw32.cc | 1 + gcc/config/i386/utf8-mingw32.rc | 3 ++ gcc/config/i386/winnt-utf8.manifest | 8 gcc/config/i386/x-mingw32 | 3 +- gcc/config/i386/x-mingw32-utf8 | 57 + 6 files changed, 73 insertions(+), 4 deletions(-) create mode 1006
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
Pinging the list and mingw maintainer. Analysis and pre-approval here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108865 On Wed, 8 Mar 2023 at 10:52, Costas Argyris wrote: > Added .manifest file to the make rule for utf8rc-mingw32.o, latest patch > attached. > > On Tue, 7 Mar 2023 at 15:27, Costas Argyris > wrote: > >> Hi Jacek, >> >> "but I think it should work just fine if you didn't explicitly limit the >> patch to x86_64." >> >> I would think so too. >> >> Actually, even cygwin might benefit from this, assuming it has the same >> problem, which I don't know if it's the case. >> >> But I'm not experienced with that so I would like to explore these hosts >> separately and just focus on the most common 64-bit Windows host with this >> change, if possible. >> >> "The point that when winnt-utf8.manifest is modified, utf8-mingw32.o >> should be rebuilt." >> >> Right, makes sense. >> >> Just noting that winnt-utf8.manifest is really not meant to be modified, >> because it is copied straight from: >> >> >> https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page >> >> and will probably remain like that, but I do get your point and I am >> happy to make the change. >> >> Thanks, >> Costas >> >> On Tue, 7 Mar 2023 at 14:18, Jacek Caban wrote: >> >>> Hi Costas, >>> >>> On 3/7/23 15:00, Costas Argyris wrote: >>> > Hi Jacek, >>> > >>> > "Is there a reason to make it specific to x86_64? It seems to me that >>> > all mingw hosts could use it." >>> > >>> > Are you referring to the 32-bit host?My concern here is that this >>> > functionality (embedding the UTF-8 >>> > manifest file into the executable) is only truly supported in recent >>> > versions of Windows.From: >>> > >>> > >>> https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page >>> > >>> > It says that Windows Version 1903 (May 2019 Update) enables this, so >>> > we are looking at the 64-bit >>> > version of Windows. >>> > >>> > I suppose you are referring to the scenario where one has a 32-bit >>> > gcc + mingw running in a 64-bit >>> > Windows that is recent enough to support this?It is not clear to >>> > me based on the above doc what >>> > would happen encoding-wise in that situation, and I haven't tried it >>> > either because I assumed that >>> > most people would want the 64-bit version of gcc since they are >>> > probably running a 64-bit OS. >>> > >>> > If you think it is useful, I could look into that as a separate task >>> > to try and keep this one simple, if >>> > that makes sense. >>> >>> >>> Yes, realistically it's mostly about 32-bit gcc on 64-bit Windows >>> (perhaps aarch64 as well at some point in the future). It's probably >>> indeed not very popular configuration those days, but I think it should >>> work just fine if you didn't explicitly limit the patch to x86_64. >>> >>> >>> > "I think that .manifest file should also be a dependency here." >>> > >>> > Why is that?Windres takes only the .rc file as its input, as per >>> > its own doc, and it successfully >>> > compiles it into an object file.The .manifest file is only >>> > referenced by the .rc file, and it doesn't >>> > get passed to windres, so I don't see why it has to be listed as a >>> > prerequisite in the make rule. >>> >>> >>> The point that when winnt-utf8.manifest is modified, utf8-mingw32.o >>> should be rebuilt. Anyway, it's probably not a big deal (I should >>> disclaim that I'm not very familiar with gcc build system; I'm mostly on >>> this ML due to mingw-w64 contributions). >>> >>> >>> Thanks, >>> >>> Jacek >>> >>> From 694d6f4860a08f690070df411f3f72d66a48a981 Mon Sep 17 00:00:00 2001 From: Costas Argyris Date: Tue, 28 Feb 2023 17:10:18 + Subject: [PATCH] Enable UTF-8 code page on Windows 64-bit host [PR108865] Compile a resource object that contains the utf8 manifest. Then link that object into the driver and compiler proper. For compiler proper the link has to be forced because the resource obj
Re: [PATCH] driver: Treat include path args the same way between cpp_unique_options and asm_options. [PR71850]
Pinging list and driver reviewer.
Details here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71850
On Thu, 2 Mar 2023 at 19:25, Costas Argyris
wrote:
> This is a proposal to fix PR71850 by applying the existing logic for
> passing include paths to cc1 to as.
>
> Thanks,
> Costas
>
From 393aff0d006ee9372cc8b9321c612c2dfb4b0a31 Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Thu, 2 Mar 2023 18:27:22 +
Subject: [PATCH] driver: Treat include path args the same way between
cpp_unique_options and asm_options. [PR71850]
On Windows, when a @file with many include paths is passed to gcc, it forwards those include paths to cc1 through a temporary @file as well, so they don't end up in the command line.This is because cpp_unique_options has %@{I* which passes -I args in a temporary file, if a temporary file was passed to the driver in the first place.
The same logic is not applied in asm_options, and this leads to the include paths being passed as command line arguments to the assembler, which causes the failure on Windows seen in PR71850.
Treating the -I args to the assembler the same way as to the compiler (that is, through a @tempfile if @file was passed to gcc) solves the issue, allowing a large number of include paths to be passed to gcc on Windows through a @file.
---
gcc/gcc.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index becc56051a8..b1fa80cde4f 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -1278,7 +1278,7 @@ static const char *asm_options =
#if HAVE_GNU_AS
/* If GNU AS is used, then convert -w (no warnings), -I, and -v
to the assembler equivalents. */
-"%{v} %{w:-W} %{I*} "
+"%{v} %{w:-W} %@{I*} "
#endif
"%(asm_debug_option)"
ASM_COMPRESS_DEBUG_SPEC
--
2.30.2
[PATCH] libiberty: On Windows pass a >32k cmdline through a response file.
Currently on Windows, when CreateProcess is called with a command-line
that exceeds the 32k Windows limit, we get a very bad error:
"CreateProcess: No such file or directory"
This patch detects the case where this would happen and writes the
long command-line to a temporary response file and calls CreateProcess
with @file instead.
From 5c7237c102cdaca34e5907cd25c31610bda51919 Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Mon, 22 May 2023 13:55:56 +0100
Subject: [PATCH] libiberty: On Windows, pass a >32k cmdline through a response
file.
pex-win32.c (win32_spawn): If the command line for CreateProcess
exceeds the 32k Windows limit, try to store it in a temporary
response file and call CreateProcess with @file instead (PR71850).
Signed-off-by: Costas Argyris
---
libiberty/pex-win32.c | 57 +--
1 file changed, 44 insertions(+), 13 deletions(-)
diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index 23c6c190a2c..0fd8b38734c 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -569,7 +569,8 @@ env_compare (const void *a_ptr, const void *b_ptr)
* target is not actually an executable, such as if it is a shell script. */
static pid_t
-win32_spawn (const char *executable,
+win32_spawn (struct pex_obj *obj,
+ const char *executable,
BOOL search,
char *const *argv,
char *const *env, /* array of strings of the form: VAR=VALUE */
@@ -624,8 +625,37 @@ win32_spawn (const char *executable,
cmdline = argv_to_cmdline (argv);
if (!cmdline)
goto exit;
-
- /* Create the child process. */
+ /* If cmdline is too large, CreateProcess will fail with a bad
+ 'No such file or directory' error. Try passing it through a
+ temporary response file instead. */
+ if (strlen (cmdline) > 32767)
+{
+ char *response_file = make_temp_file ("");
+ /* Register the file for deletion by pex_free. */
+ ++obj->remove_count;
+ obj->remove = XRESIZEVEC (char *, obj->remove, obj->remove_count);
+ obj->remove[obj->remove_count - 1] = response_file;
+ int fd = pex_win32_open_write (obj, response_file, 0, 0);
+ if (fd == -1)
+goto exit;
+ FILE *f = pex_win32_fdopenw (obj, fd, 0);
+ /* Don't write argv[0] (program name) to the response file. */
+ if (writeargv (&argv[1], f))
+{
+ fclose (f);
+ goto exit;
+}
+ fclose (f); /* Also closes fd and the underlying OS handle. */
+ char *response_arg = concat ("@", response_file, NULL);
+ char *response_argv[3] = {argv[0], response_arg, NULL};
+ free (cmdline);
+ cmdline = argv_to_cmdline (response_argv);
+ free (response_arg);
+ if (!cmdline)
+goto exit;
+}
+
+ /* Create the child process. */
if (CreateProcess (full_executable, cmdline,
/*lpProcessAttributes=*/NULL,
/*lpThreadAttributes=*/NULL,
@@ -645,7 +675,7 @@ win32_spawn (const char *executable,
free (env_block);
free (cmdline);
free (full_executable);
-
+
return pid;
}
@@ -653,7 +683,8 @@ win32_spawn (const char *executable,
This function is called as a fallback if win32_spawn fails. */
static pid_t
-spawn_script (const char *executable, char *const *argv,
+spawn_script (struct pex_obj *obj,
+ const char *executable, char *const *argv,
char* const *env,
DWORD dwCreationFlags,
LPSTARTUPINFO si,
@@ -703,20 +734,20 @@ spawn_script (const char *executable, char *const *argv,
executable = strrchr (executable1, '\\') + 1;
if (!executable)
executable = executable1;
- pid = win32_spawn (executable, TRUE, argv, env,
+ pid = win32_spawn (obj, executable, TRUE, argv, env,
dwCreationFlags, si, pi);
#else
if (strchr (executable1, '\\') == NULL)
- pid = win32_spawn (executable1, TRUE, argv, env,
+ pid = win32_spawn (obj, executable1, TRUE, argv, env,
dwCreationFlags, si, pi);
else if (executable1[0] != '\\')
- pid = win32_spawn (executable1, FALSE, argv, env,
+ pid = win32_spawn (obj, executable1, FALSE, argv, env,
dwCreationFlags, si, pi);
else
{
const char *newex = mingw_rootify (executable1);
*avhere = newex;
- pid = win32_spawn (newex, FALSE, argv, env,
+ pid = win32_spawn (obj, newex, FALSE, argv, env,
dwCreationFlags, si, pi);
if (executable1 != newex)
free ((char *) newex);
@@ -726,7 +757,7 @@ spawn_script (const char *executable, char *const *argv,
if (newex != executable1)
{
*avhere = newex;
- pid = win32_spawn (newex, FALSE, argv, env,
+ pid = win32_spawn (obj, newex, FALSE, argv, env,
dwCreationFlags, si, pi);
free ((char *) newex);
}
@@ -745,7 +776,7 @@ spawn_script (const char *executable, char *const *argv,
/
Re: [PATCH] driver: Treat include path args the same way between cpp_unique_options and asm_options. [PR71850]
ping On Thu, 9 Mar 2023 at 13:39, Costas Argyris wrote: > Pinging list and driver reviewer. > > Details here: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71850 > > On Thu, 2 Mar 2023 at 19:25, Costas Argyris > wrote: > >> This is a proposal to fix PR71850 by applying the existing logic for >> passing include paths to cc1 to as. >> >> Thanks, >> Costas >> >
[PATCH] Fix native MSYS2 build failure [PR108865, PR109188]
Patch to fix native Windows MSYS2 package build failures. The patch has been confirmed by both original reporters in the PRs, and myself. cc'd mingw-w64 maintainer for mingw-w64-specific issue. 0001-Fix-native-MSYS2-build-failure-PR108865-PR109188.patch Description: Binary data
Re: [PATCH] driver: Treat include path args the same way between cpp_unique_options and asm_options. [PR71850]
[ping^3] This looks like it fixes the bug and also unifies the way include paths are passed from the driver to the compiler and assembler (when a @file has been passed to the driver in the first place). That is, when @file has been passed to the driver, put the include paths in a temp @file and pass them to the assembler.Note this is already happening for the compiler, so this patch merely extends this logic to the assembler. Is there any reason not to go for it? On Mon, 20 Mar 2023 at 09:47, Costas Argyris wrote: > ping > > On Thu, 9 Mar 2023 at 13:39, Costas Argyris > wrote: > >> Pinging list and driver reviewer. >> >> Details here: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71850 >> >> On Thu, 2 Mar 2023 at 19:25, Costas Argyris >> wrote: >> >>> This is a proposal to fix PR71850 by applying the existing logic for >>> passing include paths to cc1 to as. >>> >>> Thanks, >>> Costas >>> >>
Re: [PATCH] driver: Treat include path args the same way between cpp_unique_options and asm_options. [PR71850]
Would it be possible to make it version-dependent, then? As in, if GNU assembler is greater or equal to the version that supports @FILE, then pass @FILE to it, otherwise fall back to the current behavior. I assume most people nowadays would have a version of Binutils later than 2005, but if we could make it conditional on the version then even those with earlier version wouldn't break, they would just get the current behavior. On Mon, 27 Mar 2023 at 11:00, Xi Ruoyao wrote: > On Mon, 2023-03-27 at 10:36 +0100, Costas Argyris via Gcc-patches wrote: > > [ping^3] > > > > This looks like it fixes the bug and also unifies the way include paths > are > > passed from the driver to the compiler and assembler (when a @file has > > been passed to the driver in the first place). > > > > That is, when @file has been passed to the driver, put the include paths > > in a temp @file and pass them to the assembler.Note this is already > > happening for the compiler, so this patch merely extends this logic to > the > > assembler. > > > > Is there any reason not to go for it? > > It's not supported by all GNU assembler releases. For example, GCC > installation doc says we require Binutils >= 2.13.1 for i?86-*-linux*. > Binutils 2.13.1 was released in 2002, but @FILE support was added into > Binutils in 2005. > > > > > -- > Xi Ruoyao > School of Aerospace Science and Technology, Xidian University >
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
The patch attached to this email extends the UTF-8 support of the
driver and compiler processes to the 32-bit mingw host.Initially,
only the 64-bit host got it.
About the changes in sym-mingw32.cc:
Even though the 64-bit host was building fine with the symbol being
simply declared as a char, the 32-bit host was failing to find the
symbol at link time because a leading underscore was being added
to it by the compiler.The asm keyword ensures that the symbol
always appears with that exact name, such that the linker will
always find it.
The patch also includes Jacek's flag about adding the .manifest file
as a prerequisite for the object file (this was actually done from before
but an earlier version of the patch was pushed so it was missed).
Tested building from master for both 32 and 64-bit mingw hosts using:
1) cross-compilation from a Debian machine using configure + make
2) native-compilation from a Windows machine using MSYS2
On Thu, 9 Mar 2023 at 15:03, Jonathan Yong <10wa...@gmail.com> wrote:
> On 3/9/23 13:33, Costas Argyris wrote:
> > Pinging the list and mingw maintainer.
> >
> > Analysis and pre-approval here:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108865
> >
>
> Thanks, pushed to master branch.
>
>
>
From 0ed08739f44116eaef46b552df923959ba945afa Mon Sep 17 00:00:00 2001
From: Costas Argyris
Date: Sun, 26 Mar 2023 11:32:13 +0100
Subject: [PATCH] Extend UTF-8 support to the 32-bit mingw host.
Prevent any name mangling in HOST_EXTRA_OBJS_SYMBOL
such that the linker always finds it by that name.
Also add the .manifest file as an explicit
dependency in the make rule such that the
object gets re-built if it changes.
---
gcc/config.host| 5 +++--
gcc/config/i386/sym-mingw32.cc | 4 +++-
gcc/config/i386/x-mingw32-utf8 | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/gcc/config.host b/gcc/config.host
index 4abb32ad73d..5df85752ed4 100644
--- a/gcc/config.host
+++ b/gcc/config.host
@@ -232,10 +232,11 @@ case ${host} in
;;
i[34567]86-*-mingw32*)
host_xm_file=i386/xm-mingw32.h
-host_xmake_file="${host_xmake_file} i386/x-mingw32"
+host_xmake_file="${host_xmake_file} i386/x-mingw32 i386/x-mingw32-utf8"
host_exeext=.exe
out_host_hook_obj=host-mingw32.o
-host_extra_gcc_objs="${host_extra_gcc_objs} driver-mingw32.o"
+host_extra_objs="${host_extra_objs} utf8-mingw32.o"
+host_extra_gcc_objs="${host_extra_gcc_objs} driver-mingw32.o utf8rc-mingw32.o"
host_lto_plugin_soname=liblto_plugin.dll
;;
x86_64-*-mingw*)
diff --git a/gcc/config/i386/sym-mingw32.cc b/gcc/config/i386/sym-mingw32.cc
index f369698abc4..2f8dee6c1ec 100644
--- a/gcc/config/i386/sym-mingw32.cc
+++ b/gcc/config/i386/sym-mingw32.cc
@@ -1 +1,3 @@
-char HOST_EXTRA_OBJS_SYMBOL;
+/* Prevent any name mangling to make sure that the linker
+ will always find the symbol. */
+char HOST_EXTRA_OBJS_SYMBOL asm ("HOST_EXTRA_OBJS_SYMBOL");
diff --git a/gcc/config/i386/x-mingw32-utf8 b/gcc/config/i386/x-mingw32-utf8
index 9de963d7965..cf5c3db3d8b 100644
--- a/gcc/config/i386/x-mingw32-utf8
+++ b/gcc/config/i386/x-mingw32-utf8
@@ -27,7 +27,8 @@
# The resulting .o file gets added to host_extra_gcc_objs in
# config.host for x86_64-*-mingw* host and gets linked into
# the driver as a .o file, so it's lack of symbols is OK.
-utf8rc-mingw32.o : $(srcdir)/config/i386/utf8-mingw32.rc
+utf8rc-mingw32.o : $(srcdir)/config/i386/utf8-mingw32.rc \
+ $(srcdir)/config/i386/winnt-utf8.manifest
$(WINDRES) $< $@
# Create an object file that just exports the global symbol
--
2.30.2
Re: Enable UTF-8 code page in driver and compiler on 64-bit mingw host [PR108865]
I forgot to update the relevant comments with the previous patch. This is a comment-only patch that brings them up-to-date. On Tue, 28 Mar 2023 at 09:05, Jonathan Yong <10wa...@gmail.com> wrote: > On 3/27/23 17:17, Costas Argyris wrote: > > The patch attached to this email extends the UTF-8 support of the > > driver and compiler processes to the 32-bit mingw host.Initially, > > only the 64-bit host got it. > > > > About the changes in sym-mingw32.cc: > > > > Even though the 64-bit host was building fine with the symbol being > > simply declared as a char, the 32-bit host was failing to find the > > symbol at link time because a leading underscore was being added > > to it by the compiler.The asm keyword ensures that the symbol > > always appears with that exact name, such that the linker will > > always find it. > > > > The patch also includes Jacek's flag about adding the .manifest file > > as a prerequisite for the object file (this was actually done from before > > but an earlier version of the patch was pushed so it was missed). > > > > Tested building from master for both 32 and 64-bit mingw hosts using: > > > > 1) cross-compilation from a Debian machine using configure + make > > 2) native-compilation from a Windows machine using MSYS2 > > > Thanks, approved and pushed to master branch. > > From 87da0323ec5b08d44f17713d8d4e19664d7a3aa6 Mon Sep 17 00:00:00 2001 From: Costas Argyris Date: Tue, 28 Mar 2023 11:29:06 +0100 Subject: [PATCH] mingw: Fix comments in x-mingw32-utf8 This is a comment-only change that I should have done with the previous commit (304c7d44a) but forgot to do so. --- gcc/config/i386/x-mingw32-utf8 | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/gcc/config/i386/x-mingw32-utf8 b/gcc/config/i386/x-mingw32-utf8 index cf5c3db3d8b..2783dd259a6 100644 --- a/gcc/config/i386/x-mingw32-utf8 +++ b/gcc/config/i386/x-mingw32-utf8 @@ -17,15 +17,15 @@ # <http://www.gnu.org/licenses/>. # # -# For 64-bit Windows host, embed a manifest that sets the active +# For mingw Windows hosts, embed a manifest that sets the active # code page of the driver and compiler proper processes to utf8. -# This only has an effect on Windows version 1903 (May 2019 Update) -# or later. +# This only has an effect when gcc is hosted on Windows version +# 1903 (May 2019 Update) or later. # The resource .rc file references the utf8 .manifest file. # Compile it into an object file using windres. # The resulting .o file gets added to host_extra_gcc_objs in -# config.host for x86_64-*-mingw* host and gets linked into +# config.host for mingw hosts and gets linked into # the driver as a .o file, so it's lack of symbols is OK. utf8rc-mingw32.o : $(srcdir)/config/i386/utf8-mingw32.rc \ $(srcdir)/config/i386/winnt-utf8.manifest @@ -39,7 +39,7 @@ sym-mingw32.o : $(srcdir)/config/i386/sym-mingw32.cc # Combine the two object files into one which has both the # compiled utf8 resource and the HOST_EXTRA_OBJS_SYMBOL symbol. # The resulting .o file gets added to host_extra_objs in -# config.host for x86_64-*-mingw* host and gets archived into +# config.host for mingw hosts and gets archived into # libbackend.a which gets linked into the compiler proper. # If nothing references it into libbackend.a, it will not # get linked into the compiler proper eventually. @@ -54,4 +54,8 @@ utf8-mingw32.o : utf8rc-mingw32.o sym-mingw32.o # This is expected because the resource object is not supposed # to have any symbols, it just has to be linked into the # executable in order for Windows to use the utf8 code page. +# Some build environments are passing these flags to other +# programs as well, so make the symbol definition optional +# such that these programs don't fail to build when they +# don't find it. $(COMPILERS) : override LDFLAGS += -Wl,--undefined=HOST_EXTRA_OBJS_SYMBOL -- 2.30.2
[PATCH] mingw: Support building with older gcc versions
This is proposed to fix PR109460 where an older version of gcc (7.3) was used to build for windows (mingw) host. From e5b608072f80a83cca65e88bb75ecc62ab0bbb87 Mon Sep 17 00:00:00 2001 From: Costas Argyris Date: Wed, 12 Apr 2023 08:48:18 +0100 Subject: [PATCH] mingw: Support building with older gcc versions The $@ argument to the compiler is causing only a warning in some gcc versions but an error in others. In any case, $@ was never necessary so remove it completely, just like the rules in x-mingw32 where the object file gets named after the source file. This fixes both warnings and errors about sym-mingw32.o appearing in the command line unnecessarily. The -nostdlib flag is required along with -r for older gcc versions that don't apply it automatically with -r, resulting in main functions erroneously entering a partial link. --- gcc/config/i386/x-mingw32-utf8 | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gcc/config/i386/x-mingw32-utf8 b/gcc/config/i386/x-mingw32-utf8 index 2783dd259a6..b5a6cfcf702 100644 --- a/gcc/config/i386/x-mingw32-utf8 +++ b/gcc/config/i386/x-mingw32-utf8 @@ -34,7 +34,7 @@ utf8rc-mingw32.o : $(srcdir)/config/i386/utf8-mingw32.rc \ # Create an object file that just exports the global symbol # HOST_EXTRA_OBJS_SYMBOL sym-mingw32.o : $(srcdir)/config/i386/sym-mingw32.cc - $(COMPILER) -c $< $@ + $(COMPILER) -c $< # Combine the two object files into one which has both the # compiled utf8 resource and the HOST_EXTRA_OBJS_SYMBOL symbol. @@ -44,8 +44,10 @@ sym-mingw32.o : $(srcdir)/config/i386/sym-mingw32.cc # If nothing references it into libbackend.a, it will not # get linked into the compiler proper eventually. # Therefore we need to request the symbol at compiler link time. +# -nostdlib is required for supporting old gcc versions that +# don't apply it automatically with -r. utf8-mingw32.o : utf8rc-mingw32.o sym-mingw32.o - $(COMPILER) -r utf8rc-mingw32.o sym-mingw32.o -o $@ + $(COMPILER) -r -nostdlib utf8rc-mingw32.o sym-mingw32.o -o $@ # Force compilers to link against the utf8 resource by # requiring the symbol to be defined. -- 2.30.2
