| Message ID | cb5dab6038dfe5156f5d68424cf372f7eed1b934.1680912057.git.jpoimboe@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sprinkle more __noreturn | expand |
On Fri, Apr 07, 2023 at 05:09:55PM -0700, Josh Poimboeuf wrote: > Fixes the following warning: > > vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction > > Reported-by: kernel test robot <lkp@intel.com> > Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/ > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> Ah, I just realized that my series will conflict with this. https://lore.kernel.org/llvm/20230412-no_stackp-v1-1-46a69b507a4b@google.com/ Perhaps if my series gets positive feedback; I can rebase it on top of this and it can become part of your series? For this patch, Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Though I'm curious, it does look like it's necessary because of 01/11 in the series? Any idea how the 0day bot report happened before 1/11 existed? (Surely gcc isn't assuming a weak function is implicitly noreturn and make optimizations based on that (that's one hazard I'm worried about)?) It looks like perhaps the link to https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/ on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your kernel tree https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn , which had 1/11 in it, IIUC? Perhaps this link should go on 1/11 rather than 2/11? Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day reports. Are you able to flesh out more info how/what/when such objtool warnings are observed? Are the warnings ever results of patches earlier in the series?
On Wed, Apr 12, 2023 at 01:29:49PM -0700, Nick Desaulniers wrote: > On Fri, Apr 07, 2023 at 05:09:55PM -0700, Josh Poimboeuf wrote: > > Fixes the following warning: > > > > vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction > > > > Reported-by: kernel test robot <lkp@intel.com> > > Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/ > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > Ah, I just realized that my series will conflict with this. > https://lore.kernel.org/llvm/20230412-no_stackp-v1-1-46a69b507a4b@google.com/ > Perhaps if my series gets positive feedback; I can rebase it on top of > this and it can become part of your series? Sure, I can take these on top. > For this patch, > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks! > Though I'm curious, it does look like it's necessary because of 01/11 in > the series? Any idea how the 0day bot report happened before 1/11 > existed? > > > (Surely gcc isn't assuming a weak function is implicitly noreturn and > make optimizations based on that (that's one hazard I'm worried about)?) As far as I can tell, GCC has been doing the right thing here, and it's instead been objtool getting confused by weak noreturns. That gets fixed later in patch 9. > It looks like perhaps the link to > https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/ > on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your > kernel tree > https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn > , which had 1/11 in it, IIUC? Perhaps this link should go on 1/11 > rather than 2/11? Good catch, patch 1 does introduce the warning. I think I'll just squash patches 1 and 2 so as not to break bisection. > Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day > reports. Are you able to flesh out more info how/what/when such objtool > warnings are observed? Are the warnings ever results of patches earlier > in the series? Hopefully not, it's best to not introduce warnings even temporarily. I was doing a lot of build testing at the time with various branches, so it's possible. I'll see if I can figure out how I triggered those warnings and document that in the commit logs if possible.
On Wed, Apr 12, 2023 at 02:57:57PM -0700, Josh Poimboeuf wrote: > > It looks like perhaps the link to > > https://lore.kernel.org/all/202302161142.K3ziREaj-lkp@intel.com/ > > on 2/11 was 0day testing the arch-cpu-idle-dead-noreturn branch of your > > kernel tree > > https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=arch-cpu-idle-dead-noreturn > > , which had 1/11 in it, IIUC? Perhaps this link should go on 1/11 > > rather than 2/11? > > Good catch, patch 1 does introduce the warning. I think I'll just > squash patches 1 and 2 so as not to break bisection. > > > Looking back at 1/11, 3/11, 8/11 I noticed not all patches have links to 0day > > reports. Are you able to flesh out more info how/what/when such objtool > > warnings are observed? Are the warnings ever results of patches earlier > > in the series? > > Hopefully not, it's best to not introduce warnings even temporarily. I > was doing a lot of build testing at the time with various branches, so > it's possible. I'll see if I can figure out how I triggered those > warnings and document that in the commit logs if possible. On second thought I won't squash, keeping them separate is useful for both patches 1 & 2 and patches 5-7. The patch order goes up the call stack, i.e. fix callees before callers. The opposite order would trigger actual compiler warnings rather than measly objtool warnings :-) I agree some of the commit logs are indeed confusing and sometimes even wrong. I'll clarify the justifications, and remove references to build bot warnings if they apply to previous patches in the set.
diff --git a/include/linux/start_kernel.h b/include/linux/start_kernel.h index 864921e54c92..a9806a44a605 100644 --- a/include/linux/start_kernel.h +++ b/include/linux/start_kernel.h @@ -8,7 +8,7 @@ /* Define the prototype for start_kernel here, rather than cluttering up something else. */ -extern asmlinkage void __init start_kernel(void); +extern asmlinkage void __init __noreturn start_kernel(void); extern void __init __noreturn arch_call_rest_init(void); extern void __ref __noreturn rest_init(void); diff --git a/init/main.c b/init/main.c index 161ed956d738..65aab4e9bb49 100644 --- a/init/main.c +++ b/init/main.c @@ -937,7 +937,7 @@ static void __init print_unknown_bootoptions(void) memblock_free(unknown_options, len); } -asmlinkage __visible void __init __no_sanitize_address start_kernel(void) +asmlinkage __visible void __init __no_sanitize_address __noreturn start_kernel(void) { char *command_line; char *after_dashes; diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e7442e7ad676..a6f9a4aeb77b 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -222,6 +222,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func, "rewind_stack_and_make_dead", "sev_es_terminate", "snp_abort", + "start_kernel", "stop_this_cpu", "usercopy_abort", "xen_cpu_bringup_again",
Fixes the following warning: vmlinux.o: warning: objtool: x86_64_start_reservations+0x28: unreachable instruction Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/r/202302161142.K3ziREaj-lkp@intel.com/ Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- include/linux/start_kernel.h | 2 +- init/main.c | 2 +- tools/objtool/check.c | 1 + 3 files changed, 3 insertions(+), 2 deletions(-)