LGTM! I really like this. I'm all for adding it as another example if you're happy to maintain it.

LGTM. Please go ahead and commit. If you do not have commit access just let me know and I can commit on your behalf.

Hi Axis,

Actually, since I'm pontificating about Error on this thread anyway, the other big change that I would like to see (which is also impacted by ErrorList) would be to ditch handleErrors entirely. If we had the "one Error/Expected == one failure" invariant, we could switch to an "err_cast" idiom:

This seems like a nice change for ErrorList.

I need to check this out and play around with it a bit to wrap my head around it, especially the performance tuning issues. This is very exciting though!

I think this would be better explained through comments. There is a comment regarding this code sequence at the top of the function, but it would make sense to add a second one directly above where we use the constant.

This landed in d2751f8fdf6c072045bab62f6035511e028f46ee (I failed to format the revision number correctly in the commit message, so it wasn’t automatically picked up here, sorry).

The current workaround creates and copies a std::shared_pointer. That's quite expensive. In a first step we can move-capture it in the Work lambda to avoid the copy. I think we don't get around the creation (assuming we keep a smart pointer), so why not receive it as a std::shared_ptr directly? The only disproportionate overhead I see is in the materializeOnCurrentThread case. I changed that to not calling the function at all.

Actually I missed that clone-to-new-context update in e9e26c01cd865da678b1af6ba5f417c713956a66. I've fixed it in c75180258e6. Thanks for this Don!

In rGece8fed609061638bc867ecb74e179cb86426d60#884349, @sgraenitz wrote:

I was about to say enjoy your time off and have a look here on Monday: D72563 I guess we don't need it anymore :)

Hi Stefan,

Hi Stefan. Thanks for posting this! I’m out on vacation this week, but looking forward to reviewing when I get back to the office next week.

LGTM. Do you have commit access? If not just let me know and I can commit on your behalf.

In D70628#1762343, @hintonda wrote:

In D70628#1762116, @hintonda wrote:

In D70628#1761330, @lhames wrote:

...
I think the best long term solution to this problem is to teach llvm-symbolize how to cope with debug info in objects, rather than requiring a dSYM. An intermediate step (possibly generically useful) would be to add an option to llvm-symbolize to run llvm-dsymutil if there's no dSYM available.

I suppose that might be useful for individuals, but I don't think it would be a good idea for the bots, which is my primary target.

My last comment concerned the intermediate step. I think your long-term solution would be great.

Oh, I remember part of my motivation for designing it this way: While failure values are expected to be expensive, we don't want to make them unnecessarily expensive, which is why the error-geneation-point trace is under a flag: You don't want to stack-trace every error that is ever generated. However, making this conditional means we need to check a global flag to decide whether we *should* do the stack trace. Once you're doing that, you may as well make your "flag" a pointer to the side table.

In 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.

In D71683#1793698, @myhsu wrote:

In D71683#1793661, @lhames wrote:

Actually, are you seeing this bug on ORC? Or just on MCJIT?

ORC, specifically ORCv2.

LGTM.

Actually, are you seeing this bug on ORC? Or just on MCJIT?

Hi myhsu -- This looks good to me. Do you have commit access? If so please feel free to commit. If not just let me know and I will commit on your behalf.

In D71397#1784325, @teemperor wrote:

I would prefer landing this as-is because it is just doing the same we do in other places in LLVM. Afterwards migrating the whole RTTI system in LLVM to a new system (whether that will be D39111 or just a ClassID type) sounds good to me.

Side note: This https://reviews.llvm.org/D39111 seems related. The patch has languished as I have been busy with other work, but if it would be useful for you guys I'd be happy to try to land it.

In D70628#1758031, @hintonda wrote:

Just a standard llvm Debug build. Is there an cmake option I'm missing?

Coming to this a bit late -- Thanks for working on this kaadam! And thanks for the review mstorsjo.

In D70628#1757813, @hintonda wrote:

This is awesome, thanks for working on it!

I pulled down your patch and built it, but am not getting expected results. Did I miss something?

• Remove check for an impossible magic value.

• Remove byte-swapping code. This will run in process, so endianness shouldn't
• Fix variable naming.

• Remove byte-swapping code. This will run in process, so endianness shouldn't

Looking for feedback on the approach before I go too much further with this. I was motivated to enable file + line info by Don Hinton's work in https://reviews.llvm.org/D70259 and https://reviews.llvm.org/D70263, where he attaches source location information certain calls to aid in debugging of error handling failures. I have a review out for an alternative scheme that produces full backtraces for error handling failures (see https://reviews.llvm.org/D70600), but the lack of file and line info in the traces is a drawback compared to Don's approach.

• Replace report_fatal_error declaration that was accidentally removed.

In D70263#1748442, @hintonda wrote:

In D70263#1748105, @lhames wrote:

In my initial commit, I had added a new class specifically for this, SourceLocationError, but @beanz convinced me to reuse FileError, since the only difference was the message formatting.

Should I add back SourceLocationError?

I'm not sure I follow this. My understanding is that FileError was designed to make it easy to attach a source location to an error generated while operating on a file, but this patch is adapting it to track the LLVM source location that generated the error, right?

Hi Don,

Ok. So the documentation (sparse as it is) syncs up with what you're seeing in existing implementations. Looks good to me. :)

I couldn’t reproduce this -DLLVM_ENABLE_MODULES=On and my system compiler. I’ll try a 2-stage build and see if I have any luck with that.

Looks sane to me, but I'm not a cfi_signal_frame expert.

In D69783#1733210, @wyjw wrote:

No I do not have commit access. Please help me commit this.
Thanks for your help!

I think Chris is right. This isn’t an ideal breakdown of the library, but it’s a step in the right direction: OrcError should should only depend on Support (not the rest of ExecutionEngine), and ExecutionEngine should depend on OrcError.

Looks good to me. Thanks Jian!

LGTM. Thanks Alexandre!