Prune invalid filename due to makefile syntax.
diff mbox series

Message ID 144dce4b-4163-c4b0-2817-aebe74cbca0f@suse.cz
State New
Headers show
Series
  • Prune invalid filename due to makefile syntax.
Related show

Commit Message

Martin Liška Jan. 21, 2020, 3:05 p.m. UTC
Hi.

The patch strips '#' in filenames used for Makefile.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2020-01-21  Martin Liska  <mliska@suse.cz>

	PR driver/93057
	* lto-wrapper.c (prune_filename_for_make): Prune
	characters like '#'.
---
  gcc/lto-wrapper.c | 11 +++++++++++
  1 file changed, 11 insertions(+)

Comments

Jan Hubicka Jan. 21, 2020, 3:08 p.m. UTC | #1
> Hi.
> 
> The patch strips '#' in filenames used for Makefile.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2020-01-21  Martin Liska  <mliska@suse.cz>
> 
> 	PR driver/93057
> 	* lto-wrapper.c (prune_filename_for_make): Prune
> 	characters like '#'.

I think this is not enough - you need to take into consideration all
special characters used by make and bash, such as $ and others...

Honza
> ---
>  gcc/lto-wrapper.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> 

> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index fe8f292f877..f2504cc5b4f 100644
> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c
> @@ -1241,6 +1241,16 @@ jobserver_active_p (void)
>  	  && is_valid_fd (wfd));
>  }
>  
> +/* Prune invalid characters that can't be in name due to Makefile syntax.  */
> +
> +static void
> +prune_filename_for_make (char *filename)
> +{
> +  for (unsigned i = 0; i < strlen (filename); i++)
> +    if (filename[i] == '#')
> +      filename[i] = '_';
> +}
> +
>  /* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */
>  
>  static void
> @@ -1780,6 +1790,7 @@ cont:
>  	  obstack_grow (&env_obstack, input_name, strlen (input_name) - 2);
>  	  obstack_grow (&env_obstack, ".ltrans.o", sizeof (".ltrans.o"));
>  	  output_name = XOBFINISH (&env_obstack, char *);
> +	  prune_filename_for_make (output_name);
>  
>  	  /* Adjust the dumpbase if the linker output file was seen.  */
>  	  if (linker_output)
>
Andreas Schwab Jan. 21, 2020, 3:15 p.m. UTC | #2
On Jan 21 2020, Martin Liška wrote:

> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index fe8f292f877..f2504cc5b4f 100644
> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c
> @@ -1241,6 +1241,16 @@ jobserver_active_p (void)
>  	  && is_valid_fd (wfd));
>  }
>  
> +/* Prune invalid characters that can't be in name due to Makefile syntax.  */
> +
> +static void
> +prune_filename_for_make (char *filename)
> +{
> +  for (unsigned i = 0; i < strlen (filename); i++)

That's quadratic runtime.

Andreas.
Martin Liška Jan. 21, 2020, 3:18 p.m. UTC | #3
On 1/21/20 4:15 PM, Andreas Schwab wrote:
> On Jan 21 2020, Martin Liška wrote:
> 
>> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
>> index fe8f292f877..f2504cc5b4f 100644
>> --- a/gcc/lto-wrapper.c
>> +++ b/gcc/lto-wrapper.c
>> @@ -1241,6 +1241,16 @@ jobserver_active_p (void)
>>   	  && is_valid_fd (wfd));
>>   }
>>   
>> +/* Prune invalid characters that can't be in name due to Makefile syntax.  */
>> +
>> +static void
>> +prune_filename_for_make (char *filename)
>> +{
>> +  for (unsigned i = 0; i < strlen (filename); i++)
> 
> That's quadratic runtime.
> 
> Andreas.
> 

Thanks for heads up.

Martin
Martin Liška Jan. 21, 2020, 3:34 p.m. UTC | #4
On 1/21/20 4:08 PM, Jan Hubicka wrote:
> I think this is not enough - you need to take into consideration all
> special characters used by make and bash, such as $ and others...

Hm, you are right. Do you have a reasonable list which we should support?
Or should we leave this known limitation?

Martin
Jan Hubicka Jan. 21, 2020, 3:37 p.m. UTC | #5
> On 1/21/20 4:08 PM, Jan Hubicka wrote:
> > I think this is not enough - you need to take into consideration all
> > special characters used by make and bash, such as $ and others...
> 
> Hm, you are right. Do you have a reasonable list which we should support?
No - I guess one needs to dig into manual, try them out or just assume
that all letters in filenams except for a-z, 0-9, _, -, ., ',' and
perhaps more common caracters needs to be taken out?  It is used only to
make more meaningful temp filenames, right?
> Or should we leave this known limitation?
I would say we want to fix that... Even GCC gets idea to put # info
filenames and I am sure except for HHVM there are others.

Honza
> 
> Martin
Richard Biener Jan. 21, 2020, 4:26 p.m. UTC | #6
On January 21, 2020 4:37:00 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On 1/21/20 4:08 PM, Jan Hubicka wrote:
>> > I think this is not enough - you need to take into consideration
>all
>> > special characters used by make and bash, such as $ and others...
>> 
>> Hm, you are right. Do you have a reasonable list which we should
>support?
>No - I guess one needs to dig into manual, try them out or just assume
>that all letters in filenams except for a-z, 0-9, _, -, ., ',' and
>perhaps more common caracters needs to be taken out?  It is used only
>to
>make more meaningful temp filenames, right?
>> Or should we leave this known limitation?
>I would say we want to fix that... Even GCC gets idea to put # info
>filenames and I am sure except for HHVM there are others.

Can't w just quote them somehow? 

Richard. 

>
>Honza
>> 
>> Martin
Nathan Sidwell Jan. 21, 2020, 4:40 p.m. UTC | #7
On 1/21/20 11:26 AM, Richard Biener wrote:
> On January 21, 2020 4:37:00 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:

>> I would say we want to fix that... Even GCC gets idea to put # info
>> filenames and I am sure except for HHVM there are others.
> 
> Can't w just quote them somehow?

IIUC there are two issues

1) Make's syntax.  Its quoting is baroque, (and incomplete?).  See the 
comment in libcpp/mkdeps.c, which I think is curtesy of ZackW.  I see 
that the just-released Make 4.3 has changed something to do with # and 
its need to be quoted in some contexts.

