For data relocations to missing ENDBR, it will be useful in some cases
to print the data symbol + offset. Add support for that.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/include/objtool/warn.h | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index 802cfda0a6f6..dab0dda7c617 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -17,16 +17,19 @@ extern const char *objname;
static inline char *offstr(struct section *sec, unsigned long offset)
{
- struct symbol *func;
- char *name, *str;
+ bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+ struct symbol *sym = NULL;
unsigned long name_off;
+ char *name, *str;
+
+ if (is_text)
+ sym = find_func_containing(sec, offset);
+ if (!sym)
+ sym = find_symbol_containing(sec, offset);
- func = find_func_containing(sec, offset);
- if (!func)
- func = find_symbol_containing(sec, offset);
- if (func) {
- name = func->name;
- name_off = offset - func->offset;
+ if (sym) {
+ name = sym->name;
+ name_off = offset - sym->offset;
} else {
name = sec->name;
name_off = offset;
@@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
str = malloc(strlen(name) + 20);
- if (func)
- sprintf(str, "%s()+0x%lx", name, name_off);
+ if (sym)
+ sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
else
sprintf(str, "%s+0x%lx", name, name_off);
--
2.34.1
On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
>
> str = malloc(strlen(name) + 20);
>
> - if (func)
> - sprintf(str, "%s()+0x%lx", name, name_off);
> + if (sym)
> + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> else
> sprintf(str, "%s+0x%lx", name, name_off);
So I like the patch, except that "()" thing is something where we differ
from the kernel's %ps format and I've cursed it a number of times
because I then have to manually edit (iow remove) things when pasting it
in various scripts etc..
That said, it totally makes sense to differentiate between a text and
data symbol this way :/
On Thu, Apr 14, 2022 at 08:21:48AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> >
> > > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> > >
> > > str = malloc(strlen(name) + 20);
> > >
> > > - if (func)
> > > - sprintf(str, "%s()+0x%lx", name, name_off);
> > > + if (sym)
> > > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > > else
> > > sprintf(str, "%s+0x%lx", name, name_off);
> >
> > So I like the patch, except that "()" thing is something where we differ
> > from the kernel's %ps format and I've cursed it a number of times
> > because I then have to manually edit (iow remove) things when pasting it
> > in various scripts etc..
>
> Oh, hm, that's true. I can remove them if you prefer.
Yeah, I think taking it out is best, easier if we're consistent with %ps
for everybody.
> > That said, it totally makes sense to differentiate between a text and
> > data symbol this way :/
>
> Yes, but if we're keeping the "Add sec+offset to warnings" patch then
> that distinction is already (kind of) being made by showing the data
> section name. And the data symbol warnings should be rare.
Yes, I'd not seen that yet, what's that for? The Changelog alludes to
something, but I don't think it actually does get used later.
On Thu, Apr 14, 2022 at 05:31:57PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:21:48AM -0700, Josh Poimboeuf wrote:
> > On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> > >
> > > > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> > > >
> > > > str = malloc(strlen(name) + 20);
> > > >
> > > > - if (func)
> > > > - sprintf(str, "%s()+0x%lx", name, name_off);
> > > > + if (sym)
> > > > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > > > else
> > > > sprintf(str, "%s+0x%lx", name, name_off);
> > >
> > > So I like the patch, except that "()" thing is something where we differ
> > > from the kernel's %ps format and I've cursed it a number of times
> > > because I then have to manually edit (iow remove) things when pasting it
> > > in various scripts etc..
> >
> > Oh, hm, that's true. I can remove them if you prefer.
>
> Yeah, I think taking it out is best, easier if we're consistent with %ps
> for everybody.
Will do.
> > > That said, it totally makes sense to differentiate between a text and
> > > data symbol this way :/
> >
> > Yes, but if we're keeping the "Add sec+offset to warnings" patch then
> > that distinction is already (kind of) being made by showing the data
> > section name. And the data symbol warnings should be rare.
>
> Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> something, but I don't think it actually does get used later.
Nick had asked for something like that, it's just a way to avoid doing
math every time we look at a warning, i.e. to convert func+offset to
sec+offset.
But it's kind of ugly and I'm not 100% happy with it.
Maybe it should be behind an option (--sec-offsets)?
--
Josh
On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
>
> > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> >
> > str = malloc(strlen(name) + 20);
> >
> > - if (func)
> > - sprintf(str, "%s()+0x%lx", name, name_off);
> > + if (sym)
> > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > else
> > sprintf(str, "%s+0x%lx", name, name_off);
>
> So I like the patch, except that "()" thing is something where we differ
> from the kernel's %ps format and I've cursed it a number of times
> because I then have to manually edit (iow remove) things when pasting it
> in various scripts etc..
Oh, hm, that's true. I can remove them if you prefer.
> That said, it totally makes sense to differentiate between a text and
> data symbol this way :/
Yes, but if we're keeping the "Add sec+offset to warnings" patch then
that distinction is already (kind of) being made by showing the data
section name. And the data symbol warnings should be rare.
--
Josh
On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
> > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > something, but I don't think it actually does get used later.
>
> Nick had asked for something like that, it's just a way to avoid doing
> math every time we look at a warning, i.e. to convert func+offset to
> sec+offset.
>
> But it's kind of ugly and I'm not 100% happy with it.
>
> Maybe it should be behind an option (--sec-offsets)?
Can do I suppose... Myself, I have this script:
$ cat objdump-func.sh
#!/bin/bash
OBJ=$1; shift
FUNC=$1; shift
objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
That prints a symbol relative offset next to the section, something
like:
$ ./objdump-func.sh defconfig-build/vmlinux.o pick_next_task_idle
0000 00000000000a9eb0 <pick_next_task_idle>:
0000 a9eb0: 41 54 push %r12
0002 a9eb2: 4c 8b a7 28 09 00 00 mov 0x928(%rdi),%r12
0009 a9eb9: 53 push %rbx
000a a9eba: 48 89 fb mov %rdi,%rbx
000d a9ebd: 66 90 xchg %ax,%ax
000f a9ebf: 66 90 xchg %ax,%ax
0011 a9ec1: 4c 89 e0 mov %r12,%rax
0014 a9ec4: 5b pop %rbx
0015 a9ec5: 41 5c pop %r12
0017 a9ec7: c3 ret
0018 a9ec8: e8 00 00 00 00 call a9ecd <pick_next_task_idle+0x1d> a9ec9: R_X86_64_PLT32 __update_idle_core-0x4
001d a9ecd: eb f0 jmp a9ebf <pick_next_task_idle+0xf>
001f a9ecf: 4c 89 e0 mov %r12,%rax
0022 a9ed2: 83 83 b8 0b 00 00 01 addl $0x1,0xbb8(%rbx)
0029 a9ed9: 5b pop %rbx
002a a9eda: 41 5c pop %r12
002c a9edc: c3 ret
002d a9edd: 0f 1f 00 nopl (%rax)
On Thu, Apr 14, 2022 at 10:01:04AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:36:51PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
> >
> > > > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > > > something, but I don't think it actually does get used later.
> > >
> > > Nick had asked for something like that, it's just a way to avoid doing
> > > math every time we look at a warning, i.e. to convert func+offset to
> > > sec+offset.
> > >
> > > But it's kind of ugly and I'm not 100% happy with it.
> > >
> > > Maybe it should be behind an option (--sec-offsets)?
> >
> > Can do I suppose... Myself, I have this script:
> >
> > $ cat objdump-func.sh
> > #!/bin/bash
> >
> > OBJ=$1; shift
> > FUNC=$1; shift
> >
> > objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
>
> That is nice, just added to my ~/bin.
>
> And how am I just learning about objdump "-w" ?!?!
>
> I wrote up a new version of that patch which adds a '--sec-address'
> option (see below), but maybe I'll just drop it for now. It's not
> really relevant to this set anyway.
But now, testing the IBT code, I realize it would still be helpful for
data addresses. So maybe I'll keep it.
--
Josh
On Thu, Apr 14, 2022 at 06:36:51PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
>
> > > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > > something, but I don't think it actually does get used later.
> >
> > Nick had asked for something like that, it's just a way to avoid doing
> > math every time we look at a warning, i.e. to convert func+offset to
> > sec+offset.
> >
> > But it's kind of ugly and I'm not 100% happy with it.
> >
> > Maybe it should be behind an option (--sec-offsets)?
>
> Can do I suppose... Myself, I have this script:
>
> $ cat objdump-func.sh
> #!/bin/bash
>
> OBJ=$1; shift
> FUNC=$1; shift
>
> objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
That is nice, just added to my ~/bin.
And how am I just learning about objdump "-w" ?!?!
I wrote up a new version of that patch which adds a '--sec-address'
option (see below), but maybe I'll just drop it for now. It's not
really relevant to this set anyway.
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] objtool: Add option to print section addresses
To help prevent objtool users from having to do math, add an option to
print the section address in addition to the function address.
Normal:
vmlinux.o: warning: objtool: fixup_exception()+0x2d1: unreachable instruction
With '--sec-address':
vmlinux.o: warning: objtool: fixup_exception()+0x2d1 (.text+0x76c51): unreachable instruction
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/builtin-check.c | 1 +
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/warn.h | 31 ++++++++++++++-----------
3 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 3df46e9b4b03..2c562e3dec55 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -55,6 +55,7 @@ const struct option check_options[] = {
OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
OPT_BOOLEAN(0, "no-fp", &opts.no_fp, "skip frame pointer validation"),
OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
+ OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
OPT_BOOLEAN(0, "vmlinux", &opts.vmlinux, "vmlinux.o validation"),
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 0cac9bd6a97f..e6910a66317a 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -33,6 +33,7 @@ struct opts {
bool module;
bool no_fp;
bool no_unreachable;
+ bool sec_address;
bool stats;
bool vmlinux;
};
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index c4bde3e2a79c..a3e79ae75f2e 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -11,30 +11,33 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <objtool/builtin.h>
#include <objtool/elf.h>
extern const char *objname;
static inline char *offstr(struct section *sec, unsigned long offset)
{
- struct symbol *func;
- char *name, *str;
- unsigned long name_off;
+ bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+ struct symbol *sym = NULL;
+ char *str;
+ int len;
- func = find_func_containing(sec, offset);
- if (!func)
- func = find_symbol_containing(sec, offset);
- if (func) {
- name = func->name;
- name_off = offset - func->offset;
+ if (is_text)
+ sym = find_func_containing(sec, offset);
+ if (!sym)
+ sym = find_symbol_containing(sec, offset);
+
+ if (sym) {
+ str = malloc(strlen(sym->name) + strlen(sec->name) + 40);
+ len = sprintf(str, "%s+0x%lx", sym->name, offset - sym->offset);
+ if (opts.sec_address)
+ sprintf(str+len, " (%s+0x%lx)", sec->name, offset);
} else {
- name = sec->name;
- name_off = offset;
+ str = malloc(strlen(sec->name) + 20);
+ sprintf(str, "%s+0x%lx", sec->name, offset);
}
- str = malloc(strlen(name) + 20);
- sprintf(str, "%s+0x%lx", name, name_off);
-
return str;
}
--
2.34.1