clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (208 w, 2 d)

Recent Activity

Yesterday

clayborg accepted D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.

Looks good

Thu, Sep 20, 9:30 AM

Wed, Sep 19

clayborg accepted D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.

Just a documentation suggestion, but looks good.

Wed, Sep 19, 4:13 PM
clayborg added a comment to D51934: [target] Change target create's behavior wrt loading dependent files..

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

Wed, Sep 19, 10:00 AM

Tue, Sep 18

clayborg requested changes to D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.
Tue, Sep 18, 3:06 PM
clayborg accepted D44437: Avoid GEP when creating a breakpoint.
Tue, Sep 18, 1:40 PM

Mon, Sep 17

clayborg accepted D52065: WWW docs for scripted breakpoint resolvers.

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

Mon, Sep 17, 1:01 PM
clayborg added inline comments to D52065: WWW docs for scripted breakpoint resolvers.
Mon, Sep 17, 7:31 AM

Fri, Sep 14

clayborg added a comment to D52065: WWW docs for scripted breakpoint resolvers.

See inlined comments.

Fri, Sep 14, 4:15 PM
clayborg accepted D52111: Make eSearchDepthFunction work for scripted resolvers.

Very nice. You got it right.

Fri, Sep 14, 11:34 AM

Thu, Sep 13

clayborg accepted D51935: [LLDB] - Improved DWARF5 support..

Thanks for doing all requested changes! Looks great.

Thu, Sep 13, 8:37 AM
clayborg added a comment to D51578: DWARFConcatenatingDataExtractor for D32167.

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.

Thu, Sep 13, 8:37 AM · Restricted Project
clayborg accepted D51730: [DWARFExpression] Read literars as unsigned values..

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.

Thu, Sep 13, 7:18 AM · Restricted Project
clayborg accepted D51830: Add a way to make scripted breakpoints.
Thu, Sep 13, 7:12 AM

Wed, Sep 12

clayborg added a comment to D51935: [LLDB] - Improved DWARF5 support..

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.

Wed, Sep 12, 1:25 PM
clayborg requested changes to D51935: [LLDB] - Improved DWARF5 support..

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.

Wed, Sep 12, 10:30 AM
clayborg added a comment to D51966: Do not create new terminals when launching process on Windows with --no-stdio.

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

Wed, Sep 12, 9:13 AM
clayborg added a comment to D51966: Do not create new terminals when launching process on Windows with --no-stdio.

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

Wed, Sep 12, 8:11 AM
clayborg added inline comments to D51934: [target] Change target create's behavior wrt loading dependent files..
Wed, Sep 12, 7:39 AM
clayborg added a comment to D51966: Do not create new terminals when launching process on Windows with --no-stdio.

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

Wed, Sep 12, 7:35 AM
clayborg accepted D51959: Add compatability version to liblldb in framework builds.
Wed, Sep 12, 7:33 AM
clayborg added a comment to D51830: Add a way to make scripted breakpoints.

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

Wed, Sep 12, 7:30 AM

Tue, Sep 11

clayborg requested changes to D51935: [LLDB] - Improved DWARF5 support..

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

Tue, Sep 11, 9:53 AM
clayborg added a reviewer for D51935: [LLDB] - Improved DWARF5 support.: clayborg.
Tue, Sep 11, 9:22 AM
clayborg added a comment to D51934: [target] Change target create's behavior wrt loading dependent files..

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

Tue, Sep 11, 9:22 AM
clayborg added a comment to D51934: [target] Change target create's behavior wrt loading dependent files..

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?

Tue, Sep 11, 9:17 AM
clayborg added inline comments to D51546: [WIP] Fix parsing of OSO entries for LTO optimized compile units.
Tue, Sep 11, 8:43 AM · Restricted Project
clayborg added a comment to D51830: Add a way to make scripted breakpoints.

See inlined comments. Cool stuff.

Tue, Sep 11, 8:11 AM

Thu, Sep 6

clayborg added a comment to D51730: [DWARFExpression] Read literars as unsigned values..

Can we add a test for this?

Thu, Sep 6, 7:07 AM · Restricted Project

Wed, Sep 5

clayborg accepted D51661: Print column info in backtraces et al. if available.
Wed, Sep 5, 4:48 PM
clayborg added inline comments to D51661: Print column info in backtraces et al. if available.
Wed, Sep 5, 10:32 AM
clayborg added a comment to D51661: Print column info in backtraces et al. if available.

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

