Make profile handle longjmps correctly #2444
Conversation
src/profile.c
Outdated
| // | ||
| // We need to store the actual values, as RecursionDepth can increase | ||
| // by more than one when a new GAP function is entered | ||
| Obj VisitedDepths; |
There was a problem hiding this comment.
It's attached to the profile really. We could of course put the whole profile in the state (at the moment, you can only profile one state at a time, one thread in hpc gap).
src/profile.c
Outdated
| @@ -151,6 +160,51 @@ struct ProfileState | |||
| /* We keep this seperate as it is exported for use in other files */ | |||
| UInt profileState_Active; | |||
|
|
|||
| /**************************************************************************** | |||
| ** Clean up after Longjmps occur. | |||
There was a problem hiding this comment.
What is this commenting? The LongJmpOccurred variable? But that doesn't seem to fit... ?
src/profile.c
Outdated
| // occurred, or when no function was longjmped over. | ||
| // modifier: When entering a function GetRecursionDepth() returns | ||
| // the depth inside the function we are entering, so modifier | ||
| // provides a way of passing an offset. |
There was a problem hiding this comment.
I don't understand the explanation for modifier
src/profile.c
Outdated
| { | ||
| // This > 0 should not be necessary, but it avoids | ||
| // a crash if the function stack has become badly | ||
| // corrupted. |
There was a problem hiding this comment.
So... it is necessary after all?? Perhaps say something along the line of
Normally VisitedDepths should never be empty at this point, but it can happen if ...
There was a problem hiding this comment.
Honestly, I can't think how this list could end up empty. However, if it did, the best thing to do would be to carry on -- it might lead to a slightly weird profile.
There was a problem hiding this comment.
I've just thought how it could get empty -- starting a profile in a function and returning from that function.
ChrisJefferson
force-pushed
the
improve-profile
branch
2 times, most recently
from
d837bd6
to
4bfd263
Compare
Codecov Report
@@ Coverage Diff @@
## master #2444 +/- ##
==========================================
+ Coverage 74.04% 74.06% +0.02%
==========================================
Files 484 484
Lines 245486 245519 +33
==========================================
+ Hits 181767 181846 +79
+ Misses 63719 63673 -46
|
ChrisJefferson
force-pushed
the
improve-profile
branch
4 times, most recently
from
2ddb301
to
ec886d7
Compare
Call HookedLineIntoFunction before RecursionDepth is incremented, so the value of RecursionDepth is the same when calling both HookedLineIntoFunction and HookedLineOutFunction for the same function call.
This was previously not necessary, as we always checked when entering and leaving a function, but I want to improve coverage in HPC-GAP to stop outputting enter/leave functions (which get mixed up between threads)
Previously, profiling did not detect when Error caused one or more functions to exit. This lead to incorrect and very deep stack traces.
ChrisJefferson
force-pushed
the
improve-profile
branch
from
ec886d7
to
d193020
Compare
olexandr-konovalov
added
the
release notes: added
This is quite a long patch, fixing a fairly small problem.
When GAP calls longjmp (in the case of profiling, this is generally when one of the
Errorfunctions is called), profiling doesn't realise the function has returned. This makes profiles incorrect, and also (more practically) can lead to them having extremely deep stack traces (as they are missing returns) which then crashes parsing the profiles back in.This patch adds a longjmp observer, and whenever a longjmp occurs, inserts function returns. We don't immedately output the returns (as this is a bad idea in hpcgap), so instead we wait until we would next output some profile, and then output the function exits.
With this patch, and v2.0.1 of the profiling package, it is once again possible to run testinstall and generate a profile (this was previously possible, and at some point broke).
While this is a big patch, I'd like to backport it to 4.9, as it shouldn't effect any code outside of profiling.