Patchwork avoid '//' prefixes when sysroot is set to '/'

login
register
mail settings
Submitter Matthias Klose
Date Jan. 26, 2012, 2:34 p.m.
Message ID <4F21646A.2030501@ubuntu.com>
Download mbox | patch
Permalink /patch/137935/
State New
Headers show

Comments

Matthias Klose - Jan. 26, 2012, 2:34 p.m.
On 25.01.2012 17:45, Joseph S. Myers wrote:
> On Wed, 25 Jan 2012, Matthias Klose wrote:
>
>> This can end up in generation for dependency files, and other files parsing
>> the output. The solution I came up with is to check for sysroot set to '/' and
>> special case this in two places. Afaics, there are no other places.
>
> I could imagine a sysroot path that isn't just '/' but ends with '/'
> resulting in duplicate '/' in the middle of the path - although that's not
> a correctness issue in the way that '//' at the start could be, maybe the
> best check is actually for '/' at end of sysroot (in which case skip the
> '/' at the start of the path within the sysroot)?

as in the attached trailing.diff? built and regression tested.

> Or remove such a '/' at
> end of sysroot at configure time rather than checking for it later....

that's not enough. you can pass --sysroot=/ to the driver as well. I tried that 
in the attached tsr.diff, but I don't like it because the driver then pases 
`-isysroot ""' to the cc*1 binaries, which looks a bit misleading.

   Matthias

Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 183514)
+++ gcc/gcc.c	(working copy)
@@ -3417,6 +3417,12 @@
 
     case OPT__sysroot_:
       target_system_root = arg;
+      {
+        int target_system_root_len = strlen (target_system_root);
+
+        if (target_system_root[target_system_root_len - 1] == DIR_SEPARATOR)
+          target_system_root[target_system_root_len - 1] = '\0';
+      }
       target_system_root_changed = 1;
       do_save = false;
       break;
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 183514)
+++ gcc/configure.ac	(working copy)
@@ -767,7 +767,7 @@
 [
  case ${with_sysroot} in
  yes) TARGET_SYSTEM_ROOT='${exec_prefix}/${target_noncanonical}/sys-root' ;;
- *) TARGET_SYSTEM_ROOT=$with_sysroot ;;
+ *) TARGET_SYSTEM_ROOT=`echo $with_sysroot | sed 's,/$,,'` ;;
  esac
    
  TARGET_SYSTEM_ROOT_DEFINE='-DTARGET_SYSTEM_ROOT=\"$(TARGET_SYSTEM_ROOT)\"'
@@ -1830,7 +1830,7 @@
 		*)
 			;;
 	esac
-elif test "x$TARGET_SYSTEM_ROOT" != x; then
+elif test "x$TARGET_SYSTEM_ROOT_DEFINE" != x; then
         SYSTEM_HEADER_DIR=$build_system_header_dir 
 fi
 
@@ -4547,7 +4547,7 @@
 [Define to PREFIX/include if cpp should also search that directory.])
 fi
 
-if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT" != x; then
+if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT_DEFINE" != x; then
   if test "x$with_headers" != x; then
     target_header_dir=$with_headers
   elif test "x$with_sysroot" = x; then
Joseph S. Myers - Jan. 26, 2012, 5:57 p.m.
On Thu, 26 Jan 2012, Matthias Klose wrote:

> On 25.01.2012 17:45, Joseph S. Myers wrote:
> > On Wed, 25 Jan 2012, Matthias Klose wrote:
> > 
> > > This can end up in generation for dependency files, and other files
> > > parsing
> > > the output. The solution I came up with is to check for sysroot set to '/'
> > > and
> > > special case this in two places. Afaics, there are no other places.
> > 
> > I could imagine a sysroot path that isn't just '/' but ends with '/'
> > resulting in duplicate '/' in the middle of the path - although that's not
> > a correctness issue in the way that '//' at the start could be, maybe the
> > best check is actually for '/' at end of sysroot (in which case skip the
> > '/' at the start of the path within the sysroot)?
> 
> as in the attached trailing.diff? built and regression tested.

Yes, that's OK (with copyright date updates in incpath.c).

Patch

Index: gcc/incpath.c
===================================================================
--- gcc/incpath.c	(revision 183555)
+++ gcc/incpath.c	(working copy)
@@ -166,7 +166,15 @@ 
 
 	  /* Should this directory start with the sysroot?  */
 	  if (sysroot && p->add_sysroot)
-	    str = concat (sysroot, p->fname, NULL);
+	    {
+	      char *sysroot_no_trailing_dir_separator = xstrdup (sysroot);
+	      int sysroot_len = strlen (sysroot);
+
+	      if (sysroot_len > 0 && sysroot[sysroot_len - 1] == DIR_SEPARATOR)
+		sysroot_no_trailing_dir_separator[sysroot_len - 1] = '\0';
+	      str = concat (sysroot_no_trailing_dir_separator, p->fname, NULL);
+	      free (sysroot_no_trailing_dir_separator);
+	    }
 	  else if (!p->add_sysroot && relocated
 		   && !filename_ncmp (p->fname, cpp_PREFIX, cpp_PREFIX_len))
 	    {
Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 183555)
+++ gcc/gcc.c	(working copy)
@@ -2445,9 +2445,17 @@ 
 
   if (target_system_root)
     {
+      char *sysroot_no_trailing_dir_separator = xstrdup (target_system_root);
+      int sysroot_len = strlen (target_system_root);
+
+      if (sysroot_len > 0
+	  && target_system_root[sysroot_len - 1] == DIR_SEPARATOR)
+	sysroot_no_trailing_dir_separator[sysroot_len - 1] = '\0';
+
       if (target_sysroot_suffix)
 	  prefix = concat (target_sysroot_suffix, prefix, NULL);
-      prefix = concat (target_system_root, prefix, NULL);
+      prefix = concat (sysroot_no_trailing_dir_separator, prefix, NULL);
+      free (sysroot_no_trailing_dir_separator);
 
       /* We have to override this because GCC's notion of sysroot
 	 moves along with GCC.  */