[PATCH v2 35/62] objtool: Refactor add_jump_destinations()

Josh Poimboeuf posted 62 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v2 35/62] objtool: Refactor add_jump_destinations()
Posted by Josh Poimboeuf 7 months, 1 week ago
The add_jump_destinations() logic is a bit weird and convoluted after
being incrementally tweaked over the years.  Refactor it to hopefully be
more logical and straightforward.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 tools/objtool/check.c               | 227 +++++++++++++---------------
 tools/objtool/include/objtool/elf.h |   4 +-
 2 files changed, 104 insertions(+), 127 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 66cbeebd16ea..e4ca5edf73ad 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1439,9 +1439,14 @@ static void add_return_call(struct objtool_file *file, struct instruction *insn,
 }
 
 static bool is_first_func_insn(struct objtool_file *file,
-			       struct instruction *insn, struct symbol *sym)
+			       struct instruction *insn)
 {
-	if (insn->offset == sym->offset)
+	struct symbol *func = insn_func(insn);
+
+	if (!func)
+		return false;
+
+	if (insn->offset == func->offset)
 		return true;
 
 	/* Allow direct CALL/JMP past ENDBR */
@@ -1449,52 +1454,32 @@ static bool is_first_func_insn(struct objtool_file *file,
 		struct instruction *prev = prev_insn_same_sym(file, insn);
 
 		if (prev && prev->type == INSN_ENDBR &&
-		    insn->offset == sym->offset + prev->len)
+		    insn->offset == func->offset + prev->len)
 			return true;
 	}
 
 	return false;
 }
 
