Hi Mark,
as discussed in [PATCH] stack: Add new '-n MAXFRAMES' option. Resolve addresses after unwind.
I have found I forgot to implement what GDB has as gdb.dwarf2/dw2-dup-frame.exp Backtrace stopped: previous frame identical to this frame (corrupt stack?)
I find it easier in the case of elfutils as all supported unwinding archs have grows-down kind of stack.
It breaks support for -fsplit-stack (GDB gdb.base/morestack.exp) but that is IMO an add-on extension which just coincidentally worked with current checked-in elfutils unwinder (I did not test it).
Thanks, Jan
Hi,
Petr pointed out on irc this patch had slipped through the cracks and hadn't been reviewed yet. Sorry about that. I like the idea, but have some questions on the heuristics used to detect the corrupt stack frame.
On Fri, Jan 03, 2014 at 10:18:09PM +0100, Jan Kratochvil wrote:
I have found I forgot to implement what GDB has as gdb.dwarf2/dw2-dup-frame.exp Backtrace stopped: previous frame identical to this frame (corrupt stack?)
I find it easier in the case of elfutils as all supported unwinding archs have grows-down kind of stack.
It breaks support for -fsplit-stack (GDB gdb.base/morestack.exp) but that is IMO an add-on extension which just coincidentally worked with current checked-in elfutils unwinder (I did not test it).
I don't think it is coincidence that split-stack would work. I believe we don't assume anything about the stack values and so it should work as expected since DWARF is expressive enough to describe it. http://gcc.gnu.org/wiki/SplitStacks#Debugging
Using different stacks seems like a valid execution model and something that is also used by things like sigaltstack. So if we assume CFA values correspond to stack values then detecting corruption through ordering of the CFA values seems not ideal.
We could use the PC combined with the CFA or SP as frame identifier. If both are equal to the values in the previous frame then the unwind stack frame is corrupted.
- // Set unwound->CFA.
- Dwarf_Op *cfa_ops;
- size_t cfa_nops;
- if (dwarf_frame_cfa (frame, &cfa_ops, &cfa_nops) != 0
|| ! expr_eval (state, cfa_ops, cfa_nops, &unwound->cfa, bias, elfclass))
- {
__libdwfl_seterrno (DWFL_E_LIBDW);
return;
- }
- if (unwound->cfa == 0
|| (! state->initial_frame && unwound->cfa <= state->cfa))
- {
__libdwfl_seterrno (DWFL_E_UNWIND_BAD_CFA);
return;
- }
So I don't think the "<=" is correct here. That should be "==" and I would combine it with checking the PC because the CFA/stack could be reused. Another question is whether it is guaranteed that the CFA is always defined. I think the standard says an activation doesn't need to have a CFA defined. If not, then we probably should use the value of the SP register instead. The problem with using the SP value is that it is architecture specific. But we could define the register value through ebl.
Thanks,
Mark
On Sat, 18 Jan 2014 21:25:51 +0100, Mark Wielaard wrote:
I don't think it is coincidence that split-stack would work. I believe we don't assume anything about the stack values and so it should work as expected since DWARF is expressive enough to describe it. http://gcc.gnu.org/wiki/SplitStacks#Debugging
OK, so the patch can also already implement the GDB logic: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ebedcab50d2c76...
Using different stacks seems like a valid execution model and something that is also used by things like sigaltstack.
sigaltstack can be handled by permitting arbitrary CFA adress change only during signal frames. Although in practice on supported archs it does not happen as original stack is always above the sigaltstack.
We could use the PC combined with the CFA or SP as frame identifier. If both are equal to the values in the previous frame then the unwind stack frame is corrupted.
That will not work for endless backtraces looping over >= 2 frames.
Another question is whether it is guaranteed that the CFA is always defined. I think the standard says an activation doesn't need to have a CFA defined. If not, then we probably should use the value of the SP register instead.
It depends on whether elfutils should support any hypothetical case or whether the real world cases are sufficient. I am not aware of CFIs not defining CFA. If there happen some it can be always fixed.
The problem with using the SP value is that it is architecture specific. But we could define the register value through ebl.
Right, that's why SP was not used in that patch.
Thanks, Jan
On Sat, 2014-01-18 at 21:53 +0100, Jan Kratochvil wrote:
On Sat, 18 Jan 2014 21:25:51 +0100, Mark Wielaard wrote:
I don't think it is coincidence that split-stack would work. I believe we don't assume anything about the stack values and so it should work as expected since DWARF is expressive enough to describe it. http://gcc.gnu.org/wiki/SplitStacks#Debugging
OK, so the patch can also already implement the GDB logic: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ebedcab50d2c76...
That is not very pretty. Lets just make sure unwinding works normally in all cases, so we don't need such hacks.
Using different stacks seems like a valid execution model and something that is also used by things like sigaltstack.
sigaltstack can be handled by permitting arbitrary CFA adress change only during signal frames.
That seems backwards. We shouldn't need to list all "allowed" contexts for CFA changes. CFA changes between frames is normal. We just need to detect bad changes like when the CFA or SP plus PC equals a previous frame.
We could use the PC combined with the CFA or SP as frame identifier. If both are equal to the values in the previous frame then the unwind stack frame is corrupted.
That will not work for endless backtraces looping over >= 2 frames.
Yes, given that you don't have a defined ordering for the CFA values. You could check all previous frames to check for duplicate values. But that seems wasteful given that backtraces can be pretty deep. Just checking the last frame will probably catch most bad call stacks.
Another question is whether it is guaranteed that the CFA is always defined. I think the standard says an activation doesn't need to have a CFA defined. If not, then we probably should use the value of the SP register instead.
It depends on whether elfutils should support any hypothetical case or whether the real world cases are sufficient. I am not aware of CFIs not defining CFA. If there happen some it can be always fixed.
elfutils should support programs using the ELF and DWARF spec in reasonable ways, plus commonly used GNU toolchain extensions. I don't know whether the CFA is always defined, that is why I asked. But it seems reasonable to expect it not always being defined. When an architecture uses a link register and passes arguments and the return values through registers then when a leaf function doesn't use any local stack values then it doesn't seem necessary for example. At least that is how I read the spec.
The problem with using the SP value is that it is architecture specific. But we could define the register value through ebl.
Right, that's why SP was not used in that patch.
Lets see whether we need it or not.
Thanks,
Mark
On Mon, 20 Jan 2014 11:08:45 +0100, Mark Wielaard wrote:
On Sat, 2014-01-18 at 21:53 +0100, Jan Kratochvil wrote:
OK, so the patch can also already implement the GDB logic: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ebedcab50d2c76...
That is not very pretty. Lets just make sure unwinding works normally in all cases, so we don't need such hacks.
"very pretty" is not a pragmatic engineering decision. Could there be given more factual rasons?
Yes, given that you don't have a defined ordering for the CFA values.
arch/ebl can define it.
You could check all previous frames to check for duplicate values. But that seems wasteful given that backtraces can be pretty deep.
There is dynamicsizehash.[ch] which can handle it fast enough.
Just checking the last frame will probably catch most bad call stacks.
I do not think so, stack frames contain absolutely any value one can imagine. Particularly one finds all close-to-SP and close-to-PC values at the top of stack.
The problem is that as long as there is no real fix for infinite backtraces one cannot get rid of the broken '-n MAXFRAMES' option. One may speculate the purpose of blocking real infinite backtrace fix is to keep the broken '-n MAXFRAMES' option in place. Factual reasons why the '-n MAXFRAMES' default is broken have been already discussed by multiple people at this list.
Right, that's why SP was not used in that patch.
Lets see whether we need it or not.
As long as there is a requirement to handle non-CFA defining frames (which I have no idea whether exist in real world or not) then SP is required from the backend.
Trying to gather all the requirements and blocks on this feature to figure out which variant of it would be acceptable.
Jan
On Mon, 2014-01-20 at 15:48 +0100, Jan Kratochvil wrote:
On Mon, 20 Jan 2014 11:08:45 +0100, Mark Wielaard wrote:
On Sat, 2014-01-18 at 21:53 +0100, Jan Kratochvil wrote:
OK, so the patch can also already implement the GDB logic: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ebedcab50d2c76...
That is not very pretty. Lets just make sure unwinding works normally in all cases, so we don't need such hacks.
"very pretty" is not a pragmatic engineering decision. Could there be given more factual rasons?
Sorry I thought that was obvious. It disables consistency checking completely when a magic symbol can be found. That IMHO is two wrongs. First it shows there is something wrong with the consistency checking because it checks something that isn't always actually wrong. Secondly it introduces symbol lookups in the middle of an frame unwind, making it depend on symbol resolution which looks like a layering violation.
Yes, given that you don't have a defined ordering for the CFA values.
arch/ebl can define it.
It might be able to define something like a frame identity depending on arch specific registers in a frame. So you could use that to compare/detect previously seen frames. But I don't immediately see how you can define an ordering. What do you suggest to use as ordering function?
You could check all previous frames to check for duplicate values. But that seems wasteful given that backtraces can be pretty deep.
There is dynamicsizehash.[ch] which can handle it fast enough.
OK, but it might still take up a lot of memory. Although I guess you could restrict it to just the last few frames.
The problem is that as long as there is no real fix for infinite backtraces one cannot get rid of the broken '-n MAXFRAMES' option. One may speculate the purpose of blocking real infinite backtrace fix is to keep the broken '-n MAXFRAMES' option in place.
You speculate wrongly. Please stop that. Concentrate on the technical feedback/questions.
I don't think these things are really related. One is a low-level consistency check in an internal implementation of a library, the other is a user command line option default. Both are good to have, they are just at different abstraction layers.
Right, that's why SP was not used in that patch.
Lets see whether we need it or not.
As long as there is a requirement to handle non-CFA defining frames (which I have no idea whether exist in real world or not) then SP is required from the backend.
Agreed.
Mark
On Tue, 21 Jan 2014 17:04:32 +0100, Mark Wielaard wrote:
Sorry I thought that was obvious. It disables consistency checking completely when a magic symbol can be found.
As on the real world supported platforms this magic symbol is the only way when ebl-defined arch frames ordering can be violated.
That IMHO is two wrongs. First it shows there is something wrong with the consistency checking because it checks something that isn't always actually wrong. Secondly it introduces symbol lookups in the middle of an frame unwind, making it depend on symbol resolution which looks like a layering violation.
It may be violating some abstract layers but it just works.
Yes, given that you don't have a defined ordering for the CFA values.
arch/ebl can define it.
It might be able to define something like a frame identity depending on arch specific registers in a frame. So you could use that to compare/detect previously seen frames. But I don't immediately see how you can define an ordering. What do you suggest to use as ordering function?
For all supported archs obviously: ($sp0 < $sp1) <=> $sp0 is inner to $sp1
You could check all previous frames to check for duplicate values. But that seems wasteful given that backtraces can be pretty deep.
There is dynamicsizehash.[ch] which can handle it fast enough.
OK, but it might still take up a lot of memory.
When the backtrace is long then dynamicsizehash will be equally also long.
Stack is normally 8MB so dynamicsizehash can be also approx. say 20MB. If it was for example >200MB in the overflown stack case then one may think about it a bit but does 20MB mean anything worth a word?
Although I guess you could restrict it to just the last few frames.
This again makes this feature working only sometimes, that is useless for low-level tools upon which other high-level functionality depends.
To make a summary what can remain from this patch to get it accepted: * "__morestack" check is forbidden * frames ordering by $sp0 < $sp1 is forbidden * dynamicsizehash.[ch] is forbidden * only $sp0 == $sp1 can be checked and >= 2 frames loops are not detected * therefore signal frames / sigaltstack makes no difference for this patch * CFA check is forbidden, it should check real ebl-supplied SP
Jan
On Tue, 21 Jan 2014 17:26:51 +0100, Jan Kratochvil wrote:
To make a summary what can remain from this patch to get it accepted:
- "__morestack" check is forbidden
- frames ordering by $sp0 < $sp1 is forbidden
- dynamicsizehash.[ch] is forbidden
- only $sp0 == $sp1 can be checked and >= 2 frames loops are not detected
- therefore signal frames / sigaltstack makes no difference for this patch
- CFA check is forbidden, it should check real ebl-supplied SP
There is remaining this issue of the elfutils unwinder.
It seems a proper fix catching all infinite backtraces is not possible under the constraints for the patch acceptance from this mail thread so asking for verification if the constraints above which I deducted from the mail thread are the ones required for the patch acceptance so one can write down at least a partial fix of the problem.
Thanks, Jan
Hi -
That is not very pretty. Lets just make sure unwinding works normally in all cases, so we don't need such hacks.
(Note that in the context of backtraces, you need to consider not only 100% standards-compliant and perfect state, but the possible presence of corrupted data. Handling "all cases" including those with corrupted register / stack / ELF data could be a challenge.
- FChE
On Mon, 2014-01-20 at 10:57 -0500, Frank Ch. Eigler wrote:
That is not very pretty. Lets just make sure unwinding works normally in all cases, so we don't need such hacks.
(Note that in the context of backtraces, you need to consider not only 100% standards-compliant and perfect state, but the possible presence of corrupted data. Handling "all cases" including those with corrupted register / stack / ELF data could be a challenge.
Right. That is actually why I like having a consistency check, it can not just be bad CFI/DWARF data, but also a corrupt user state that makes us return invalid frames. My point was that we should make sure that any consistency check we do use doesn't reject normally valid cases. Especially since we seem to have identified various cases already that would need exception handling to suppress the check otherwise.
Cheers,
Mark
elfutils-devel@lists.fedorahosted.org