diff mbox series

libcpp: Fix ICE on #include after a line marker directive [PR61474]

Message ID 20230916010009.3672058-1-lhyatt@gmail.com
State Accepted
Headers
Series libcpp: Fix ICE on #include after a line marker directive [PR61474] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Lewis Hyatt Sept. 16, 2023, 1 a.m. UTC 
  Hello-

This fixes an old PR, bootstrap + regtest on x86-64 Linux. Please let me know if it's ok? Thanks!

-Lewis

-- >8 --

As noted in the PR, GCC will segfault if a file name is first seen in a
linemarker directive, and then later seen in a normal #include.  This is
because the fake include process adds the file to the cache with a null PATH
member. The normal #include finds this file in the cache and then attempts
to use the null PATH.  Resolve by adding the file to the cache with a unique
starting directory, so that the fake entry will only be found by a
subsequent fake include, not by a real one.

libcpp/ChangeLog:

	PR preprocessor/61474
	* files.cc (_cpp_find_file): Set DONT_READ to TRUE for fake
	include files.
	(_cpp_fake_include): Pass a unique cpp_dir* address so
	the fake file will not be found when looked up for real.

gcc/testsuite/ChangeLog:

	PR preprocessor/61474
	* c-c++-common/cpp/pr61474-2.h: New test.
	* c-c++-common/cpp/pr61474.c: New test.
	* c-c++-common/cpp/pr61474.h: New test.
---
 libcpp/files.cc                            | 11 +++++++++--
 gcc/testsuite/c-c++-common/cpp/pr61474-2.h |  1 +
 gcc/testsuite/c-c++-common/cpp/pr61474.c   |  5 +++++
 gcc/testsuite/c-c++-common/cpp/pr61474.h   |  6 ++++++
 4 files changed, 21 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474-2.h
 create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.c
 create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.h

Comments

Richard Sandiford Sept. 19, 2023, 5:08 p.m. UTC | #1 
Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hello-
>
> This fixes an old PR, bootstrap + regtest on x86-64 Linux. Please let me know if it's ok? Thanks!
>
> -Lewis
>
> -- >8 --
>
> As noted in the PR, GCC will segfault if a file name is first seen in a
> linemarker directive, and then later seen in a normal #include.  This is
> because the fake include process adds the file to the cache with a null PATH
> member. The normal #include finds this file in the cache and then attempts
> to use the null PATH.  Resolve by adding the file to the cache with a unique
> starting directory, so that the fake entry will only be found by a
> subsequent fake include, not by a real one.
>
> libcpp/ChangeLog:
>
> 	PR preprocessor/61474
> 	* files.cc (_cpp_find_file): Set DONT_READ to TRUE for fake
> 	include files.
> 	(_cpp_fake_include): Pass a unique cpp_dir* address so
> 	the fake file will not be found when looked up for real.
>
> gcc/testsuite/ChangeLog:
>
> 	PR preprocessor/61474
> 	* c-c++-common/cpp/pr61474-2.h: New test.
> 	* c-c++-common/cpp/pr61474.c: New test.
> 	* c-c++-common/cpp/pr61474.h: New test.

Neat fix!  I don't know this code very well, but I agree it looks
correct.  OK if no-one objects in 24 hours.

Thanks,
Richard

