[RFC] -grecord-gcc-switches (PR other/32998)

Hi!

As discussed in the PR, this patch implements IMHO a better alternative
to the current -frecord-gcc-switches option which isn't very usable unless
inspected on relocatable files only.  As each switch is zero terminated and
the section is mergeable, for whole binary or shared library we end up with
a set of all options which ever showed up on the command line when compiling
some of the CUs, but it is impossible to narrow it down back to which CU has
been compiled with what options.

This patch insteads appends the options as one long string separated by
spaces to DW_AT_producer attribute (which is normally just a reference
to a string in .debug_str).  Thus, ideally if all or most of the CUs
are compiled with the same options there is just one or just small number of

	Jakub

On 07/12/2011 07:46 PM, Jakub Jelinek wrote:
> The aim is to include just (or primarily) code generation affecting options
> explicitly passed on the command line.  So that the merging actually works,
> options or arguments which include filenames or paths shouldn't be added,
> on Roland's request -D*/-U* options aren't added either (that should be
> covered by .debug_macinfo)

...but only with -g3.

> Ideally we'd just include explicitly passed options from command line that
> haven't been overridden by other command line options, and would sort them,
> so that there are higher chances of DW_AT_producer strings being merged
> (e.g. -O2 -ffast-math vs. -ffast-math -O2 are now different strings, and
> similarly -O2 vs. -O3 -O2 vs. -O0 -O1 -Ofast -O2), but I'm not sure if it is
> easily possible using current option handling framework.

Why not?  Sorting sounds pretty straightforward to me, though you might 
want to copy the array first.

On the other hand, it probably isn't worthwhile; presumably most 
relocatables being linked together will share the same CFLAGS, so you'll 
get a high degree of merging without any sorting.

> --- gcc/testsuite/lib/dg-pch.exp.jj	2011-01-03 18:58:03.000000000 +0100
> +++ gcc/testsuite/lib/dg-pch.exp	2011-07-12 23:13:50.943670171 +0200
> -	dg-test -keep-output "./$bname$suffix" "$otherflags $flags" ""
> +	dg-test -keep-output "./$bname$suffix" "-gno-record-gcc-switches $otherflags $flags" ""

Why is this necessary?

Jason

On Wed, Jul 13, 2011 at 09:56:58AM -0400, Jason Merrill wrote:
> On 07/12/2011 07:46 PM, Jakub Jelinek wrote:
> >The aim is to include just (or primarily) code generation affecting options
> >explicitly passed on the command line.  So that the merging actually works,
> >options or arguments which include filenames or paths shouldn't be added,
> >on Roland's request -D*/-U* options aren't added either (that should be
> >covered by .debug_macinfo)
> 
> ...but only with -g3.

Sure.  But if we put -D*/-U* into DW_AT_producer, -D_FORTIFY_SOURCE=2
on the command line acts the same as #define _FORTIFY_SOURCE 2
before including the first header and the latter wouldn't be recorded.
I'm working on smaller .debug_macinfo right now.

> >Ideally we'd just include explicitly passed options from command line that
> >haven't been overridden by other command line options, and would sort them,
> >so that there are higher chances of DW_AT_producer strings being merged
> >(e.g. -O2 -ffast-math vs. -ffast-math -O2 are now different strings, and
> >similarly -O2 vs. -O3 -O2 vs. -O0 -O1 -Ofast -O2), but I'm not sure if it is
> >easily possible using current option handling framework.
> 
> Why not?  Sorting sounds pretty straightforward to me, though you
> might want to copy the array first.

If the command line options contain options that override each other, then
sorting would drop important information what comes last and thus overrides
other options.  If we would have only options which weren't overridden,
we could sort.  Otherwise -O2 -O0 would be sorted as -O0 -O2 and we'd think
the code was optimized when it wasn't.

> On the other hand, it probably isn't worthwhile; presumably most
> relocatables being linked together will share the same CFLAGS, so
> you'll get a high degree of merging without any sorting.
> 
> >--- gcc/testsuite/lib/dg-pch.exp.jj	2011-01-03 18:58:03.000000000 +0100
> >+++ gcc/testsuite/lib/dg-pch.exp	2011-07-12 23:13:50.943670171 +0200
> >-	dg-test -keep-output "./$bname$suffix" "$otherflags $flags" ""
> >+	dg-test -keep-output "./$bname$suffix" "-gno-record-gcc-switches $otherflags $flags" ""
> 
> Why is this necessary?

It is only necessary if somebody wants to make -grecord-gcc-switches
the default (for bootstrap/regtest I've tweaked common.opt to do that
to test it better).  PCH is a big mess and screws debuginfo in many ways,
in this case it was just small differences in DW_AT_producer, but
we have e.g. ICEs with PCH and -feliminate-dwarf-dups etc.

	Jakub

On 07/13/2011 10:06 AM, Jakub Jelinek wrote:
>>> --- gcc/testsuite/lib/dg-pch.exp.jj	2011-01-03 18:58:03.000000000 +0100
>>> +++ gcc/testsuite/lib/dg-pch.exp	2011-07-12 23:13:50.943670171 +0200
>>> -	dg-test -keep-output "./$bname$suffix" "$otherflags $flags" ""
>>> +	dg-test -keep-output "./$bname$suffix" "-gno-record-gcc-switches $otherflags $flags" ""
>>
> It is only necessary if somebody wants to make -grecord-gcc-switches
> the default (for bootstrap/regtest I've tweaked common.opt to do that
> to test it better).  PCH is a big mess and screws debuginfo in many ways,
> in this case it was just small differences in DW_AT_producer, but
> we have e.g. ICEs with PCH and -feliminate-dwarf-dups etc.

