diff mbox series

objtool: Make 'sec-address' always on

Message ID e7e1de1d01194df3ff4053cb0815fc2ddba33213.1687360711.git.christophe.leroy@csgroup.eu
State New
Headers
Series objtool: Make 'sec-address' always on |

Commit Message

Christophe Leroy June 21, 2023, 3:20 p.m. UTC 
  Most of the time objtool warnings are useless without the
absolute address within the section.

Today there is --sec-address option to get it printed, but
that option is nowhere used and requires a change in Makefile
to use it.

Having the address inside the section at all time in addition
to the address within the object doesn't hurt and will help.

Remove the --sec-address option and print it at all time.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 tools/objtool/builtin-check.c           | 1 -
 tools/objtool/include/objtool/builtin.h | 1 -
 tools/objtool/include/objtool/warn.h    | 6 ++----
 3 files changed, 2 insertions(+), 6 deletions(-)

Comments

Josh Poimboeuf June 22, 2023, 6:31 a.m. UTC | #1 
On Wed, Jun 21, 2023 at 05:20:31PM +0200, Christophe Leroy wrote:
> Most of the time objtool warnings are useless without the
> absolute address within the section.
> 
> Today there is --sec-address option to get it printed, but
> that option is nowhere used and requires a change in Makefile
> to use it.
> 
> Having the address inside the section at all time in addition
> to the address within the object doesn't hurt and will help.
> 
> Remove the --sec-address option and print it at all time.

This option actually feels like clutter to me.  The func+offset format
works fine, combined with scripts like objdump-func and faddr2line.  And
we also have a new OBJTOOL_VERBOSE=1 option which auto-disassembles the
affected function.

On x86 we've been using func+offset for console stack traces for many
years, due to KASLR.  So maybe we're just more used to it.  But the
scripts make it fine.

