[PATCH] Remove size limit of PCH

[PATCH] Remove size limit of PCH

[LIU Hao]

[-- Attachment #1.1.1: Type: text/plain, Size: 432 bytes --]

Remove the limit and solve https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14940.

I have tested the patch on `x86_64-w64-mingw32` with MSVCRT, by pre-compiling a header of many 
standard and boost headers to generate a 313-MiB-large .gch file and using it to compile a simple 
'hello world' program, and seen no problems so far.


After eighteen years this has eventually been settled. (sigh)


-- 
Best regards,
LIU Hao

[-- Attachment #1.1.2: 0010-Fix-using-large-PCH.patch --]
[-- Type: text/plain, Size: 1990 bytes --]

From c084ab275f47cc27621e664d1c63e177525cf321 Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Tue, 10 May 2022 13:19:07 +0800
Subject: [PATCH] Remove size limit of PCH

Reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14940
Signed-off-by: LIU Hao <lh_mouse@126.com>
---
 gcc/config/i386/host-mingw32.cc | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/gcc/config/i386/host-mingw32.cc b/gcc/config/i386/host-mingw32.cc
index 3b0d83ffc60..f915b85abd0 100644
--- a/gcc/config/i386/host-mingw32.cc
+++ b/gcc/config/i386/host-mingw32.cc
@@ -44,9 +44,6 @@ static size_t mingw32_gt_pch_alloc_granularity (void);
 
 static inline void w32_error(const char*, const char*, int, const char*);
 
-/* FIXME: Is this big enough?  */
-static const size_t pch_VA_max_size  = 128 * 1024 * 1024;
-
 /* Granularity for reserving address space.  */
 static size_t va_granularity = 0x10000;
 
@@ -88,9 +85,6 @@ static void *
 mingw32_gt_pch_get_address (size_t size, int)
 {
   void* res;
-  size = (size + va_granularity - 1) & ~(va_granularity - 1);
-  if (size > pch_VA_max_size)
-    return NULL;
 
   /* FIXME: We let system determine base by setting first arg to NULL.
      Allocating at top of available address space avoids unnecessary
@@ -100,7 +94,7 @@ mingw32_gt_pch_get_address (size_t size, int)
      If we allocate at bottom we need to reserve the address as early
      as possible and at the same point in each invocation. */
  
-  res = VirtualAlloc (NULL, pch_VA_max_size,
+  res = VirtualAlloc (NULL, size,
 		      MEM_RESERVE | MEM_TOP_DOWN,
 		      PAGE_NOACCESS);
   if (!res)
@@ -150,7 +144,7 @@ mingw32_gt_pch_use_address (void *&addr, size_t size, int fd,
 
   /* Offset must be also be a multiple of allocation granularity for
      this to work.  We can't change the offset. */ 
-  if ((offset & (va_granularity - 1)) != 0 || size > pch_VA_max_size)
+  if ((offset & (va_granularity - 1)) != 0)
     return -1;
 
 
-- 
2.36.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] Remove size limit of PCH

[Xi Ruoyao]

On Tue, 2022-05-10 at 19:35 +0800, LIU Hao via Gcc-patches wrote:

> Subject: [PATCH] Remove size limit of PCH

Make it "Remove size limit of PCH [PR14940]", so once it's committed a
message will show up in bugzilla.

> Reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14940
> Signed-off-by: LIU Hao <lh_mouse@126.com>

You'll need to add yourself into "Contributing under the DCO" section in
MAINTAINERS file first to use Signed-off-by, I guess.

A ChangeLog entry should be included in the commit message like

gcc/

	PR pch/14940
	* config/i386/host-mingw32.cc (pch_VA_max_size): Remove.
	(mingw32_gt_pch_get_address): ... ...

I know almost nothing about Windoge/MinGW so the technical review is
left for others.
-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Remove size limit of PCH

[LIU Hao]

[-- Attachment #1.1.1: Type: text/plain, Size: 348 bytes --]

在 2022-05-10 20:00, Xi Ruoyao 写道:
> On Tue, 2022-05-10 at 19:35 +0800, LIU Hao via Gcc-patches wrote:
> 
>> Subject: [PATCH] Remove size limit of PCH
> 
> Make it "Remove size limit of PCH [PR14940]", so once it's committed a
> message will show up in bugzilla.
> 

Here is the revised patch.





-- 
Best regards,
LIU Hao

[-- Attachment #1.1.2: 0001-PR14940-Remove-size-limit-of-PCH.patch --]
[-- Type: text/plain, Size: 2106 bytes --]

From 2e314baed84fd80b3b3c4c67787a032b86dd54dc Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Tue, 10 May 2022 13:19:07 +0800
Subject: [PATCH] [PR14940] Remove size limit of PCH

There shouldn't be such a limit in practice.

2022-05-03  LIU Hao <lh_mouse@126.com>

	PR pch/14940
	* config/i386/host-mingw32.cc (pch_VA_max_size): Remove.
	(mingw32_gt_pch_get_address): Remove size limit.
---
 gcc/config/i386/host-mingw32.cc | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/gcc/config/i386/host-mingw32.cc b/gcc/config/i386/host-mingw32.cc
index 3b0d83ffc606..f915b85abd06 100644
--- a/gcc/config/i386/host-mingw32.cc
+++ b/gcc/config/i386/host-mingw32.cc
@@ -44,9 +44,6 @@ static size_t mingw32_gt_pch_alloc_granularity (void);
 
 static inline void w32_error(const char*, const char*, int, const char*);
 
-/* FIXME: Is this big enough?  */
-static const size_t pch_VA_max_size  = 128 * 1024 * 1024;
-
 /* Granularity for reserving address space.  */
 static size_t va_granularity = 0x10000;
 
@@ -88,9 +85,6 @@ static void *
 mingw32_gt_pch_get_address (size_t size, int)
 {
   void* res;
-  size = (size + va_granularity - 1) & ~(va_granularity - 1);
-  if (size > pch_VA_max_size)
-    return NULL;
 
   /* FIXME: We let system determine base by setting first arg to NULL.
      Allocating at top of available address space avoids unnecessary
@@ -100,7 +94,7 @@ mingw32_gt_pch_get_address (size_t size, int)
      If we allocate at bottom we need to reserve the address as early
      as possible and at the same point in each invocation. */
  
-  res = VirtualAlloc (NULL, pch_VA_max_size,
+  res = VirtualAlloc (NULL, size,
 		      MEM_RESERVE | MEM_TOP_DOWN,
 		      PAGE_NOACCESS);
   if (!res)
@@ -150,7 +144,7 @@ mingw32_gt_pch_use_address (void *&addr, size_t size, int fd,
 
   /* Offset must be also be a multiple of allocation granularity for
      this to work.  We can't change the offset. */ 
-  if ((offset & (va_granularity - 1)) != 0 || size > pch_VA_max_size)
+  if ((offset & (va_granularity - 1)) != 0)
     return -1;
 
 
-- 
2.36.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] Remove size limit of PCH

[Jakub Jelinek]

On Tue, May 10, 2022 at 09:30:03PM +0800, LIU Hao via Gcc-patches wrote:
> From 2e314baed84fd80b3b3c4c67787a032b86dd54dc Mon Sep 17 00:00:00 2001
> From: LIU Hao <lh_mouse@126.com>
> Date: Tue, 10 May 2022 13:19:07 +0800
> Subject: [PATCH] [PR14940] Remove size limit of PCH
> 
> There shouldn't be such a limit in practice.
> 
> 2022-05-03  LIU Hao <lh_mouse@126.com>
> 
> 	PR pch/14940
> 	* config/i386/host-mingw32.cc (pch_VA_max_size): Remove.
> 	(mingw32_gt_pch_get_address): Remove size limit.

This looks reasonable, but doesn't contain the most important part.
As mentioned in https://gcc.gnu.org/r12-5855
the generic part can now support relocation of PCH, so it is fine if it is
mapped at some other address, it is preferrable if it is mapped at the same
address as it was written for, but if not, the generic code can relocate it.

So, beyond your changes, I'd suggest to change:
  /* Retry five times, as here might occure a race with multiple gcc's
     instances at same time.  */
  for (r = 0; r < 5; r++)
   {
      mmap_addr = MapViewOfFileEx (mmap_handle, FILE_MAP_COPY, 0, offset,
                                   size, addr);
      if (mmap_addr == addr)
        break;
      if (r != 4)
        Sleep (500);
   }

  if (mmap_addr != addr)
    {
      w32_error (__FUNCTION__, __FILE__, __LINE__, "MapViewOfFileEx");
      CloseHandle(mmap_handle);
      return  -1;
    }

IMHO you can just drop the loop, sleep of half a second is almost certainly
slower than the relocation handling, so I'd just
  mmap_addr = MapViewOfFileEx (mmap_handle, FILE_MAP_COPY, 0, offset,
			       size, addr);
  if (mmap_addr == NULL)
    {
      w32_error (__FUNCTION__, __FILE__, __LINE__, "MapViewOfFileEx");
      CloseHandle(mmap_handle);
      return -1;
    }

  addr = mmap_addr;

This, if it mmaps the file at the right address, nice, if not, let the
caller know (through updating addr) that it needs to relocate it,
but if the mapping failed, fail.

	Jakub

Re: [PATCH] Remove size limit of PCH

[Jonathan Yong]

[-- Attachment #1.1.1: Type: text/plain, Size: 433 bytes --]

On 5/10/22 13:30, LIU Hao wrote:
> 在 2022-05-10 20:00, Xi Ruoyao 写道:
>> On Tue, 2022-05-10 at 19:35 +0800, LIU Hao via Gcc-patches wrote:
>>
>>> Subject: [PATCH] Remove size limit of PCH
>>
>> Make it "Remove size limit of PCH [PR14940]", so once it's committed a
>> message will show up in bugzilla.
>>
> 
> Here is the revised patch.
> 
> 
> 
> 
> 

Thanks, I will commit soon if there are no objections.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 8075 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] Remove size limit of PCH

[LIU Hao]

[-- Attachment #1.1: Type: text/plain, Size: 821 bytes --]

在 2022-05-10 22:28, Jakub Jelinek 写道:
> 
> This looks reasonable, but doesn't contain the most important part.
> As mentioned in https://gcc.gnu.org/r12-5855
> the generic part can now support relocation of PCH, so it is fine if it is
> mapped at some other address, it is preferrable if it is mapped at the same
> address as it was written for, but if not, the generic code can relocate it.
> 
> (... ...)
> 
> IMHO you can just drop the loop, sleep of half a second is almost certainly
> slower than the relocation handling, so I'd just

Thanks for the hint. I have very little knowledge about how PCH works in GCC, thus I'm gonna propose 
a patch to MSYS2 and see how it works. This can be committed to GCC later, after having been tested 
through many packages.


-- 
Best regards,
LIU Hao

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]