Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 5:24 PM (225 w, 5 d)

Recent Activity

Fri, Jan 18

jingham committed rLLDB351585: Add BreakpadRecords to the Xcode project..
Add BreakpadRecords to the Xcode project.
Fri, Jan 18, 12:25 PM
jingham committed rL351585: Add BreakpadRecords to the Xcode project..
Add BreakpadRecords to the Xcode project.
Fri, Jan 18, 12:25 PM

Tue, Jan 15

jingham accepted D56755: [lldb-mi] Remove use of dialog box.

LGTM

Tue, Jan 15, 4:07 PM
jingham added a comment to D56741: [CMake] Explicitly list User32 as dependency of lldb-mi.

That seems right. TTTT, IME on a decently fast Linux or macOS machine "attach -w" will get lldb attached to the process way before it would have gotten this far, so I'm not sure how useful this convenience is altogether. But maybe "attach -w" is slower on Windows.

Tue, Jan 15, 3:42 PM
jingham added a comment to D56741: [CMake] Explicitly list User32 as dependency of lldb-mi.

As far as I can tell, this dialog is only used when you build the lldb-mi with MICONFIG_DEBUG_SHOW_ATTACH_DBG_DLG, which you would only do if you wanted to debug lldb-mi. The point is to stall the lldb-mi launch at an early point till you've gotten a debugger attached to it. Then you can click the MessageBox OK to let lldb-mi proceed. On all the other systems it does a while (var) sleep; type thing, so you attach and change the value of var with the expression parser and continue.

Tue, Jan 15, 3:31 PM

Mon, Jan 14

jingham added a comment to D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit.

But if everyone else thinks differently, I'll go with the majority. Just my initial thoughts.

Mon, Jan 14, 6:37 PM

Dec 21 2018

jingham added a comment to D55318: [Expressions] Add support of expressions evaluation in some object's context.

The only reservation I have is that this really ought to work correctly for ObjC as well as C++, and there's code to support that, but it isn't tested. I don't have time to write ObjC tests before the new year for sure. Is it possible to rope somebody else into that? If that's not possible, maybe check it in with the ObjC support off and a big FIXME and file a bug to re-enable that support with tests.

Dec 21 2018, 11:07 AM · Restricted Project

Dec 20 2018

jingham committed rL349874: "help finish" tells you it is an alias. "help fin" doesn't..
"help finish" tells you it is an alias. "help fin" doesn't.
Dec 20 2018, 5:49 PM
jingham committed rLLDB349874: "help finish" tells you it is an alias. "help fin" doesn't..
"help finish" tells you it is an alias. "help fin" doesn't.
Dec 20 2018, 5:48 PM
jingham accepted D55954: [lldb] Add a "display-recognized-arguments" target setting to show recognized arguments by default.

Excellent!

Dec 20 2018, 3:39 PM · Restricted Project
jingham added a comment to D55954: [lldb] Add a "display-recognized-arguments" target setting to show recognized arguments by default.

That's great. Can you add a test that if you make a default SBVariableOption and then flip the target setting you get the target's default value?

Dec 20 2018, 1:55 PM · Restricted Project

Dec 19 2018

jingham accepted D44072: [lldb] Retrieve currently handled Obj-C exception via __cxa_current_exception_type and add GetCurrentExceptionBacktrace SB ABI.

That's great. There are a couple places where you use self.assertEqual(..., False) where you can use self.assertFalse if you want. A very minor point...

Dec 19 2018, 5:49 PM

Dec 17 2018

jingham committed rLLDB349435: Call DeleteCurrentProcess before we replace the old process..
Call DeleteCurrentProcess before we replace the old process.
Dec 17 2018, 5:52 PM
jingham committed rL349435: Call DeleteCurrentProcess before we replace the old process..
Call DeleteCurrentProcess before we replace the old process.
Dec 17 2018, 5:52 PM
jingham closed D55631: Delay replacing the process till after we've finalized it.
Dec 17 2018, 5:52 PM

