Page MenuHomePhabricator

clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Yesterday

clayborg requested changes to D59611: Move the rest of the section loading over to DWARFContext.

Use reference to DWARFContext instead of pointers? Apply to entire patch and good to go

Wed, Mar 20, 2:07 PM · Restricted Project
clayborg accepted D59562: [SymbolFileDWARF] Introduce DWARFContext.

The suggestion just avoids the Clear() call at the expense of a branch. No big deal. Fine either way.

Wed, Mar 20, 1:41 PM · Restricted Project
clayborg added inline comments to D59562: [SymbolFileDWARF] Introduce DWARFContext.
Wed, Mar 20, 1:36 PM · Restricted Project
clayborg added inline comments to D59562: [SymbolFileDWARF] Introduce DWARFContext.
Wed, Mar 20, 1:15 PM · Restricted Project
clayborg added inline comments to D59562: [SymbolFileDWARF] Introduce DWARFContext.
Wed, Mar 20, 1:14 PM · Restricted Project
clayborg added a comment to D59606: [lldb] Add missing EINTR handling.

LGTM as long as the test suite is happy

Wed, Mar 20, 1:03 PM · Restricted Project, Restricted Project
clayborg committed rG621e8b438701: Fix UUID decoding from minidump files (authored by clayborg).
Fix UUID decoding from minidump files
Wed, Mar 20, 9:53 AM
clayborg added a comment to D59482: [ObjectYAML] Add basic minidump generation support.

LGTM

Wed, Mar 20, 9:27 AM · Restricted Project
clayborg added inline comments to D59562: [SymbolFileDWARF] Introduce DWARFContext.
Wed, Mar 20, 8:38 AM · Restricted Project
clayborg added a comment to D59562: [SymbolFileDWARF] Introduce DWARFContext.

We can get rid of the m_dwarf_data all together. As Pavel suggested, we used to not mmap the entire file, and now we do most of the time. The Section::GetSectionData() will ref count the mmap data in the data extractor and use the mmap already, so we should only support grabbing the contents via the Section::GetSectionData() and don't propagate this legacy code in the new DWARFContext.

Wed, Mar 20, 8:32 AM · Restricted Project
clayborg added inline comments to D59433: Fix UUID decoding from minidump files..
Wed, Mar 20, 8:11 AM · Restricted Project
clayborg requested changes to D59562: [SymbolFileDWARF] Introduce DWARFContext.
Wed, Mar 20, 7:39 AM · Restricted Project
clayborg added a comment to D59532: Fix a bug where the bitwise-not'ed bitset was accidently being used.

A unit test would be nice so we catch future issues.

Wed, Mar 20, 7:22 AM
clayborg added inline comments to D59482: [ObjectYAML] Add basic minidump generation support.
Wed, Mar 20, 7:19 AM · Restricted Project

Mon, Mar 18

clayborg accepted D59498: [DWARF] Remove a couple of log statements.

I haven't used it in a long time. I can add it back temporarily if I ever need to.

Mon, Mar 18, 2:28 PM · Restricted Project
clayborg added a comment to D59498: [DWARF] Remove a couple of log statements.

Most of these logs seem useful. See inline comments.

Mon, Mar 18, 10:43 AM · Restricted Project

Fri, Mar 15

clayborg added a comment to D59433: Fix UUID decoding from minidump files..

"MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name?

Fri, Mar 15, 3:29 PM · Restricted Project
clayborg created D59433: Fix UUID decoding from minidump files..
Fri, Mar 15, 2:28 PM · Restricted Project
clayborg accepted D59414: Remove the TypePair class.
Fri, Mar 15, 7:53 AM

Thu, Mar 14

clayborg added inline comments to D59381: Change CompileUnit and ARanges interfaces to propagate errors.
Thu, Mar 14, 10:33 PM · Restricted Project
clayborg requested changes to D59381: Change CompileUnit and ARanges interfaces to propagate errors.

Must fix logic error as mentioned in inlined comments.

Thu, Mar 14, 4:24 PM · Restricted Project
clayborg accepted D59297: Delete type_sp member from TypePair.
Thu, Mar 14, 11:57 AM · Restricted Project
clayborg accepted D59370: Return llvm::Error and llvm::Expected from some DWARF parsing functions.

As long as there is not a large performance regress when parsing large DWARF files this looks good to me. Abbreviation declarations are small, so I don't expect one.

