Re: [PATCH] PR86957

[Martin Liška <mliska at suse dot cz>]

On 09/05/2018 09:28 PM, Indu Bhagat wrote:

Hi.

Thanks for working on that. I believe it's useful enhancement. Note that I'm not profile feedback maintainer,
but I'll provide some feedback:

> Patch for PR 86957 " gcc should warn about missing profiles for a compilation unit or a new function with -fprofile-use".
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86957
> 
> The patch adds -Wmissing-profile warning flag to alert user about cases when there is no profile data for a compilation unit or a function when using -fprofile-use.
> 
> The flag is on by default when -fprofile-use is specified and generates errors
> by default (like -Wcoverage-mismatch).
> 
> The attachment pr86957-missing-profile-diagnostic shows the behavior of GCC with the patch.
> 
> Thanks
> Indu
> 
> ------------------------------------------------------
> 
> 
> gcc/ChangeLog:
> 
> 2018-09-05  "Indu Bhagat"  <"indu.bhagat@oracle.com">
> 
>         * common.opt: New warning option -Wmissing-profile.
>         * coverage.c (get_coverage_counts): Add warning for missing .gcda file.
>         * doc/invoke.texi: Document -Wmissing-profile.
>         * profile.c (get_exec_counts): Add warning for missing feedback
>           profile for a function.
>         * toplev.c (process_options): -Wmissing-profile warning as error.
> 
> 
> pr86957-patch
> 
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index ebc3ef4..d93ddca 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -811,6 +811,10 @@ Wcoverage-mismatch
>  Common Var(warn_coverage_mismatch) Init(1) Warning
>  Warn in case profiles in -fprofile-use do not match.
>  
> +Wmissing-profile
> +Common Var(warn_missing_profile) Init(1) Warning
> +Warn in case profiles in -fprofile-use do not exist.

Maybe 'Want about missing profile for a function in -fprofile-use build.' ?

> +
>  Wvector-operation-performance
>  Common Var(warn_vector_operation_performance) Warning
>  Warn when a vector operation is compiled outside the SIMD.
> diff --git a/gcc/coverage.c b/gcc/coverage.c
> index bae6f5c..df031df 100644
> --- a/gcc/coverage.c
> +++ b/gcc/coverage.c
> @@ -341,16 +341,24 @@ get_coverage_counts (unsigned counter, unsigned expected,
>      {
>        static int warned = 0;
>  
> -      if (!warned++ && dump_enabled_p ())
> +      if (!warned++)
>  	{
> -	  dump_user_location_t loc
> -	    = dump_user_location_t::from_location_t (input_location);
> -	  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
> +	  warning (OPT_Wmissing_profile,
> +		   "%qs profile count data file not found,"
> +		   " regenerating profiles may help",

I would not append 'regenerating profiles may help'. We don't do it for other
profile corruption warnings/errors.

> +		   da_file_name);
> +	  if (dump_enabled_p ())
> +	    {
> +	      dump_user_location_t loc
> +		= dump_user_location_t::from_location_t (input_location);
> +	      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
> +			       "file %s not found\n",
> +			       da_file_name);
> +	      dump_printf (MSG_OPTIMIZED_LOCATIONS,
>  			   (flag_guess_branch_prob
> -			    ? "file %s not found, execution counts estimated\n"
> -			    : "file %s not found, execution counts assumed to "
> -			    "be zero\n"),
> -			   da_file_name);
> +			    ? "execution counts estimated\n"
> +			    : "execution counts assumed to be zero\n"));
> +	    }
>  	}
>        return NULL;
>      }
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index ca92028..e62bcae 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -314,7 +314,7 @@ Objective-C and Objective-C++ Dialects}.
>  -Wlogical-op  -Wlogical-not-parentheses  -Wlong-long @gol
>  -Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset-transposed-args @gol
>  -Wmisleading-indentation  -Wmissing-attributes -Wmissing-braces @gol
> --Wmissing-field-initializers  -Wmissing-include-dirs @gol
> +-Wmissing-field-initializers  -Wmissing-include-dirs  -Wmissing-profile @gol
>  -Wno-multichar  -Wmultistatement-macros  -Wnonnull  -Wnonnull-compare @gol
>  -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
>  -Wnull-dereference  -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
> @@ -4816,6 +4816,25 @@ This warning is enabled by @option{-Wall}.
>  @opindex Wno-missing-include-dirs
>  Warn if a user-supplied include directory does not exist.
>  
> +@item -Wmissing-profile
> +@opindex Wmissing-profile
> +@opindex Wno-missing-profile
> +Warn if feedback profiles are missing when using the
> +@option{-fprofile-use} option.
> +If a new function or a new file is added to the user code between compiling
> +with @option{-fprofile-gen} and with @option{-fprofile-use} without

