Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make profile handle longjmps correctly #2444

Merged
merged 4 commits into from
May 17, 2018

Conversation

ChrisJefferson
Copy link
Contributor

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 Error functions 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.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be in STATE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just thought how it could get empty -- starting a profile in a function and returning from that function.

@ChrisJefferson ChrisJefferson force-pushed the improve-profile branch 2 times, most recently from d837bd6 to 4bfd263 Compare May 13, 2018 15:47
@codecov
Copy link

codecov bot commented May 13, 2018

Codecov Report

Merging #2444 into master will increase coverage by 0.02%.
The diff coverage is 88.57%.

@@            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
Impacted Files Coverage Δ
src/funcs.c 89.89% <ø> (ø) ⬆️
src/system.c 74.08% <50%> (+1.56%) ⬆️
src/profile.c 41.21% <90.9%> (+5.04%) ⬆️
hpcgap/lib/hpc/stdtasks.g 63.68% <0%> (-0.52%) ⬇️
hpcgap/lib/ffeconway.gi 59.79% <0%> (+0.11%) ⬆️
lib/ffeconway.gi 82.54% <0%> (+0.11%) ⬆️
src/stats.c 87.19% <0%> (+0.13%) ⬆️
src/integer.c 92.2% <0%> (+0.88%) ⬆️
lib/ffe.gi 76.57% <0%> (+2.17%) ⬆️
... and 2 more

@ChrisJefferson ChrisJefferson force-pushed the improve-profile branch 4 times, most recently from 2ddb301 to ec886d7 Compare May 13, 2018 19:32
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.
@fingolfin fingolfin merged commit e4ef9e5 into gap-system:master May 17, 2018
@ChrisJefferson ChrisJefferson deleted the improve-profile branch June 11, 2018 19:02
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.2 milestone Jun 16, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants