Abandoned since suggested functionality is already done in https://reviews.llvm.org/D49739.

As we discussed previously, it would be nice to have a class in SB API that will manipulate with SBTarget's properties.
I started to work on this and , as I expected, faced with a problem:
TargetProperties class doesn't have a property with image search paths while Target does. It means that we should have a pointer to Target to implement things like
SBTargetSettings::AppendImageSearchPath(const char *from, const char *to, SBError &error).

Removed unnecessary m_lldbResult attribute, updated python test to successfully process new error messages in exec-step command.

Removed unnecessary m_lldbResult attribute.

Updated because of commited -exec-next and -exec-step commands.

Changes for using thread ids in a gdb way. Also added case of specified thread into test case.

Changes for using thread ids in a gdb way. Also added case of specified thread into test case.

In D47838#1124730, @labath wrote:

I don't know if there is any spec about what the gdb-mi protocol should use for thread identification, but I think the most important part is to be consistent. If you use indexes in one place and tid's in another you'll confuse the hell out of clients trying to talk to lldb-mi.

I'm a little bit confused about situation with thread id. For example:

(gdb)
thread list
~"Process 31642 stopped\n* thread #1: tid = 31642, 0x000000000041eed0 bash`main, name = 'bash', stop reason = breakpoint 1.1\n"
^done

Added some checks for invalid thread id.

Do you know another way of implementing AppendImageSearchPaths method?

Thank you.

In D47679#1120172, @stella.stamenova wrote:

In D47679#1120081, @polyakov.alex wrote:

Due to https://reviews.llvm.org/D47678 we can enable this test with a XFAIL directive for Windows.

Not true. As I said, the original review when I disabled the test has the log of what happens when you give it the correct path and the test still hangs in that scenario. If you XFAIL it, it will hang.

In D47679#1120039, @stella.stamenova wrote:

You can actually follow the pattern in the other tests and do:

%cc -o %t %p/inputs/break-insert.c -g

%t then becomes the name of the output file and you can use that in our other commands. This is better than specifying a hardcoded name.

Due to https://reviews.llvm.org/D47678 we can enable this test with a XFAIL directive for Windows.

Added the test.

You are right. I think that the reason of exiting *normally is much deeper. I'll look at this, but for now I don't have any idea about what this may be caused by, do you have one?

I would really appreciate if you guys can check this test on Windows again.

A bit of refactoring.

These changes fix the hanging of the lldb-mi lit tests. For example:
-file-exec-and-symbols a.out
-exec-run
commands in test may cause a hanging if a.out is invalid name of binary.

I have got a few ideas about what may cause such a behavior. I'll check them out as soon as possible.

In D47302#1111637, @clayborg wrote:

In D47302#1111569, @polyakov.alex wrote:

In D47302#1111497, @clayborg wrote:

no. Create a new SBTargetSettings class. SBTarget will hand one out for global and for target instance settings, Add all settings accessors to SBTargetSettings class

What is a difference between SBTargetSettings and TargetProperties classes except API level?

