public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* include path on windows
@ 2008-05-20 12:42 Nathan Sidwell
  2008-05-20 14:05 ` Andrew Pinski
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Nathan Sidwell @ 2008-05-20 12:42 UTC (permalink / raw)
  To: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

This patch fixes include path checking on non-inode file systems.  The current 
behaviour is to presume all -I include paths are unique on such systems.  We 
therefore fail to spot users trying to reorder the system include paths. 
However, we can do better than that by case-insensitive string compare.  That 
won't catch all duplicates, but does improve the situation.

tested on linux and windows hosts.

ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery


[-- Attachment #2: inc-path.patch --]
[-- Type: text/x-patch, Size: 2939 bytes --]

2008-05-20  Nathan Sidwell  <nathan@codesourcery.com>

	* c-incpath.c (INO_T_EQ): Do not define on non-inode systems.
	(DIRS_EQ): New.
	(remove_duplicates): Do not set inode on non-inode systems.  Use
	DIRS_EQ.

Index: c-incpath.c
===================================================================
--- c-incpath.c	(revision 135602)
+++ c-incpath.c	(working copy)
@@ -37,15 +37,18 @@
 #ifdef VMS
 # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
 # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
-#else
-# if (defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__
-#  define INO_T_EQ(A, B) 0
-# else
-#  define INO_T_EQ(A, B) ((A) == (B))
-# endif
+#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
+# define INO_T_EQ(A, B) ((A) == (B))
 # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
 #endif
 
+#if defined INO_T_EQ
+#define DIRS_EQ(A, B) ((A)->dev == (B)->dev \
+	&& INO_T_EQ((A)->ino, (B)->ino))
+#else
+#define DIRS_EQ(A, B) (!strcasecmp ((A)->name, (B)->name))
+#endif
+
 static const char dir_separator_str[] = { DIR_SEPARATOR, 0 };
 
 static void add_env_var_paths (const char *, int);
@@ -241,14 +244,15 @@ remove_duplicates (cpp_reader *pfile, st
 			     "%s: not a directory", cur->name);
       else
 	{
+#if defined (INO_T_COPY)
 	  INO_T_COPY (cur->ino, st.st_ino);
 	  cur->dev  = st.st_dev;
+#endif
 
 	  /* Remove this one if it is in the system chain.  */
 	  reason = REASON_DUP_SYS;
 	  for (tmp = system; tmp; tmp = tmp->next)
-	   if (INO_T_EQ (tmp->ino, cur->ino) && tmp->dev == cur->dev
-	       && cur->construct == tmp->construct)
+	   if (DIRS_EQ (tmp, cur) && cur->construct == tmp->construct)
 	      break;
 
 	  if (!tmp)
@@ -256,16 +260,14 @@ remove_duplicates (cpp_reader *pfile, st
 	      /* Duplicate of something earlier in the same chain?  */
 	      reason = REASON_DUP;
 	      for (tmp = head; tmp != cur; tmp = tmp->next)
-	       if (INO_T_EQ (cur->ino, tmp->ino) && cur->dev == tmp->dev
-		   && cur->construct == tmp->construct)
+	       if (DIRS_EQ (cur, tmp) && cur->construct == tmp->construct)
 		  break;
 
 	      if (tmp == cur
 		  /* Last in the chain and duplicate of JOIN?  */
 		  && !(cur->next == NULL && join
-		       && INO_T_EQ (cur->ino, join->ino)
-		      && cur->dev == join->dev
-		      && cur->construct == join->construct))
+		       && DIRS_EQ (cur, join)
+		       && cur->construct == join->construct))
 		{
 		  /* Unique, so keep this directory.  */
 		  pcur = &cur->next;
@@ -297,8 +299,8 @@ add_sysroot_to_chain (const char *sysroo
 }
 
 /* Merge the four include chains together in the order quote, bracket,
-   system, after.  Remove duplicate dirs (as determined by
-   INO_T_EQ()).
+   system, after.  Remove duplicate dirs (determined in
+   system-specific manner).
 
    We can't just merge the lists and then uniquify them because then
    we may lose directories from the <> search path that should be

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 12:42 include path on windows Nathan Sidwell
@ 2008-05-20 14:05 ` Andrew Pinski
  2008-05-20 15:11   ` Daniel Jacobowitz
  2008-05-20 16:36 ` Joseph S. Myers
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Andrew Pinski @ 2008-05-20 14:05 UTC (permalink / raw)
  To: Nathan Sidwell; +Cc: GCC Patches

