Page MenuHomePhabricator

clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (277 w, 3 d)

Recent Activity

Yesterday

clayborg accepted D72920: [lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress.

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

Fri, Jan 17, 2:15 PM · Restricted Project
clayborg added a comment to D72946: [lldb] Remove ClangASTImporter reference from Target.

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

Fri, Jan 17, 1:26 PM · Restricted Project

Thu, Jan 16

clayborg accepted D72813: Fixes to lldb's eLaunchFlagLaunchInTTY feature on macOS.

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.

Thu, Jan 16, 1:26 PM · Restricted Project
clayborg accepted D72684: [lldb][NFC] Rename ClangASTContext to TypeSystemClang.

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

Thu, Jan 16, 11:58 AM · Restricted Project

Wed, Jan 15

clayborg added inline comments to D72813: Fixes to lldb's eLaunchFlagLaunchInTTY feature on macOS.
Wed, Jan 15, 5:10 PM · Restricted Project
clayborg added inline comments to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.
Wed, Jan 15, 11:27 AM · Restricted Project
clayborg added a comment to D70314: [lldb] Add expect_expr function for testing expression evaluation in dotests..

nice!

Wed, Jan 15, 11:18 AM · Restricted Project
clayborg added a comment to D72748: [lldb/IOHandler] Change the way we manage IO handler.

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
Wed, Jan 15, 11:18 AM · Restricted Project
clayborg added a reviewer for D72748: [lldb/IOHandler] Change the way we manage IO handler: clayborg.
Wed, Jan 15, 11:18 AM · Restricted Project

Tue, Jan 14

clayborg added a comment to D72684: [lldb][NFC] Rename ClangASTContext to TypeSystemClang.

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

Tue, Jan 14, 1:09 PM · Restricted Project
clayborg added a comment to D71487: [LLDB] Fix address computation for inline function.

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.

Tue, Jan 14, 10:51 AM · Restricted Project

Mon, Jan 13

clayborg added a comment to D69820: [Symbol] Add TypeSystem::GetClassName.

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

Mon, Jan 13, 2:02 PM · Restricted Project
clayborg added a comment to D69820: [Symbol] Add TypeSystem::GetClassName.

Or do we

Mon, Jan 13, 1:52 PM · Restricted Project
clayborg added a comment to D69820: [Symbol] Add TypeSystem::GetClassName.

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

Mon, Jan 13, 10:54 AM · Restricted Project
clayborg added inline comments to D72595: Fix lookup of symbols at the same address with no size vs. size.
Mon, Jan 13, 10:35 AM · Restricted Project
clayborg added inline comments to D72597: [lldb][DWARF] Added support for new forms in DWARFv5 macro..
Mon, Jan 13, 10:16 AM · Restricted Project
clayborg added a comment to D71875: [DWARF] Return Error from DWARFDebugArangeSet::extract()..

lgtm with Error being returned everywhere now.

Mon, Jan 13, 10:07 AM · Restricted Project, debug-info
clayborg accepted D71761: [lldb] Add a setting to not install the main executable.

LGTM.

Mon, Jan 13, 9:28 AM · Restricted Project

Thu, Jan 9

clayborg added inline comments to D71875: [DWARF] Return Error from DWARFDebugArangeSet::extract()..
Thu, Jan 9, 3:24 PM · Restricted Project, debug-info
clayborg added a comment to D70238: [lldb] Allow loading of minidumps with no process id.

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?

Thu, Jan 9, 3:15 PM · Restricted Project
clayborg added a comment to D70238: [lldb] Allow loading of minidumps with no process id.

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?

Thu, Jan 9, 3:06 PM · Restricted Project

Wed, Jan 8

clayborg added a comment to D72391: [lldb] Add a display name to ClangASTContext instances.

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?

Wed, Jan 8, 5:01 PM · Restricted Project
clayborg added inline comments to D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end..
Wed, Jan 8, 4:51 PM · Restricted Project
clayborg accepted D72133: Data formatters: Look through array element typedefs.

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.

Wed, Jan 8, 3:46 PM · Restricted Project

Mon, Jan 6

clayborg added a comment to D72133: Data formatters: Look through array element typedefs.

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
Mon, Jan 6, 2:07 PM · Restricted Project
clayborg requested changes to D72133: Data formatters: Look through array element typedefs.
Mon, Jan 6, 1:57 PM · Restricted Project
clayborg added inline comments to D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging.
Mon, Jan 6, 1:20 PM · Restricted Project
clayborg added a comment to D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging.

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.

Mon, Jan 6, 12:52 PM · Restricted Project
clayborg added a comment to D71825: [lldb/Lua] Support loading Lua modules..

lgtm. Pavel?

Mon, Jan 6, 12:33 PM · Restricted Project

Thu, Dec 19

clayborg accepted D71487: [LLDB] Fix address computation for inline function.

Looks good, thanks for tracking this down!

Thu, Dec 19, 3:30 PM · Restricted Project
clayborg added a comment to D71235: [lldb/Lua] Generate Lua Bindings and Make Them Available to the Script Interpreter.

Very cool BTW!

Thu, Dec 19, 3:10 PM · Restricted Project
clayborg added a comment to D71034: [lldb-vscode] Added support for ‘totalFrames’ field in StackTraces response .

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.

Thu, Dec 19, 3:10 PM · Restricted Project
clayborg added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

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.

Thu, Dec 19, 2:30 PM · Restricted Project
clayborg added a comment to D71487: [LLDB] Fix address computation for inline function.

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.

Thu, Dec 19, 2:21 PM · Restricted Project
clayborg requested changes to D71487: [LLDB] Fix address computation for inline function.
Thu, Dec 19, 2:14 PM · Restricted Project
clayborg added a comment to D71487: [LLDB] Fix address computation for inline function.

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.

Thu, Dec 19, 2:14 PM · Restricted Project
clayborg accepted D71372: [lldb] Add additional validation on return address in 'thread step-out'.

LGTM

Thu, Dec 19, 2:04 PM · Restricted Project

Dec 18 2019

clayborg added a comment to D71633: [lldb-vscode] Only close the debuggers in/out when DAP is over stdin/out.

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 18 2019, 12:24 PM · Restricted Project

Dec 17 2019

clayborg requested changes to D71633: [lldb-vscode] Only close the debuggers in/out when DAP is over stdin/out.
Dec 17 2019, 7:14 PM · Restricted Project
clayborg added a comment to D71633: [lldb-vscode] Only close the debuggers in/out when DAP is over stdin/out.

minor nit and this is good

Dec 17 2019, 7:14 PM · Restricted Project

Dec 16 2019

clayborg added a comment to D71498: Fix ARM32 inferior calls.

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.

Dec 16 2019, 11:50 AM · Restricted Project
clayborg added a comment to D71498: Fix ARM32 inferior calls.

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:

Dec 16 2019, 11:40 AM · Restricted Project
clayborg added a comment to D71498: Fix ARM32 inferior calls.

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...

Dec 16 2019, 11:21 AM · Restricted Project
clayborg added a comment to D71498: Fix ARM32 inferior calls.

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

Dec 16 2019, 11:12 AM · Restricted Project

Dec 15 2019

clayborg added a comment to D71487: [LLDB] Fix address computation for inline function.

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 15 2019, 9:33 PM · Restricted Project

Dec 12 2019

clayborg added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

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

Dec 12 2019, 7:46 PM · Restricted Project
clayborg accepted D71440: Extending step-over range past calls was causing deadlocks, fix that..

That must have been fun to find! LGTM.

Dec 12 2019, 7:28 PM · Restricted Project
clayborg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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.

Dec 12 2019, 2:25 PM · Restricted Project
clayborg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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?

Dec 12 2019, 2:09 PM · Restricted Project
clayborg added a comment to D71372: [lldb] Add additional validation on return address in 'thread step-out'.

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 12 2019, 2:04 PM · Restricted Project

Dec 11 2019

clayborg requested changes to D71372: [lldb] Add additional validation on return address in 'thread step-out'.
Dec 11 2019, 1:05 PM · Restricted Project

Dec 10 2019

clayborg accepted D71262: [lldb] "See through" atomic types in ClangASTContext.
Dec 10 2019, 11:51 AM · Restricted Project
clayborg accepted D71268: [lldb/DWARF] Add support for DW_AT_loclists_base&DW_FORM_loclistx.

LGTM!

Dec 10 2019, 11:42 AM · Restricted Project

Dec 9 2019

clayborg added a comment to D70532: [lldb] Improve/fix base address selection in location lists.

Sorry for the delay, lgtm

Dec 9 2019, 10:52 AM · Restricted Project

Dec 5 2019

clayborg committed rGaeda128a96c4: Add lookup functions for efficient lookups of addresses when using GsymReader… (authored by clayborg).
Add lookup functions for efficient lookups of addresses when using GsymReader…
Dec 5 2019, 4:52 PM
clayborg closed D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..
Dec 5 2019, 4:52 PM · Restricted Project
clayborg added a comment to D71034: [lldb-vscode] Added support for ‘totalFrames’ field in StackTraces response .

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

Dec 5 2019, 4:42 PM · Restricted Project
clayborg accepted D71034: [lldb-vscode] Added support for ‘totalFrames’ field in StackTraces response .

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 5 2019, 4:42 PM · Restricted Project
clayborg accepted D71003: [lldb/DWARF] Switch to llvm location list parser.

lgtm

Dec 5 2019, 1:03 PM · Restricted Project

Dec 4 2019

clayborg updated the diff for D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..

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

Dec 4 2019, 2:48 PM · Restricted Project
clayborg added inline comments to D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..
Dec 4 2019, 2:38 PM · Restricted Project
clayborg updated the diff for D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..

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
Dec 4 2019, 2:31 PM · Restricted Project

Dec 3 2019

clayborg updated subscribers of D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..
Dec 3 2019, 5:05 PM · Restricted Project
clayborg created D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes..
Dec 3 2019, 5:05 PM · Restricted Project
clayborg added a reviewer for D70883: [vscode.py] Make read_packet only return None when EOF: labath.
Dec 3 2019, 4:00 PM · Restricted Project
clayborg added a comment to D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs.

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

Dec 3 2019, 3:51 PM · Restricted Project
clayborg added inline comments to D70979: [lldb][NFC] Migrate to raw_ostream in ArchSpec::DumpTriple.
Dec 3 2019, 3:51 PM · Restricted Project
clayborg added a comment to D70883: [vscode.py] Make read_packet only return None when EOF.

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:

Dec 3 2019, 3:51 PM · Restricted Project
clayborg added a comment to D70989: [TypeCategory] IsApplicable doesn't seem to apply..

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 3 2019, 3:42 PM · Restricted Project
clayborg accepted D70954: [lldb] Don't put compile unit name into the support file list and support DWARF5 line tables.
Dec 3 2019, 10:05 AM · Restricted Project
clayborg added a comment to D70851: [lldb] s/FileSpec::Equal/FileSpec::Match.

lgtm

Dec 3 2019, 9:56 AM · Restricted Project

Dec 2 2019

clayborg added inline comments to D70882: Add environment var LLDBVSCODE_SKIP_INIT_FILES to lldb-vscode.
Dec 2 2019, 2:25 PM · Restricted Project
clayborg added inline comments to D70848: [LLDB] Set the right address size on output DataExtractors from ObjectFile.
Dec 2 2019, 2:16 PM · Restricted Project
clayborg added a comment to D70847: [lldb] Set executable module when adding modules to the Target.

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

Dec 2 2019, 11:38 AM · Restricted Project
clayborg requested changes to D70847: [lldb] Set executable module when adding modules to the Target.

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.

Dec 2 2019, 11:29 AM · Restricted Project
clayborg added inline comments to D70851: [lldb] s/FileSpec::Equal/FileSpec::Match.
Dec 2 2019, 10:43 AM · Restricted Project
clayborg added a comment to D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs.

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

Dec 2 2019, 9:29 AM · Restricted Project
clayborg added a comment to D70882: Add environment var LLDBVSCODE_SKIP_INIT_FILES to lldb-vscode.

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?

Dec 2 2019, 9:29 AM · Restricted Project
clayborg added inline comments to D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs.
Dec 2 2019, 9:29 AM · Restricted Project
clayborg added a comment to D70883: [vscode.py] Make read_packet only return None when EOF.

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

Dec 2 2019, 9:21 AM · Restricted Project
clayborg requested changes to D70885: [lldb] Use explicit lldb commands on tests.

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?

Dec 2 2019, 9:20 AM · Restricted Project
clayborg accepted D70884: [lldb] Fix TestFormattersSBAPI test.
Dec 2 2019, 9:20 AM · Restricted Project
clayborg requested changes to D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs.

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

Dec 2 2019, 9:20 AM · Restricted Project
clayborg added a comment to D70887: [lldb] Use realpath to avoid issues with symlinks in source paths.

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:

Dec 2 2019, 9:11 AM · Restricted Project
clayborg added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

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:

Dec 2 2019, 9:02 AM · Restricted Project
clayborg added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

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

Dec 2 2019, 9:02 AM · Restricted Project
clayborg accepted D70906: [lldb] Move register info "augmentation" from gdb-remote into ABI.

lgtm

Dec 2 2019, 8:24 AM · Restricted Project

Nov 25 2019

clayborg added a comment to D70532: [lldb] Improve/fix base address selection in location lists.

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.

Nov 25 2019, 10:07 AM · Restricted Project
clayborg added a comment to D70532: [lldb] Improve/fix base address selection in location lists.

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?

Nov 25 2019, 8:33 AM · Restricted Project
clayborg added a comment to D69309: Support template instantiation in the expression evaluator.

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.
Nov 25 2019, 8:15 AM · Restricted Project

Nov 18 2019

clayborg added a comment to D70387: [LLDB] [Windows] Allow making subprocesses use the debugger's console with LLDB_INHERIT_CONSOLE=true.

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 18 2019, 11:08 AM · Restricted Project

Nov 12 2019

clayborg accepted D69873: [lldb-vscode] support the completion request.
Nov 12 2019, 4:07 PM · Restricted Project
clayborg added a comment to D70127: [lldb-vscode] Fix a race in test_extra_launch_commands.

--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 12 2019, 3:57 PM · Restricted Project
clayborg added a comment to D70134: dotest: Add a way for the run_to_* helpers to register dylibs.

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.

Nov 12 2019, 3:57 PM · Restricted Project

Nov 11 2019

clayborg added a comment to D69873: [lldb-vscode] support the completion request.

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.

Nov 11 2019, 2:01 PM · Restricted Project

Oct 21 2019

clayborg added a comment to D69210: [Disassembler] Simplify MCInst predicates.
In D69210#1717042, @vsk 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?

Oct 21 2019, 11:42 AM
clayborg accepted D69273: ValueObject: Fix a crash related to children address type computation.

Looks good!

Oct 21 2019, 11:05 AM · Restricted Project
clayborg added a comment to D69210: [Disassembler] Simplify MCInst predicates.
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?

Oct 21 2019, 9:58 AM

Oct 18 2019

clayborg accepted D69106: MemoryRegion: Print "don't know" permission values as such.

Sounds good, thanks for doing this

Oct 18 2019, 10:02 AM · Restricted Project
clayborg accepted D69105: minidump: Create memory regions from the sections of loaded modules.
Oct 18 2019, 10:02 AM · Restricted Project