perf probe bugfixes
cover perf probe bugfixes
Hi Ingo,
Here are several bugfixes of "perf probe" which I've found recently. I think these should go into perf/urgent branch for safe.
Thank you,
---
Masami Hiramatsu (5):
[BUGFIX] perf probe: Warn when more than one line are given [BUGFIX] perf probe: Fix the order of searching scopes for variables [BUGFIX] perf probe: Fix to walk all inline instances [BUGFIX] perf probe: Fix line walker to check CU correctly [BUGFIX] perf-probe: Fix a memory leak for scopes array
tools/perf/builtin-probe.c | 14 +++++++++++--- tools/perf/util/probe-finder.c | 36 +++++++++++++++++++++--------------- 2 files changed, 32 insertions(+), 18 deletions(-)
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
PATCH 1/5 perf-probe: Fix a memory leak for scopes array
Fix a memory leak for scopes array when it finds a variable in the global scope.
---
tools/perf/util/probe-finder.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
tools/perf/util/probe-finder.c +2 -3
@@ -1177,6 +1177,7 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf)else {/* Search upper class */ nscopes =dwarf_getscopes_die (sp_die, &scopes); + ret = -ENOENT ;while (nscopes-- >1 ) {pr_debug ("Searching variables in %s\n" ,dwarf_diename (&scopes[nscopes]));
@@ -1185,14 +1186,12 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf) pf->pvar->var, 0, &vr_die)) { ret =convert_variable (&vr_die, pf); - goto found; +break ; } }if (scopes)free (scopes); - ret = -ENOENT ; } -found:if (ret <0 )pr_warning ("Failed to find '%s' in this function.\n" , pf->pvar->var);
Re: [PATCH -tip 1/5] [BUGFIX] perf-probe: Fix a memory leak for scopes array
Quoted 8 lines · Masami Hiramatsu
Fix a memory leak for scopes array when it finds a variable in the global scope.
Signed-off-by Masami Hiramatsu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Paul Mackerras <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>
PATCH 2/5 perf probe: Fix line walker to check CU correctly
In reply to Masami HiramatsuFix line walker to check whether a given DIE is CU or not. Actually this function accepts CU, subprogram and inlined_subroutine DIEs.
---
tools/perf/util/probe-finder.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
tools/perf/util/probe-finder.c +5 -4
@@ -599,8 +599,9 @@ static int __die_walk_culines_cb(Dwarf_Die *sp_die, void *data) }/* -* Walk on lines inside given PDIE. If the PDIE is subprogram, walk only on -* the lines inside the subprogram, otherwise PDIE must be a CU DIE. +* Walk on lines inside given PDIE. If the PDIE is subprogram or +* inlined_subprogram, walk only on the lines inside the DIE, +* otherwise PDIE must be a CU DIE. */ static int die_walk_lines (Dwarf_Die *pdie,line_walk_handler_t handler,void *data)
@@ -614,12 +615,12 @@ static int die_walk_lines(Dwarf_Die *pdie, line_walk_handler_t handler,size_t nlines, i;/* Get the CU die */ -if (dwarf_tag (pdie) == DW_TAG_subprogram) +if (dwarf_tag (pdie) != DW_TAG_compile_unit) cu_die =dwarf_diecu (pdie, &die_mem,NULL ,NULL );else cu_die = pdie;if (!cu_die) { -pr_debug2 ("Failed to get CU from subprogram\n" ); +pr_debug2 ("Failed to get CU from given DIE\n" );return -EINVAL ; }
PATCH 3/5 perf probe: Fix to walk all inline instances
In reply to Masami HiramatsuFix line-range collector to walk all instances of inlined function, because some execution paths can be optimized out depends on the function argument of instances.
E.g.)
inline_func (arg) {
if (arg)
do_something;
else
do_another;
}func_A() {
inline_func(1)
}func_B() {
inline_func(0)
}In this case, func_A may have only do_something code and func_B may have only do_another.
---
tools/perf/util/probe-finder.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
tools/perf/util/probe-finder.c +7 -1
@@ -1932,7 +1932,13 @@ static int line_range_inline_cb(Dwarf_Die *in_die, void *data)struct dwarf_callback_param *param = data; param->retval =find_line_range_by_line (in_die, param->data); -return DWARF_CB_ABORT ;/* No need to find other instances */ + +/* +* We have to check all instances of inlined function, because +* some execution paths can be optimized out depends on the +* function argument of instances +*/ +return DWARF_CB_OK ; }/* Search function from function name */
PATCH 4/5 perf probe: Fix the order of searching scopes for variables
In reply to Masami HiramatsuFix variable searching routine to search from inner scope to outer scope.
---
tools/perf/util/probe-finder.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
tools/perf/util/probe-finder.c +7 -7
@@ -1135,7 +1135,7 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf) {Dwarf_Die vr_die, *scopes;char buf[32 ], *ptr; -int ret, nscopes; +int ret, nscopes, i;if (!is_c_varname (pf->pvar->var)) {/* Copy raw parameters */
@@ -1179,11 +1179,11 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf)/* Search upper class */ nscopes =dwarf_getscopes_die (sp_die, &scopes); ret = -ENOENT ; -while (nscopes-- >1 ) { +for (i =1 ; i < nscopes; i++) {/* scopes[0] is sp_die */ pr_debug ("Searching variables in %s\n" , -dwarf_diename (&scopes[nscopes])); +dwarf_diename (&scopes[i]));/* We should check this scope, so give dummy address */ -if (die_find_variable_at (&scopes[nscopes], +if (die_find_variable_at (&scopes[i], pf->pvar->var,0 , &vr_die)) { ret =convert_variable (&vr_die, pf);
@@ -1693,7 +1693,7 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf) container_of(pf, struct available_var_finder, pf);struct variable_list *vl;Dwarf_Die die_mem, *scopes =NULL ; -int ret, nscopes; +int ret, nscopes, i;/* Check number of tevs */ if (af->nvls == af->max_vls) {
@@ -1723,8 +1723,8 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf)/* Don't need to search child DIE for externs. */ af->child = false; nscopes =dwarf_getscopes_die (sp_die, &scopes); -while (nscopes-- >1 ) -die_find_child (&scopes[nscopes], collect_variables_cb, +for (i =1 ; i < nscopes; i++)/* scopes[0] is sp_die */ +die_find_child (&scopes[i], collect_variables_cb, (void *)af, &die_mem);if (scopes)free (scopes);
Re: [PATCH -tip 4/5] [BUGFIX] perf probe: Fix the order of searching scopes for variables
Quoted 2 lines · Masami Hiramatsu
Fix variable searching routine to search from inner scope to outer scope.
Admittedly, I don't really know this code at all. However, the changelog is somewhat vague. Why do we need this change? What problem does it fix? Why does the order in which we iterate 'scopes' matter?
Quoted 65 lines
Signed-off-by Masami Hiramatsu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Paul Mackerras <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>---
tools/perf/util/probe-finder.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)tools/perf/util/probe-finder.c +7
-8@@ -1135,7 +1135,7 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf) { Dwarf_Die vr_die, *scopes; char buf[32], *ptr; - int ret, nscopes; + int ret, nscopes, i; if (!is_c_varname(pf->pvar->var)) { /* Copy raw parameters */@@ -1179,11 +1179,11 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf) /* Search upper class */ nscopes = dwarf_getscopes_die(sp_die, &scopes); ret = -ENOENT; - while (nscopes-- > 1) { + for (i = 1; i < nscopes; i++) { /* scopes[0] is sp_die */ pr_debug("Searching variables in %s\n", - dwarf_diename(&scopes[nscopes])); + dwarf_diename(&scopes[i])); /* We should check this scope, so give dummy address */ - if (die_find_variable_at(&scopes[nscopes], + if (die_find_variable_at(&scopes[i], pf->pvar->var, 0, &vr_die)) { ret = convert_variable(&vr_die, pf);@@ -1693,7 +1693,7 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf) container_of(pf, struct available_var_finder, pf); struct variable_list *vl; Dwarf_Die die_mem, *scopes = NULL; - int ret, nscopes; + int ret, nscopes, i; /* Check number of tevs */ if (af->nvls == af->max_vls) {@@ -1723,8 +1723,8 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf) /* Don't need to search child DIE for externs. */ af->child = false; nscopes = dwarf_getscopes_die(sp_die, &scopes); - while (nscopes-- > 1) - die_find_child(&scopes[nscopes], collect_variables_cb, + for (i = 1; i < nscopes; i++) /* scopes[0] is sp_die */ + die_find_child(&scopes[i], collect_variables_cb, (void *)af, &die_mem); if (scopes) free(scopes); --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -tip 4/5] [BUGFIX] perf probe: Fix the order of searching scopes for variables
Hi Pekka,
Quoted 8 lines · Pekka Enberg
Quoted 2 lines · Masami Hiramatsu
Fix variable searching routine to search from inner scope to outer scope.
Admittedly, I don't really know this code at all. However, the changelog is somewhat vague. Why do we need this change? What problem does it fix? Why does the order in which we iterate 'scopes' matter?
Hmm, I see.
Debuginfo expresses functions and nested inlined functions as DIE(Dwarf Information Entry) tree, and "scope" means each level of the DIE.
Outer
^
|
CU DIE [A](compile unit: it's an object file)
+-global variable DIEs
+-inline function declaration DIEs
+-function [B] DIE
+-local variable DIEs
+-inline function instance [C] DIE
+-inline local variable DIEs
+-(nested)inline function instance [D] DIE
+...|
v
Inner
Imagine that if we are at [C],
nscopes = dwarf_getscopes_die([C], &scopes)
this returns scopes[] DIEs containing [C] DIE, and nscopes = 3 On that array, scopes[0] is [C], scopes[1] is [B], and scopes[2] is [A].
And, the below loop
while (nscopes-- > 1)
die_find_variable_at(&scopes[nscopes], ...
starts to find variables from the outermost scope [A] and find a global variable first. This changes searching order from inner scope ([B]) to outer ([A]) to search global vars last.
Thank you!
Quoted 67 lines
Quoted 66 lines
Signed-off-by Masami Hiramatsu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Paul Mackerras <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>---
tools/perf/util/probe-finder.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)tools/perf/util/probe-finder.c +7
-8@@ -1135,7 +1135,7 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf) {Dwarf_Die vr_die, *scopes;char buf[32 ], *ptr; -int ret, nscopes; +int ret, nscopes, i;if (!is_c_varname (pf->pvar->var)) {/* Copy raw parameters */ @@ -1179,11 +1179,11 @@ static int find_variable(Dwarf_Die *sp_die, struct probe_finder *pf)/* Search upper class */ nscopes =dwarf_getscopes_die (sp_die, &scopes); ret = -ENOENT ; -while (nscopes-- >1 ) { +for (i =1 ; i < nscopes; i++) {/* scopes[0] is sp_die */ pr_debug ("Searching variables in %s\n" , -dwarf_diename (&scopes[nscopes])); +dwarf_diename (&scopes[i]));/* We should check this scope, so give dummy address */ -if (die_find_variable_at (&scopes[nscopes], +if (die_find_variable_at (&scopes[i], pf->pvar->var,0 , &vr_die)) { ret =convert_variable (&vr_die, pf);@@ -1693,7 +1693,7 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf) container_of(pf, struct available_var_finder, pf);struct variable_list *vl;Dwarf_Die die_mem, *scopes =NULL ; -int ret, nscopes; +int ret, nscopes, i;/* Check number of tevs */ if (af->nvls == af->max_vls) {@@ -1723,8 +1723,8 @@ static int add_available_vars(Dwarf_Die *sp_die, struct probe_finder *pf)/* Don't need to search child DIE for externs. */ af->child = false; nscopes =dwarf_getscopes_die (sp_die, &scopes); -while (nscopes-- >1 ) -die_find_child (&scopes[nscopes], collect_variables_cb, +for (i =1 ; i < nscopes; i++)/* scopes[0] is sp_die */ +die_find_child (&scopes[i], collect_variables_cb, (void *)af, &die_mem);if (scopes)free (scopes); --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
Re: [PATCH -tip 4/5] [BUGFIX] perf probe: Fix the order of searching scopes for variables
Quoted 44 lines · Masami Hiramatsu
Hi Pekka,
Quoted 8 lines · Pekka Enberg
Quoted 2 lines · Masami Hiramatsu
Fix variable searching routine to search from inner scope to outer scope.
Admittedly, I don't really know this code at all. However, the changelog is somewhat vague. Why do we need this change? What problem does it fix? Why does the order in which we iterate 'scopes' matter?
Hmm, I see.
Debuginfo expresses functions and nested inlined functions as DIE(Dwarf Information Entry) tree, and "scope" means each level of the DIE.
Outer
^
|
CU DIE [A](compile unit: it's an object file)+-global variable DIEs +-inline function declaration DIEs +-function [B] DIE +-local variable DIEs +-inline function instance [C] DIE +-inline local variable DIEs +-(nested)inline function instance [D] DIE+...
|
v
InnerImagine that if we are at [C],
nscopes =
dwarf_getscopes_die([C], &scopes)this returns scopes[] DIEs containing [C] DIE, and nscopes = 3 On that array, scopes[0] is [C], scopes[1] is [B], and scopes[2] is [A].
And, the below loop
Quoted 7 lines
while (nscopes-- > 1)
die_find_variable_at(&scopes[nscopes], ...starts to find variables from the outermost scope [A] and find a global variable first. This changes searching order from inner scope ([B]) to outer ([A]) to search global vars last.
Thanks for the explanation! What kind of problems does this cause for 'perf probe' users?
Re: [PATCH -tip 4/5] [BUGFIX] perf probe: Fix the order of searching scopes for variables
2011/7/11 Pekka Enberg <[email protected]>:
Quoted 49 lines
Quoted 47 lines · Masami Hiramatsu
Hi Pekka,
Quoted 8 lines · Pekka Enberg
Quoted 2 lines · Masami Hiramatsu
Fix variable searching routine to search from inner scope to outer scope.
Admittedly, I don't really know this code at all. However, the changelog is somewhat vague. Why do we need this change? What problem does it fix? Why does the order in which we iterate 'scopes' matter?
Hmm, I see.
Debuginfo expresses functions and nested inlined functions as DIE(Dwarf Information Entry) tree, and "scope" means each level of the DIE.
Outer
^
|
CU DIE [A](compile unit: it's an object file)+-global variable DIEs +-inline function declaration DIEs +-function [B] DIE +-local variable DIEs +-inline function instance [C] DIE +-inline local variable DIEs +-(nested)inline function instance [D] DIE +...|
v
InnerImagine that if we are at [C],
nscopes =
dwarf_getscopes_die([C], &scopes)this returns scopes[] DIEs containing [C] DIE, and nscopes = 3 On that array, scopes[0] is [C], scopes[1] is [B], and scopes[2] is [A].
And, the below loop
while (nscopes-- > 1)
die_find_variable_at(&scopes[nscopes], ...
Quoted 8 lines
Quoted 4 lines
starts to find variables from the outermost scope [A] and find a global variable first. This changes searching order from inner scope ([B]) to outer ([A]) to search global vars last.
Thanks for the explanation! What kind of problems does this cause for 'perf probe' users?
Hmm, wait, I must say "drop this! Ingo" since I've found that both of original routine and this fix will not fix a problem.
OK, here is the problem I have - if there are some same-name variables in different scopes, which one should be referred first? for example, imagine the below situation;
int var;
void inline infunc(int i)
{
i++; <----here, a user put a probe with 'var' argument.
}
void func(void)
{
int var = 10;
infunc(var);
}
This patch searches in the inline-local scope (infunc), after that, it searches from inner scope (func) to outer scope (global). In this case, it picks up the function local variable if it found, before searching global variables. This is same
logic as libdw does. But this looks insane in C manner. So, I'd like to drop this.
On the other hand, original one searches in the inline-local scope (infunc), after
that, searches in the outermost (global) scope, and then inner scope (func). This will pick the global variable first if it failed to find in inline-local scopes.
This seems sane.
... But wait, if there is no global 'var', what happened? In that case, it will also
return the 'var' in 'func' scope. This also might not be good. User will expect "there is no var" error in that case, but perf-probe will have no error and define an event with unexpected variable as an argument.
So I think the correct fix is not to use several scopes, but search var only in the outermost scope. I'm not so sure about scopes, there might be several other scopes (not only functions, maybe namespace or something?) I've just tried to mimic the libdw logic in this patch.
Anyway, thanks Pekka! for giving me a chance to find it. ;-)
Thank you again,
Signature · 2 lines
Masami Hiramatsu mailto:[email protected]
PATCH 5/5 perf probe: Warn when more than one line are given
In reply to Masami HiramatsuCheck multiple --lines option and print warning which warns only the first specified --line option is valid.
Changes from previous version:
- Accept only the first option instead of the last.
- Fix warning message according to David's comment.
- Mark as a bugfix.
---
tools/perf/builtin-probe.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)
tools/perf/builtin-probe.c +11 -3
@@ -134,10 +134,18 @@ static int opt_show_lines(const struct option *opt __used, {int ret =0 ; -if (str) - ret =parse_line_range_desc (str, ¶ms.line_range); -INIT_LIST_HEAD (¶ms.line_range.line_list); +if (!str) +return 0 ; + +if (params.show_lines) { +pr_warning ("Warning: more than one --line options are" +" detected. Only the first one is valid.\n" ); +return 0 ; + } + params.show_lines = true; + ret =parse_line_range_desc (str, ¶ms.line_range); +INIT_LIST_HEAD (¶ms.line_range.line_list);return ret; }