clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.
User Since
Sep 23 2014, 10:11 AM (130 w, 2 d)

Recent Activity

Today

clayborg requested changes to D31298: Bypass GetNormalizedFile when not necessary....

Since we are going for speed here, I added a few suggestions for making it quicker.

Thu, Mar 23, 5:03 PM · Restricted Project
clayborg added a comment to D31280: [LLDB][MIPS] Fix Core file Architecture and OS information.

Let me know what you think about the MergeFrom comment. I am generally ok with this, but just wanted to check in case the merge made any sense in this patch somewhere.

Thu, Mar 23, 10:55 AM
clayborg added a comment to D31172: Move stop info override callback code from ArchSpec into Process.

It almost seems like we need some sort of an architecture plug-in here. Maybe something like Architecture plugins. The Architecture::FindPlugin() would take an ArchSpec and return a lldb_private::Architecture class instance that can be cached in the target or process. Currently the class would look like:

Thu, Mar 23, 10:27 AM

Thu, Mar 16

clayborg added a comment to D30785: [DWARF] Versioning for DWARF constants; verify FORMs.

Much better, I am good with this. I will let Dave Blaikie give the final OK.

Thu, Mar 16, 7:47 PM
clayborg accepted D31065: Don't assign line table entries to code not contained in the entry....
Thu, Mar 16, 7:46 PM
clayborg accepted D30454: [LLDB][MIPS] Check if memory_info.GetName() is empty before finding corresponding module..

Looks good, sorry for the delay.

Thu, Mar 16, 7:45 PM
clayborg accepted D31015: Get ObjectFileMachO to handle @executable_path in main executable's LC_LOAD_DYLIB commands.

Looks fine.

Thu, Mar 16, 7:43 PM

Fri, Mar 10

clayborg added a comment to D30006: [lldb] Remove the "MemoryHistory" plugin type and merge it into InstrumentationRuntime [NFC].

Looks ok to me. Jim, are you ok with this? If Jim is OK with this, then I am.

Fri, Mar 10, 4:34 PM · Restricted Project
clayborg added a comment to D30785: [DWARF] Versioning for DWARF constants; verify FORMs.

I wonder since we are adding new macro parameters to some macros if we shouldn't add two params: version and vendor for anything that uses the special versions. Currently you are overloading version with vendor into a single integer, why not add both version and vendor? Then vendor could be

Fri, Mar 10, 4:28 PM

Tue, Mar 7

clayborg accepted D30702: Fix remaining threading issues in Log.h.

With my limited std::atomic experience, this seems fine.

Tue, Mar 7, 1:33 PM
clayborg added inline comments to D29581: Initial implementation of SB APIs for Tracing support..
Tue, Mar 7, 8:49 AM
clayborg added a comment to D30454: [LLDB][MIPS] Check if memory_info.GetName() is empty before finding corresponding module..

How does VSDO get loaded when debugging a normal process? In the core file case you probably need to create the VSDO module from memory by locating it somehow and add it to the target before the dynamic loader goes looking for it. Wouldn't that fix the issue?

Tue, Mar 7, 8:34 AM

Mon, Mar 6

clayborg accepted D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions.

Ok, sounds like everyone thought through the solution. Lets start with this and we can iterate if needed.

Mon, Mar 6, 5:24 PM
clayborg added a comment to D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions.

Jim Ingham said (in email):

Mon, Mar 6, 4:56 PM
clayborg requested changes to D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions.

Very close. Can we try to get UpdateAutomaticSignalFiltering out of lldb_private::Process as my inline comments suggest? It would be cleaner and I am not sure we actually need Process::UpdateAutomaticSignalFiltering() for all processes.

Mon, Mar 6, 4:10 PM
clayborg added a comment to D30664: [DWARFv5] Update definitions to match published spec.

The reason I bring up the version is we want a "llvm-dwarfdump --verify" eventually and this might help to provide warnings like:

Mon, Mar 6, 3:14 PM
clayborg added a comment to D30664: [DWARFv5] Update definitions to match published spec.

Seem like a lot of the Dwarf.def macros could use an extra "version" parameter. Might help code to verify that certain tags, attributes, forms, or more are valid for the current DWARF version.

Mon, Mar 6, 2:41 PM
clayborg added a comment to D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions.

My main objection is that if we have 10 "lldb_private::Process::Will*" functions and only some require you to call the superclass, then it is confusing. It is also hard to enforce. We probably have other process subclasses that override these functions and they all would be broken. It also makes it harder when merging code to other branches that might have an extra process subclass. The merge would go fine, but any process subclasses that exist only in other branches would now be out of date and doing the wrong thing by not calling the superclass.

Mon, Mar 6, 2:36 PM
clayborg requested changes to D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions.

Pretty close. My only objection is we have many "lldb_private::Process::Will" and "lldb_private::Process::Did" prefixed functions and none of them are required to call the superclass version. I would prefer that this doesn't change. See my inlined comments,

Mon, Mar 6, 2:34 PM
clayborg requested changes to D29581: Initial implementation of SB APIs for Tracing support..

Much better. Found some extra stuff. In general we should be using SBStructuredData when ever we want to get/set stuff from structured data like JSON, XML or Apple plist, etc. If we make the APIs use SBStructuredData instead of using a SBStream, we can add constructors to SBStructuredData to init with JSON, XML, Apple plist, and more. All this will be parsed into a SBStructuredData or StructuredData::ObjectSP underneath, and then all people will use the StructuredData::ObjectSP on the inside.

Mon, Mar 6, 2:18 PM
clayborg added a comment to D30454: [LLDB][MIPS] Check if memory_info.GetName() is empty before finding corresponding module..

So a ModuleSpec allows you to specify a module by path, UUID and many other things. This is falling down for a magic file that doesn't actually exist right? "vsdo" is just a made up name for the table of loaded shared libraries? Is that correct? I need to understand what is going on before I can offer correct guidance.

Mon, Mar 6, 1:48 PM

Tue, Feb 28

clayborg resigned from D30457: [LLDB][MIPS] Core Dump Support.

I will let Pavel do this review as it is in his area of expertise.

Tue, Feb 28, 9:33 AM
clayborg added a comment to D30454: [LLDB][MIPS] Check if memory_info.GetName() is empty before finding corresponding module..

Object name is used for a module spec where the file is "/tmp/foo.a" and the object name is "bar.o". So this is for named objects within a container file like a BSD archive. If there is no object name, it doesn't need to be matched. If there is an object name, then the object name must match. All of the other tests are returning false when a there is no match. The original code is correct.

Tue, Feb 28, 8:47 AM
clayborg requested changes to D30454: [LLDB][MIPS] Check if memory_info.GetName() is empty before finding corresponding module..

This logic is wrong. If there is no object name, true should be returned. The old logic is correct.

Tue, Feb 28, 8:45 AM

Mon, Feb 27

clayborg accepted D30374: Support NetBSD Thread ID in lldb-server tests.
Mon, Feb 27, 8:48 AM · Restricted Project
clayborg accepted D30334: Remove the callback-based log channel registration mechanism.
Mon, Feb 27, 8:35 AM

Thu, Feb 23

clayborg accepted D30287: Introduce support for Debug Registers in RegisterContextNetBSD_x86_64.

That is fine, just wanted to check.

Thu, Feb 23, 2:08 PM · Restricted Project
clayborg added a comment to D30054: Delete DataBufferMemoryMap.

I would still vote to check Buffer for NULL. GetByteSize() and GetBytes() are usually accessed one time so there won't be a performance issue. If anyone wanted to actually use a DataBufferLLVM as a member variable, they would need to use a std::unique_ptr to one with the current designed because it can't be default constructed.

Thu, Feb 23, 1:01 PM
clayborg requested changes to D30054: Delete DataBufferMemoryMap.

In general code around crashes, we don't want to introduce any crashes in LLDB (no llvm_unreachable, asserts ok if code still functions without them). See inlined comments.

Thu, Feb 23, 12:17 PM
clayborg accepted D30288: Switch NetBSD from paccept(2) to accept4(2).
Thu, Feb 23, 11:51 AM · Restricted Project
clayborg added inline comments to D30287: Introduce support for Debug Registers in RegisterContextNetBSD_x86_64.
Thu, Feb 23, 11:50 AM · Restricted Project

Wed, Feb 22

clayborg accepted D30255: Ensure lldb-server waits for child debug servers to start up when passing them a port number to listen on..

Looks good.

Wed, Feb 22, 11:15 AM
clayborg accepted D30251: Map ELF files as writable so we can update the relocations in them.
Wed, Feb 22, 11:12 AM

Feb 20 2017

clayborg added a comment to D29669: Hardware breakpoints implementation for Arm/AArch64 targets.

LGTM, but Pavel should give the ok as well.

Feb 20 2017, 11:25 AM
clayborg accepted D30168: Log: Fix race in accessing the stream variable.
Feb 20 2017, 9:55 AM

Feb 17 2017

clayborg accepted D30094: Fix a couple of corner cases in NameMatches.
Feb 17 2017, 9:02 AM

