[PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Allan Xavier]

As of commit 13810435b9a7 ("objtool: Support GCC 8's cold subfunctions")
objtool can get stuck in an infinite loop when processing cold functions
compiled without -freorder-functions. Using v4.17.2 and "gcc version
8.1.1 20180502 (Red Hat 8.1.1-1) (GCC)" from Fedora, this can be
reproduced trivially with make EXTRA_CFLAGS='-fno-reorder-functions'.

When -freorder-functions is used (enabled by default for -O2 in GCC 8)
cold functions are emitted in the .text.unlikely section.

0000000000000230 g     F .text  000000000000002c nmi_panic
00000000000002df l     F .text.unlikely 000000000000000c nmi_panic.cold.7

Without -freorder-functions, cold functions are emitted in the .text
section within the range of their parent function.

0000000000000500 g     F .text  0000000000000034 nmi_panic
0000000000000528 l     F .text  000000000000000c nmi_panic.cold.7

When objtool processes the second case it associates all of the
instructions in the parent function range 0x500-0x534 with nmi_panic
since that symbol is processed first. No instructions are associated
with nmi_panic.cold.7.

Later on, objtool iterates through the instructions in nmi_panic using
next_insn_same_func. Once it reaches the end of nmi_panic at 0x534 it
jumps to 0x528 as that's the start of nmi_panic.cold.7. However, since
the instructions starting at 0x528 are still associated with nmi_panic
objtool will get stuck in a loop, continually jumping back to 0x528
after reaching 0x534.

This doesn't happen with -freorder-functions in the first example as the
symbols don't overlap. So when 0x25c in .text is reached objtool will
jump to 0x2df in .text.unlikely where the instructions are correctly
associated with nmi_panic.cold.7.

Fix this issue by modifying decode_instructions to check whether an
instruction is associated with the parent function of the cold function
currently being processed. If so, reference the cold function rather
than the parent.

Signed-off-by: Allan Xavier <allan.x.xavier@oracle.com>
---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3a31b238f8856..2a5eea534ab3e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -314,7 +314,7 @@ static int decode_instructions(struct objtool_file *file)
 			}
 
 			func_for_each_insn(file, func, insn)
-				if (!insn->func)
+				if (!insn->func || insn->func == func->pfunc)
 					insn->func = func;
 		}
 	}
-- 
2.14.1

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Peter Zijlstra]

On Tue, Jun 26, 2018 at 05:20:45PM +0100, Allan Xavier wrote:
> 0000000000000500 g     F .text  0000000000000034 nmi_panic
> 0000000000000528 l     F .text  000000000000000c nmi_panic.cold.7
> 
> This doesn't happen with -freorder-functions in the first example as the
> symbols don't overlap. 

Urgh and I don't suppose we can 'fix' the overlap in read_symbols() ?

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Allan Xavier]

Hi Peter,

On 26/06/18 17:43, Peter Zijlstra wrote:
> On Tue, Jun 26, 2018 at 05:20:45PM +0100, Allan Xavier wrote:
>> 0000000000000500 g     F .text  0000000000000034 nmi_panic
>> 0000000000000528 l     F .text  000000000000000c nmi_panic.cold.7
>>
>> This doesn't happen with -freorder-functions in the first example as the
>> symbols don't overlap. 
> 
> Urgh and I don't suppose we can 'fix' the overlap in read_symbols() ?
> 

It should be fixable in read_symbols too if you don't mind sym->len not being
the same as sym->st_size in the ELF.

Is there a particular concern you're trying to address by having the logic there
instead?

Will have a look into reworking the patch in any case.

Thanks,
Allan

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Josh Poimboeuf]

On Tue, Jun 26, 2018 at 07:31:18PM +0100, Allan Xavier wrote:
> Hi Peter,
> 
> On 26/06/18 17:43, Peter Zijlstra wrote:
> > On Tue, Jun 26, 2018 at 05:20:45PM +0100, Allan Xavier wrote:
> >> 0000000000000500 g     F .text  0000000000000034 nmi_panic
> >> 0000000000000528 l     F .text  000000000000000c nmi_panic.cold.7
> >>
> >> This doesn't happen with -freorder-functions in the first example as the
> >> symbols don't overlap. 
> > 
> > Urgh and I don't suppose we can 'fix' the overlap in read_symbols() ?
> > 
> 
> It should be fixable in read_symbols too if you don't mind sym->len not being
> the same as sym->st_size in the ELF.
> 
> Is there a particular concern you're trying to address by having the logic there
> instead?
> 
> Will have a look into reworking the patch in any case.

Thanks for the patch and the excellent problem description.  I think the
concern is that the overlap might cause other unforeseen issues (now or
in the future).  How about something like this?  (The diff also includes
a cleanup to reduce the indent level.)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e60e105583e..36518393a960 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -302,19 +302,32 @@ static int read_symbols(struct elf *elf)
 				continue;
 			sym->pfunc = sym->cfunc = sym;
 			coldstr = strstr(sym->name, ".cold.");
