* [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML @ 2021-01-19 11:18 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-19 11:18 UTC (permalink / raw) To: linux-um Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Peter Oberparleiter, Johannes Berg From: Johannes Berg <johannes.berg@intel.com> On ARCH=um, loading a module doesn't result in its constructors getting called, which breaks module gcov since the debugfs files are never registered. On the other hand, in-kernel constructors have already been called by the dynamic linker, so we can't call them again. Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both of which are enabled by default if CONFIG_CONSTRUCTORS is turned on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's not used on ARCH=um. Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, since we really do want CONSTRUCTORS, just not kernel binary ones. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- Tested with a kernel configured with CONFIG_GCOV_KERNEL, without the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart from the reset file), and with it we get the files and they work. I have no idea which tree this might go through, any suggestions? --- include/asm-generic/vmlinux.lds.h | 6 +++--- include/linux/module.h | 2 +- init/Kconfig | 9 ++++++++- init/main.c | 2 +- kernel/gcov/Kconfig | 2 +- kernel/module.c | 4 ++-- 6 files changed, 16 insertions(+), 9 deletions(-) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index b2b3d81b1535..87b300471c54 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -698,7 +698,7 @@ INIT_TASK_DATA(align) \ } -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_KERNEL #define KERNEL_CTORS() . = ALIGN(8); \ __ctors_start = .; \ KEEP(*(SORT(.ctors.*))) \ @@ -990,11 +990,11 @@ /* * Clang's -fsanitize=kernel-address and -fsanitize=thread produce * unwanted sections (.eh_frame and .init_array.*), but - * CONFIG_CONSTRUCTORS wants to keep any .init_array.* sections. + * CONFIG_CONSTRUCTORS_KERNEL wants to keep any .init_array.* sections. * https://bugs.llvm.org/show_bug.cgi?id=46478 */ #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) -# ifdef CONFIG_CONSTRUCTORS +# ifdef CONFIG_CONSTRUCTORS_KERNEL # define SANITIZER_DISCARDS \ *(.eh_frame) # else diff --git a/include/linux/module.h b/include/linux/module.h index 7a0bcb5b1ffc..027cfdbd84bd 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -528,7 +528,7 @@ struct module { atomic_t refcnt; #endif -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE /* Constructor functions. */ ctor_fn_t *ctors; unsigned int num_ctors; diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..e409de8d6c17 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -76,7 +76,14 @@ config CC_HAS_ASM_INLINE config CONSTRUCTORS bool - depends on !UML + +config CONSTRUCTORS_KERNEL + def_bool y + depends on CONSTRUCTORS && !UML + +config CONSTRUCTORS_MODULE + def_bool y + depends on CONSTRUCTORS config IRQ_WORK bool diff --git a/init/main.c b/init/main.c index c68d784376ca..51eb4802511c 100644 --- a/init/main.c +++ b/init/main.c @@ -1066,7 +1066,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) /* Call all constructor functions linked into the kernel. */ static void __init do_ctors(void) { -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_KERNEL ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; for (; fn < (ctor_fn_t *) __ctors_end; fn++) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index 3110c77230c7..f62de2dea8a3 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" config GCOV_KERNEL bool "Enable gcov-based kernel profiling" depends on DEBUG_FS - select CONSTRUCTORS if !UML + select CONSTRUCTORS default n help This option enables gcov-based code profiling (e.g. for code coverage diff --git a/kernel/module.c b/kernel/module.c index 4bf30e4b3eaa..c161a360d929 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3257,7 +3257,7 @@ static int find_module_sections(struct module *mod, struct load_info *info) &mod->num_unused_gpl_syms); mod->unused_gpl_crcs = section_addr(info, "__kcrctab_unused_gpl"); #endif -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE mod->ctors = section_objs(info, ".ctors", sizeof(*mod->ctors), &mod->num_ctors); if (!mod->ctors) @@ -3612,7 +3612,7 @@ static bool finished_loading(const char *name) /* Call module constructors. */ static void do_mod_ctors(struct module *mod) { -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE unsigned long i; for (i = 0; i < mod->num_ctors; i++) -- 2.26.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML @ 2021-01-19 11:18 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-19 11:18 UTC (permalink / raw) To: linux-um Cc: linux-arch, Arnd Bergmann, Johannes Berg, Peter Oberparleiter, linux-kernel, Jessica Yu From: Johannes Berg <johannes.berg@intel.com> On ARCH=um, loading a module doesn't result in its constructors getting called, which breaks module gcov since the debugfs files are never registered. On the other hand, in-kernel constructors have already been called by the dynamic linker, so we can't call them again. Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both of which are enabled by default if CONFIG_CONSTRUCTORS is turned on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's not used on ARCH=um. Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, since we really do want CONSTRUCTORS, just not kernel binary ones. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- Tested with a kernel configured with CONFIG_GCOV_KERNEL, without the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart from the reset file), and with it we get the files and they work. I have no idea which tree this might go through, any suggestions? --- include/asm-generic/vmlinux.lds.h | 6 +++--- include/linux/module.h | 2 +- init/Kconfig | 9 ++++++++- init/main.c | 2 +- kernel/gcov/Kconfig | 2 +- kernel/module.c | 4 ++-- 6 files changed, 16 insertions(+), 9 deletions(-) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index b2b3d81b1535..87b300471c54 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -698,7 +698,7 @@ INIT_TASK_DATA(align) \ } -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_KERNEL #define KERNEL_CTORS() . = ALIGN(8); \ __ctors_start = .; \ KEEP(*(SORT(.ctors.*))) \ @@ -990,11 +990,11 @@ /* * Clang's -fsanitize=kernel-address and -fsanitize=thread produce * unwanted sections (.eh_frame and .init_array.*), but - * CONFIG_CONSTRUCTORS wants to keep any .init_array.* sections. + * CONFIG_CONSTRUCTORS_KERNEL wants to keep any .init_array.* sections. * https://bugs.llvm.org/show_bug.cgi?id=46478 */ #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) -# ifdef CONFIG_CONSTRUCTORS +# ifdef CONFIG_CONSTRUCTORS_KERNEL # define SANITIZER_DISCARDS \ *(.eh_frame) # else diff --git a/include/linux/module.h b/include/linux/module.h index 7a0bcb5b1ffc..027cfdbd84bd 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -528,7 +528,7 @@ struct module { atomic_t refcnt; #endif -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE /* Constructor functions. */ ctor_fn_t *ctors; unsigned int num_ctors; diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..e409de8d6c17 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -76,7 +76,14 @@ config CC_HAS_ASM_INLINE config CONSTRUCTORS bool - depends on !UML + +config CONSTRUCTORS_KERNEL + def_bool y + depends on CONSTRUCTORS && !UML + +config CONSTRUCTORS_MODULE + def_bool y + depends on CONSTRUCTORS config IRQ_WORK bool diff --git a/init/main.c b/init/main.c index c68d784376ca..51eb4802511c 100644 --- a/init/main.c +++ b/init/main.c @@ -1066,7 +1066,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) /* Call all constructor functions linked into the kernel. */ static void __init do_ctors(void) { -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_KERNEL ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; for (; fn < (ctor_fn_t *) __ctors_end; fn++) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index 3110c77230c7..f62de2dea8a3 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" config GCOV_KERNEL bool "Enable gcov-based kernel profiling" depends on DEBUG_FS - select CONSTRUCTORS if !UML + select CONSTRUCTORS default n help This option enables gcov-based code profiling (e.g. for code coverage diff --git a/kernel/module.c b/kernel/module.c index 4bf30e4b3eaa..c161a360d929 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3257,7 +3257,7 @@ static int find_module_sections(struct module *mod, struct load_info *info) &mod->num_unused_gpl_syms); mod->unused_gpl_crcs = section_addr(info, "__kcrctab_unused_gpl"); #endif -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE mod->ctors = section_objs(info, ".ctors", sizeof(*mod->ctors), &mod->num_ctors); if (!mod->ctors) @@ -3612,7 +3612,7 @@ static bool finished_loading(const char *name) /* Call module constructors. */ static void do_mod_ctors(struct module *mod) { -#ifdef CONFIG_CONSTRUCTORS +#ifdef CONFIG_CONSTRUCTORS_MODULE unsigned long i; for (i = 0; i < mod->num_ctors; i++) -- 2.26.2 _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML 2021-01-19 11:18 ` Johannes Berg @ 2021-01-20 16:07 ` Peter Oberparleiter -1 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 16:07 UTC (permalink / raw) To: Johannes Berg, linux-um Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Johannes Berg, Andrew Morton On 19.01.2021 12:18, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into > CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both > of which are enabled by default if CONFIG_CONSTRUCTORS is turned > on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's > not used on ARCH=um. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Do you expect other users for these new config symbols? If not it seems to me that the goal of enabling module constructors for UML could also be achieved without introducing new config symbols. For example you could suppress calling any built-in kernel constructors in case of UML by extending the ifdef in do_ctors() to depend on both CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). Of course you'd still need to remove the !UML dependency in CONFIG_GCOV_KERNEL. > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. > > I have no idea which tree this might go through, any suggestions? So far Andrew Morton was kind enough to pick up gcov-kernel related changes, so that route might be an option. -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML @ 2021-01-20 16:07 ` Peter Oberparleiter 0 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 16:07 UTC (permalink / raw) To: Johannes Berg, linux-um Cc: linux-arch, Arnd Bergmann, Johannes Berg, linux-kernel, Jessica Yu, Andrew Morton On 19.01.2021 12:18, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by splitting CONFIG_CONSTRUCTORS into > CONFIG_CONSTRUCTORS_KERNEL and CONFIG_CONSTRUCTORS_MODULE, both > of which are enabled by default if CONFIG_CONSTRUCTORS is turned > on, but CONFIG_CONSTRUCTORS_KERNEL depends on !UML so that it's > not used on ARCH=um. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Do you expect other users for these new config symbols? If not it seems to me that the goal of enabling module constructors for UML could also be achieved without introducing new config symbols. For example you could suppress calling any built-in kernel constructors in case of UML by extending the ifdef in do_ctors() to depend on both CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). Of course you'd still need to remove the !UML dependency in CONFIG_GCOV_KERNEL. > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. > > I have no idea which tree this might go through, any suggestions? So far Andrew Morton was kind enough to pick up gcov-kernel related changes, so that route might be an option. -- Peter Oberparleiter Linux on Z Development - IBM Germany _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML 2021-01-20 16:07 ` Peter Oberparleiter @ 2021-01-20 16:09 ` Johannes Berg -1 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 16:09 UTC (permalink / raw) To: Peter Oberparleiter, linux-um Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Andrew Morton On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote: > Do you expect other users for these new config symbols? Probably not. > If not it seems > to me that the goal of enabling module constructors for UML could also > be achieved without introducing new config symbols. Yeah, true. > For example you could suppress calling any built-in kernel constructors > in case of UML by extending the ifdef in do_ctors() to depend on both > CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). > > Of course you'd still need to remove the !UML dependency in > CONFIG_GCOV_KERNEL. Right. I can post a separate patch and we can see which one looks nicer? johannes ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML @ 2021-01-20 16:09 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 16:09 UTC (permalink / raw) To: Peter Oberparleiter, linux-um Cc: linux-arch, Andrew Morton, linux-kernel, Arnd Bergmann, Jessica Yu On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote: > Do you expect other users for these new config symbols? Probably not. > If not it seems > to me that the goal of enabling module constructors for UML could also > be achieved without introducing new config symbols. Yeah, true. > For example you could suppress calling any built-in kernel constructors > in case of UML by extending the ifdef in do_ctors() to depend on both > CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). > > Of course you'd still need to remove the !UML dependency in > CONFIG_GCOV_KERNEL. Right. I can post a separate patch and we can see which one looks nicer? johannes _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML 2021-01-20 16:09 ` Johannes Berg @ 2021-01-20 16:20 ` Peter Oberparleiter -1 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 16:20 UTC (permalink / raw) To: Johannes Berg, linux-um Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Andrew Morton On 20.01.2021 17:09, Johannes Berg wrote: > On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote: > >> Do you expect other users for these new config symbols? > > Probably not. > >> If not it seems >> to me that the goal of enabling module constructors for UML could also >> be achieved without introducing new config symbols. > > Yeah, true. > >> For example you could suppress calling any built-in kernel constructors >> in case of UML by extending the ifdef in do_ctors() to depend on both >> CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). >> >> Of course you'd still need to remove the !UML dependency in >> CONFIG_GCOV_KERNEL. > > Right. > > I can post a separate patch and we can see which one looks nicer? Sounds good! -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML @ 2021-01-20 16:20 ` Peter Oberparleiter 0 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 16:20 UTC (permalink / raw) To: Johannes Berg, linux-um Cc: linux-arch, Andrew Morton, linux-kernel, Arnd Bergmann, Jessica Yu On 20.01.2021 17:09, Johannes Berg wrote: > On Wed, 2021-01-20 at 17:07 +0100, Peter Oberparleiter wrote: > >> Do you expect other users for these new config symbols? > > Probably not. > >> If not it seems >> to me that the goal of enabling module constructors for UML could also >> be achieved without introducing new config symbols. > > Yeah, true. > >> For example you could suppress calling any built-in kernel constructors >> in case of UML by extending the ifdef in do_ctors() to depend on both >> CONFIG_CONSTRUCTORS and !CONFIG_UML (maybe with an explanatory comment). >> >> Of course you'd still need to remove the !UML dependency in >> CONFIG_GCOV_KERNEL. > > Right. > > I can post a separate patch and we can see which one looks nicer? Sounds good! -- Peter Oberparleiter Linux on Z Development - IBM Germany _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-01-20 16:09 ` Johannes Berg @ 2021-01-20 16:20 ` Johannes Berg -1 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 16:20 UTC (permalink / raw) To: linux-um Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Peter Oberparleiter, Andrew Morton, Johannes Berg From: Johannes Berg <johannes.berg@intel.com> On ARCH=um, loading a module doesn't result in its constructors getting called, which breaks module gcov since the debugfs files are never registered. On the other hand, in-kernel constructors have already been called by the dynamic linker, so we can't call them again. Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be selected, but avoiding the in-kernel constructor calls. Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, since we really do want CONSTRUCTORS, just not kernel binary ones. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- Tested with a kernel configured with CONFIG_GCOV_KERNEL, without the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart from the reset file), and with it we get the files and they work. v2: * special-case UML instead of splitting the CONSTRUCTORS config --- init/Kconfig | 1 - init/main.c | 8 +++++++- kernel/gcov/Kconfig | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..29ad68325028 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE config CONSTRUCTORS bool - depends on !UML config IRQ_WORK bool diff --git a/init/main.c b/init/main.c index c68d784376ca..a626e78dbf06 100644 --- a/init/main.c +++ b/init/main.c @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) /* Call all constructor functions linked into the kernel. */ static void __init do_ctors(void) { -#ifdef CONFIG_CONSTRUCTORS +/* + * For UML, the constructors have already been called by the + * normal setup code as it's just a normal ELF binary, so we + * cannot do it again - but we do need CONFIG_CONSTRUCTORS + * even on UML for modules. + */ +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; for (; fn < (ctor_fn_t *) __ctors_end; fn++) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index 3110c77230c7..f62de2dea8a3 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" config GCOV_KERNEL bool "Enable gcov-based kernel profiling" depends on DEBUG_FS - select CONSTRUCTORS if !UML + select CONSTRUCTORS default n help This option enables gcov-based code profiling (e.g. for code coverage -- 2.26.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-01-20 16:20 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 16:20 UTC (permalink / raw) To: linux-um Cc: linux-arch, Johannes Berg, Arnd Bergmann, Peter Oberparleiter, linux-kernel, Jessica Yu, Andrew Morton From: Johannes Berg <johannes.berg@intel.com> On ARCH=um, loading a module doesn't result in its constructors getting called, which breaks module gcov since the debugfs files are never registered. On the other hand, in-kernel constructors have already been called by the dynamic linker, so we can't call them again. Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be selected, but avoiding the in-kernel constructor calls. Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, since we really do want CONSTRUCTORS, just not kernel binary ones. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- Tested with a kernel configured with CONFIG_GCOV_KERNEL, without the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart from the reset file), and with it we get the files and they work. v2: * special-case UML instead of splitting the CONSTRUCTORS config --- init/Kconfig | 1 - init/main.c | 8 +++++++- kernel/gcov/Kconfig | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..29ad68325028 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE config CONSTRUCTORS bool - depends on !UML config IRQ_WORK bool diff --git a/init/main.c b/init/main.c index c68d784376ca..a626e78dbf06 100644 --- a/init/main.c +++ b/init/main.c @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) /* Call all constructor functions linked into the kernel. */ static void __init do_ctors(void) { -#ifdef CONFIG_CONSTRUCTORS +/* + * For UML, the constructors have already been called by the + * normal setup code as it's just a normal ELF binary, so we + * cannot do it again - but we do need CONFIG_CONSTRUCTORS + * even on UML for modules. + */ +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; for (; fn < (ctor_fn_t *) __ctors_end; fn++) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index 3110c77230c7..f62de2dea8a3 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" config GCOV_KERNEL bool "Enable gcov-based kernel profiling" depends on DEBUG_FS - select CONSTRUCTORS if !UML + select CONSTRUCTORS default n help This option enables gcov-based code profiling (e.g. for code coverage -- 2.26.2 _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-01-20 16:20 ` Johannes Berg @ 2021-01-20 17:04 ` Peter Oberparleiter -1 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 17:04 UTC (permalink / raw) To: Johannes Berg, Andrew Morton Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Johannes Berg, linux-um On 20.01.2021 17:20, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be > selected, but avoiding the in-kernel constructor calls. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Looks good+nicer than v1 to me! I also tested this patch on s390 and can confirm that it doesn't break gcov-kernel there. Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> Andrew, do we need additional reviews/sign-offs? Could this go in via your tree? > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. Just to be sure: could you confirm that this test statement for UML also applies to v2, i.e. that it fixes the original problem you were seeing? > > v2: > * special-case UML instead of splitting the CONSTRUCTORS config > --- > init/Kconfig | 1 - > init/main.c | 8 +++++++- > kernel/gcov/Kconfig | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..29ad68325028 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE > > config CONSTRUCTORS > bool > - depends on !UML > > config IRQ_WORK > bool > diff --git a/init/main.c b/init/main.c > index c68d784376ca..a626e78dbf06 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) > /* Call all constructor functions linked into the kernel. */ > static void __init do_ctors(void) > { > -#ifdef CONFIG_CONSTRUCTORS > +/* > + * For UML, the constructors have already been called by the > + * normal setup code as it's just a normal ELF binary, so we > + * cannot do it again - but we do need CONFIG_CONSTRUCTORS > + * even on UML for modules. > + */ > +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) > ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; > > for (; fn < (ctor_fn_t *) __ctors_end; fn++) > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > index 3110c77230c7..f62de2dea8a3 100644 > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" > config GCOV_KERNEL > bool "Enable gcov-based kernel profiling" > depends on DEBUG_FS > - select CONSTRUCTORS if !UML > + select CONSTRUCTORS > default n > help > This option enables gcov-based code profiling (e.g. for code coverage > -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-01-20 17:04 ` Peter Oberparleiter 0 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-01-20 17:04 UTC (permalink / raw) To: Johannes Berg, Andrew Morton Cc: linux-arch, Arnd Bergmann, Johannes Berg, linux-um, linux-kernel, Jessica Yu On 20.01.2021 17:20, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be > selected, but avoiding the in-kernel constructor calls. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Looks good+nicer than v1 to me! I also tested this patch on s390 and can confirm that it doesn't break gcov-kernel there. Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> Andrew, do we need additional reviews/sign-offs? Could this go in via your tree? > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. Just to be sure: could you confirm that this test statement for UML also applies to v2, i.e. that it fixes the original problem you were seeing? > > v2: > * special-case UML instead of splitting the CONSTRUCTORS config > --- > init/Kconfig | 1 - > init/main.c | 8 +++++++- > kernel/gcov/Kconfig | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..29ad68325028 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE > > config CONSTRUCTORS > bool > - depends on !UML > > config IRQ_WORK > bool > diff --git a/init/main.c b/init/main.c > index c68d784376ca..a626e78dbf06 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) > /* Call all constructor functions linked into the kernel. */ > static void __init do_ctors(void) > { > -#ifdef CONFIG_CONSTRUCTORS > +/* > + * For UML, the constructors have already been called by the > + * normal setup code as it's just a normal ELF binary, so we > + * cannot do it again - but we do need CONFIG_CONSTRUCTORS > + * even on UML for modules. > + */ > +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) > ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; > > for (; fn < (ctor_fn_t *) __ctors_end; fn++) > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > index 3110c77230c7..f62de2dea8a3 100644 > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" > config GCOV_KERNEL > bool "Enable gcov-based kernel profiling" > depends on DEBUG_FS > - select CONSTRUCTORS if !UML > + select CONSTRUCTORS > default n > help > This option enables gcov-based code profiling (e.g. for code coverage > -- Peter Oberparleiter Linux on Z Development - IBM Germany _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-01-20 17:04 ` Peter Oberparleiter @ 2021-01-20 17:38 ` Johannes Berg -1 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 17:38 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um On Wed, 2021-01-20 at 18:04 +0100, Peter Oberparleiter wrote: > > > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > > from the reset file), and with it we get the files and they work. > > Just to be sure: could you confirm that this test statement for UML also > applies to v2, i.e. that it fixes the original problem you were seeing? Yes, I tested this version too :) johannes ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-01-20 17:38 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-01-20 17:38 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: linux-arch, linux-um, linux-kernel, Arnd Bergmann, Jessica Yu On Wed, 2021-01-20 at 18:04 +0100, Peter Oberparleiter wrote: > > > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > > from the reset file), and with it we get the files and they work. > > Just to be sure: could you confirm that this test statement for UML also > applies to v2, i.e. that it fixes the original problem you were seeing? Yes, I tested this version too :) johannes _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-01-20 17:04 ` Peter Oberparleiter (?) (?) @ 2021-05-08 13:50 ` Lambert 2021-05-10 11:37 ` Johannes Berg 2021-05-14 13:55 ` Peter Oberparleiter -1 siblings, 2 replies; 21+ messages in thread From: Lambert @ 2021-05-08 13:50 UTC (permalink / raw) To: Peter Oberparleiter, Johannes Berg Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Johannes Berg, linux-um, Lambert. > On Jan 21, 2021, at 1:04 AM, Peter Oberparleiter <oberpar@linux.ibm.com> wrote: > > On 20.01.2021 17:20, Johannes Berg wrote: >> From: Johannes Berg <johannes.berg@intel.com> >> >> On ARCH=um, loading a module doesn't result in its constructors >> getting called, which breaks module gcov since the debugfs files >> are never registered. On the other hand, in-kernel constructors >> have already been called by the dynamic linker, so we can't call >> them again. >> >> Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be >> selected, but avoiding the in-kernel constructor calls. >> >> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, >> since we really do want CONSTRUCTORS, just not kernel binary >> ones. >> >> Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > Looks good+nicer than v1 to me! > > I also tested this patch on s390 and can confirm that it doesn't break > gcov-kernel there. > > Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> > > Andrew, do we need additional reviews/sign-offs? Could this go in via > your tree? > >> --- >> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without >> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart >> from the reset file), and with it we get the files and they work. > > Just to be sure: could you confirm that this test statement for UML also > applies to v2, i.e. that it fixes the original problem you were seeing? > Hi Johannes and Peter, sorry to bother but I have one question on this change. The do_ctors() won’t be executed for UML because *the constructors have already been called for ELF*. The init_array section for each C file is linked to *vmlinux* between *__ctors_start* and *__ctors_end* symbols. See link: https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 In my environment, UML+GCC 10, I can't find __gcov_init executed before kernel starts. So I did some trace and found glibc __libc_csu_init will only execute constructors between *__init_array_start*and *__init_array_end*. Which means if do_ctors() is not executed for UML, no elsewhere will the constructors be executed. Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed some steps to make the GCOV work for UML? >> >> v2: >> * special-case UML instead of splitting the CONSTRUCTORS config >> --- >> init/Kconfig | 1 - >> init/main.c | 8 +++++++- >> kernel/gcov/Kconfig | 2 +- >> 3 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/init/Kconfig b/init/Kconfig >> index b77c60f8b963..29ad68325028 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE >> >> config CONSTRUCTORS >> bool >> - depends on !UML >> >> config IRQ_WORK >> bool >> diff --git a/init/main.c b/init/main.c >> index c68d784376ca..a626e78dbf06 100644 >> --- a/init/main.c >> +++ b/init/main.c >> @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) >> /* Call all constructor functions linked into the kernel. */ >> static void __init do_ctors(void) >> { >> -#ifdef CONFIG_CONSTRUCTORS >> +/* >> + * For UML, the constructors have already been called by the >> + * normal setup code as it's just a normal ELF binary, so we >> + * cannot do it again - but we do need CONFIG_CONSTRUCTORS >> + * even on UML for modules. >> + */ >> +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) >> ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; >> >> for (; fn < (ctor_fn_t *) __ctors_end; fn++) >> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig >> index 3110c77230c7..f62de2dea8a3 100644 >> --- a/kernel/gcov/Kconfig >> +++ b/kernel/gcov/Kconfig >> @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" >> config GCOV_KERNEL >> bool "Enable gcov-based kernel profiling" >> depends on DEBUG_FS >> - select CONSTRUCTORS if !UML >> + select CONSTRUCTORS >> default n >> help >> This option enables gcov-based code profiling (e.g. for code coverage >> > > > -- > Peter Oberparleiter > Linux on Z Development - IBM Germany > _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-05-08 13:50 ` Lambert @ 2021-05-10 11:37 ` Johannes Berg 2021-05-14 13:55 ` Peter Oberparleiter 1 sibling, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-05-10 11:37 UTC (permalink / raw) To: Lambert, Peter Oberparleiter Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um, Lambert. Hi, > Hi Johannes and Peter, sorry to bother but I have one question > on this change. The do_ctors() won’t be executed for UML > because *the constructors have already been called for ELF*. > > *__ctors_start* and *__ctors_end* symbols. See link: > https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 > > In my environment, UML+GCC 10, I can't find __gcov_init executed > before kernel starts. So I did some trace and found glibc > __libc_csu_init > will only execute constructors between *__init_array_start*and > *__init_array_end*. > Which means if do_ctors() is not executed for UML, no elsewhere will > the constructors be executed. > > Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed > some steps to make the GCOV work for UML? No, that doesn't seem like the right solution. Perhaps then with that toolchain (or configuration thereof) we need to provide __init_array_start/end labels? Or ... maybe that actually just needs to be removed, so that the toolchain gets to choose? Hmm. Pretty sure it worked for me, I think also with gcc 10, but not sure exactly where I tested. johannes ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-05-10 11:37 ` Johannes Berg 0 siblings, 0 replies; 21+ messages in thread From: Johannes Berg @ 2021-05-10 11:37 UTC (permalink / raw) To: Lambert, Peter Oberparleiter Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um, Lambert. Hi, > Hi Johannes and Peter, sorry to bother but I have one question > on this change. The do_ctors() won’t be executed for UML > because *the constructors have already been called for ELF*. > > *__ctors_start* and *__ctors_end* symbols. See link: > https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 > > In my environment, UML+GCC 10, I can't find __gcov_init executed > before kernel starts. So I did some trace and found glibc > __libc_csu_init > will only execute constructors between *__init_array_start*and > *__init_array_end*. > Which means if do_ctors() is not executed for UML, no elsewhere will > the constructors be executed. > > Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed > some steps to make the GCOV work for UML? No, that doesn't seem like the right solution. Perhaps then with that toolchain (or configuration thereof) we need to provide __init_array_start/end labels? Or ... maybe that actually just needs to be removed, so that the toolchain gets to choose? Hmm. Pretty sure it worked for me, I think also with gcc 10, but not sure exactly where I tested. johannes _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-05-10 11:37 ` Johannes Berg @ 2021-05-10 13:31 ` lambertdev -1 siblings, 0 replies; 21+ messages in thread From: lambertdev @ 2021-05-10 13:31 UTC (permalink / raw) To: Johannes Berg, Peter Oberparleiter Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um Hi Johannes, My original Email address is blocked by the server of kernel.org, so I have to change an Email address. Please see my reply inline. >Hi, > >> Hi Johannes and Peter, sorry to bother but I have one question >> on this change. The do_ctors() won’t be executed for UML >> because *the constructors have already been called for ELF*. >> >> *__ctors_start* and *__ctors_end* symbols. See link: >> https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 >> >> In my environment, UML+GCC 10, I can't find __gcov_init executed >> before kernel starts. So I did some trace and found glibc >> __libc_csu_init >> will only execute constructors between *__init_array_start*and >> *__init_array_end*. >> Which means if do_ctors() is not executed for UML, no elsewhere will >> the constructors be executed. >> >> Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed >> some steps to make the GCOV work for UML? > >No, that doesn't seem like the right solution. > >Perhaps then with that toolchain (or configuration thereof) we need to >provide __init_array_start/end labels? Yes, that's how I worked around in my local environment (change linker script to add _init_array_start/end labels). So the __gcov_init is called before start_kernel. > >Or ... maybe that actually just needs to be removed, so that the >toolchain gets to choose? > >Hmm. Pretty sure it worked for me, I think also with gcc 10, but not >sure exactly where I tested. I will make sure my environment is clean and try again. Could you please use gdb to check when __gcov_init is called in your setup? e.g. (1) gdb *your uml elf* (2) b __gcov_init (3) r (4) when the execution is breaked, use 'bt' to check the call stack Thanks Lambert > >johannes > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-05-10 13:31 ` lambertdev 0 siblings, 0 replies; 21+ messages in thread From: lambertdev @ 2021-05-10 13:31 UTC (permalink / raw) To: Johannes Berg, Peter Oberparleiter Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, linux-um Hi Johannes, My original Email address is blocked by the server of kernel.org, so I have to change an Email address. Please see my reply inline. >Hi, > >> Hi Johannes and Peter, sorry to bother but I have one question >> on this change. The do_ctors() won’t be executed for UML >> because *the constructors have already been called for ELF*. >> >> *__ctors_start* and *__ctors_end* symbols. See link: >> https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 >> >> In my environment, UML+GCC 10, I can't find __gcov_init executed >> before kernel starts. So I did some trace and found glibc >> __libc_csu_init >> will only execute constructors between *__init_array_start*and >> *__init_array_end*. >> Which means if do_ctors() is not executed for UML, no elsewhere will >> the constructors be executed. >> >> Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed >> some steps to make the GCOV work for UML? > >No, that doesn't seem like the right solution. > >Perhaps then with that toolchain (or configuration thereof) we need to >provide __init_array_start/end labels? Yes, that's how I worked around in my local environment (change linker script to add _init_array_start/end labels). So the __gcov_init is called before start_kernel. > >Or ... maybe that actually just needs to be removed, so that the >toolchain gets to choose? > >Hmm. Pretty sure it worked for me, I think also with gcc 10, but not >sure exactly where I tested. I will make sure my environment is clean and try again. Could you please use gdb to check when __gcov_init is called in your setup? e.g. (1) gdb *your uml elf* (2) b __gcov_init (3) r (4) when the execution is breaked, use 'bt' to check the call stack Thanks Lambert > >johannes > > > _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov 2021-05-08 13:50 ` Lambert @ 2021-05-14 13:55 ` Peter Oberparleiter 2021-05-14 13:55 ` Peter Oberparleiter 1 sibling, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-05-14 13:55 UTC (permalink / raw) To: Lambert, Johannes Berg, lambertdev, Jeff Dike, richard, anton.ivanov Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Johannes Berg, linux-um, Lambert. I'm adding UML maintainers for their input. On 08.05.2021 15:50, Lambert wrote: >> On Jan 21, 2021, at 1:04 AM, Peter Oberparleiter <oberpar@linux.ibm.com> wrote: >> On 20.01.2021 17:20, Johannes Berg wrote: >>> From: Johannes Berg <johannes.berg@intel.com> >>> >>> On ARCH=um, loading a module doesn't result in its constructors >>> getting called, which breaks module gcov since the debugfs files >>> are never registered. On the other hand, in-kernel constructors >>> have already been called by the dynamic linker, so we can't call >>> them again. >>> >>> Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be >>> selected, but avoiding the in-kernel constructor calls. >>> >>> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, >>> since we really do want CONSTRUCTORS, just not kernel binary >>> ones. >>> >>> Signed-off-by: Johannes Berg <johannes.berg@intel.com> >> >> Looks good+nicer than v1 to me! >> >> I also tested this patch on s390 and can confirm that it doesn't break >> gcov-kernel there. >> >> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> >> >> Andrew, do we need additional reviews/sign-offs? Could this go in via >> your tree? >> >>> --- >>> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without >>> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart >>> from the reset file), and with it we get the files and they work. >> >> Just to be sure: could you confirm that this test statement for UML also >> applies to v2, i.e. that it fixes the original problem you were seeing? >> > > Hi Johannes and Peter, sorry to bother but I have one question > on this change. The do_ctors() won’t be executed for UML > because *the constructors have already been called for ELF*. > > The init_array section for each C file is linked to *vmlinux* between > *__ctors_start* and *__ctors_end* symbols. See link: > https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 > > In my environment, UML+GCC 10, I can't find __gcov_init executed > before kernel starts. So I did some trace and found glibc __libc_csu_init > will only execute constructors between *__init_array_start*and *__init_array_end*. > Which means if do_ctors() is not executed for UML, no elsewhere will > the constructors be executed. > > Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed > some steps to make the GCOV work for UML? I'm not an expert for the UML-arch, but based on this report I did some investigations. I think that this patch might be partially in error. Before the patch no constructors were called for UML and as a result gcov-kernel profiling was not available, neither for built-in code nor for module code. With the patch CONFIG_CONSTRUCTORS was made available on UML and with it gcov-kernel profiling. This works well for kernel modules (which I assume is what Johannes tested), but it fails for built-in code. Note that since UML does not specify ARCH_HAS_GCOV_PROFILE_ALL you need to manually add GCOV_PROFILE := y to any kernel Makefile if you want to enable profiling for that built-in code. The subject patch is based on the assumption that constructors for built-in kernel code is called from glibc when the vmlinux executable is started. This does not work the way it was assumed it would, and would be broken if it did. It does not work because, as was already pointed out, glibc is looking for the __init_array_start symbol to locate the list of constructor function pointers, but the kernel uses a different symbol name (__ctors_start). If you add the __init_array_start and _end symbols glibc will call these functions correctly, but the kernel will crash. The reason for the crash is that the kernel implementation of __gcov_init() which is called from the constructors emitted by GCC's gcov-profiling code assumes that base kernel facilities (such as printk) have been initialized and are working as expected. Of course this is not the case when the function is called by glibc before any kernel init function was run. I see multiple ways to go forward: 0. Do nothing This patch doesn't break anything - it's just only partially effective in that in enables gcov-kernel profiling for modules, but not for built-in code. 1. Revert this patch => gcov-kernel profiling for kernel modules on UML would no longer work 2. Enable do_ctors() on UML => gcov-kernel profiling would work on UML for both kernel modules and built-in code. There's a risk though that other constructors unrelated to gcov might be called too late (I saw at least one register_tm_clones() call while looking at the .init_array section of an on UML executable). This risk is specific to UML code that runs before start_kernel(). I'm not sure if there is anything there that consciously relies on constructors. 3. Change gcov_init() to work without relying on kernel facilities and add init_array_start and _end symbols to KERNEL_CTORS in include/asm-generic/vmlinux.lds.h so that glibc can find and run constructors before start_kernel(). Not sure if this is easily possible. Certainly this is the option that results in the most amount of work. => gcov-kernel profiling would work on UML, any other constructors for code run before start_kernel() would work, but this would also affect other kernel code that relies on constructors (currently KASAN)... so might also have further unwanted side-effects. Any thoughts, comments or ideas for other alternatives? > >>> >>> v2: >>> * special-case UML instead of splitting the CONSTRUCTORS config >>> --- >>> init/Kconfig | 1 - >>> init/main.c | 8 +++++++- >>> kernel/gcov/Kconfig | 2 +- >>> 3 files changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/init/Kconfig b/init/Kconfig >>> index b77c60f8b963..29ad68325028 100644 >>> --- a/init/Kconfig >>> +++ b/init/Kconfig >>> @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE >>> >>> config CONSTRUCTORS >>> bool >>> - depends on !UML >>> >>> config IRQ_WORK >>> bool >>> diff --git a/init/main.c b/init/main.c >>> index c68d784376ca..a626e78dbf06 100644 >>> --- a/init/main.c >>> +++ b/init/main.c >>> @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) >>> /* Call all constructor functions linked into the kernel. */ >>> static void __init do_ctors(void) >>> { >>> -#ifdef CONFIG_CONSTRUCTORS >>> +/* >>> + * For UML, the constructors have already been called by the >>> + * normal setup code as it's just a normal ELF binary, so we >>> + * cannot do it again - but we do need CONFIG_CONSTRUCTORS >>> + * even on UML for modules. >>> + */ >>> +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) >>> ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; >>> >>> for (; fn < (ctor_fn_t *) __ctors_end; fn++) >>> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig >>> index 3110c77230c7..f62de2dea8a3 100644 >>> --- a/kernel/gcov/Kconfig >>> +++ b/kernel/gcov/Kconfig >>> @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" >>> config GCOV_KERNEL >>> bool "Enable gcov-based kernel profiling" >>> depends on DEBUG_FS >>> - select CONSTRUCTORS if !UML >>> + select CONSTRUCTORS >>> default n >>> help >>> This option enables gcov-based code profiling (e.g. for code coverage >>> >> >> >> -- >> Peter Oberparleiter >> Linux on Z Development - IBM Germany -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov @ 2021-05-14 13:55 ` Peter Oberparleiter 0 siblings, 0 replies; 21+ messages in thread From: Peter Oberparleiter @ 2021-05-14 13:55 UTC (permalink / raw) To: Lambert, Johannes Berg, lambertdev, Jeff Dike, richard, anton.ivanov Cc: Andrew Morton, linux-arch, linux-kernel, Arnd Bergmann, Jessica Yu, Johannes Berg, linux-um, Lambert. I'm adding UML maintainers for their input. On 08.05.2021 15:50, Lambert wrote: >> On Jan 21, 2021, at 1:04 AM, Peter Oberparleiter <oberpar@linux.ibm.com> wrote: >> On 20.01.2021 17:20, Johannes Berg wrote: >>> From: Johannes Berg <johannes.berg@intel.com> >>> >>> On ARCH=um, loading a module doesn't result in its constructors >>> getting called, which breaks module gcov since the debugfs files >>> are never registered. On the other hand, in-kernel constructors >>> have already been called by the dynamic linker, so we can't call >>> them again. >>> >>> Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be >>> selected, but avoiding the in-kernel constructor calls. >>> >>> Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, >>> since we really do want CONSTRUCTORS, just not kernel binary >>> ones. >>> >>> Signed-off-by: Johannes Berg <johannes.berg@intel.com> >> >> Looks good+nicer than v1 to me! >> >> I also tested this patch on s390 and can confirm that it doesn't break >> gcov-kernel there. >> >> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> >> >> Andrew, do we need additional reviews/sign-offs? Could this go in via >> your tree? >> >>> --- >>> Tested with a kernel configured with CONFIG_GCOV_KERNEL, without >>> the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart >>> from the reset file), and with it we get the files and they work. >> >> Just to be sure: could you confirm that this test statement for UML also >> applies to v2, i.e. that it fixes the original problem you were seeing? >> > > Hi Johannes and Peter, sorry to bother but I have one question > on this change. The do_ctors() won’t be executed for UML > because *the constructors have already been called for ELF*. > > The init_array section for each C file is linked to *vmlinux* between > *__ctors_start* and *__ctors_end* symbols. See link: > https://elixir.bootlin.com/linux/v5.12.2/source/include/asm-generic/vmlinux.lds.h#L676 > > In my environment, UML+GCC 10, I can't find __gcov_init executed > before kernel starts. So I did some trace and found glibc __libc_csu_init > will only execute constructors between *__init_array_start*and *__init_array_end*. > Which means if do_ctors() is not executed for UML, no elsewhere will > the constructors be executed. > > Shall we remove the *!defined(CONFIG_UML)* for GCC, or I just missed > some steps to make the GCOV work for UML? I'm not an expert for the UML-arch, but based on this report I did some investigations. I think that this patch might be partially in error. Before the patch no constructors were called for UML and as a result gcov-kernel profiling was not available, neither for built-in code nor for module code. With the patch CONFIG_CONSTRUCTORS was made available on UML and with it gcov-kernel profiling. This works well for kernel modules (which I assume is what Johannes tested), but it fails for built-in code. Note that since UML does not specify ARCH_HAS_GCOV_PROFILE_ALL you need to manually add GCOV_PROFILE := y to any kernel Makefile if you want to enable profiling for that built-in code. The subject patch is based on the assumption that constructors for built-in kernel code is called from glibc when the vmlinux executable is started. This does not work the way it was assumed it would, and would be broken if it did. It does not work because, as was already pointed out, glibc is looking for the __init_array_start symbol to locate the list of constructor function pointers, but the kernel uses a different symbol name (__ctors_start). If you add the __init_array_start and _end symbols glibc will call these functions correctly, but the kernel will crash. The reason for the crash is that the kernel implementation of __gcov_init() which is called from the constructors emitted by GCC's gcov-profiling code assumes that base kernel facilities (such as printk) have been initialized and are working as expected. Of course this is not the case when the function is called by glibc before any kernel init function was run. I see multiple ways to go forward: 0. Do nothing This patch doesn't break anything - it's just only partially effective in that in enables gcov-kernel profiling for modules, but not for built-in code. 1. Revert this patch => gcov-kernel profiling for kernel modules on UML would no longer work 2. Enable do_ctors() on UML => gcov-kernel profiling would work on UML for both kernel modules and built-in code. There's a risk though that other constructors unrelated to gcov might be called too late (I saw at least one register_tm_clones() call while looking at the .init_array section of an on UML executable). This risk is specific to UML code that runs before start_kernel(). I'm not sure if there is anything there that consciously relies on constructors. 3. Change gcov_init() to work without relying on kernel facilities and add init_array_start and _end symbols to KERNEL_CTORS in include/asm-generic/vmlinux.lds.h so that glibc can find and run constructors before start_kernel(). Not sure if this is easily possible. Certainly this is the option that results in the most amount of work. => gcov-kernel profiling would work on UML, any other constructors for code run before start_kernel() would work, but this would also affect other kernel code that relies on constructors (currently KASAN)... so might also have further unwanted side-effects. Any thoughts, comments or ideas for other alternatives? > >>> >>> v2: >>> * special-case UML instead of splitting the CONSTRUCTORS config >>> --- >>> init/Kconfig | 1 - >>> init/main.c | 8 +++++++- >>> kernel/gcov/Kconfig | 2 +- >>> 3 files changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/init/Kconfig b/init/Kconfig >>> index b77c60f8b963..29ad68325028 100644 >>> --- a/init/Kconfig >>> +++ b/init/Kconfig >>> @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE >>> >>> config CONSTRUCTORS >>> bool >>> - depends on !UML >>> >>> config IRQ_WORK >>> bool >>> diff --git a/init/main.c b/init/main.c >>> index c68d784376ca..a626e78dbf06 100644 >>> --- a/init/main.c >>> +++ b/init/main.c >>> @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) >>> /* Call all constructor functions linked into the kernel. */ >>> static void __init do_ctors(void) >>> { >>> -#ifdef CONFIG_CONSTRUCTORS >>> +/* >>> + * For UML, the constructors have already been called by the >>> + * normal setup code as it's just a normal ELF binary, so we >>> + * cannot do it again - but we do need CONFIG_CONSTRUCTORS >>> + * even on UML for modules. >>> + */ >>> +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) >>> ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; >>> >>> for (; fn < (ctor_fn_t *) __ctors_end; fn++) >>> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig >>> index 3110c77230c7..f62de2dea8a3 100644 >>> --- a/kernel/gcov/Kconfig >>> +++ b/kernel/gcov/Kconfig >>> @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" >>> config GCOV_KERNEL >>> bool "Enable gcov-based kernel profiling" >>> depends on DEBUG_FS >>> - select CONSTRUCTORS if !UML >>> + select CONSTRUCTORS >>> default n >>> help >>> This option enables gcov-based code profiling (e.g. for code coverage >>> >> >> >> -- >> Peter Oberparleiter >> Linux on Z Development - IBM Germany -- Peter Oberparleiter Linux on Z Development - IBM Germany _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-05-14 13:58 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-19 11:18 [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML Johannes Berg 2021-01-19 11:18 ` Johannes Berg 2021-01-20 16:07 ` Peter Oberparleiter 2021-01-20 16:07 ` Peter Oberparleiter 2021-01-20 16:09 ` Johannes Berg 2021-01-20 16:09 ` Johannes Berg 2021-01-20 16:20 ` Peter Oberparleiter 2021-01-20 16:20 ` Peter Oberparleiter 2021-01-20 16:20 ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg 2021-01-20 16:20 ` Johannes Berg 2021-01-20 17:04 ` Peter Oberparleiter 2021-01-20 17:04 ` Peter Oberparleiter 2021-01-20 17:38 ` Johannes Berg 2021-01-20 17:38 ` Johannes Berg 2021-05-08 13:50 ` Lambert 2021-05-10 11:37 ` Johannes Berg 2021-05-10 11:37 ` Johannes Berg 2021-05-10 13:31 ` lambertdev 2021-05-10 13:31 ` lambertdev 2021-05-14 13:55 ` Peter Oberparleiter 2021-05-14 13:55 ` Peter Oberparleiter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.