CC'ing the DWARF cabal.

For the future, a clean solution would be extending the macros in Dwarf.def to list the stack effects in the definitions of the DW_OP_*, for example

In D74205#1952518, @djtodoro wrote:

@jhenderson @aprantl Thanks for the review.

This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?

In case of Apple platforms, this won't make a difference in practice, since the support for that is implemented in PlatformDarwin (which all of these inherit from), but it sounds like this will be a problem for the "linux" sdk (assuming this is what I think it is), as there the selected platform will be PlatformLinux, which has no clue about these sdk thingies.

In D74205#1949246, @jhenderson wrote:

I'm not at all familiar with Mach-O, so how do fat archives get generated (i.e. with what tool?)? Is there an LLVM equivalent we can use here?

I've reworked this a little based on your feedback.

Ping.

Remove the obsolete setter in lldb::Type.

Rebase and update based on https://reviews.llvm.org/D75488. It no longer takes a reference, but is a value type instead.

Pinging @teemperor

LLDB didn't apply DW_AT_comp_dir to DW_AT_LLVM_include_path. I fixed that now.

LGTM!

In D74205#1946026, @djtodoro wrote:

I think that @aprantl wanted a test for Mach-O and archives, back then. I've added a test for mach-o and archive, in one shot (by using pre-existing fat mach-o archive from dsymutil).

I don't really know enough about this to give a meaningful review.

LGTM but since we're on the same team (now, woot!) I'd like to give others a chance to have a look.

Couple of minor suggestions inside.

I added a comment to D75488. It's in getOrCreateModule().

I only have a bunch of superficial comments but this seems reasonable as far as I can tell.

I'll update this with either a fix in clang or a patch to lldb once I figured out what happened.

I had to revert this because it unexpectedly broke the "expr -- @import Module" test in LLDB.

In D76672#1939583, @labath wrote:

In D76672#1939377, @JDevlieghere wrote:

In D76672#1938629, @labath wrote:

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they should be automatically recorded already, right? And if it doesn't then it does not need them...

The dSYM can contain other resources than just the Mach-O companion file, such as script for the OS plugins or opt remarks, which might not be used by the reproducers or even LLDB at all. Once you have the reproducer on your system, tools will find and use it because spotlight indexed it. Having only a partial dSYM is really undesirable as it can break LLDB and other tools in really unexpected ways.

Interesting. Could that be fixed by adding the funny .noindex suffix to the reproducer name (or some subdirectory of it)?

In your example you're encoding set as a derived type over an enum. That seems like a fine approach to me, but I think the interface should enforce this. Perhaps the Verifier should reject set derived typed from anything but an enum and the DIBuilder should only accept DICompositeTypes as the base type? Are there non-enum set types that you also want to encode?
Does this also need dwarfdump support, or does this work already?

In D76569#1936796, @shivammittal99 wrote:

run by hand ASAN-built LLDB for the code modified by this patch.

How do I do that? This is my first PR to llvm, so I am not aware of the tools. Please bear with me.

Yeah, screwed up during rebasing and accidentally committed an extra file from a different review I reverted and relanded the patch now and the bots are running again. Sorry for the noise.

Since it's late on Friday afternoon I'm taking the liberty to revert the patch to get the bots running again. Feel free to re-land with a fix or ask for another round of review if you have more questions.

It looks like this test doesn't work on the bots: http://green.lab.llvm.org/green//view/LLDB/job/lldb-cmake/lastCompletedBuild//testReport/junit/lldb-api/python_api_sbenvironment/TestSBEnvironment_py/

Looks like a great start! (From my side)

Rebased and addressed review comments.

Thanks, Pavel, these are all very valid concerns and in retrospect I should have at least slapped an RFC label on this before dumping into phabricator last night. The goal of this (and I should have mentioned that in the description) is to make the Xcode SDK something that behaves more like the compiler's resource directory, as in that it ships with LLDB rather than with the debugged program. This important primarily for importing Swift and Clang modules in the expression evaluator, and getting at the APINotes from the SDK in Swift.

Should we then call it SmallCoalescingBitVector? Probably not :-)

Don't try to be too clever with the strings.

Don't try to be too clever.

Rolled https://reviews.llvm.org/D75626 back into this patch, addressed more review feedback.

And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..

I'm merging this back into https://reviews.llvm.org/D75488 for easier reviewing.

I think this is starting to look good now. I found two more opportunities for improving the output... sorry about finding these piecemeal.

You could add a unit test for this, but it would effectively be a reimplementation of the function. I'm fine either way.