On Tue, May 20, 2008 at 4:19 AM, Nathan Sidwell <nathan@codesourcery.com> wrote:
> This patch fixes include path checking on non-inode file systems.  The
> current behaviour is to presume all -I include paths are unique on such
> systems.  We therefore fail to spot users trying to reorder the system
> include paths. However, we can do better than that by case-insensitive
> string compare.  That won't catch all duplicates, but does improve the
> situation.

We (Sony) have a patch which improves on this patch even further and
catches more dups.  Though I think we still don't catch the dups where
someone uses an UNC path and a mapped file system path (I don't know
if there is a way off hand to figure that out).

I can provide the patch but it will take a while as I have many other
responsibilities.

Thanks,
Andrew Pinski

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 14:05 ` Andrew Pinski
@ 2008-05-20 15:11   ` Daniel Jacobowitz
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Jacobowitz @ 2008-05-20 15:11 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: Nathan Sidwell, GCC Patches

On Tue, May 20, 2008 at 06:47:31AM -0700, Andrew Pinski wrote:
> On Tue, May 20, 2008 at 4:19 AM, Nathan Sidwell <nathan@codesourcery.com> wrote:
> > This patch fixes include path checking on non-inode file systems.  The
> > current behaviour is to presume all -I include paths are unique on such
> > systems.  We therefore fail to spot users trying to reorder the system
> > include paths. However, we can do better than that by case-insensitive
> > string compare.  That won't catch all duplicates, but does improve the
> > situation.
> 
> We (Sony) have a patch which improves on this patch even further and
> catches more dups.  Though I think we still don't catch the dups where
> someone uses an UNC path and a mapped file system path (I don't know
> if there is a way off hand to figure that out).
> 
> I can provide the patch but it will take a while as I have many other
> responsibilities.

Wouldn't using lrealpath do it?

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 12:42 include path on windows Nathan Sidwell
  2008-05-20 14:05 ` Andrew Pinski
@ 2008-05-20 16:36 ` Joseph S. Myers
  2008-05-20 23:36   ` Danny Smith
  2008-05-21  6:57 ` Danny Smith
  2008-05-22 14:32 ` Ian Lance Taylor
  3 siblings, 1 reply; 14+ messages in thread
From: Joseph S. Myers @ 2008-05-20 16:36 UTC (permalink / raw)
  To: Nathan Sidwell; +Cc: GCC Patches

On Tue, 20 May 2008, Nathan Sidwell wrote:

> This patch fixes include path checking on non-inode file systems.  The current
> behaviour is to presume all -I include paths are unique on such systems.  We
> therefore fail to spot users trying to reorder the system include paths.
> However, we can do better than that by case-insensitive string compare.  That
> won't catch all duplicates, but does improve the situation.
> 
> tested on linux and windows hosts.
> 
> ok?

This is OK; we can make any further improvements on top of this patch.  
(I think you can apply patches to c-* code shared by C and C++ as a C++ 
maintainer, without needing approval, and approve such changes yourself.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 16:36 ` Joseph S. Myers
@ 2008-05-20 23:36   ` Danny Smith
  2008-05-21  7:24     ` Nathan Sidwell
  0 siblings, 1 reply; 14+ messages in thread
From: Danny Smith @ 2008-05-20 23:36 UTC (permalink / raw)
  To: Joseph S. Myers; +Cc: Nathan Sidwell, GCC Patches

On Wed, May 21, 2008 at 3:14 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Tue, 20 May 2008, Nathan Sidwell wrote:
>
>> This patch fixes include path checking on non-inode file systems.  The current
>> behaviour is to presume all -I include paths are unique on such systems.  We
>> therefore fail to spot users trying to reorder the system include paths.
>> However, we can do better than that by case-insensitive string compare.  That
>> won't catch all duplicates, but does improve the situation.
>>
>> tested on linux and windows hosts.
>>
>> ok?
>
> This is OK; we can make any further improvements on top of this patch.