2) Bad chars for the underlying filesystem.  But, if the temp files are 
on the same FS as the thing they're temping for, then there shouldn't be 
a conflict.  If they are different, then one might hope the temp-fs 
allows a superset.

nathan
Richard Biener Jan. 22, 2020, 8:12 a.m. UTC | #8
On Tue, Jan 21, 2020 at 5:40 PM Nathan Sidwell <nathan@acm.org> wrote:
>
> On 1/21/20 11:26 AM, Richard Biener wrote:
> > On January 21, 2020 4:37:00 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>
> >> I would say we want to fix that... Even GCC gets idea to put # info
> >> filenames and I am sure except for HHVM there are others.
> >
> > Can't w just quote them somehow?
>
> IIUC there are two issues
>
> 1) Make's syntax.  Its quoting is baroque, (and incomplete?).  See the
> comment in libcpp/mkdeps.c, which I think is curtesy of ZackW.  I see
> that the just-released Make 4.3 has changed something to do with # and
> its need to be quoted in some contexts.

Ah, maybe it's worth to export this functionality to libiberty?

> 2) Bad chars for the underlying filesystem.  But, if the temp files are
> on the same FS as the thing they're temping for, then there shouldn't be
> a conflict.  If they are different, then one might hope the temp-fs
> allows a superset.

Yeah, so I'd suggest to simply avoid the bad names being created from
the WPA stage and instead sanitize it there already.  That would make
it also consistent when debugging.  Not to say that using a tempfile
by default when not doing -save-temps would be appropriate anyway.

Richard.

> nathan
>
> --
> Nathan Sidwell
Martin Liška Jan. 23, 2020, 9:54 a.m. UTC | #9
On 1/22/20 9:12 AM, Richard Biener wrote:
> Ah, maybe it's worth to export this functionality to libiberty?

That would make sense to me. Can you please Nathan factor out
the function to libiberty?

Thanks,
Martin
Nathan Sidwell Jan. 23, 2020, 11:43 a.m. UTC | #10
On 1/23/20 4:54 AM, Martin Liška wrote:
> On 1/22/20 9:12 AM, Richard Biener wrote:
>> Ah, maybe it's worth to export this functionality to libiberty?
> 
> That would make sense to me. Can you please Nathan factor out
> the function to libiberty?

you're not the boss of me :)

But seriously, I'm not really going to have time to invest in this in 
the near future.

nathan

Patch
diff mbox series

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index fe8f292f877..f2504cc5b4f 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1241,6 +1241,16 @@  jobserver_active_p (void)
 	  && is_valid_fd (wfd));
 }
 
+/* Prune invalid characters that can't be in name due to Makefile syntax.  */
+
+static void
+prune_filename_for_make (char *filename)
+{
+  for (unsigned i = 0; i < strlen (filename); i++)
+    if (filename[i] == '#')
+      filename[i] = '_';
+}
+
 /* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */
 
 static void
@@ -1780,6 +1790,7 @@  cont:
 	  obstack_grow (&env_obstack, input_name, strlen (input_name) - 2);
 	  obstack_grow (&env_obstack, ".ltrans.o", sizeof (".ltrans.o"));
 	  output_name = XOBFINISH (&env_obstack, char *);
+	  prune_filename_for_make (output_name);
 
 	  /* Adjust the dumpbase if the linker output file was seen.  */
 	  if (linker_output)