[PATCH v2 28/62] objtool: Fix weak symbol hole detection for .cold functions

Josh Poimboeuf posted 62 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v2 28/62] objtool: Fix weak symbol hole detection for .cold functions
Posted by Josh Poimboeuf 7 months, 1 week ago
When ignore_unreachable_insn() looks for weak function holes which jump
to their .cold functions, it assumes the parent function comes before
the corresponding .cold function in the symbol table.  That's not
necessarily the case with -ffunction-sections.

Mark all the holes beforehand (including .cold functions) so the
ordering of the discovery doesn't matter.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/check.c                 | 84 ++++++++++++++-------------
 tools/objtool/include/objtool/check.h |  3 +-
 2 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 043c36b70f26..a2a025ec57e8 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2527,6 +2527,44 @@ static void mark_rodata(struct objtool_file *file)
 	file->rodata = found;
 }
 
+static void mark_holes(struct objtool_file *file)
+{
+	struct instruction *insn;
+	bool in_hole = false;
+
+	if (!opts.link)
+		return;
+
+	/*
+	 * Whole archive runs might encounter dead code from weak symbols.
+	 * This is where the linker will have dropped the weak symbol in
+	 * favour of a regular symbol, but leaves the code in place.
+	 */
+	for_each_insn(file, insn) {
+		if (insn->sym || !find_symbol_hole_containing(insn->sec, insn->offset)) {
+			in_hole = false;
+			continue;
+		}
+
+		/* Skip function padding and pfx code */
+		if (!in_hole && insn->type == INSN_NOP)
+			continue;
+
+		in_hole = true;
+		insn->hole = 1;
+
+		/*
+		 * If this hole jumps to a .cold function, mark it ignore.
+		 */
+		if (insn->jump_dest) {
+			struct symbol *dest_func = insn_func(insn->jump_dest);
+
+			if (dest_func && dest_func->cold)
+				dest_func->ignore = true;
+		}
+	}
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -2592,6 +2630,9 @@ static int decode_sections(struct objtool_file *file)
 	if (ret)
 		return ret;
 
+	/* Must be after add_jump_destinations() */
+	mark_holes(file);
+
 	/*
 	 * Must be after add_call_destinations() such that it can override
 	 * dead_end_function() marks.
@@ -4021,7 +4062,8 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
 	struct instruction *prev_insn;
 	int i;
 
-	if (insn->type == INSN_NOP || insn->type == INSN_TRAP || (func && func->ignore))
+	if (insn->type == INSN_NOP || insn->type == INSN_TRAP ||
+	    insn->hole || (func && func->ignore))
 		return true;
 
 	/*
@@ -4032,46 +4074,6 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
 	    !strcmp(insn->sec->name, ".altinstr_aux"))
 		return true;
 
-	/*
-	 * Whole archive runs might encounter dead code from weak symbols.
-	 * This is where the linker will have dropped the weak symbol in
-	 * favour of a regular symbol, but leaves the code in place.
-	 *
-	 * In this case we'll find a piece of code (whole function) that is not
-	 * covered by a !section symbol. Ignore them.
-	 */
-	if (opts.link && !func) {
-		int size = find_symbol_hole_containing(insn->sec, insn->offset);
-		unsigned long end = insn->offset + size;
-
-		if (!size) /* not a hole */
-			return false;
-
-		if (size < 0) /* hole until the end */
-			return true;
-
-		sec_for_each_insn_continue(file, insn) {
-			/*
-			 * If we reach a visited instruction at or before the
-			 * end of the hole, ignore the unreachable.
-			 */
-			if (insn->visited)
-				return true;
-
-			if (insn->offset >= end)
-				break;
-
-			/*
-			 * If this hole jumps to a .cold function, mark it ignore too.
-			 */
-			if (insn->jump_dest && insn_func(insn->jump_dest) &&
-			    insn_func(insn->jump_dest)->cold)
-				insn_func(insn->jump_dest)->ignore = true;
-		}
-
-		return false;
-	}
-
 	if (!func)
 		return false;
 
diff --git a/tools/objtool/include/objtool/check.h b/tools/objtool/include/objtool/check.h
index 00fb745e7233..0f4e7ac929ef 100644
--- a/tools/objtool/include/objtool/check.h
+++ b/tools/objtool/include/objtool/check.h
@@ -64,7 +64,8 @@ struct instruction {
 	    noendbr		: 1,
 	    unret		: 1,
 	    visited		: 4,
-	    no_reloc		: 1;
+	    no_reloc		: 1,
+	    hole		: 1;
 		/* 10 bit hole */
 
 	struct alt_group *alt_group;
-- 
2.49.0
Re: [PATCH v2 28/62] objtool: Fix weak symbol hole detection for .cold functions
Posted by Peter Zijlstra 6 months, 3 weeks ago
On Fri, May 09, 2025 at 01:16:52PM -0700, Josh Poimboeuf wrote:
> When ignore_unreachable_insn() looks for weak function holes which jump
> to their .cold functions, it assumes the parent function comes before
> the corresponding .cold function in the symbol table.  That's not
> necessarily the case with -ffunction-sections.
> 
> Mark all the holes beforehand (including .cold functions) so the
> ordering of the discovery doesn't matter.

One of the things I have a 'todo' entry on, is rewriting all sections
that reference any one of these instructions.

That is, things like fentry, alternatives, retpoline, static_call,
jump_label.  Everything that can cause runtime code patching.

Once we are sure none of those sections will contain references to this
dead code, we can go and wipe the actual code. Perhaps fill it with a
UD1 instruction with some identifying immediate.
Re: [PATCH v2 28/62] objtool: Fix weak symbol hole detection for .cold functions
Posted by Josh Poimboeuf 6 months, 2 weeks ago
On Mon, May 26, 2025 at 12:38:22PM +0200, Peter Zijlstra wrote:
> On Fri, May 09, 2025 at 01:16:52PM -0700, Josh Poimboeuf wrote:
> > When ignore_unreachable_insn() looks for weak function holes which jump
> > to their .cold functions, it assumes the parent function comes before
> > the corresponding .cold function in the symbol table.  That's not
> > necessarily the case with -ffunction-sections.
> > 
> > Mark all the holes beforehand (including .cold functions) so the
> > ordering of the discovery doesn't matter.
> 
> One of the things I have a 'todo' entry on, is rewriting all sections
> that reference any one of these instructions.
> 
> That is, things like fentry, alternatives, retpoline, static_call,
> jump_label.  Everything that can cause runtime code patching.
> 
> Once we are sure none of those sections will contain references to this
> dead code, we can go and wipe the actual code. Perhaps fill it with a
> UD1 instruction with some identifying immediate.

Yeah, that would be nice.

-- 
Josh