Thanks for doing this Nathan.  There are still problem with '/'  vs
'\\' and case-insensitivity .

eg
gcc -Wall -v  -I../.  -I..\.  -Ic:/ -IC:/  nul.c

doesn't find duplicates, but outputs

#include <...> search starts here:
  ../.
 ..\.
 c:/
 C:/

Should I file this with Bugzilla?

Daniel's suggestion of using lrealpath to canonicalize the the path
string is obvious improvement, but when I tried something similar in
past with lrealpath I ran into new FAILS with .pch testcases     I'll
revisit.

Danny

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 12:42 include path on windows Nathan Sidwell
  2008-05-20 14:05 ` Andrew Pinski
  2008-05-20 16:36 ` Joseph S. Myers
@ 2008-05-21  6:57 ` Danny Smith
  2008-05-22 15:52   ` Ian Lance Taylor
  2008-05-22 14:32 ` Ian Lance Taylor
  3 siblings, 1 reply; 14+ messages in thread
From: Danny Smith @ 2008-05-21  6:57 UTC (permalink / raw)
  To: Nathan Sidwell; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On Tue, May 20, 2008 at 11:19 PM, Nathan Sidwell
<nathan@codesourcery.com> wrote:
> This patch fixes include path checking on non-inode file systems.  The
> current behaviour is to presume all -I include paths are unique on such
> systems.  We therefore fail to spot users trying to reorder the system
> include paths. However, we can do better than that by case-insensitive
> string compare.  That won't catch all duplicates, but does improve the
> situation.
>

Hello

This improves the  change to c-incpath.c for Windows hosts by using
lrealpath to generate a name for the DIRS_EQ comparison. Also, rather
than using OS name defines to identify the crippled hosts, we use the
HOST_LACKS_INODE_NUMBERS macro.

The patch adds a new field in struct cpp_dir to store the canonical name
generated by lrealpath. This new field is necessary because for some
usages (eg, assembler output, PCH files) the user-provided (relative)
path name may be more useful. Currently, the only usage of the canonical
name is in the DIRS_EQ comprison.

If this is too invasive, then I can work up an alternative (but more
expensive for non-POSIX host) patch that allows the DIRS_EQ macro to be
overriden by xm-host.h file code,

Danny

Tested on i686-pc-mingw32

libcpp/ChangeLog
	* include/cpplib.h (struct cpp_dir): Add new field, canonical_name.

gcc/ChangeLog

	* c-incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
	rather than OS names to choose INO_T_EQ definition.
	(DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
	(add_path) [INO_T_EQ]: Use lrealpath to fill canonical_name field.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: inc_realpath.diff --]
[-- Type: text/x-diff; name=inc_realpath.diff, Size: 1807 bytes --]

Index: libcpp/include/cpplib.h
=================================================================== ---
libcpp/include/cpplib.h	(revision 135016) +++ libcpp/include/cpplib.h
(working copy) @@ -507,6 +507,9 @@ char *name; unsigned int len;
 
+  /* The canonicalized NAME as determined by lrealpath.  */
+  char* canonical_name;
+
   /* One if a system header, two if a system header that has extern
      "C" guards for C++.  */
   unsigned char sysp;
Index: gcc/c-incpath.c
===================================================================
--- gcc/c-incpath.c	(revision 135676)
+++ gcc/c-incpath.c	(working copy)
@@ -37,7 +37,7 @@
 #ifdef VMS
 # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
 # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
-#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
+#elif !defined (HOST_LACKS_INODE_NUMBERS)
 # define INO_T_EQ(A, B) ((A) == (B))
 # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
 #endif
@@ -46,7 +46,7 @@
 #define DIRS_EQ(A, B) ((A)->dev == (B)->dev \
 	&& INO_T_EQ((A)->ino, (B)->ino))
 #else
-#define DIRS_EQ(A, B) (!strcasecmp ((A)->name, (B)->name))
+#define DIRS_EQ(A, B) (!strcmp ((A)->canonical_name, (B)->canonical_name))
 #endif
 
 static const char dir_separator_str[] = { DIR_SEPARATOR, 0 };
@@ -388,7 +388,7 @@
 void
 add_path (char *path, int chain, int cxx_aware, bool user_supplied_p)
 {
-  cpp_dir *p;
+  cpp_dir *p = XNEW (cpp_dir);
 
 #if defined (HAVE_DOS_BASED_FILE_SYSTEM)
   /* Remove unnecessary trailing slashes.  On some versions of MS
@@ -405,9 +405,11 @@
     *end = 0;
 #endif
 
-  p = XNEW (cpp_dir);
   p->next = NULL;
   p->name = path;
+#ifndef INO_T_EQ
+  p->canonical_name = lrealpath (path);
+#endif
   if (chain == SYSTEM || chain == AFTER)
     p->sysp = 1 + !cxx_aware;
   else

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 23:36   ` Danny Smith
@ 2008-05-21  7:24     ` Nathan Sidwell
  0 siblings, 0 replies; 14+ messages in thread
From: Nathan Sidwell @ 2008-05-21  7:24 UTC (permalink / raw)
  To: Danny Smith; +Cc: Joseph S. Myers, GCC Patches

Danny Smith wrote:

> eg
> gcc -Wall -v  -I../.  -I..\.  -Ic:/ -IC:/  nul.c
> 
> doesn't find duplicates, but outputs
> 
> #include <...> search starts here:
>   ../.
>  ..\.
>  c:/
>  C:/
> 
> Should I file this with Bugzilla?
> 
> Daniel's suggestion of using lrealpath to canonicalize the the path
> string is obvious improvement, but when I tried something similar in
> past with lrealpath I ran into new FAILS with .pch testcases     I'll
> revisit.

Hmm, it's possible we have some other local patch that already does the 
lrealpath thing.

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-20 12:42 include path on windows Nathan Sidwell
                   ` (2 preceding siblings ...)
  2008-05-21  6:57 ` Danny Smith
@ 2008-05-22 14:32 ` Ian Lance Taylor
  3 siblings, 0 replies; 14+ messages in thread
