LGTM. Much cleaner using the newer DWARFDIE code. Not sure if we can unit test this?

Is an AST importer specific to a target? Can we just put it into the Clang AST type system subclass and create it lazily?

ok if this works for now that is fine. Just close the socket_fd when we fail to write the pid to the socket and this is good to go.

Looks good. Making TypeSystem's into real plugins in the future would be great too.

nice!

So I did a bunch of original IOHandler. And there are many complex cases for sure. One thing to be aware of is that if you won't use editline() and we call fgets() in the default implementation, there is no way to cancel this IIRC. So it might be worth trying this without editline support to make sure things don't deadlock. If the test suite is happy, then this looks worth trying, though with all the complexities I don't think we can guarantee this doesn't cause issues in some unexpected way. The main things that worry me are:

• REPL issues since the REPL and (lldb) prompt switch between themselves in a tricky way where they swap IOHandlers
• running from python script directly when no IOHandlers are pushed because no command interpreter is being run and all the fallout from the cases (HandleCommand that results in a python breakpoint callback etc)
• the process IO handler that does STDIO for a process
• when no editline, we use fgets() that can't be canceled

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

In D71487#1820074, @probinson wrote:

In D71487#1791824, @clayborg wrote:

BTW: is used to be that both DW_AT_low_pc and DW_AT_high_pc would be set to zero when a function was dead stripped. This was back when both the low and high pc used DW_FORM_addr (a file address). But then DWARF changed such that DW_AT_high_pc could be encoded as a data form: DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, or DW_FORM_data8. This is used to mean it is an offset from the low PC. Seems the linkers now didn't have a relocation for the DW_AT_high_pc so they couldn't zero it out. This is sad because we can end up with many functions at address zero that didn't get linked, and if zero is a valid address, then our DWARF contains a bunch of useless info that only hides which function is the real function for address zero.

One solution, which we do in Sony, is to make the linker fix up undefined references to be -1 instead of 0 (at least, in the .debug_* sections). That's more obviously an invalid address. Doesn't help with existing objects in the wild but I'd like to keep that idea in the air as a forward evolutionary step.

if this wasn't just getting a fully qualified class name then ignore my comments.

Or do we

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

lgtm with Error being returned everywhere now.

LGTM.

The pid usually comes from the MinidumpMiscInfoFlags::ProcessID right? This is a minidump supported directory entry. We don't actually rely on /proc/%d/status right?

I would prefer that we just pick a PID of 1 for minidumps if they are missing and we have minidump files that don't have PIDs. Then no other logic needs to change? Have you seen real minidumps that have memory and threads and no process ID?

I don't mess with the expression parser all that much, but do we still want to see the pointer value in the log output?

Ok thanks for clarifying all my points! Looks good. And it would be nice to fix any locations that were canonicalizing types when they shouldn't be in a future patch.

So as long as the following are true from this patch I am ok:

• if I ask for the array element type of "str" in the test that was added, it should return "MCHAR". We shouldn't be removing any typedefs from the type. The user can call SBType::GetCanonicalType() if they need to (or equivalent with internal APIs)
• If there are no formatters for "MCHAR[]" we can fall back to "char[]".
• If there are no formatters for "MMCHAR[]" from my example we fall back to the _first_ array formatter that supports the array of typedefs, or back to the the array of canonical types. Only the first array based formatter should be returned as the desired formatter

I would suggest removing GetVaruint7 and GetVaruint32 and adding "llvm::Optional<uint8_t> DataExtractor::GetULEB128(uint64_t *offset_ptr, uint64_t max_value);" as mentioned in inlined comments.

lgtm. Pavel?

Looks good, thanks for tracking this down!

Very cool BTW!

No worries. Large large stacks are rare and I would rather backtraces work correctly in the IDE if the schema was wrong. Sorry for the delay, medical issues for the past few months on my end.

In D70840#1783351, @labath wrote:

I am strongly opposed to ArchSpec owing an Architecture object. The latter is far more complicated -- it reads bytes from target memory and disassembles them -- whereas ArchSpec just returns a bunch of constants. If anything, it should be the other way around. That way the code which just fiddles with triples does not need to depend on the whole world.

BTW: is used to be that both DW_AT_low_pc and DW_AT_high_pc would be set to zero when a function was dead stripped. This was back when both the low and high pc used DW_FORM_addr (a file address). But then DWARF changed such that DW_AT_high_pc could be encoded as a data form: DW_FORM_data1, DW_FORM_data2, DW_FORM_data4, or DW_FORM_data8. This is used to mean it is an offset from the low PC. Seems the linkers now didn't have a relocation for the DW_AT_high_pc so they couldn't zero it out. This is sad because we can end up with many functions at address zero that didn't get linked, and if zero is a valid address, then our DWARF contains a bunch of useless info that only hides which function is the real function for address zero.