s/profile-gen/profile-generate

> +regenerating the profiles, the profile feedback data files do not contain any
> +profile feedback information for the newly
> +added function or file respectively. 

I would suggest to split the sentence.

 Also, in the case when profile count data
> +(.gcda) files are wiped out, GCC cannot use any profile feedback
> +information.  In all these cases, warnings are issued to inform the user that a
> +profile generation step is due.  By default, this warning is enabled and is
> +treated as an error.  @option{-Wno-missing-profile} can be used to disable the
> +warning or @option{-Wno-error=missing-profile} can be used to disable the
> +error.  Disabling the error for this warning can result in poorly optimized
> +code and is useful only in the cases when non-existent profile data is
> +justified.  Completely disabling the warning is not recommended.
> +
>  @item -Wmultistatement-macros
>  @opindex Wmultistatement-macros
>  @opindex Wno-multistatement-macros
> @@ -9899,8 +9918,10 @@ Before you can use this option, you must first generate profiling information.
>  
>  By default, GCC emits an error message if the feedback profiles do not
>  match the source code.  This error can be turned into a warning by using
> -@option{-Wcoverage-mismatch}.  Note this may result in poorly optimized
> -code.
> +@option{-Wno-error=coverage-mismatch}.  Note this may result in poorly
> +optimized code.  Additionally, by default, GCC also emits an error message if
> +the feedback profiles do not exist.  The latter error can be turned into a
> +warning by using @option{-Wno-error=missing-profile}.
>  
>  If @var{path} is specified, GCC looks at the @var{path} to find
>  the profile feedback data files. See @option{-fprofile-dir}.
> diff --git a/gcc/profile.c b/gcc/profile.c
> index cb51e0d..40f9763 100644
> --- a/gcc/profile.c
> +++ b/gcc/profile.c
> @@ -286,7 +286,13 @@ get_exec_counts (unsigned cfg_checksum, unsigned lineno_checksum)
>    counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, cfg_checksum,
>  				lineno_checksum, &profile_info);
>    if (!counts)
> -    return NULL;
> +    {
> +      warning (OPT_Wmissing_profile,
> +	       "profile for function %qE not found in profile data,"
> +	       " regenerating profiles may help",
> +	       DECL_ASSEMBLER_NAME (current_function_decl));
> +      return NULL;
> +    }

I hope we can do better here:

      warning_at (DECL_SOURCE_LOCATION (current_function_decl),
		  OPT_Wmissing_profile,
		  "profile for function %qD not found in profile data",
		  current_function_decl);

Then you'll see better location:

main.c:19:5: warning: profile for function ‘main’ not found in profile data [-Wmissing-profile]
19 | int main(int argc, char **argv)
   |     ^~~~

I'm wondering whether we want to report missing profiles for functions that live
in a file for which we reported missing .gcda file?

main.c: In function ‘main’:
main.c:22:1: warning: ‘/tmp/main.gcda’ profile count data file not found, regenerating profiles may help [-Wmissing-profile]
22 | }
   | ^
main.c:19:5: warning: profile for function ‘main’ not found in profile data [-Wmissing-profile]
19 | int main(int argc, char **argv)
   |     ^~~~
main.c: In function ‘bar’:
main.c:14:1: warning: profile for function ‘bar’ not found in profile data [-Wmissing-profile]
14 | bar()
   | ^~~
main.c: In function ‘foo’:
main.c:7:1: warning: profile for function ‘foo’ not found in profile data [-Wmissing-profile]
7 | foo()
  | ^~~

I would limit that just to the first warning. Otherwise one can end up with a wall of text.

>  
>    get_working_sets ();
>  
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 9fb83d4..5891554 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1782,14 +1782,24 @@ process_options (void)
>        || !targetm.have_prologue () || !targetm.have_epilogue ())
>      flag_ipa_ra = 0;
>  
> -  /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> -     have not been set.  */
> -  if (!global_options_set.x_warnings_are_errors
> -      && warn_coverage_mismatch
> -      && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
> -          DK_UNSPECIFIED))
> -    diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_mismatch,
> -                                    DK_ERROR, UNKNOWN_LOCATION);
> +  if (!global_options_set.x_warnings_are_errors)
> +    {
> +      /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
> +	 have not been set.  */
> +      if (warn_coverage_mismatch
> +	  && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
> +	      DK_UNSPECIFIED))
> +	diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_mismatch,
> +					DK_ERROR, UNKNOWN_LOCATION);
> +
> +      /* Enable -Werror=missing-profile when -Werror and -Wno-error
> +	 have not been set.  */
> +      if (warn_missing_profile
> +	  && (global_dc->classify_diagnostic[OPT_Wmissing_profile] ==
> +	      DK_UNSPECIFIED))
> +	diagnostic_classify_diagnostic (global_dc, OPT_Wmissing_profile,
> +					DK_ERROR, UNKNOWN_LOCATION);
> +    }

