Page MenuHomePhabricator

jingham (Jim Ingham)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Wed, May 15

jingham added a comment to D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough.

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

+1

Yeah this seems reasonable.

On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift. But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded. A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process. So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones. Not that we didn't need one or the other iterator.

Okay, that makes sense to me. Thanks for clearing that up. If I understand correctly (which I think I do), creating an iterator abstraction to iterate over the process' currently loaded runtimes instead of over every available runtime is the better option here. I'll follow up with that change later today or tomorrow unless somebody believes this is the wrong idea.

Wed, May 15, 11:17 AM
jingham added a comment to D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough.

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

It's fine to do that in a separate patch. If you do that it would be good to back port it to the other iterations over the LanguageRuntimes.

Excellent, sounds good. I definitely intend to backport it to any other iterations over LanguageRuntimes that I find. As for iterating over all available LanguageRuntimes, this patch and my last few have sufficient motivation for doing so, no? Maybe I'm misunderstanding what you mean.

Wed, May 15, 10:29 AM
jingham added a comment to D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough.

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

Wed, May 15, 10:20 AM

Tue, May 14

jingham added a comment to D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough.

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

Tue, May 14, 6:22 PM
jingham committed rG7d7b788fb186: Make SBDebugger.RunCommandInterpreter callable from Python. (authored by jingham).
Make SBDebugger.RunCommandInterpreter callable from Python.
Tue, May 14, 5:08 PM
jingham committed rL360730: Make SBDebugger.RunCommandInterpreter callable from Python..
Make SBDebugger.RunCommandInterpreter callable from Python.
Tue, May 14, 5:08 PM
jingham committed rLLDB360730: Make SBDebugger.RunCommandInterpreter callable from Python..
Make SBDebugger.RunCommandInterpreter callable from Python.
Tue, May 14, 5:08 PM
jingham closed D61602: Handle function parameters of RunCommandInterpreter (script bridge).
Tue, May 14, 5:08 PM · Restricted Project
jingham added a comment to D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough.

There is a TypeSystemEnumerateSupportedLanguages that we use so that we don't have to enumerate over all the language in the languages enums. After all the plugin manager knows which languages we have type systems for. If you're going to be doing a lot of this generalization, can you add a similar enumeration for the LanguageRuntimes? There are 40 or so languages in the language enum but we only have a couple of LanguageRuntimes...

Tue, May 14, 4:48 PM

Mon, May 13

jingham accepted D61602: Handle function parameters of RunCommandInterpreter (script bridge).

Excellent, thanks! Can you check this in or do you need me to?

Mon, May 13, 10:00 AM · Restricted Project

Fri, May 10

jingham added a comment to D61737: [lldb] add -ex CLI option as alias to --one-line.

If you have simple gdb startup commands then translating them into the lldb way should be straightforward, and the burden of learning how lldb works from the command line for these purposes is just not that great and if you are going to use lldb you should just buck up and browse "lldb --help" a bit... If you have complex gdb command lines, you are going to have to rework them anyway to deal with other differences between lldb and gdb. I don't think mixing the two would add enough value to be worth the uglification.

I use GDB to debug LLDB like: gdb -q -ex 'set break pend on' -ex 'set build-id-verbose 0' -ex 'set env ASAN_OPTIONS=alloc_dealloc_mismatch=0:detect_leaks=0' -ex 'set env LD_PRELOAD=/lib64/libasan.so.5' -ex "set env PYTHONPATH=$PWD/lib64/python2.7/site-packages:$PWD/lib64/python2.7/site-packages/lldb" -ex "set env LD_LIBRARY_PATH=$PWD/lib:$PWD/lib64/python2.7/site-packages/lldb" -iex 'set breakpoint pending on' -iex 'b __sanitizer::internal__exit' -ex 'b lldb_private::Module::ReportError' -ex 'b lldb_private::Host::SystemLog' -ex r -ex 'set scheduler-locking on' --args python ../llvm-git/tools/lldb/test/dotest.py --executable $PWD/bin/lldb -C $PWD/bin/clang --log-success -t ../llvm-git/tools/lldb/packages/Python/lldbsuite/test/ -f HelloWatchpointTestCase.test_hello_watchpoint_using_watchpoint_set_dwarf