-			if (coldstr) {
-				coldstr[0] = '\0';
-				pfunc = find_symbol_by_name(elf, sym->name);
-				coldstr[0] = '.';
-
-				if (!pfunc) {
-					WARN("%s(): can't find parent function",
-					     sym->name);
-					goto err;
-				}
-
-				sym->pfunc = pfunc;
-				pfunc->cfunc = sym;
+			if (!coldstr)
+				continue;
+
+			coldstr[0] = '\0';
+			pfunc = find_symbol_by_name(elf, sym->name);
+			coldstr[0] = '.';
+
+			if (!pfunc) {
+				WARN("%s(): can't find parent function",
+				     sym->name);
+				goto err;
+			}
+
+			sym->pfunc = pfunc;
+			pfunc->cfunc = sym;
+
+			/*
+			 * Unfortunately, -fnoreorder-functions puts the child
+			 * inside the parent.  Remove the overlap so we can
+			 * have sane assumptions.
+			 */
+			if (sym->sec == pfunc->sec &&
+			    sym->offset >= pfunc->offset &&
+			    sym->offset < pfunc->offset + pfunc->len &&
+			    sym->offset + sym->len == pfunc->offset + pfunc->len) {
+				pfunc->len -= sym->len;
 			}
 		}
 	}

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Peter Zijlstra]

On Tue, Jun 26, 2018 at 01:44:05PM -0500, Josh Poimboeuf wrote:
> On Tue, Jun 26, 2018 at 07:31:18PM +0100, Allan Xavier wrote:

> > Is there a particular concern you're trying to address by having the logic there
> > instead?
> 
> Thanks for the patch and the excellent problem description.  I think the
> concern is that the overlap might cause other unforeseen issues (now or
> in the future).

Yeah, overlapping symbols are just really weird and unfortunate, I've no
idea what the GCC people were thinking.

Note how find_symbol_containing() already more or less assumed !overlap.

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Josh Poimboeuf]

On Tue, Jun 26, 2018 at 09:43:27PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 26, 2018 at 01:44:05PM -0500, Josh Poimboeuf wrote:
> > On Tue, Jun 26, 2018 at 07:31:18PM +0100, Allan Xavier wrote:
> 
> > > Is there a particular concern you're trying to address by having the logic there
> > > instead?
> > 
> > Thanks for the patch and the excellent problem description.  I think the
> > concern is that the overlap might cause other unforeseen issues (now or
> > in the future).
> 
> Yeah, overlapping symbols are just really weird and unfortunate, I've no
> idea what the GCC people were thinking.
> 
> Note how find_symbol_containing() already more or less assumed !overlap.

If there are no objections to my patch, I'll run it through 0-day
testing and post an official version tomorrow-ish if it doesn't break
anything.

-- 
Josh

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Allan Xavier]

Looks good overall, just one comment.

On 26/06/18 19:44, Josh Poimboeuf wrote:
> +			/*
> +			 * Unfortunately, -fnoreorder-functions puts the child
> +			 * inside the parent.  Remove the overlap so we can
> +			 * have sane assumptions.
> +			 */
> +			if (sym->sec == pfunc->sec &&
> +			    sym->offset >= pfunc->offset &&
> +			    sym->offset < pfunc->offset + pfunc->len &&
> +			    sym->offset + sym->len == pfunc->offset + pfunc->len) {
> +				pfunc->len -= sym->len;

It's a bit of a nit but I'd say you could drop the third condition of the if
since sym->offset would have to be less than pfunc->offset + pfunc->len for the
fourth condition to ever be true. The only situation it would have caught is
where sym->len == 0, which I think (hope?) is reasonable to assume wont happen
and wouldn't have had an effect on pfunc->len anyway.

if (sym->sec == pfunc->sec &&
    sym->offset >= pfunc->offset &&
    sym->offset + sym->len == pfunc->offset + pfunc->len)

Re: [PATCH] objtool: Fix GCC 8 cold function processing without -freorder-functions

[Josh Poimboeuf]

On Wed, Jun 27, 2018 at 10:35:03AM +0100, Allan Xavier wrote:
> Looks good overall, just one comment.
> 
> On 26/06/18 19:44, Josh Poimboeuf wrote:
> > +			/*
> > +			 * Unfortunately, -fnoreorder-functions puts the child
> > +			 * inside the parent.  Remove the overlap so we can
> > +			 * have sane assumptions.
> > +			 */
> > +			if (sym->sec == pfunc->sec &&
> > +			    sym->offset >= pfunc->offset &&
> > +			    sym->offset < pfunc->offset + pfunc->len &&
> > +			    sym->offset + sym->len == pfunc->offset + pfunc->len) {
> > +				pfunc->len -= sym->len;
> 
> It's a bit of a nit but I'd say you could drop the third condition of the if
> since sym->offset would have to be less than pfunc->offset + pfunc->len for the
> fourth condition to ever be true. The only situation it would have caught is
> where sym->len == 0, which I think (hope?) is reasonable to assume wont happen
> and wouldn't have had an effect on pfunc->len anyway.
> 
> if (sym->sec == pfunc->sec &&
>     sym->offset >= pfunc->offset &&
>     sym->offset + sym->len == pfunc->offset + pfunc->len)

Thanks, will do.

-- 
Josh