Thu, Mar 14, 11:48 AM · Restricted Project

Tue, Mar 12

clayborg added a comment to D59235: Remove Support for DWARF64.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

Tue, Mar 12, 7:19 AM · Restricted Project

Mon, Mar 11

clayborg added a comment to D59235: Remove Support for DWARF64.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Mon, Mar 11, 4:21 PM · Restricted Project
clayborg accepted D59164: [SymbolFileDWARF] Move ElaboratingDIEIterator into implementation file.
Mon, Mar 11, 4:17 PM · Restricted Project
clayborg added a comment to D59217: Fix/unify SBType comparison.

If no one is depending on type_sp, then ok to delete. Should be easy to test by removing it and seeing what breaks (if anything).

Mon, Mar 11, 4:16 PM · Restricted Project
clayborg added a comment to D59235: Remove Support for DWARF64.

What part of parsing DWARF64 wasn't working? I think the SPARC compiler was the only one producing DWARF64. Be a shame to lose the support. What is the plan here after this patch?

Mon, Mar 11, 4:07 PM · Restricted Project
clayborg committed rG0d6f681292d5: Fix a crasher in StackFrame::GetValueForVariableExpressionPath() (authored by clayborg).
Fix a crasher in StackFrame::GetValueForVariableExpressionPath()
Mon, Mar 11, 11:16 AM
clayborg accepted D59165: Remove DWARFDIECollection.
Mon, Mar 11, 10:14 AM · Restricted Project
clayborg accepted D59217: Fix/unify SBType comparison.
Mon, Mar 11, 10:09 AM · Restricted Project
clayborg added a comment to D58347: Reinitialize UnwindTable when the SymbolFile changes.

Since unwinding is Jason's thing, he should give the ok for this patch.

Mon, Mar 11, 10:03 AM · Restricted Project
clayborg added a comment to D58347: Reinitialize UnwindTable when the SymbolFile changes.

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

Mon, Mar 11, 10:03 AM · Restricted Project
clayborg updated the diff for D59200: Fix a crasher in StackFrame::GetValueForVariableExpressionPath().

Adrian's suggestion was right, much simpler test case now!

Mon, Mar 11, 9:11 AM · Restricted Project

Sun, Mar 10

clayborg created D59200: Fix a crasher in StackFrame::GetValueForVariableExpressionPath().
Sun, Mar 10, 5:37 PM · Restricted Project

Thu, Mar 7

clayborg accepted D59114: [lldb-vscode] Don't hang indefinitely on invalid program.
Thu, Mar 7, 3:53 PM · Restricted Project
clayborg added a comment to D59101: [SBAPI] Log from record macro.

As long as we are still seeing all argument values, I check the first 10 or so calls and it looked like this was the case, then I am good with this.

Thu, Mar 7, 2:18 PM · Restricted Project
clayborg accepted D59104: [lldb-vscode] Make server mode work on Windows.
Thu, Mar 7, 11:40 AM · Restricted Project

Wed, Mar 6

clayborg added a comment to D59030: Pass ConstString by value.

LGTM!

Wed, Mar 6, 2:24 PM · Restricted Project, Restricted Project
clayborg accepted D59043: [lldb-vscode] Correctly propagate errors back to VS Code.
Wed, Mar 6, 2:24 PM
clayborg committed rG6795eb388442: Fix core files for 32 bit architectures that are supported in ProcessELFCore.cpp (authored by clayborg).
Fix core files for 32 bit architectures that are supported in ProcessELFCore.cpp
Wed, Mar 6, 10:05 AM

Tue, Mar 5

clayborg accepted D58792: Add "operator bool" to SB APIs.
Tue, Mar 5, 2:56 PM · Restricted Project
clayborg updated the diff for D58985: Fix core files for 32 bit architectures that are supported in ProcessELFCore.cpp.

Fix zturner's suggestion.

Tue, Mar 5, 11:26 AM · Restricted Project
clayborg created D58985: Fix core files for 32 bit architectures that are supported in ProcessELFCore.cpp.
Tue, Mar 5, 10:52 AM · Restricted Project
clayborg added a comment to D58976: Introduce core2yaml tool.

Strong ownership is needed for this class IMHO because it hands out pointers to native data

Tue, Mar 5, 9:08 AM
clayborg added inline comments to D58976: Introduce core2yaml tool.
Tue, Mar 5, 9:08 AM
clayborg added a comment to D58976: Introduce core2yaml tool.

