[PATCH] D70600: [Error] Add stack traces for llvm::Error invariant violations.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D70600#1803899 <https://reviews.llvm.org/D70600#1803899>, @hintonda wrote:

> In D70600#1803789 <https://reviews.llvm.org/D70600#1803789>, @dblaikie wrote:
>
> > In D70600#1802516 <https://reviews.llvm.org/D70600#1802516>, @sammccall wrote:
> >
> > > In D70600#1802227 <https://reviews.llvm.org/D70600#1802227>, @lhames wrote:
> > >
> > > > In D70600#1801726 <https://reviews.llvm.org/D70600#1801726>, @sammccall wrote:
> > > >
> > > > > FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?
> > > > >
> > > > > thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.
> > > >
> > > >
> > > > An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.
> > >
> > >
> > > I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
> > >  Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.
> > >
> > > Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning `Expected<T>` and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.
> >
> >
> > Note this feature's only intended to be opt-in for debugging purposes, so for production use cases it shouldn't be a huge deal (though could be cheaper/free if it was a build-time setting rather than something you could opt into/out of at runtime - perhaps that's the point you're making? Though we do check various globals (cl::opts, etc) in a fair number of codepaths, I think, such that adding one doesn't seem likely to be super problematic?). (as much as I agree with you that I'd rather not have a side table)
>
>
> Part of the reason for this patch was a need to provide file/line number for errors that lead to aborts via llvm::cantFail.  This can be very important when debugging failing bots for which you have not access and are unable to reproduce.  Since this solution is too expensive to turn on all the time, it's doubtful it'll be enabled on any bots, which makes it useless for my primary use case.
>
> That doesn't mean it wouldn't be useful in cases where you can reproduce locally, but that wasn't my initial motivation.  I'm also wary of the side-table.


Sorry - I meant opt-in for production use cases. I think it'd make a lot of sense to enable it for buildbots (same way that we enable assertions on buildbots - but you'd never want assertions enabled in your clang production build, it'd be much too slow). But I'd like to see some functionality to specifically disable all crash symbolication for crash-expected test cases (GUnit EXPECT_DEATH) - those crashes aren't reported anyway (gunit swallows the output of the crashing program).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70600/new/

https://reviews.llvm.org/D70600