Fri, May 10, 1:55 PM · Restricted Project
jingham accepted D61776: [Target] Generalize some behavior in Thread.

With appropriate comments, this seems fine.

Fri, May 10, 12:03 PM · Restricted Project
jingham added a comment to D61737: [lldb] add -ex CLI option as alias to --one-line.

I really don't want to pollute the lldb driver options with gdb equivalents (or really any duplicate spellings of already present functionality). For the lldb command line, I want us to have the freedom to do the best job of making the lldb options consistent and easy to document, understand and use, and adding in random duplicate options from gdb will only make this harder.

Fri, May 10, 11:17 AM · Restricted Project
jingham added a comment to D61776: [Target] Generalize some behavior in Thread.

If you really are going to support many languages you need to figure out how to tell folks what really happened with more specificity. For instance, you can use C++ to throw anything, so you could throw an ObjC object from a C++ exception throw. So you need to distinguish between the language of the exception thrown (which is captured by the ValueObject you return) and the language runtime throwing the language. So we need a way to query that. Also, there's no formula reason why you couldn't have two languages throwing an exception at the same time (for instance if a C++ exception is unwinding the stack and the destructor of some ObjC object that is getting destroyed throws an NSException, etc... So there needs to be some way to handle that.

Fri, May 10, 10:15 AM · Restricted Project

Thu, May 9

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

The one outstanding bit of work here is that this change requires that the MSInst "IsCall" function has to mean "will return to the next instruction after call" or we might lose control of the program. It seems obvious that that SHOULD be what it means, but we need to make sure that is always going to be what it means or we risk losing control of the program. Greg is going to follow up on that.

Thu, May 9, 6:03 PM · Restricted Project
jingham requested changes to D61737: [lldb] add -ex CLI option as alias to --one-line.

I would rather not clutter up the lldb command driver's options with gdb command flags. That seems like it will make lldb harder to figure out and reduce our freedom to choose reasonable short names for lldb driver options.

Thu, May 9, 1:29 PM · Restricted Project
jingham accepted D61746: [Breakpoint] Make breakpoint language agnostic.

That looks fine, thanks for untangling this.

Thu, May 9, 1:23 PM · Restricted Project

Wed, May 8

jingham committed rG2dda1269abbd: Fix the output file dependency for Options.inc. (authored by jingham).
Fix the output file dependency for Options.inc.
Wed, May 8, 6:43 PM
jingham committed rLLDB360304: Fix the output file dependency for Options.inc..
Fix the output file dependency for Options.inc.
Wed, May 8, 6:43 PM
jingham committed rL360304: Fix the output file dependency for Options.inc..
Fix the output file dependency for Options.inc.
Wed, May 8, 6:43 PM

Tue, May 7

jingham added inline comments to D61579: Propagate command interpreter errors from lldlbinit.
Tue, May 7, 3:41 PM · Restricted Project
jingham added a comment to D61579: Propagate command interpreter errors from lldlbinit.

I wonder if it would make this a little more convenient if we added a SetNoisy as the opposite of SetSilent?

Tue, May 7, 2:36 PM · Restricted Project
jingham added a comment to D61578: [Driver] Add command line option to allow loading local lldbinit file.

The dotest.py tests all disable reading the global .lldbinit file. Do the lit tests not do that as well? For tests that actually test reading the user's .lldbinit file we will need to do something fancy (can we change the HOME environment variable to some other place so that it gets read from there?)

Yes, they do. However, this test specifically disables that behavior because otherwise we wouldn't even read the CWD lldbinit. Changing the HOME variable might be enough to address this though (and I see Jonas already did that).

Tue, May 7, 11:20 AM · Restricted Project, Restricted Project
jingham added a comment to D61578: [Driver] Add command line option to allow loading local lldbinit file.

Just wanted to verify that we can put a:

settings set target.load-cwd-lldbinit true

in our ~/.lldbinit file and it will then load the local init file in the current working directory?

Yep, that behavior remains unchanged.

Unfortunately, I guess that also means that if somebody does that (set this setting in the global config file), this test will fail for him. I think that means that one day we will have to add a flag to specifically disable reading of the global config file, or find some way to mock its contents.

Tue, May 7, 10:33 AM · Restricted Project, Restricted Project

Mon, May 6

jingham added a comment to D61483: [www] list command: lldb run <args>.

The www was the original version, which Jonas replaced with with the rst version. We were waiting to get rid of the www version till we were sure we could iron out all the wrinkles of generating the docs on lldb.llvm.org from the rst version. That's close, though we still don't have the API docs working yet.

Mon, May 6, 12:07 PM · Restricted Project
jingham added a comment to D61602: Handle function parameters of RunCommandInterpreter (script bridge).

LGTM, thanks for doing this and for the test! Can I ask you to write a doc string showing how to call this and what it returns in Python. It's not entirely obvious. There are %feature("docstring" examples around in that file that you can copy from.

Mon, May 6, 12:00 PM · Restricted Project

Fri, May 3

jingham accepted D61292: Include inlined functions when figuring out a contiguous address range.

LGTM, you should probably wait on Pavel's okay to the testing framework changes since that's more his baby...

Fri, May 3, 5:54 PM · Restricted Project, Restricted Project
jingham accepted D61469: [Alias] Add 're' alias for register.

LGTM

Fri, May 3, 1:32 PM · Restricted Project
jingham added a comment to D61438: [ASTImporter] Use llvm::Expected and Error in the importer API.

IIUC, when Expected returns fails, it returns an Error object that might have information about what went wrong. Would it be possible to include the contents of that error n the log message? We often get "I can't run an expression in a really complex proprietary code base" and need to try to sort out what went wrong from these logs. So every crumb of information we can preserve is useful...

Fri, May 3, 10:23 AM · Restricted Project, Restricted Project, Restricted Project

Thu, May 2

jingham accepted D61451: Hide runtime support values such as clang's __vla_expr from frame variable.

LGTM

Thu, May 2, 2:50 PM · Restricted Project, Restricted Project
jingham added inline comments to D61292: Include inlined functions when figuring out a contiguous address range.
Thu, May 2, 2:06 PM · Restricted Project, Restricted Project
jingham requested changes to D61292: Include inlined functions when figuring out a contiguous address range.

This makes sense to me. Pinning the extension to the call site prevented all the cases I could think of where this might go wrong.

Thu, May 2, 1:01 PM · Restricted Project, Restricted Project
jingham added a comment to D61451: Hide runtime support values such as clang's __vla_expr from frame variable.

Could you add a comment (probably best in LanguageRuntime.h) saying what a RuntimeSupportValue is?

Thu, May 2, 10:23 AM · Restricted Project, Restricted Project

Wed, May 1

jingham committed rGe91ad7d290ed: Mention the thread-format & frame-format settings in help. (authored by jingham).
Mention the thread-format & frame-format settings in help.
Wed, May 1, 7:13 PM
jingham committed rL359752: Mention the thread-format & frame-format settings in help..
Mention the thread-format & frame-format settings in help.
Wed, May 1, 7:13 PM
jingham committed rLLDB359752: Mention the thread-format & frame-format settings in help..
Mention the thread-format & frame-format settings in help.
Wed, May 1, 7:13 PM

Fri, Apr 26

jingham accepted D61211: [ScriptInterpreter] Move ownership into debugger (NFC).

That looks nicer!

Fri, Apr 26, 3:05 PM · Restricted Project
jingham accepted D61172: [ScriptInterpreter] Pass the debugger instead of the command interpreter.

Oops, forgot to do the action part of this...

Fri, Apr 26, 10:58 AM · Restricted Project, Restricted Project
jingham added a comment to D61172: [ScriptInterpreter] Pass the debugger instead of the command interpreter.

Looks good to me too. The fact that you were mostly eliminating redirections from CommandInterpreter -> Debugger is good evidence this was wrongly structured initially.

Fri, Apr 26, 10:29 AM · Restricted Project, Restricted Project

Thu, Apr 25

jingham added a comment to D61090: [SBHostOS} Remove getting the python script interpreter path.

I really thought there was one at the SB layer, because in terms of design that is what makes sense. I guess we never really needed it until now, so we didn't add it. Once there's more than one hard-coded script interpreter, we will need to add some way to select & direct scripts at the various script interpreters so we will need SBScriptInterpreter at the SB layer. So maybe now is the time to add it in preparation...

Thu, Apr 25, 10:39 AM · Restricted Project
jingham added a comment to D61090: [SBHostOS} Remove getting the python script interpreter path.

I'm a bit confused. If we get to the world where we can support multiple script interpreters, I would expect to add a static method on ScriptInterpreter that enumerates the available interpreters. I would not expect that to be on SBHostOS since this is not a feature of the Host OS but of how lldb was configured. An lldb running on macOS with the Python2 plugin available behaves - w.r.t. macOS - exactly the same as one built with a Python3 plugin available, etc.

Thu, Apr 25, 10:16 AM · Restricted Project

Wed, Apr 24

jingham added a comment to D61090: [SBHostOS} Remove getting the python script interpreter path.

Yeah, I must have not been paying attention at some point in the past. OTOH, I don't know if you can change the access to the ScriptInterpreter in the SB API's, since that seems likely to break people's scripts.

Wed, Apr 24, 5:02 PM · Restricted Project
jingham added a comment to D61090: [SBHostOS} Remove getting the python script interpreter path.

It seems weird to be getting the ScriptInterpreter's ScriptModuleDirectory from the CommandInterpreter. I would have expected:

Wed, Apr 24, 4:38 PM · Restricted Project
jingham added a comment to D61090: [SBHostOS} Remove getting the python script interpreter path.

BTW, congrats on getting rid of all the LLDB_DISABLE_PYTHON uses. That was a goodly chunk of work and removed an obvious wart.

Wed, Apr 24, 3:33 PM · Restricted Project
jingham requested changes to D61090: [SBHostOS} Remove getting the python script interpreter path.

I agree with Greg, I really don't want to lose the "lldb -P" functionality. That's quite handy.

Wed, Apr 24, 3:10 PM · Restricted Project
jingham accepted D61044: Fix infinite recursion when calling C++ template functions.

This seems like an okay workaround.

Wed, Apr 24, 12:33 PM · Restricted Project

Tue, Apr 23

jingham accepted D61000: Kill modify-python-lldb.py.

Since this just changes how we fix up comments for the generated functions in lldb.py, I can't see any harm in removing it. It is clearly showing you a C-view of the function, so char -> str doesn't seem terribly helpful to me.

Tue, Apr 23, 11:04 AM · Restricted Project

Apr 19 2019

jingham committed rGd42b38144537: This test doesn't need to be run for all debug formats. (authored by jingham).
This test doesn't need to be run for all debug formats.
Apr 19 2019, 11:45 AM
jingham committed rLLDB358776: This test doesn't need to be run for all debug formats..
This test doesn't need to be run for all debug formats.
Apr 19 2019, 11:45 AM
jingham committed rL358776: This test doesn't need to be run for all debug formats..
This test doesn't need to be run for all debug formats.
Apr 19 2019, 11:45 AM
jingham accepted D60588: Adjusting libc++ std::list formatter to act better with pointers and references and adding a test to cover a previous related fix.

LGTM

Apr 19 2019, 11:00 AM · Restricted Project

Apr 17 2019

jingham accepted D60737: [lldb] Don't filter variable list when doing a lookup by mangled name in SymbolFileDWARF::FindGlobalVariables.

LGTM

Apr 17 2019, 11:00 AM · Restricted Project

Apr 16 2019

jingham added a comment to D60737: [lldb] Don't filter variable list when doing a lookup by mangled name in SymbolFileDWARF::FindGlobalVariables.

One picky comment about the test, otherwise this looks good to me. Pavel had his hands in this code most recently, however, so we should wait on his opinion.

Apr 16 2019, 11:16 AM · Restricted Project

Apr 9 2019

jingham added a comment to D60468: Lock accesses to OptionValueFileSpecList objects.

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

As Jim said, there are parts of the debugger that simply do not have access to a debugger by design. I'm honestly not sure this is the correct design in hindsight. I tried to make the global state per-debugger, but I stopped when I needed to change the Plugin registration to take a debugger (because SymbolVendor plugins would need a debugger to access search paths for example). This approach also has other challenges and breaks command completion in non-trivial ways.

Note that once a target is created, there is no cross-debugger contamination anymore except if you force things by passing '-g' to 'settings set'. But it is super weird (or rather wrong) that simple 'settings set' on one debugger have an impact on other debuggers.

Apr 9 2019, 1:08 PM · Restricted Project
jingham added a comment to D60468: Lock accesses to OptionValueFileSpecList objects.

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

Apr 9 2019, 10:13 AM · Restricted Project

Apr 8 2019

jingham committed rGb78094abcf52: Get the run locker before you ask if your thread is valid. (authored by jingham).
Get the run locker before you ask if your thread is valid.
Apr 8 2019, 6:32 PM
jingham committed rLLDB357963: Get the run locker before you ask if your thread is valid..
Get the run locker before you ask if your thread is valid.
Apr 8 2019, 6:31 PM
jingham committed rL357963: Get the run locker before you ask if your thread is valid..
Get the run locker before you ask if your thread is valid.
Apr 8 2019, 6:31 PM

Apr 5 2019

jingham accepted D60340: Unify random timeouts throughout LLDB and make them configurable..

LGTM

Apr 5 2019, 3:27 PM · Restricted Project

Mar 29 2019

jingham committed rGcdd4892f12e8: Use the multi-lockable form of std::lock for operator= (authored by jingham).
Use the multi-lockable form of std::lock for operator=
Mar 29 2019, 10:07 AM
jingham committed rLLDB357276: Use the multi-lockable form of std::lock for operator=.
Use the multi-lockable form of std::lock for operator=
Mar 29 2019, 10:06 AM
jingham committed rL357276: Use the multi-lockable form of std::lock for operator=.
Use the multi-lockable form of std::lock for operator=
Mar 29 2019, 10:06 AM
jingham closed D59957: Convert = operators that take object mutexes to the multi-lock version of std::lock.
Mar 29 2019, 10:06 AM · Restricted Project, Restricted Project

Mar 28 2019

jingham added a comment to D59911: Don't abort() in lldb_assert and document why..

My (maybe unpopolar) opinion on the subject is that "soft assertions" are a way to cleanse your conscience of guilt, but they don't work really well in practice.
When I started working on lldb, I was a fairly strong proponent of assertions everywhere. My view changed somewhat radically over the course of the past 18 months, and I would like to summarize some points here.

  1. Vendors (some) ship a release or two of debuggers every 6 months. Even if you just look at open source, llvm gets released every 6 months and distributions downstream package just the release version. Not everybody runs gentoo or exherbo or compiles his toolchain from svn. For these people the debugger has just to work. Assertions are always compiled out in release mode so this is not an issue. I strongly agree that for internal interfaces they have to be used as liberally as possible, mostly because they catch bugs in development. llvm libraries tend to use assertions galore, and developers put them in "just in case", and I think this is really not something we should do in lldb.
  2. Errors have to be handled, properly. I don't think returning a default constructed object in case something goes bad is better than throwing an error. We now have rich/proper error handling in llvm to make sure the process blows up if the error is not checked. This is a good thing.
  3. The above points are IMHO the only two needed ways of handling invariants/invalid input. Anything else in the middle is just going to cause confusion to new and existing developer. Adrian (or anybody else), do you have any real example where something wasn't either a user error or a strong invariant that couldn't be violated? I personally didn't, hence I don't understand the need for "soft" assertions here.

Here is a concrete example of the sort of thing I thought lldbassert was for. In DWARFExpression::AddressRangeForLocationListEntry if we come across a LocationList format we don't understand, we do:

default:
  // Not supported entry type
  lldbassert(false && "Not supported location list type");
  return false;

IMO, this example should be changed to

return llvm::make_error<DWARFError>("Unsupported location list type");

and let someone higher up handle this.

Mar 28 2019, 5:05 PM · Restricted Project, Restricted Project
jingham added a comment to D59911: Don't abort() in lldb_assert and document why..

My (maybe unpopolar) opinion on the subject is that "soft assertions" are a way to cleanse your conscience of guilt, but they don't work really well in practice.
When I started working on lldb, I was a fairly strong proponent of assertions everywhere. My view changed somewhat radically over the course of the past 18 months, and I would like to summarize some points here.

  1. Vendors (some) ship a release or two of debuggers every 6 months. Even if you just look at open source, llvm gets released every 6 months and distributions downstream package just the release version. Not everybody runs gentoo or exherbo or compiles his toolchain from svn. For these people the debugger has just to work. Assertions are always compiled out in release mode so this is not an issue. I strongly agree that for internal interfaces they have to be used as liberally as possible, mostly because they catch bugs in development. llvm libraries tend to use assertions galore, and developers put them in "just in case", and I think this is really not something we should do in lldb.
  2. Errors have to be handled, properly. I don't think returning a default constructed object in case something goes bad is better than throwing an error. We now have rich/proper error handling in llvm to make sure the process blows up if the error is not checked. This is a good thing.
  3. The above points are IMHO the only two needed ways of handling invariants/invalid input. Anything else in the middle is just going to cause confusion to new and existing developer. Adrian (or anybody else), do you have any real example where something wasn't either a user error or a strong invariant that couldn't be violated? I personally didn't, hence I don't understand the need for "soft" assertions here.
Mar 28 2019, 4:11 PM · Restricted Project, Restricted Project
jingham created D59957: Convert = operators that take object mutexes to the multi-lock version of std::lock.
Mar 28 2019, 1:00 PM · Restricted Project, Restricted Project
jingham committed rG43aaafc0e1fd: Fix the swig typemap for "uint32_t *versions, uint32_t num_versions". (authored by jingham).
Fix the swig typemap for "uint32_t *versions, uint32_t num_versions".
Mar 28 2019, 12:26 PM
jingham committed rL357207: Fix the swig typemap for "uint32_t *versions, uint32_t num_versions"..
Fix the swig typemap for "uint32_t *versions, uint32_t num_versions".
Mar 28 2019, 12:24 PM
jingham committed rLLDB357207: Fix the swig typemap for "uint32_t *versions, uint32_t num_versions"..
Fix the swig typemap for "uint32_t *versions, uint32_t num_versions".
Mar 28 2019, 12:24 PM
jingham closed D59913: Get Version SWIG wrapper should fill the list it makes.
Mar 28 2019, 12:24 PM · Restricted Project
jingham updated the diff for D59913: Get Version SWIG wrapper should fill the list it makes.

Use yaml2obj to create the dylib with version 0.0.0

Mar 28 2019, 10:27 AM · Restricted Project

Mar 27 2019

jingham committed rG1432b9780b3e: Copy the breakpoint site owner's collection so we can drop the collection lock… (authored by jingham).
Copy the breakpoint site owner's collection so we can drop the collection lock…
Mar 27 2019, 6:51 PM
jingham committed rLLDB357141: Copy the breakpoint site owner's collection so we can drop.
Copy the breakpoint site owner's collection so we can drop
Mar 27 2019, 6:50 PM
jingham committed rL357141: Copy the breakpoint site owner's collection so we can drop.
Copy the breakpoint site owner's collection so we can drop
Mar 27 2019, 6:50 PM
jingham created D59913: Get Version SWIG wrapper should fill the list it makes.
Mar 27 2019, 6:22 PM · Restricted Project

Mar 26 2019

jingham added a comment to D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary.

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

StringRef's aren't guaranteed to be NULL terminated. Or rather, the Data that gets returned by the data() method is just the raw pointer in the StringRef, which isn't guaranteed to be NULL terminated.

While true, I don't think this is a good reason to make the interface accept a const char*, because that means that the interface is exposing an implementation detail. Instead, the interface should accept a StringRef, and the implementation can pass foo.str().c_str(). It might be slightly less efficient (almost to the point of minutia), but this isn't really performance sensitive code, as loading the library will dominate the call to malloc / memcpy.

Mar 26 2019, 5:37 PM · Restricted Project
jingham added a comment to D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary.

It isn't safe to pass "data()" of a StringRef to dlsym.

Why?

Mar 26 2019, 5:22 PM · Restricted Project
jingham added a comment to D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary.

It isn't safe to pass "data()" of a StringRef to dlsym.

Mar 26 2019, 5:06 PM · Restricted Project

Mar 25 2019

jingham accepted D59708: [ExpressionParser] Add swift-lldb case for finding clang resource dir.

Except for a typo and a little quibble about a comment, this looks okay to me.

Mar 25 2019, 5:56 PM · Restricted Project

Mar 22 2019

jingham added a comment to D59708: [ExpressionParser] Add swift-lldb case for finding clang resource dir.

The behavior when verify is false seems a little weird to me. In that case you will just always get the $install_dir/lib/clang/$clang_version path. That's fairly different from the verify = true case. OTOH, I couldn't see anybody passing verify = false anywhere. Do we need that?

Mar 22 2019, 12:08 PM · Restricted Project

Mar 13 2019

jingham added a comment to D59314: Fix an invalid static cast in ClangExpressionParser.cpp.

Also naming quibble...

Mar 13 2019, 12:29 PM · Restricted Project
jingham added a comment to D59314: Fix an invalid static cast in ClangExpressionParser.cpp.

I think you have to protect against your dyn_cast failing.

Mar 13 2019, 11:46 AM · Restricted Project

Mar 12 2019

jingham committed rGe62366bf1e57: This test is failing on and off on the bots. Disable it for now till I can… (authored by jingham).
This test is failing on and off on the bots. Disable it for now till I can…
Mar 12 2019, 4:18 PM
jingham committed rL355992: This test is failing on and off on the bots. .
This test is failing on and off on the bots.
Mar 12 2019, 4:17 PM
jingham committed rLLDB355992: This test is failing on and off on the bots. .
This test is failing on and off on the bots.
Mar 12 2019, 4:17 PM
jingham committed rG027bf7603fc7: Check the result of creating a node from __next_ in the std::list formatter. (authored by jingham).
Check the result of creating a node from __next_ in the std::list formatter.
Mar 12 2019, 12:28 PM
jingham committed rLLDB355957: Check the result of creating a node from __next_ in the std::list formatter..
Check the result of creating a node from __next_ in the std::list formatter.
Mar 12 2019, 12:27 PM
jingham committed rL355957: Check the result of creating a node from __next_ in the std::list formatter..
Check the result of creating a node from __next_ in the std::list formatter.
Mar 12 2019, 12:27 PM
jingham committed rG2ca0ebf6b432: Re-enable this test, the underlying bug was fixed and the test now passes. (authored by jingham).
Re-enable this test, the underlying bug was fixed and the test now passes.
Mar 12 2019, 12:25 PM
jingham committed rLLDB355956: Re-enable this test, the underlying bug was fixed and the test now passes..
Re-enable this test, the underlying bug was fixed and the test now passes.
Mar 12 2019, 12:24 PM
jingham committed rL355956: Re-enable this test, the underlying bug was fixed and the test now passes..
Re-enable this test, the underlying bug was fixed and the test now passes.
Mar 12 2019, 12:24 PM
jingham committed rG85c2955f455e: Fix the project for r355939 (ASTUtils.{h,c}) (authored by jingham).
Fix the project for r355939 (ASTUtils.{h,c})
Mar 12 2019, 11:48 AM
jingham committed rL355951: Fix the project for r355939 (ASTUtils.{h,c}).
Fix the project for r355939 (ASTUtils.{h,c})
Mar 12 2019, 11:48 AM
jingham committed rLLDB355951: Fix the project for r355939 (ASTUtils.{h,c}).
Fix the project for r355939 (ASTUtils.{h,c})
Mar 12 2019, 11:48 AM

Mar 11 2019

jingham added a comment to D59158: Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands.

It's a little weird that the CommandObjectExpression is telling the REPL what its options should be, it would be more natural to go the other way. Maybe have the repl_sp provide a properly set up set of EvaluateExpressionOptions and then copy them over to this CommandObjectExpression execution?

Mar 11 2019, 2:50 PM · Restricted Project, Restricted Project

Mar 6 2019

jingham committed rG798174455f4f: Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp. (authored by jingham).
Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp.
Mar 6 2019, 2:53 PM
jingham committed rLLDB355561: Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp..
Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp.
Mar 6 2019, 2:52 PM
jingham committed rL355561: Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp..
Fix Cmake files for ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp.
Mar 6 2019, 2:52 PM
jingham committed rGea401ec7f402: Factor the clang specific parts of ExpressionSourceCode.{h,cpp} into the clang… (authored by jingham).
Factor the clang specific parts of ExpressionSourceCode.{h,cpp} into the clang…
Mar 6 2019, 2:45 PM
jingham committed rL355560: Factor the clang specific parts of ExpressionSourceCode.{h,cpp} into the clang….
Factor the clang specific parts of ExpressionSourceCode.{h,cpp} into the clang…
Mar 6 2019, 2:45 PM