Do we want this in LLVM instead of lldb?

Tue, Mar 5, 8:42 AM
clayborg accepted D58975: Introduce MinidumpEnums.def textual header.
Tue, Mar 5, 8:41 AM
clayborg accepted D58946: [SBAPI] Don't check IsValid in constructor.

lgtm

Tue, Mar 5, 7:49 AM · Restricted Project, Restricted Project

Fri, Mar 1

clayborg added a comment to D58792: Add "operator bool" to SB APIs.

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

Fri, Mar 1, 2:45 PM · Restricted Project
clayborg added a comment to D58792: Add "operator bool" to SB APIs.

If this is new API I guess it is ok to have them all be const. I was mostly worried that you wouldn't be able to call a non const function from a const function. Do we have any IsValid() calls that are const? I believe we do. If so, how are we calling IsValid() if it is const from a non const conversion operator?

Fri, Mar 1, 2:38 PM · Restricted Project
clayborg added a comment to D58792: Add "operator bool" to SB APIs.

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

  1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
  2. give up on constness and just have a non-const operator bool everywhere
  3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that
Fri, Mar 1, 11:43 AM · Restricted Project
clayborg added a comment to D58838: Remove tautological #ifdefs.

I think we mostly wanted to catch these in the test suite. Probably best to convert to lldbassert

Fri, Mar 1, 11:27 AM · Restricted Project
clayborg added a comment to D58838: Remove tautological #ifdefs.

So it affects Apple directly!

Fri, Mar 1, 11:27 AM · Restricted Project
clayborg requested changes to D58838: Remove tautological #ifdefs.

LLDB_CONFIGURATION_BUILD_AND_INTEGRATION doesn't have them. So you can change these to

Fri, Mar 1, 11:26 AM · Restricted Project

Wed, Feb 27

clayborg accepted D58730: Rename Symbols.cpp from Host to Symbols/LocateSymbolFile.{h,cpp}.
Wed, Feb 27, 1:12 PM · Restricted Project
clayborg accepted D58699: Adapt the ObjC checker instrumentation to handle objc_msgSend with struct returns.

Sounds good.

Wed, Feb 27, 11:37 AM · Restricted Project
clayborg accepted D48752: Quiet command regex instructions during batch execution.
Wed, Feb 27, 11:34 AM · Restricted Project
clayborg accepted D58720: Remove unnecessary demangling operation (hopefully NFC).
Wed, Feb 27, 10:15 AM · Restricted Project, Restricted Project

Tue, Feb 26

clayborg accepted D58350: Insert random blocks of python code with swig instead of modify-python-lldb.py.
Tue, Feb 26, 2:54 PM · Restricted Project
clayborg added inline comments to D58699: Adapt the ObjC checker instrumentation to handle objc_msgSend with struct returns.
Tue, Feb 26, 2:51 PM · Restricted Project
clayborg accepted D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

Actually on re-reading the patch, we are detecting if anything wasn't set and setting it. Makes sense.

Tue, Feb 26, 2:34 PM · Restricted Project
clayborg requested changes to D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

Still issues from what I see. We need to know if something was specified or not and with your change we don't know the difference between the "none" state (which is current enum = unknown and string = "unknown") and the just not specified case (which is current enum = unknown and string = <empty>)

Tue, Feb 26, 2:30 PM · Restricted Project
clayborg added a comment to D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

@clayborg I recently ran into a similar issue and I think that perhaps adding explicit llvm::Triple::Any{Vendor|OS|...} enumerators to llvm::Triple to make this distinction explicit would be the cleanest solution.

Tue, Feb 26, 2:27 PM · Restricted Project
clayborg added a comment to D58678: Improve step over performance by not stopping at branches that are function calls and stepping into and them out of each one.

I have two questions about this patch.

  1. I want some llvm expert to weigh in on whether

    m_instructions[i]->IsCall()

    always means it returns to the next instruction after the call. That seems obvious, but since this patch depends on that being true, I'd like to know that it is guaranteed.
Tue, Feb 26, 11:38 AM · Restricted Project
clayborg added a comment to D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

I would be all for adding a "none" as a valid vendor, os or environment to the llvm triple class. Then all of the unspecified unknown stuff goes away. But I am not sure how likely the LLVM community will like this change since it won't be useful for non LLDB code.

