| Message ID | 1481554551-2116-1-git-send-email-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 12, 2016 at 02:55:51PM +0000, Ross Lagerwall wrote: > GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which > means that .rodata.str1.[0-9]+ sections are now split by function. We > could probably be smarter about including just the sections we need, but > for now, simply include the string sections for all functions as is done > for previous versions of GCC. Are there plans to push this to the upstream kpatch repo? Or did they do it in some other way? > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > Reported-by: M A Young <m.a.young@durham.ac.uk> > --- > > Changed in v2: > * Clarified commit message. Thanks. Two little nitpicks - feel free to either ignore them or take them into account. Either way: Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > create-diff-object.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/create-diff-object.c b/create-diff-object.c > index 69bcd88..b0d1348 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -1184,6 +1184,43 @@ static void kpatch_process_special_sections(struct kpatch_elf *kelf) > } > } > > +/* Returns true if s is a string of only numbers with length > 0. */ > +static int isnumber(const char *s) Could you use bool? > +{ > + do { > + if (!*s || !isdigit(*s)) > + return 0; > + } while (*++s); > + > + return 1; > +} > + > +/* > + * 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. > + */ > +static int should_include_str_section(const char *name) Ditto here? bool? > +{ > + const char *s; > + > + if (strncmp(name, ".rodata.", 8)) > + return 0; > + > + /* Check if name matches ".rodata.str1.[0-9]+" */ > + if (!strncmp(name, ".rodata.str1.", 13)) > + return isnumber(name + 13); I would make an #define for the '13' > + > + /* Check if name matches ".rodata.<func>.str1.[0-9]+" */ > + s = strstr(name, ".str1."); > + if (!s) > + return 0; > + return isnumber(s + 6); > +} > + > static void kpatch_include_standard_elements(struct kpatch_elf *kelf) > { > struct section *sec; > @@ -1193,7 +1230,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf) > if (!strcmp(sec->name, ".shstrtab") || > !strcmp(sec->name, ".strtab") || > !strcmp(sec->name, ".symtab") || > - !strncmp(sec->name, ".rodata.str1.", 13)) { > + should_include_str_section(sec->name)) { > sec->include = 1; > if (sec->secsym) > sec->secsym->include = 1; > -- > 2.7.4 >
On 12/12/2016 03:01 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Dec 12, 2016 at 02:55:51PM +0000, Ross Lagerwall wrote: >> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >> means that .rodata.str1.[0-9]+ sections are now split by function. We >> could probably be smarter about including just the sections we need, but >> for now, simply include the string sections for all functions as is done >> for previous versions of GCC. > > Are there plans to push this to the upstream kpatch repo? Or did > they do it in some other way? There isn't an upstream change for this. I _think_ the reason is that differences in the way that the Linux kernel is built (e.g. without -fPIC), and the way that the kpatch module is created means that kpatch is not affected by this. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> Reported-by: M A Young <m.a.young@durham.ac.uk> >> --- >> >> Changed in v2: >> * Clarified commit message. > > Thanks. Two little nitpicks - feel free to either ignore them > or take them into account. > > Either way: > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Pushed with the suggested changes. Thanks.
diff --git a/create-diff-object.c b/create-diff-object.c index 69bcd88..b0d1348 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -1184,6 +1184,43 @@ static void kpatch_process_special_sections(struct kpatch_elf *kelf) } } +/* Returns true if s is a string of only numbers with length > 0. */ +static int isnumber(const char *s) +{ + do { + if (!*s || !isdigit(*s)) + return 0; + } while (*++s); + + return 1; +} + +/* + * 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. + */ +static int should_include_str_section(const char *name) +{ + const char *s; + + if (strncmp(name, ".rodata.", 8)) + return 0; + + /* Check if name matches ".rodata.str1.[0-9]+" */ + if (!strncmp(name, ".rodata.str1.", 13)) + return isnumber(name + 13); + + /* Check if name matches ".rodata.<func>.str1.[0-9]+" */ + s = strstr(name, ".str1."); + if (!s) + return 0; + return isnumber(s + 6); +} + static void kpatch_include_standard_elements(struct kpatch_elf *kelf) { struct section *sec; @@ -1193,7 +1230,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf) if (!strcmp(sec->name, ".shstrtab") || !strcmp(sec->name, ".strtab") || !strcmp(sec->name, ".symtab") || - !strncmp(sec->name, ".rodata.str1.", 13)) { + should_include_str_section(sec->name)) { sec->include = 1; if (sec->secsym) sec->secsym->include = 1;
GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which means that .rodata.str1.[0-9]+ sections are now split by function. We could probably be smarter about including just the sections we need, but for now, simply include the string sections for all functions as is done for previous versions of GCC. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Reported-by: M A Young <m.a.young@durham.ac.uk> --- Changed in v2: * Clarified commit message. create-diff-object.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)