Fine if we get things in quick!

Ok, as long as we get things in soon, some functionality is better than none.

Are type units still disabled with the kill switch we had in? Or is this on top of the .debug_types patch? We have an internal .debug_types patch we have been using on an older LLVM/Clang/LLDB and we had some major performance issues regarding line tables so I am not sure where we would need to put these fixes (in this patch or in the .debug_types specific patch. This patch seems to enable parsing .debug_types already. The main issue we ran into were:

• only search actual compile unit line tables for breakpoints as many .debug_type units will point to existing line tables from real compile units
• don't add address ranges to type unit DWARFUnits from the line tables
• re-work how support files are parsed by sharing the line tables within a SymbolFileDWARF. .debug_types has type units that reuse line tables from real compile units (thousands of times) and we ended up seeing the same line table being parsed over and over and over. Another way to fix this is to no vend a lldb_private::CompileUnit for any units that aren't really compile units (DW_UT_type and DW_UT_split_type).

Just a few simplifications to GetName() and AppendTypeName() are in question and can optionally be done if needed.

See inline comments. Nice patch overall though, real close.

Might want the person that did DWO support to chime in here if that wasn't you?

Thanks for the clarification! LGTM

lgtm.

Looks good except the inline comment about all the unwind plans we have now. Would love to simplify this so that EH frame, compact unwind, ARM unwind, breakpad unwind all come from the symbol file or object files. Also be nice to have a register resolver that works without having a process so we can dump unwind info without needing a process.

A few nits and waiting for the test case.

actually lldb-vscode is always running locally so my previous concern is no longer valid.

It is very dangerous to play with any real STDIN, STDOUT, or STDERR in the lldb-vscode world since this is what is usually the file descriptors used for all the packets.

Just wanted to verify that we can put a:

settings set target.load-cwd-lldbinit true

in our ~/.lldbinit file and it will then load the local init file in the current working directory?

Need to add a test for this as well. And to cover all cases, you will need to test in a multi-compile unit example so we can ensure the relative CU offset of the call2 or call4 works in compile units that aren't the first one.

lgtm

Jim: this is a modified version of the patch we had talked about a while ago with needed fixes for making sure the inline function call site is the same as the starting file and line. We have iterated on this locally already, so I am good with this patch. Let us know what you think.

lgtm

LGTM. If there are any other " std::unique_ptr<char[]>" instances you would like to fix after this patch, feel free to submit more fixes without need for review.

FYI: for all DWARF expressions found in EH frame stuff, they expect the CFA address to be pushed onto the stack prior to executing the expression. Do we just want to follow suit with the these expressions and avoid the extra needed node?

I agree with all that was said above.

I agree with Pavel: no new SB APIs needed as this call is where it is supposed to be on SBHostOS.

lgtm

In D61090#1477709, @davide wrote:

@clayborg I would also like to point out that some of us fair amount of time cleaning LLDB_DISABLE_PYTHON leaking all over the codebase, and this is basically the last occurrence, so we're motivated to have this going away/fixed in some way.

People use this to populate the extra system path in python scripts when "import lldb" fails and throws an exception as you can run "lldb -P" to get the python path for the lldb module for the current lldb in your path.

Love having the AST output available so we can compare real to DWARF converted stuff!

LGTM, but since in llvm someone else should ok too.

Fine to fix the current issue. We should think about building this into the base class eventually. We can always have multi-threaded access, so we will always need to protect modifying operations.

Thanks for the diff! LGTM

Can you post a diff of things before and after? Hard to tell what we are opting out of just by seeing this patch

Got errors trying to compile this .s file on mac:

$ ~/Documents/src/lldb/svn/lldb/llvm-build/Release+Asserts/x86_64/bin/clang foo.s -o foo.o
foo.s:3:9: error: unknown directive
        .type bar, @function
        ^
foo.s:11:9: error: unknown directive
        .size bar, .-bar
        ^
foo.s:13:9: error: unknown directive
        .type foo, @function
        ^
foo.s:25:9: error: unknown directive
        .size foo, .-foo
        ^
foo.s:27:9: error: unknown directive
        .type main, @function
        ^
foo.s:49:9: error: unknown directive
        .size main, .-main

Not sure what functionality this really adds? It seems to just wrap the OS plug-in python class in a different way?

LGTM.

Jim Ingham said:

Just thought of 1 additional way to allow us to pull in fewer var declarations: get a list of all of the member variable names in the current class when stopped in a class method and only add ones that match local variables. If we are in a static member variable then skip of course. Comments? Thoughts?

Nice!

I would really like to see a better solution than just adding all var decls to an expression as this is already costly for C++ and we shouldn't propagate this. I know it used to be really really expensive. Not sure if it still is. But it would be worth measuring by running expressions in C++ and adding and removing this feature to see how much it costs us when we have many variables all with unique and complex types.

Almost seems like we can build the mutex into the base class OptionValue as we need general threaded protection for every setting. They any function that gets or sets the value should be able to protect itself using the base mutex

LGTM. Probably should get one more ok

The motivation for this test was the observation that an asanified LLDB would often exhibit seemingly random test failures that could be traced back to debugserver packets getting out of sync. With this path applied I can no longer reproduce the one particular failure mode that I was investigating.

In D60300#1456468, @JDevlieghere wrote:

In D60300#1455790, @davide wrote:

I think this is good regardless for readability, but I would really appreciate if we can collect some numbers to see how effective this change actually is.

I see 120 -> 12 seconds on my iMac Pro.

Thanks for getting this stuff reliably working. I debug using VS Code every day using lldb-vscode and it is my favorite LLDB based debugger! I look forward to seeing support for Windows and linux being tested and available.

In D59826#1452771, @martong wrote:

In D59826#1451536, @clayborg wrote:

Any way to dump the AST in a test to verify we don't create multiple?

I think I might be able to use the log object to dump the AST and then from python it would be possible to check for duplicates. Is that a good direction?

ok, so I think I figured out what was going on: I had the .so files still in my build packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new directory. They didn't show up in the "svn stat" command because they are ignored!!! arg! That is what was causing problems when testing on my machine.

Any way to dump the AST in a test to verify we don't create multiple?

Fix comments and address review issues.

In general any call that is doing anything through the public API needs to take the target API mutex. It is ok for quick accessors or other things to not do so, but anything that would need to keep the process in its current state, like asking a frame for a register value, should take the API mutex to ensure one thread doesn't say "run" and another says "what is the PC from frame 0 of thread 1".

Added Jim Ingham in case he wants to chime in here.