Messages in this thread Patch in this message | | | Date | Thu, 3 Jan 2013 14:13:01 +0000 | From | Russell King - ARM Linux <> | Subject | Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND |
| |
On Thu, Jan 03, 2013 at 08:36:05AM -0500, Steven Rostedt wrote: > On Thu, 2013-01-03 at 20:36 +0900, Keun-O Park wrote: > > So what have you done about the issue referred in this > > comment? Or do you > > believe that fixing warnings (even if they are explicit > > #warning statements) > > is far more important than code being functionally correct? > > > > I admit that I missed to add notrace to unwind.c. > > Do you think there's more to add? > > I think Russell wants a better change log. What was written sounds like > the fix was to remove a warning. It wasn't very clear to how the warning > is no longer relevant because of the new changes. > > The old change log: > > --- > This fixes a warning saying: > > warning: #warning "TODO: return_address should use unwind tables" > > And, this enables return_address using unwind information. If ARM_UNWIND is > selected, unwind_frame in unwind.c will be called in walk_stackframe. > --- > > > Maybe you wanted to say something like: > > --- > Now that the return_address code can safely use unwind tables, fix up > the #ifdef statements to reflect this. > --- > > Or something similar, if that's what was done.
No. What I want is some evidence that the patch author is not just removing warnings by patching them away, but has thought about why the warning is there, and that they've resolved the issues for why that warning is there - and most importantly why the code is not built in that configuration.
The unwinder has many functions, none of which is marked in any way. This is alluded to in this comment:
/* * return_address uses walk_stackframe to do it's work. If both * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind * information. For this to work in the function tracer many functions would * have to be marked with __notrace. So for now just depend on * !CONFIG_ARM_UNWIND. */
So, what this means is that the function tracer can have hooks in the unwinder code. If one of those hooks is active, then there's the possibility for recursion.
I see no evidence in either of these patches that this issue has been addressed in any way (let alone thought about.) The approach here seems to be "lets remove the comment, lets change the ifdefs to make the code build, and lets remove the warning".
That's not acceptable. We don't fix warnings to make them go away while effectively hiding the original problem.
> > -- Steve > > > > > > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c > > index 00df012..52ff2d4 100644 > > --- a/arch/arm/kernel/unwind.c > > +++ b/arch/arm/kernel/unwind.c > > @@ -327,7 +327,7 @@ static int unwind_exec_insn(struct > > unwind_ctrl_block *ctrl) > > * Unwind a single frame starting with *sp for the symbol at *pc. It > > * updates the *pc and *sp with the new values. > > */ > > -int unwind_frame(struct stackframe *frame) > > +int notrace unwind_frame(struct stackframe *frame) > > { > > unsigned long high, low; > > const struct unwind_idx *idx; > >
And what about search_index(), unwind_find_origin(), unwind_find_idx(), unwind_get_byte(), unwind_exec_insn(), pr_debug(), pr_warning()? Maybe the compiler currently inlines all those functions, but todays compiler behaviour is no guarantee of tomorrow's compiler behaviour. We'd also hope that list_move() would always be inlined and never traced.
It is possible that the ftrace code has some re-entrancy protection to prevent the unwinder recursing back into the ftrace code, but that's not mentioned in this patch... so it could be that needs to be explained.
In summary, from what I can see in the patch, the reason why the ifdefs are the way they are, and the reason the warning is there has not been addressed; these patches just seem to be aimed just at removing a #warning statement to make the warning go away.
| |