[PATCH] Change pretty stack trace signal handler to being an opt-in feature rather than an opt-out feature

[Filip Pizlo]

On Nov 3, 2013, at 11:07 AM, Alp Toker <alp at nuanti.com> wrote:

> Hello Filip,
> 
> Very tidy solution, thanks for putting in the time.
> 
> 
> Index: tools/libclang/CIndex.cpp
> ===================================================================
> --- tools/libclang/CIndex.cpp    (revision 193960)
> +++ tools/libclang/CIndex.cpp    (working copy)
> @@ -2544,10 +2544,6 @@
>  extern "C" {
>  CXIndex clang_createIndex(int excludeDeclarationsFromPCH,
>                            int displayDiagnostics) {
> -  // Disable pretty stack trace functionality, which will otherwise be a very
> -  // poor citizen of the world and set up all sorts of signal handlers.
> -  llvm::DisablePrettyStackTrace = true;
> -
>    // We use crash recovery to make some of our APIs more reliable, implicitly
>    // enable it.
>    llvm::CrashRecoveryContext::Enable();
> 
> 
> You can remove the old PrettyStackTrace.h include here as well.

Good point!

> 
> Also lldb will need patching:
> 
> source/Expression/ClangExpressionParser.cpp:            llvm::DisablePrettyStackTrace = true;

OK - I will make sure I change lldb, also.

> 
> Otherwise I think this is ready to go

Thanks for the review!

-Filip

> 
> Alp.
> 
> 
> On 03/11/2013 17:17, Filip Pizlo wrote:
>> Hi!
>> 
>> Currently some large subset of LLVM embedders are expected to call to set DisablePrettyStackTrace to true to disable LLVM's pretty stack trace printing.
>> 
>> Probably, it would be even better to flip this around: if you want pretty stack traces, you should ask for them explicitly.  This patch implements just such behavior, and does it in a way that I believe requires minimal changes from LLVM clients.
>> 
>> Previously the pretty stack trace signal handler would be installed whenever a PrettyStackTraceEntry was constructed, unless the DisablePrettyStackTrace variable was set to true.  The C API would set this variable to true by calling LLVMDisablePrettyStackTrace().  Now, the signal handler is only installed if you call llvm::EnablePrettyStackTrace().
>> 
>> At first glance this would seem to imply that any LLVM client that does not currently set DisablePrettyStackTrace to true should now add a call to llvm::EnablePrettyStackTrace().  But I believe this patch avoids this: instead, I've made PrettyStackTraceProgram's constructor call EnablePrettyStackTrace().  PrettyStackTraceProgram is never used in LLVM library code - only in LLVM tools and in clients (like clang).  I believe that this automatically gives us the behavior we want:
>> 
>> - Existing LLVM-based tools continue to have pretty stack traces even though I didn't have to change any of their code.
>> 
>> - LLVM embedders that use the C API will not get pretty stack traces anymore unless they call LLVMEnablePrettyStackTrace().  I believe that this is a good thing, since if embedders were getting pretty stack traces just by calling into the LLVM C API, then that was probably a bug.
>> 
>> - LLVM embedders that use the C++ API will not get pretty stack traces unless they either use PrettyStackTraceProgram or call llvm::EnablePrettyStackTrace().
>> 
>> I also include a corresponding clang patch, which removes CIndex's use of DisablePrettyStackTrace.
>> 
>> -Filip
>> 
>> 
> 
> -- 
> http://www.nuanti.com
> the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131103/225d2824/attachment.html>