> ---
>  libcpp/files.cc                            | 11 +++++++++--
>  gcc/testsuite/c-c++-common/cpp/pr61474-2.h |  1 +
>  gcc/testsuite/c-c++-common/cpp/pr61474.c   |  5 +++++
>  gcc/testsuite/c-c++-common/cpp/pr61474.h   |  6 ++++++
>  4 files changed, 21 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474-2.h
>  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.c
>  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.h
>
> diff --git a/libcpp/files.cc b/libcpp/files.cc
> index 43a8894b7de..27301d79fa4 100644
> --- a/libcpp/files.cc
> +++ b/libcpp/files.cc
> @@ -541,7 +541,9 @@ _cpp_find_file (cpp_reader *pfile, const char *fname, cpp_dir *start_dir,
>      = (kind == _cpp_FFK_PRE_INCLUDE
>         || (pfile->buffer && pfile->buffer->file->implicit_preinclude));
>  
> -  if (kind != _cpp_FFK_FAKE)
> +  if (kind == _cpp_FFK_FAKE)
> +    file->dont_read = true;
> +  else
>      /* Try each path in the include chain.  */
>      for (;;)
>        {
> @@ -1490,7 +1492,12 @@ cpp_clear_file_cache (cpp_reader *pfile)
>  void
>  _cpp_fake_include (cpp_reader *pfile, const char *fname)
>  {
> -  _cpp_find_file (pfile, fname, pfile->buffer->file->dir, 0, _cpp_FFK_FAKE, 0);
> +  /* It does not matter what are the contents of fake_source_dir, it will never
> +     be inspected; we just use its address to uniquely signify that this file
> +     was added as a fake include, so a later call to _cpp_find_file (to include
> +     the file for real) won't find the fake one in the hash table.  */
> +  static cpp_dir fake_source_dir;
> +  _cpp_find_file (pfile, fname, &fake_source_dir, 0, _cpp_FFK_FAKE, 0);
>  }
>  
>  /* Not everyone who wants to set system-header-ness on a buffer can
> diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474-2.h b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
> new file mode 100644
> index 00000000000..6f70f09beec
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
> @@ -0,0 +1 @@
> +#pragma once
> diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.c b/gcc/testsuite/c-c++-common/cpp/pr61474.c
> new file mode 100644
> index 00000000000..f835a40fc7a
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/cpp/pr61474.c
> @@ -0,0 +1,5 @@
> +/* { dg-do preprocess } */
> +#include "pr61474.h"
> +/* Make sure that the file can be included for real, after it was
> +   fake-included by the linemarker directives in pr61474.h.  */
> +#include "pr61474-2.h"
> diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.h b/gcc/testsuite/c-c++-common/cpp/pr61474.h
> new file mode 100644
> index 00000000000..d9e8c3a1fec
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/cpp/pr61474.h
> @@ -0,0 +1,6 @@
> +/* Create a fake include for pr61474-2.h and exercise looking it up.  */
> +/* Use #pragma once to check also that the fake-include entry in the file
> +   cache does not cause a problem in libcpp/files.cc:has_unique_contents().  */
> +#pragma once
> +# 1 "pr61474-2.h" 1
> +# 2 "pr61474-2.h" 1
Marek Polacek Sept. 19, 2023, 5:12 p.m. UTC | #2 
On Tue, Sep 19, 2023 at 06:08:50PM +0100, Richard Sandiford wrote:
> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > Hello-
> >
> > This fixes an old PR, bootstrap + regtest on x86-64 Linux. Please let me know if it's ok? Thanks!
> >
> > -Lewis
> >
> > -- >8 --
> >
> > As noted in the PR, GCC will segfault if a file name is first seen in a
> > linemarker directive, and then later seen in a normal #include.  This is
> > because the fake include process adds the file to the cache with a null PATH
> > member. The normal #include finds this file in the cache and then attempts
> > to use the null PATH.  Resolve by adding the file to the cache with a unique
> > starting directory, so that the fake entry will only be found by a
> > subsequent fake include, not by a real one.
> >
> > libcpp/ChangeLog:
> >
> > 	PR preprocessor/61474
> > 	* files.cc (_cpp_find_file): Set DONT_READ to TRUE for fake
> > 	include files.
> > 	(_cpp_fake_include): Pass a unique cpp_dir* address so
> > 	the fake file will not be found when looked up for real.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 	PR preprocessor/61474
> > 	* c-c++-common/cpp/pr61474-2.h: New test.
> > 	* c-c++-common/cpp/pr61474.c: New test.
> > 	* c-c++-common/cpp/pr61474.h: New test.
> 
> Neat fix!  I don't know this code very well, but I agree it looks
> correct.  OK if no-one objects in 24 hours.

Looks fine to me too, thanks Lewis.
 
> Thanks,
> Richard
> 
> > ---
> >  libcpp/files.cc                            | 11 +++++++++--
> >  gcc/testsuite/c-c++-common/cpp/pr61474-2.h |  1 +
> >  gcc/testsuite/c-c++-common/cpp/pr61474.c   |  5 +++++
> >  gcc/testsuite/c-c++-common/cpp/pr61474.h   |  6 ++++++
> >  4 files changed, 21 insertions(+), 2 deletions(-)
> >  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474-2.h
> >  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.c
> >  create mode 100644 gcc/testsuite/c-c++-common/cpp/pr61474.h
> >
> > diff --git a/libcpp/files.cc b/libcpp/files.cc
> > index 43a8894b7de..27301d79fa4 100644
> > --- a/libcpp/files.cc
> > +++ b/libcpp/files.cc
> > @@ -541,7 +541,9 @@ _cpp_find_file (cpp_reader *pfile, const char *fname, cpp_dir *start_dir,
> >      = (kind == _cpp_FFK_PRE_INCLUDE
> >         || (pfile->buffer && pfile->buffer->file->implicit_preinclude));
> >  
> > -  if (kind != _cpp_FFK_FAKE)
> > +  if (kind == _cpp_FFK_FAKE)
> > +    file->dont_read = true;
> > +  else
> >      /* Try each path in the include chain.  */
> >      for (;;)
> >        {
> > @@ -1490,7 +1492,12 @@ cpp_clear_file_cache (cpp_reader *pfile)
> >  void
> >  _cpp_fake_include (cpp_reader *pfile, const char *fname)
> >  {
> > -  _cpp_find_file (pfile, fname, pfile->buffer->file->dir, 0, _cpp_FFK_FAKE, 0);
> > +  /* It does not matter what are the contents of fake_source_dir, it will never
> > +     be inspected; we just use its address to uniquely signify that this file
> > +     was added as a fake include, so a later call to _cpp_find_file (to include
> > +     the file for real) won't find the fake one in the hash table.  */
> > +  static cpp_dir fake_source_dir;
> > +  _cpp_find_file (pfile, fname, &fake_source_dir, 0, _cpp_FFK_FAKE, 0);
> >  }
> >  
> >  /* Not everyone who wants to set system-header-ness on a buffer can
> > diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474-2.h b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
> > new file mode 100644
> > index 00000000000..6f70f09beec
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
> > @@ -0,0 +1 @@
> > +#pragma once
> > diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.c b/gcc/testsuite/c-c++-common/cpp/pr61474.c
> > new file mode 100644
> > index 00000000000..f835a40fc7a
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/cpp/pr61474.c
> > @@ -0,0 +1,5 @@
> > +/* { dg-do preprocess } */
> > +#include "pr61474.h"
> > +/* Make sure that the file can be included for real, after it was
> > +   fake-included by the linemarker directives in pr61474.h.  */
> > +#include "pr61474-2.h"
> > diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.h b/gcc/testsuite/c-c++-common/cpp/pr61474.h
> > new file mode 100644
> > index 00000000000..d9e8c3a1fec
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/cpp/pr61474.h
> > @@ -0,0 +1,6 @@
> > +/* Create a fake include for pr61474-2.h and exercise looking it up.  */
> > +/* Use #pragma once to check also that the fake-include entry in the file
> > +   cache does not cause a problem in libcpp/files.cc:has_unique_contents().  */
> > +#pragma once
> > +# 1 "pr61474-2.h" 1
> > +# 2 "pr61474-2.h" 1
> 

Marek
Lewis Hyatt Sept. 19, 2023, 11 p.m. UTC | #3 
On Tue, Sep 19, 2023 at 1:13 PM Marek Polacek <polacek@redhat.com> wrote:
>
> On Tue, Sep 19, 2023 at 06:08:50PM +0100, Richard Sandiford wrote:
> > Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > > Hello-
> > >
> > > This fixes an old PR, bootstrap + regtest on x86-64 Linux. Please let me know if it's ok? Thanks!
> > >
> > > -Lewis
> > >
> > > -- >8 --
> > >
> > > As noted in the PR, GCC will segfault if a file name is first seen in a
> > > linemarker directive, and then later seen in a normal #include.  This is
> > > because the fake include process adds the file to the cache with a null PATH
> > > member. The normal #include finds this file in the cache and then attempts
> > > to use the null PATH.  Resolve by adding the file to the cache with a unique
> > > starting directory, so that the fake entry will only be found by a
> > > subsequent fake include, not by a real one.
> > >
> > > libcpp/ChangeLog:
> > >
> > >     PR preprocessor/61474
> > >     * files.cc (_cpp_find_file): Set DONT_READ to TRUE for fake
> > >     include files.
> > >     (_cpp_fake_include): Pass a unique cpp_dir* address so
> > >     the fake file will not be found when looked up for real.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >     PR preprocessor/61474
> > >     * c-c++-common/cpp/pr61474-2.h: New test.
> > >     * c-c++-common/cpp/pr61474.c: New test.
> > >     * c-c++-common/cpp/pr61474.h: New test.
> >
> > Neat fix!  I don't know this code very well, but I agree it looks
> > correct.  OK if no-one objects in 24 hours.
>
> Looks fine to me too, thanks Lewis.

Thank you both, much appreciated. I will push it tomorrow evening then.

-Lewis
diff mbox series

Patch

diff --git a/libcpp/files.cc b/libcpp/files.cc
index 43a8894b7de..27301d79fa4 100644
--- a/libcpp/files.cc
+++ b/libcpp/files.cc
@@ -541,7 +541,9 @@  _cpp_find_file (cpp_reader *pfile, const char *fname, cpp_dir *start_dir,
     = (kind == _cpp_FFK_PRE_INCLUDE
        || (pfile->buffer && pfile->buffer->file->implicit_preinclude));
 
-  if (kind != _cpp_FFK_FAKE)
+  if (kind == _cpp_FFK_FAKE)
+    file->dont_read = true;
+  else
     /* Try each path in the include chain.  */
     for (;;)
       {
@@ -1490,7 +1492,12 @@  cpp_clear_file_cache (cpp_reader *pfile)
 void
 _cpp_fake_include (cpp_reader *pfile, const char *fname)
 {
-  _cpp_find_file (pfile, fname, pfile->buffer->file->dir, 0, _cpp_FFK_FAKE, 0);
+  /* It does not matter what are the contents of fake_source_dir, it will never
+     be inspected; we just use its address to uniquely signify that this file
+     was added as a fake include, so a later call to _cpp_find_file (to include
+     the file for real) won't find the fake one in the hash table.  */
+  static cpp_dir fake_source_dir;
+  _cpp_find_file (pfile, fname, &fake_source_dir, 0, _cpp_FFK_FAKE, 0);
 }
 
 /* Not everyone who wants to set system-header-ness on a buffer can
diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474-2.h b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
new file mode 100644
index 00000000000..6f70f09beec
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/pr61474-2.h
@@ -0,0 +1 @@ 
+#pragma once
diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.c b/gcc/testsuite/c-c++-common/cpp/pr61474.c
new file mode 100644
index 00000000000..f835a40fc7a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/pr61474.c
@@ -0,0 +1,5 @@ 
+/* { dg-do preprocess } */
+#include "pr61474.h"
+/* Make sure that the file can be included for real, after it was
+   fake-included by the linemarker directives in pr61474.h.  */
+#include "pr61474-2.h"
diff --git a/gcc/testsuite/c-c++-common/cpp/pr61474.h b/gcc/testsuite/c-c++-common/cpp/pr61474.h
new file mode 100644
index 00000000000..d9e8c3a1fec
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/pr61474.h
@@ -0,0 +1,6 @@ 
+/* Create a fake include for pr61474-2.h and exercise looking it up.  */
+/* Use #pragma once to check also that the fake-include entry in the file
+   cache does not cause a problem in libcpp/files.cc:has_unique_contents().  */
+#pragma once
+# 1 "pr61474-2.h" 1
+# 2 "pr61474-2.h" 1