| Message ID | 20230916010009.3672058-1-lhyatt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | libcpp: Fix ICE on #include after a line marker directive [PR61474] | expand |
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
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
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 --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