[PATCH][RFC] Come up with -flive-patching master option.

[Martin Liška]

On 11/13/18 10:16 PM, Qing Zhao wrote:
> Hi,
> 
>> On Nov 13, 2018, at 1:18 PM, Miroslav Benes <mbenes@suse.cz> wrote:
>>
>>> Attached is the patch for new -flive-patching=[inline-only-static | inline-clone] master option.
>>>
>>> '-flive-patching=LEVEL'
>>>     Control GCC's optimizations to provide a safe compilation for
>>>     live-patching.  Provides multiple-level control on how many of the
>>>     optimizations are enabled by users' request.  The LEVEL argument
>>>     should be one of the following:
>>>
>>>     'inline-only-static'
>>>
>>>          Only enable inlining of static functions, disable all other
>>>          ipa optimizations/analyses.  As a result, when patching a
>>>          static routine, all its callers need to be patches as well.
>>>
>>>     'inline-clone'
>>>
>>>          Only enable inlining and all optimizations that internally
>>>          create clone, for example, cloning, ipa-sra, partial inlining,
>>>          etc.; disable all other ipa optimizations/analyses.  As a
>>>          result, when patching a routine, all its callers and its
>>>          clones' callers need to be patched as well.
>> Based on our previous discussion I assume that "clone" optimizations are 
>> safe (for LP) and the others are not. Anyway I'd welcome a note mentioning 
>> that disabled optimizations are dangerous for LP.
> actually, I don’t think that those disabled optimizations are “dangerous” for live-patching. one of the major reasons we disable them
> is because that currently the compiler does NOT provide a good way to compute the impacted function list for those optimizations.
> therefore, we disable them at this time. 
> 
> many of them could be enabled too if the compiler can report the impacted function list accurately in the future.
> 
> 
> 
>> I know it may be the same for you, but it is not for me as a GCC user. 
>> "internally create clone" sounds very... well, internal. It does not 
>> describe the option much for ordinary user whow has no knowledge about GCC 
>> internals.
>>
>> So could you rephrase it a bit, please?
> I tried to make this clear. please see the following:
> 
> '-flive-patching=LEVEL'
>      Control GCC's optimizations to provide a safe compilation for
>      live-patching.
> 
>      If the compiler's optimization uses a function's body or
>      information extracted from its body to optimize/change another
>      function, the latter is called an impacted function of the former.
>      If a function is patched, its impacted functions should be patched
>      too.
> 
>      The impacted functions are decided by the compiler's
>      interprocedural optimizations.  For example, inlining a function
>      into its caller, cloning a function and changing its caller to call
>      this new clone, or extracting a function's pureness/constness
>      information to optimize its direct or indirect callers, etc.
> 
>      Usually, the more ipa optimizations enabled, the larger the number
>      of impacted functions for each function.  In order to control the
>      number of impacted functions and computed the list of impacted
>      function easily, we provide control to partially enable ipa
>      optimizations on two different levels.
> 
>      The LEVEL argument should be one of the following:
> 
>      'inline-only-static'
> 
>           Only enable inlining of static functions, disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a static routine, all its callers need to be patches
>           as well.
> 
>      'inline-clone'
> 
>           Only enable inlining and cloning optimizations, which includes
>           inlining, cloning, interprocedural scalar replacement of
>           aggregates and partial inlining.  Disable all other
>           interprocedural optimizations/analyses.  As a result, when
>           patching a routine, all its callers and its clones' callers
>           need to be patched as well.
> 
>      When -flive-patching specified without any value, the default value
>      is "inline-clone".
> 
>      This flag is disabled by default.
> 
> 
>>>     When -flive-patching specified without any value, the default value
>>>     is "inline-clone".
>>>
>>>     This flag is disabled by default.
>>>
>>> let me know your comments and suggestions on the implementation.
>> I compared it to Martin's patch and ipa-icf-variables is not covered in 
>> yours (I may have missed something).
> Yes, you are right. I added this into my patch.
> 
> I am attaching the new patch here.

Hello.

Please use
git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py /tmp/patch

in order to address many formatting issues of the patch (skip the ones reported in common.opt).

