Ping

> -----Original Message-----
> From: Joey Ye [mailto:joey.ye@arm.com]
> Sent: Monday, February 18, 2013 11:32
> To: 'Joseph Myers'
> Cc: gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Fix PR50293 - LTO plugin with space in path
> 
> Joseph, Thanks for your valuable comments. See my reply and new patch
> below.
> 
> > -----Original Message-----
> > From: Joseph Myers [mailto:joseph@codesourcery.com]
> > Sent: Monday, February 18, 2013 06:16
> > To: Joey Ye
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: Re: [PATCH] Fix PR50293 - LTO plugin with space in path
> >
> > On Sun, 17 Feb 2013, Joey Ye wrote:
> >
> > > +static char * convert_white_space(char * orig);
> >
> > Please fix formatting in many places in this patch to follow the GNU
> > Coding Standards.  No space after '*', but space before '('; there
> seem
> > to
> > be various other formatting problems as well.
> My bad. All fixed.
> 
> >
> > > +/* Insert back slash before spaces in a string, to avoid path
> > > +   that has space in it broken into multiple arguments.  */
> >
> > That doesn't seem to be a proper specification of the interface to
> this
> > function.  What are the semantics of ORIG?  A string that is a
> filename,
> > or something else?  What are the exact semantics of the return value
> for
> > quoting - is it correct for the function to convert a (backslash,
> space)
> > pair to (backslash, backslash, space) or not?  Is anything special in
> > the
> > return value other than backslash and space, and how are any special
> > characters in the return value to be interpreted?
> >
> > As it seems like this function frees the argument (why?) this also
> needs
> > to be specified in the comment as part of the semantics of the
> function.
> This function might need a string longer than original one to
> accommodate
> additional back slashes. So it has to xmalloc a new string. The original
> string should be freed in such a case. However, it is tedious to caller
> to figure out that conversion does happens and free the orig. The
> solution
> is for this function to free it when conversion happens.
> 
> By doing so it is required that orig must be allocated and can be freed,
> as the newly added comments described explicitly.
> >
> > It would be a good idea for you to give a more detailed explanation in
> > the
> > next version of the patch submission of how the path, before the patch,
> > got processed so that the spaces were wrongly interpreted.  That might
> > help make clearer whether the interface to this new function is
> actually
> > correct, since the subsequent operations on the return value should
> act
> > as
> > an inverse to the operation carried out by this function.
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com
> 
> Index: gcc/gcc.c
> ===================================================================
> --- gcc/gcc.c	(revision 195189)
> +++ gcc/gcc.c	(working copy)
> @@ -265,6 +265,7 @@
>  static const char *compare_debug_auxbase_opt_spec_function (int, const
> char **);
>  static const char *pass_through_libs_spec_func (int, const char **);
>  static const char *replace_extension_spec_func (int, const char **);
> +static char * convert_white_space (char *);
> 
> 
>  /* The Specs Language
> 
> @@ -6595,6 +6596,7 @@
>  				    X_OK, false);
>    if (lto_wrapper_file)
>      {
> +      lto_wrapper_file = convert_white_space (lto_wrapper_file);
>        lto_wrapper_spec = lto_wrapper_file;
>        obstack_init (&collect_obstack);
>        obstack_grow (&collect_obstack, "COLLECT_LTO_WRAPPER=",
> @@ -7005,12 +7007,13 @@
>  			      + strlen (fuse_linker_plugin), 0))
>  #endif
>  	    {
> -	      linker_plugin_file_spec = find_a_file (&exec_prefixes,
> +	      char * temp_spec = find_a_file (&exec_prefixes,
>  						     LTOPLUGINSONAME, R_OK,
>  						     false);
> -	      if (!linker_plugin_file_spec)
> +	      if (!temp_spec)
>  		fatal_error ("-fuse-linker-plugin, but %s not found",
>  			     LTOPLUGINSONAME);
> +	      linker_plugin_file_spec = convert_white_space (temp_spec);
>  	    }
>  #endif
>  	  lto_gcc_spec = argv[0];
> @@ -8506,3 +8509,52 @@
>    free (name);
>    return result;
>  }
> +
> +/* Insert back slash before spaces in orig (usually a file path), to
> +   avoid being broken by spec parser.
> +
> +   This function is needed as do_spec_1 treats white space (' ' and
> '\t')
> +   as the end of an argument. But in case of -plugin /usr/gcc
> install/xxx.so,
> +   the filename should be treated as a single argument rather than
> being
> +   broken into multiple. Solution is to insert '\\' before the space in
> a
> +   filename.
> +
> +   This function converts and only converts all occurrance of ' '
> +   to '\\' + ' ' and '\t' to '\\' + '\t'.  For example:
> +   "a b"  -> "a\\ b"
> +   "a  b" -> "a\\ \\ b"
> +   "a\tb" -> "a\\\tb"
> +   "a\\ b" -> "a\\\\ b"
> +
> +   orig: input null-terminating string that was allocated by xalloc.
> The
> +   memory it points to might be freed in this function. Behavior
> undefined
> +   if orig isn't xalloced or is freed already at entry.
> +
> +   Return: orig if no conversion needed. orig if conversion needed but
> no
> +   sufficient memory for a new string. Otherwise a newly allocated
> string
> +   that was converted from orig.  */
> +
> +static char * convert_white_space (char *orig)
> +{
> +  int len, number_of_space = 0;
> +  if (orig == NULL) return orig;
> +
> +  for (len=0; orig[len]; len++)
> +    if (orig[len] == ' ' || orig[len] == '\t') number_of_space ++;
> +
> +  if (number_of_space)
> +    {
> +      char * new_spec = (char *)xmalloc (len + number_of_space + 1);
> +      int j,k;
> +      if (new_spec == NULL) return orig;
> +
> +      for (j=0, k=0; j<=len; j++, k++)
> +	{
> +	  if (orig[j] == ' ' || orig[j] == '\t') new_spec[k++] = '\\';
> +	  new_spec[k] = orig[j];
> +	}
> +      free (orig);
> +      return new_spec;
> +  }
> +  else return orig;
> +}