Re: [PATCH] [PR libcpp/71681] Fix handling header.gcc in subdirectories

[Jeff Law]

On 10/06/2016 11:37 AM, Andris Pavenis wrote:

On 09/08/2016 12:09 PM, Thomas Schwinge wrote:

Hi!

A few review comments:

On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis
 wrote:

This patch fixes handling header.gcc in subdirectories when command
line option -remap has been
used.

(I have not yet looked up what that mechanism actually does.)  ;-)


Current version finds header.gcc in directories from GCC include
directory search path but
fails to find them in subdirectories due to missing directory separator.

Can you provide some test cases? (Ah, I now see you got some "Test
script to reproduce problem" attached to  --
this should be turned into a regression test for the GCC testsuite.)


Separate patch with test-cases is in the attachment.

I added also test for header.gcc directly in directory on GCC includes
search path. This case have always worked but there is no test-case for
it in gcc test-suite yet.
Other test case (pr71681-2.c) fails without initial patch for
libcpp/files.c
(https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00395.html) applied but
passes after it is applied.

I would prefer to applied this new patch at first and initial patch
after that if they are acceptable.

Andris

ChangeLog entry for new patch:

2016-10-06  Andris Pavenis   Add test-cases for
pr71681

* gcc.dg/cpp/pr71681-1.c: New testcase (-remap, header.gcc in directory
on includes search path)
* gcc.dg/cpp/pr71681-2.c: New testcase (-remap, header.gcc in subdirectory)
* gcc.dg/cpp/remap/header.gcc: File for added test-cases
* gcc.dg/cpp/remap/a/header.gcc: Likewise
* gcc.dg/cpp/remap/a/t_1.h: Likewise
* gcc.dg/cpp/remap/a/t_2.h: Likewise
The testsuite fixes are fine.  However, please don't check them in until 
you've also checked in the actual fix to remap_filename.


In your change to remap_file you have:

> +  if (dir->len && !IS_DIR_SEPARATOR(dir->name[dir->len - 1]))
Please insert a space after IS_DIR_SEPARATOR, but before the open 
parenthesis.


Your change to remap_filename is OK with that formatting nit fixed.

Thanks for your patience,

jeff

Re: [PATCH] [PR libcpp/71681] Fix handling header.gcc in subdirectories

[Andris Pavenis]

On 09/08/2016 12:09 PM, Thomas Schwinge wrote:

Hi!

A few review comments:

On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis  wrote:

This patch fixes handling header.gcc in subdirectories when command line option 
-remap has been
used.

(I have not yet looked up what that mechanism actually does.)  ;-)


Current version finds header.gcc in directories from GCC include directory 
search path but
fails to find them in subdirectories due to missing directory separator.

Can you provide some test cases? (Ah, I now see you got some "Test
script to reproduce problem" attached to  --
this should be turned into a regression test for the GCC testsuite.)


Separate patch with test-cases is in the attachment.

I added also test for header.gcc directly in directory on GCC includes search path. This case have 
always worked but there is no test-case for it in gcc test-suite yet.
Other test case (pr71681-2.c) fails without initial patch for libcpp/files.c 
(https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00395.html) applied but passes after it is applied.


I would prefer to applied this new patch at first and initial patch after that 
if they are acceptable.

Andris

ChangeLog entry for new patch:

2016-10-06  Andris Pavenis   Add test-cases for pr71681

* gcc.dg/cpp/pr71681-1.c: New testcase (-remap, header.gcc in directory on 
includes search path)
* gcc.dg/cpp/pr71681-2.c: New testcase (-remap, header.gcc in subdirectory)
* gcc.dg/cpp/remap/header.gcc: File for added test-cases
* gcc.dg/cpp/remap/a/header.gcc: Likewise
* gcc.dg/cpp/remap/a/t_1.h: Likewise
* gcc.dg/cpp/remap/a/t_2.h: Likewise

>From 2aed23fd806e8140c92f7681935ba0b9724f16e5 Mon Sep 17 00:00:00 2001
From: Andris Pavenis 
Date: Wed, 5 Oct 2016 21:17:36 +0300
Subject: [PATCH 1/2] Add test-cases for pr71681

* gcc.dg/cpp/pr71681-1.c: New testcase
* gcc.dg/cpp/pr71681-2.c: Likewise
* gcc.dg/cpp/remap/header.gcc: File for added test-cases
* gcc.dg/cpp/remap/a/header.gcc: Likewise
* gcc.dg/cpp/remap/a/t_1.h: Likewise
* gcc.dg/cpp/remap/a/t_2.h: Likewise
---
 gcc/testsuite/gcc.dg/cpp/pr71681-1.c| 5 +
 gcc/testsuite/gcc.dg/cpp/pr71681-2.c| 5 +
 gcc/testsuite/gcc.dg/cpp/remap/a/header.gcc | 1 +
 gcc/testsuite/gcc.dg/cpp/remap/a/t_1.h  | 1 +
 gcc/testsuite/gcc.dg/cpp/remap/a/t_2.h  | 1 +
 gcc/testsuite/gcc.dg/cpp/remap/header.gcc   | 1 +
 6 files changed, 14 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/pr71681-1.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/pr71681-2.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/remap/a/header.gcc
 create mode 100644 gcc/testsuite/gcc.dg/cpp/remap/a/t_1.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/remap/a/t_2.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/remap/header.gcc