Feb 16 2017

clayborg added inline comments to D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin.
Feb 16 2017, 9:22 AM
clayborg requested changes to D29964: Finish breaking the dependency from lldbUtility -> Host.

Please add Xcode changes I sent you offline and this will be good to go.

Feb 16 2017, 8:53 AM
clayborg accepted D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin.

Looks good.

Feb 16 2017, 8:52 AM

Feb 15 2017

clayborg added a comment to D30007: [lldb] Provide API to know which sanitizer generated an eStopReasonInstrumentation.

I am fine as long is Jim Ingham is fine. Jim, if you are good with this, just put it into Accept Revision.

Feb 15 2017, 3:06 PM · Restricted Project

Feb 14 2017

clayborg accepted D29932: Fix TestNameLookup for GCC.
Feb 14 2017, 9:12 AM

Feb 13 2017

clayborg added a comment to D29909: Break some more dependencies in lldbUtility.

I don't know enough to say for sure. Go ahead and try it with this patch as the files are pretty simple and losing the history wouldn't be too bad on these files. If it fails to have history, then you should use an SVN workflow for any future moves.

Feb 13 2017, 3:11 PM
clayborg added a comment to D19603: Fix entry point lookup for ObjectFilePECOFF..

Anyone still care about this? It would be nice to move it along or abandon it.

Feb 13 2017, 3:01 PM
clayborg added a comment to D29909: Break some more dependencies in lldbUtility.

Hopefully we are maintaining the SVN history for these files? Hard to tell from the patch.

Feb 13 2017, 3:00 PM
clayborg accepted D29909: Break some more dependencies in lldbUtility.

Looks fine.

Feb 13 2017, 2:59 PM
clayborg accepted D29895: Refactor log channel registration mechanism.

Looks good!

Feb 13 2017, 10:07 AM
clayborg requested changes to D29581: Initial implementation of SB APIs for Tracing support..

Overall comments:

  • Check the SB API guidelines and submit new patch (https://lldb.llvm.org/SB-api-coding-rules.html)
  • It might be nice to make a lldb_private::Trace and a lldb::SBTrace object and have that be the object we used instead of adding many calls to SBProcess. You get a SBTrace object out of a SBProcess with:
class SBProcess
{
  SBTrace StartTrace(SBTraceOptions &opts, SBError &error);
  SBTrace GetTrace(); // Get current trace object, might return invalid/empty SBTrace object
};

Then all calls to start, stop, get config, get data, get metadata, are on the SBTrace object.

Feb 13 2017, 9:50 AM

Feb 10 2017

clayborg accepted D29823: Clean up debug logging.
Feb 10 2017, 8:37 AM

Feb 9 2017

clayborg added a comment to D29669: Hardware breakpoints implementation for Arm/AArch64 targets.

If it isn't making too much extra code I am fine with it being separate if you believe this is the right way forward. Give it some thought and if you still believe they should be separate, I won't object. It might be nice to send the HardwareBreakpoint structure down to the functions that are going to enable/disable the HW breakpoints instead of individual arguments as you may end up adding members to the HardwareBreakpoint in the future and this will keep you from having to update all of the signatures.

Feb 9 2017, 9:20 AM

Feb 8 2017

clayborg requested changes to D29669: Hardware breakpoints implementation for Arm/AArch64 targets.

I would prefer to see NativeBreakpoint struct expanded to have more member variables instead of adding a new hardware breakpoint list. Then you just ask any breakpoint to enable/disable/remove itself and the structure contains all of the info we need. Keeping two lists means we have to check two lists. Let me know if any of my inline comments weren't clear?

Feb 8 2017, 10:11 AM
clayborg accepted D29615: Convert Log class to llvm streams.

Looks good as long as if we type two log enable commands like:

Feb 8 2017, 9:38 AM

Feb 6 2017

clayborg accepted D29288: Switch std::call_once to llvm::call_once.

Thanks for doing the right thing in LLVM first, looks great!

Feb 6 2017, 9:07 AM · Restricted Project
clayborg added a comment to D29288: Switch std::call_once to llvm::call_once.

See inlined comment about initialization when llvm::once_flag is a member variable

Feb 6 2017, 9:00 AM · Restricted Project

Feb 3 2017

clayborg added a comment to D29514: Change Error::PutToLog to LLDB_LOG_ERROR.

Much better, just clean up the unnecessary includes and we are good to go.

Feb 3 2017, 3:14 PM
clayborg added inline comments to D29510: Remove LIBLLDB_LOG_VERBOSE category.
Feb 3 2017, 3:12 PM
clayborg accepted D29510: Remove LIBLLDB_LOG_VERBOSE category.

Looks good.

Feb 3 2017, 2:29 PM
clayborg added a comment to D29514: Change Error::PutToLog to LLDB_LOG_ERROR.

So I agree with Pavel. Let us know what you think

Feb 3 2017, 2:04 PM
clayborg added a comment to D29514: Change Error::PutToLog to LLDB_LOG_ERROR.

I realize the functionality would add a "error: " prefix if it wasn't there, but it seems like we could add this as a formatting option of the lldb_private::Error class? Then we can just switch the logging code to put the error in as a log item and add the extra flag to ensure we print "error:" it is isn't already prefixed with that?

Feb 3 2017, 2:04 PM
clayborg added a comment to D29496: Push down more common code into PlatformPOSIX.

indeed very nice

Feb 3 2017, 9:18 AM
clayborg accepted D29496: Push down more common code into PlatformPOSIX.
Feb 3 2017, 9:06 AM

Feb 1 2017

clayborg accepted D29406: Unify PlatformPOSIX::ResolveExecutable.
Feb 1 2017, 2:22 PM
clayborg accepted D29403: Fix multi-process-driver.cpp build on NetBSD.
Feb 1 2017, 10:28 AM · Restricted Project

Jan 31 2017

clayborg accepted D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..

If we can add a comment around "bndcfgu" where we use an SBData since it is a vector register and SBValue::GetValueAsUnsigned(...) won't work because it is a vector that would be nice for people to know. Other than that this is good to go!

Jan 31 2017, 8:49 AM

Jan 30 2017

clayborg added a comment to D29296: Make llvm::call_once more convenient to reuse out of LLVM.

LGTM, but I will let other LLVM folks give it the final approval.

Jan 30 2017, 12:43 PM
clayborg requested changes to D29288: Switch std::call_once to llvm::call_once.

Be very careful when using this, you can't change member variables that used to be std::once to be statics. We also don't need the llvm namespace to be included with "using namespace llvm;" in many of the files.

Jan 30 2017, 9:56 AM · Restricted Project
clayborg accepted D29215: [LLDB][MIPS] Fix TestMiniDumpNew.
Jan 30 2017, 9:31 AM
clayborg accepted D29266: Synchronize PlatformNetBSD with Linux.
Jan 30 2017, 9:27 AM · Restricted Project
clayborg accepted D29264: Add NetBSD support in Host::GetCurrentThreadID.
Jan 30 2017, 9:26 AM · Restricted Project

Jan 25 2017

clayborg added a comment to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..

Let me know why your GetValueAsUnsigned isn't working on your register by stepping through it.

Jan 25 2017, 2:02 PM
clayborg added a comment to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..

I fixed SBData with:

Sending        packages/Python/lldbsuite/test/python_api/sbdata/TestSBData.py
Sending        source/API/SBData.cpp
Transmitting file data ..done
Committing transaction...
Committed revision 293102.
Jan 25 2017, 2:02 PM
clayborg added inline comments to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..
Jan 25 2017, 11:30 AM
clayborg added inline comments to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..
Jan 25 2017, 11:25 AM

Jan 24 2017

clayborg accepted D29091: Recognize Real-Time Signals on NetBSD.
Jan 24 2017, 10:57 AM · Restricted Project
clayborg accepted D29089: Switch HostInfoNetBSD::GetProgramFileSpec to sysctl(7).
Jan 24 2017, 10:14 AM · Restricted Project
clayborg added a comment to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..

Noticed another efficiency thing, see inlined comment.

Jan 24 2017, 9:47 AM
clayborg requested changes to D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables..

There are a few places where you are reading memory and then wanting to decode data from it. Right now, you first read memory into a local buffer and then we create an SBData and set the data to the bytes. It would be nice to have the ability to read memory and get an SBData back:

class SBProcess
{
  SBData ReadData(lldb::addr_t addr, size_t size, SBError &error);
};

Since the process can fill in the byte order and address byte size it can return an SBData that is ready to use. If you are interested in adding this in this patch, let me know.

Jan 24 2017, 9:41 AM

Jan 23 2017

clayborg accepted D29036: Add format_provider for lldb::StateType.

Looks good as long as the test suite is happy.

Jan 23 2017, 11:11 AM
clayborg added inline comments to D29036: Add format_provider for lldb::StateType.
Jan 23 2017, 9:45 AM
clayborg requested changes to D29036: Add format_provider for lldb::StateType.

Very close, just add a test for a bad value since someone can have code like:

Jan 23 2017, 9:43 AM
clayborg accepted D28808: Fix a bug where lldb does not respect the packet size..

Looks good.

Jan 23 2017, 8:49 AM
clayborg accepted D28944: Provide option to set pc of the file loaded in memory..

Looks good.

Jan 23 2017, 8:48 AM

Jan 20 2017

clayborg requested changes to D28808: Fix a bug where lldb does not respect the packet size..

See easy inline fix and this will be good to go

Jan 20 2017, 10:06 AM
clayborg requested changes to D28944: Provide option to set pc of the file loaded in memory..

Just set an error when "set_pc" is true and you get no entry point back from thee object file and this is good to go.

Jan 20 2017, 10:04 AM

Jan 19 2017

clayborg accepted D28804: Provide a substitute to load command of gdb.

Very nice!

Jan 19 2017, 9:14 AM

Jan 18 2017

clayborg requested changes to D28808: Fix a bug where lldb does not respect the packet size..

A few cleanups on the logging. See inlined comments.

Jan 18 2017, 8:50 AM

Jan 17 2017

clayborg added a comment to D28828: [DWARF] [ObjectYAML] Adding APIs for unittesting.

Looks fine to me. I'll let David chime in though just to be sure.

Jan 17 2017, 3:06 PM
clayborg accepted D27459: Add a more succinct logging syntax.

Looks good.

Jan 17 2017, 10:54 AM
clayborg requested changes to D28808: Fix a bug where lldb does not respect the packet size..

We need to check stub_max_size to make sure we don't get an integer underflow.

Jan 17 2017, 9:33 AM
clayborg requested changes to D28804: Provide a substitute to load command of gdb.

We need to agree on options names and move the object file loading code into ObjectFile.cpp. See inlined comments.

Jan 17 2017, 9:23 AM

Jan 13 2017

clayborg retitled D28704: Add a variant of DWARFDie::find() and DWARFDie::findRecursively() that takes a llvm::ArrayRef<dwarf::Attribute>. from to Add a variant of DWARFDie::find() and DWARFDie::findRecursively() that takes a llvm::ArrayRef<dwarf::Attribute>. .
Jan 13 2017, 2:30 PM
clayborg accepted D28677: FileSpec: Fix PrependPathComponent("/").
Jan 13 2017, 1:26 PM

Jan 12 2017

clayborg updated the diff for D28386: Add the ability to iterate across all attributes in a DIE..

Remove code after llvm_unreachable

Jan 12 2017, 4:22 PM
clayborg updated the diff for D28386: Add the ability to iterate across all attributes in a DIE..

Updated with suggestions. I can't make an invalid form test yet until we can use the yam2obj.

Jan 12 2017, 4:12 PM
clayborg abandoned D28613: Alternative proposal to https://reviews.llvm.org/D28581 where we return a DWARFormValue from DWARFDie::find*() functions.

After speaking with Adrian who had spoken with Dave Blaike, we settled on using the other proposal.

Jan 12 2017, 4:08 PM
clayborg updated the diff for D28581: Change DWARFDie::getAttributeValue() to DWARFDie::find(), add DWARFDie::findRecurse() and dwarf::toString() helper functions..

This is a proposed final solution with tests and all needed fixes.

Jan 12 2017, 3:14 PM
clayborg added inline comments to D28386: Add the ability to iterate across all attributes in a DIE..
Jan 12 2017, 10:55 AM
clayborg accepted D28616: Remove a couple of Stream flags.

Most of the DWARF stuff is about to go away anyway in favor of using the LLVM DWARF parser as I am currently modifying it to support all we need in LLDB so we can get rid of the entire DWARF parsers, so many of these changes are fine.

Jan 12 2017, 10:31 AM
clayborg added a comment to D28613: Alternative proposal to https://reviews.llvm.org/D28581 where we return a DWARFormValue from DWARFDie::find*() functions.

Whoops I accidentally added "aam" as a reviewer instead of Adrian...

Jan 12 2017, 9:27 AM
clayborg added a comment to D28581: Change DWARFDie::getAttributeValue() to DWARFDie::find(), add DWARFDie::findRecurse() and dwarf::toString() helper functions..

Alternate approach in https://reviews.llvm.org/D28613

Jan 12 2017, 9:21 AM
clayborg retitled D28613: Alternative proposal to https://reviews.llvm.org/D28581 where we return a DWARFormValue from DWARFDie::find*() functions from to Alternative proposal to https://reviews.llvm.org/D28581 where we return a DWARFormValue from DWARFDie::find*() functions.
Jan 12 2017, 9:20 AM