Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Tue, Jun 10, 2025 at 08:39:10AM -0400, Joe Lawrence wrote:
> >> Should we check for other data section prefixes here, like:
> >>
> >>else {
> >>snprintf(sec_name, SEC_NAME_LEN, ".rodata.%s",
> >> sym->name);
> >>if (!strcmp(sym->sec->name, sec_name))
> >>found_data = true;
> >>}
> >
> > Indeed. And also .bss.*.
> >
> >> At the same time, while we're here, what about other .text.* section
> >> prefixes?
> >
> > AFAIK, .text.* is the only one.
> >
>
> What about .text.unlikely, .text.hot (not sure if these can come alone
> or are only optimization copies) ?
Hm, I think .text.unlikely.foo is at least theoretically possible
without .text.foo. Seems "unlikely" though.
IIRC, .text.hot is used for profile-guided optimization, probably not a
concern here.
There are actually several edge cases that would cause this validation
to fail. If a module only had init/exit then it wouldn't have any
.text.foo. Or if it didn't have global data then there'd be no
.[ro]data.foo.
This function could get pretty fiddly, and honestly I'm not sure this
validation buys us much anyway. I'm thinking about just removing it
altogether...
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On 6/9/25 7:21 PM, Josh Poimboeuf wrote:
> On Mon, Jun 09, 2025 at 02:32:19PM -0400, Joe Lawrence wrote:
>> On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
>>> +static int validate_ffunction_fdata_sections(struct elf *elf)
>>> +{
>>> + struct symbol *sym;
>>> + bool found_text = false, found_data = false;
>>> +
>>> + for_each_sym(elf, sym) {
>>> + char sec_name[SEC_NAME_LEN];
>>> +
>>> + if (!found_text && is_func_sym(sym)) {
>>> + snprintf(sec_name, SEC_NAME_LEN, ".text.%s", sym->name);
>>> + if (!strcmp(sym->sec->name, sec_name))
>>> + found_text = true;
>>> + }
>>> +
>>> + if (!found_data && is_object_sym(sym)) {
>>> + snprintf(sec_name, SEC_NAME_LEN, ".data.%s", sym->name);
>>> + if (!strcmp(sym->sec->name, sec_name))
>>> + found_data = true;
>>
>> Hi Josh,
>>
>> Should we check for other data section prefixes here, like:
>>
>> else {
>> snprintf(sec_name, SEC_NAME_LEN, ".rodata.%s",
>> sym->name);
>> if (!strcmp(sym->sec->name, sec_name))
>> found_data = true;
>> }
>
> Indeed. And also .bss.*.
>
>> At the same time, while we're here, what about other .text.* section
>> prefixes?
>
> AFAIK, .text.* is the only one.
>
What about .text.unlikely, .text.hot (not sure if these can come alone
or are only optimization copies) ?
--
Joe
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, Jun 09, 2025 at 02:32:19PM -0400, Joe Lawrence wrote:
> On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> > +static int validate_ffunction_fdata_sections(struct elf *elf)
> > +{
> > + struct symbol *sym;
> > + bool found_text = false, found_data = false;
> > +
> > + for_each_sym(elf, sym) {
> > + char sec_name[SEC_NAME_LEN];
> > +
> > + if (!found_text && is_func_sym(sym)) {
> > + snprintf(sec_name, SEC_NAME_LEN, ".text.%s", sym->name);
> > + if (!strcmp(sym->sec->name, sec_name))
> > + found_text = true;
> > + }
> > +
> > + if (!found_data && is_object_sym(sym)) {
> > + snprintf(sec_name, SEC_NAME_LEN, ".data.%s", sym->name);
> > + if (!strcmp(sym->sec->name, sec_name))
> > + found_data = true;
>
> Hi Josh,
>
> Should we check for other data section prefixes here, like:
>
> else {
> snprintf(sec_name, SEC_NAME_LEN, ".rodata.%s",
> sym->name);
> if (!strcmp(sym->sec->name, sec_name))
> found_data = true;
> }
Indeed. And also .bss.*.
> At the same time, while we're here, what about other .text.* section
> prefixes?
AFAIK, .text.* is the only one.
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> +static int validate_ffunction_fdata_sections(struct elf *elf)
> +{
> + struct symbol *sym;
> + bool found_text = false, found_data = false;
> +
> + for_each_sym(elf, sym) {
> + char sec_name[SEC_NAME_LEN];
> +
> + if (!found_text && is_func_sym(sym)) {
> + snprintf(sec_name, SEC_NAME_LEN, ".text.%s", sym->name);
> + if (!strcmp(sym->sec->name, sec_name))
> + found_text = true;
> + }
> +
> + if (!found_data && is_object_sym(sym)) {
> + snprintf(sec_name, SEC_NAME_LEN, ".data.%s", sym->name);
> + if (!strcmp(sym->sec->name, sec_name))
> + found_data = true;
Hi Josh,
Should we check for other data section prefixes here, like:
else {
snprintf(sec_name, SEC_NAME_LEN, ".rodata.%s",
sym->name);
if (!strcmp(sym->sec->name, sec_name))
found_data = true;
}
because on my system, I tried patching net/netfilter/nft_tunnel.c, but
if I look at its OBJECTS and their corresponding sections, I see:
24: 24 OBJECT LOCAL DEFAULT 71 __msg.92
[71] .rodata.__msg.92 PROGBITS
43: 003f22 OBJECT LOCAL DEFAULT 72
__UNIQUE_ID_alias_1010
42: 002f16 OBJECT LOCAL DEFAULT 72
__UNIQUE_ID_alias_1011
44: 005547 OBJECT LOCAL DEFAULT 72
__UNIQUE_ID_author_1009
41: 47 OBJECT LOCAL DEFAULT 72
__UNIQUE_ID_description_1012
45: 008412 OBJECT LOCAL DEFAULT 72
__UNIQUE_ID_license_1008
[72] .modinfo
47: 8 OBJECT LOCAL DEFAULT 73
__UNIQUE_ID_addressable_cleanup_module_1007
[73] .exit.data
49: 8 OBJECT LOCAL DEFAULT 75
__UNIQUE_ID_addressable_init_module_1006
[72] .modinfo
51: 56 OBJECT LOCAL DEFAULT 77 nft_tunnel_obj_ops
[77] .rodata.nft_tunnel_obj_ops
5: 64 OBJECT LOCAL DEFAULT 79 nft_tunnel_obj_type
4: 004080 OBJECT LOCAL DEFAULT 79 nft_tunnel_type
[79] .data..read_mostly
53: 160 OBJECT LOCAL DEFAULT 81 nft_tunnel_key_policy
[81] .rodata.nft_tunnel_key_policy
30: 64 OBJECT LOCAL DEFAULT 82
nft_tunnel_opts_policy
[82] .rodata.nft_tunnel_opts_policy
23: 64 OBJECT LOCAL DEFAULT 83
nft_tunnel_opts_geneve_policy
[83] .rodata.nft_tunnel_opts_geneve_policy
32: 80 OBJECT LOCAL DEFAULT 84
nft_tunnel_opts_erspan_policy
[84] .rodata.nft_tunnel_opts_erspan_policy
31: 32 OBJECT LOCAL DEFAULT 85
nft_tunnel_opts_vxlan_policy
[85] .rodata.nft_tunnel_opts_vxlan_policy
20: 64 OBJECT LOCAL DEFAULT 86 nft_tunnel_ip6_policy
[86] .rodata.nft_tunnel_ip6_policy
29: 48 OBJECT LOCAL DEFAULT 87 nft_tunnel_ip_policy
[87] .rodata.nft_tunnel_ip_policy
62: 136 OBJECT LOCAL DEFAULT 88 nft_tunnel_get_ops
[88] .rodata.nft_tunnel_get_ops
64: 64 OBJECT LOCAL DEFAULT 91 nft_tunnel_policy
[91] .rodata.nft_tunnel_policy
I believe there are others like this, drivers/firmware/iscsi_ibft.o for
one, so even though validate_ffunction_fdata_sections() only needs to
find one .text. and one .data., not all objects may be
able to provide that.
At the same time, while we're here, what about other .text.* section
prefixes?
--
Joe
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, May 26, 2025 at 08:57:16PM +0200, Peter Zijlstra wrote:
> > @@ -50,10 +51,12 @@ struct section {
> > bool _changed, text, rodata, noinstr, init, truncate;
> > struct reloc *relocs;
> > unsigned long nr_alloc_relocs;
> > + struct section *twin;
> > };
> >
> > struct symbol {
> > struct list_head list;
> > + struct list_head global_list;
> > struct rb_node node;
> > struct elf_hash_node hash;
> > struct elf_hash_node name_hash;
> > @@ -79,10 +82,13 @@ struct symbol {
> > u8 cold : 1;
> > u8 prefix: 1;
> > u8 debug_checksum: 1;
> > + u8 changed : 1;
> > + u8 included : 1;
> > struct list_head pv_target;
> > struct reloc *relocs;
> > struct section *group_sec;
> > struct checksum csum;
> > + struct symbol *twin, *clone;
> > };
> >
> > struct reloc {
> > @@ -100,6 +106,7 @@ struct elf {
> > const char *name, *tmp_name;
> > unsigned int num_files;
> > struct list_head sections;
> > + struct list_head symbols;
> > unsigned long num_relocs;
> >
> > int symbol_bits;
>
> ISTR us spending significant effort shrinking all this stuff. How does
> this affect vmlinux.o memory footprint etc?
IIRC, most of our shrinking efforts were related to instructions and
relocs, which use up the bulk of the memory. This set doesn't touch
those.
Before and after the set, with a Fedora config:
Maximum resident set size (kbytes): 2934116
Maximum resident set size (kbytes): 2953708
So about ~0.67% more memory.
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, May 26, 2025 at 08:50:40PM +0200, Peter Zijlstra wrote: > > Let me hand you a fresh bucket of curlies, you must've run out :-) Thanks for the freebies, imported curlies aren't so cheap these days! -- Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, May 26, 2025 at 08:47:00PM +0200, Peter Zijlstra wrote:
> On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> > +#define SEC_NAME_LEN 512
> > #define SYM_NAME_LEN 512
> >
>
> > +static int validate_ffunction_fdata_sections(struct elf *elf)
> > +{
> > + struct symbol *sym;
> > + bool found_text = false, found_data = false;
> > +
> > + for_each_sym(elf, sym) {
> > + char sec_name[SEC_NAME_LEN];
> > +
> > + if (!found_text && is_func_sym(sym)) {
> > + snprintf(sec_name, SEC_NAME_LEN, ".text.%s", sym->name);
>
> So given SYM_NAME_LEN is 512, this SEC_NAME_LEN should be at least 6
> more, no?
I suppose so. There's also the .rela.text.* and .klp.rela.sec_objname.*
prefixes. I'll just bump SEC_NAME_LEN to 1024.
I should also double check the snprintf() return codes.
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, May 26, 2025 at 08:27:19PM +0200, Peter Zijlstra wrote:
> On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> > +static void scan_for_insn(struct section *sec, unsigned long offset,
> > + unsigned long *insn_off, unsigned int *insn_len)
> > +{
> > + unsigned long o = 0;
> > + struct insn insn;
> > +
> > + while (1) {
> > +
> > + insn_decode(&insn, sec->data->d_buf + o, sec_size(sec) - o,
> > + INSN_MODE_64);
> > +
> > + if (o + insn.length > offset) {
> > + *insn_off = o;
> > + *insn_len = insn.length;
> > + return;
> > + }
> > +
> > + o += insn.length;
> > + }
> > +}
> > +
> > +u64 arch_adjusted_addend(struct reloc *reloc)
> > +{
> > + unsigned int type = reloc_type(reloc);
> > + s64 addend = reloc_addend(reloc);
> > + unsigned long insn_off;
> > + unsigned int insn_len;
> > +
> > + if (type == R_X86_64_PLT32)
> > + return addend + 4;
> > +
> > + if (type != R_X86_64_PC32 || !is_text_sec(reloc->sec->base))
> > + return addend;
> > +
> > + scan_for_insn(reloc->sec->base, reloc_offset(reloc),
> > + &insn_off, &insn_len);
> > +
> > + return addend + insn_off + insn_len - reloc_offset(reloc);
> > +}
>
> This looks like a rather expensive proposition; it will have to decode
> the section nr_reloc times.
>
> Does it not make more sense to fully decode the section like 'normal' ?
Yeah, I'm not crazy about it either, but it at least keeps the pain
nicely localized to x86, and avoids pulling in struct instruction,
struct objtool_file, etc.
Also this typically doesn't need to be all that fast as this is only
done for changed functions, and only for a subset of relocations (those
which might be references to non-bundled data in a text section).
To give a general idea, in one of my tests, for a patch with 22
functions, it only calls scan_for_insn() 41 times.
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Mon, May 26, 2025 at 08:22:59PM +0200, Peter Zijlstra wrote: > On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote: > > > Without '-ffunction-sections -fdata-sections', reliable object diffing > > would be infeasible due to toolchain limitations: > > > > - For intra-file+intra-section references, the compiler might > > occasionally generated hard-coded instruction offsets instead of > > relocations. > > > > - Section-symbol-based references can be ambiguous: > > > > - Overlapping or zero-length symbols create ambiguity as to which > > symbol is being referenced. > > > > - A reference to the end of a symbol (e.g., checking array bounds) > > can be misinterpreted as a reference to the next symbol, or vice > > versa. > > > > A potential future alternative to '-ffunction-sections -fdata-sections' > > would be to introduce a toolchain option that forces symbol-based > > (non-section) relocations. > > Urgh.. So the first issue we can fix with objtool, but the ambiguous > cases are indeed very hard to fix up in post. > > Did you already talk to toolchain people about this? For now, I want to stick with -ffunction-sections -fdata-sections, as that's what kpatch has done for 10+ years and it works well. That's the only option we have for current compilers anyway. The above mentioned possibility of diffing without -ffunction-sections -fdata-sections is theoretical, and needs more exploration. If it indeed works then we can try to get toolchain support for that. -- Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote: > +#define KLP_RELOC_SEC_PREFIX ".klp.rela." > +#define KLP_SYM_PREFIX ".klp.sym." This max symbol length test is getting more and more broken every day :-)
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> diff --git a/tools/objtool/include/objtool/elf.h
> b/tools/objtool/include/objtool/elf.h
> index 4cfd09e66cb5..f62ac8081f27 100644
> --- a/tools/objtool/include/objtool/elf.h
> +++ b/tools/objtool/include/objtool/elf.h
> @@ -17,6 +17,7 @@
> #include
> #include
>
> +#define SEC_NAME_LEN 512
> #define SYM_NAME_LEN 512
>
> #ifdef LIBELF_USE_DEPRECATED
> @@ -50,10 +51,12 @@ struct section {
> bool _changed, text, rodata, noinstr, init, truncate;
> struct reloc *relocs;
> unsigned long nr_alloc_relocs;
> + struct section *twin;
> };
>
> struct symbol {
> struct list_head list;
> + struct list_head global_list;
> struct rb_node node;
> struct elf_hash_node hash;
> struct elf_hash_node name_hash;
> @@ -79,10 +82,13 @@ struct symbol {
> u8 cold : 1;
> u8 prefix: 1;
> u8 debug_checksum: 1;
> + u8 changed : 1;
> + u8 included : 1;
> struct list_head pv_target;
> struct reloc *relocs;
> struct section *group_sec;
> struct checksum csum;
> + struct symbol *twin, *clone;
> };
>
> struct reloc {
> @@ -100,6 +106,7 @@ struct elf {
> const char *name, *tmp_name;
> unsigned int num_files;
> struct list_head sections;
> + struct list_head symbols;
> unsigned long num_relocs;
>
> int symbol_bits;
ISTR us spending significant effort shrinking all this stuff. How does
this affect vmlinux.o memory footprint etc?
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
Let me hand you a fresh bucket of curlies, you must've run out :-)
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> +static struct symbol *first_file_symbol(struct elf *elf)
> +{
> + struct symbol *sym;
> +
> + for_each_sym(elf, sym)
{
> + if (is_file_sym(sym))
> + return sym;
}
> +
> + return NULL;
> +}
> +
> +static struct symbol *next_file_symbol(struct elf *elf, struct symbol *sym)
> +{
> + for_each_sym_continue(elf, sym)
{
> + if (is_file_sym(sym))
> + return sym;
}
> +
> + return NULL;
> +}
> +static bool is_special_section(struct section *sec)
> +{
> + static const char * const specials[] = {
> + ".altinstructions",
> + ".smp_locks",
> + "__bug_table",
> + "__ex_table",
> + "__jump_table",
> + "__mcount_loc",
> +
> + /*
> + * Extract .static_call_sites here to inherit non-module
> + * preferential treatment. The later static call processing
> + * during klp module build will be skipped when it sees this
> + * section already exists.
> + */
> + ".static_call_sites",
> + };
> +
> + static const char * const non_special_discards[] = {
> + ".discard.addressable",
> + SYM_CHECKSUM_SEC,
> + };
> +
> + for (int i = 0; i < ARRAY_SIZE(specials); i++)
> + if (!strcmp(sec->name, specials[i]))
> + return true;
> +
> + /* Most .discard sections are special */
> + for (int i = 0; i < ARRAY_SIZE(non_special_discards); i++)
{
> + if (!strcmp(sec->name, non_special_discards[i]))
> + return false;
}
> +
> + return strstarts(sec->name, ".discard.");
> +}
> +
> +/*
> + * These sections are referenced by special sections but aren't considered
> + * special sections themselves.
> + */
> +static bool is_special_section_aux(struct section *sec)
> +{
> + static const char * const specials_aux[] = {
> + ".altinstr_replacement",
> + ".altinstr_aux",
> + };
> +
> + for (int i = 0; i < ARRAY_SIZE(specials_aux); i++)
{
> + if (!strcmp(sec->name, specials_aux[i]))
> + return true;
}
> +
> + return false;
> +}
> +
And possibly more..
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> +#define SEC_NAME_LEN 512
> #define SYM_NAME_LEN 512
>
> +static int validate_ffunction_fdata_sections(struct elf *elf)
> +{
> + struct symbol *sym;
> + bool found_text = false, found_data = false;
> +
> + for_each_sym(elf, sym) {
> + char sec_name[SEC_NAME_LEN];
> +
> + if (!found_text && is_func_sym(sym)) {
> + snprintf(sec_name, SEC_NAME_LEN, ".text.%s", sym->name);
So given SYM_NAME_LEN is 512, this SEC_NAME_LEN should be at least 6
more, no?
> + if (!strcmp(sym->sec->name, sec_name))
> + found_text = true;
> + }
> +
> + if (!found_data && is_object_sym(sym)) {
> + snprintf(sec_name, SEC_NAME_LEN, ".data.%s", sym->name);
> + if (!strcmp(sym->sec->name, sec_name))
> + found_data = true;
> + }
> +
> + if (found_text && found_data)
> + return 0;
> + }
> +
> + ERROR("changed object '%s' not built with -ffunction-sections and
> -fdata-sections", elf->name);
> + return -1;
> +}
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote:
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index cdf385e54c69..ae4f83fcbadf 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -95,6 +95,46 @@ s64 arch_insn_adjusted_addend(struct instruction *insn,
> struct reloc *reloc)
> return phys_to_virt(addend);
> }
>
> +static void scan_for_insn(struct section *sec, unsigned long offset,
> + unsigned long *insn_off, unsigned int *insn_len)
> +{
> + unsigned long o = 0;
> + struct insn insn;
> +
> + while (1) {
> +
> + insn_decode(&insn, sec->data->d_buf + o, sec_size(sec) - o,
> + INSN_MODE_64);
> +
> + if (o + insn.length > offset) {
> + *insn_off = o;
> + *insn_len = insn.length;
> + return;
> + }
> +
> + o += insn.length;
> + }
> +}
> +
> +u64 arch_adjusted_addend(struct reloc *reloc)
> +{
> + unsigned int type = reloc_type(reloc);
> + s64 addend = reloc_addend(reloc);
> + unsigned long insn_off;
> + unsigned int insn_len;
> +
> + if (type == R_X86_64_PLT32)
> + return addend + 4;
> +
> + if (type != R_X86_64_PC32 || !is_text_sec(reloc->sec->base))
> + return addend;
> +
> + scan_for_insn(reloc->sec->base, reloc_offset(reloc),
> + &insn_off, &insn_len);
> +
> + return addend + insn_off + insn_len - reloc_offset(reloc);
> +}
This looks like a rather expensive proposition; it will have to decode
the section nr_reloc times.
Does it not make more sense to fully decode the section like 'normal' ?
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Fri, May 09, 2025 at 01:17:16PM -0700, Josh Poimboeuf wrote: > Without '-ffunction-sections -fdata-sections', reliable object diffing > would be infeasible due to toolchain limitations: > > - For intra-file+intra-section references, the compiler might > occasionally generated hard-coded instruction offsets instead of > relocations. > > - Section-symbol-based references can be ambiguous: > > - Overlapping or zero-length symbols create ambiguity as to which > symbol is being referenced. > > - A reference to the end of a symbol (e.g., checking array bounds) > can be misinterpreted as a reference to the next symbol, or vice > versa. > > A potential future alternative to '-ffunction-sections -fdata-sections' > would be to introduce a toolchain option that forces symbol-based > (non-section) relocations. Urgh.. So the first issue we can fix with objtool, but the ambiguous cases are indeed very hard to fix up in post. Did you already talk to toolchain people about this?
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On 5/14/2025 4:45 AM, Josh Poimboeuf wrote:
On Tue, May 13, 2025 at 10:49:59PM +0800, laokz wrote:
On 5/10/2025 4:17 AM, Josh Poimboeuf wrote:
+
+#define sym_for_each_reloc(elf, sym, reloc)\
+ for (reloc = find_reloc_by_dest_range(elf, sym->sec, \
+ sym->offset, sym->len); \
+reloc && reloc_offset(reloc) < sym->offset + sym->len; \
+reloc = rsec_next_reloc(sym->sec->rsec, reloc))
This macro intents to walk through ALL relocations for the 'sym'. It seems
we have the assumption that, there is at most one single relocation for the
same offset and find_reloc_by_dest_range only needs to do 'less than' offset
comparison:
elf_hash_for_each_possible(reloc, reloc, hash,
sec_offset_hash(rsec, o)) {
if (reloc->sec != rsec)
continue;
if (reloc_offset(reloc) >= offset &&
reloc_offset(reloc) < offset + len) {
less than ==>if (!r || reloc_offset(reloc) < reloc_offset(r))
r = reloc;
Because if there were multiple relocations for the same offset, the returned
one would be the last one in section entry order(hash list has reverse order
against section order), then broken the intention.
Right. Is that a problem? I don't believe I've ever seen two
relocations for the same offset.
Thanks for the clarification. I asked this because I noticed the
patchset have done some code refactoring, so guess if we could make it
more general to other architectures which not support objtool yet. Such
as RISC-V, it is not unusual having multiple relocs for same offset,
like vmlinux.o might have:
000c 010a0017 R_RISCV_PCREL_HI20 .LANCHOR0 + 48
000c 0033 R_RISCV_RELAX 48
0044 06170023 R_RISCV_ADD32 0048 pe_head_start + 0
0044 000dd5b90027 R_RISCV_SUB32 0002 _start + 0
But it is a bit off-topic:/
Regards,
laokz
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On Tue, May 13, 2025 at 10:49:59PM +0800, laokz wrote:
> On 5/10/2025 4:17 AM, Josh Poimboeuf wrote:
> > +
> > +#define sym_for_each_reloc(elf, sym, reloc)
> > \
> > + for (reloc = find_reloc_by_dest_range(elf, sym->sec,\
> > + sym->offset, sym->len); \
> > +reloc && reloc_offset(reloc) < sym->offset + sym->len;\
> > +reloc = rsec_next_reloc(sym->sec->rsec, reloc))
>
> This macro intents to walk through ALL relocations for the 'sym'. It seems
> we have the assumption that, there is at most one single relocation for the
> same offset and find_reloc_by_dest_range only needs to do 'less than' offset
> comparison:
>
> elf_hash_for_each_possible(reloc, reloc, hash,
> sec_offset_hash(rsec, o)) {
> if (reloc->sec != rsec)
> continue;
> if (reloc_offset(reloc) >= offset &&
> reloc_offset(reloc) < offset + len) {
> less than ==> if (!r || reloc_offset(reloc) < reloc_offset(r))
> r = reloc;
>
> Because if there were multiple relocations for the same offset, the returned
> one would be the last one in section entry order(hash list has reverse order
> against section order), then broken the intention.
Right. Is that a problem? I don't believe I've ever seen two
relocations for the same offset.
--
Josh
Re: [PATCH v2 52/62] objtool/klp: Introduce klp diff subcommand for diffing object files
On 5/10/2025 4:17 AM, Josh Poimboeuf wrote:
+
+#define sym_for_each_reloc(elf, sym, reloc)\
+ for (reloc = find_reloc_by_dest_range(elf, sym->sec, \
+ sym->offset, sym->len); \
+reloc && reloc_offset(reloc) < sym->offset + sym->len; \
+reloc = rsec_next_reloc(sym->sec->rsec, reloc))
This macro intents to walk through ALL relocations for the 'sym'. It
seems we have the assumption that, there is at most one single
relocation for the same offset and find_reloc_by_dest_range only needs
to do 'less than' offset comparison:
elf_hash_for_each_possible(reloc, reloc, hash,
sec_offset_hash(rsec, o)) {
if (reloc->sec != rsec)
continue;
if (reloc_offset(reloc) >= offset &&
reloc_offset(reloc) < offset + len) {
less than ==>if (!r || reloc_offset(reloc) < reloc_offset(r))
r = reloc;
Because if there were multiple relocations for the same offset, the
returned one would be the last one in section entry order(hash list has
reverse order against section order), then broken the intention.
Right?
Thanks,
laokz
