This feels like a nice generalization (and I like the shape of the new code better), but I'm wondering if it's not too general. For example a DT_AT_location can be DW_FORM_addr, and applying PCOffset to it would be wrong. I'm not sure what PCOffset is set to in a variable DIE, but it does feel somewhat wrong even if it's 0. I cannot be 100% sure, but I believe that fear of mishandling non-PC addresses was the original thinking that led to spelling out all the supported cases.

In D136651#4067012, @glandium wrote:

Oh yes, we're using -DLLVM_LINK_LLVM_DYLIB=ON too, so that would be the main trigger.

In D136651#4066960, @mstorsjo wrote:

In D136651#4066949, @friss wrote:

Thanks for the report, unfortunately I have no way of testing this setup.

Can you try a build with -DLLVM_LINK_LLVM_DYLIB=ON?

In D136651#4064474, @glandium wrote:

In D136651#4064260, @glandium wrote:

This broke our mac builds with errors like:

ld64.lld: error: undefined symbol: CFRunLoopRun
>>> referenced by tools/clang/tools/clang-stat-cache/CMakeFiles/clang-stat-cache.dir/clang-stat-cache.cpp.o:(symbol main+0x11be)

(and many more symbols)

Additional information: those are cross-compiled, and for some reason the -framework CoreServices in the CMakeLists.txt doesn't seem to make it to the command line, which is surprising because something similar works fine for e.g. dsymutil.

In D136651#4063632, @lebedev.ri wrote:

In D136651#4063597, @friss wrote:

In D136651#4060071, @lebedev.ri wrote:

Docs still missing.

Sorry @lebedev.ri I missed your earlier comment about this. What format of doc would you like to see? A more elaborate comment in the tool's source or something else?

Right now it's impossible to discover that tool unless you already know it exists (or it's mentioned in some xcode doc, i guess)
It should have it's own description in it's --help, and ideally an .rst documentation page in docs.

In D136651#4060071, @lebedev.ri wrote:

Docs still missing.

More Windows fixes

The pre-commit CI showed some test failures on Windows. Try to address these.

In D136651#4030008, @benlangmuir wrote:

Re-added a few of my minor comments that I think got lost on a specific version of the diff. Otherwise LGTM.

Fix Windows build (Thanks @ravi-ramaseshan)
Address @benlangmuir comments

Rebase correctly

Address review feedback
Move StatCacheFileSystem to its own file.

LGTM with minor nit

Fix unit test build

Add version number in cache file.

Address some review feedback

Thanks for the initial feedback!

LGTM

LGTM

LGTM

LGTM

Use make_shared

One little comment: should one of the tests where you removed the pubnames testing actually be replaced a CHECK-NOT: .debug_pubnames contents: ?

In D97739#2607961, @clayborg wrote:

In D97739#2607869, @jingham wrote:

This way of doing progress is going to look odd in anything that uses multiple debuggers. If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity.

that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.

In D96035#2585324, @avl wrote:

How would you guarantee that the artificial CU is always emitted in the same order? It seems like you need to stash the types somewhere, then create a deterministic ordering and then emit it, but I don't remember whether that's doable with LLVM's DIEs (It's been a while since I touched this code).

We might sort types on name basis inside that artificial CU. In this case it would always be emitted in the same order.

In D96035#2584686, @avl wrote:

The scheme implemented in this patch and which might be done with additional compile unit keeping types - visit CU only once and then it might be unloaded from the memory:

1. the first pass enumerates in a multithreaded manner all object files, all compile units. Each CU is loaded, analyzed for types(there would be created a list of attribute referencing types), emitted. types moved into the artificial CU. unloaded.

LGTM, thanks!

Couple comments/questions, but this generally looks good

This LGTM.
There is one thing that you might want to address, but I'll leave it up to you (and even if you do it can be a different commit): with the introduction of idx variables, it becomes easy to get confused between id and idx(Or CheckID and CheckIndex) in the codebase. Not sure how to disambiguate, or if it's really worth it.

The change itself looks fine, but I'm wondering about the test. There is nothing that really guarantees that the SB calls and return values are going to be intertwined, is there? Does it rely solely on the fact that it's very very unlikely to succeed?

This LGTM. I like that it's way easier than what we have discussed offline.

This is really hard to wrap one's head around, but after starring at it for some time the logic seems sound. There's some conditional code depending on whether we built with -gmodules or not. Can you make sure to test both clang builds? I left a couple other comments that you might want to consider. Otherwise this LGTM

LGTM

LGTM

Sure. As I said I know this is unrelated to the path. this LGTM

LGTM

lldbremote.py looks awfully Apple-specific, yet it is going to be called from the generic code. Is that not an issue?

This LGTM. The only comment I had is about a comment you added on preexisting code:

LGTM!

LGTM

The logic of the patch itself looks fine, but the names, description and commit message are off. A process cannot disable TCC, it's always active. What your patch decides is whether the inferior is responsible for its own TCC permissions. If you don't make the inferior responsible, it inherits the permissions of its parent (which might have inherited them from its own parent). On the command line, you'll most likely get the Terminal.app permissions if nothing in the middle resets the responsible process.

This looks good outside of 2 cross-platform issues in the tests themselves.

Simplify the logic.

LGTM!

In D83552#2162782, @labath wrote:

In D83552#2162555, @friss wrote:

The lldbtest.py part LGTM, but I'm failing to see how this interacts with TestWeakSymbols.py? IIRC, registerSharedLibraryWithTarget is an API we call explicitly in the tests, but I don't see it called here.

It gets called from run_to_source_breakpoint, keying off of the extra_images argument.

The lldbtest.py part LGTM, but I'm failing to see how this interacts with TestWeakSymbols.py? IIRC, registerSharedLibraryWithTarget is an API we call explicitly in the tests, but I don't see it called here.

I'll commit after I've re-built top of tree and fully retested

Address review feedback

• Rebase on top of D83512
• Change the ObjectFileMachO pieces to rewrite offsets to look like a standard Mach-o image instead of adding a bunch of conditionals to handle the new cases.