Wed, Sep 5, 10:32 AM
clayborg accepted D51569: Hold GIL while allocating memory for PythonString..
Wed, Sep 5, 9:49 AM
clayborg added a comment to D51661: Print column info in backtraces et al. if available.

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

Wed, Sep 5, 9:42 AM
clayborg requested changes to D51661: Print column info in backtraces et al. if available.
Wed, Sep 5, 9:42 AM

Tue, Sep 4

clayborg added a comment to D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames).

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.

Tue, Sep 4, 11:32 AM
clayborg added a comment to D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames).

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.

Tue, Sep 4, 11:29 AM
clayborg added a comment to D51604: Terminate debugger if an assert was hit.

Thanks for review, commited as rL341387

Tue, Sep 4, 10:25 AM
clayborg added a comment to D51604: Terminate debugger if an assert was hit.

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.

Tue, Sep 4, 9:34 AM
clayborg added a comment to D51569: Hold GIL while allocating memory for PythonString..

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

Tue, Sep 4, 9:21 AM
clayborg added a comment to D51600: [NFC] Fixed enum constant in boolean context error.

If you insert the text:

Tue, Sep 4, 9:03 AM
clayborg added a comment to D48704: [LLDB] Fix for "Bug 37950: ExecutionContext::GetByteOrder() always returns endian::InlHostByteOrder()".

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

Tue, Sep 4, 8:27 AM · Restricted Project
clayborg added a comment to D51557: Replace uses of LazyBool with LazyBool template.

Very nice. LGTM

Tue, Sep 4, 8:25 AM
clayborg requested changes to D51546: [WIP] Fix parsing of OSO entries for LTO optimized compile units.
Tue, Sep 4, 8:18 AM · Restricted Project
clayborg accepted D51594: [ARC] Make char unsigned by default.
Tue, Sep 4, 7:41 AM
clayborg added a comment to D51569: Hold GIL while allocating memory for PythonString..

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?

Tue, Sep 4, 7:36 AM

Thu, Aug 30

clayborg accepted D51387: Allow Template argument accessors to automatically unwrap parameter packs.

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

Thu, Aug 30, 1:42 PM
clayborg added a comment to D44437: Avoid GEP when creating a breakpoint.

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?

Thu, Aug 30, 10:27 AM

Wed, Aug 29

clayborg updated the diff for D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files.

Make 16 byte UUIDs Apple specific.

Wed, Aug 29, 11:07 AM
clayborg added a comment to D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files.

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

Wed, Aug 29, 11:07 AM
clayborg added a comment to D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files.

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?

Wed, Aug 29, 11:07 AM
clayborg created D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files.
Wed, Aug 29, 10:11 AM
clayborg added a comment to D51387: Allow Template argument accessors to automatically unwrap parameter packs.

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.

Wed, Aug 29, 9:27 AM

Tue, Aug 28

clayborg added a comment to D51208: [DWARF] Fix dwarf5-index-is-used.cpp.

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.

Tue, Aug 28, 7:33 AM · Restricted Project
clayborg accepted D51245: Allow IRInterpreter to deal with non-power-of-2 sized types to support some bitfield accesses..
Tue, Aug 28, 7:16 AM
clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

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.

Tue, Aug 28, 7:13 AM

Thu, Aug 23

clayborg added a comment to D51176: Restrict the set of plugins used for ProcessMinidump.

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

Thu, Aug 23, 1:27 PM · Restricted Project
clayborg accepted D51176: Restrict the set of plugins used for ProcessMinidump.

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

Thu, Aug 23, 1:17 PM · Restricted Project
clayborg added a comment to D51176: Restrict the set of plugins used for ProcessMinidump.

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.

Thu, Aug 23, 12:17 PM · Restricted Project
clayborg added a comment to D50912: Don't cancel the current IOHandler when we push a handler for an utility function run..

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

Thu, Aug 23, 11:36 AM
clayborg added a comment to D51176: Restrict the set of plugins used for ProcessMinidump.

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.

Thu, Aug 23, 11:35 AM · Restricted Project
clayborg added a comment to D51176: Restrict the set of plugins used for ProcessMinidump.

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.

Thu, Aug 23, 11:08 AM · Restricted Project
clayborg requested changes to D51176: Restrict the set of plugins used for ProcessMinidump.

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.

Thu, Aug 23, 11:07 AM · Restricted Project
clayborg added inline comments to D50912: Don't cancel the current IOHandler when we push a handler for an utility function run..
Thu, Aug 23, 11:04 AM

