Adding functionality to Stack Tracing

[• dibya001]

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.

[• dibya001]

Formatting the changes

[• dibya001]

ping ?

[aganea]

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]

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

[aganea]

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]

llvm/include/llvm/Support/PrettyStackTrace.h

99

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

[aganea]

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

[aganea]

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.

[• dibya001]

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

[aganea]

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

In D85458#2210411, @dibya001 wrote:

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.

[• dibya001]

In D85458#2210475, @aganea wrote:

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 D85458#2210411, @dibya001 wrote:

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.

[• dibya001]

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

[• dibya001]

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

[aganea]

In D85458#2213234, @dibya001 wrote:

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.

[• dibya001]

ping ?

llvm/include/llvm/Support/PrettyStackTrace.h

     int ArgC;
     const char *const *ArgV;
   public:
+    PrettyStackTraceProgram() : ArgC(0), ArgV(nullptr) {
+      EnablePrettyStackTrace();
+    }
+
+    void setArgs(int argC, const char *const *argV) {
+      this->ArgV = argV;
+      this->ArgC = argC;
+    }
     PrettyStackTraceProgram(int argc, const char * const*argv)
       : ArgC(argc), ArgV(argv) {
       EnablePrettyStackTrace();

llvm/include/llvm/Support/Signals.h

   void DisableSystemDialogsOnCrash();
 
   /// Print the stack trace using the given \c raw_ostream object.
-  void PrintStackTrace(raw_ostream &OS);
+  /// optional Depth parameter refers to the number of stackframes
+  /// to print. If not specified, entire frame is printed.
+  void PrintStackTrace(raw_ostream &OS, int Depth = 0);
 
   // Run all registered signal handlers.
   void RunSignalHandlers();

llvm/lib/Support/Unix/Signals.inc

 //
 // On glibc systems we have the 'backtrace' function, which works nicely, but
 // doesn't demangle symbols.
-void llvm::sys::PrintStackTrace(raw_ostream &OS) {
+void llvm::sys::PrintStackTrace(raw_ostream &OS, int Depth) {
 #if ENABLE_BACKTRACES
   static void *StackTrace[256];
   int depth = 0;

 #endif
   if (!depth)
     return;
-
-  if (printSymbolizedStackTrace(Argv0, StackTrace, depth, OS))
+  // If "Depth" value is not provided by the caller, assign
+  // return value of backtrace() i.e. "depth" to "Depth"
+  // which will be then used for printing symbolized stack trace.
+  if (!Depth)
+    Depth = depth;
+  if (printSymbolizedStackTrace(Argv0, StackTrace, Depth, OS))
     return;
 #if HAVE_DLFCN_H && HAVE_DLADDR
   int width = 0;

     OS << '\n';
   }
 #elif defined(HAVE_BACKTRACE)
-  backtrace_symbols_fd(StackTrace, depth, STDERR_FILENO);
+  backtrace_symbols_fd(StackTrace, Depth, STDERR_FILENO);
 #endif
 #endif
 }