> 
> 
> flive-patching.patch
> 
> From c0785675eb29599754aaf11c70901c12dd3ea821 Mon Sep 17 00:00:00 2001
> From: qing zhao <qing.zhao@oracle.com>
> Date: Tue, 13 Nov 2018 13:02:57 -0800
> Subject: [PATCH] Add -flive-patching to support live patching
> 
> ---
>  gcc/cif-code.def                       |  6 +++
>  gcc/common.opt                         | 18 +++++++++
>  gcc/doc/invoke.texi                    | 49 +++++++++++++++++++++++-
>  gcc/flag-types.h                       |  8 ++++
>  gcc/ipa-inline.c                       |  6 +++
>  gcc/opts.c                             | 68 ++++++++++++++++++++++++++++++++++
>  gcc/testsuite/gcc.dg/live-patching-1.c | 22 +++++++++++
>  7 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/live-patching-1.c
> 
> diff --git a/gcc/cif-code.def b/gcc/cif-code.def
> index 19a7621..dffe15f 100644
> --- a/gcc/cif-code.def
> +++ b/gcc/cif-code.def
> @@ -132,6 +132,12 @@ DEFCIFCODE(USES_COMDAT_LOCAL, CIF_FINAL_ERROR,
>  DEFCIFCODE(ATTRIBUTE_MISMATCH, CIF_FINAL_ERROR,
>  	   N_("function attribute mismatch"))
>  
> +/* We can't inline because the user requests only static functions 
> +   but the function has external linkage for live patching purpose.  */
> +DEFCIFCODE(EXTERN_LIVE_ONLY_STATIC, CIF_FINAL_ERROR,
> +	   N_("function has external linkage when the user requests only"
> +	      " inlining static for live patching"))
> +
>  /* We proved that the call is unreachable.  */
>  DEFCIFCODE(UNREACHABLE, CIF_FINAL_ERROR,
>  	   N_("unreachable"))
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 98e8eb0..346a361 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2152,6 +2152,24 @@ starts and when the destructor finishes.
>  flifetime-dse=
>  Common Joined RejectNegative UInteger Var(flag_lifetime_dse) Optimization IntegerRange(0, 2)
>  
> +flive-patching
> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
> +
> +flive-patching=
> +Common Report Joined RejectNegative Enum(live_patching_level) Var(flag_live_patching) Init(LIVE_NONE) Optimization
> +-flive-patching=[inline-only-static|inline-clone]	Control ipa optimizations to provide a 

Please use 'IPA' instead of 'ipa', similarly in documentation.

> +safe compilation for live-patching. At the same time, provides multiple-level control on the  
> +enabled optimizations. 
> +
> +Enum
> +Name(live_patching_level) Type(enum live_patching_level) UnknownError(unknown Live-Patching Level %qs)
> +
> +EnumValue
> +Enum(live_patching_level) String(inline-only-static) Value(LIVE_INLINE_ONLY_STATIC)
> +
> +EnumValue
> +Enum(live_patching_level) String(inline-clone) Value(LIVE_INLINE_CLONE)
> +
>  flive-range-shrinkage
>  Common Report Var(flag_live_range_shrinkage) Init(0) Optimization
>  Relief of register pressure through live range shrinkage.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 4ea93a7..c473049 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}.
>  -fgcse-sm  -fhoist-adjacent-loads  -fif-conversion @gol
>  -fif-conversion2  -findirect-inlining @gol
>  -finline-functions  -finline-functions-called-once  -finline-limit=@var{n} @gol
> --finline-small-functions  -fipa-cp  -fipa-cp-clone @gol
> +-finline-small-functions -fipa-cp  -fipa-cp-clone @gol

This changes is probably not intended.

