| Message ID | 20190416120716.26269-6-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [livepatch-build-tools,part2,1/6] common: Add is_standard_section() helper function | expand |
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. 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 .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). > > 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@amazon.de> > Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Norbert Manthey <nmanthey@amazon.de> > --- > 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)) Regards,
> On 29. Apr 2019, at 18:11, Ross Lagerwall <ross.lagerwall@citrix.com> 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@amazon.de> >> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com> >> Reviewed-by: Bjoern Doebel <doebel@amazon.de> >> Reviewed-by: Norbert Manthey <nmanthey@amazon.de> >> --- >> 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
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; + } + } +} + #define inc_printf(fmt, ...) \ log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__); @@ -1531,6 +1544,16 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf) errs++; } + if (sec->include) { + if (!is_standard_section(sec) && !is_rela_section(sec) && + !is_debug_section(sec) && !is_special_section(sec)) { + if (!is_referenced_section(sec, kelf)) { + log_normal("section %s included, but not referenced\n", sec->name); + errs++; + } + } + } + /* * ensure we aren't including .data.* or .bss.* * (.data.unlikely is ok b/c it only has __warned vars) @@ -2062,6 +2085,8 @@ int main(int argc, char *argv[]) kpatch_include_debug_sections(kelf_patched); log_debug("Include hook elements\n"); kpatch_include_hook_elements(kelf_patched); + log_debug("Include standard string elements\n"); + kpatch_include_standard_string_elements(kelf_patched); log_debug("Include new globals\n"); new_globals_exist = kpatch_include_new_globals(kelf_patched); log_debug("new_globals_exist = %d\n", new_globals_exist);