Tue, Feb 26, 11:01 AM · Restricted Project
clayborg added a comment to D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

A "specified unknown" is when the user actually typed "unknown" in their triple. IIRC there is no "none" to specify that there is no OS, so we use "specified unknowns" for this case. In this case we expect the enum _and_ the string accessor value for vendor and/or OS to be the string "unknown".

This will work when the user types "armv7" as their triple, but when they type "armv7--linux" the vendor gets implicitly set to "unknown" even though the user didn't specify it.

I think this basically defeats the purpose of the IsUnspecifiedUnknown functions, which was to differentiate between foo-unknown-bar and foo--bar triples. That in itself was a pretty big hack, but it seems that there is functionality which depends on this. There was a discussion about this back in december http://lists.llvm.org/pipermail/lldb-dev/2018-December/014437.html, and IIRC it converged to some sort of a conclusion, but I don't think there hasn't been any effort to try to implement it. I'm not sure what exactly it means about the future of this patch, but I'd be cautious about it.

Part of what you said is inconsistent with my understanding. Specifically, if you specify a Triple with the string "foo-unknown-bar" and another Triple with the string "foo--bar" you end up with the same triple. We rely on the llvm::Triple::normalize function to create triples from strings, and there is no way to indicate using this method that a triple has unspecified non-optional parts. We are relying on ArchSpec being created with a Triple object and not a StringRef for this behavior to work.

Tue, Feb 26, 10:55 AM · Restricted Project
clayborg added inline comments to D58678: Improve step over performance by not stopping at branches that are function calls and stepping into and them out of each one.
Tue, Feb 26, 10:43 AM · Restricted Project
clayborg accepted D58664: [Utility] Fix ArchSpec.MergeFrom to correctly merge environments.
Tue, Feb 26, 9:59 AM · Restricted Project
clayborg created D58678: Improve step over performance by not stopping at branches that are function calls and stepping into and them out of each one.
Tue, Feb 26, 8:34 AM · Restricted Project
clayborg added a comment to D53379: GSYM symbolication format.

Hi Greg,

I've had a lot of time to review this (thanks for that) and do apologize for taking so long. I have a couple of concerns about this so bear with me and let's see where we can get:

a) This looks like it's a standalone tool that's being added to llvm, but that really doesn't involve anything coming out of llvm right now?