-/*
- * A sibling call is a tail-call to another symbol -- to differentiate from a
- * recursive tail-call which is to the same symbol.
- */
-static bool jump_is_sibling_call(struct objtool_file *file,
-				 struct instruction *from, struct instruction *to)
-{
-	struct symbol *fs = from->sym;
-	struct symbol *ts = to->sym;
-
-	/* Not a sibling call if from/to a symbol hole */
-	if (!fs || !ts)
-		return false;
-
-	/* Not a sibling call if not targeting the start of a symbol. */
-	if (!is_first_func_insn(file, to, ts))
-		return false;
-
-	/* Disallow sibling calls into STT_NOTYPE */
-	if (is_notype_sym(ts))
-		return false;
-
-	/* Must not be self to be a sibling */
-	return fs->pfunc != ts->pfunc;
-}
-
 /*
  * Find the destination instructions for all jumps.
  */
 static int add_jump_destinations(struct objtool_file *file)
 {
-	struct instruction *insn, *jump_dest;
+	struct instruction *insn;
 	struct reloc *reloc;
-	struct section *dest_sec;
-	unsigned long dest_off;
 	int ret;
 
 	for_each_insn(file, insn) {
 		struct symbol *func = insn_func(insn);
+		struct instruction *dest_insn;
+		struct section *dest_sec;
+		struct symbol *dest_sym;
+		unsigned long dest_off;
+		bool dest_undef = false;
+
+		if (!is_static_jump(insn))
+			continue;
 
 		if (insn->jump_dest) {
 			/*
@@ -1503,129 +1488,121 @@ static int add_jump_destinations(struct objtool_file *file)
 			 */
 			continue;
 		}
-		if (!is_static_jump(insn))
-			continue;
 
 		reloc = insn_reloc(file, insn);
 		if (!reloc) {
 			dest_sec = insn->sec;
 			dest_off = arch_jump_destination(insn);
-		} else if (is_sec_sym(reloc->sym)) {
+		} else if (is_undef_sym(reloc->sym)) {
+			dest_sym = reloc->sym;
+			dest_undef = true;
+		} else {
 			dest_sec = reloc->sym->sec;
-			dest_off = arch_insn_adjusted_addend(insn, reloc);
-		} else if (reloc->sym->retpoline_thunk) {
+			dest_off = reloc->sym->sym.st_value +
+				   arch_insn_adjusted_addend(insn, reloc);
+		}
+
+		if (!dest_undef) {
+			dest_insn = find_insn(file, dest_sec, dest_off);
+			if (!dest_insn) {
+				struct symbol *sym = find_symbol_by_offset(dest_sec, dest_off);
+
+				/*
+				 * retbleed_untrain_ret() jumps to
+				 * __x86_return_thunk(), but objtool can't find
+				 * the thunk's starting RET instruction,
+				 * because the RET is also in the middle of
+				 * another instruction.  Objtool only knows
+				 * about the outer instruction.
+				 */
+				if (sym && sym->embedded_insn) {
+					add_return_call(file, insn, false);
+					continue;
+				}
+
+				/*
+				 * GCOV/KCOV dead code can jump to the end of
+				 * the function/section.
+				 */
+				if (file->ignore_unreachables && func &&
+				    dest_sec == insn->sec &&
+				    dest_off == func->offset + func->len)
+					continue;
+
+				ERROR_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
+					  dest_sec->name, dest_off);
+				return -1;
+			}
+
+			dest_sym = dest_insn->sym;
+			if (!dest_sym)
+				goto set_jump_dest;
+		}
+
+		if (dest_sym->retpoline_thunk) {
 			ret = add_retpoline_call(file, insn);
 			if (ret)
 				return ret;
 			continue;
-		} else if (reloc->sym->return_thunk) {
+		}
+
+		if (dest_sym->return_thunk) {
 			add_return_call(file, insn, true);
 			continue;
-		} else if (func) {
-			/*
-			 * External sibling call or internal sibling call with
-			 * STT_FUNC reloc.
-			 */
-			ret = add_call_dest(file, insn, reloc->sym, true);
-			if (ret)
-				return ret;
-			continue;
-		} else if (reloc->sym->sec->idx) {
-			dest_sec = reloc->sym->sec;
-			dest_off = reloc->sym->sym.st_value +
-				   arch_dest_reloc_offset(reloc_addend(reloc));
-		} else {
-			/* non-func asm code jumping to another file */
-			continue;
 		}
 
-		jump_dest = find_insn(file, dest_sec, dest_off);
-		if (!jump_dest) {
-			struct symbol *sym = find_symbol_by_offset(dest_sec, dest_off);
-
-			/*
-			 * This is a special case for retbleed_untrain_ret().
-			 * It jumps to __x86_return_thunk(), but objtool
-			 * can't find the thunk's starting RET
-			 * instruction, because the RET is also in the
-			 * middle of another instruction.  Objtool only
-			 * knows about the outer instruction.
-			 */
-			if (sym && sym->embedded_insn) {
-				add_return_call(file, insn, false);
-				continue;
-			}
-
-			/*
-			 * GCOV/KCOV dead code can jump to the end of the
-			 * function/section.
-			 */
-			if (file->ignore_unreachables && func &&
-			    dest_sec == insn->sec &&
-			    dest_off == func->offset + func->len)
-				continue;
-
-			ERROR_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
-				   dest_sec->name, dest_off);
-			return -1;
-		}
-
-		/*
-		 * An intra-TU jump in retpoline.o might not have a relocation
-		 * for its jump dest, in which case the above
-		 * add_{retpoline,return}_call() didn't happen.
-		 */
-		if (jump_dest->sym && jump_dest->offset == jump_dest->sym->offset) {
-			if (jump_dest->sym->retpoline_thunk) {
-				ret = add_retpoline_call(file, insn);
+		if (dest_undef) {
+			/* External symbol */
+			if (func) {
+				/* External sibling call */
+				ret = add_call_dest(file, insn, dest_sym, true);
 				if (ret)
 					return ret;
 				continue;
 			}
-			if (jump_dest->sym->return_thunk) {
-				add_return_call(file, insn, true);
-				continue;
-			}
+
+			/* Non-func asm code jumping to external symbol */
+			continue;
 		}
 
+		if (!insn->sym || insn->sym == dest_insn->sym)
+			goto set_jump_dest;
+
 		/*
-		 * Cross-function jump.
+		 * Internal cross-function jump.
 		 */
 
-		if (func && insn_func(jump_dest) && !func->cold &&
-		    insn_func(jump_dest)->cold) {
-
-			/*
-			 * For GCC 8+, create parent/child links for any cold
-			 * subfunctions.  This is _mostly_ redundant with a
-			 * similar initialization in read_symbols().
-			 *
-			 * If a function has aliases, we want the *first* such
-			 * function in the symbol table to be the subfunction's
-			 * parent.  In that case we overwrite the
-			 * initialization done in read_symbols().
-			 *
-			 * However this code can't completely replace the
-			 * read_symbols() code because this doesn't detect the
-			 * case where the parent function's only reference to a
-			 * subfunction is through a jump table.
-			 */
-			func->cfunc = insn_func(jump_dest);
-			insn_func(jump_dest)->pfunc = func;
+		/*
+		 * For GCC 8+, create parent/child links for any cold
+		 * subfunctions.  This is _mostly_ redundant with a
+		 * similar initialization in read_symbols().
+		 *
+		 * If a function has aliases, we want the *first* such
+		 * function in the symbol table to be the subfunction's
+		 * parent.  In that case we overwrite the
+		 * initialization done in read_symbols().
+		 *
+		 * However this code can't completely replace the
+		 * read_symbols() code because this doesn't detect the
+		 * case where the parent function's only reference to a
+		 * subfunction is through a jump table.
+		 */
+		if (func && dest_sym->cold) {
+			func->cfunc = dest_sym;
+			dest_sym->pfunc = func;
+			goto set_jump_dest;
 		}
 
-		if (jump_is_sibling_call(file, insn, jump_dest)) {
-			/*
-			 * Internal sibling call without reloc or with
-			 * STT_SECTION reloc.
-			 */
-			ret = add_call_dest(file, insn, insn_func(jump_dest), true);
+		if (is_first_func_insn(file, dest_insn)) {
+			/* Internal sibling call */
+			ret = add_call_dest(file, insn, dest_sym, true);
 			if (ret)
 				return ret;
 			continue;
 		}
 
-		insn->jump_dest = jump_dest;
+set_jump_dest:
+		insn->jump_dest = dest_insn;
 	}
 
 	return 0;
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index 842faec1b9a9..74c7b84b5310 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -180,9 +180,9 @@ static inline unsigned int elf_text_rela_type(struct elf *elf)
 	return elf_addr_size(elf) == 4 ? R_TEXT32 : R_TEXT64;
 }
 
-static inline bool sym_has_sec(struct symbol *sym)
+static inline bool is_undef_sym(struct symbol *sym)
 {
-	return sym->sec->idx;
+	return !sym->sec->idx;
 }
 
 static inline bool is_null_sym(struct symbol *sym)
-- 
2.49.0
Re: [PATCH v2 35/62] objtool: Refactor add_jump_destinations()
Posted by Joe Lawrence 6 months, 3 weeks ago
On 5/9/25 4:16 PM, Josh Poimboeuf wrote:
> The add_jump_destinations() logic is a bit weird and convoluted after
> being incrementally tweaked over the years.  Refactor it to hopefully be
> more logical and straightforward.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  tools/objtool/check.c               | 227 +++++++++++++---------------
>  tools/objtool/include/objtool/elf.h |   4 +-
>  2 files changed, 104 insertions(+), 127 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 66cbeebd16ea..e4ca5edf73ad 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1439,9 +1439,14 @@ static void add_return_call(struct objtool_file *file, struct instruction *insn,
>  }
>  
>  static bool is_first_func_insn(struct objtool_file *file,
> -			       struct instruction *insn, struct symbol *sym)
> +			       struct instruction *insn)
>  {
> -	if (insn->offset == sym->offset)
> +	struct symbol *func = insn_func(insn);
> +
> +	if (!func)
> +		return false;
> +
> +	if (insn->offset == func->offset)
>  		return true;
>  
>  	/* Allow direct CALL/JMP past ENDBR */
> @@ -1449,52 +1454,32 @@ static bool is_first_func_insn(struct objtool_file *file,
>  		struct instruction *prev = prev_insn_same_sym(file, insn);
>  
>  		if (prev && prev->type == INSN_ENDBR &&
> -		    insn->offset == sym->offset + prev->len)
> +		    insn->offset == func->offset + prev->len)
>  			return true;
>  	}
>  
>  	return false;
>  }
>  
> -/*
> - * A sibling call is a tail-call to another symbol -- to differentiate from a
> - * recursive tail-call which is to the same symbol.
> - */
> -static bool jump_is_sibling_call(struct objtool_file *file,
> -				 struct instruction *from, struct instruction *to)
> -{
> -	struct symbol *fs = from->sym;
> -	struct symbol *ts = to->sym;
> -
> -	/* Not a sibling call if from/to a symbol hole */
> -	if (!fs || !ts)
> -		return false;
> -
> -	/* Not a sibling call if not targeting the start of a symbol. */
> -	if (!is_first_func_insn(file, to, ts))
> -		return false;
> -
> -	/* Disallow sibling calls into STT_NOTYPE */
> -	if (is_notype_sym(ts))
> -		return false;
> -
> -	/* Must not be self to be a sibling */
> -	return fs->pfunc != ts->pfunc;
> -}
> -
>  /*
>   * Find the destination instructions for all jumps.
>   */
>  static int add_jump_destinations(struct objtool_file *file)
>  {
> -	struct instruction *insn, *jump_dest;
> +	struct instruction *insn;
>  	struct reloc *reloc;
> -	struct section *dest_sec;
> -	unsigned long dest_off;
>  	int ret;
>  
>  	for_each_insn(file, insn) {
>  		struct symbol *func = insn_func(insn);
> +		struct instruction *dest_insn;
> +		struct section *dest_sec;
> +		struct symbol *dest_sym;
> +		unsigned long dest_off;
> +		bool dest_undef = false;
> +
> +		if (!is_static_jump(insn))
> +			continue;
>  
>  		if (insn->jump_dest) {
>  			/*
> @@ -1503,129 +1488,121 @@ static int add_jump_destinations(struct objtool_file *file)
>  			 */
>  			continue;
>  		}
> -		if (!is_static_jump(insn))
> -			continue;
>  
>  		reloc = insn_reloc(file, insn);
>  		if (!reloc) {
>  			dest_sec = insn->sec;
>  			dest_off = arch_jump_destination(insn);
> -		} else if (is_sec_sym(reloc->sym)) {
> +		} else if (is_undef_sym(reloc->sym)) {
> +			dest_sym = reloc->sym;
> +			dest_undef = true;
> +		} else {
>  			dest_sec = reloc->sym->sec;
> -			dest_off = arch_insn_adjusted_addend(insn, reloc);
> -		} else if (reloc->sym->retpoline_thunk) {
> +			dest_off = reloc->sym->sym.st_value +
> +				   arch_insn_adjusted_addend(insn, reloc);
> +		}
> +
> +		if (!dest_undef) {
> +			dest_insn = find_insn(file, dest_sec, dest_off);
> +			if (!dest_insn) {
> +				struct symbol *sym = find_symbol_by_offset(dest_sec, dest_off);
> +
> +				/*
> +				 * retbleed_untrain_ret() jumps to
> +				 * __x86_return_thunk(), but objtool can't find
> +				 * the thunk's starting RET instruction,
> +				 * because the RET is also in the middle of
> +				 * another instruction.  Objtool only knows
> +				 * about the outer instruction.
> +				 */
> +				if (sym && sym->embedded_insn) {
> +					add_return_call(file, insn, false);
> +					continue;
> +				}
> +
> +				/*
> +				 * GCOV/KCOV dead code can jump to the end of
> +				 * the function/section.
> +				 */
> +				if (file->ignore_unreachables && func &&
> +				    dest_sec == insn->sec &&
> +				    dest_off == func->offset + func->len)
> +					continue;
> +
> +				ERROR_INSN(insn, "can't find jump dest instruction at %s+0x%lx",
> +					  dest_sec->name, dest_off);
> +				return -1;
> +			}
> +
> +			dest_sym = dest_insn->sym;
> +			if (!dest_sym)
> +				goto set_jump_dest;
> +		}
> +
> +		if (dest_sym->retpoline_thunk) {
>  			ret = add_retpoline_call(file, insn);
>  			if (ret)
>  				return ret;
>  			continue;
> -		} else if (reloc->sym->return_thunk) {
> +		}
> +
> +		if (dest_sym->return_thunk) {
>  			add_return_call(file, insn, true);
>  			continue;
> -		} else if (func) {
> -			/*
> -			 * External sibling call or internal sibling call with
> -			 * STT_FUNC reloc.
> -			 */
> -			ret = add_call_dest(file, insn, reloc->sym, true);
> -			if (ret)
> -				return ret;
> -			continue;
> -		} else if (reloc->sym->sec->idx) {
> -			dest_sec = reloc->sym->sec;
> -			dest_off = reloc->sym->sym.st_value +
> -				   arch_dest_reloc_offset(reloc_addend(reloc));

Way back in ("[PATCH v2 18/62] objtool: Fix x86 addend calculation"),
arch_dest_reloc_offset() was replaced with arch_insn_adjusted_addend(),
so I think that patch missed this callsite and breaks bisectability.

-- 
Joe
Re: [PATCH v2 35/62] objtool: Refactor add_jump_destinations()
Posted by Josh Poimboeuf 6 months, 2 weeks ago
On Fri, May 23, 2025 at 07:46:19AM -0400, Joe Lawrence wrote:
> > -		} else if (reloc->sym->sec->idx) {
> > -			dest_sec = reloc->sym->sec;
> > -			dest_off = reloc->sym->sym.st_value +
> > -				   arch_dest_reloc_offset(reloc_addend(reloc));
> 
> Way back in ("[PATCH v2 18/62] objtool: Fix x86 addend calculation"),
> arch_dest_reloc_offset() was replaced with arch_insn_adjusted_addend(),
> so I think that patch missed this callsite and breaks bisectability.

Fixed, thanks.

-- 
Josh