| Message ID | 20190808123916.8706-1-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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@amazon.de> > Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com> > Reviewed-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Norbert Manthey <nmanthey@amazon.de> > --- > 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,
> On 16. Aug 2019, at 11:57, Ross Lagerwall <ross.lagerwall@citrix.com> wrote: > > On 8/8/19 1:39 PM, Pawel Wieczorkiewicz wrote: >> …snip... >> #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. > */ > Oh yes, right. Let me update the comment. Thanks! > 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. ACK, will do. > > Thanks, > -- > Ross Lagerwall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
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__); @@ -1541,6 +1554,17 @@ 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) @@ -2072,6 +2096,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);