tracing: Remove redundant assignment to variable ret
PATCH 1/1 tracing: Remove redundant assignment to variable ret
The value assigned to variable ret is never read, the assignment is redundant and can be removed.
--- kernel/trace/ftrace.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/trace/ftrace.c +3 -3
@@ -7122,9 +7122,9 @@ void __init ftrace_init(void) last_ftrace_enabled = ftrace_enabled =1 ; - ret =ftrace_process_locs (NULL , - __start_mcount_loc, - __stop_mcount_loc); +ftrace_process_locs (NULL , + __start_mcount_loc, + __stop_mcount_loc);pr_info ("ftrace: allocated %ld pages with %ld groups\n" , ftrace_number_of_pages, ftrace_number_of_groups);
Signature · 1 line
2.34.1
Re: [PATCH] tracing: Remove redundant assignment to variable ret'
Dear Yuntao,
when you consider removing dead-store assignments guided by some static analyzer, you need to check if the code you are looking at is actually missing an error-handling branch.
In this case, ftrace_process_locs() may return -ENOMEM, and the caller needs to appropriately deal with this error return code. Your patch does not change the code at all, i.e., the compiled object code is the same as after the patch as before.
Think about how to deal appropriately with the -ENOMEM return in this caller and submit a patch that implements the right error-handling branch or argue in your commit message why that is not needed at all.
If you do not understand or cannot check such basic code properties for dead-store assignments, it might be better to work on some other aspect and area of the kernel repository. E.g., the kernel documentation build also has a few warnings that deserve patches to be fixed.
Best regards,
Lukas
Quoted 22 lines · Lukas Bulwahn
Dear Yuntao,
when you consider removing dead-store assignments guided by some static analyzer, you need to check if the code you are looking at is actually missing an error-handling branch.
In this case,
ftrace_process_locs()may return-ENOMEM, and the caller needs to appropriately deal with this error return code. Your patch does not change the code at all, i.e., the compiled object code is the same as after the patch as before.Think about how to deal appropriately with the
-ENOMEMreturn in this caller and submit a patch that implements the right error-handling branch or argue in your commit message why that is not needed at all.If you do not understand or cannot check such basic code properties for dead-store assignments, it might be better to work on some other aspect and area of the kernel repository. E.g., the kernel documentation build also has a few warnings that deserve patches to be fixed.
Quoted 3 lines
Best regards,
Lukas
Dear Lukas,
Thanks for your reply.
Actually, I had read the source code carefully and noticed the possible error return code -ENOMEM of the ftrace_process_locs() function.
At first I was going to implement an error-handling branch as you said, but after digging into more details, I discovered:
- The ftrace_init() function did not handle the error return code of the ftrace_process_locs() function since the first version.
- The ftrace_module_init() function did not handle it either.
To keep consistent with the existing code, I just removed the assignment in that patch.
Maybe we should deal with the error return code more appropriately, at least print some warnings?
Best regards,
Yuntao
Quoted 32 lines · Yuntao Wang
Quoted 22 lines · Lukas Bulwahn
Dear Yuntao,
when you consider removing dead-store assignments guided by some static analyzer, you need to check if the code you are looking at is actually missing an error-handling branch.
In this case,
ftrace_process_locs()may return-ENOMEM, and the caller needs to appropriately deal with this error return code. Your patch does not change the code at all, i.e., the compiled object code is the same as after the patch as before.Think about how to deal appropriately with the
-ENOMEMreturn in this caller and submit a patch that implements the right error-handling branch or argue in your commit message why that is not needed at all.If you do not understand or cannot check such basic code properties for dead-store assignments, it might be better to work on some other aspect and area of the kernel repository. E.g., the kernel documentation build also has a few warnings that deserve patches to be fixed.
Quoted 3 lines
Best regards,
Lukas
Dear Lukas,
Thanks for your reply.
Quoted 9 lines
Actually, I had read the source code carefully and noticed the possible error return code
-ENOMEMof theftrace_process_locs()function.At first I was going to implement an error-handling branch as you said, but after digging into more details, I discovered:
- The
ftrace_init()function did not handle the error return code of theftrace_process_locs()function since the first version.
- Theftrace_module_init()function did not handle it either.
This is certainly important information on the rationale, and hence, this needs to go into the commit message explaining why you propose this change.
Now, you should also explain: why do you consider it not a problem that this error situation -ENOMEM is not handled by the caller?
And if so, why should ftrace_process_locs() even return an error code if this error return is not considered?
Your commit message should really explain this reasoning.
Quoted 6 lines
To keep consistent with the existing code, I just removed the assignment in that patch.
Maybe we should deal with the error return code more appropriately, at least print some warnings?
This might be one way of dealing with it.
Lukas
Steven Rostedt
maintainerQuoted 5 lines · On Wed, 19 Jan 2022 08:15:10 +0100 Lukas Bulwahn <[email protected]>…
Quoted 3 lines
Maybe we should deal with the error return code more appropriately, at least print some warnings?
This might be one way of dealing with it.
Actually that is the way it was suppose to be dealt with. I just never got around to writing the message :-p
-- Steve
PATCH v2 1/1 tracing: Deal with error return code of the ftrace_process_locs() function
The ftrace_process_locs() function may return -ENOMEM error code, which should be handled by the callers.
---
Changes since v1:
- Deal with error return code instead of just removing it
kernel/trace/ftrace.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
kernel/trace/ftrace.c +13 -4
@@ -6791,11 +6791,16 @@ void ftrace_module_enable(struct module *mod)void ftrace_module_init (struct module *mod) { +int ret; +if (ftrace_disabled || !mod->num_ftrace_callsites)return ; -ftrace_process_locs (mod, mod->ftrace_callsites, - mod->ftrace_callsites + mod->num_ftrace_callsites); + ret =ftrace_process_locs (mod, mod->ftrace_callsites, + mod->ftrace_callsites + mod->num_ftrace_callsites); +if (ret) +pr_warn ("ftrace: failed to allocate entries for module '%s' functions\n" , + mod->name); } static void save_ftrace_mod_rec(struct ftrace_mod_map *mod_map,
@@ -7126,15 +7131,19 @@ void __init ftrace_init(void)pr_info ("ftrace: allocating %ld entries in %ld pages\n" , count, count /ENTRIES_PER_PAGE +1 ); - last_ftrace_enabled = ftrace_enabled =1 ; - ret =ftrace_process_locs (NULL , __start_mcount_loc, __stop_mcount_loc); +if (ret) { +pr_warn ("ftrace: failed to allocate entries for functions\n" ); + goto failed; + }pr_info ("ftrace: allocated %ld pages with %ld groups\n" , ftrace_number_of_pages, ftrace_number_of_groups); + last_ftrace_enabled = ftrace_enabled =1 ; +set_ftrace_early_filters ();return ;
Signature · 1 line
2.34.1.75.gabe6bb3905
Steven Rostedt
maintainerRe: [PATCH v2] tracing: Deal with error return code of the ftrace_process_locs() function
Quoted 9 lines · On Thu, 20 Jan 2022 06:59:49 +0000 Yuntao Wang <[email protected]> wrote:
The
ftrace_process_locs()function may return-ENOMEMerror code, which should be handled by the callers.Signed-off-by Yuntao Wang <[email protected]>---
Changes since v1:
- Deal with error return code instead of just removing it
Thanks, I added this to my queue. Note, I renamed the subject to start with "ftrace:" instead of "tracing:" as this is specific to ftrace (the function hook infrastructure).
-- Steve