Thanks for looking into this.

Thanks for looking into this. I'll take a look this afternoon.

LGTM, thanks for fixing this...

LGTM...

In D42459#2383753, @ldionne wrote:

@hintonda Is there a reason why this was done except for "standardizing the usage across projects"?

@lhames took care of this in e9e26c01cd865da678b1af6ba5f417c713956a66 yesterday.

post holiday ping...

post holiday ping...

In D70600#1803789, @dblaikie wrote:

In D70600#1802516, @sammccall wrote:

In D70600#1802227, @lhames wrote:

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.

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)

In D71438#1787248, @MaskRay wrote:

I recall that you have a patch that touched this area, which was committed and reverted for several times. Is this patch a similar one? Can you find pointers to the old one?

• Also move over DisableSymbolication option from Signals.cpp, and put all variables in llvm namespace.

In D71228#1778285, @hintonda wrote:

In D71228#1777711, @serge-sans-paille wrote:

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

When you statically link in LLVMSupport, you only load what you reference. In the case of Debug options, they are referenced all over, so that TU gets loaded. If it also included static Options, which register themselves with the global parser in CommandLine.cpp, that TU gets loaded as well. By moving the Debug options over to CommandLine.cpp, those statics won't be loaded unless the TU is loaded, which will only happened if the user calls function in CommandLine.cpp.

We have a case where we use functionality in LLVMSupport, but don't call parseCommandLineOptions. We can still access the DebugFlag, etc, but don't actually need the option since it is only used by the parser -- those options use cl::location, so the global already exists. Hope that makes sense.

In D71228#1777711, @serge-sans-paille wrote:

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

• Remove unneeded cl::ValueRequired, since it is the default

• Address comments

• Rename traits and group asserts for readability. Add example to help.

• Add static_asserts for arg_type

• Extract arg type from callback so caller won't need to provide it.

• Address comments.

• Remove void* and templatize callback struct.

In D70628#1762116, @hintonda wrote:

In D70628#1761330, @lhames wrote:

In D70628#1758031, @hintonda wrote:

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

I'm not sure whether there is a cmake option to generate dSYMs yet -- you'll have to run dsymutil on whatever tool you're testing with.

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.

In D70628#1761330, @lhames wrote:

In D70628#1758031, @hintonda wrote:

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

I'm not sure whether there is a cmake option to generate dSYMs yet -- you'll have to run dsymutil on whatever tool you're testing with.

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.

• Remove unnecessary casts.

I'd just like to acknowledge a few issues with this patch, and request feedback:

In D70628#1757860, @lhames wrote:

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?

Did you generate a dSYM for the binary with dsymutil? Unfortunately llvm-symbolize doesn't seem to know how to symbolize directly from objects containing debug info, but I'm hoping we can eventually teach it to do that.

• Fixed typos.

• Add support for callbacks for Options, and remove cl::implied, since they are now OBE.

In D70620#1757692, @serge-sans-paille wrote:

This looks nice. I think it's missing an entry somewhere in the user doc (so that people can start using it!).

This is awesome, thanks for working on it!

• Fix test case order.

Switch to builder.

@lhames, I think I've applied all your verbal changes, but please let me
know if I missed anything.

Append "_annotated" suffix to detail functions.
Add "llvm_" prefix to macros.
Revert changes to all non-llvm project files.
Update calls in llvm to use new macro.

• Add back original llvm::cantFail signatures so they'll still be

• Replace macro magic with matching 3-parameter template functions.
• Refactor cantFail move into detail namespace. Change macro name to

In D70263#1748105, @lhames wrote:

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?

• Remove SourceLocationError class and reuse FileError.

• Replace macro magic with matching 3-parameter template functions.

LGTM, but I'll wait a few days to land it for you to give @Bigcheese a chance to comment.

I'll go ahead and land this for you tomorrow unless @Bigcheese has any objections.

I see you haven't landed this yet. Do you have commit rights? If not, I'd be happy to land it for you.

LGTM, thanks for the patch!

In D69501#1723723, @dsprenkels wrote:

@jhenderson, I added the test.

The only person that has modified llvm/lib/Support/CommandLine.cpp a couple of times in the last year is @hintonda. I added them as a reviewer.

• Wrap new login in !NDEBUG.

In D62445#1630518, @NoQ wrote:

I didn't make much progress so far, but i marked the test as // UNSUPPORTED: darwin in rC368765, so the buildbot is now green starting with http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6519/.

In D59754#1651894, @rsmith wrote:

I'm taking this over to finish it off and submit.

In D62445#1613268, @NoQ wrote:

Ugh, there seems to be one more forgotten buildbot with plugins problems: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6406/consoleText

<snip>

It got suddenly fixed in http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6409/ but then immediately failed again later and it's still failing in a similar manner, and nobody noticed for two months =/

In D62445#1613268, @NoQ wrote:

Ugh, there seems to be one more forgotten buildbot with plugins problems: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6406/consoleText

FAIL: Clang :: Analysis/checker-plugins.c (467 of 14955)
******************** TEST 'Clang :: Analysis/checker-plugins.c' FAILED ********************
Script:
--
: 'RUN: at line 3';   /Users/buildslave/jenkins/workspace/clang-stage2-cmake-RgSan/clang-build/bin/clang -cc1 -internal-isystem /Users/buildslave/jenkins/workspace/clang-stage2-cmake-RgSan/clang-build/lib/clang/9.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -verify /Users/buildslave/jenkins/workspace/clang-stage2-cmake-RgSan/llvm/tools/clang/test/Analysis/checker-plugins.c    -load /Users/buildslave/jenkins/workspace/clang-stage2-cmake-RgSan/clang-build/./lib/SampleAnalyzerPlugin.dylib    -analyzer-checker='example.MainCallChecker'

<snip>

Exit Code: 1

The error message doesn't seem to be very expressive. I also can't reproduce it locally.

It got suddenly fixed in http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6409/ but then immediately failed again later and it's still failing in a similar manner, and nobody noticed for two months =/

I'll keep looking into this but i'd love to hear if you have any immediate thoughts on that :)

In D62455#1576306, @beanz wrote:

One thought, we should probably evaluate why those tools are using add_library instead of add_llvm_library. The right fix might be to move them over. This is exactly the kind of behavior that add_llvm_library and llvm_add_library were designed to handle.

Make GeneralCategory a ManagedStatic.

Patch is ready, so I'll post as soon as I complete rebase/build/test.

Reopening to track fix for buildbot failure -- need to make GeneralCategory a ManagedStatic.