[llvm-commits] [PATCH] proposed fix for CR # 12109 - Registration of CrashHandler in lib/Support/PrettyStackTraceEntry::PrettyStackTraceEntry() not thread-safe causing crash

[Igor Boehm]

Hello,

Registering a signal handler as a side-effect of a constructor
as it is done in PrettyStackTraceEntry::PrettyStackTraceEntry()
is sub-optimal, and probably unexpected behaviour for the user
of such a class.

The other problem is the fact that the following is undefined in a
multi-threaded context according to the current C++ standard and causes
problems (see http://llvm.org/bugs/show_bug.cgi?id=12109):

----------------------------------------
PrettyStackTraceEntry::PrettyStackTraceEntry() {
 // The first time this is called, we register the crash printer.
 static bool HandlerRegistered = RegisterCrashPrinter();
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 (void)HandlerRegistered;

 // Link ourselves.
 NextEntry = PrettyStackTraceHead.get();
 PrettyStackTraceHead.set(this);
}
----------------------------------------

See  http://stackoverflow.com/questions/2280630/c-threadsafe-static-constructor
for a discussion.

Registration of a CrashHandler should be done explicitly, similar to the way crash
recovery in the CrashRecoveryContext can be enabled via CrashRecoveryContext::Enable().

In the case of registering a custom CrashHandler for PrettyStackTraceEntries it
should also be possible to explicitly disable the registration of a CrashHandler
if the host application wants to install its own signal handlers (libclang/CIndex.cpp
is a candidate that does this). 

The attached patches remove the registration of a CrashHandler from the
PrettyStackTraceEntry constructor and introduce a PrettyStackTraceCrashHandler
class for explicit registration:

#include "llvm/Support/PrettyStackTrace.h"
llvm::PrettyStackTraceCrashHandler::Register(); // thread-safe CrashHandler registration

This will probably cause some controversy as the CrashHandler must now be
explicitly registered in contrast to the current implementation that does this
in a non thread-safe way when constructing the first PrettyStackTraceEntry
instance.

So this probably needs further discussion.

Thanks,
Igor

Full CR details available at: http://llvm.org/bugs/show_bug.cgi?id=12109

-------------- next part --------------
A non-text attachment was scrubbed...
Name: trunk_Support_PrettyStackTrace.patch
Type: application/octet-stream
Size: 4450 bytes
Desc: trunk_Support_PrettyStackTrace.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120229/a41ccdea/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libclang_CIndex.patch
Type: application/octet-stream
Size: 635 bytes
Desc: libclang_CIndex.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120229/a41ccdea/attachment-0001.obj>