Why would PCH change DW_AT_producer?  Because we're restoring 
single_comp_unit_die from the PCH?  Then perhaps we should set 
DW_AT_producer in output_comp_unit rather than gen_compile_unit_die.

Jason

On Wed, 13 Jul 2011, Jakub Jelinek wrote:

> Ideally we'd just include explicitly passed options from command line that
> haven't been overridden by other command line options, and would sort them,
> so that there are higher chances of DW_AT_producer strings being merged
> (e.g. -O2 -ffast-math vs. -ffast-math -O2 are now different strings, and
> similarly -O2 vs. -O3 -O2 vs. -O0 -O1 -Ofast -O2), but I'm not sure if it is
> easily possible using current option handling framework.

I don't think there's enough structure for that, although prune_options 
could always be enhanced to cover more cases of overridden options.

> +	  default:
> +	    if (save_decoded_options[j].orig_option_with_args_text[0] != '-')
> +	      continue;

Should instead be ignoring OPT_SPECIAL_*.

> +	    switch (save_decoded_options[j].orig_option_with_args_text[1])
> +	      {
> +	      case 'M':
> +	      case 'i':
> +	      case 'W':

It would be better to check the canonical option text rather than the 
original text.  Normally this wouldn't make any difference because the 
driver will have passed canonical options to cc1, but if someone calls cc1 
directly then I think aliases should still act the same as the options 
they are aliases for in this regard.

> +		continue;
> +	      case 'n':
> +		if (save_decoded_options[j].orig_option_with_args_text[2]
> +		    == 'o')
> +		  continue;

When would a "-no" option ever get passed to cc1?

On Thu, Jul 21, 2011 at 08:06:49PM +0000, Joseph S. Myers wrote:
> On Wed, 13 Jul 2011, Jakub Jelinek wrote:
> 
> > Ideally we'd just include explicitly passed options from command line that
> > haven't been overridden by other command line options, and would sort them,
> > so that there are higher chances of DW_AT_producer strings being merged
> > (e.g. -O2 -ffast-math vs. -ffast-math -O2 are now different strings, and
> > similarly -O2 vs. -O3 -O2 vs. -O0 -O1 -Ofast -O2), but I'm not sure if it is
> > easily possible using current option handling framework.
> 
> I don't think there's enough structure for that, although prune_options 
> could always be enhanced to cover more cases of overridden options.

Perhaps we can emit it in the original order even with duplicates for now
and if we get better infrastructure later, reduce its size.

> > +	  default:
> > +	    if (save_decoded_options[j].orig_option_with_args_text[0] != '-')
> > +	      continue;
> 
> Should instead be ignoring OPT_SPECIAL_*.

It already ignores them above:
+      case OPT_SPECIAL_unknown:
+      case OPT_SPECIAL_ignore:
+      case OPT_SPECIAL_program_name:
+      case OPT_SPECIAL_input_file:

I just didn't want to crash if it wouldn't start with -.
I can make it a gcc_checking_assert instead.

> 
> > +	    switch (save_decoded_options[j].orig_option_with_args_text[1])
> > +	      {
> > +	      case 'M':
> > +	      case 'i':
> > +	      case 'W':
> 
> It would be better to check the canonical option text rather than the 
> original text.  Normally this wouldn't make any difference because the 
> driver will have passed canonical options to cc1, but if someone calls cc1 
> directly then I think aliases should still act the same as the options 
> they are aliases for in this regard.

So 

	gcc_checking_assert (save_decoded_options[j].canonical_option[0][0] == '-');
	switch (save_decoded_options[j].canonical_option[0][1])

instead?  The reason for checking the option text instead of code
was just because there are hundreds of -W options etc. and I didn't want to
list them all and create a maintanance nightmare.

> 
> > +		continue;
> > +	      case 'n':
> > +		if (save_decoded_options[j].orig_option_with_args_text[2]
> > +		    == 'o')
> > +		  continue;
> 
> When would a "-no" option ever get passed to cc1?

If -no can't make it to cc1, I'll drop it.  Is -fdump* checking that way
ok (with the orig_option_with_args_text -> canonical_option[0] change)?

	Jakub

On Thu, 21 Jul 2011, Jakub Jelinek wrote:

> So 
> 
> 	gcc_checking_assert (save_decoded_options[j].canonical_option[0][0] == '-');
> 	switch (save_decoded_options[j].canonical_option[0][1])
> 
> instead?  The reason for checking the option text instead of code

Yes.

> was just because there are hundreds of -W options etc. and I didn't want to
> list them all and create a maintanance nightmare.

Flags such as CL_WARNING and the various bit-fields in struct cl_option 
(bit-fields are preferred if --help doesn't need to care about a property 
of an option) could be used, but I don't think it would really be an 
improvement in this case.

> If -no can't make it to cc1, I'll drop it.  Is -fdump* checking that way
> ok (with the orig_option_with_args_text -> canonical_option[0] change)?

Yes, that seems reasonable.