[PATCH 2/2] Introduce the gcc option --record-gcc-command-line

[Egeyar Bagcioglu]

Hello again Segher!

On 11/7/19 9:03 AM, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Nov 06, 2019 at 06:21:34PM +0100, Egeyar Bagcioglu wrote:
>> +static const char *
>> +record_gcc_command_line_spec_function(int argc ATTRIBUTE_UNUSED, const char **argv)
>> +{
>> +  const char *filename = argv[0];
>> +  FILE *out = fopen (filename, "w");
>> +  if (out)
>> +    {
>> +      fputs (_gcc_argv[0], out);
>> +      for (int i = 1; i < _gcc_argc; i += 1)
>> +	{
>> +	  fputc (' ', out);
>> +	  fputs (_gcc_argv[i], out);
>> +	}
>> +      fclose (out);
>> +    }
>> +  return filename;
>> +}
> Pet peeve: just use fprintf?

okay.

> If there is an error, should this return 0 or indicate error some way?
> Making the error silent ("if (out)") seems weird, otherwise -- if it is
> on purpose, a comment might help.

It was on purpose so that this does not interfere with the builds. 
However, re-watching today Nick's talk at Cauldron where he mentions 
it's good to fail even in such cases, I am rethinking if we would like 
to error out here. If anyone has any preference on this, I'd like to hear.

>> +int
>> +elf_record_gcc_command_line ()
>> +{
>> +  char cmdline[256];
>> +  section * sec;
> section *sec;

right.

>> +  sec = get_section (targetm.asm_out.record_gcc_switches_section,
>> +		     SECTION_DEBUG | SECTION_MERGE
>> +		     | SECTION_STRINGS | (SECTION_ENTSIZE & 1),
>> +		     NULL);
>> +  switch_to_section (sec);
>> +
>> +  ASM_OUTPUT_ASCII(asm_out_file, version_string, strlen(version_string));
>> +
>> +  FILE *out_stream = fopen (gcc_command_line_file, "r");
>> +  if (out_stream)
>> +    {
>> +      ASM_OUTPUT_ASCII(asm_out_file, " : ", 3);
>> +      ssize_t cmdline_length;
>> +      while ((cmdline_length = fread(cmdline, 1, 256, out_stream)))
> fread (
> (and many more like it).

okay, I will fix them. Thanks for catching.

>> +	ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
>> +    }
>> +  cmdline[0] = 0;
>> +  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
>> +
>> +  /* The return value is currently ignored by the caller, but must be 0.  */
>> +  return 0;
>> +}
> A temporary file like this isn't so great.

GCC operates with temporary files, doesn't it? What is the concern that 
is specific to this one? That is the most reasonable way I found to pass 
the argv of gcc to child processes for saving. Other ways of passing it 
that I could think of, or the idea of saving it in the driver were 
actually very bad ideas.

> Opening a file as "r" but then
> accessing it with "fread" is peculiar, too.

I am not sure what you mean here. Is it that you prefer "wb" and "rb" 
instead of "w" and "r"? I thought it was enough to use a consistent pair.

> HTH,

It does! Thanks a lot for the review!

Regards
Egeyar