Dec 14 2018

jingham committed rLLDB349211: Fix the unittests for the move of Listener & Broadcaster.
Fix the unittests for the move of Listener & Broadcaster
Dec 14 2018, 3:30 PM
jingham committed rL349211: Fix the unittests for the move of Listener & Broadcaster.
Fix the unittests for the move of Listener & Broadcaster
Dec 14 2018, 3:30 PM
jingham updated the diff for D55631: Delay replacing the process till after we've finalized it.

This is better. Instead of just finalizing, I call DeleteCurrentProcess. All the plugins that implement DebugProcess actually call DeleteCurrentProcess, so this just makes sure it happens before we drop our (potentially last) reference to the process sp.

Dec 14 2018, 3:24 PM
jingham accepted D55559: Remove the Disassembly benchmarks..

LGTM

Dec 14 2018, 12:58 PM
jingham requested changes to D55608: Make crashlog.py work or binaries with spaces in their names.

The frame regex will get confused if you had a binary called "foo 0x1" Then we would treat "foo" as the binary name, 0x1 as the pc, and the rest would be the function name. I don't see how to avoid that altogether. Maybe we could insist that the address have at least 8 numbers in it? Then your binary would have to be called "foo 0x123456789" before we get confused, at which point my caring level has dropped pretty low.

Dec 14 2018, 12:57 PM

Dec 13 2018

jingham added a comment to D55631: Delay replacing the process till after we've finalized it.

Yeah, I'll simplify the logic after this change.

Dec 13 2018, 11:02 AM

Dec 12 2018

jingham created D55631: Delay replacing the process till after we've finalized it.
Dec 12 2018, 6:41 PM
jingham added a comment to D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext..

This change looks good to me too.

Dec 12 2018, 10:52 AM

Dec 11 2018

jingham added a comment to D55574: Remove else statements after returns.

When one or the other side of the if/else test is trivial, then this rewrite is fine. When both the if and the else have a decent bit of code in them, however, removing the "} else {" and the concluding "}" obscures the fact that these are two coequal branches in the code. Before, regardless of whether I'm reading from the bottom of the function up or the top down, the structure is obvious. But when the else removed, particularly when reading from the bottom up, I can't tell this till I get to the } which happens to have a return just before it to know that. So that sort of change doesn't improve readability to my mind.

Dec 11 2018, 6:46 PM
jingham committed rL348890: Add ObjectFileBreakpad.{cpp,h} to the Xcode project..
Add ObjectFileBreakpad.{cpp,h} to the Xcode project.
Dec 11 2018, 11:28 AM
jingham committed rLLDB348890: Add ObjectFileBreakpad.{cpp,h} to the Xcode project..
Add ObjectFileBreakpad.{cpp,h} to the Xcode project.
Dec 11 2018, 11:28 AM
jingham added a comment to D55356: Add a method to get the "base" file address of an object file.

Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the load address by first getting the base address and then converting that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had this function unimplemented, and for MachO the base address was section-relative, and so it wasn't resolved to a load address without the section being loaded. However, with this patch, in the ELF (and presumably PECOFF) case, the load address is not section-relative and so the GetLoadAddress function happily returns the address.

Is this the expected behavior here? (i.e., object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value even though the target is not running)

Not unless someone has manually set the section load address in the test?

The test is not setting the load address manually. This simply falls out of how Address::GetLoadAddress is implemented:

addr_t Address::GetLoadAddress(Target *target) const {
  SectionSP section_sp(GetSection());
  if (section_sp) {
    ...
  } else {
    // We don't have a section so the offset is the load address
    return m_offset;
  }
}

So, where's the bug here? It's not clear to me how to change Address::GetLoadAddress to do something different than what it is doing now.

So this is a bug really. When there is no section, we should specify what the m_offset is using lldb_private::AddressType in maybe a new ivar name "m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And the above code would become:

