This is an archive of the discontinued LLVM Phabricator instance.

Adding functionality to Stack Tracing
ClosedPublic

Authored by dibya001 on Aug 6 2020, 11:21 AM.

Details

Summary
Added optional Depth parameter to PrintStackTrace() to allow the client code to print stacktrace upto
    a particular level. If not provided, then the default behaviour is same asprevious i.e to print all the trace
    information.

Diff Detail

Event Timeline

dibya001 created this revision.Aug 6 2020, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 11:21 AM
dibya001 requested review of this revision.Aug 6 2020, 11:21 AM
dibya001 edited the summary of this revision. (Show Details)

Formatting the changes

Hello @dibya001! Thanks for the patch!

Could you please explain your usage?

You would also need to add test coverage (in SupportTests maybe).

llvm/include/llvm/Support/PrettyStackTrace.h
99

Please explain why this is needed. How would things work out in your code if neither setArgs nor the empty constructor were there?

llvm/lib/Support/Windows/Signals.inc
556 ↗(On Diff #283687)

Implement this for Windows as well or add a // FIXME: if you can't.

dibya001 added inline comments.Aug 10 2020, 10:50 AM
llvm/include/llvm/Support/PrettyStackTrace.h
99

I want to to declare PrettyStackTraceProgram's object globally so that I have access to it in other portions of code. While declaring globally I don't have argc and argv, Hence I need this default constructor. And in main, I will call the setArgs with argc and argv values

dibya001 marked an inline comment as done.
aganea added inline comments.Aug 10 2020, 11:09 AM
llvm/include/llvm/Support/PrettyStackTrace.h
99

PrettyStackTraceProgram is just meant as a deferred instruction to the crash handler, telling it to print our custom stack of operations. It is not meant to be used explictly. The changes here seem specific to your use-case.
How do you use in other parts of the code? I suppose you want .print()? I'm trying to understand what is your underlying need.

In your code, could you instead do:

PrintStackTraceProgram *GlobalX{};
int main(int argc, const char **argv) {
  PrintStackTraceProgram X(argc, argv);
  GlobalX = &X;
}
// --- in another .CPP file
PrintStackTraceProgram *GlobalX;
int foo() { GlobalX->print(...); }

And avoid the changes here altogether?

dibya001 added inline comments.Aug 10 2020, 12:10 PM
llvm/include/llvm/Support/PrettyStackTrace.h
99

Thank you for the suggestion. This will help in my use case. I will update the patch.

dibya001 edited the summary of this revision. (Show Details)Aug 10 2020, 12:15 PM

Code looks good, but I'm still wondering if we could add test coverage? Can we test for PrintStackTrace(StrOS, 1); and validate that only one line is emitted? You could add such a test in one of the files in llvm/unittests/Support/ - or add a new test file if it doesn't fit anywhere. Also don't forget to skip Windows OS (see the Host.cpp test for an example).

Thanks for the update, almost good, see below.
Also, could you please explain what is your practical application for chopping the callstack?

llvm/unittests/Support/CrashRecoveryTest.cpp
94 ↗(On Diff #284685)

We're trying to avoid platform-specific includes as much as possible. In this case, you would still want to compile the code on all platforms, but dynamically skip the parts that don't work yet. So in essence, please remove the #if / #endif lines, and enclose the EXPECT_EQ into a test:

// FIXME: (instruct to fix the Windows implementation for PrintStackTrace)
if (!Triple(sys::getProcessTriple()).isOSWindows())
  EXPECT_EQ(...)
94 ↗(On Diff #284685)

I meant platform-specific defines, not includes.

In some cases we would like to show just the call stack of the llvm pass that crashed, and not the complete call stack of the application which could be very big. And also "optionally" print other information for better diagnostics when the crash happens

dibya001 marked 2 inline comments as done.
aganea accepted this revision.Aug 11 2020, 9:12 AM

Change LGTM, do you have commit access? You should perhaps request it if you're planning on further patches.

In some cases we would like to show just the call stack of the llvm pass that crashed, and not the complete call stack of the application which could be very big.

How do you decide on the depth of the callstack? Depending on where the crash occurs, the pass entry (runOnFunction) could be at different depth? How do you handle that?

And also "optionally" print other information for better diagnostics when the crash happens

Ah yes, that would be really nice. You mean adding more llvm::PrettyStackTraceEntry contexts?

llvm/unittests/Support/CrashRecoveryTest.cpp
95 ↗(On Diff #284767)

Minor: res and str below should start with an uppercase.

This revision is now accepted and ready to land.Aug 11 2020, 9:12 AM

Change LGTM, do you have commit access? You should perhaps request it if you're planning on further patches.
No, I dont have access as of now.

In some cases we would like to show just the call stack of the llvm pass that crashed, and not the complete call stack of the application which could be very big.

How do you decide on the depth of the callstack? Depending on where the crash occurs, the pass entry (runOnFunction) could be at different depth? How do you handle that?
So from the caller function, I would store the stack_trace depth value from the backtrace function. let's say the value is x1 and when the crash happens., in the signal/crash handler, I would get the depth at the postiton(let's say the value is x2). So in the PrintStackTrace() func I will pass the depth as x2-x1+1

And also "optionally" print other information for better diagnostics when the crash happens

Ah yes, that would be really nice. You mean adding more llvm::PrettyStackTraceEntry contexts?

Yes. Or in a simple case, we could print using the PrettyStackTraceProgram-print() function.

Added missing Headers which was incorrectly missed last time during amending of commit

dibya001 marked an inline comment as done.Aug 11 2020, 10:15 PM

As I don't have commit access, could someone please help in merging it?

As I don't have commit access, could someone please help in merging it?

I will, but I was waiting a few days see if there are other comments.

This revision was automatically updated to reflect the committed changes.