>  -fipa-bit-cp -fipa-vrp @gol
>  -fipa-pta  -fipa-profile  -fipa-pure-const  -fipa-reference  -fipa-reference-addressable @gol
>  -fipa-stack-alignment  -fipa-icf  -fira-algorithm=@var{algorithm} @gol
> +-flive-patching=@var{level} @gol
>  -fira-region=@var{region}  -fira-hoist-pressure @gol
>  -fira-loop-pressure  -fno-ira-share-save-slots @gol
>  -fno-ira-share-spill-slots @gol
> @@ -8982,6 +8983,52 @@ equivalences that are found only by GCC and equivalences found only by Gold.
>  
>  This flag is enabled by default at @option{-O2} and @option{-Os}.
>  
> +@item -flive-patching=@var{level}
> +@opindex flive-patching
> +Control GCC's optimizations to provide a safe compilation for live-patching.
> +
> +If the compiler's optimization uses a function's body or information extracted 
> +from its body to optimize/change another function, the latter is called an
> +impacted function of the former.  If a function is patched, its impacted
> +functions should be patched too.
> +
> +The impacted functions are decided by the compiler's interprocedural
> +optimizations. For example, inlining a function into its caller, cloning 
> +a function and changing its caller to call this new clone, or extracting
> +a function's pureness/constness information to optimize its direct or 
> +indirect callers, etc.
> +
> +Usually, the more ipa optimizations enabled, the larger the number of
> +impacted functions for each function. In order to control the number of
> +impacted functions and computed the list of impacted function easily, 
> +we provide control to partially enable ipa optimizations on two different 
> +levels.
> +
> +The @var{level} argument should be one of the following:
> +
> +@table @samp
> +
> +@item inline-only-static
> +
> +Only enable inlining of static functions, disable all other interprocedural 
> +optimizations/analyses. As a result, when patching a static routine,
> +all its callers need to be patches as well.
> +
> +@item inline-clone 
> +
> +Only enable inlining and cloning optimizations, which includes inlining, 
> +cloning, interprocedural scalar replacement of aggregates and partial inlining.
> +Disable all other interprocedural optimizations/analyses.
> +As a result, when patching a routine, all its callers and its clones'
> +callers need to be patched as well.
> +
> +@end table
> +
> +When -flive-patching specified without any value, the default value
> +is "inline-clone".
> +
> +This flag is disabled by default.
> +
>  @item -fisolate-erroneous-paths-dereference
>  @opindex fisolate-erroneous-paths-dereference
>  Detect paths that trigger erroneous or undefined behavior due to
> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
> index 500f663..72e0f0f 100644
> --- a/gcc/flag-types.h
> +++ b/gcc/flag-types.h
> @@ -123,6 +123,14 @@ enum stack_reuse_level
>    SR_ALL
>  };
>  
> +/* The live patching level. */
> +enum live_patching_level
> +{
> +  LIVE_NONE = 0,
> +  LIVE_INLINE_ONLY_STATIC,
> +  LIVE_INLINE_CLONE

Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum
values are bit unclear.

> +};
> +
>  /* The algorithm used for basic block reordering.  */
>  enum reorder_blocks_algorithm
>  {
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index e04ede7..fadb437 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -377,6 +377,12 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
>        e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
>        inlinable = false;
>      }
> +  else if (callee->externally_visible 
> +	   && flag_live_patching == LIVE_INLINE_ONLY_STATIC) 
> +    {
> +      e->inline_failed = CIF_EXTERN_LIVE_ONLY_STATIC;
> +      inlinable = false;
> +    }
>    if (!inlinable && report)
>      report_inline_failed_reason (e);
>    return inlinable;
> diff --git a/gcc/opts.c b/gcc/opts.c
> index e21967b..281f9f4 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -691,6 +691,64 @@ default_options_optimization (struct gcc_options *opts,
>  			 lang_mask, handlers, loc, dc);
>  }
>  
> +/* Control ipa optimizations based on different live patching LEVEL */
> +static void
> +control_optimizations_for_live_patching (struct gcc_options *opts, 
> +					 struct gcc_options *opts_set, 
> +					 enum live_patching_level level)
> +{
> +  gcc_assert (level > LIVE_NONE);
> +
> +  switch (level)
> +    {
> +    case LIVE_INLINE_ONLY_STATIC:
> +      if (!opts_set->x_flag_ipa_cp_clone)
> +        opts->x_flag_ipa_cp_clone = 0;      
> +      if (!opts_set->x_flag_ipa_sra)
> +        opts->x_flag_ipa_sra = 0;
> +      if (!opts_set->x_flag_partial_inlining)
> +        opts->x_flag_partial_inlining = 0;
> +      if (!opts_set->x_flag_ipa_cp)
> +        opts->x_flag_ipa_cp = 0;      
> +      /* FALLTHROUGH */
> +    case LIVE_INLINE_CLONE:
> +      /* live patching should disable whole-program optimization.  */
> +      if (!opts_set->x_flag_whole_program)
> +        opts->x_flag_whole_program = 0;
> +      /* visibility change should be excluded by !flag_whole_program 
> +	 && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra 

You added sorry about LTO, maybe then !in_lto_p would be always true?

> +	 && !flag_partial_inlining.  */
> +      if (!opts_set->x_flag_ipa_pta)
> +        opts->x_flag_ipa_pta = 0;
> +      if (!opts_set->x_flag_ipa_reference)
> +        opts->x_flag_ipa_reference = 0;
> +      if (!opts_set->x_flag_ipa_ra)
> +        opts->x_flag_ipa_ra = 0;
> +      if (!opts_set->x_flag_ipa_icf)
> +        opts->x_flag_ipa_icf = 0;
> +      if (!opts_set->x_flag_ipa_icf_functions)
> +        opts->x_flag_ipa_icf_functions = 0;
> +      if (!opts_set->x_flag_ipa_icf_variables)
> +        opts->x_flag_ipa_icf_variables = 0;
> +      if (!opts_set->x_flag_ipa_bit_cp)
> +        opts->x_flag_ipa_bit_cp = 0;
> +      if (!opts_set->x_flag_ipa_vrp)
> +        opts->x_flag_ipa_vrp = 0;
> +      if (!opts_set->x_flag_ipa_pure_const)

Can you please explain why you included:
      if (!opts_set->x_flag_ipa_bit_cp)
        opts->x_flag_ipa_bit_cp = 0;
      if (!opts_set->x_flag_ipa_vrp)
        opts->x_flag_ipa_vrp = 0;

?

> +        opts->x_flag_ipa_pure_const = 0;
> +      /* unreachable code removal.  */
> +      /* discovery of functions/variables with no address taken.  */

^^^ these 2 comments looks misaligned.

> +      if (!opts_set->x_flag_ipa_reference_addressable)
> +        opts->x_flag_ipa_reference_addressable = 0;
> +      /* ipa stack alignment propagation.  */
> +      if (!opts_set->x_flag_ipa_stack_alignment)
> +        opts->x_flag_ipa_stack_alignment = 0;
> +      break;
> +    default: 
> +      gcc_assert (0);  
> +    }
> +}
> +
>  /* After all options at LOC have been read into OPTS and OPTS_SET,
>     finalize settings of those options and diagnose incompatible
>     combinations.  */
> @@ -1040,6 +1098,10 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>    if ((opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS) && opts->x_flag_tm)
>      sorry ("transactional memory is not supported with "
>  	   "%<-fsanitize=kernel-address%>");
> +
> +  /* Currently live patching is not support for LTO.  */
> +  if (opts->x_flag_live_patching && in_lto_p) 
> +    sorry ("live patching is not supported with LTO");
>  }
>  
>  #define LEFT_COLUMN	27
> @@ -2266,6 +2328,12 @@ common_handle_option (struct gcc_options *opts,
>  	(&opts->x_flag_instrument_functions_exclude_files, arg);
>        break;
>  
> +    case OPT_flive_patching_:
> +      if (value)
> +    	control_optimizations_for_live_patching (opts, opts_set,
> +						 opts->x_flag_live_patching);
> +      break;
> +
>      case OPT_fmessage_length_:
>        pp_set_line_maximum_length (dc->printer, value);
>        diagnostic_set_caret_max_width (dc, value);
> diff --git a/gcc/testsuite/gcc.dg/live-patching-1.c b/gcc/testsuite/gcc.dg/live-patching-1.c
> new file mode 100644
> index 0000000..6a1ea38
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline" } */
> +
> +extern int sum, n, m;
> +
> +int foo (int a)
> +{
> +  return a + n;
> +}
> +
> +static int bar (int b)
> +{
> +  return b * m;
> +}
> +
> +int main()
> +{
> +  sum = foo (m) + bar (n); 
> +  return 0;
> +}
> +
> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when the user requests only inlining static for live patching"  "inline" } } */
> -- 1.9.1

It would be also handy to test the newly added option.
Please add ChangeLog entry for the patch. Have you bootstrapped the patch and run test-suite?

Thanks,
Martin

> 
> 
> 
>> Thanks,
>> Miroslav
>