Brilliant. Could you revert the API addition of
LLVMDisablePrettyStackTrace() in r193937 while we figure this out?
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.
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.
-Filip
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.
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.
I believe that in between LLVM releases, the C API is expected to be unstable. We can change our minds about it.
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.
I will amend the comment.
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?
I certainly could. I’m just playing out the trade-offs and I don’t see the upside of reverting. I think we agree that we will replace this API with something more thought-out. So, we agree that this new function of mine will have a short shelf-life and should not make it into a release. We can either leave my horrible function in svn for the next few days, or we can revert it. Leaving it in has the downside that maybe others will take a liking to it. Reverting it means more hassle for C API clients, like me. Seems like leaving it in is the lesser evil?
-Filip
Filip, as long as you're committed to following this up -- amending the
comment now and removing the function in the next few days, this is OK
with me as long as others don't object.
FWIW I'll be around to help test as soon as you have an Enable() patch.
Most of all, please make sure the function doesn't inadvertently end up
in 3.4 (!) or it'll be the wrong outcome to a long-standing bug.
Alp.
I certainly could. I’m just playing out the trade-offs and I don’t
see the upside of reverting. I think we agree that we will replace
this API with something more thought-out. So, we agree that this new
function of mine will have a short shelf-life and should not make it
into a release. We can either leave my horrible function in svn for
the next few days, or we can revert it. Leaving it in has the
downside that maybe others will take a liking to it. Reverting it
means more hassle for C API clients, like me. Seems like leaving it
in is the lesser evil?
Filip, as long as you’re committed to following this up – amending the
comment now and removing the function in the next few days, this is OK
with me as long as others don’t object.
Thank you!
Here’s my proposal for how to do it:
- Enabling pretty stack tracing is done via an explicit function call, which has pthread_once-like behavior (you can call it many times and it’s idempotent).
- PrettyStackTraceProgram will call this function.
- The function will be exposed via the C API and DisablePrettyStackTrace will be removed.
This should mean that none of LLVM’s command-line tools should need any changes, since AFAICT, they all do PrettyStackTraceProgram.
Does this sound like the right thing for clang and others? If so, I can hack up the patch.
-Filip
I certainly could. I'm just playing out the trade-offs and I don't
see the upside of reverting. I think we agree that we will replace
this API with something more thought-out. So, we agree that this new
function of mine will have a short shelf-life and should not make it
into a release. We can either leave my horrible function in svn for
the next few days, or we can revert it. Leaving it in has the
downside that maybe others will take a liking to it. Reverting it
means more hassle for C API clients, like me. Seems like leaving it
in is the lesser evil?
Filip, as long as you're committed to following this up -- amending the
comment now and removing the function in the next few days, this is OK
with me as long as others don't object.
Thank you!
Here's my proposal for how to do it:
- Enabling pretty stack tracing is done via an explicit function call,
which has pthread_once-like behavior (you can call it many times and it's
idempotent).
- PrettyStackTraceProgram will call this function.
- The function will be exposed via the C API and DisablePrettyStackTrace
will be removed.
This *should* mean that none of LLVM's command-line tools should need any
changes, since AFAICT, they all do PrettyStackTraceProgram.
Does this sound like the right thing for clang and others? If so, I can
hack up the patch.
This sounds totally reasonable. Probably some docs updates for
PrettyStackTraceProgram et al saying that it needs to be called if you want
pretty stack traces.
-eric