diff --git a/gcc/testsuite/gcc.dg/cpp/pr71681-1.c b/gcc/testsuite/gcc.dg/cpp/pr71681-1.c
new file mode 100644
index 000..a185351
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/pr71681-1.c
@@ -0,0 +1,5 @@
+// PR preprocessor/71681
+// { dg-do preprocess }
+// { dg-options "-remap -I$srcdir/gcc.dg/cpp/remap" }
+
+#include "a/t1.h"
diff --git a/gcc/testsuite/gcc.dg/cpp/pr71681-2.c b/gcc/testsuite/gcc.dg/cpp/pr71681-2.c
new file mode 100644
index 000..162e366
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/pr71681-2.c
@@ -0,0 +1,5 @@
+// PR preprocessor/71681
+// { dg-do preprocess }
+// { dg-options "-remap -I$srcdir/gcc.dg/cpp/remap" }
+
+#include "a/t2.h"
diff --git a/gcc/testsuite/gcc.dg/cpp/remap/a/header.gcc b/gcc/testsuite/gcc.dg/cpp/remap/a/header.gcc
new file mode 100644
index 000..a0e2b7e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/remap/a/header.gcc
@@ -0,0 +1 @@
+t2.h t_2.h
diff --git a/gcc/testsuite/gcc.dg/cpp/remap/a/t_1.h b/gcc/testsuite/gcc.dg/cpp/remap/a/t_1.h
new file mode 100644
index 000..600cfce8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/remap/a/t_1.h
@@ -0,0 +1 @@
+/* Test file for cpp option -remap test */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/cpp/remap/a/t_2.h b/gcc/testsuite/gcc.dg/cpp/remap/a/t_2.h
new file mode 100644
index 000..600cfce8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/remap/a/t_2.h
@@ -0,0 +1 @@
+/* Test file for cpp option -remap test */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/cpp/remap/header.gcc b/gcc/testsuite/gcc.dg/cpp/remap/header.gcc
new file mode 100644
index 000..0331f89
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/remap/header.gcc
@@ -0,0 +1 @@
+a/t1.h a/t_1.h
-- 
2.7.4

Re: [PATCH] [PR libcpp/71681] Fix handling header.gcc in subdirectories

[Andris Pavenis]

On 09/08/2016 12:09 PM, Thomas Schwinge wrote:

Hi!

A few review comments:

On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis  wrote:

This patch fixes handling header.gcc in subdirectories when command line option 
-remap has been
used.

(I have not yet reviewed the logic of your change itself.)  Wouldn't it
be simpler to just unconditionally add a directory separator here?

Is it OK to assume that a directory separator always is "/"?  (Think DOS,
Windows etc.  But maybe there's some "translation layer" beneath this --
I don't know.)

No.

DJGPP supports both '/' and '\'. '\' is OK except in some cases (special handling of paths 
beginning with /dev/). Blind conversion off all '/' to '\' do not work for DJGPP due to this reason

(had related problems in directory gcc/ada).

Windows targets (WINGW, Cygwin): at least in Ada gcc/ada/s-os_lib.adb converts all '/' to '\' for 
Windows targets and without submitted but not yet committed patch also for DJGPP (that broke 
bootstrapping gcc for DJGPP due to gnatmake not working). I have not done however real testing for 
Windows targets myself.

Can you provide some test cases?  (Ah, I now see you got some "Test
script to reproduce problem" attached to  --
this should be turned into a regression test for the GCC testsuite.)

Which could more appropriate place for test-case?
- gcc/testsuite/c-c++-common/cpp
- gcc/testsuite/gcc.dg/cpp

Also should this test be in a separate subdirectory under either of them?

It is good practice to assign a GCC PR to yourself if you're working on
it, and it also helps to post to the PR a comment with a link to the
mailing list archive for patch submissions, etc.

Done

Andris

Re: [PATCH] [PR libcpp/71681] Fix handling header.gcc in subdirectories

[Thomas Schwinge]

Hi!

A few review comments:

On Wed, 7 Sep 2016 20:19:20 +0300, Andris Pavenis  wrote:
> This patch fixes handling header.gcc in subdirectories when command line 
> option -remap has been 
> used.

(I have not yet looked up what that mechanism actually does.)  ;-)

> Current version finds header.gcc in directories from GCC include directory 
> search path but 
> fails to find them in subdirectories due to missing directory separator.

> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -1672,7 +1672,7 @@ static char *
>  remap_filename (cpp_reader *pfile, _cpp_file *file)
>  {
>const char *fname, *p;
> -  char *new_dir;
> +  char *new_dir, *p3;
>cpp_dir *dir;
>size_t index, len;
>  
> @@ -1701,9 +1701,15 @@ remap_filename (cpp_reader *pfile, _cpp_file *file)
>   return NULL;
>  
>len = dir->len + (p - fname + 1);
> -  new_dir = XNEWVEC (char, len + 1);
> +  new_dir = XNEWVEC (char, len + 2);
> +  p3 = new_dir + dir->len;
>memcpy (new_dir, dir->name, dir->len);
> -  memcpy (new_dir + dir->len, fname, p - fname + 1);
> +  if (dir->len && !IS_DIR_SEPARATOR(dir->name[dir->len - 1]))
> + {
> +   *p3++ = '/';
> +   len++;
> + }
> +  memcpy (p3, fname, p - fname + 1);
>new_dir[len] = '\0';
>  
>dir = make_cpp_dir (pfile, new_dir, dir->sysp);

(I have not yet reviewed the logic of your change itself.)  Wouldn't it
be simpler to just unconditionally add a directory separator here?

Is it OK to assume that a directory separator always is "/"?  (Think DOS,
Windows etc.  But maybe there's some "translation layer" beneath this --
I don't know.)


Can you provide some test cases?  (Ah, I now see you got some "Test
script to reproduce problem" attached to  --
this should be turned into a regression test for the GCC testsuite.)


It is good practice to assign a GCC PR to yourself if you're working on
it, and it also helps to post to the PR a comment with a link to the
mailing list archive for patch submissions, etc.


Grüße
 Thomas


signature.asc
Description: PGP signature