[stage1][PATCH] Add gcc_assert that &global_options are not dirty modified.

[Martin Liška]

On 3/19/20 5:13 PM, Martin Sebor wrote:
> On 3/19/20 9:32 AM, Martin Liška wrote:
>> On 3/19/20 10:09 AM, Jakub Jelinek wrote:
>>> I mean, optimize for the !flag_checking case...
>>>> Sure, I transformed both situations into heap memory allocation.
>> In gcc/c-family/c-attribs.c I faced maybe uninitialized warning when
>> I only assigned (and checked) the variable in flag_checking context.
> 

Hi Martin.

> I was mostly just curious about what was being checked and how so
> I might be misunderstanding something but it looks to me like
> the code generated by the loop below will be a very long series
> (of hundreds?) of if statements, each doing the same thing but
> each hardcoding different options names.  If that's correct, is
> there an easy way to turn that repetitive series into a loop to
> keep code (and string) size growth to a minimum?

Thank you very much for the feedback, it was useful. I realized that
the patch made a huge code bloat (mainly because of the string constants
and the fact that it didn't end quickly (with an internal_error).

The loop is here not possible because we compare struct fields.

> 
> Also, since the function prints output and the caller then aborts
> on failure, would calling internal_error for the first mismatch
> instead be more in keeping with how internal errors with additional
> output are reported?

I decided to call internal_error without string description of the error.
With that I have a reasonable code growth:

bloaty /tmp/cc1plus-after -- /tmp/cc1plus-before
      VM SIZE                     FILE SIZE
  --------------               --------------
   +0.1% +20.9Ki .text         +20.9Ki  +0.1%
   [ = ]       0 .debug_line   +2.38Ki  +0.0%
   [ = ]       0 .debug_info      +369  +0.0%
   [ = ]       0 .debug_str        +75  +0.0%
   +0.0%     +64 .rodata           +64  +0.0%
   [ = ]       0 .debug_ranges     +48  +0.0%
   +0.0%     +45 .dynstr           +45  +0.0%
   [ = ]       0 .strtab           +45  +0.0%
   +0.0%     +40 .eh_frame         +40  +0.0%
   +0.0%     +24 .dynsym           +24  +0.0%
   [ = ]       0 .symtab           +24  +0.0%
   +0.0%      +8 .eh_frame_hdr      +8  +0.0%
   +0.0%      +4 .gnu.hash          +4  +0.0%
   +0.0%      +4 .hash              +4  +0.0%
   +0.0%      +2 .gnu.version       +2  +0.0%
    +14%      +1 [LOAD [R]]         +1   +14%
   [ = ]       0 .debug_loc       -355  -0.0%
   [ = ]       0 [Unmapped]    -1.05Ki -36.5%
   +0.1% +21.0Ki TOTAL         +22.6Ki  +0.0%

So for debugging one will have to take a look at corresponding option-save.c line.
It does not seem to me a problem.

> 
> One last question/suggestion: if the efficiency of the checking is
> at all a concern, would calling memcmp on the arrays first and only
> looping to find the position of the mismatch, be viable? (I have no
> idea if changes to some of the options are acceptable; if they are
> this wouldn't work).

Well, I skip some fields in the struct and there can be string pointers, so I'm suggesting
not to use memcmp.

There's updated tested patch.
Martin

> 
> Martin
> 
> PS Since the function doesn't modify the option arrays it could
> declare them const.
> 
> diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
> index 856a69168a5..586213da3d3 100644
> --- a/gcc/opth-gen.awk
> +++ b/gcc/opth-gen.awk
> @@ -119,6 +119,41 @@ print "#endif"
>   print "#endif"
>   print ""
> 
> +print "#if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS)"
> +print "#ifndef GENERATOR_FILE"
> +print "static inline bool gcc_options_check (gcc_options *ptr1, gcc_options *ptr2)"
> +print "{"
> +print "  bool result = true;"
> +
> +# all these options are mentioned in PR92860
> +checked_options["flag_merge_constants"]++
> +checked_options["param_max_fields_for_field_sensitive"]++
> +checked_options["flag_omit_frame_pointer"]++
> +checked_options["unroll_only_small_loops"]++
> +
> +for (i = 0; i < n_opts; i++) {
> +    name = var_name(flags[i]);
> +    if (name == "")
> +        continue;
> +
> +    if (name in checked_options)
> +        continue;
> +    checked_options[name]++
> +
> +    print "  if (ptr1->x_" name " != ptr2->x_" name ")"
> +    print "  {"
> +    print "    if (result)"
> +    print "      fprintf (stderr, \"Error: global_options are modified in local context:\\n\");"
> +    print "    fprintf (stderr, \"  " name " (%ld/%ld)\\n\", (long int)ptr1->x_" name ", (long int)ptr2->x_" name ");"
> +    print "    result = false;"
> +    print "  }"
> +}
> +
> +print "  return result;"
> +print "}"
> +print "#endif"
> +print "#endif"
> +
>   # All of the optimization switches gathered together so they can be saved and restored.
>   # This will allow attribute((cold)) to turn on space optimization.
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-gcc_assert-that-global_options-are-not-dirty-mod.patch
Type: text/x-patch
Size: 4963 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20200320/da701405/attachment.bin>