Wed, Aug 22

clayborg accepted D50918: Add include directory for libxml on macOS.
Wed, Aug 22, 3:25 PM
clayborg added a comment to D50912: Don't cancel the current IOHandler when we push a handler for an utility function run..

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

Wed, Aug 22, 3:11 PM
clayborg added a comment to D50481: Fix broken builtin functions in the expression command.

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.

Wed, Aug 22, 2:10 PM · Restricted Project
clayborg added inline comments to D50912: Don't cancel the current IOHandler when we push a handler for an utility function run..
Wed, Aug 22, 2:07 PM

Aug 20 2018

clayborg added a comment to D49685: LLDB does not respect platform sysroot when loading core on Linux.

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)
Aug 20 2018, 10:55 AM

Aug 16 2018

clayborg added inline comments to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
Aug 16 2018, 10:29 AM
clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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

Aug 16 2018, 10:26 AM
clayborg updated subscribers of D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

Inline comments really help if you don't mind.

Aug 16 2018, 9:26 AM
clayborg added inline comments to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
Aug 16 2018, 9:20 AM
clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

Fix all of zturner's inline comments.

Aug 16 2018, 9:18 AM

Aug 15 2018

clayborg added inline comments to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
Aug 15 2018, 4:48 PM
clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

Fixed almost all issues mentioned by zturner.

Aug 15 2018, 4:41 PM
clayborg added inline comments to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
Aug 15 2018, 2:49 PM
clayborg added a comment to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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

Aug 15 2018, 8:15 AM

Aug 14 2018

clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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

Aug 14 2018, 5:46 PM

Aug 13 2018

clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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

Aug 13 2018, 5:02 PM
clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
  • Use the LLVM JSON parser
  • Split lldb-vscode.cpp into smaller files
  • Fix function names
  • ran clang format on everything
Aug 13 2018, 3:20 PM

Aug 10 2018

clayborg added inline comments to D44437: Avoid GEP when creating a breakpoint.
Aug 10 2018, 10:50 AM

Aug 9 2018

clayborg added a comment to D50473: [Demangle] Add another test for ItaniumPartialDemangler.

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]
 }
 ^
Aug 9 2018, 9:23 AM

Aug 8 2018

clayborg added a comment to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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.
Aug 8 2018, 2:04 PM
clayborg added a comment to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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.

Aug 8 2018, 8:40 AM

Aug 7 2018

clayborg added a comment to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

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

Aug 7 2018, 10:05 AM
clayborg updated the diff for D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

Removed dead code from python files.

Aug 7 2018, 9:24 AM

Aug 6 2018

clayborg created D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.
Aug 6 2018, 3:24 PM
clayborg added a comment to D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2)..

More offsetof issues:

Aug 6 2018, 10:27 AM
clayborg added a comment to D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2)..

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

Aug 6 2018, 10:09 AM
clayborg added a comment to D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2)..

Fixed offsetof issues with:

Aug 6 2018, 10:08 AM
clayborg updated the diff for D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2)..

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

Aug 6 2018, 9:54 AM
clayborg created D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2)..
Aug 6 2018, 8:13 AM

Aug 4 2018

clayborg added a comment to D50274: Misc module/dwarf logging improvements.

LGTM after Pavel's inline comments are addressed.

Aug 4 2018, 10:06 AM · Restricted Project

Aug 1 2018

clayborg added inline comments to D49750: Add support for ARM and ARM64 breakpad generated minidump files..
Aug 1 2018, 2:07 PM
clayborg updated the diff for D49750: Add support for ARM and ARM64 breakpad generated minidump files..
  • run clang-format
  • fix doxygen parameter names
Aug 1 2018, 2:06 PM
clayborg added a comment to D50038: Introduce install-lldb-framework target.

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

Aug 1 2018, 9:40 AM

Jul 31 2018

clayborg added inline comments to D49750: Add support for ARM and ARM64 breakpad generated minidump files..
Jul 31 2018, 1:29 PM
clayborg updated the diff for D49750: Add support for ARM and ARM64 breakpad generated minidump files..

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

Jul 31 2018, 1:17 PM
clayborg added a comment to D49963: Preliminary patch to support prompt interpolation.

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.

Jul 31 2018, 11:32 AM
clayborg added inline comments to D49750: Add support for ARM and ARM64 breakpad generated minidump files..
Jul 31 2018, 10:47 AM