It is sad that we can't tell if a DW_AT_low_pc with a value of zero is valid or not. Some shared libraries might be able to have a function at address zero, so we need to be careful here. My proposed fix above will check the section that contains the low PC to see if it has execute permissions, so that should cover all cases (executables and shared libraries). Not sure if there is more we can do.

LGTM

It would be nice if we could test this works as well. We would need to spawn the lldb-vscode manually and specify a socket, connect with a socket with the vscode.py test stuff, and get able to get some output from it once the session is up and running. Something like sending:

`script print("stdout outut")

to the debugger console after we stop (note the leading backtick so it will not evaluate an expression, but it will cause STDOUT to happen). Make sure the session doesn't die due to the output, and make sure we get the output from the process we spawned in the test suite

minor nit and this is good

In D71498#1786343, @jankratochvil wrote:

In D71498#1786319, @clayborg wrote:

For the printf style statement, we can't use just one cast to "uintptr_t" because on 32 bit systems it won't be converted to 64 bit.

That pointer-to-uint64_t is now used for printf with PRIx64 so if we use pointer-to-uintptr_t we could use PRIxPTR.

As I am reading this, I just wanted to send out a note of something else that can cause crashes in ARM/Thumb code. For anyone working with ARM/Thumb on systems that don't use the ARM and Thumb BKPT instruction when setting software breakpoints (like all lldb linux and android flavors IIRC): if you try to overwrite a 32 bit thumb instruction that is a conditional instruction in a Thumb IT instruction with a 16 bit trap or illegal instruction you can crash your program. The issue arises for code like:

In other places you're replacing a reinterpret_cast<addr_t> with two c casts. I guess this was done because two c++ casts were too long (?). In these cases the second cast is not really needed, as uintptr_t->addr_t should convert automatically. I think I'd prefer that we just have a single reinterpret_cast<uintptr_t> instead of two c casts. Then our rule can be "always convert a pointer to uintptr_t". I don't know if there's a clang-tidy check for that, but it sounds like that could be something which could be checked/enforced there...

Seems like it might be nice to have a macro defined in a LLDB header file like lldb-defines.h. Something like:

Can you attach a paste of the DWARF that is emitted by lld and the symbol table and annotate which one should be picked. I am having trouble understanding what the right solution for this fix would be. It makes me nervous to require a symbol in the symbol table since symbol tables can be stripped from binaries.

Sorry for the delay. I have had medical issues going on for the past month.

That must have been fun to find! LGTM.

In D71372#1782567, @jingham wrote:

In D71372#1782549, @clayborg wrote:

In D71372#1782538, @jingham wrote:

In D71372#1782536, @clayborg wrote:

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

[0x00000000 - 0x100000000) ---

no permisssions are represented as "---". Then you can take the end address and look it up, and continue. So as long as we aren't getting bogus values back, we are good.

Then what does the bool return mean?

If there is no memory map info in the process plug-in at all, then false will be returned.

That means to answer the question "did GetLoadAddressPermissions return valid permissions for load address 0xABCD" I have to check both the return value and if any of the permission values are unknown? That seems like an awkward API.

In D71372#1782538, @jingham wrote:

In D71372#1782536, @clayborg wrote:

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

[0x00000000 - 0x100000000) ---

no permisssions are represented as "---". Then you can take the end address and look it up, and continue. So as long as we aren't getting bogus values back, we are good.

Then what does the bool return mean?

I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return

LGTM!

Sorry for the delay, lgtm

I seems this is optional in https://microsoft.github.io/debug-adapter-protocol/specification

If this does make things not behave correctly, we should add it in, I don't have any problem with it since most of the time we won't have that many stack frames.

lgtm

Change over to using ASSERT_THAT_EXPECTED(LR, Succeeded())

Fixed:

• Updates LLVM license
• Added gsym::SourceLocation operator==
• Added gsym::SourceLocation operator<< for raw_ostream
• Change gsym::LookupResult::dump to operator<< for raw_ostream
• Converted tests to use testing::ElementsAre to make tests easier to read

We could just add a new flag to lldb-vscode like "--no-lldb-init" and always pass that when we run our test suite?

Although I am ok with this patch, it will only work if the unexpected output comes before we expect any packets or perfectly in between packets. Not sure we should do this. For example this would work:

This seems like it will make a lot more work happen. Many C++ formatters are regex based and are quite expensive to compare against type names. Seems worthwhile to skip if the language doesn't match?

lgtm

The other fix would be to add a new LLDB API function:

I think the fix is better done to fix the root cause issue instead of working around it. I have suggested a fix in inline comments.

Abandon this one and modify https://reviews.llvm.org/D70882?

If MS VS Code IDE itself will never pass this to the "initialize" packet, then we might just want to use an env var instead? You should probably get rid of https://reviews.llvm.org/D70886 and just modify this patch?

So what is this fix actually doing? Allowing random STDOUT to be ignored?

We shouldn't be running the test suite that allows your ~/.lldbinit file to be run. This can really hose up many things, so we should change the lldb-vscode test suite to not allow your ~/.lldbinit file to be loaded instead?

Would be good to write a test for this where the init file does "script print('hello')" which would hose up the connection

Might be a better idea to realpath this once in the setup code instead of calling realpath thousands and thousands of times? Maybe we just santize "LLDB_TEST" and "LLDB_BUILD" one time with top level code like:

So the main goal here is to let the Architecture plug-in handle the address masking in all places and make sure there is no code like:

So some background on how address masks are handled in LLDB:

lgtm

In D70532#1758962, @labath wrote:

In D70532#1758907, @clayborg wrote:

So I am worried by seeing any mention of "load address" and "load bias" in this patch. All information that is extracted from the DWARF should be solely in terms if "file addresses" IMHO,

This is still true. The information for which goes into the construction of the DWARFExpression is based on information from the object file alone -- this is the CU base (file) address, and function base (file) address. Essentially, the only difference here is that previously we were passing the difference between these two values, whereas this patch passes the two values separately.

So I am worried by seeing any mention of "load address" and "load bias" in this patch. All information that is extracted from the DWARF should be solely in terms if "file addresses" IMHO, unless I am not understanding something from the new location lists. For mach-o DWARF in .o files, the "file address" will be a file address from the .o file (which is fine), but it will need to be linked into an executable file address before being handed out. We fix up addresses like this for any DW_OP_addr opcodes in location expressions. So typically we would have a virtual SymbolFileDWARF method that SymbolFileDWARFDebugMap would override and this would use the default SymbolFileDWARF::GetLocationList(...) function, and then link up the location list or DWARFExpression prior to handing it out. Is there something I am not understanding here?

I wanted to make sure that people understand about how templates are done in DWARF and the implications, just for completeness:

1. DWARF represents only specializations of templates (foo<int>) and not the generic template definition in terms of type T (foo<T>)
2. DWARF only creates the methods that are used in each compile unit. So we might only have std::vector<int>::erase() and std::vector<int>::back() in one compile unit and std::vector<int>::operator[](size_t) in another
3. The template type definitions we create in the clang AST context therefore have no generic template methods. The definition for foo<T> has no methods. All methods for the specialized type (foo<int>) are treated as specializations specific to the <int> type. We have to do this since we item #2 above might only have a fraction of the real template definition's methods.
4. #3 means that we must iterate over all instances of foo<int> to find as many methods as possible when creating the type since each one might only have a fraction of all methods from the original foo<T> definition in the code.

When running the lldb command line debugger in a console, it can be convenient to get the output of the debuggee inline in the same console.

--stop-at-entry only really works for Darwin since its posix_spawn has a flag that will stop it at the entry point before anything executes. On linux, does this flag do anything?

In D70134#1742647, @jingham wrote:

This is fine.

I wondered a bit about whether it would be generally useful to add the 'dylibs that have to be copied' to the SBLaunchInfo? It has some other platform'y like things. I'm not strongly promoting the idea, just thought I'd float it.

Maybe limit the matches if posssible if that works. If you type "target variable <tab>" you can complete a list of all global variables everywhere which might be quite a few.

In D69210#1717042, @vsk wrote:

In D69210#1716861, @clayborg wrote:

In D69210#1715679, @vsk wrote:

Hm, this patch is bugging me.

It looks a bit like instructions are still decoded multiple times in different ways (e.g. in the Decode and CalculateMnemonicOperandsAndComment methods, which both modify m_opcode). Any ideas on whether/how to consolidate these?

I am all for anything that will improve efficiency. This class has evolved over time where we started with just the "CalculateMnemonicOperandsAndComment" and then many other features (can branch, etc) were later built into the class. I don't believe instructions are kept around for long so they typically serve one of two purposes:

• disassembly of instruction stream where only CalculateMnemonicOperandsAndComment is needed
• inspection of multiple instructions for stepping looking at can branch and other information requests
  
  So I am not sure the decoded multiple times in different ways is really important unless we do have a costly client that does both CalculateMnemonicOperandsAndComment and inspecting of instruction attributes (can branch, etc). Again, these objects are created, used and discarded currently AFAIK.

Thanks for your comment Greg. Let me try and restate the issue I see as my concern isn't about performance.

It looks like Decode and CalculateMnemonicOperandsAndComment mutate m_opcode in different ways. Separately, the predicates read m_opcode. So I'm not sure whether/in-which-order the mutating methods need to be run before the predicates can safely be called. I'd like to consolidate all the code that mutates m_opcode in one place, to make the predicates always safe to call. Does that seem reasonable? Or am I overthinking something?

Looks good!

In D69210#1715679, @vsk wrote:

Hm, this patch is bugging me.

It looks a bit like instructions are still decoded multiple times in different ways (e.g. in the Decode and CalculateMnemonicOperandsAndComment methods, which both modify m_opcode). Any ideas on whether/how to consolidate these?

Sounds good, thanks for doing this