Looks good

Just a documentation suggestion, but looks good.

I am fine with this, Jim or Jason should ok this too just to be sure

Thanks for doing all the updates. Pretty clear and concise now.

See inlined comments.

Very nice. You got it right.

Thanks for doing all requested changes! Looks great.

Overall I am ok with minimal regression in speed if a few percent is all that it is costing us. I am generally ok with this patch. A few questions below.

The test really should encode actual DWARF that contains a DW_OP_litXXX opcode (using obj2yaml/yaml2obj or llvm-mc) as compiling the code for the test won't always produce the needed DWARF opcode,, but the change is simple and correct.

In D51935#1232203, @grimar wrote:

In D51935#1232195, @clayborg wrote:

Patch looks good. A few fixes mentioned in inlined comments. And I am unsure how the test suite will run with various compilers if they don't support the -gdwarf-5 flag. We might need to run obj2yaml on a binary and then yaml2obj it for the test to ensure we can run the test. We might be able to just symbolicate a few addresses in the test instead of requiring us to run something.

What about using the llvm-mc? I can convert c++ code to the assembler I think.
I'll try to investigate this problem tomorrow. Maybe we can just turn this test off somehow for that case.

Patch looks good. A few fixes mentioned in inlined comments. And I am unsure how the test suite will run with various compilers if they don't support the -gdwarf-5 flag. We might need to run obj2yaml on a binary and then yaml2obj it for the test to ensure we can run the test. We might be able to just symbolicate a few addresses in the test instead of requiring us to run something.

In D51966#1232057, @xbolva00 wrote:

So basically the hardest problem is to "find a way to get stdio from a process and feed to to LLDB so it shows up when neither --tty nor --no-stdio is set" ..

The other option is to only remove this flag if "--no-stdio" is set.

We should obey the --tty option here and set CREATE_NEW_CONSOLE if it is set no?

Only issue now is documentation and why are we requiring addresses to be in compile unit. See inlined comments.

Nice patch! A few minor fixes, see inlined comments.

Or add a new option that is --dependents that takes the enum and leave the --no-dependents there for compatability

I would rather not change the argument to be required if we can help it. That might break existing scripts or command files. I think if -d is specified without a value it should default to "no". Can we make the value optional?

See inlined comments. Cool stuff.

Can we add a test for this?

See my inlined comments about returning true and false correctly for the column and the correction to the format string

We shouldn't have the colon character be part of the ${line.column} formatting itself. See inlined comments.

Actually, we might need to still emit some Apple tables as the objective C and namespace tables might still be needed even with the DWARF5 tables.

We want to ensure that both Apple and DWARF5 tables never get generated though. That would waste a lot of space. I would say if DWARF5 tables are enabled, then we need ensure we disable Apple tables.

In D51604#1223451, @xbolva00 wrote:

Thanks for review, commited as rL341387

Yes abort is better as exit will cause the global C++ destructor chain to be called. I was always thinking lldbassert() was aborting, but seeing as this is not the case this could cause problems if you enable it as it will be a change. Watch the buildbots carefully.

What kind of scenario causes this crash to happen? Seems like a hack?

If you insert the text:

Should be easy to write unit test for ExecutionContext.cpp. See FileSpecTest.cpp for how to implement a unit test for a .cpp file.

Very nice. LGTM

Not sure about this one. IIUC we now wouldn't take the GIL for these functions now and hope that the str() function doesn't do something that would require thread safety?

Looks good to me. Jim should take a look as well.

In D44437#1219196, @luporl wrote:

Hello @clayborg, any update on this?

Is it clear why, at least on PPC64, the SymbolContext is not needed, as the needed info is at the symbol, and only at the symbol?

Make 16 byte UUIDs Apple specific.

In D51442#1217894, @lemo wrote:

I'm curious too: where did the PDB70 age create matching problems?

In D51442#1217829, @zturner wrote:

For PE/COFF files, the Age is also in the executable and Guid+Age actually
constitute a 20-byte UUID. Is this not the case on Apple? What object file
format are you dealing with?

