* [PATCH] gcov: fix clang-11+ support @ 2021-03-12 19:21 Nick Desaulniers 2021-03-12 19:58 ` Nathan Chancellor 2021-03-12 20:25 ` [PATCH] gcov: fix clang-11+ support Fangrui Song 0 siblings, 2 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 19:21 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: Nick Desaulniers, Fangrui Song, Prasad Sodagudi, Nathan Chancellor, linux-kernel, clang-built-linux LLVM changed the expected function signatures for llvm_gcda_start_file() and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 or newer may have noticed their kernels failing to boot due to a panic when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up the function signatures so calling these functions doesn't panic the kernel. When we drop clang-10 support from the kernel, we should carefully update the original implementations to try to preserve git blame, deleting these implementations. Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 Cc: Fangrui Song <maskray@google.com> Reported-by: Prasad Sodagudi<psodagud@quicinc.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c index c94b820a1b62..20e6760ec05d 100644 --- a/kernel/gcov/clang.c +++ b/kernel/gcov/clang.c @@ -75,7 +75,9 @@ struct gcov_fn_info { u32 num_counters; u64 *counters; +#if __clang_major__ < 11 const char *function_name; +#endif }; static struct gcov_info *current_info; @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) } EXPORT_SYMBOL(llvm_gcov_init); +#if __clang_major__ < 11 void llvm_gcda_start_file(const char *orig_filename, const char version[4], u32 checksum) { @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], current_info->checksum = checksum; } EXPORT_SYMBOL(llvm_gcda_start_file); +#else +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) +{ + current_info->filename = orig_filename; + current_info->version = version; + current_info->checksum = checksum; +} +EXPORT_SYMBOL(llvm_gcda_start_file); +#endif +#if __clang_major__ < 11 void llvm_gcda_emit_function(u32 ident, const char *function_name, u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) { @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, list_add_tail(&info->head, ¤t_info->functions); } EXPORT_SYMBOL(llvm_gcda_emit_function); +#else +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, + u8 use_extra_checksum, u32 cfg_checksum) +{ + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); + + if (!info) + return; + + INIT_LIST_HEAD(&info->head); + info->ident = ident; + info->checksum = func_checksum; + info->use_extra_checksum = use_extra_checksum; + info->cfg_checksum = cfg_checksum; + list_add_tail(&info->head, ¤t_info->functions); +} +EXPORT_SYMBOL(llvm_gcda_emit_function); +#endif void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) { @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) } } +#if __clang_major__ < 11 static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) { size_t cv_size; /* counter values size */ @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) kfree(fn_dup); return NULL; } +#else +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) +{ + size_t cv_size; /* counter values size */ + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), + GFP_KERNEL); + if (!fn_dup) + return NULL; + INIT_LIST_HEAD(&fn_dup->head); + + cv_size = fn->num_counters * sizeof(fn->counters[0]); + fn_dup->counters = vmalloc(cv_size); + if (!fn_dup->counters) { + kfree(fn_dup); + return NULL; + } + + memcpy(fn_dup->counters, fn->counters, cv_size); + + return fn_dup; +} +#endif /** * gcov_info_dup - duplicate profiling data set @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) * gcov_info_free - release memory for profiling data set duplicate * @info: profiling data set duplicate to free */ +#if __clang_major__ < 11 void gcov_info_free(struct gcov_info *info) { struct gcov_fn_info *fn, *tmp; @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) kfree(info->filename); kfree(info); } +#else +void gcov_info_free(struct gcov_info *info) +{ + struct gcov_fn_info *fn, *tmp; + + list_for_each_entry_safe(fn, tmp, &info->functions, head) { + vfree(fn->counters); + list_del(&fn->head); + kfree(fn); + } + kfree(info->filename); + kfree(info); +} +#endif #define ITER_STRIDE PAGE_SIZE base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 19:21 [PATCH] gcov: fix clang-11+ support Nick Desaulniers @ 2021-03-12 19:58 ` Nathan Chancellor 2021-03-12 20:14 ` Nick Desaulniers 2021-03-12 20:25 ` [PATCH] gcov: fix clang-11+ support Fangrui Song 1 sibling, 1 reply; 17+ messages in thread From: Nathan Chancellor @ 2021-03-12 19:58 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, linux-kernel, clang-built-linux On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > LLVM changed the expected function signatures for llvm_gcda_start_file() > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > or newer may have noticed their kernels failing to boot due to a panic > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > the function signatures so calling these functions doesn't panic the > kernel. > > When we drop clang-10 support from the kernel, we should carefully > update the original implementations to try to preserve git blame, > deleting these implementations. > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > Cc: Fangrui Song <maskray@google.com> > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> I can reproduce the panic (as a boot hang) in QEMU before this patch and it is resolved after it so: Tested-by: Nathan Chancellor <nathan@kernel.org> However, the duplication hurts :( would it potentially be better to just do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? depends on CC_IS_GCC || CLANG_VERSION >= 110000? > --- > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > index c94b820a1b62..20e6760ec05d 100644 > --- a/kernel/gcov/clang.c > +++ b/kernel/gcov/clang.c > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > u32 num_counters; > u64 *counters; > +#if __clang_major__ < 11 > const char *function_name; > +#endif > }; > > static struct gcov_info *current_info; > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > } > EXPORT_SYMBOL(llvm_gcov_init); > > +#if __clang_major__ < 11 > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > u32 checksum) > { > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > current_info->checksum = checksum; > } > EXPORT_SYMBOL(llvm_gcda_start_file); > +#else > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > +{ > + current_info->filename = orig_filename; > + current_info->version = version; > + current_info->checksum = checksum; > +} > +EXPORT_SYMBOL(llvm_gcda_start_file); > +#endif > > +#if __clang_major__ < 11 > void llvm_gcda_emit_function(u32 ident, const char *function_name, > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > { > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > list_add_tail(&info->head, ¤t_info->functions); > } > EXPORT_SYMBOL(llvm_gcda_emit_function); > +#else > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > + u8 use_extra_checksum, u32 cfg_checksum) > +{ > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > + > + if (!info) > + return; > + > + INIT_LIST_HEAD(&info->head); > + info->ident = ident; > + info->checksum = func_checksum; > + info->use_extra_checksum = use_extra_checksum; > + info->cfg_checksum = cfg_checksum; > + list_add_tail(&info->head, ¤t_info->functions); > +} > +EXPORT_SYMBOL(llvm_gcda_emit_function); > +#endif > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > { > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > } > } > > +#if __clang_major__ < 11 > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > { > size_t cv_size; /* counter values size */ > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > kfree(fn_dup); > return NULL; > } > +#else > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > +{ > + size_t cv_size; /* counter values size */ > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > + GFP_KERNEL); > + if (!fn_dup) > + return NULL; > + INIT_LIST_HEAD(&fn_dup->head); > + > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > + fn_dup->counters = vmalloc(cv_size); > + if (!fn_dup->counters) { > + kfree(fn_dup); > + return NULL; > + } > + > + memcpy(fn_dup->counters, fn->counters, cv_size); > + > + return fn_dup; > +} > +#endif > > /** > * gcov_info_dup - duplicate profiling data set > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > * gcov_info_free - release memory for profiling data set duplicate > * @info: profiling data set duplicate to free > */ > +#if __clang_major__ < 11 > void gcov_info_free(struct gcov_info *info) > { > struct gcov_fn_info *fn, *tmp; > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > kfree(info->filename); > kfree(info); > } > +#else > +void gcov_info_free(struct gcov_info *info) > +{ > + struct gcov_fn_info *fn, *tmp; > + > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > + vfree(fn->counters); > + list_del(&fn->head); > + kfree(fn); > + } > + kfree(info->filename); > + kfree(info); > +} > +#endif > > #define ITER_STRIDE PAGE_SIZE > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > -- > 2.31.0.rc2.261.g7f71774620-goog > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 19:58 ` Nathan Chancellor @ 2021-03-12 20:14 ` Nick Desaulniers 2021-03-12 20:46 ` Nick Desaulniers 2021-03-12 20:51 ` Nathan Chancellor 0 siblings, 2 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 20:14 UTC (permalink / raw) To: Nathan Chancellor Cc: Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, LKML, clang-built-linux On Fri, Mar 12, 2021 at 11:58 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > > LLVM changed the expected function signatures for llvm_gcda_start_file() > > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > > or newer may have noticed their kernels failing to boot due to a panic > > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > > the function signatures so calling these functions doesn't panic the > > kernel. > > > > When we drop clang-10 support from the kernel, we should carefully > > update the original implementations to try to preserve git blame, > > deleting these implementations. > > > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > > Cc: Fangrui Song <maskray@google.com> > > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > I can reproduce the panic (as a boot hang) in QEMU before this patch and > it is resolved after it so: > > Tested-by: Nathan Chancellor <nathan@kernel.org> > > However, the duplication hurts :( would it potentially be better to just > do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? > > depends on CC_IS_GCC || CLANG_VERSION >= 110000? I'm not opposed, and value your input on the matter. Either way, this will need to be back ported to stable. Should we be concerned with users of stable's branches before we mandated clang-10 as the minimum supported version? commit 1f7a44f63e6c ("compiler-clang: add build check for clang 10.0.1") first landed in v5.10-rc1. Does not exist in v5.4.y. The diff you suggest is certainly easier to review to observe the differences, and I we don't have users of the latest Android or CrOS kernels using older clang, but I suspect there may be older kernel versions where if they try to upgrade their version of clang, GCOV support will regress for them. Though, I guess that's fine since either approach will fix this for them. I guess if they don't want to upgrade from clang-10 say for example, then this approach can be backported to stable. > > > --- > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > > index c94b820a1b62..20e6760ec05d 100644 > > --- a/kernel/gcov/clang.c > > +++ b/kernel/gcov/clang.c > > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > > > u32 num_counters; > > u64 *counters; > > +#if __clang_major__ < 11 > > const char *function_name; > > +#endif > > }; > > > > static struct gcov_info *current_info; > > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > > } > > EXPORT_SYMBOL(llvm_gcov_init); > > > > +#if __clang_major__ < 11 > > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > u32 checksum) > > { > > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > current_info->checksum = checksum; > > } > > EXPORT_SYMBOL(llvm_gcda_start_file); > > +#else > > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > > +{ > > + current_info->filename = orig_filename; > > + current_info->version = version; > > + current_info->checksum = checksum; > > +} > > +EXPORT_SYMBOL(llvm_gcda_start_file); > > +#endif > > > > +#if __clang_major__ < 11 > > void llvm_gcda_emit_function(u32 ident, const char *function_name, > > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > > { > > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > > list_add_tail(&info->head, ¤t_info->functions); > > } > > EXPORT_SYMBOL(llvm_gcda_emit_function); > > +#else > > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > > + u8 use_extra_checksum, u32 cfg_checksum) > > +{ > > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > > + > > + if (!info) > > + return; > > + > > + INIT_LIST_HEAD(&info->head); > > + info->ident = ident; > > + info->checksum = func_checksum; > > + info->use_extra_checksum = use_extra_checksum; > > + info->cfg_checksum = cfg_checksum; > > + list_add_tail(&info->head, ¤t_info->functions); > > +} > > +EXPORT_SYMBOL(llvm_gcda_emit_function); > > +#endif > > > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > > { > > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > > } > > } > > > > +#if __clang_major__ < 11 > > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > { > > size_t cv_size; /* counter values size */ > > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > kfree(fn_dup); > > return NULL; > > } > > +#else > > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > +{ > > + size_t cv_size; /* counter values size */ > > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > > + GFP_KERNEL); > > + if (!fn_dup) > > + return NULL; > > + INIT_LIST_HEAD(&fn_dup->head); > > + > > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > > + fn_dup->counters = vmalloc(cv_size); > > + if (!fn_dup->counters) { > > + kfree(fn_dup); > > + return NULL; > > + } > > + > > + memcpy(fn_dup->counters, fn->counters, cv_size); > > + > > + return fn_dup; > > +} > > +#endif > > > > /** > > * gcov_info_dup - duplicate profiling data set > > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > > * gcov_info_free - release memory for profiling data set duplicate > > * @info: profiling data set duplicate to free > > */ > > +#if __clang_major__ < 11 > > void gcov_info_free(struct gcov_info *info) > > { > > struct gcov_fn_info *fn, *tmp; > > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > > kfree(info->filename); > > kfree(info); > > } > > +#else > > +void gcov_info_free(struct gcov_info *info) > > +{ > > + struct gcov_fn_info *fn, *tmp; > > + > > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > > + vfree(fn->counters); > > + list_del(&fn->head); > > + kfree(fn); > > + } > > + kfree(info->filename); > > + kfree(info); > > +} > > +#endif > > > > #define ITER_STRIDE PAGE_SIZE > > > > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > > -- > > 2.31.0.rc2.261.g7f71774620-goog > > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 20:14 ` Nick Desaulniers @ 2021-03-12 20:46 ` Nick Desaulniers 2021-03-12 20:51 ` Nathan Chancellor 1 sibling, 0 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 20:46 UTC (permalink / raw) To: Nathan Chancellor Cc: Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, LKML, clang-built-linux On Fri, Mar 12, 2021 at 12:14 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Mar 12, 2021 at 11:58 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > > > LLVM changed the expected function signatures for llvm_gcda_start_file() > > > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > > > or newer may have noticed their kernels failing to boot due to a panic > > > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > > > the function signatures so calling these functions doesn't panic the > > > kernel. > > > > > > When we drop clang-10 support from the kernel, we should carefully > > > update the original implementations to try to preserve git blame, > > > deleting these implementations. > > > > > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > > > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > > > Cc: Fangrui Song <maskray@google.com> > > > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > I can reproduce the panic (as a boot hang) in QEMU before this patch and > > it is resolved after it so: > > > > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > > However, the duplication hurts :( would it potentially be better to just > > do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? > > > > depends on CC_IS_GCC || CLANG_VERSION >= 110000? > > I'm not opposed, and value your input on the matter. Either way, this > will need to be back ported to stable. Should we be concerned with > users of stable's branches before we mandated clang-10 as the minimum > supported version? > > commit 1f7a44f63e6c ("compiler-clang: add build check for clang 10.0.1") > > first landed in v5.10-rc1. Does not exist in v5.4.y. The diff you > suggest is certainly easier to review to observe the differences, and > I we don't have users of the latest Android or CrOS kernels using > older clang, but I suspect there may be older kernel versions where if > they try to upgrade their version of clang, GCOV support will regress > for them. Though, I guess that's fine since either approach will fix > this for them. I guess if they don't want to upgrade from clang-10 say > for example, then this approach can be backported to stable. Thinking more about this over lunch; what are your thoughts on a V2 that does this first, then what you suggest as a second patch on top, with the first tagged for inclusion into stable, but the second one not? Hopefully maintainers don't consider that too much churn? > > > > > > --- > > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 69 insertions(+) > > > > > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > > > index c94b820a1b62..20e6760ec05d 100644 > > > --- a/kernel/gcov/clang.c > > > +++ b/kernel/gcov/clang.c > > > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > > > > > u32 num_counters; > > > u64 *counters; > > > +#if __clang_major__ < 11 > > > const char *function_name; > > > +#endif > > > }; > > > > > > static struct gcov_info *current_info; > > > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > > > } > > > EXPORT_SYMBOL(llvm_gcov_init); > > > > > > +#if __clang_major__ < 11 > > > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > u32 checksum) > > > { > > > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > current_info->checksum = checksum; > > > } > > > EXPORT_SYMBOL(llvm_gcda_start_file); > > > +#else > > > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > > > +{ > > > + current_info->filename = orig_filename; > > > + current_info->version = version; > > > + current_info->checksum = checksum; > > > +} > > > +EXPORT_SYMBOL(llvm_gcda_start_file); > > > +#endif > > > > > > +#if __clang_major__ < 11 > > > void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > > > { > > > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > list_add_tail(&info->head, ¤t_info->functions); > > > } > > > EXPORT_SYMBOL(llvm_gcda_emit_function); > > > +#else > > > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > > > + u8 use_extra_checksum, u32 cfg_checksum) > > > +{ > > > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > > > + > > > + if (!info) > > > + return; > > > + > > > + INIT_LIST_HEAD(&info->head); > > > + info->ident = ident; > > > + info->checksum = func_checksum; > > > + info->use_extra_checksum = use_extra_checksum; > > > + info->cfg_checksum = cfg_checksum; > > > + list_add_tail(&info->head, ¤t_info->functions); > > > +} > > > +EXPORT_SYMBOL(llvm_gcda_emit_function); > > > +#endif > > > > > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > > > { > > > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > > > } > > > } > > > > > > +#if __clang_major__ < 11 > > > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > { > > > size_t cv_size; /* counter values size */ > > > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > kfree(fn_dup); > > > return NULL; > > > } > > > +#else > > > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > +{ > > > + size_t cv_size; /* counter values size */ > > > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > > > + GFP_KERNEL); > > > + if (!fn_dup) > > > + return NULL; > > > + INIT_LIST_HEAD(&fn_dup->head); > > > + > > > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > > > + fn_dup->counters = vmalloc(cv_size); > > > + if (!fn_dup->counters) { > > > + kfree(fn_dup); > > > + return NULL; > > > + } > > > + > > > + memcpy(fn_dup->counters, fn->counters, cv_size); > > > + > > > + return fn_dup; > > > +} > > > +#endif > > > > > > /** > > > * gcov_info_dup - duplicate profiling data set > > > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > > > * gcov_info_free - release memory for profiling data set duplicate > > > * @info: profiling data set duplicate to free > > > */ > > > +#if __clang_major__ < 11 > > > void gcov_info_free(struct gcov_info *info) > > > { > > > struct gcov_fn_info *fn, *tmp; > > > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > > > kfree(info->filename); > > > kfree(info); > > > } > > > +#else > > > +void gcov_info_free(struct gcov_info *info) > > > +{ > > > + struct gcov_fn_info *fn, *tmp; > > > + > > > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > > > + vfree(fn->counters); > > > + list_del(&fn->head); > > > + kfree(fn); > > > + } > > > + kfree(info->filename); > > > + kfree(info); > > > +} > > > +#endif > > > > > > #define ITER_STRIDE PAGE_SIZE > > > > > > > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > > > -- > > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 20:14 ` Nick Desaulniers 2021-03-12 20:46 ` Nick Desaulniers @ 2021-03-12 20:51 ` Nathan Chancellor 2021-03-12 21:57 ` Nick Desaulniers 1 sibling, 1 reply; 17+ messages in thread From: Nathan Chancellor @ 2021-03-12 20:51 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, LKML, clang-built-linux On Fri, Mar 12, 2021 at 12:14:42PM -0800, Nick Desaulniers wrote: > On Fri, Mar 12, 2021 at 11:58 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > > > LLVM changed the expected function signatures for llvm_gcda_start_file() > > > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > > > or newer may have noticed their kernels failing to boot due to a panic > > > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > > > the function signatures so calling these functions doesn't panic the > > > kernel. > > > > > > When we drop clang-10 support from the kernel, we should carefully > > > update the original implementations to try to preserve git blame, > > > deleting these implementations. > > > > > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > > > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > > > Cc: Fangrui Song <maskray@google.com> > > > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > I can reproduce the panic (as a boot hang) in QEMU before this patch and > > it is resolved after it so: > > > > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > > However, the duplication hurts :( would it potentially be better to just > > do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? > > > > depends on CC_IS_GCC || CLANG_VERSION >= 110000? > > I'm not opposed, and value your input on the matter. Either way, this > will need to be back ported to stable. Should we be concerned with > users of stable's branches before we mandated clang-10 as the minimum > supported version? > > commit 1f7a44f63e6c ("compiler-clang: add build check for clang 10.0.1") > > first landed in v5.10-rc1. Does not exist in v5.4.y. The diff you Hmmm fair point, I did not realize that this support had landed in 5.2 meaning that 5.4 needs it as well at 5.10. > suggest is certainly easier to review to observe the differences, and > I we don't have users of the latest Android or CrOS kernels using > older clang, but I suspect there may be older kernel versions where if > they try to upgrade their version of clang, GCOV support will regress > for them. Though, I guess that's fine since either approach will fix > this for them. I guess if they don't want to upgrade from clang-10 say > for example, then this approach can be backported to stable. If people are happy with this approach, it is the more "stable friendly" change because it fixes it for all versions of clang that should have been supported at their respective times. Maybe it is worthwhile to do both? This change gets picked up with a Cc: stable@vger.kernel.org then in a follow up patch, we remove the #ifdef's and gate GCOV on clang-11? The CLANG_VERSION string is usually what we will search for when removing old workarounds. Additionally, your patch could just use #if CLANG_VERSION <= 110000 to more easily see this. I have no strong opinion one way or the other though. If people are happy with this approach, let's do it. Cheers, Nathan > > > > > --- > > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 69 insertions(+) > > > > > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > > > index c94b820a1b62..20e6760ec05d 100644 > > > --- a/kernel/gcov/clang.c > > > +++ b/kernel/gcov/clang.c > > > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > > > > > u32 num_counters; > > > u64 *counters; > > > +#if __clang_major__ < 11 > > > const char *function_name; > > > +#endif > > > }; > > > > > > static struct gcov_info *current_info; > > > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > > > } > > > EXPORT_SYMBOL(llvm_gcov_init); > > > > > > +#if __clang_major__ < 11 > > > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > u32 checksum) > > > { > > > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > current_info->checksum = checksum; > > > } > > > EXPORT_SYMBOL(llvm_gcda_start_file); > > > +#else > > > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > > > +{ > > > + current_info->filename = orig_filename; > > > + current_info->version = version; > > > + current_info->checksum = checksum; > > > +} > > > +EXPORT_SYMBOL(llvm_gcda_start_file); > > > +#endif > > > > > > +#if __clang_major__ < 11 > > > void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > > > { > > > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > list_add_tail(&info->head, ¤t_info->functions); > > > } > > > EXPORT_SYMBOL(llvm_gcda_emit_function); > > > +#else > > > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > > > + u8 use_extra_checksum, u32 cfg_checksum) > > > +{ > > > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > > > + > > > + if (!info) > > > + return; > > > + > > > + INIT_LIST_HEAD(&info->head); > > > + info->ident = ident; > > > + info->checksum = func_checksum; > > > + info->use_extra_checksum = use_extra_checksum; > > > + info->cfg_checksum = cfg_checksum; > > > + list_add_tail(&info->head, ¤t_info->functions); > > > +} > > > +EXPORT_SYMBOL(llvm_gcda_emit_function); > > > +#endif > > > > > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > > > { > > > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > > > } > > > } > > > > > > +#if __clang_major__ < 11 > > > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > { > > > size_t cv_size; /* counter values size */ > > > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > kfree(fn_dup); > > > return NULL; > > > } > > > +#else > > > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > +{ > > > + size_t cv_size; /* counter values size */ > > > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > > > + GFP_KERNEL); > > > + if (!fn_dup) > > > + return NULL; > > > + INIT_LIST_HEAD(&fn_dup->head); > > > + > > > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > > > + fn_dup->counters = vmalloc(cv_size); > > > + if (!fn_dup->counters) { > > > + kfree(fn_dup); > > > + return NULL; > > > + } > > > + > > > + memcpy(fn_dup->counters, fn->counters, cv_size); > > > + > > > + return fn_dup; > > > +} > > > +#endif > > > > > > /** > > > * gcov_info_dup - duplicate profiling data set > > > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > > > * gcov_info_free - release memory for profiling data set duplicate > > > * @info: profiling data set duplicate to free > > > */ > > > +#if __clang_major__ < 11 > > > void gcov_info_free(struct gcov_info *info) > > > { > > > struct gcov_fn_info *fn, *tmp; > > > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > > > kfree(info->filename); > > > kfree(info); > > > } > > > +#else > > > +void gcov_info_free(struct gcov_info *info) > > > +{ > > > + struct gcov_fn_info *fn, *tmp; > > > + > > > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > > > + vfree(fn->counters); > > > + list_del(&fn->head); > > > + kfree(fn); > > > + } > > > + kfree(info->filename); > > > + kfree(info); > > > +} > > > +#endif > > > > > > #define ITER_STRIDE PAGE_SIZE > > > > > > > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > > > -- > > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 20:51 ` Nathan Chancellor @ 2021-03-12 21:57 ` Nick Desaulniers 2021-03-12 22:05 ` Nathan Chancellor 0 siblings, 1 reply; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 21:57 UTC (permalink / raw) To: Nathan Chancellor, Masahiro Yamada, Miguel Ojeda Cc: Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, LKML, clang-built-linux On Fri, Mar 12, 2021 at 12:51 PM Nathan Chancellor <nathan@kernel.org> wrote: > > On Fri, Mar 12, 2021 at 12:14:42PM -0800, Nick Desaulniers wrote: > > On Fri, Mar 12, 2021 at 11:58 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > > > > LLVM changed the expected function signatures for llvm_gcda_start_file() > > > > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > > > > or newer may have noticed their kernels failing to boot due to a panic > > > > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > > > > the function signatures so calling these functions doesn't panic the > > > > kernel. > > > > > > > > When we drop clang-10 support from the kernel, we should carefully > > > > update the original implementations to try to preserve git blame, > > > > deleting these implementations. > > > > > > > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > > > > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > > > > Cc: Fangrui Song <maskray@google.com> > > > > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > I can reproduce the panic (as a boot hang) in QEMU before this patch and > > > it is resolved after it so: > > > > > > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > > > > However, the duplication hurts :( would it potentially be better to just > > > do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? > > > > > > depends on CC_IS_GCC || CLANG_VERSION >= 110000? > > > > I'm not opposed, and value your input on the matter. Either way, this > > will need to be back ported to stable. Should we be concerned with > > users of stable's branches before we mandated clang-10 as the minimum > > supported version? > > > > commit 1f7a44f63e6c ("compiler-clang: add build check for clang 10.0.1") > > > > first landed in v5.10-rc1. Does not exist in v5.4.y. The diff you > > Hmmm fair point, I did not realize that this support had landed in 5.2 > meaning that 5.4 needs it as well at 5.10. > > > suggest is certainly easier to review to observe the differences, and > > I we don't have users of the latest Android or CrOS kernels using > > older clang, but I suspect there may be older kernel versions where if > > they try to upgrade their version of clang, GCOV support will regress > > for them. Though, I guess that's fine since either approach will fix > > this for them. I guess if they don't want to upgrade from clang-10 say > > for example, then this approach can be backported to stable. > > If people are happy with this approach, it is the more "stable friendly" > change because it fixes it for all versions of clang that should have > been supported at their respective times. Maybe it is worthwhile to do > both? This change gets picked up with a Cc: stable@vger.kernel.org then > in a follow up patch, we remove the #ifdef's and gate GCOV on clang-11? > The CLANG_VERSION string is usually what we will search for when > removing old workarounds. Sounds like we're on the same page; will send a v2 with your recommendation on top. > Additionally, your patch could just use > > #if CLANG_VERSION <= 110000 > > to more easily see this. I have no strong opinion one way or the other > though. If people are happy with this approach, let's do it. Err that would be nicer, but: kernel/gcov/clang.c:78:5: warning: 'CLANG_VERSION' is not defined, evaluates to 0 [-Wundef] #if CLANG_VERSION < 110000 ^ kernel/gcov/clang.c:110:5: warning: 'CLANG_VERSION' is not defined, evaluates to 0 [-Wundef] #if CLANG_VERSION < 110000 ^ kernel/gcov/clang.c:130:5: warning: 'CLANG_VERSION' is not defined, evaluates to 0 [-Wundef] #if CLANG_VERSION < 110000 ^ kernel/gcov/clang.c:330:5: warning: 'CLANG_VERSION' is not defined, evaluates to 0 [-Wundef] #if CLANG_VERSION < 110000 ^ kernel/gcov/clang.c:420:5: warning: 'CLANG_VERSION' is not defined, evaluates to 0 [-Wundef] #if CLANG_VERSION < 110000 ^ Did we just break this in commit aec6c60a01d3 ("kbuild: check the minimum compiler version in Kconfig") in v5.12-rc1? So I'll keep it as is for v2, but we should discuss with Masahiro and Miguel if we should be removing CLANG_VERSION even if there are no in tree users at the moment. (I guess I could re-introduce it in my series for v2, but that will unnecessarily complicate the backports, so I won't). My fault for not catching that in code review. > > Cheers, > Nathan > > > > > > > > --- > > > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 69 insertions(+) > > > > > > > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > > > > index c94b820a1b62..20e6760ec05d 100644 > > > > --- a/kernel/gcov/clang.c > > > > +++ b/kernel/gcov/clang.c > > > > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > > > > > > > u32 num_counters; > > > > u64 *counters; > > > > +#if __clang_major__ < 11 > > > > const char *function_name; > > > > +#endif > > > > }; > > > > > > > > static struct gcov_info *current_info; > > > > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > > > > } > > > > EXPORT_SYMBOL(llvm_gcov_init); > > > > > > > > +#if __clang_major__ < 11 > > > > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > > u32 checksum) > > > > { > > > > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > > current_info->checksum = checksum; > > > > } > > > > EXPORT_SYMBOL(llvm_gcda_start_file); > > > > +#else > > > > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > > > > +{ > > > > + current_info->filename = orig_filename; > > > > + current_info->version = version; > > > > + current_info->checksum = checksum; > > > > +} > > > > +EXPORT_SYMBOL(llvm_gcda_start_file); > > > > +#endif > > > > > > > > +#if __clang_major__ < 11 > > > > void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > > > > { > > > > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > > list_add_tail(&info->head, ¤t_info->functions); > > > > } > > > > EXPORT_SYMBOL(llvm_gcda_emit_function); > > > > +#else > > > > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > > > > + u8 use_extra_checksum, u32 cfg_checksum) > > > > +{ > > > > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > > > > + > > > > + if (!info) > > > > + return; > > > > + > > > > + INIT_LIST_HEAD(&info->head); > > > > + info->ident = ident; > > > > + info->checksum = func_checksum; > > > > + info->use_extra_checksum = use_extra_checksum; > > > > + info->cfg_checksum = cfg_checksum; > > > > + list_add_tail(&info->head, ¤t_info->functions); > > > > +} > > > > +EXPORT_SYMBOL(llvm_gcda_emit_function); > > > > +#endif > > > > > > > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > > > > { > > > > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > > > > } > > > > } > > > > > > > > +#if __clang_major__ < 11 > > > > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > { > > > > size_t cv_size; /* counter values size */ > > > > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > kfree(fn_dup); > > > > return NULL; > > > > } > > > > +#else > > > > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > +{ > > > > + size_t cv_size; /* counter values size */ > > > > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > > > > + GFP_KERNEL); > > > > + if (!fn_dup) > > > > + return NULL; > > > > + INIT_LIST_HEAD(&fn_dup->head); > > > > + > > > > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > > > > + fn_dup->counters = vmalloc(cv_size); > > > > + if (!fn_dup->counters) { > > > > + kfree(fn_dup); > > > > + return NULL; > > > > + } > > > > + > > > > + memcpy(fn_dup->counters, fn->counters, cv_size); > > > > + > > > > + return fn_dup; > > > > +} > > > > +#endif > > > > > > > > /** > > > > * gcov_info_dup - duplicate profiling data set > > > > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > > > > * gcov_info_free - release memory for profiling data set duplicate > > > > * @info: profiling data set duplicate to free > > > > */ > > > > +#if __clang_major__ < 11 > > > > void gcov_info_free(struct gcov_info *info) > > > > { > > > > struct gcov_fn_info *fn, *tmp; > > > > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > > > > kfree(info->filename); > > > > kfree(info); > > > > } > > > > +#else > > > > +void gcov_info_free(struct gcov_info *info) > > > > +{ > > > > + struct gcov_fn_info *fn, *tmp; > > > > + > > > > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > > > > + vfree(fn->counters); > > > > + list_del(&fn->head); > > > > + kfree(fn); > > > > + } > > > > + kfree(info->filename); > > > > + kfree(info); > > > > +} > > > > +#endif > > > > > > > > #define ITER_STRIDE PAGE_SIZE > > > > > > > > > > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > > > > -- > > > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 21:57 ` Nick Desaulniers @ 2021-03-12 22:05 ` Nathan Chancellor 2021-03-12 22:41 ` [PATCH v2 0/2] gcov fixes for clang-11 Nick Desaulniers 0 siblings, 1 reply; 17+ messages in thread From: Nathan Chancellor @ 2021-03-12 22:05 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Miguel Ojeda, Peter Oberparleiter, Andrew Morton, Fangrui Song, Prasad Sodagudi, LKML, clang-built-linux On Fri, Mar 12, 2021 at 01:57:47PM -0800, 'Nick Desaulniers' via Clang Built Linux wrote: > On Fri, Mar 12, 2021 at 12:51 PM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Fri, Mar 12, 2021 at 12:14:42PM -0800, Nick Desaulniers wrote: > > > On Fri, Mar 12, 2021 at 11:58 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > > > On Fri, Mar 12, 2021 at 11:21:39AM -0800, Nick Desaulniers wrote: > > > > > LLVM changed the expected function signatures for llvm_gcda_start_file() > > > > > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > > > > > or newer may have noticed their kernels failing to boot due to a panic > > > > > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > > > > > the function signatures so calling these functions doesn't panic the > > > > > kernel. > > > > > > > > > > When we drop clang-10 support from the kernel, we should carefully > > > > > update the original implementations to try to preserve git blame, > > > > > deleting these implementations. > > > > > > > > > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > > > > > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > > > > > Cc: Fangrui Song <maskray@google.com> > > > > > Reported-by: Prasad Sodagudi<psodagud@quicinc.com> > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > > I can reproduce the panic (as a boot hang) in QEMU before this patch and > > > > it is resolved after it so: > > > > > > > > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > > > > > > However, the duplication hurts :( would it potentially be better to just > > > > do the full update to clang-11+ and require it for CONFIG_GCOV_KERNEL? > > > > > > > > depends on CC_IS_GCC || CLANG_VERSION >= 110000? > > > > > > I'm not opposed, and value your input on the matter. Either way, this > > > will need to be back ported to stable. Should we be concerned with > > > users of stable's branches before we mandated clang-10 as the minimum > > > supported version? > > > > > > commit 1f7a44f63e6c ("compiler-clang: add build check for clang 10.0.1") > > > > > > first landed in v5.10-rc1. Does not exist in v5.4.y. The diff you > > > > Hmmm fair point, I did not realize that this support had landed in 5.2 > > meaning that 5.4 needs it as well at 5.10. > > > > > suggest is certainly easier to review to observe the differences, and > > > I we don't have users of the latest Android or CrOS kernels using > > > older clang, but I suspect there may be older kernel versions where if > > > they try to upgrade their version of clang, GCOV support will regress > > > for them. Though, I guess that's fine since either approach will fix > > > this for them. I guess if they don't want to upgrade from clang-10 say > > > for example, then this approach can be backported to stable. > > > > If people are happy with this approach, it is the more "stable friendly" > > change because it fixes it for all versions of clang that should have > > been supported at their respective times. Maybe it is worthwhile to do > > both? This change gets picked up with a Cc: stable@vger.kernel.org then > > in a follow up patch, we remove the #ifdef's and gate GCOV on clang-11? > > The CLANG_VERSION string is usually what we will search for when > > removing old workarounds. > > Sounds like we're on the same page; will send a v2 with your > recommendation on top. > > > Additionally, your patch could just use > > > > #if CLANG_VERSION <= 110000 > > > > to more easily see this. I have no strong opinion one way or the other > > though. If people are happy with this approach, let's do it. > > Err that would be nicer, but: > kernel/gcov/clang.c:78:5: warning: 'CLANG_VERSION' is not defined, > evaluates to 0 [-Wundef] > #if CLANG_VERSION < 110000 > ^ > kernel/gcov/clang.c:110:5: warning: 'CLANG_VERSION' is not defined, > evaluates to 0 [-Wundef] > #if CLANG_VERSION < 110000 > ^ > kernel/gcov/clang.c:130:5: warning: 'CLANG_VERSION' is not defined, > evaluates to 0 [-Wundef] > #if CLANG_VERSION < 110000 > ^ > kernel/gcov/clang.c:330:5: warning: 'CLANG_VERSION' is not defined, > evaluates to 0 [-Wundef] > #if CLANG_VERSION < 110000 > ^ > kernel/gcov/clang.c:420:5: warning: 'CLANG_VERSION' is not defined, > evaluates to 0 [-Wundef] > #if CLANG_VERSION < 110000 > ^ Ah sorry, CONFIG_CLANG_VERSION. > Did we just break this in commit aec6c60a01d3 ("kbuild: check the > minimum compiler version in Kconfig") in v5.12-rc1? So I'll keep it > as is for v2, but we should discuss with Masahiro and Miguel if we > should be removing CLANG_VERSION even if there are no in tree users at > the moment. (I guess I could re-introduce it in my series for v2, but > that will unnecessarily complicate the backports, so I won't). My > fault for not catching that in code review. Technically yes, but the {CLANG,GCC}_VERSION macros are not portable because they are only defined in their respective headers, resulting in problems like commit df3da04880b4 ("mips: Fix unroll macro when building with Clang"). Cheers, Nathan > > > > Cheers, > > Nathan > > > > > > > > > > > --- > > > > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 69 insertions(+) > > > > > > > > > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > > > > > index c94b820a1b62..20e6760ec05d 100644 > > > > > --- a/kernel/gcov/clang.c > > > > > +++ b/kernel/gcov/clang.c > > > > > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > > > > > > > > > u32 num_counters; > > > > > u64 *counters; > > > > > +#if __clang_major__ < 11 > > > > > const char *function_name; > > > > > +#endif > > > > > }; > > > > > > > > > > static struct gcov_info *current_info; > > > > > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > > > > > } > > > > > EXPORT_SYMBOL(llvm_gcov_init); > > > > > > > > > > +#if __clang_major__ < 11 > > > > > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > > > u32 checksum) > > > > > { > > > > > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > > > > > current_info->checksum = checksum; > > > > > } > > > > > EXPORT_SYMBOL(llvm_gcda_start_file); > > > > > +#else > > > > > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > > > > > +{ > > > > > + current_info->filename = orig_filename; > > > > > + current_info->version = version; > > > > > + current_info->checksum = checksum; > > > > > +} > > > > > +EXPORT_SYMBOL(llvm_gcda_start_file); > > > > > +#endif > > > > > > > > > > +#if __clang_major__ < 11 > > > > > void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > > > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > > > > > { > > > > > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > > > > > list_add_tail(&info->head, ¤t_info->functions); > > > > > } > > > > > EXPORT_SYMBOL(llvm_gcda_emit_function); > > > > > +#else > > > > > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > > > > > + u8 use_extra_checksum, u32 cfg_checksum) > > > > > +{ > > > > > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > > > > > + > > > > > + if (!info) > > > > > + return; > > > > > + > > > > > + INIT_LIST_HEAD(&info->head); > > > > > + info->ident = ident; > > > > > + info->checksum = func_checksum; > > > > > + info->use_extra_checksum = use_extra_checksum; > > > > > + info->cfg_checksum = cfg_checksum; > > > > > + list_add_tail(&info->head, ¤t_info->functions); > > > > > +} > > > > > +EXPORT_SYMBOL(llvm_gcda_emit_function); > > > > > +#endif > > > > > > > > > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > > > > > { > > > > > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > > > > > } > > > > > } > > > > > > > > > > +#if __clang_major__ < 11 > > > > > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > > { > > > > > size_t cv_size; /* counter values size */ > > > > > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > > kfree(fn_dup); > > > > > return NULL; > > > > > } > > > > > +#else > > > > > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > > > > > +{ > > > > > + size_t cv_size; /* counter values size */ > > > > > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > > > > > + GFP_KERNEL); > > > > > + if (!fn_dup) > > > > > + return NULL; > > > > > + INIT_LIST_HEAD(&fn_dup->head); > > > > > + > > > > > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > > > > > + fn_dup->counters = vmalloc(cv_size); > > > > > + if (!fn_dup->counters) { > > > > > + kfree(fn_dup); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + memcpy(fn_dup->counters, fn->counters, cv_size); > > > > > + > > > > > + return fn_dup; > > > > > +} > > > > > +#endif > > > > > > > > > > /** > > > > > * gcov_info_dup - duplicate profiling data set > > > > > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > > > > > * gcov_info_free - release memory for profiling data set duplicate > > > > > * @info: profiling data set duplicate to free > > > > > */ > > > > > +#if __clang_major__ < 11 > > > > > void gcov_info_free(struct gcov_info *info) > > > > > { > > > > > struct gcov_fn_info *fn, *tmp; > > > > > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > > > > > kfree(info->filename); > > > > > kfree(info); > > > > > } > > > > > +#else > > > > > +void gcov_info_free(struct gcov_info *info) > > > > > +{ > > > > > + struct gcov_fn_info *fn, *tmp; > > > > > + > > > > > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > > > > > + vfree(fn->counters); > > > > > + list_del(&fn->head); > > > > > + kfree(fn); > > > > > + } > > > > > + kfree(info->filename); > > > > > + kfree(info); > > > > > +} > > > > > +#endif > > > > > > > > > > #define ITER_STRIDE PAGE_SIZE > > > > > > > > > > > > > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > > > > > -- > > > > > 2.31.0.rc2.261.g7f71774620-goog > > > > > > > > > > > > > > > > > -- > > > Thanks, > > > ~Nick Desaulniers > > > > -- > Thanks, > ~Nick Desaulniers > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdmV5co%2BmMSBbnnXyBXiwOha%3DS987PMA68Xe9jP8gJYkdw%40mail.gmail.com. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/2] gcov fixes for clang-11 2021-03-12 22:05 ` Nathan Chancellor @ 2021-03-12 22:41 ` Nick Desaulniers 2021-03-12 22:41 ` [PATCH v2 1/2] gcov: fix clang-11+ support Nick Desaulniers 2021-03-12 22:41 ` [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older Nick Desaulniers 0 siblings, 2 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 22:41 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: Nathan Chancellor, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi, Nick Desaulniers LLVM changed the expected function signatures for llvm_gcda_start_file() and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 or newer may have noticed their kernels failing to boot due to a panic when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up the function signatures so calling these functions doesn't panic the kernel. The first patch should allow us to backport it to stable; the second drops support for older toolchains. Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 Nick Desaulniers (2): gcov: fix clang-11+ support gcov: clang: drop support for clang-10 and older kernel/gcov/Kconfig | 1 + kernel/gcov/clang.c | 32 ++++++++------------------------ 2 files changed, 9 insertions(+), 24 deletions(-) base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] gcov: fix clang-11+ support 2021-03-12 22:41 ` [PATCH v2 0/2] gcov fixes for clang-11 Nick Desaulniers @ 2021-03-12 22:41 ` Nick Desaulniers 2021-03-15 13:51 ` Peter Oberparleiter 2021-03-15 18:13 ` Nathan Chancellor 2021-03-12 22:41 ` [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older Nick Desaulniers 1 sibling, 2 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 22:41 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: Nathan Chancellor, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi, Nick Desaulniers, stable LLVM changed the expected function signatures for llvm_gcda_start_file() and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 or newer may have noticed their kernels failing to boot due to a panic when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up the function signatures so calling these functions doesn't panic the kernel. Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 Cc: stable@vger.kernel.org # 5.4 Reported-by: Prasad Sodagudi <psodagud@quicinc.com> Suggested-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Fangrui Song <maskray@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nathan Chancellor <nathan@kernel.org> --- Changes V1 -> V2: * Use CONFIG_CLANG_VERSION instead of __clang_major__. * Pick up and retain Suggested-by, Tested-by, and Reviewed-by tags. * Drop note from commit message about `git blame`; I did what was sugguested in V1, but it still looks to git like I wrote those functions. Oh well. kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c index c94b820a1b62..8743150db2ac 100644 --- a/kernel/gcov/clang.c +++ b/kernel/gcov/clang.c @@ -75,7 +75,9 @@ struct gcov_fn_info { u32 num_counters; u64 *counters; +#if CONFIG_CLANG_VERSION < 110000 const char *function_name; +#endif }; static struct gcov_info *current_info; @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) } EXPORT_SYMBOL(llvm_gcov_init); +#if CONFIG_CLANG_VERSION < 110000 void llvm_gcda_start_file(const char *orig_filename, const char version[4], u32 checksum) { @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], current_info->checksum = checksum; } EXPORT_SYMBOL(llvm_gcda_start_file); +#else +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) +{ + current_info->filename = orig_filename; + current_info->version = version; + current_info->checksum = checksum; +} +EXPORT_SYMBOL(llvm_gcda_start_file); +#endif +#if CONFIG_CLANG_VERSION < 110000 void llvm_gcda_emit_function(u32 ident, const char *function_name, u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) { @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, list_add_tail(&info->head, ¤t_info->functions); } EXPORT_SYMBOL(llvm_gcda_emit_function); +#else +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, + u8 use_extra_checksum, u32 cfg_checksum) +{ + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); + + if (!info) + return; + + INIT_LIST_HEAD(&info->head); + info->ident = ident; + info->checksum = func_checksum; + info->use_extra_checksum = use_extra_checksum; + info->cfg_checksum = cfg_checksum; + list_add_tail(&info->head, ¤t_info->functions); +} +EXPORT_SYMBOL(llvm_gcda_emit_function); +#endif void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) { @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) } } +#if CONFIG_CLANG_VERSION < 110000 static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) { size_t cv_size; /* counter values size */ @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) kfree(fn_dup); return NULL; } +#else +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) +{ + size_t cv_size; /* counter values size */ + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), + GFP_KERNEL); + if (!fn_dup) + return NULL; + INIT_LIST_HEAD(&fn_dup->head); + + cv_size = fn->num_counters * sizeof(fn->counters[0]); + fn_dup->counters = vmalloc(cv_size); + if (!fn_dup->counters) { + kfree(fn_dup); + return NULL; + } + + memcpy(fn_dup->counters, fn->counters, cv_size); + + return fn_dup; +} +#endif /** * gcov_info_dup - duplicate profiling data set @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) * gcov_info_free - release memory for profiling data set duplicate * @info: profiling data set duplicate to free */ +#if CONFIG_CLANG_VERSION < 110000 void gcov_info_free(struct gcov_info *info) { struct gcov_fn_info *fn, *tmp; @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) kfree(info->filename); kfree(info); } +#else +void gcov_info_free(struct gcov_info *info) +{ + struct gcov_fn_info *fn, *tmp; + + list_for_each_entry_safe(fn, tmp, &info->functions, head) { + vfree(fn->counters); + list_del(&fn->head); + kfree(fn); + } + kfree(info->filename); + kfree(info); +} +#endif #define ITER_STRIDE PAGE_SIZE base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gcov: fix clang-11+ support 2021-03-12 22:41 ` [PATCH v2 1/2] gcov: fix clang-11+ support Nick Desaulniers @ 2021-03-15 13:51 ` Peter Oberparleiter 2021-03-15 18:13 ` Nathan Chancellor 1 sibling, 0 replies; 17+ messages in thread From: Peter Oberparleiter @ 2021-03-15 13:51 UTC (permalink / raw) To: Nick Desaulniers, Andrew Morton Cc: Nathan Chancellor, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi, stable On 12.03.2021 23:41, Nick Desaulniers wrote: > LLVM changed the expected function signatures for llvm_gcda_start_file() > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > or newer may have noticed their kernels failing to boot due to a panic > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > the function signatures so calling these functions doesn't panic the > kernel. > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > Cc: stable@vger.kernel.org # 5.4 > Reported-by: Prasad Sodagudi <psodagud@quicinc.com> > Suggested-by: Nathan Chancellor <nathan@kernel.org> > Reviewed-by: Fangrui Song <maskray@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > Tested-by: Nathan Chancellor <nathan@kernel.org> Looks good to me (minus the code duplication - but that's IMO acceptable since it's cleaned up again with patch 2). Acked-by: Peter Oberparleiter <oberpar@linux.ibm.com> That said, I'm currently thinking of adding a compile time check that performs a dry-run gcov_info => gcda conversion in user space to detect these kind of issues before kernels fail unpredictably [1]. I'm confident that this could work for the GCC gcov kernel code, not sure about the Clang version though. But if it's possible I guess it would make sense to extend this to include the Clang code as well. Note that this check wouldn't work for cross-compiles since the build machine must be able to run code for the target machine. [1] https://lore.kernel.org/lkml/1c7a49e7-0e27-561b-a2f9-d42a83dc4c29@linux.ibm.com/ Regards, Peter -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] gcov: fix clang-11+ support 2021-03-12 22:41 ` [PATCH v2 1/2] gcov: fix clang-11+ support Nick Desaulniers 2021-03-15 13:51 ` Peter Oberparleiter @ 2021-03-15 18:13 ` Nathan Chancellor 1 sibling, 0 replies; 17+ messages in thread From: Nathan Chancellor @ 2021-03-15 18:13 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi, stable On Fri, Mar 12, 2021 at 02:41:31PM -0800, Nick Desaulniers wrote: > LLVM changed the expected function signatures for llvm_gcda_start_file() > and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 > or newer may have noticed their kernels failing to boot due to a panic > when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up > the function signatures so calling these functions doesn't panic the > kernel. > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > Cc: stable@vger.kernel.org # 5.4 > Reported-by: Prasad Sodagudi <psodagud@quicinc.com> > Suggested-by: Nathan Chancellor <nathan@kernel.org> > Reviewed-by: Fangrui Song <maskray@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > Tested-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes V1 -> V2: > * Use CONFIG_CLANG_VERSION instead of __clang_major__. > * Pick up and retain Suggested-by, Tested-by, and Reviewed-by tags. > * Drop note from commit message about `git blame`; I did what was > sugguested in V1, but it still looks to git like I wrote those > functions. Oh well. > > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > index c94b820a1b62..8743150db2ac 100644 > --- a/kernel/gcov/clang.c > +++ b/kernel/gcov/clang.c > @@ -75,7 +75,9 @@ struct gcov_fn_info { > > u32 num_counters; > u64 *counters; > +#if CONFIG_CLANG_VERSION < 110000 > const char *function_name; > +#endif > }; > > static struct gcov_info *current_info; > @@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > } > EXPORT_SYMBOL(llvm_gcov_init); > > +#if CONFIG_CLANG_VERSION < 110000 > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > u32 checksum) > { > @@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > current_info->checksum = checksum; > } > EXPORT_SYMBOL(llvm_gcda_start_file); > +#else > +void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > +{ > + current_info->filename = orig_filename; > + current_info->version = version; > + current_info->checksum = checksum; > +} > +EXPORT_SYMBOL(llvm_gcda_start_file); > +#endif > > +#if CONFIG_CLANG_VERSION < 110000 > void llvm_gcda_emit_function(u32 ident, const char *function_name, > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > { > @@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > list_add_tail(&info->head, ¤t_info->functions); > } > EXPORT_SYMBOL(llvm_gcda_emit_function); > +#else > +void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > + u8 use_extra_checksum, u32 cfg_checksum) > +{ > + struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > + > + if (!info) > + return; > + > + INIT_LIST_HEAD(&info->head); > + info->ident = ident; > + info->checksum = func_checksum; > + info->use_extra_checksum = use_extra_checksum; > + info->cfg_checksum = cfg_checksum; > + list_add_tail(&info->head, ¤t_info->functions); > +} > +EXPORT_SYMBOL(llvm_gcda_emit_function); > +#endif > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > { > @@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > } > } > > +#if CONFIG_CLANG_VERSION < 110000 > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > { > size_t cv_size; /* counter values size */ > @@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > kfree(fn_dup); > return NULL; > } > +#else > +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > +{ > + size_t cv_size; /* counter values size */ > + struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > + GFP_KERNEL); > + if (!fn_dup) > + return NULL; > + INIT_LIST_HEAD(&fn_dup->head); > + > + cv_size = fn->num_counters * sizeof(fn->counters[0]); > + fn_dup->counters = vmalloc(cv_size); > + if (!fn_dup->counters) { > + kfree(fn_dup); > + return NULL; > + } > + > + memcpy(fn_dup->counters, fn->counters, cv_size); > + > + return fn_dup; > +} > +#endif > > /** > * gcov_info_dup - duplicate profiling data set > @@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > * gcov_info_free - release memory for profiling data set duplicate > * @info: profiling data set duplicate to free > */ > +#if CONFIG_CLANG_VERSION < 110000 > void gcov_info_free(struct gcov_info *info) > { > struct gcov_fn_info *fn, *tmp; > @@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > kfree(info->filename); > kfree(info); > } > +#else > +void gcov_info_free(struct gcov_info *info) > +{ > + struct gcov_fn_info *fn, *tmp; > + > + list_for_each_entry_safe(fn, tmp, &info->functions, head) { > + vfree(fn->counters); > + list_del(&fn->head); > + kfree(fn); > + } > + kfree(info->filename); > + kfree(info); > +} > +#endif > > #define ITER_STRIDE PAGE_SIZE > > > base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 > -- > 2.31.0.rc2.261.g7f71774620-goog > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older 2021-03-12 22:41 ` [PATCH v2 0/2] gcov fixes for clang-11 Nick Desaulniers 2021-03-12 22:41 ` [PATCH v2 1/2] gcov: fix clang-11+ support Nick Desaulniers @ 2021-03-12 22:41 ` Nick Desaulniers 2021-03-15 13:54 ` Peter Oberparleiter 2021-03-15 18:14 ` Nathan Chancellor 1 sibling, 2 replies; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 22:41 UTC (permalink / raw) To: Peter Oberparleiter, Andrew Morton Cc: Nathan Chancellor, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi, Nick Desaulniers LLVM changed the expected function signatures for llvm_gcda_start_file() and llvm_gcda_emit_function() in the clang-11 release. Drop the older implementations and require folks to upgrade their compiler if they're interested in GCOV support. Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 Suggested-by: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- For an easier time reviewing this series, reviewers may want to apply these patches, then check the overall diff with `git diff origin/HEAD`. kernel/gcov/Kconfig | 1 + kernel/gcov/clang.c | 85 --------------------------------------------- 2 files changed, 1 insertion(+), 85 deletions(-) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index f62de2dea8a3..58f87a3092f3 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -4,6 +4,7 @@ menu "GCOV-based kernel profiling" config GCOV_KERNEL bool "Enable gcov-based kernel profiling" depends on DEBUG_FS + depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 select CONSTRUCTORS default n help diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c index 8743150db2ac..14de5644b5cc 100644 --- a/kernel/gcov/clang.c +++ b/kernel/gcov/clang.c @@ -75,9 +75,6 @@ struct gcov_fn_info { u32 num_counters; u64 *counters; -#if CONFIG_CLANG_VERSION < 110000 - const char *function_name; -#endif }; static struct gcov_info *current_info; @@ -107,16 +104,6 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) } EXPORT_SYMBOL(llvm_gcov_init); -#if CONFIG_CLANG_VERSION < 110000 -void llvm_gcda_start_file(const char *orig_filename, const char version[4], - u32 checksum) -{ - current_info->filename = orig_filename; - memcpy(¤t_info->version, version, sizeof(current_info->version)); - current_info->checksum = checksum; -} -EXPORT_SYMBOL(llvm_gcda_start_file); -#else void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) { current_info->filename = orig_filename; @@ -124,29 +111,7 @@ void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) current_info->checksum = checksum; } EXPORT_SYMBOL(llvm_gcda_start_file); -#endif - -#if CONFIG_CLANG_VERSION < 110000 -void llvm_gcda_emit_function(u32 ident, const char *function_name, - u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) -{ - struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); - - if (!info) - return; - INIT_LIST_HEAD(&info->head); - info->ident = ident; - info->checksum = func_checksum; - info->use_extra_checksum = use_extra_checksum; - info->cfg_checksum = cfg_checksum; - if (function_name) - info->function_name = kstrdup(function_name, GFP_KERNEL); - - list_add_tail(&info->head, ¤t_info->functions); -} -EXPORT_SYMBOL(llvm_gcda_emit_function); -#else void llvm_gcda_emit_function(u32 ident, u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) { @@ -163,7 +128,6 @@ void llvm_gcda_emit_function(u32 ident, u32 func_checksum, list_add_tail(&info->head, ¤t_info->functions); } EXPORT_SYMBOL(llvm_gcda_emit_function); -#endif void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) { @@ -326,7 +290,6 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) } } -#if CONFIG_CLANG_VERSION < 110000 static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) { size_t cv_size; /* counter values size */ @@ -335,47 +298,15 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) if (!fn_dup) return NULL; INIT_LIST_HEAD(&fn_dup->head); - - fn_dup->function_name = kstrdup(fn->function_name, GFP_KERNEL); - if (!fn_dup->function_name) - goto err_name; - - cv_size = fn->num_counters * sizeof(fn->counters[0]); - fn_dup->counters = vmalloc(cv_size); - if (!fn_dup->counters) - goto err_counters; - memcpy(fn_dup->counters, fn->counters, cv_size); - - return fn_dup; - -err_counters: - kfree(fn_dup->function_name); -err_name: - kfree(fn_dup); - return NULL; -} -#else -static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) -{ - size_t cv_size; /* counter values size */ - struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), - GFP_KERNEL); - if (!fn_dup) - return NULL; - INIT_LIST_HEAD(&fn_dup->head); - cv_size = fn->num_counters * sizeof(fn->counters[0]); fn_dup->counters = vmalloc(cv_size); if (!fn_dup->counters) { kfree(fn_dup); return NULL; } - memcpy(fn_dup->counters, fn->counters, cv_size); - return fn_dup; } -#endif /** * gcov_info_dup - duplicate profiling data set @@ -416,21 +347,6 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) * gcov_info_free - release memory for profiling data set duplicate * @info: profiling data set duplicate to free */ -#if CONFIG_CLANG_VERSION < 110000 -void gcov_info_free(struct gcov_info *info) -{ - struct gcov_fn_info *fn, *tmp; - - list_for_each_entry_safe(fn, tmp, &info->functions, head) { - kfree(fn->function_name); - vfree(fn->counters); - list_del(&fn->head); - kfree(fn); - } - kfree(info->filename); - kfree(info); -} -#else void gcov_info_free(struct gcov_info *info) { struct gcov_fn_info *fn, *tmp; @@ -443,7 +359,6 @@ void gcov_info_free(struct gcov_info *info) kfree(info->filename); kfree(info); } -#endif #define ITER_STRIDE PAGE_SIZE -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older 2021-03-12 22:41 ` [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older Nick Desaulniers @ 2021-03-15 13:54 ` Peter Oberparleiter 2021-03-15 18:14 ` Nathan Chancellor 1 sibling, 0 replies; 17+ messages in thread From: Peter Oberparleiter @ 2021-03-15 13:54 UTC (permalink / raw) To: Nick Desaulniers, Andrew Morton Cc: Nathan Chancellor, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi On 12.03.2021 23:41, Nick Desaulniers wrote: > LLVM changed the expected function signatures for llvm_gcda_start_file() > and llvm_gcda_emit_function() in the clang-11 release. Drop the older > implementations and require folks to upgrade their compiler if they're > interested in GCOV support. > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > Suggested-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Acked-by: Peter Oberparleiter <oberpar@linux.ibm.com> -- Peter Oberparleiter Linux on Z Development - IBM Germany ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older 2021-03-12 22:41 ` [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older Nick Desaulniers 2021-03-15 13:54 ` Peter Oberparleiter @ 2021-03-15 18:14 ` Nathan Chancellor 1 sibling, 0 replies; 17+ messages in thread From: Nathan Chancellor @ 2021-03-15 18:14 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, linux-kernel, clang-built-linux, Fangrui Song, Prasad Sodagudi On Fri, Mar 12, 2021 at 02:41:32PM -0800, Nick Desaulniers wrote: > LLVM changed the expected function signatures for llvm_gcda_start_file() > and llvm_gcda_emit_function() in the clang-11 release. Drop the older > implementations and require folks to upgrade their compiler if they're > interested in GCOV support. > > Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 > Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 > Suggested-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > For an easier time reviewing this series, reviewers may want to apply > these patches, then check the overall diff with `git diff origin/HEAD`. > > kernel/gcov/Kconfig | 1 + > kernel/gcov/clang.c | 85 --------------------------------------------- > 2 files changed, 1 insertion(+), 85 deletions(-) > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > index f62de2dea8a3..58f87a3092f3 100644 > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -4,6 +4,7 @@ menu "GCOV-based kernel profiling" > config GCOV_KERNEL > bool "Enable gcov-based kernel profiling" > depends on DEBUG_FS > + depends on !CC_IS_CLANG || CLANG_VERSION >= 110000 > select CONSTRUCTORS > default n > help > diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c > index 8743150db2ac..14de5644b5cc 100644 > --- a/kernel/gcov/clang.c > +++ b/kernel/gcov/clang.c > @@ -75,9 +75,6 @@ struct gcov_fn_info { > > u32 num_counters; > u64 *counters; > -#if CONFIG_CLANG_VERSION < 110000 > - const char *function_name; > -#endif > }; > > static struct gcov_info *current_info; > @@ -107,16 +104,6 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > } > EXPORT_SYMBOL(llvm_gcov_init); > > -#if CONFIG_CLANG_VERSION < 110000 > -void llvm_gcda_start_file(const char *orig_filename, const char version[4], > - u32 checksum) > -{ > - current_info->filename = orig_filename; > - memcpy(¤t_info->version, version, sizeof(current_info->version)); > - current_info->checksum = checksum; > -} > -EXPORT_SYMBOL(llvm_gcda_start_file); > -#else > void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > { > current_info->filename = orig_filename; > @@ -124,29 +111,7 @@ void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) > current_info->checksum = checksum; > } > EXPORT_SYMBOL(llvm_gcda_start_file); > -#endif > - > -#if CONFIG_CLANG_VERSION < 110000 > -void llvm_gcda_emit_function(u32 ident, const char *function_name, > - u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > -{ > - struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); > - > - if (!info) > - return; > > - INIT_LIST_HEAD(&info->head); > - info->ident = ident; > - info->checksum = func_checksum; > - info->use_extra_checksum = use_extra_checksum; > - info->cfg_checksum = cfg_checksum; > - if (function_name) > - info->function_name = kstrdup(function_name, GFP_KERNEL); > - > - list_add_tail(&info->head, ¤t_info->functions); > -} > -EXPORT_SYMBOL(llvm_gcda_emit_function); > -#else > void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > u8 use_extra_checksum, u32 cfg_checksum) > { > @@ -163,7 +128,6 @@ void llvm_gcda_emit_function(u32 ident, u32 func_checksum, > list_add_tail(&info->head, ¤t_info->functions); > } > EXPORT_SYMBOL(llvm_gcda_emit_function); > -#endif > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > { > @@ -326,7 +290,6 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > } > } > > -#if CONFIG_CLANG_VERSION < 110000 > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > { > size_t cv_size; /* counter values size */ > @@ -335,47 +298,15 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > if (!fn_dup) > return NULL; > INIT_LIST_HEAD(&fn_dup->head); > - > - fn_dup->function_name = kstrdup(fn->function_name, GFP_KERNEL); > - if (!fn_dup->function_name) > - goto err_name; > - > - cv_size = fn->num_counters * sizeof(fn->counters[0]); > - fn_dup->counters = vmalloc(cv_size); > - if (!fn_dup->counters) > - goto err_counters; > - memcpy(fn_dup->counters, fn->counters, cv_size); > - > - return fn_dup; > - > -err_counters: > - kfree(fn_dup->function_name); > -err_name: > - kfree(fn_dup); > - return NULL; > -} > -#else > -static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > -{ > - size_t cv_size; /* counter values size */ > - struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), > - GFP_KERNEL); > - if (!fn_dup) > - return NULL; > - INIT_LIST_HEAD(&fn_dup->head); > - > cv_size = fn->num_counters * sizeof(fn->counters[0]); > fn_dup->counters = vmalloc(cv_size); > if (!fn_dup->counters) { > kfree(fn_dup); > return NULL; > } > - > memcpy(fn_dup->counters, fn->counters, cv_size); > - > return fn_dup; > } > -#endif > > /** > * gcov_info_dup - duplicate profiling data set > @@ -416,21 +347,6 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > * gcov_info_free - release memory for profiling data set duplicate > * @info: profiling data set duplicate to free > */ > -#if CONFIG_CLANG_VERSION < 110000 > -void gcov_info_free(struct gcov_info *info) > -{ > - struct gcov_fn_info *fn, *tmp; > - > - list_for_each_entry_safe(fn, tmp, &info->functions, head) { > - kfree(fn->function_name); > - vfree(fn->counters); > - list_del(&fn->head); > - kfree(fn); > - } > - kfree(info->filename); > - kfree(info); > -} > -#else > void gcov_info_free(struct gcov_info *info) > { > struct gcov_fn_info *fn, *tmp; > @@ -443,7 +359,6 @@ void gcov_info_free(struct gcov_info *info) > kfree(info->filename); > kfree(info); > } > -#endif > > #define ITER_STRIDE PAGE_SIZE > > -- > 2.31.0.rc2.261.g7f71774620-goog > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 19:21 [PATCH] gcov: fix clang-11+ support Nick Desaulniers 2021-03-12 19:58 ` Nathan Chancellor @ 2021-03-12 20:25 ` Fangrui Song 2021-03-12 22:05 ` Nick Desaulniers 1 sibling, 1 reply; 17+ messages in thread From: Fangrui Song @ 2021-03-12 20:25 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, Prasad Sodagudi, Nathan Chancellor, linux-kernel, clang-built-linux On 2021-03-12, Nick Desaulniers wrote: >LLVM changed the expected function signatures for llvm_gcda_start_file() >and llvm_gcda_emit_function() in the clang-11 release. Users of clang-11 >or newer may have noticed their kernels failing to boot due to a panic >when enabling CONFIG_GCOV_KERNEL=y +CONFIG_GCOV_PROFILE_ALL=y. Fix up >the function signatures so calling these functions doesn't panic the >kernel. > >When we drop clang-10 support from the kernel, we should carefully >update the original implementations to try to preserve git blame, >deleting these implementations. > >Link: https://reviews.llvm.org/rGcdd683b516d147925212724b09ec6fb792a40041 >Link: https://reviews.llvm.org/rG13a633b438b6500ecad9e4f936ebadf3411d0f44 >Cc: Fangrui Song <maskray@google.com> >Reported-by: Prasad Sodagudi<psodagud@quicinc.com> >Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> >--- > kernel/gcov/clang.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > >diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c >index c94b820a1b62..20e6760ec05d 100644 >--- a/kernel/gcov/clang.c >+++ b/kernel/gcov/clang.c >@@ -75,7 +75,9 @@ struct gcov_fn_info { > > u32 num_counters; > u64 *counters; >+#if __clang_major__ < 11 > const char *function_name; >+#endif function_name can be unconditionally deleted. It is not used by llvm-cov gcov. You'll need to delete a few assignments to gcov_info_free but you can then unify the gcov_fn_info_dup and gcov_info_free implementations. > }; > > static struct gcov_info *current_info; >@@ -105,6 +107,7 @@ void llvm_gcov_init(llvm_gcov_callback writeout, llvm_gcov_callback flush) > } > EXPORT_SYMBOL(llvm_gcov_init); > >+#if __clang_major__ < 11 > void llvm_gcda_start_file(const char *orig_filename, const char version[4], > u32 checksum) > { >@@ -113,7 +116,17 @@ void llvm_gcda_start_file(const char *orig_filename, const char version[4], > current_info->checksum = checksum; > } > EXPORT_SYMBOL(llvm_gcda_start_file); >+#else >+void llvm_gcda_start_file(const char *orig_filename, u32 version, u32 checksum) >+{ >+ current_info->filename = orig_filename; >+ current_info->version = version; >+ current_info->checksum = checksum; >+} >+EXPORT_SYMBOL(llvm_gcda_start_file); >+#endif LG. On big-endian systems, clang < 11 emitted .gcno/.gcda files do not work with llvm-cov gcov < 11. To fix it and make .gcno/.gcda work with gcc gcov I chose to break compatibility (and make all the breaking changes like deleting some CC1 options) in a short window. At that time I was not aware that there is the kernel implementation. Later on I was CCed on a few https://github.com/ClangBuiltLinux/linux/ gcov issues but I forgot to mention the interface change. Now in clang 11 onward, clang --coverage defaults to the gcov 4.8 compatible format. You can specify the CC1 option (internal option, subject to change) -coverage-version to make it compatible with other versions' gcov. -Xclang -coverage-version='407*' => 4.7 -Xclang -coverage-version='704*' => 7.4 -Xclang -coverage-version='B02*' => 10.2 (('B'-'A')*10 = 10) Reviewed-by: Fangrui Song <maskray@google.com> >+#if __clang_major__ < 11 > void llvm_gcda_emit_function(u32 ident, const char *function_name, > u32 func_checksum, u8 use_extra_checksum, u32 cfg_checksum) > { >@@ -133,6 +146,24 @@ void llvm_gcda_emit_function(u32 ident, const char *function_name, > list_add_tail(&info->head, ¤t_info->functions); > } > EXPORT_SYMBOL(llvm_gcda_emit_function); >+#else >+void llvm_gcda_emit_function(u32 ident, u32 func_checksum, >+ u8 use_extra_checksum, u32 cfg_checksum) >+{ >+ struct gcov_fn_info *info = kzalloc(sizeof(*info), GFP_KERNEL); >+ >+ if (!info) >+ return; >+ >+ INIT_LIST_HEAD(&info->head); >+ info->ident = ident; >+ info->checksum = func_checksum; >+ info->use_extra_checksum = use_extra_checksum; >+ info->cfg_checksum = cfg_checksum; >+ list_add_tail(&info->head, ¤t_info->functions); >+} >+EXPORT_SYMBOL(llvm_gcda_emit_function); >+#endif > > void llvm_gcda_emit_arcs(u32 num_counters, u64 *counters) > { >@@ -295,6 +326,7 @@ void gcov_info_add(struct gcov_info *dst, struct gcov_info *src) > } > } > >+#if __clang_major__ < 11 > static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > { > size_t cv_size; /* counter values size */ >@@ -322,6 +354,28 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) > kfree(fn_dup); > return NULL; > } >+#else >+static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn) >+{ >+ size_t cv_size; /* counter values size */ >+ struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn), >+ GFP_KERNEL); >+ if (!fn_dup) >+ return NULL; >+ INIT_LIST_HEAD(&fn_dup->head); >+ >+ cv_size = fn->num_counters * sizeof(fn->counters[0]); >+ fn_dup->counters = vmalloc(cv_size); >+ if (!fn_dup->counters) { >+ kfree(fn_dup); >+ return NULL; >+ } >+ >+ memcpy(fn_dup->counters, fn->counters, cv_size); >+ >+ return fn_dup; >+} >+#endif > > /** > * gcov_info_dup - duplicate profiling data set >@@ -362,6 +416,7 @@ struct gcov_info *gcov_info_dup(struct gcov_info *info) > * gcov_info_free - release memory for profiling data set duplicate > * @info: profiling data set duplicate to free > */ >+#if __clang_major__ < 11 > void gcov_info_free(struct gcov_info *info) > { > struct gcov_fn_info *fn, *tmp; >@@ -375,6 +430,20 @@ void gcov_info_free(struct gcov_info *info) > kfree(info->filename); > kfree(info); > } >+#else >+void gcov_info_free(struct gcov_info *info) >+{ >+ struct gcov_fn_info *fn, *tmp; >+ >+ list_for_each_entry_safe(fn, tmp, &info->functions, head) { >+ vfree(fn->counters); >+ list_del(&fn->head); >+ kfree(fn); >+ } >+ kfree(info->filename); >+ kfree(info); >+} >+#endif > > #define ITER_STRIDE PAGE_SIZE > > >base-commit: f78d76e72a4671ea52d12752d92077788b4f5d50 >-- >2.31.0.rc2.261.g7f71774620-goog > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 20:25 ` [PATCH] gcov: fix clang-11+ support Fangrui Song @ 2021-03-12 22:05 ` Nick Desaulniers 2021-03-12 22:24 ` Fangrui Song 0 siblings, 1 reply; 17+ messages in thread From: Nick Desaulniers @ 2021-03-12 22:05 UTC (permalink / raw) To: Fangrui Song Cc: Peter Oberparleiter, Andrew Morton, Prasad Sodagudi, Nathan Chancellor, LKML, clang-built-linux On Fri, Mar 12, 2021 at 12:25 PM 'Fangrui Song' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > function_name can be unconditionally deleted. It is not used by llvm-cov > gcov. You'll need to delete a few assignments to gcov_info_free but you > can then unify the gcov_fn_info_dup and gcov_info_free implementations. > > LG. On big-endian systems, clang < 11 emitted .gcno/.gcda files do not > work with llvm-cov gcov < 11. To fix it and make .gcno/.gcda work with > gcc gcov I chose to break compatibility (and make all the breaking > changes like deleting some CC1 options) in a short window. At that time > I was not aware that there is the kernel implementation. Later on I was > CCed on a few https://github.com/ClangBuiltLinux/linux/ gcov issues but > I forgot to mention the interface change. These are all good suggestions. Since in v2 I'll drop support for clang < 11, I will skip additional patches to disable GCOV when using older clang for BE, and the function_name cleanup. > Now in clang 11 onward, clang --coverage defaults to the gcov 4.8 > compatible format. You can specify the CC1 option (internal option, > subject to change) -coverage-version to make it compatible with other > versions' gcov. > > -Xclang -coverage-version='407*' => 4.7 > -Xclang -coverage-version='704*' => 7.4 > -Xclang -coverage-version='B02*' => 10.2 (('B'-'A')*10 = 10) How come LLVM doesn't default to 10.2 format, if it can optionally produce it? We might be able to reuse more code in the kernel between the two impelementations, though I expect the symbols the runtime is expected to provide will still differ. Seeing the `B` in `B02*` is also curious. Thanks for the review, will include your tag in v2. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] gcov: fix clang-11+ support 2021-03-12 22:05 ` Nick Desaulniers @ 2021-03-12 22:24 ` Fangrui Song 0 siblings, 0 replies; 17+ messages in thread From: Fangrui Song @ 2021-03-12 22:24 UTC (permalink / raw) To: Nick Desaulniers Cc: Peter Oberparleiter, Andrew Morton, Prasad Sodagudi, Nathan Chancellor, LKML, clang-built-linux On 2021-03-12, Nick Desaulniers wrote: >On Fri, Mar 12, 2021 at 12:25 PM 'Fangrui Song' via Clang Built Linux ><clang-built-linux@googlegroups.com> wrote: >> >> function_name can be unconditionally deleted. It is not used by llvm-cov >> gcov. You'll need to delete a few assignments to gcov_info_free but you >> can then unify the gcov_fn_info_dup and gcov_info_free implementations. >> >> LG. On big-endian systems, clang < 11 emitted .gcno/.gcda files do not >> work with llvm-cov gcov < 11. To fix it and make .gcno/.gcda work with >> gcc gcov I chose to break compatibility (and make all the breaking >> changes like deleting some CC1 options) in a short window. At that time >> I was not aware that there is the kernel implementation. Later on I was >> CCed on a few https://github.com/ClangBuiltLinux/linux/ gcov issues but >> I forgot to mention the interface change. > >These are all good suggestions. Since in v2 I'll drop support for >clang < 11, I will skip additional patches to disable GCOV when using >older clang for BE, and the function_name cleanup. Only llvm_gcda_start_file & llvm_gcda_emit_function need version dispatch. In that case (since there will just be two #if in the file) we don't even need depends on CC_IS_GCC || CLANG_VERSION >= 110000 >> Now in clang 11 onward, clang --coverage defaults to the gcov 4.8 >> compatible format. You can specify the CC1 option (internal option, >> subject to change) -coverage-version to make it compatible with other >> versions' gcov. >> >> -Xclang -coverage-version='407*' => 4.7 >> -Xclang -coverage-version='704*' => 7.4 >> -Xclang -coverage-version='B02*' => 10.2 (('B'-'A')*10 = 10) > >How come LLVM doesn't default to 10.2 format, if it can optionally >produce it? We might be able to reuse more code in the kernel between >the two impelementations, though I expect the symbols the runtime is >expected to provide will still differ. Seeing the `B` in `B02*` is >also curious. > >Thanks for the review, will include your tag in v2. 4.8 has the widest range of compiler support. gcov 4.8~7.* use the same format. clang instrumentation does not support the column field (useless in my opinion) introduced in gcov 9, so it just writes zeros. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-15 18:15 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 19:21 [PATCH] gcov: fix clang-11+ support Nick Desaulniers 2021-03-12 19:58 ` Nathan Chancellor 2021-03-12 20:14 ` Nick Desaulniers 2021-03-12 20:46 ` Nick Desaulniers 2021-03-12 20:51 ` Nathan Chancellor 2021-03-12 21:57 ` Nick Desaulniers 2021-03-12 22:05 ` Nathan Chancellor 2021-03-12 22:41 ` [PATCH v2 0/2] gcov fixes for clang-11 Nick Desaulniers 2021-03-12 22:41 ` [PATCH v2 1/2] gcov: fix clang-11+ support Nick Desaulniers 2021-03-15 13:51 ` Peter Oberparleiter 2021-03-15 18:13 ` Nathan Chancellor 2021-03-12 22:41 ` [PATCH v2 2/2] gcov: clang: drop support for clang-10 and older Nick Desaulniers 2021-03-15 13:54 ` Peter Oberparleiter 2021-03-15 18:14 ` Nathan Chancellor 2021-03-12 20:25 ` [PATCH] gcov: fix clang-11+ support Fangrui Song 2021-03-12 22:05 ` Nick Desaulniers 2021-03-12 22:24 ` Fangrui Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).