[cfe-dev] [llvm] r193937 - When LLVM is embedded in a larger application, it's not OK for LLVM to intercept crashes. LLVM already has

[Alp Toker]

On 03/11/2013 04:18, Filip Pizlo wrote:
> On Nov 2, 2013, at 9:15 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>> Brilliant. Could you revert the API addition of
>> LLVMDisablePrettyStackTrace() in r193937 while we figure this out?
>
> I'd rather not.  Without this function, it's impossible for a C API
> client to embed LLVM.
>
> I agree that there is a better solution out there, but I'd rather have
> this as a stop-gap until then.  Is there any real harm to having this
> in the tree in the meantime?  Because there is clear harm on my end
> from removing it - namely, the usual crash recording machinery for OS
> X applications doesn't work as soon as LLVM is embedded.

I'm concerned that with this function now in the C API and documented,
others tracking SVN including other LLVM subprojects will start using
it, adding a burden of support and making it difficult to implement a
proper fix.

After all this clearly should have been the default all along :-)

So either let's amend the comment in the C header and work on the Enable
solution quickly or let's revert for that reason.

FWIW we've also been patching LLVM for a long time to fix this, so
couldn't you hold on a day or two longer to get the right fix in place
for OS X?

Alp.



>
> -Filip
>
>
>>
>> The new approach will be a step towards cleaning up libclang as well:
>>
>> tools/libclang/CIndex.cpp-  // Disable pretty stack trace functionality,
>> which will otherwise be a very
>> tools/libclang/CIndex.cpp-  // poor citizen of the world and set up all
>> sorts of signal handlers.
>> tools/libclang/CIndex.cpp:  llvm::DisablePrettyStackTrace = true;
>>
>> Thanks for noticing the issue and starting on a solution!
>>
>> Alp.
>>
>>
>> On 03/11/2013 04:10, Filip Pizlo wrote:
>>>
>>> On Nov 2, 2013, at 9:07 PM, Alp Toker <alp at nuanti.com
>>> <mailto:alp at nuanti.com>
>>> <mailto:alp at nuanti.com>> wrote:
>>>
>>>> Could we make this the default, then provide
>>>> LLVMEnablePrettyStackTrace() instead?
>>>
>>> I like this.
>>>
>>>>
>>>> By definition the C API is for embedding, and embedders don't expect
>>>> each API they use to install global signal handlers.
>>>
>>> Right, and we could also do the same thing for the fatal error
>>> handler.  The default fatal error handler does exit(1), which is
>>> clearly not right for embedders.
>>>
>>> It seems like both the pretty stack traces and the fatal error handler
>>> could be manually enabled by those clients that depend on it.
>>>
>>>>
>>>> Alp.
>>>>
>>>>
>>>> On 03/11/2013 00:29, Filip Pizlo wrote:
>>>>> Author: fpizlo
>>>>> Date: Sat Nov  2 19:29:47 2013
>>>>> New Revision: 193937
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=193937&view=rev
>>>>> Log:
>>>>> When LLVM is embedded in a larger application, it's not OK for LLVM
>>>>> to intercept crashes.  LLVM already has 
>>>>> the ability to disable this functionality.  This patch exposes it
>>>>> via the C API.
>>>>>
>>>>>
>>>>> Modified:
>>>>>   llvm/trunk/include/llvm-c/Core.h
>>>>>   llvm/trunk/lib/Support/PrettyStackTrace.cpp
>>>>>
>>>>> Modified: llvm/trunk/include/llvm-c/Core.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm-c/Core.h?rev=193937&r1=193936&r2=193937&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm-c/Core.h (original)
>>>>> +++ llvm/trunk/include/llvm-c/Core.h Sat Nov  2 19:29:47 2013
>>>>> @@ -434,6 +434,12 @@ void LLVMInstallFatalErrorHandler(LLVMFa
>>>>> void LLVMResetFatalErrorHandler(void);
>>>>>
>>>>> /**
>>>>> + * Disable LLVM's built-in stack trace code. This must be called
>>>>> before any
>>>>> + * other LLVM APIs; otherwise the results are undefined.
>>>>> + */
>>>>> +void LLVMDisablePrettyStackTrace(void);
>>>>> +
>>>>> +/**
>>>>> * @defgroup LLVMCCoreContext Contexts
>>>>> *
>>>>> * Contexts are execution states for the core LLVM IR system.
>>>>>
>>>>> Modified: llvm/trunk/lib/Support/PrettyStackTrace.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/PrettyStackTrace.cpp?rev=193937&r1=193936&r2=193937&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Support/PrettyStackTrace.cpp (original)
>>>>> +++ llvm/trunk/lib/Support/PrettyStackTrace.cpp Sat Nov  2
>>>>> 19:29:47 2013
>>>>> @@ -20,6 +20,7 @@
>>>>> #include "llvm/Support/ThreadLocal.h"
>>>>> #include "llvm/Support/Watchdog.h"
>>>>> #include "llvm/Support/raw_ostream.h"
>>>>> +#include "llvm-c/Core.h"
>>>>>
>>>>> #ifdef HAVE_CRASHREPORTERCLIENT_H
>>>>> #include <CrashReporterClient.h>
>>>>> @@ -147,3 +148,7 @@ void PrettyStackTraceProgram::print(raw_
>>>>>    OS << ArgV[i] << ' ';
>>>>>  OS << '\n';
>>>>> }
>>>>> +
>>>>> +void LLVMDisablePrettyStackTrace() {
>>>>> +  DisablePrettyStackTrace = true;
>>>>> +}
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> <mailto:llvm-commits at cs.uiuc.edu> <mailto:llvm-commits at cs.uiuc.edu>
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>> -- 
>>>> http://www.nuanti.com <http://www.nuanti.com/> <http://www.nuanti.com/>
>>>> the browser experts
>>>
>>
>> -- 
>> http://www.nuanti.com <http://www.nuanti.com/>
>> the browser experts
>

-- 
http://www.nuanti.com
the browser experts