Looks fine. A bit of cleanup might be nice on the TypeSystem.h to provide default implementations that just return 0 for some of these that every type system currently is required to override for no reason (all template related type system calls seem to have default "return 0;" functions in each type system. We can make future changes to these touch fewer files if we provide default implementations in TypeSystem.h.

In D51208#1214743, @dblaikie wrote:

But if LLDB has different performance characteristics, or the default should be different for other reasons - I'm fine with that. I think I left it on for Apple so as not to mess with their stuff because of the MachO/dsym sort of thing that's a bit different from the environments I'm looking at.

These are the numbers from my llvm-dev email in June:

setting a breakpoint on a non-existing function without the use of
accelerator tables:
real 0m5.554s
user 0m43.764s
sys 0m6.748s

setting a breakpoint on a non-existing function with accelerator tables:
real 0m3.517s
user 0m3.136s
sys 0m0.376s

This is an extreme case,

What was being tested here? Is it a realistic case (ie: not a pathalogical case with an absurd number of symbols, etc)? Using ELF? Fission or not?

How's it compare to GDB performance, I wonder? Perhaps LLDB hasn't been optimized for the non-indexed case and could be - though that'd still potentially motivate turning on indexes by default for LLDB until someone has a motivation to do any non-indexed performance tuning/improvements.

In D32167#1212783, @jankratochvil wrote:

Just a ping, that ConcatenatingDataExtractor is still not coded, even as some draft patch? Just checking to prevent duplicating some existing work.
FYI providing a rebase with few simple conflicts resolved. I had to drop conflicting lldb.xcodeproj/project.pbxproj changes there.

In D51176#1211433, @jingham wrote:

The other option would be to move the code that populates the section load list into the mini dump dynamic loader. That has the benefit of keeping this consistent with the other process plugins, but OTOH is just moving code around...

I am fine with this change then. Probably best to get the OK from Zach as well.

So this might actually work. Take a look around and see what else the dynamic loader is used for and make sure that they won't be needed for anything else. If not, this fix should work, but we should document it.

Sounds good. I am ok with this as long as Jim Ingham is.

Dynamic loaders will fill out the section load list in the target that saids "__TEXT" from "/tmp/a.out" was loaded at address 0x1000020200. So yes they are needed. If the process minidump is manually setting the section load list itself, then there really is no need for a dynamic loader and this fix might work.

No dynamic loader plug-ins should be affecting the module list during the plug-in loading/selection, if that is happening, that is a bug and it should be fixed.

Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX and iOS processes. They should also be needed for loading any minidumps that have stack traces.

In D50912#1209994, @jingham wrote:

What would happen to any output that the process produced while running the utility function if we did this.

Changed look fine. Wondering if we want to be adding pexpect style tests though. Those tests tend to be flaky. This could easily be done with SB API calls instead of using pexpect.

You can always run a.out through obj2yaml and check that in. There is test suite support for using a yaml file that converts it to a binary and debugs it

functionalities/gdb_remote_client/gdbclientutils.py
429:    def createTarget(self, yaml_path):
431:        Create a target by auto-generating the object based on the given yaml
437:        yaml_base, ext = os.path.splitext(yaml_path)
438:        obj_path = self.getBuildArtifact(yaml_base)
439:        self.yaml2obj(yaml_path, obj_path)

Change void SourceBreakpoint::SetBreakpoint() to use llvm::StringRef

Inline comments really help if you don't mind.

Fix all of zturner's inline comments.

Fixed almost all issues mentioned by zturner.

This should be in great shape now. Can anyone find time for this?

Replace README.txt with README.md and add full content to it.

Sort the &#%&$# Xcode project changes...

• Use the LLVM JSON parser
• Split lldb-vscode.cpp into smaller files
• Fix function names
• ran clang format on everything

BTW: warning I saw during recent build:

[98/790] Building CXX object tools/lldb/source/Core/CMakeFiles/lldbCore.dir/RichManglingContext.cpp.o
/home/gclayton/local/src/llvm/svn/llvm/tools/lldb/source/Core/RichManglingContext.cpp: In member function ‘bool lldb_private::RichManglingContext::IsCtorOrDtor() const’:
/home/gclayton/local/src/llvm/svn/llvm/tools/lldb/source/Core/RichManglingContext.cpp:77:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
/home/gclayton/local/src/llvm/svn/llvm/tools/lldb/source/Core/RichManglingContext.cpp: In member function ‘bool lldb_private::RichManglingContext::IsFunction() const’:
/home/gclayton/local/src/llvm/svn/llvm/tools/lldb/source/Core/RichManglingContext.cpp:89:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

In D50365#1191944, @labath wrote:

I am not sure I'll have the resources to see this review through, so I'd prefer to leave this to someone else.

The thoughts I have had so far are:

• the patch is very big and probably runs afoul of the "you shall develop incrementally" section in the LLVM developer policy. At least the JSON parts should be split off into a separate patch and tested independently.
• however, the choice of the JSON library is also an open question. We currently have at least three options to choose from:
  • llvm/Support/JSON.h
  • lldb/Utility/JSON.h
  • debugserver/source/JSON.h
    
    Of these, the third one is the one I'd least expect to be used here.

In D50365#1192447, @zturner wrote:

To elaborate, if you install the C/C++ plugin, and you go to Debug ->
Configurations, and you go down to the MICmdMode property, it is by default
set to "gdb". But you can change this to "lldb" and it works out of the
box.

In D50365#1191059, @lemo wrote:

Really cool! Are you planning to add some documentation for it? (set up instructions, etc...)

Removed dead code from python files.

More offsetof issues:

Passing patches between linux and mac the offsetof fixes got lost. When binary files are involved, patches are trickier to pass between to machines.

Fixed offsetof issues with:

Added CMakeList.txt changes, tested on linux, and removed unused "log" variable.

LGTM after Pavel's inline comments are addressed.

• run clang-format
• fix doxygen parameter names

Might be nice to put a blurb in the build page about this in the MacOS section?

Removed unnecessary Xcode project changes and removed #include that wasn't needed.

This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by making sure the frame and thread are not filled in when the process is running. It might also be racy. For example if you say "continue", hopefully the process will be resumed by the time the prompt is asked to refresh itself. Since we get events asynchronously this might be a problem.