Re: [Xen-devel] [livepatch-build-tools part2 v2 6/6] create-diff-object: Do not include all .rodata sections

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 8/8/19 1:39 PM, Pawel Wieczorkiewicz wrote:
> Older versions of GCC did not split .rodata.str sections by function.
> Because of that, the entire section was always included.
> The livepatch-build-tools commit [1] fixed patch creation and kept
> including all .rodata.str sections, in order to maintain existing
> behavior for GCC 6.1+.
> This means all .rodata.str sections are always included by default,
> regardless of whether they are needed or not.
>
> During stacked hotpatch builds it leads to unnecessary accumulation of
> the .rodata.str sections as each and every consecutive hotpatch module
> contains all the .rodata.str sections of previous modules.
>
> To prevent this situation, mark the .rodata.str sections for inclusion
> only if they are referenced by any of the current hotpatch symbols (or
> a corresponding RELA section).
>
> Extend patchability verification to detect all non-standard, non-rela,
> non-debug and non-special sections that are not referenced by any of
> the symbols or RELA sections.
>
> [1] 2af6f1aa6233 Fix patch creation with GCC 6.1+
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@xxxxxxxxx>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
> Reviewed-by: Bjoern Doebel <doebel@xxxxxxxxx>
> Reviewed-by: Norbert Manthey <nmanthey@xxxxxxxxx>
> ---
> v2:
> * Made the commit message more precise and accurate (based on
>    Ross' comments to the v1 patch)
> * Kept lines limited to 80 chars
> ---
>   create-diff-object.c | 30 ++++++++++++++++++++++++++++--
>   1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 8365af0..4252175 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1350,8 +1350,7 @@ static void kpatch_include_standard_elements(struct 
> kpatch_elf *kelf)
>   
>   	list_for_each_entry(sec, &kelf->sections, list) {
>                 /* include these sections even if they haven't changed */
> -               if (is_standard_section(sec) ||
> -                   should_include_str_section(sec->name)) {
> +               if (is_standard_section(sec)) {
>                         sec->include = 1;
>                         if (sec->secsym)
>                                 sec->secsym->include = 1;
> @@ -1362,6 +1361,20 @@ static void kpatch_include_standard_elements(struct 
> kpatch_elf *kelf)
>         list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
>   }
>   
> +static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
> +{
> +       struct section *sec;
> +
> +       list_for_each_entry(sec, &kelf->sections, list) {
> +               if (should_include_str_section(sec->name) &&
> +                   is_referenced_section(sec, kelf)) {
> +                       sec->include = 1;
> +                       if (sec->secsym)
> +                               sec->secsym->include = 1;
> +               }
> +       }
> +}
> +
>   #define inc_printf(fmt, ...) \
>         log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
This patch looks good. There is a comment at the top of 
should_include_str_section() which should probably be updated as well:
/*
 * String sections are always included even if unchanged.
 * The format is either:
 * .rodata.<func>.str1.[0-9]+ (new in GCC 6.1.0)
 * or .rodata.str1.[0-9]+ (older versions of GCC)
 * For the new format we could be smarter and only include the needed
 * strings sections.
 */

In fact, it is probably a good idea to rename the function to something 
like "is_rodata_str_section()" since this more accurately describes what 
it does now.
Thanks,
--
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel