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

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 29. Apr 2019, at 18:11, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> Starting with the "2af6f1aa6233 Fix patch creation with GCC 6.1+" commit
>> all .rodata sections are included by default (regardless of whether they
>> are needed or not).
>> During stacked hotpatch builds it leads to unnecessary duplication of
>> the .rodata sections as each and every consecutive hotpatch contains all
>> the .rodata section of previous hotpatches.
> 
> This commit message is a bit confusing.
> 
> 1) All of this only applies to .rodata.str* sections. Other .rodata sections 
> are handled separately.

Yes, that’s right. I will make the commit message precise.

ACK. Will fix.

> 
> 2) The above commit _did not_ introduce the problem. Previous versions of GCC 
> did not split .rodata.str sections by function which meant that the entire 
> section was always included. The commit fixed patch creation and _maintained_ 
> the existing behaviour for GCC 6.1+ by including all the

I see, thanks for the clarification. I will update the wording to avoid 
confusion and incorrect attribution here.

> .rodata.str* sections. This patch now includes only what is needed (but it 
> should be noted that this would likely not be useful on older versions of GCC 
> since they don't split .rodata.str properly).

OK, I suppose users of older versions of GCC have to accept the fact.

> 
>> To prevent this situation, mark the .rodata section 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.
>> 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>
>> ---
>>  create-diff-object.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>> diff --git a/create-diff-object.c b/create-diff-object.c
>> index f6060cd..f7eb421 100644
>> --- a/create-diff-object.c
>> +++ b/create-diff-object.c
>> @@ -1341,7 +1341,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;
>> @@ -1352,6 +1352,19 @@ 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;
>> +            }
>> +    }
>> +}
> 
> I think it would be simpler to just amend the previous function rather than 
> introducing a new one. E.g.
> 
> ... || (should_include_str_section(sec->name) && is_referenced_section(sec, 
> kelf))

IIRC, it is unfortunately too early to do that from within 
kpatch_include_standard_elements().
I want to call the kpatch_include_standard_string_elements() after the 
following functions are called:
- kpatch_include_changed_functions()
- kpatch_include_debug_sections()
- kpatch_include_hook_elements()
because they may affect is_referenced_section() result.

> 
> Regards,
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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