breaks basically anything that examines debugger output

If it's the latter, then, yeah, some "transparent to debugger" source attribute might be appropriate - that lowers to a bit on the DISubprogram and instructs LLVM to, if the function definition ends up lowering to a trampoline, mark that for the debugger so it's transparent - and if it lowers to a trampoline but doesn't have the source attribute, you can then still step into the function separately from stepping into the subsequent function?

So this attribute will lower into a DW_AT_trampoline("target_func_name") attribute on the DW_TAG_subprogram of the function definition?

Thanks for adding the check!

Thanks! It works.

Oh wait, I didn't realize you haven't landed it yet :-)

Thanks, but this does not seem to have worked: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52610/consoleText

You could add a test similar to lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp

Otherwise LGTM

Here is an example of how the Swift Runtime plugin detects both itself and whether ObjC interop is enabled. Basically all I'm asking is to not accidentally break this mechanism.
https://github.com/apple/llvm-project/blob/7750ffa35111df6d38a5c73ab7e78ccebc9b43c3/lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp#L124
If you want to make 100% sure, you could checkout the Swift compiler on Linux (or I suppose Windows) from github.com/apple/swift run swift/utils/update-checkout --clone apply your patch to llvm-project and run swift/utils/build-script --lldb --test -- --skip-build-benchmarks --skip-test-cmark --skip-test-swift and make sure the lldb tests still pass after applying you patch. But if you check for the symbol that would be sufficient for me.

In D146154#4198730, @sgraenitz wrote:

In D146154#4197454, @aprantl wrote:

One thing I just realized — we need to make sure that we don't accidentally create a GNUstep ObjC runtime in a Swift process that was built without ObjC support on Linux.

Yes, thanks for bringing this up. The goal definitely is to avoid any accidental conflicts with existing use cases that don't need or expect a GNUstep runtime. I really want to get my focus to the Windows side and PDB parsing. It's useful to have Linux working as well, so that we have a testable feature set to match. Otherwise, we don't want to invest a lot of effort here yet.

How can we ensure this works for both cases?

Shouldn't the Swift processes report eLanguageTypeSwift? Then GNUstepObjCRuntime::CreateInstance() rejects it reliably.

This somehow broke the -DLLVM_ENABLE_MODULES=1 build:

I.e., can you detect based on the presence of a symbol or shared object that an GNUstep runtime is present?

One thing I just realized — we need to make sure that we don't accidentally create a GNUstep ObjC runtime in a Swift process that was built without ObjC support on Linux. How can we ensure this works for both cases?

In general I don't think I have a problem with adding this, especially since Clang also supports the runtime as a compilation target, and LLDB is tightly integrated with Clang.

I don't /think/ that's the case - I know Apple used to have a pretty significant dependence on IR upgrading at least invalidating the IR metadata & dropping it (so maybe all you need to do is update the debug info IR verifier so that old IR gets deemed invalid and dropped - not necessarily upgraded).

In D144008#4185551, @dblaikie wrote:

@aprantl

Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

Mechanically, this patch looks great to me. @JDevlieghere, do you have any high-level concerns?

I'll defer to @JDevlieghere

We should consider factoring this function out and writing a unit test for it. The current tests are very indirect.

This doesn't affect any tests?

We might consider using dwim-print here in the future?

That seems to be clearly better, since p could be aliased to anything.

From an end-user perspective, I like this.

Nice! I appreciate the detailed explanation of the algorithm. I have some suggestions for how to streamline it a bit and potentially make it easier to read inside.

Deleting specialized code while improving correctness? That's always welcome!

I'm fine with taking this, thanks!

I think this is fine. It's concise, adds useful plugin functionality and we're already hardcoding all sorts of special knowledge about libc++.

Nice.

Looking good!

So from my point of view, this is a good path forward, so assuming that the other reviewers agree, this LGTM.

So, one could say that (right now) this patch is NFC for end-users, because dwim-print supports all use-cases that the old p alias supported?

Looks pretty good, some nitpicks inside.

I originally was hoping we wouldn't have to introduce a new attribute for this, but it looks like there are legitimate concerns about the alternatives, so in that sense this looks good!

How is this related to https://reviews.llvm.org/D142283?

I think this SGTM.

Did you forget to git-add the test?

Yeah, this is better.

So your build system guarantees that all bitcode that is being consumed was generated by compilers linked against the same version of LLVM that is used in the LTO plugin and if that version were to serialize malformed metadata, you wouldn't be any worse off than a non-LTO build, which also doesn't verify between passes.

Size-wise this looks like an acceptable increase. If we created a new DW_AT_LLVM_abi_tag, we could save an extra 4 bytes (assuming DW_FORM_strp) per DIE. That might be worth it?

Conceptually, I think this looks good to me.

Alternatively, I suppose, the DWARF emission could just look at the preferred name and use that as the DW_AT_type in all cases anyway? Avoids needing a new attribute, etc, though would be a bit quirky in its own way.

It seems reasonable to me to provide a way to skip this for situations where the build system guarantees that the bitcode is always generated by the same compiler, e.g. in a distributed ThinLTO build where any change in compiler will force a rebuild anyway, which is the case @aeubanks is looking at.

Test looks good!

In D143921#4123218, @fdeazeve wrote:

Any testing suggestions here? I can use what we have on GH (cpp -> codegen test), but I'm not sure if there's a finer grained test we could use.

Should we put something like

Why are only these two tests affected? Should this be something we set globally for all the tests? The API tests already have support for forwarding ASAN_OPTIONS and lit has a similar concept.

Generally LGTM, with one possible improvement inline!

Nice!

Looks like this was fixed in

commit 2700d66a054327c2dbb90d8b44b1d5cf3b0043b8
Author: Kazu Hirata <kazu@google.com>
Date:   Wed Feb 8 21:00:45 2023 -0800