I would prefer not to enable error of the option. Mainly due to configure
scripts that have missing profiles (example from postgres):

configure: loading site script /usr/share/site/x86_64-unknown-linux-gnu
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking which template to use... linux
checking whether NLS is wanted... no
checking for default port number... 5432
checking for block size... 8kB
checking for segment size... 1GB
checking for WAL block size... 8kB
checking for gcc... gcc
checking whether the C compiler works... no
configure: error: in `/home/marxin/Programming/postgres':
configure: error: C compiler cannot create executables
See `config.log' for more details

configure:3942: gcc -O2 -fprofile-use -g  -O2 -fprofile-use -g conftest.c  >&5
conftest.c: In function 'main':
conftest.c:23:1: error: '/home/marxin/Programming/postgres/conftest.gcda' profile count data file not found, regenerating profiles may help [-Werror=missing-profile]
23 | }
   | ^
conftest.c:23:1: error: profile for function 'main' not found in profile data, regenerating profiles may help [-Werror=missing-profile]

Anyway, thanks for working on that.

Martin

>  
>    /* Save the current optimization options.  */
>    optimization_default_node = build_optimization_node (&global_options);
> 
> 
> pr86957-missing-profile-diagnostic
> 
> 
> [Testcase 1] Missing profile data file with -fprofile-use. The sort.c file contains two functions main() and stop()
> 
> gcc -c sort.c -fprofile-use -O1 -fprofile-dir=/data2/user/gcc-temp/fdo/profdata/
> sort.c: In function ‘main’:
> sort.c:29:1: error: ‘/data2/user/gcc-temp/fdo/profdata//#data2#user#gcc-temp#fdo#sort.gcda’ profile count data file not found, regenerating profiles may help [-Werror=missing-profile]
> 29 | }
>    | ^
> sort.c:29:1: error: profile for function ‘main’ not found in profile data, regenerating profiles may help [-Werror=missing-profile]
> sort.c: In function ‘stop’:
> sort.c:29:1: error: profile for function ‘stop’ not found in profile data, regenerating profiles may help [-Werror=missing-profile]
> cc1: some warnings being treated as errors
> make: *** [sort.o] Error 1
> 
> [Testcase 2] bubble_sort.c has a non-empty function bubble_sort() for which profile data exists.
> Now at profile-use phase, a user adds empty function before1() before an existing lone function bubble_sort() and empty function after1() after the lone existing function bubble_sort()
> 
> gcc -c bubble_sort.c -fprofile-use -O1 -fprofile-dir=/data2/user/gcc-temp/fdo/profdata/
> bubble_sort.c: In function ‘after1’:
> bubble_sort.c:19:1: error: profile for function ‘after1’ not found in profile data, regenerating profiles may help [-Werror=missing-profile]
> 19 | void after1() { }
>    | ^~~~
> bubble_sort.c: In function ‘bubble_sort’:
> bubble_sort.c:19:1: error: source locations for function ‘bubble_sort’ have changed, the profile data may be out of date [-Werror=coverage-mismatch]
> bubble_sort.c: In function ‘before1’:
> bubble_sort.c:19:1: error: profile for function ‘before1’ not found in profile data, regenerating profiles may help [-Werror=missing-profile]
> cc1: some warnings being treated as errors
> make: *** [bubble_sort.o] Error 1
> 
> [Testcase 3] Use -Wno-error=missing-profile to treat them as warnings, not errors
> 
> gcc -c bubble_sort.c -fprofile-use -O1 -Wno-error=missing-profile -fprofile-dir=/data2/user/gcc-temp/fdo/profdata/
> bubble_sort.c: In function ‘after1’:
> bubble_sort.c:19:1: warning: profile for function ‘after1’ not found in profile data, regenerating profiles may help [-Wmissing-profile]
> 19 | void after1() { }
>    | ^~~~
> bubble_sort.c: In function ‘bubble_sort’:
> bubble_sort.c:19:1: error: source locations for function ‘bubble_sort’ have changed, the profile data may be out of date [-Werror=coverage-mismatch]
> bubble_sort.c: In function ‘before1’:
> bubble_sort.c:19:1: warning: profile for function ‘before1’ not found in profile data, regenerating profiles may help [-Wmissing-profile]
> cc1: some warnings being treated as errors
> make: *** [bubble_sort.o] Error 1 
>