From: Ian Lance Taylor @ 2008-05-22 14:32 UTC (permalink / raw)
  To: Nathan Sidwell; +Cc: GCC Patches

Nathan Sidwell <nathan@codesourcery.com> writes:

> +#if defined INO_T_EQ
> +#define DIRS_EQ(A, B) ((A)->dev == (B)->dev \
> +	&& INO_T_EQ((A)->ino, (B)->ino))
> +#else
> +#define DIRS_EQ(A, B) (!strcasecmp ((A)->name, (B)->name))
> +#endif

It looks like this was already approved, but that shouldn't be
strcasecmp, it should be filename_cmp from libiberty.

Ian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-21  6:57 ` Danny Smith
@ 2008-05-22 15:52   ` Ian Lance Taylor
  2008-05-26  7:10     ` Christopher Faylor
  2008-05-29 23:53     ` Danny Smith
  0 siblings, 2 replies; 14+ messages in thread
From: Ian Lance Taylor @ 2008-05-22 15:52 UTC (permalink / raw)
  To: Danny Smith; +Cc: Nathan Sidwell, GCC Patches

"Danny Smith" <dansmister@gmail.com> writes:

> libcpp/ChangeLog
> 	* include/cpplib.h (struct cpp_dir): Add new field, canonical_name.
>
> gcc/ChangeLog
>
> 	* c-incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
> 	rather than OS names to choose INO_T_EQ definition.
> 	(DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
> 	(add_path) [INO_T_EQ]: Use lrealpath to fill canonical_name field.

> +  /* The canonicalized NAME as determined by lrealpath.  */
> +  char* canonical_name;

Please expand the comment to say that this field is only used on hosts
which don't have reliable inode values.


>  #ifdef VMS
>  # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
>  # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
> -#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
> +#elif !defined (HOST_LACKS_INODE_NUMBERS)
>  # define INO_T_EQ(A, B) ((A) == (B))
>  # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
>  #endif

I believe this changes the behaviour on cygwin.  cygwin does not
appear to define HOST_LACKS_INODE_NUMBERS, but I believe that it
formerly would not define INO_T_EQ.  Are cygwin inode numbers
sufficiently reliable that they can be compared for equality?


>  static const char dir_separator_str[] = { DIR_SEPARATOR, 0 };
> @@ -388,7 +388,7 @@
>  void
>  add_path (char *path, int chain, int cxx_aware, bool user_supplied_p)
>  {
> -  cpp_dir *p;
> +  cpp_dir *p = XNEW (cpp_dir);
>  
>  #if defined (HAVE_DOS_BASED_FILE_SYSTEM)
>    /* Remove unnecessary trailing slashes.  On some versions of MS
> @@ -405,9 +405,11 @@
>      *end = 0;
>  #endif
>  
> -  p = XNEW (cpp_dir);

Moving the XNEW call sems to be unnecessary and it's not my preferred
style.  I think it's better to keep the initialization at the right
point in the code.

The patch is OK with those changes if you can promise that it will
work correctly on cygwin.

Thanks.

Ian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-22 15:52   ` Ian Lance Taylor
@ 2008-05-26  7:10     ` Christopher Faylor
  2008-05-29 23:53     ` Danny Smith
  1 sibling, 0 replies; 14+ messages in thread
From: Christopher Faylor @ 2008-05-26  7:10 UTC (permalink / raw)
  To: Ian Lance Taylor, Nathan Sidwell, GCC Patches, Danny Smith

On Thu, May 22, 2008 at 07:17:42AM -0700, Ian Lance Taylor wrote:
>"Danny Smith" <dansmister@gmail.com> writes:
>
>> libcpp/ChangeLog
>> 	* include/cpplib.h (struct cpp_dir): Add new field, canonical_name.
>>
>> gcc/ChangeLog
>>
>> 	* c-incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
>> 	rather than OS names to choose INO_T_EQ definition.
>> 	(DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
>> 	(add_path) [INO_T_EQ]: Use lrealpath to fill canonical_name field.
>
>> +  /* The canonicalized NAME as determined by lrealpath.  */
>> +  char* canonical_name;
>
>Please expand the comment to say that this field is only used on hosts
>which don't have reliable inode values.
>
>
>>  #ifdef VMS
>>  # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
>>  # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
>> -#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
>> +#elif !defined (HOST_LACKS_INODE_NUMBERS)
>>  # define INO_T_EQ(A, B) ((A) == (B))
>>  # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
>>  #endif
>
>I believe this changes the behaviour on cygwin.  cygwin does not
>appear to define HOST_LACKS_INODE_NUMBERS, but I believe that it
>formerly would not define INO_T_EQ.  Are cygwin inode numbers
>sufficiently reliable that they can be compared for equality?

Yes, they should be reliable.

cgf

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-22 15:52   ` Ian Lance Taylor
  2008-05-26  7:10     ` Christopher Faylor
@ 2008-05-29 23:53     ` Danny Smith
  2008-05-29 23:58       ` Ian Lance Taylor
  2008-05-30  0:05       ` DJ Delorie
  1 sibling, 2 replies; 14+ messages in thread
From: Danny Smith @ 2008-05-29 23:53 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: Nathan Sidwell, GCC Patches, dj

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

On Fri, May 23, 2008 at 2:17 AM, Ian Lance Taylor <iant@google.com> wrote:
> "Danny Smith" <dansmister@gmail.com> writes:
>
>> libcpp/ChangeLog
>>       * include/cpplib.h (struct cpp_dir): Add new field, canonical_name.
>>
>> gcc/ChangeLog
>>
>>       * c-incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
>>       rather than OS names to choose INO_T_EQ definition.
>>       (DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
>>       (add_path) [INO_T_EQ]: Use lrealpath to fill canonical_name field.
>
>> +  /* The canonicalized NAME as determined by lrealpath.  */
>> +  char* canonical_name;
>
> Please expand the comment to say that this field is only used on hosts
> which don't have reliable inode values.

OK.


>>  #ifdef VMS
>>  # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
>>  # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
>> -#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
>> +#elif !defined (HOST_LACKS_INODE_NUMBERS)
>>  # define INO_T_EQ(A, B) ((A) == (B))
>>  # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
>>  #endif
>
> I believe this changes the behaviour on cygwin.  cygwin does not
> appear to define HOST_LACKS_INODE_NUMBERS, but I believe that it
> formerly would not define INO_T_EQ.  Are cygwin inode numbers
> sufficiently reliable that they can be compared for equality?

As Chris indicated, the inode numbers on cygwin should be reliable.
Indeed they are more reliable than a simple strcmp of realpath names
that we fallback to on mingw32. The #ifdef _WIN32 condition doesn't
affect a host=cygwin build normally, since _WIN32 is only defined by
compiler with -mwin32 option. Recent (since at least 2004) releases of
cygwin have apparently been built without that switch, and detection of
duplicate directories works fine.

Also, cygwin reliably uses inode numbers to detect file identity with
--save-temps in gcc.c:do_spec_1 and to identify duplicates in
libgfortran/io/unix.c.

As for DJGPP (which defines __MSDOS__), I have not tested, but google
for st_ino and DJGPP strongly suggests that it is reliable. As with
cygwin, gcc.c:do_spec_1 and libgfortran/io/unix.c relies on st_ino for
DJGPP and no problems have been reported. DJ, can you verify that st_ino
is reliable for DJGPP?

<snip>
> Moving the XNEW call sems to be unnecessary and it's not my preferred
> style.  I think it's better to keep the initialization at the right
> point in the code.

OK


This is the revised patch.


libcpp/ChangeLog
	* include/cpplib.h (struct cpp_dir): Add new field, canonical_name.

gcc/ChangeLog

	* incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
	rather than OS names to choose INO_T_EQ definition.
	(DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
	(add_path) [!INO_T_EQ]: Use lrealpath to fill canonical_name field.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: incpath[2].diff --]
[-- Type: text/x-diff; name="incpath[2].diff", Size: 1884 bytes --]

Index: libcpp/include/cpplib.h
===================================================================
--- libcpp/include/cpplib.h	(revision 135892)
+++ libcpp/include/cpplib.h	(working copy)
@@ -508,6 +508,10 @@
   char *name;
   unsigned int len;
 
+  /* The canonicalized NAME as determined by lrealpath.  This field 
+     is only used by hosts that lack reliable inode numbers.  */
+  char* canonical_name;
+
   /* One if a system header, two if a system header that has extern
      "C" guards for C++.  */
   unsigned char sysp;
Index: gcc/incpath.c
===================================================================
--- gcc/incpath.c	(revision 135892)
+++ gcc/incpath.c	(working copy)
@@ -31,13 +31,12 @@
 #include "incpath.h"
 #include "cppdefault.h"
 
-/* Windows does not natively support inodes, and neither does MSDOS.
-   Cygwin's emulation can generate non-unique inodes, so don't use it.
+/* Microsoft Windows does not natively support inodes.
    VMS has non-numeric inodes.  */
 #ifdef VMS
 # define INO_T_EQ(A, B) (!memcmp (&(A), &(B), sizeof (A)))
 # define INO_T_COPY(DEST, SRC) memcpy(&(DEST), &(SRC), sizeof (SRC))
-#elif !((defined _WIN32 && !defined (_UWIN)) || defined __MSDOS__)
+#elif !defined (HOST_LACKS_INODE_NUMBERS)
 # define INO_T_EQ(A, B) ((A) == (B))
 # define INO_T_COPY(DEST, SRC) (DEST) = (SRC)
 #endif
@@ -46,7 +45,7 @@
 #define DIRS_EQ(A, B) ((A)->dev == (B)->dev \
 	&& INO_T_EQ((A)->ino, (B)->ino))
 #else
-#define DIRS_EQ(A, B) (!strcasecmp ((A)->name, (B)->name))
+#define DIRS_EQ(A, B) (!strcmp ((A)->canonical_name, (B)->canonical_name))
 #endif
 
 static const char dir_separator_str[] = { DIR_SEPARATOR, 0 };
@@ -408,6 +407,9 @@
   p = XNEW (cpp_dir);
   p->next = NULL;
   p->name = path;
+#ifndef INO_T_EQ
+  p->canonical_name = lrealpath (path);
+#endif
   if (chain == SYSTEM || chain == AFTER)
     p->sysp = 1 + !cxx_aware;
   else

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-29 23:53     ` Danny Smith
@ 2008-05-29 23:58       ` Ian Lance Taylor
  2008-05-30  7:12         ` Danny Smith
  2008-05-30  0:05       ` DJ Delorie
  1 sibling, 1 reply; 14+ messages in thread
From: Ian Lance Taylor @ 2008-05-29 23:58 UTC (permalink / raw)
  To: Danny Smith; +Cc: Nathan Sidwell, GCC Patches, dj

"Danny Smith" <dansmister@gmail.com> writes:

> libcpp/ChangeLog
> 	* include/cpplib.h (struct cpp_dir): Add new field, canonical_name.
>
> gcc/ChangeLog
>
> 	* incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
> 	rather than OS names to choose INO_T_EQ definition.
> 	(DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
> 	(add_path) [!INO_T_EQ]: Use lrealpath to fill canonical_name field.

> +  /* The canonicalized NAME as determined by lrealpath.  This field 
> +     is only used by hosts that lack reliable inode numbers.  */
> +  char* canonical_name;

I just noticed: "char *canonical_name".

This patch is OK with that change.

Thanks.

Ian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-29 23:53     ` Danny Smith
  2008-05-29 23:58       ` Ian Lance Taylor
@ 2008-05-30  0:05       ` DJ Delorie
  1 sibling, 0 replies; 14+ messages in thread
From: DJ Delorie @ 2008-05-30  0:05 UTC (permalink / raw)
  To: Danny Smith; +Cc: iant, nathan, gcc-patches


> As for DJGPP (which defines __MSDOS__), I have not tested, but
> google for st_ino and DJGPP strongly suggests that it is
> reliable. As with cygwin, gcc.c:do_spec_1 and libgfortran/io/unix.c
> relies on st_ino for DJGPP and no problems have been reported. DJ,
> can you verify that st_ino is reliable for DJGPP?

It should be, but I can't recall the details at the moment.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: include path on windows
  2008-05-29 23:58       ` Ian Lance Taylor
@ 2008-05-30  7:12         ` Danny Smith
  0 siblings, 0 replies; 14+ messages in thread
From: Danny Smith @ 2008-05-30  7:12 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: Nathan Sidwell, GCC Patches, dj

On Fri, May 30, 2008 at 10:52 AM, Ian Lance Taylor <iant@google.com> wrote:
> "Danny Smith" <dansmister@gmail.com> writes:
>
>> libcpp/ChangeLog
>>       * include/cpplib.h (struct cpp_dir): Add new field, canonical_name.
>>
>> gcc/ChangeLog
>>
>>       * incpath.c: Use HOST_LACKS_INODE_NUMBERS conditional
>>       rather than OS names to choose INO_T_EQ definition.
>>       (DIRS_EQ) [!INO_T_EQ]: Don't worry about case in comparison.
>>       (add_path) [!INO_T_EQ]: Use lrealpath to fill canonical_name field.
>
>> +  /* The canonicalized NAME as determined by lrealpath.  This field
>> +     is only used by hosts that lack reliable inode numbers.  */
>> +  char* canonical_name;
>
> I just noticed: "char *canonical_name".
>
> This patch is OK with that change.
>
Committed as revision 136196
Danny

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2008-05-30  1:00 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-20 12:42 include path on windows Nathan Sidwell
2008-05-20 14:05 ` Andrew Pinski
2008-05-20 15:11   ` Daniel Jacobowitz
2008-05-20 16:36 ` Joseph S. Myers
2008-05-20 23:36   ` Danny Smith
2008-05-21  7:24     ` Nathan Sidwell
2008-05-21  6:57 ` Danny Smith
2008-05-22 15:52   ` Ian Lance Taylor
2008-05-26  7:10     ` Christopher Faylor
2008-05-29 23:53     ` Danny Smith
2008-05-29 23:58       ` Ian Lance Taylor
2008-05-30  7:12         ` Danny Smith
2008-05-30  0:05       ` DJ Delorie
2008-05-22 14:32 ` Ian Lance Taylor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).