addr_t Address::GetLoadAddress(Target *target) const {

SectionSP section_sp(GetSection());
if (section_sp) {
  ...
} else if (m_offset_type == eAddressTypeLoad) {
  // We don't have a section so the offset is the load address
  return m_offset;
}

}

We just need to be careful and see if we can not make lldb_private::Address get any bigger byte size wise if we can at all avoid it.
Dec 11 2018, 11:10 AM

Dec 6 2018

jingham committed rL348561: Add SBInitializerOptions.h to the Xcode project..
Add SBInitializerOptions.h to the Xcode project.
Dec 6 2018, 6:31 PM
jingham committed rLLDB348561: Add SBInitializerOptions.h to the Xcode project..
Add SBInitializerOptions.h to the Xcode project.
Dec 6 2018, 6:31 PM
jingham committed rL348559: Handle detecting exec for DynamicLoaderMacOS with older debugservers.
Handle detecting exec for DynamicLoaderMacOS with older debugservers
Dec 6 2018, 5:23 PM
jingham committed rLLDB348559: Handle detecting exec for DynamicLoaderMacOS with older debugservers.
Handle detecting exec for DynamicLoaderMacOS with older debugservers
Dec 6 2018, 5:23 PM
jingham closed D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.
Dec 6 2018, 5:23 PM
jingham added inline comments to D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.
Dec 6 2018, 5:20 PM
jingham added inline comments to D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.
Dec 6 2018, 5:11 PM
jingham added inline comments to D55318: [Expressions] Add support of expressions evaluation in some object's context.
Dec 6 2018, 4:58 PM · Restricted Project
jingham added inline comments to D55318: [Expressions] Add support of expressions evaluation in some object's context.
Dec 6 2018, 4:52 PM · Restricted Project
jingham added inline comments to D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.
Dec 6 2018, 4:40 PM
jingham updated the diff for D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.

Added a comment and removed an errant space.

Dec 6 2018, 4:39 PM
jingham created D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's.
Dec 6 2018, 4:19 PM
jingham committed rLLDB348519: Fix the Xcode project build for the addition of….
Fix the Xcode project build for the addition of…
Dec 6 2018, 11:26 AM
jingham committed rL348519: Fix the Xcode project build for the addition of….
Fix the Xcode project build for the addition of…
Dec 6 2018, 11:26 AM

Dec 5 2018

jingham added a comment to D44072: [lldb] Retrieve currently handled Obj-C exception via __cxa_current_exception_type and add GetCurrentExceptionBacktrace SB ABI.

This looks good. Can you add two more tests:

Dec 5 2018, 2:36 PM
jingham requested changes to D55318: [Expressions] Add support of expressions evaluation in some object's context.

This is a little bit odd, but it does make it easy to call methods on a value you got from FindVariable without having to cons up an expression. That seems worthwhile.

Dec 5 2018, 12:42 PM · Restricted Project

Dec 3 2018

jingham accepted D54843: [Expr] Check the language before ignoring Objective C keywords.

I think the ideal behavior would be "if we don't have debug info for the frame, choose ObjC++". But if you are in frame with debug info, obey that frame's language (except that we have to use C++ because the expression parser uses C++ references to play some of the games it plays.) The reasoning behind that is because in the first case, you won' have local names and are calling out to global objects and having the widest language available is the most convenient. If we did it that way, we could also make the user's choice of language always hold (except again you can't use "class" anywhere because of the use of C++...).

Dec 3 2018, 6:51 PM · Restricted Project

Nov 28 2018

jingham accepted D43886: [lldb] Add GetCurrentException APIs to SBThread, add frame recognizer for objc_exception_throw for Obj-C runtimes.

This isn't an external API, so we can generalize it when we get around to extending the Recognizers to C++ exceptions. This is fine for now.

Nov 28 2018, 1:24 PM

Nov 27 2018

jingham added a comment to D43886: [lldb] Add GetCurrentException APIs to SBThread, add frame recognizer for objc_exception_throw for Obj-C runtimes.

