In D152012#4391336, @bulbazord wrote:

The key here is that you're expanding the path and checking for the validity before calling CrashLogParser.create() right? It looks like for the load_crashlog_in_scripted_process case nothing has changed. If my understanding is correct, then this looks good to me.

Address @bulbazord comments:

• Make Target::SetLabel return an llvm::Error to propagate error messages to both the CommandObject and the SBAPI callers.

Address @bulbazord last comments

LGTM!

Address @bulbazord & @jingham's comments:

• Rename name -> label
• Add test
• Add sanity check to avoid having integer only labels
• Add target index when label already exist for another target

Reload the previous version of this diff

Address @bulbazord & @jingham's comments:

• Rename name -> label
• Add test
• Add sanity check to avoid having integer only labels
• Add target index when label already exist for another target

LGTM! Sorry I didn't look at this earlier...

I still need to add some tests.

TBH, it feels like a lot of - risky - changes just so IOHandlerGetControlSequence can return a llvm::StringRef ? Is that really necessary ?

LGTM! Can we add a test for this ?

LGTM! Are you considering doing it for the rest of the codebase as well ? That would be very nice :)

LGTM!

LGTM!

In D151497#4374080, @bulbazord wrote:

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function. For example:

alex@alangford build % ./bin/lldb ~/tmp/foo
(lldb) target create "/Users/alex/tmp/foo"
Current executable set to '/Users/alex/tmp/foo' (arm64).
(lldb) b main
Breakpoint 1: where = foo`main + 28 at foo.c:6:7, address = 0x0000000100003f68
(lldb) r
Process 44467 launched: '/Users/alex/tmp/foo' (arm64)
Process 44467 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100003f68 foo`main at foo.c:6:7
   1    #include <stdio.h>
   2
   3    int g_foo = 5;
   4
   5    int main() {
-> 6      int val = 7;
   7      printf("Hello World!: %d\n", val);
   8      return 0;
   9    }
(lldb) c
Process 44467 resuming
Hello World!: 7
Process 44467 exited with status = 0 (0x00000000)
(lldb) p val
error: Can't make a function caller while the process is running

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

In D151366#4372731, @aprantl wrote:

@aprantl Do you know if we can detect the end of the scope ? I'm not sure it's possible currently ... But even if we could do it, that wouldn't cover the following case:

When setting a variable watchpoint, you would have to store the scope the variable is in, and then ignore all hits that are (1) on the same thread but (2) where the scope is not any of the lexical scopes in any of the stack frames. Because you still want to find modifications in child frames or on other threads.

Can you add this commit hash to the .git-blame-ignore-revs file so it doesn't show up in the git history

In D151366#4370123, @aprantl wrote:

Just so I understand the limitations:
This works for

int main() {
  int val = 0;
  // Break here
  val++;
  val++;
  return 0;
}

but not for

int main() {
  {
    int val = 0;
    // Break here
    val++;
    val++;
  }
  {
    int other = 42;
    printf("assuming that other and val aliases");
  }
  return 0;
}

?

To be clear, I think this is still useful!

Address @bulbazord & @aprantl comments.

More refactoring

Address @JDevlieghere & @bulbazord comments.

Address @bulbazord comments.

In D151044#4362084, @bulbazord wrote:

The bug that you're fixing is with thread handling in MultiplexerScriptedProcess right? That looks fine to me too.

In D151043#4360968, @JDevlieghere wrote:

LGTM. Are there other stop reasons that are currently not supported by scripted threads that we should consider supporting?

Many changes:

• Change SBAPI templated specialization for 2 distinct methods GetUnsignedIntegerValue & GetSignedIntegerValue
• Add new tests
• Update previous tests

Address @kastiglione comments and reformat.

Address @bulbazord comments

In D151002#4357886, @bulbazord wrote:

You probably encountered this somewhere, is there a simple test we could add here? The change looks fine to me nonetheless.

Address feedbacks:

• Simplify and Improve regex
• Add test

LGTM with nit.

Let's fix the Lua build failures first

We've already discussed that offline but I really think there should be a ScriptInterpreter SWIGBridge plugin and that would have all the method the Python and Lua SWIGBridge have in common, so the SB classes won't have to be specialized for every scripting language.

I guess the InterruptionControl argument makes it clearer. LGTM

LGTM!

I don't understand why the succeeding return value for GetFramesUpTo is false. It looks counter-intuitive to me. What's the motivation behind that ?

LGTM with some comments.

LGTM!

Very cool stuff!