Also it helps with identifying the same warning across different
configs/compilers.
kernel test robot June 24, 2023, 7:33 a.m. UTC | #2 
Hi Christophe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.4-rc7 next-20230623]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-Leroy/objtool-Make-sec-address-always-on/20230621-232305
base:   linus/master
patch link:    https://lore.kernel.org/r/e7e1de1d01194df3ff4053cb0815fc2ddba33213.1687360711.git.christophe.leroy%40csgroup.eu
patch subject: [PATCH] objtool: Make 'sec-address' always on
config: x86_64-randconfig-c002-20230622 (https://download.01.org/0day-ci/archive/20230624/202306241520.4jIgEhK4-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230624/202306241520.4jIgEhK4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306241520.4jIgEhK4-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> vmlinux.o: warning: objtool: ibt_selftest+0x14 (.text+0x92b54): sibling call from callable instruction with modified stack frame
   vmlinux.o: warning: objtool: .altinstr_replacement+0x19a4: redundant UACCESS disable
   vmlinux.o: warning: objtool: iovec_from_user.part.0+0xb1 (.text+0x1c47761): call to copy_iovec_from_user.part.0() with UACCESS enabled
   vmlinux.o: warning: objtool: ibt_selftest+0x1e (.text+0x92b5e): return with modified stack frame


objdump-func vmlinux.o ibt_selftest:
0000 0000000000092b40 <ibt_selftest>:
0000    92b40:	f3 0f 1e fa          	endbr64
0004    92b44:	e8 00 00 00 00       	call   92b49 <ibt_selftest+0x9>	92b45: R_X86_64_PLT32	__fentry__-0x4
0009    92b49:	55                   	push   %rbp
000a    92b4a:	48 89 e5             	mov    %rsp,%rbp
000d    92b4d:	48 8d 05 02 00 00 00 	lea    0x2(%rip),%rax        # 92b56 <ibt_selftest_ip>
0014    92b54:	ff e0                	jmp    *%rax
0000 0000000000092b56 <ibt_selftest_ip>:
0000    92b56:	90                   	nop
0001    92b57:	48 85 c0             	test   %rax,%rax
0004    92b5a:	5d                   	pop    %rbp
0005    92b5b:	0f 94 c0             	sete   %al
0008    92b5e:	e9 00 00 00 00       	jmp    92b63 <ibt_selftest_ip+0xd>	92b5f: R_X86_64_PLT32	__x86_return_thunk-0x4
000d    92b63:	66 2e 0f 1f 84 00 00 00 00 00 	cs nopw 0x0(%rax,%rax,1)
0017    92b6d:	0f 1f 00             	nopl   (%rax)
Christophe Leroy June 24, 2023, 8:30 a.m. UTC | #3 
Le 24/06/2023 à 09:33, kernel test robot a écrit :
> Hi Christophe,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.4-rc7 next-20230623]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-Leroy/objtool-Make-sec-address-always-on/20230621-232305
> base:   linus/master
> patch link:    https://lore.kernel.org/r/e7e1de1d01194df3ff4053cb0815fc2ddba33213.1687360711.git.christophe.leroy%40csgroup.eu
> patch subject: [PATCH] objtool: Make 'sec-address' always on
> config: x86_64-randconfig-c002-20230622 (https://download.01.org/0day-ci/archive/20230624/202306241520.4jIgEhK4-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20230624/202306241520.4jIgEhK4-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202306241520.4jIgEhK4-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>>> vmlinux.o: warning: objtool: ibt_selftest+0x14 (.text+0x92b54): sibling call from callable instruction with modified stack frame
>     vmlinux.o: warning: objtool: .altinstr_replacement+0x19a4: redundant UACCESS disable
>     vmlinux.o: warning: objtool: iovec_from_user.part.0+0xb1 (.text+0x1c47761): call to copy_iovec_from_user.part.0() with UACCESS enabled
>     vmlinux.o: warning: objtool: ibt_selftest+0x1e (.text+0x92b5e): return with modified stack frame
> 

I can't really see any link between that warning and the changes in the 
patch.

Whatever, this patch will be discarded as pointed out by Josh.

Christophe

> 
> objdump-func vmlinux.o ibt_selftest:
> 0000 0000000000092b40 <ibt_selftest>:
> 0000    92b40:	f3 0f 1e fa          	endbr64
> 0004    92b44:	e8 00 00 00 00       	call   92b49 <ibt_selftest+0x9>	92b45: R_X86_64_PLT32	__fentry__-0x4
> 0009    92b49:	55                   	push   %rbp
> 000a    92b4a:	48 89 e5             	mov    %rsp,%rbp
> 000d    92b4d:	48 8d 05 02 00 00 00 	lea    0x2(%rip),%rax        # 92b56 <ibt_selftest_ip>
> 0014    92b54:	ff e0                	jmp    *%rax
> 0000 0000000000092b56 <ibt_selftest_ip>:
> 0000    92b56:	90                   	nop
> 0001    92b57:	48 85 c0             	test   %rax,%rax
> 0004    92b5a:	5d                   	pop    %rbp
> 0005    92b5b:	0f 94 c0             	sete   %al
> 0008    92b5e:	e9 00 00 00 00       	jmp    92b63 <ibt_selftest_ip+0xd>	92b5f: R_X86_64_PLT32	__x86_return_thunk-0x4
> 000d    92b63:	66 2e 0f 1f 84 00 00 00 00 00 	cs nopw 0x0(%rax,%rax,1)
> 0017    92b6d:	0f 1f 00             	nopl   (%rax)
>
Christophe Leroy June 24, 2023, 8:32 a.m. UTC | #4 
Le 22/06/2023 à 08:31, Josh Poimboeuf a écrit :
> On Wed, Jun 21, 2023 at 05:20:31PM +0200, Christophe Leroy wrote:
>> Most of the time objtool warnings are useless without the
>> absolute address within the section.
>>
>> Today there is --sec-address option to get it printed, but
>> that option is nowhere used and requires a change in Makefile
>> to use it.
>>
>> Having the address inside the section at all time in addition
>> to the address within the object doesn't hurt and will help.
>>
>> Remove the --sec-address option and print it at all time.
> 
> This option actually feels like clutter to me.  The func+offset format
> works fine, combined with scripts like objdump-func and faddr2line.  And
> we also have a new OBJTOOL_VERBOSE=1 option which auto-disassembles the
> affected function.
> 
> On x86 we've been using func+offset for console stack traces for many
> years, due to KASLR.  So maybe we're just more used to it.  But the
> scripts make it fine.

Ah right, I didn't know that script, I was struggling with objtool -dr

Therefore the patch is not needed, and neither is the --sec-address 
option, maybe we can remove it completely.

> 
> Also it helps with identifying the same warning across different
> configs/compilers.
> 

Good point.

Christophe
Josh Poimboeuf June 26, 2023, 3:20 a.m. UTC | #5 
On Sat, Jun 24, 2023 at 08:30:48AM +0000, Christophe Leroy wrote:
> >>> vmlinux.o: warning: objtool: ibt_selftest+0x14 (.text+0x92b54): sibling call from callable instruction with modified stack frame
> >     vmlinux.o: warning: objtool: .altinstr_replacement+0x19a4: redundant UACCESS disable
> >     vmlinux.o: warning: objtool: iovec_from_user.part.0+0xb1 (.text+0x1c47761): call to copy_iovec_from_user.part.0() with UACCESS enabled
> >     vmlinux.o: warning: objtool: ibt_selftest+0x1e (.text+0x92b5e): return with modified stack frame
> > 
> 
> I can't really see any link between that warning and the changes in the 
> patch.

I suspect it's a pre-existing warning, but because the patch made a
change to the default formatting (adding .text+off), it looks like a new
warning to the bots.
diff mbox series

Patch

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 7c175198d09f..d5024a95467a 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -91,7 +91,6 @@  static const struct option check_options[] = {
 	OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
 	OPT_BOOLEAN(0, "mnop", &opts.mnop, "nop out mcount call sites"),
 	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_END(),
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 2a108e648b7a..af79618cf6ab 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -35,7 +35,6 @@  struct opts {
 	bool mnop;
 	bool module;
 	bool no_unreachable;
-	bool sec_address;
 	bool stats;
 };
 
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index b1c920dc9516..2db9717d0558 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -21,7 +21,6 @@  static inline char *offstr(struct section *sec, unsigned long offset)
 	bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
 	struct symbol *sym = NULL;
 	char *str;
-	int len;
 
 	if (is_text)
 		sym = find_func_containing(sec, offset);
@@ -30,9 +29,8 @@  static inline char *offstr(struct section *sec, unsigned long 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);
+		sprintf(str, "%s+0x%lx (%s+0x%lx)", sym->name,
+			offset - sym->offset, sec->name, offset);
 	} else {
 		str = malloc(strlen(sec->name) + 20);
 		sprintf(str, "%s+0x%lx", sec->name, offset);