The exception breakpoints know that libobjc.B.dylib`objc_exception_throw is where you stop for exception breakpoints, and now the ObjCExceptionRecognizer knows the same thing. It always makes me nervous when two different places have the same hard-coded string. Can we add an API to ObjCLanguageRuntime to get the FileSpec for the module and a function name for the exception function, and then have both CreateExceptionResolver and RegisterObjCExceptionRecognizer use that? Actually it would be better to just add this to the LanguageRuntime, because then we could use it for the C++ CreateExceptionResolver and later (when one of us gets to it) for RegisterCPlusPlusExceptionRecognizer as well. But since C++ can throw in a couple of places, it will need to return a vector of name+library pairs.

Nov 27 2018, 6:11 PM

Nov 26 2018

jingham added a comment to D54692: [Driver] Use libOption with tablegen..

You have to gather all the -O's and -S's and -Q's and add them to the list of code that gets sourced before the file is loaded in the order in which you find them. There can be more than one of each of these and they can be interspersed anywhere among the other command options. Ditto for the -o's, -s's and -q's (except they get run after the file). I don't know how complicated a set of tests I wrote when I originally added these, so you might want to check that all these cases are covered.

Nov 26 2018, 6:38 PM · Restricted Project
jingham added a comment to D54692: [Driver] Use libOption with tablegen..

I didn't read the patch in detail yet, these are just meta-comments:

Nov 26 2018, 12:15 PM · Restricted Project

Nov 14 2018

jingham accepted D54221: Add setting to require hardware breakpoints..

Looks good to me.

Nov 14 2018, 2:49 PM · Restricted Project
jingham requested changes to D54221: Add setting to require hardware breakpoints..

The lldb API's parameters are ordered input first than output. Pretty much all the API's that take a Status as a parameter take it as the last parameter. So it looks weird to have the Status &error first in the QueueThreadPlan... API's. This pattern gets annoying when you have default parameters, so it's okay to put the out parameters before the default parameters (though default parameters are also not so great, so removing them is also okay...)

Nov 14 2018, 12:31 PM · Restricted Project
jingham added a comment to D54444: [CMake] Use extended llvm_codesign to pass entitlements for lldb-server.

We do use lldb-server, though only in the platform mode, for remote testsuite running. The gdbserver mode doesn't do anything for a macOS hosted lldb-server, so lldb-server should not require any entitlements. But we do use it.

Nov 14 2018, 10:39 AM

Nov 13 2018

jingham added a comment to D54460: Don't keep a global ABI plugin per architecture.

As for testing, Jason was careful to use a WP, so nothing crashes. The process's SP doesn't resolve and then you get the default setting of any process specific setting that uses the ABI. Since there aren't any of those yet - this currently has no observable consequences. But there will be soon, and we can use that to test that we are getting this process's settings easily once that gets checked in.

Nov 13 2018, 11:47 AM
jingham added a comment to D54460: Don't keep a global ABI plugin per architecture.

Okay, I think I'll keep what Jason did and just make them per-process. As Greg said, they are very cheap to make, since they just take a Process SP and get the weak pointer from it. None of them have any other state.

Nov 13 2018, 11:43 AM
jingham committed rLLDB346775: Since ABI's now hold a process WP, they should be handed.
Since ABI's now hold a process WP, they should be handed
Nov 13 2018, 10:22 AM
jingham committed rL346775: Since ABI's now hold a process WP, they should be handed.
Since ABI's now hold a process WP, they should be handed
Nov 13 2018, 10:22 AM
jingham closed D54460: Don't keep a global ABI plugin per architecture.
Nov 13 2018, 10:22 AM
jingham closed D54460: Don't keep a global ABI plugin per architecture.
Nov 13 2018, 10:22 AM

Nov 12 2018

jingham created D54460: Don't keep a global ABI plugin per architecture.
Nov 12 2018, 5:57 PM
jingham added a comment to D54454: Be more permissive in what we consider a variable name..

A dotest test can be xfailed if the debug format is not PDB, right? At least we can xfail them for all the DWARF variants so it should be possible to do that for PDB as well. So you should be able to write a test for this and then just xfail it till the DWARF parser can be fixed.

Nov 12 2018, 5:25 PM
jingham added a comment to D54454: Be more permissive in what we consider a variable name..

We do also handle [], though it isn't obvious to me after a quick glance where that gets done. This is a little cheesy because the name of the child that we are finding with [0] is actually "[0]", so you just have to be careful not to consume that when you consume the variable name.

Nov 12 2018, 4:50 PM
jingham added a comment to D54454: Be more permissive in what we consider a variable name..

Those seem legit things to try to capture, though a little esoteric. Since "frame variable" and "target variable" didn't support these constructs before you should certainly add some tests for that.

Nov 12 2018, 4:34 PM
jingham added a comment to D54454: Be more permissive in what we consider a variable name..

You were probably speaking loosely, but to be clear, the code you are changing doesn't get used for expressions - i.e. the expression command - unless I'm missing something.

Nov 12 2018, 4:11 PM

Nov 7 2018

jingham accepted D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

Looks good to me.

Nov 7 2018, 5:06 PM
jingham requested changes to D54221: Add setting to require hardware breakpoints..

This is pretty good, but in all the places where some plan tries to find a sub-plan to do its job, you are losing the text of the error when that job fails. So the controlling plan can't present a good error message. You need to hold onto the Status object and return that, either as the error from ValidatePlan if it happens when the plan is getting created, or in the plan's stop description so that StopInfoThreadPlan::GetDescription can print it properly.

Nov 7 2018, 4:44 PM · Restricted Project
jingham requested changes to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

Looking at the extant API's we do seem to prefer "const CompilerType &" over "const CompilerType" so it seems more consistent to choose that. Other than that, this looks fine to me.

Nov 7 2018, 3:17 PM

Nov 5 2018

jingham committed rLLDB346162: Fix the Xcode project for the removal of the Go, Java & OCaml.
Fix the Xcode project for the removal of the Go, Java & OCaml
Nov 5 2018, 12:17 PM
jingham committed rL346162: Fix the Xcode project for the removal of the Go, Java & OCaml.
Fix the Xcode project for the removal of the Go, Java & OCaml
Nov 5 2018, 12:17 PM
jingham updated subscribers of D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit").

Thanks for doing this! Why do we care about the file name of the test, shouldn't we be using the test class name for everything (that one I did remember to change...)

Nov 5 2018, 10:21 AM

Nov 2 2018

jingham committed rLLDB346053: Add an SBExpressionOptions setting mirroring the "exec" command's --allow-jit..
Add an SBExpressionOptions setting mirroring the "exec" command's --allow-jit.
Nov 2 2018, 4:45 PM
jingham committed rL346053: Add an SBExpressionOptions setting mirroring the "exec" command's --allow-jit..
Add an SBExpressionOptions setting mirroring the "exec" command's --allow-jit.
Nov 2 2018, 4:45 PM
jingham closed D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit").
Nov 2 2018, 4:45 PM
jingham added a comment to D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit").

--jit wouldn't describe what the flag actually does. Currently allow-jit and the SB Setting I added for it set the execution policy to eExecutionPolicyWhenNeeded, not eExecutionPolicyAlways. So this really does just allow the JIT to be used, it doesn't force it.

Nov 2 2018, 4:41 PM
jingham created D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit").
Nov 2 2018, 4:06 PM

Nov 1 2018

jingham added a comment to D53989: Fix formatting of wchar, char16, and char32.

Note also, the vast majority of the uses of LLDB_DISABLE_PYTHON are related to the requirement that we have Python to use any of the data formatter facilities. Those uses shouldn't be necessary. All the scripted interpreters should go through the generic ScriptInterpreter interface. There's a ScriptInterpreterNone that should stand in for the Python interpreter in every use except directly managing the Python interpreter. So there's no structural reason why we should need LLDB_DISABLE_PYTHON for anything but the initializers. We should just be able to not build the *Python.cpp files and use the define only to not initialize the Python script interpreter. Something got balled up in how the data formatters were implemented that this isn't true, IMHO.

Nov 1 2018, 3:17 PM
jingham added a comment to D53989: Fix formatting of wchar, char16, and char32.

It doesn't seem unreasonable to want to build lldb for smaller systems that don't have Python available, and in fact we do that internally at Apple. Actually, it DOES seem a little unreasonable to me because after all you can just run the debugserver/lldb-server and connect remotely. But I have not to date been able to convince the folks who have to work on said systems that they don't really want an on device lldb. Until I can win that argument - which I project happening only just shy of never - I'd rather not lose the ability to build lldb without Python.

Nov 1 2018, 2:58 PM
jingham added a comment to D53989: Fix formatting of wchar, char16, and char32.

So the deal is that we were relying on a summary formatter to print wchar_t before, and now you have a format option that handles them as well? Do we need both? Maybe the summary also handles wchar_t * strings?

Nov 1 2018, 2:17 PM
jingham added a comment to D53989: Fix formatting of wchar, char16, and char32.

There were a bunch of other tests testing wchar_t handling, all in:

Nov 1 2018, 12:17 PM

Oct 31 2018

jingham accepted D50254: [RFC] Add GDB remote packet reproducer..

LGTM

Oct 31 2018, 3:48 PM

Oct 30 2018

jingham accepted D44603: [lldb] Introduce StackFrameRecognizer.

LGTM. Thanks for working on this!

Oct 30 2018, 11:32 AM

Oct 29 2018

jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

Yes. You can't put them in the same CompileUnit because declaring variables of the type would be ambiguous (though all other references would be okay since ObjC method dispatch looks totally different from C++ method calling). But there's nothing keeping the same module from having a C++ and an ObjC class of the same name.

Oct 29 2018, 6:00 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

Yes, that usage is exactly the sort of use I was talking about. When we get an ObjC object and want to find its dynamic type, we read the type name by following the object's ISA pointer to the Class object. Then we go to look up the type from that name. We know we want an exact match to the name we found, but there's nothing that says the same module can't have an C++ class with the same name as an ObjC class. So that code needs to get all the types found, and sort through them for the one that is an ObjC interface type, and discard all the others. You will need to support that behavior somehow.

Oct 29 2018, 5:30 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

You could also get multiple matches if you had classes in anonymous namespaces with the same name in multiple compile units.

Oct 29 2018, 5:07 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

That depends on the definition of "fully qualified name". If you can ensure that it means "name of C++ class" - or other ODR enforcing type system, then you could make that assumption. In C you are free to redefine types on a per-function basis if you so desire; and sadly some interface generators (including MIG the one for generating Mach message handlers) do just this...) So you would need to see a way to restrict the inputs to this function. That doesn't seem like it would be straightforward to me.

Oct 29 2018, 4:59 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

Exact matches aren't a C++ specific thing. An exact match is useful for mixed C/C++ since you might want to say Foo and not Bar::Foo. IIRC a couple of the places where exact was dialed up explicitly in FindTypes were for C types. But since C and ObjC allow multiple types with the same name, that's one way you might see multiple matches for "exact match". Moreover C+ used to be fuzzy about whether non-exported classes from different linkage uses had to follow the ODR. I haven't followed whether this got nailed down but it still seems Quixotic to expect you could enforce this, since you are asking two totally unrelated library vendors to avoid each other's names for private classes... So having several classes with the same name is still a possibility IRL with C++.

Oct 29 2018, 4:42 PM
jingham accepted D53761: [Target] Do not skip a stop on a breakpoint if a plan was completed.

LGTM

Oct 29 2018, 12:15 PM · Restricted Project

Oct 26 2018

jingham requested changes to D53761: [Target] Do not skip a stop on a breakpoint if a plan was completed.

This seems safe, but you certainly want to add a comment explaining why you are doing this.

Oct 26 2018, 3:08 PM · Restricted Project

Oct 25 2018

jingham added a comment to D53731: [NativePDB] Add the ability to display global variables.

The first of the commands you showed presents info we definitely should add to type lookup. I actually have a bug around to do that, but it hasn't risen to the top of my queue because it's trivial to do with the SB API's so every time I've needed that info I get it from script. Selfish me...

Oct 25 2018, 5:41 PM
jingham added a comment to D53731: [NativePDB] Add the ability to display global variables.

dotest tests don't require a process. Presumably dotest knows how to build windows targeted PDB debug flavor files (to go along with dwarf/dsym/etc.). So it would be straightforward to make a test that had your input sources, built and made a target out of it and then poked at static variables and their types. That would straightaway run with all the different symbol file formats we support.

Oct 25 2018, 5:06 PM
jingham added a comment to D53731: [NativePDB] Add the ability to display global variables.

Could you also use Vedant's new FileCheck dotest test class? That should allow you to write the tests exactly as you are, but use the dotest mechanism to build and run the example.

Oct 25 2018, 3:32 PM
jingham added a comment to D53731: [NativePDB] Add the ability to display global variables.

Well, what's really going on is that I'm not familiar enough with lit to know that it doesn't have the ability to run different commands to produce the input file... But as you guessed, my point is that you have written a bunch of tests that would be valuable to test against any symfile format, so it is a shame to only run them against one format. OTOH, if that's not possible right now, I'm content to wait for some enhancements to lit that make it possible.

Oct 25 2018, 3:05 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

It seemed to me like Greg thought you were changing the behavior of lookups, which this patch doesn't do, it just makes it more efficient. I don't know if that alters his objections or not.

Oct 25 2018, 3:01 PM
jingham added a comment to D53731: [NativePDB] Add the ability to display global variables.

Is there anything PDB specific about the test you've added? If not, it might be good to use this as a generic SymbolFile test.

Oct 25 2018, 2:12 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

So the current approach relies on the ability to sniff the name to determine the context in which the user intends to find it. It does (and always did) use the presence of an initial "::" to tell whether a symbol is exact. That's obviously also inappropriate for a generic Type method. But OTOH, there needs to be a funnel point where you take in a string you know nothing about (from the user either in type lookup or in an expression) and find it as best you can. I don't think we want to force command line users to say "type lookup --exact " so we've got to figure it out internally.

Oct 25 2018, 11:43 AM
jingham added a comment to D53597: Don't type-erase the SymbolContextItem enum.

Ah, right... Too many patches (a good problem!)

Oct 25 2018, 11:33 AM
jingham updated subscribers of D53597: Don't type-erase the SymbolContextItem enum.

So far as I can tell, this patch will make lookup of exact types faster for PDB, but because of the way DWARF debug_names tables are constructed, I don't think there's any way we can do the same thing for DWARF.

Oct 25 2018, 11:02 AM
jingham accepted D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

This looks good to me.

Oct 25 2018, 10:56 AM

Oct 24 2018

jingham accepted D53677: Fix a bug PlatformDarwin::SDKSupportsModule.

Looks good to me.

Oct 24 2018, 5:16 PM
jingham added a comment to D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups.

I worry that your patch changes the behavior when you add the type_class explicitly in the lookup (i.e. look up "struct Struct" not "Struct". That should work...

Oct 24 2018, 5:00 PM
jingham accepted D53656: Adding formatters for libc++ std::u16string and std::u32string.

This looks reasonable to me.

Oct 24 2018, 3:33 PM
jingham accepted D52772: [Settings] Make "settings set" without a value equivalent to "settings clear".

That looks fine.

Oct 24 2018, 2:47 PM