- User Since
- Sep 23 2014, 10:11 AM (277 w, 3 d)
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?
Thu, Jan 16
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.
Wed, Jan 15
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
Tue, Jan 14
We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?
Mon, Jan 13
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.
Thu, Jan 9
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?
Wed, Jan 8
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.
Mon, Jan 6
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.
Thu, Dec 19
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.
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.
Dec 18 2019
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
Dec 17 2019
minor nit and this is good
Dec 16 2019
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:
Dec 15 2019
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.
Dec 12 2019
Sorry for the delay. I have had medical issues going on for the past month.
That must have been fun to find! LGTM.
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
Dec 11 2019
Dec 10 2019
Dec 9 2019
Sorry for the delay, lgtm
Dec 5 2019
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.
Dec 4 2019
Change over to using ASSERT_THAT_EXPECTED(LR, Succeeded())
- 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
Dec 3 2019
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?
Dec 2 2019
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:
Nov 25 2019
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:
- DWARF represents only specializations of templates (foo<int>) and not the generic template definition in terms of type T (foo<T>)
- 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
- 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.
- #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.
Nov 18 2019
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.
Nov 12 2019
--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?
Nov 11 2019
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.
Oct 21 2019
Oct 18 2019
Sounds good, thanks for doing this