On 11/13/18 10:16 PM, Qing Zhao wrote:
> Hi,
>
>> On Nov 13, 2018, at 1:18 PM, Miroslav Benes <mbenes@xxxxxxx> 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@xxxxxxxxxx>
> 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
>
