Just to make sure I don't to anything silly.

When an application crashes, it usually (at least in Lazarus) dumps a stacktrace to the console.

I used to use stabs, and the stacktrace was dumped in less than a second. Now I have switched to dwarf and the same stacktrace (maybe 15 frames) takes long enough to get a coffee (well ok, not, but at least 10 seconds or so...)

Is that normal?

Martin

Hello,

from rtl/inc/lnfodwrf.pp:

{ Current issues:

- ignores DW_LNS_SET_FILE
- slow
}

The main reason for this slowness is that that implementation (of mine) reads debug info byte-by-byte from the input stream using blockread(). I think this I/O the main performance bottleneck.

At that time I did not bother improving this since it is sufficient for my purposes. I did not test with very large debug infos because on my development platform Lazarus does not work >:)

Improving upon that should be trivial, all reading from the debug input has been encapsuled in the two ReadNext() methods in the file mentioned. It should be easy to make them to read from a (static?) buffer that is filled blockwise; note that a static buffer may give headaches for MT programs, and you are strongly discouraged to do memory allocation during crashes...

Another one might be that for every line info retrieval, the whole debug info is parsed because afair FPC does not generate the DWARF optional lookup table for searching for a compilation unit given an address (i.e. the .debug_aranges section).

Hth,
Thomas

By making the buffer a threadvar, this should not be an issue ?

I was glad to read your answer, because I was having the same problem as Martin. I ended up by simply disabling the stack trace.

Michael.

Michael Van Canneyt wrote

By making the buffer a threadvar, this should not be an issue ?

It will waste memory for each thread you start though, for functionality that you hopefully won't need anyway.

Jonas

Correct.

But let's start by improving efficiency, the MT issue can follow later :-)

Michael.

Schatzl Thomas wrote

Improving upon that should be trivial, all reading from the debug input has been encapsuled in the two ReadNext() methods in the file mentioned. It should be easy to make them to read from a (static?) buffer that is filled blockwise; note that a static buffer may give headaches for MT programs, and you are strongly discouraged to do memory allocation during crashes...

Allocating that buffer on the stack isn't sufficient?

Micha

That's not easy when the exception is raised in case of a stack overflow.

Vincent

Michael Van Canneyt wrote

But let's start by improving efficiency, the MT issue can follow later :-)

I disagree. I think correctness should always be more important than efficiency, especially for RTL functions (and in particular when it's about debugging helpers). Saying that "someone will fix it later for corner cases" usually means that no one fixes it, everyone forgets about it, and then at one point there is a bug report and someone spends half a day figuring out why something doesn't work (to find out it was basically intentional and left as an exercise for someone else). I've been there many times already.

Jonas

I was also thinking, maybe have an
buffer: Array[0..999] locally in GetLineInfo and pass a pointer around?

There is plenty of other stuff already on the stack, and there are calls made to functions which need place on the stack.
So if a stackoverflow occurs, then how does it work anyway? Only problem will be if an error occurs with minimum free stack left. Every byte allocated slightly increases the chance that the exception handler will hit the limit.

Another possibility would be a global (cross thread) pre-allocated buffer, and an exception+thread-save way to mark it as in use => the first thread that is dumbing, will grab it, other threads go byte by byte => but it is a lot of work, since it is a dual implementation (I don't like it).

Just another question: IS it thread save at the moment: OpenDwarf, uses a global variable...

Martin

Vincent Snijders :

In case of a stack overflow *anything* using the stack is doomed. But if we're talking about checking the stack it shouldn't be too hard to add some safety bytes to the check, should it?

But honestly, I don't understand the issue at all: How often would it happen that several threads try dumping their stack at the same time? In the real world, I mean?

I'd be for allocating the buffer statically and for MT safe code you still can add critical sections later. After all, dumping two stack traces at the same time wouldn't be very readable, neither. ;)

Vinzent.

Well developperrs can explicitly call the stackdump, even if no crash has occurred.

And if memory is corruupted, then a lot of threads may crash at the same time. With a single stacktrace taking half a minute at current, they can easily intersect....

I'd be for allocating the buffer statically and for MT safe code you still can add critical sections later. After all, dumping two stack traces at the same time wouldn't be very readable, neither. ;)

The best would be a non OS-dependend, cpu based spinlock, or similar. The asm implementation would have to be created once for each cpu. (making sure the cache is bypassed). Then stacktraces, can be dumped one by one.

Ok , I have created a patch. And it does the trick. Without it dumping about 15 frames took 30secs. With the patch it stays below 1 sec (too fast to measure).

The patch only uses a buffer of 100 bytes. I think this should not be a problem in terms of resource usage. (They are allocated as const buf: array [0..99] of byte)

About thread save.

The code already was not thread save, because the variable "e" Which contains the file handle etc, is a normal global variable (Apparently in order to be able to close it in the units finalisation). Of course normally that doesn't mean to add to the fault. Yet for now I have allocated the buffer as global var too.

There are 2 ways to make it thread save:
1) Make e and buf threadvar
It will still only be 100 bytes per thread (probably it would even be fine with 50 bytes per thread), so the memory usage would still not be too bad.

2) Protect "GetLineInfo" with a spin lock
That is probably preferable anyway

Otherwise, I have no idea about the coding style used in fpc. So sorry if my patch does not follow it.

Also I made 2 procedures "inline", I have not tested how much time that gains, but I guess it is minor. So if that isn't compliant with the code guidelines, remove it

Martin

I'll have to withdraw it.

I have no idea how I compiled an exe that actually did not crash when dumping a stack (yes I had one.). but the patch doesn't work.

Problem I have is how to deal with the open var param.

I need to be able to address the first (or other) byte of it

  function ReadNext(var dest; size : SizeInt) : Boolean; inline;

how to I get dest[n] ?

Martin

Martin wrote

I'll have to withdraw it. I have no idea how I compiled an exe that actually did not crash when dumping a stack (yes I had one.). but the patch doesn't work.

I had that right,, just the debugger confused me the way it displayed the values.

Attached patch works here, and dumps 15 frames in between 1 and 2 secs.

I committed the patch. Thank you.

I don't think that the thread-safety is something to worry about at this time; As you remark, the code was already not thread-safe. But it is something to keep in mind. In the meanwhile, we can at least enjoy the increase in speed.

Michael.

Michael Van Canneyt wrote

I don't think that the thread-safety is something to worry about at this time; As you remark, the code was already not thread-safe. But it is something to keep in mind.

At the very least this bug should be documented, and a {$warning ...} should be added at the appropriate place in the source code so that someone interested in fixing it can do that right away (and so that it is seen every time the rtl is compiled).

Jonas

Well, it was not thread-safe and undocumented in the first place, so I didn't see the need for that.

But I added a {$warning }. Revision 14169.

Michael.

I can't follow here. There was an undocumented problem in a unit, and now that it has been identified and sort of made worse, there is no need to tell our users about it?

But I added a {$warning }. Revision 14169.

Thanks.

Jonas

Well, IMHO without the warning they were not off worse than before. A reasoning born out of laziness more than anything else.

But your point was taken, so hence I added the warning.

Michael.

That's true. Anyway, I meant that we should mention this problem at least in the documentation now that we know about it. I've added it to lnfodwrf.xml

Jonas

You beat me to it... Thanks :-)

And while we're at it, I did the same for lineinfo, as it suffers from the same defect.

Michael.