Skip to content
Snippets Groups Projects

Capture and print exception backtrace

Merged Capture and print exception backtrace
rmatev/Gaudi:exception-backtrace into master
Merged Rosen Matev requested to merge rmatev/Gaudi:exception-backtrace into master
  • Move definitions (except getters and setters) to .cpp.
  • Remove unnecessary mutability of members.
  • Only capture backtrace when ENABLE_BACKTRACE environment variable is set and the exception is constructed with !code.isSuccess().
  • Add a test that checks for the presence of a backtrace.
  • Deal with chained exceptions.
Edited by Rosen Matev

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Rosen Matev
    Rosen Matev @rmatev ·
    Author Developer

    When enabled, this will cause the world to be recompiled as everything (indirectly) imports GaudiException.h. Why is so much in the header, IDK.

    Edited by Rosen Matev
  • C
    Marco Clemencic @clemenci ·
    Owner

    I think it's a good occasion to split definition from declaration for GaudiException. I do not really see the point in having everything inline in an exception class... for sure it cannot be for performace.

    OTOH I like the backtrace, but it might be useful to have a switch (environment variable?) to enable/disable it... or may be not... what do you think?

    From what I see, chained exceptions will carry (and print) multiple backtraces... probably not a big issue.

  • G
    Gerhard Raven @graven ·
    Developer

    Given that generating the backtrace is incredibly expensive, can we avoid doing that by default? I like having the option of doing this, but it should be optional, and not the default... (eg. one could only do this if NDEBUG is not defined)

    (I can imagine people trying to avoid this, and then use one of the std exceptions instead, at which point you won't get a backtrace at all).

    Also, should the what message that comes with the exception not clearly identify the origin (and yes, I understand that in order to solve the problem it can be handy to know the caller)

    Finally (pun intended!), if it is a way of providing a trace as the call stack is unwound, then the caller could do something like 'SCOPE_FAIL' from https://www.youtube.com/watch?v=WjTrfoiB0MQ and print a message when the scope is left due to an exception -- note that this is still going around in the standards committee, see the latest draft here, but the required building block, i.e. uncaught_exceptions is part of C++17 (see here)

  • C
    Marco Clemencic @clemenci ·
    Owner

    I kind of share your concern about performance, but I would prefer a runtime switch rather than a compile time one: I'd like to avoid having to recompile or run the debug build if not needed.

    But you should take into account that we are talking about exceptions, and the next thing that usually happens after an exception is exit from the program with a failure. In the very rare case we are dealing with a "recoverable" exception, using GaudiException is probably a bad idea (too heavy), but we can foresee an option to avoid the backtrace in specific cases (please, prove we actually have those cases, so that we can fix them removing the exception).

  • Charles Leggett
    Charles Leggett @leggett ·
    Owner

    Unfortunately, there are a lot of components that use exceptions for control flow, and not just to signal that the job is in bad shape and needs to die. Luckily, most of these are not in Gaudi, but if we enable the generation of stack traces with every GaudiException, then I think performance will suffer. I also think it would be much better to have a run-time switch, and should definitely only be enabled in DEBUG builds.

  • G
    Gerhard Raven @graven ·
    Developer

    In Functional, the main work is done inside a try-catch because the client code must return the output, and thus cannot return a StatusCode. As a result, in execute it has (see here:

           } catch ( GaudiException& e ) {
          ( e.code() ? this->warning() : this->error() ) << e.message() << endmsg;
          return e.code();
        }

    so depending on e.code() this either returns failure or success.

    This type of usage suggests the following: make the capture the backtrace conditional on the contained StatusCode: if that is .isSuccess() than skip, otherwise add...

  • Rosen Matev added 163 commits

    added 163 commits

    Compare with previous version

  • Rosen Matev added 2 commits

    added 2 commits

    • b25c0381 - Save and print stack trace in GaudiException
    • 29779c3d - Add test for GaudiException stack traces

    Compare with previous version

    • Charles Leggett
      Charles Leggett @leggett ·
      Owner
      Resolved by Marco Clemencic

      Was talking with Chris Jones about a nifty feature on CMSSW, which I think we should incorporate (though maybe not in this MR?). When an signal is caught and the stack trace printed, ALL threads are immediately paused, so that one gets a meaningful view of the state of the whole system. Otherwise, by the time the stack is captured and printed, the other threads have moved far beyond where they were when the incident happened. This is probably more important for capturing SIGs than exceptions, but it is an incredibly useful feature that we should implement.

  • Rosen Matev changed the description

    changed the description

  • Rosen Matev
  • Rosen Matev added 3 commits

    added 3 commits

    • fbdf0cea - Save and print stack trace in GaudiException
    • 8b1ea7d3 - Add test for GaudiException stack traces
    • 1adf386c - Remove mutable from GaudiException members

    Compare with previous version

  • Rosen Matev changed the description

    changed the description

  • Rosen Matev unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Rosen Matev
    Rosen Matev @rmatev ·
    Author Developer

    I removed WIP as IMHO this is ready for review and wider testing.

    This will invalidate nearly all ccache caches, so I hope I got the header right. If not, please WIP again.

  • L
    Software for LHCb @lhcbsoft ·
    Edited by Software for LHCb
  • Rosen Matev mentioned in merge request lhcb/LHCb!1422 (merged)

    mentioned in merge request lhcb/LHCb!1422 (merged)

  • Marco Clemencic mentioned in issue #43

    mentioned in issue #43

  • Marco Clemencic assigned to @leggett

    assigned to @leggett

  • Marco Clemencic changed milestone to %v30r4

    changed milestone to %v30r4

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply