clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (134 w, 6 d)

Recent Activity

Fri, Apr 21

clayborg resigned from D32340: [LLDB][MIPS] Fix TestMiExec.py failure.
Fri, Apr 21, 8:40 AM

Thu, Apr 20

clayborg requested changes to D32316: Change UniqueCStringMap to use ConstString as the key.

Very close. A few misuses of ConstString and this will be good to go.

Thu, Apr 20, 9:00 PM
clayborg requested changes to D32295: [RFC] Fix crash in expression evaluation.

This will break the Swift REPL as it relies on debug info and we won't be told about the ".debug_XXX" or "__debug_XXX" sections if we disable this. Jim or Sean should verify this and comment in the bug.

Thu, Apr 20, 5:17 PM

Wed, Apr 19

clayborg accepted D32221: Recompute ArchSpec core after MergeFrom.
Wed, Apr 19, 8:29 AM

Tue, Apr 18

clayborg added a comment to D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.

Yeah, I was having trouble getting a real read on what was failing due to a non Ubuntu setup at work for me. Is there any way to run a patch on one of the buildbots? If not, can you email me the list of what is failing?

Tue, Apr 18, 11:24 AM
clayborg resigned from D32168: [LLDB][MIPS] Fix TestStepOverBreakpoint.py failure.

Jim, can you take a look at this and see if this could be fixed in a nicer way? I would prefer to not see anything related to delay slots in a test. Can we abstract this better? Maybe ask the SBInstruction() if you can set a breakpoint on it instead of speaking in delay slot terms? Or maybe we ask the SBInstructionList to count the number of instructions between two addresses? Maybe something like:

Tue, Apr 18, 9:06 AM
clayborg created D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5.
Tue, Apr 18, 8:25 AM

Mon, Apr 17

clayborg accepted D31823: Update LLDB Host to support IPv6 over TCP.

I'm good if Pavel is good.

Mon, Apr 17, 11:27 AM

Fri, Apr 14

clayborg created D32087: Modify GDBRemoteCommunication::ScopedTimeout to not ever decrease a timeout.
Fri, Apr 14, 10:41 AM

Wed, Apr 12

clayborg added a comment to D31824: Update DebugServer to support IPv6 over TCP.

when users edit some config file they can have "localhost" map to some other port... Can't remember the unix file for this.

Wed, Apr 12, 2:37 PM
clayborg accepted D31824: Update DebugServer to support IPv6 over TCP.

Looks fine as long as all of the old modes work. :

  • "*:1234" that will accept a connection from any host on port 1234
  • "foo.bar.com:1234" only accept connections from "foo.bar.com" on port 1234
  • "localhost:1234" where make sure not to lookup "localhost" since people have been known to edit some config file and this no longer works, we currently just substitute "127.0.0.1" IPv4, and now we should or "::1" for IPv6
  • can't remember what used to happen when you only specify a port number, but that shouldn't change either
Wed, Apr 12, 2:36 PM

Mon, Apr 10

clayborg added a comment to D29581: Initial implementation of SB APIs for Tracing support..

Very close. A few main things:

  • JSON should use dictionaries and key/value pairs for all of the settings. Not sure if it was a typo where JSON array was mentioned a bunch of times?
  • Custom settings should be in a dictionary within the JSON using the plug-in's name as the key This allows us to easily detect which settings are supposed to be custom. Any custom settings must be understood or we get an error back.
  • New header file for TraceOptions
Mon, Apr 10, 8:27 AM

Thu, Mar 30

clayborg accepted D31485: Verify memory address range validity in GDBRemoteCommunicationClient.

Looks good.

Thu, Mar 30, 6:31 PM
clayborg requested changes to D31485: Verify memory address range validity in GDBRemoteCommunicationClient.

Simple logic change so we don't check the range validity more than once.

Thu, Mar 30, 2:19 PM

Mon, Mar 27

clayborg accepted D31280: [LLDB][MIPS] Fix Core file Architecture and OS information.

Looks good

Mon, Mar 27, 7:54 AM

Sun, Mar 26

clayborg accepted D31298: Bypass GetNormalizedFile when not necessary....

Much better.

Sun, Mar 26, 3:45 PM · Restricted Project

Mar 23 2017

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.

Mar 23 2017, 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.

Mar 23 2017, 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:

Mar 23 2017, 10:27 AM

Mar 16 2017

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.

Mar 16 2017, 7:47 PM
clayborg accepted D31065: Don't assign line table entries to code not contained in the entry....
Mar 16 2017, 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.

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

Looks fine.

Mar 16 2017, 7:43 PM

Mar 10 2017

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.

Mar 10 2017, 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

Mar 10 2017, 4:28 PM

Mar 7 2017

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

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

Mar 7 2017, 1:33 PM
clayborg added inline comments to D29581: Initial implementation of SB APIs for Tracing support..
Mar 7 2017, 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?

Mar 7 2017, 8:34 AM

Mar 6 2017

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.

Mar 6 2017, 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):

Mar 6 2017, 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.

Mar 6 2017, 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:

Mar 6 2017, 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.

Mar 6 2017, 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.

Mar 6 2017, 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,

Mar 6 2017, 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.

Mar 6 2017, 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.

Mar 6 2017, 1:48 PM

Feb 28 2017

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

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

Feb 28 2017, 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.

Feb 28 2017, 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.

Feb 28 2017, 8:45 AM

Feb 27 2017

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

Feb 23 2017

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

That is fine, just wanted to check.

Feb 23 2017, 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.

Feb 23 2017, 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.

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

Feb 22 2017

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.

Feb 22 2017, 11:15 AM
clayborg accepted D30251: Map ELF files as writable so we can update the relocations in them.
Feb 22 2017, 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