Tue, Feb 26, 8:30 AM
clayborg requested changes to D58653: [Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec.

I seem to remember that we currently expect that there are two cases when it comes to unknowns:

  • specified unknowns
  • unspecific unknowns
Tue, Feb 26, 7:16 AM · Restricted Project

Wed, Feb 20

clayborg accepted D58405: Merge target triple into module triple when constructing module from memory.

Looks good.

Wed, Feb 20, 1:39 PM · Restricted Project

Tue, Feb 19

clayborg added a comment to D58330: 01/03: new SectionPart for Section subranges (for effective .debug_types concatenation).

So I question the need for the SectionPart class. Seems like it would be easier to just add extra args to the ReadSectionData calls? Comments?

Tue, Feb 19, 2:45 PM · Restricted Project
clayborg added inline comments to D58405: Merge target triple into module triple when constructing module from memory.
Tue, Feb 19, 2:29 PM · Restricted Project
clayborg added inline comments to D58405: Merge target triple into module triple when constructing module from memory.
Tue, Feb 19, 2:05 PM · Restricted Project
clayborg added a comment to D58398: Add Facebook Minidump directory streams and options to dump them..

BTW: I did add a - character after the CHECK before I checked it in

Tue, Feb 19, 1:50 PM · Restricted Project
clayborg committed rGcc6ec692a4f5: Add Facebook Minidump directory streams and options to dump them. (authored by clayborg).
Add Facebook Minidump directory streams and options to dump them.
Tue, Feb 19, 1:48 PM

Feb 19 2019

clayborg accepted D51578: 02/03: Contiguous sections (.debug_info+.debug_types) for D54670==D32167 (.debug_types).
Feb 19 2019, 12:59 PM · Restricted Project
clayborg accepted D54670: 03/03: .debug_types: Update of D32167 (.debug_types) on top of D51578 (section concatenation).
Feb 19 2019, 12:53 PM · Restricted Project
clayborg added a comment to D54670: 03/03: .debug_types: Update of D32167 (.debug_types) on top of D51578 (section concatenation).

I am definitely ready for this to go in ASAP if we are good on this!

Feb 19 2019, 12:53 PM · Restricted Project
clayborg created D58398: Add Facebook Minidump directory streams and options to dump them..
Feb 19 2019, 11:25 AM · Restricted Project
clayborg added inline comments to D58347: Reinitialize UnwindTable when the SymbolFile changes.
Feb 19 2019, 9:17 AM · Restricted Project

Feb 14 2019

clayborg added a comment to D58222: [ClangExpressionParser] Reuse the FileManager from the compiler instance..

This change was probably due to things changing over time and there may be more of these kinds of cleanups to be had. nice catch.

Feb 14 2019, 2:08 PM · Restricted Project

Feb 13 2019

clayborg accepted D58129: Move UnwindTable from ObjectFile to Module.

Just a question of if we need an optional unwind table instance instead of just an instance. I am fine either way.

Feb 13 2019, 11:40 AM · Restricted Project
clayborg added inline comments to D58167: Refactor user/group name resolving code.
Feb 13 2019, 11:38 AM · Restricted Project

Feb 12 2019

clayborg committed rG46336896a33d: Fix Xcode project for RemoteAwarePlatform files. (authored by clayborg).
Fix Xcode project for RemoteAwarePlatform files.
Feb 12 2019, 8:55 AM
clayborg accepted D58131: [lldb] [unittest] Avoid mixing '127.0.0.1' and 'localhost'.

Yeah localhost can cause problems if someone has a custom mapping in their network config file.

Feb 12 2019, 8:48 AM · Restricted Project, Restricted Project
clayborg requested changes to D58129: Move UnwindTable from ObjectFile to Module.

You changed the API to return a pointer which can be NULL so we either need to check all returns for this or change the API to return a reference to a default constructed UnwindTable. I like the latter option, but I will leave that up to you. Since we are moving functionality into symbol files, we might want to opt for the latter to allow us to populate the UnwindTable as needed from any source when ever that source becomes available (like adding a symbol file to a module after the fact).

Feb 12 2019, 8:48 AM · Restricted Project
clayborg accepted D56537: ObjectFilePECOFF: Create a "container" section spanning the entire module image.
Feb 12 2019, 7:04 AM · Restricted Project, Restricted Project

Feb 11 2019

clayborg accepted D58052: Extract common PlatformPOSIX/Windows code into a separate class.
Feb 11 2019, 9:31 AM · Restricted Project
clayborg added inline comments to D56537: ObjectFilePECOFF: Create a "container" section spanning the entire module image.
Feb 11 2019, 9:27 AM · Restricted Project, Restricted Project

Feb 8 2019

clayborg accepted D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules.

Hi Greg, what do you think of my replies to your CreateModuleFromObjectFile comment?

Feb 8 2019, 10:21 AM · Restricted Project

Feb 7 2019

clayborg accepted D57895: Breakpad: auto-detect path style of file entries.
Feb 7 2019, 10:13 AM · Restricted Project

Feb 6 2019

clayborg accepted D56595: SymbolFileBreakpad: Add line table support.
Feb 6 2019, 8:01 AM · Restricted Project
clayborg added a comment to D56595: SymbolFileBreakpad: Add line table support.

Just bounds check "index" in parse compile unit and this is good to go

Feb 6 2019, 7:24 AM · Restricted Project
clayborg added a comment to D51387: Allow Template argument accessors to automatically unwrap parameter packs.

Do we have any callers that call GetNumTemplateArguments with false? If not, remove the argument?

Feb 6 2019, 7:12 AM

Feb 5 2019

clayborg accepted D57552: Handle "." in target.source-map in PathMapListing::FindFiles.

Thanks for the answers. LGTM

Feb 5 2019, 1:04 PM · Restricted Project, Restricted Project
clayborg added a comment to D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules.

Check the comment about m_platform_file

Feb 5 2019, 11:17 AM · Restricted Project
clayborg added a comment to D56595: SymbolFileBreakpad: Add line table support.

I like the way you did the compile units and the line tables and support file list. It would be nice to change this to do things more lazily. Right now we are parsing all compile unit data into CompUnitData structures and then passing their info along to the lldb_private::CompileUnit when we need to. We can be more lazy, see inlined comments.

Feb 5 2019, 7:40 AM · Restricted Project