[RFC] Use gfc_decl_attributes in fortran frontend

Hi,

Consider this patch, which reduces max_len of the oacc function 
attribute to 0:
...
...

The patch is obviously incorrect, but the idea here is to try to trigger 
this error in decl_attributes:
...
       else if (list_length (args) < spec->min_length
                || (spec->max_length >= 0
                    && list_length (args) > spec->max_length))
	{
           error ("wrong number of arguments specified for %qE"
                  " attribute",
                  name);
...

When running goacc.exp=routine-4.f90, we trigger the error, but then run 
into an assert.

The assert is caused by the fact that %qE is not handled by the fortran 
format decoder gfc_format_decoder, so this assert triggers in pp_format:
...
     ok = pp_format_decoder (pp) (pp, text, p,
				 precision, wide, plus, hash);
     gcc_assert (ok);
...


So, it seems that we call decl_attributes from the fortran frontend 
without installing a format decoder that can handle any potential errors.

This patch attempts to fix that, but having little experience in both 
diagnostics and fortran frontend, I'm not sure if this is the right way.

After applying the patch, the assert is fixed and we can see the actual 
error without having to start up the debugger:
...
src/gcc/testsuite/gfortran.dg/goacc/routine-4.f90:121:0: Error: wrong 
number of arguments specified for ‘oacc function’ attribute
...

Thanks,
- Tom

On 17/12/15 13:08, Tom de Vries wrote:
> Hi,
>
> Consider this patch, which reduces max_len of the oacc function
> attribute to 0:
> ...
> diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
> index 8556b70..60f4ad3 100644
> --- a/gcc/fortran/f95-lang.c
> +++ b/gcc/fortran/f95-lang.c
> @@ -93,7 +93,7 @@ static const struct attribute_spec
> gfc_attribute_table[] =
>          affects_type_identity } */
>     { "omp declare target", 0, 0, true,  false, false,
>       gfc_handle_omp_declare_target_attribute, false },
> -  { "oacc function", 0, -1, true,  false, false,
> +  { "oacc function", 0, 0, true,  false, false,
>       gfc_handle_omp_declare_target_attribute, false },
>     { NULL,                0, 0, false, false, false, NULL, false }
>   };
> ...
>
> The patch is obviously incorrect, but the idea here is to try to trigger
> this error in decl_attributes:
> ...
>        else if (list_length (args) < spec->min_length
>                 || (spec->max_length >= 0
>                     && list_length (args) > spec->max_length))
>      {
>            error ("wrong number of arguments specified for %qE"
>                   " attribute",
>                   name);
> ...
>
> When running goacc.exp=routine-4.f90, we trigger the error, but then run
> into an assert.
>
> The assert is caused by the fact that %qE is not handled by the fortran
> format decoder gfc_format_decoder, so this assert triggers in pp_format:
> ...
>      ok = pp_format_decoder (pp) (pp, text, p,
>                   precision, wide, plus, hash);
>      gcc_assert (ok);
> ...
>
>
> So, it seems that we call decl_attributes from the fortran frontend
> without installing a format decoder that can handle any potential errors.
>
> This patch attempts to fix that, but having little experience in both
> diagnostics and fortran frontend, I'm not sure if this is the right way.
>
> After applying the patch, the assert is fixed and we can see the actual
> error without having to start up the debugger:
> ...
> src/gcc/testsuite/gfortran.dg/goacc/routine-4.f90:121:0: Error: wrong
> number of arguments specified for ‘oacc function’ attribute
> ...
>
> Thanks,
> - Tom
>
> 0001-Use-gfc_decl_attributes-in-fortran-frontend.patch
>
>
> Use gfc_decl_attributes in fortran frontend
>
> ---
>   gcc/fortran/error.c      | 18 ++++++++++++++++--
>   gcc/fortran/gfortran.h   |  2 ++
>   gcc/fortran/trans-decl.c | 18 ++++++++++++++----
>   3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
> index 8f57aff..fd66d75 100644
> --- a/gcc/fortran/error.c
> +++ b/gcc/fortran/error.c
> @@ -1417,11 +1417,18 @@ gfc_errors_to_warnings (bool f)
>   }
>
>   void
> -gfc_diagnostics_init (void)
> +gfc_diagnostics_fortran (void)
>   {
>     diagnostic_starter (global_dc) = gfc_diagnostic_starter;
>     diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
>     diagnostic_format_decoder (global_dc) = gfc_format_decoder;
> +}
> +
> +void
> +gfc_diagnostics_init (void)
> +{
> +  gfc_diagnostics_fortran ();
> +
>     global_dc->caret_chars[0] = '1';
>     global_dc->caret_chars[1] = '2';
>     pp_warning_buffer = new (XNEW (output_buffer)) output_buffer ();
> @@ -1433,13 +1440,20 @@ gfc_diagnostics_init (void)
>   }
>
>   void
> -gfc_diagnostics_finish (void)
> +gfc_diagnostics_tree (void)
>   {
>     tree_diagnostics_defaults (global_dc);
>     /* We still want to use the gfc starter and finalizer, not the tree
>        defaults.  */
>     diagnostic_starter (global_dc) = gfc_diagnostic_starter;
>     diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
> +}
> +
> +void
> +gfc_diagnostics_finish (void)
> +{
> +  gfc_diagnostics_tree ();
> +
>     global_dc->caret_chars[0] = '^';
>     global_dc->caret_chars[1] = '^';
>   }
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index d203c32..1f7cdc2 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -2687,6 +2687,8 @@ bool gfc_find_sym_in_expr (gfc_symbol *, gfc_expr *);
>   void gfc_error_init_1 (void);
>   void gfc_diagnostics_init (void);
>   void gfc_diagnostics_finish (void);
> +void gfc_diagnostics_fortran (void);
> +void gfc_diagnostics_tree (void);
>   void gfc_buffer_error (bool);
>
>   const char *gfc_print_wide_char (gfc_char_t);
> diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
> index 8c4fa03..9ed1d07 100644
> --- a/gcc/fortran/trans-decl.c
> +++ b/gcc/fortran/trans-decl.c
> @@ -1326,6 +1326,16 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
>   }
>
>
> +static tree
> +gfc_decl_attributes (tree *node, tree attributes, int flags)
> +{
> +  tree res;
> +  gfc_diagnostics_tree ();
> +  res = decl_attributes (node, attributes, flags);
> +  gfc_diagnostics_fortran ();
> +  return res;
> +}
> +
>   static void build_function_decl (gfc_symbol * sym, bool global);
>
>
> @@ -1567,7 +1577,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
>
>     /* Add attributes to variables.  Functions are handled elsewhere.  */
>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
> -  decl_attributes (&decl, attributes, 0);
> +  gfc_decl_attributes (&decl, attributes, 0);
>
>     /* Symbols from modules should have their assembler names mangled.
>        This is done here rather than in gfc_finish_var_decl because it
> @@ -1802,7 +1812,7 @@ get_proc_pointer_decl (gfc_symbol *sym)
>       set_decl_tls_model (decl, decl_default_tls_model (decl));
>
>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
> -  decl_attributes (&decl, attributes, 0);
> +  gfc_decl_attributes (&decl, attributes, 0);
>
>     return decl;
>   }
> @@ -1995,7 +2005,7 @@ module_sym:
>     TREE_PUBLIC (fndecl) = 1;
>
>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
> -  decl_attributes (&fndecl, attributes, 0);
> +  gfc_decl_attributes (&fndecl, attributes, 0);
>
>     gfc_set_decl_assembler_name (fndecl, mangled_name);
>
> @@ -2097,7 +2107,7 @@ build_function_decl (gfc_symbol * sym, bool global)
>       TREE_USED (fndecl) = 1;
>
>     attributes = add_attributes_to_decl (attr, NULL_TREE);
> -  decl_attributes (&fndecl, attributes, 0);
> +  gfc_decl_attributes (&fndecl, attributes, 0);
>
>     /* Figure out the return type of the declared function, and build a
>        RESULT_DECL for it.  If this is a subroutine with alternate
>

On 01/13/2016 05:21 PM, Tom de Vries wrote:
> On 17/12/15 13:08, Tom de Vries wrote:
>> Hi,
>>
>> Consider this patch, which reduces max_len of the oacc function
>> attribute to 0:
>> ...
>> diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
>> index 8556b70..60f4ad3 100644
>> --- a/gcc/fortran/f95-lang.c
>> +++ b/gcc/fortran/f95-lang.c
>> @@ -93,7 +93,7 @@ static const struct attribute_spec
>> gfc_attribute_table[] =
>>          affects_type_identity } */
>>     { "omp declare target", 0, 0, true,  false, false,
>>       gfc_handle_omp_declare_target_attribute, false },
>> -  { "oacc function", 0, -1, true,  false, false,
>> +  { "oacc function", 0, 0, true,  false, false,
>>       gfc_handle_omp_declare_target_attribute, false },
>>     { NULL,                0, 0, false, false, false, NULL, false }
>>   };
>> ...
>>
>> The patch is obviously incorrect, but the idea here is to try to trigger
>> this error in decl_attributes:
>> ...
>>        else if (list_length (args) < spec->min_length
>>                 || (spec->max_length >= 0
>>                     && list_length (args) > spec->max_length))
>>      {
>>            error ("wrong number of arguments specified for %qE"
>>                   " attribute",
>>                   name);
>> ...
>>
>> When running goacc.exp=routine-4.f90, we trigger the error, but then run
>> into an assert.
>>
>> The assert is caused by the fact that %qE is not handled by the fortran
>> format decoder gfc_format_decoder, so this assert triggers in pp_format:
>> ...
>>      ok = pp_format_decoder (pp) (pp, text, p,
>>                   precision, wide, plus, hash);
>>      gcc_assert (ok);
>> ...
>>
>>
>> So, it seems that we call decl_attributes from the fortran frontend
>> without installing a format decoder that can handle any potential errors.
>>
>> This patch attempts to fix that, but having little experience in both
>> diagnostics and fortran frontend, I'm not sure if this is the right way.
>>
>> After applying the patch, the assert is fixed and we can see the actual
>> error without having to start up the debugger:
>> ...
>> src/gcc/testsuite/gfortran.dg/goacc/routine-4.f90:121:0: Error: wrong
>> number of arguments specified for ‘oacc function’ attribute
>> ...
>>
>> Thanks,
>> - Tom
>>
>> 0001-Use-gfc_decl_attributes-in-fortran-frontend.patch
>>
>>
>> Use gfc_decl_attributes in fortran frontend
>>
>> ---
>>   gcc/fortran/error.c      | 18 ++++++++++++++++--
>>   gcc/fortran/gfortran.h   |  2 ++
>>   gcc/fortran/trans-decl.c | 18 ++++++++++++++----
>>   3 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
>> index 8f57aff..fd66d75 100644
>> --- a/gcc/fortran/error.c
>> +++ b/gcc/fortran/error.c
>> @@ -1417,11 +1417,18 @@ gfc_errors_to_warnings (bool f)
>>   }
>>
>>   void
>> -gfc_diagnostics_init (void)
>> +gfc_diagnostics_fortran (void)
>>   {
>>     diagnostic_starter (global_dc) = gfc_diagnostic_starter;
>>     diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
>>     diagnostic_format_decoder (global_dc) = gfc_format_decoder;
>> +}
>> +
>> +void
>> +gfc_diagnostics_init (void)
>> +{
>> +  gfc_diagnostics_fortran ();
>> +
>>     global_dc->caret_chars[0] = '1';
>>     global_dc->caret_chars[1] = '2';
>>     pp_warning_buffer = new (XNEW (output_buffer)) output_buffer ();
>> @@ -1433,13 +1440,20 @@ gfc_diagnostics_init (void)
>>   }
>>
>>   void
>> -gfc_diagnostics_finish (void)
>> +gfc_diagnostics_tree (void)
>>   {
>>     tree_diagnostics_defaults (global_dc);
>>     /* We still want to use the gfc starter and finalizer, not the tree
>>        defaults.  */
>>     diagnostic_starter (global_dc) = gfc_diagnostic_starter;
>>     diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
>> +}
>> +
>> +void
>> +gfc_diagnostics_finish (void)
>> +{
>> +  gfc_diagnostics_tree ();
>> +
>>     global_dc->caret_chars[0] = '^';
>>     global_dc->caret_chars[1] = '^';
>>   }
>> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
>> index d203c32..1f7cdc2 100644
>> --- a/gcc/fortran/gfortran.h
>> +++ b/gcc/fortran/gfortran.h
>> @@ -2687,6 +2687,8 @@ bool gfc_find_sym_in_expr (gfc_symbol *, 
>> gfc_expr *);
>>   void gfc_error_init_1 (void);
>>   void gfc_diagnostics_init (void);
>>   void gfc_diagnostics_finish (void);
>> +void gfc_diagnostics_fortran (void);
>> +void gfc_diagnostics_tree (void);
>>   void gfc_buffer_error (bool);
>>
>>   const char *gfc_print_wide_char (gfc_char_t);
>> diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
>> index 8c4fa03..9ed1d07 100644
>> --- a/gcc/fortran/trans-decl.c
>> +++ b/gcc/fortran/trans-decl.c
>> @@ -1326,6 +1326,16 @@ add_attributes_to_decl (symbol_attribute 
>> sym_attr, tree list)
>>   }
>>
>>
>> +static tree
>> +gfc_decl_attributes (tree *node, tree attributes, int flags)
>> +{
>> +  tree res;
>> +  gfc_diagnostics_tree ();
>> +  res = decl_attributes (node, attributes, flags);
>> +  gfc_diagnostics_fortran ();
>> +  return res;
>> +}
>> +
>>   static void build_function_decl (gfc_symbol * sym, bool global);
>>
>>
>> @@ -1567,7 +1577,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
>>
>>     /* Add attributes to variables.  Functions are handled elsewhere.  */
>>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
>> -  decl_attributes (&decl, attributes, 0);
>> +  gfc_decl_attributes (&decl, attributes, 0);
>>
>>     /* Symbols from modules should have their assembler names mangled.
>>        This is done here rather than in gfc_finish_var_decl because it
>> @@ -1802,7 +1812,7 @@ get_proc_pointer_decl (gfc_symbol *sym)
>>       set_decl_tls_model (decl, decl_default_tls_model (decl));
>>
>>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
>> -  decl_attributes (&decl, attributes, 0);
>> +  gfc_decl_attributes (&decl, attributes, 0);
>>
>>     return decl;
>>   }
>> @@ -1995,7 +2005,7 @@ module_sym:
>>     TREE_PUBLIC (fndecl) = 1;
>>
>>     attributes = add_attributes_to_decl (sym->attr, NULL_TREE);
>> -  decl_attributes (&fndecl, attributes, 0);
>> +  gfc_decl_attributes (&fndecl, attributes, 0);
>>
>>     gfc_set_decl_assembler_name (fndecl, mangled_name);
>>
>> @@ -2097,7 +2107,7 @@ build_function_decl (gfc_symbol * sym, bool global)
>>       TREE_USED (fndecl) = 1;
>>
>>     attributes = add_attributes_to_decl (attr, NULL_TREE);
>> -  decl_attributes (&fndecl, attributes, 0);
>> +  gfc_decl_attributes (&fndecl, attributes, 0);
>>
>>     /* Figure out the return type of the declared function, and build a
>>        RESULT_DECL for it.  If this is a subroutine with alternate
>>
>