Not much difference except everything in SBTargetSettings will need to be public C++ API safe (no virtual functions, one member variable in the class that is a pointer std::unique_ptr or std::shared_ptr, no inheritance, no STL in args or return values, no lldb_private classes in args or return values. For this class to be API safe, we will actually need to keep a shared pointer to the target properties. So the class in the API header will be something like:

namespace lldb {
  SBTargetSettings {
    lldb::TargetPropertiesSP m_opaque_sp;
  };
}

Then m_opaque_sp will be filled in by assigning from a TargetSP (since a shared pointer to a target can be converted to a TargetPropertiesSP since Target inherits from TargetProperties) or from a call to Target::GetGlobalProperties(). And then we pass through all accessors we need.

In D47302#1111497, @clayborg wrote:

no. Create a new SBTargetSettings class. SBTarget will hand one out for global and for target instance settings, Add all settings accessors to SBTargetSettings class

In D47302#1111377, @clayborg wrote:

In D47302#1111372, @polyakov.alex wrote:

In D47302#1111351, @clayborg wrote:

In D47302#1111249, @polyakov.alex wrote:

In D47302#1111078, @clayborg wrote:

It might make sense to create a new SBTargetSettings class that has accessors. Then we can have to accessors on SBTarget:

class SBTarget {
  static SBTargetSettings GetGlobalSettings();
  SBTargetSettings GetSettings();
};

What global settings should be in your opinion? I guess that they should be stored in the SBDebugger, and a typical use case for them would be:

SBTarget target;
target.HookUpGlobalSettings;

I would code it exactly as I specified above. Ask the SBTarget for its settings. We would only expose the "target.*" settings through the SBTargetSettings class. SBDebugger could have its own settings for everything that is stored in the Debugger.cpp g_properties variable, those would be in SBDebuggerSettings if we need access to those:

class SBDebugger {
  static SBDebuggerSettings GetGlobalSettings();
  SBDebuggerSettings GetSettings();
};

This allows us to expose settings in a way that would allow us to serialize the settings and then load them again later.

class SBTargetSettings {
  // Accessors for "target...." setting
  void AppendImageSearchPath(const char *from, const char *to);
  size_t GetNumImageSearchPaths();
  const char *GetImageSearchPathAtIndex(size_t i);
  // Save and load all settings
  void Load(SBStream &s);
  void Save(SBStream &s);
};

Serialization sounds good, but, to accurately understand you, do you mean "classic" serialization with saving data into the file or serialization just for the time when debugger is run?

SBStream can be a string buffer or a file. We should probably save to JSON and load from JSON. But that doesn't need to be done in this patch.

So, for now we'll just save settings to a string buffer?

Don't worry about saving/loading in this patch. That was just a proposed future direction we can easily do once we have the SBTargetOptions class split out into its own class. So concentrate on making a SBTargetOptions and optionally the SBDebuggerOptions class if you need access to debugger settings as well.

In D47302#1111351, @clayborg wrote:

In D47302#1111249, @polyakov.alex wrote:

In D47302#1111078, @clayborg wrote:

It might make sense to create a new SBTargetSettings class that has accessors. Then we can have to accessors on SBTarget:

class SBTarget {
  static SBTargetSettings GetGlobalSettings();
  SBTargetSettings GetSettings();
};

What global settings should be in your opinion? I guess that they should be stored in the SBDebugger, and a typical use case for them would be:

SBTarget target;
target.HookUpGlobalSettings;

I would code it exactly as I specified above. Ask the SBTarget for its settings. We would only expose the "target.*" settings through the SBTargetSettings class. SBDebugger could have its own settings for everything that is stored in the Debugger.cpp g_properties variable, those would be in SBDebuggerSettings if we need access to those:

class SBDebugger {
  static SBDebuggerSettings GetGlobalSettings();
  SBDebuggerSettings GetSettings();
};

This allows us to expose settings in a way that would allow us to serialize the settings and then load them again later.

class SBTargetSettings {
  // Accessors for "target...." setting
  void AppendImageSearchPath(const char *from, const char *to);
  size_t GetNumImageSearchPaths();
  const char *GetImageSearchPathAtIndex(size_t i);
  // Save and load all settings
  void Load(SBStream &s);
  void Save(SBStream &s);
};

Serialization sounds good, but, to accurately understand you, do you mean "classic" serialization with saving data into the file or serialization just for the time when debugger is run?

SBStream can be a string buffer or a file. We should probably save to JSON and load from JSON. But that doesn't need to be done in this patch.

In D47302#1111078, @clayborg wrote:

It might make sense to create a new SBTargetSettings class that has accessors. Then we can have to accessors on SBTarget:

class SBTarget {
  static SBTargetSettings GetGlobalSettings();
  SBTargetSettings GetSettings();
};

Removed underscore in the lldb-mi variable from lit.cfg.

In D47302#1110441, @aprantl wrote:

The missing context here is that the lldb-mi -target-select command currently calls HandleCommand("target modules search-paths add", ...).
Is adding a new SBAPI the right approach to implementing this functionality without going through HandleCommand? Or is HandleCommand appropriate in this case?

Moved the option parsing code down by the others.

This version of commit uses the new lldb-mi option that I added to review https://reviews.llvm.org/D47110.

This option is needed by https://reviews.llvm.org/D46588.

In D46588#1103237, @aprantl wrote:

For the experiment you can probably just stick it into CMICmnLLDBDebugger::InitSBDebugger().

In D46588#1103180, @aprantl wrote:

My overall mental image of lldb-mi is very incomplete, but I'm imagining lldb-mi as having one thread that wakes up every n milliseconds checks for command input and then calls into the SBAPI to handle the commands. If that is how it works, then one very simple thing we can try is to call SBDebugger::SetAsync(false), thus forcing all the SBAPI calls to block until completion.

Alexander, can you see what happens to your lit test when you enable synchronous mode in the SBAPI? Does it block now as expected, or does it still finish early? If that experiment works, we could add a --async command line option to lldb-mi to enable this behavior for running the test suite.

I have all the patches I was sending. I will follow this topic until you decide what to do with lldb-mi testing.

Also, I combined FileCheck commands and lldb-mi input in a single file.

Created separate directory for an lldb-mi tests.

Added more comments about the test.

Added comments describing which lldb-mi command's output is currently parsing.

Moved tests to lit.

So, what do you think about the patch? There is no any decision about lldb-mi tests and I do not know what to do with this.

Added testcase and updated revision.

Thanks for your time.

Added . to the end of sentences in comments.

As we have current executable file in test, it would be better to use it in search pattern.