LGTM, thanks for cleaning this up!

Superseded by D95188

Superseded by D95188

LGTM

Overall this looks good. I wonder if abstracting the ExternalRedirect as a small wrapper class around a SmallString would help. There's a few operations that are repeated, like the example below, and it could also house the logic that's currently in the lambda.

• Implement makeCanonical using canonicalize
• Add missing call to makeCanonical is isLocal

LGTM but I'd like @teemperor to have a look too as he authored the original change.

LGTM!

Fixed in f354b87df23799ee0b6c718894140c846eafc82d

I replied before I actually tried to understand what your'e trying to achieve, and it's still not entirely clear to me. I think what you need is something similar like python-typemaps.swig which helps swig understand char** types and such.

@jingham should weigh in here, but I think we kind of guarantee (at least informally) that the command options to be stable. Is there an alternative way to fix this without breaking the existing option?

Thanks!

LGTM with the inline comment addressed.

In D94890#2510446, @MaskRay wrote:

Is this good? :)

In D94536#2507008, @sberg wrote:

As also reported at https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20210118/872711.html "Re: [llvm] 48d2068 - [dsymutil] Warn on timestmap mismatch between object file and debug map" about the re-revert https://github.com/llvm/llvm-project/commit/48d2068fb738302b9fba91c9bf8e4b051c3909eb "[dsymutil] Warn on timestmap mismatch between object file and debug map" (but for which there appears to be no Phabricator review of its own):

I have no idea what this warning is all about, and what exactly triggers it, but it looks like it breaks the

# Check if the C compiler supports the "visibility" function attribute
# If supported, defines HAVE_FUNC_ATTRIBUTE_VISIBILITY
AX_GCC_FUNC_ATTRIBUTE(visibility)

configure check used by Little-CMS (https://github.com/mm2/Little-CMS/blob/a0fc38ddd55a0341dafa7414ac624c1f09271401/configure.ac#L70; e.g. built as part of building LibreOffice), when building on macOS:

configure:15908: checking for __attribute__((visibility))
configure:15930: /Users/stephan/Software/llvm/inst/bin/clang -m64 -mmacosx-version-min=10.15 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -fno-sanitize-recover=all -ferror-limit=0 -o conftest  -ggdb2 -Xclang -debug-info-kind=constructor -O0 -fstrict-aliasing -fstrict-overflow  -I/Users/stephan/Software/lo/core/include  -I/Users/stephan/Software/adoptopenjdk.net/OpenJDK15U-jdk_x64_mac_hotspot_15.0.1_9/jdk-15.0.1+9/Contents/Home/include  -I/Users/stephan/Software/adoptopenjdk.net/OpenJDK15U-jdk_x64_mac_hotspot_15.0.1_9/jdk-15.0.1+9/Contents/Home/include/darwin  conftest.c  >&5
warning: /Users/stephan/Software/lo/TMPDIR/conftest-01e065.o: timestamp mismatch between object file (2021-01-18 19:22:51.045749227) and debug map (2021-01-18 19:22:51.000000000)
warning: /Users/stephan/Software/lo/TMPDIR/conftest-01e065.o: timestamp mismatch between object file (2021-01-18 19:22:51.045749227) and debug map (2021-01-18 19:22:51.000000000)
configure:15930: $? = 0
configure:15943: result: no

That is, the warning output apparently confuses the AX_GCC_FUNC_ATTRIBUTE logic and makes it erroneously assume that Clang does not support attribute((visibility)).

In D94890#2507875, @MaskRay wrote:

" (+ one additional test in the Swift fork)" --- Sounds like this can be a separate patch which should Swift folks a heads-up. I don't know how to test Swift and probably someone else can do it:)

In D94890#2507241, @MaskRay wrote:

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

I can add back it under the ifeq "$(OS)" "Darwin" guard if Apple folks think it is useful.

Ship it

LGTM

Use SmartMutex and SmartScopedLock.

Sounds like something to tablegen ;-) Joking aside, this seems easy enough to auto-generate with clang tooling, so LGTM as a temporary measure.

We guarantee that the SB API is ABI stable so you cannot change the signature of existing functions. Would an overload do the trick?

Cool. That was one of the reasons I suggested having the SB API documentation integrated with the rest of the docs.

LGTM

Personally I'd just like to get rid of the shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

LGTM. Thanks for driving this!

ping

Address @avl's review comments.

Address code review feedback

LGTM

LGTM for LLDB

In D94323#2487899, @avl wrote:

If I understood correctly, It looks like this change is not enough to work properly for DWARF5.

In D93926#2486038, @lanza wrote:

I guess if the intention of that is maintain the debugger instance around it should work, but at the moment it segfaults pretty quick with Xcode's lldb using: