Diff Detail
Event Timeline
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?
This approach is not mandatory, we could do almost the same(except errors processing) right in TargetSelect::Execute method. As you can see, the main thing here is:
target_sp->GetImageSearchPathList().Append(
ConstString(from), ConstString(to), true);Adding an SB API to set the target's search paths seems fine to me. I think in theory the totality of LLDB functionality should be available through the SB API's, (a) because it is always awkward to fall back on HandleCommand and (b) because at some point we really should make the command-line be a fairly simple client of the SB API's as that would greatly reduce our testing surface. We're only going to get there one bit at a time.
But this seems the wrong way to do it. You are setting a property on an SBTarget so this should be a method on the SBTarget. The selected target is really not reliable. For instance, having this only available for the selected target would make this impossible to use in a breakpoint command, since you could have two running targets, target A could be selected but target B could hit a breakpoint. We won't select B till we've decided that B is actually going to stop - which you don't know when you are executing breakpoint commands. So API like this should really not rely on the selected target. BTW, I see this is right below SBDebugger::SetCurrentPlatformSDKRoot, but I don't approve of that one either...
Also, if you're going to the trouble of adding a "AddSharedObjectPath, you should also add ListSharedObjectPaths, and that will make it easier to write a test for this command.
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();
};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);
};The main reason for the split up in SBTargetSettings is we have both global settings and the target specific settings. If you modify the global settings, you are modifying the target settings for all future targets. As they get created, they will copy the global settings. LLDB has global settings in a global variable, and then each new lldb_private::Target objects that are created have their own. So this new interface would mirror that setup. It also keeps all accessors from having a "bool apply_to_global" for each setting accessor.
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;
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?
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.
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.
If I understood you right, the approach is to have a separated class SBTargetSettings with own data structures consisting of the settings. Also each instance of SBTarget will have a field SBTargetSettings sb_settings;. If so, we still need a SBTarget::AddSharedObjectPath.
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.
About that, do we want to have some non-api breaking way of accessing the settings (like, SBSomething GetSetting(std::string setting_name)). This could be either an addition to the accessors for specific settings or even the only way of accessing the settings. In the former case, it would take the pressure off of adding a specific API for a setting we're not sure we want to keep. In the latter case, it would enable us to add/remove settings pretty much as we want without impacting api signature in anyway. Though, I guess it's arguable whether that would be doing anyone a favour as it would likely just replace a link error with a random runtime failure.
(I have no opinion whether we should do this or not, just throwing the idea out there.)
If so, to do:
sb_target.settings.AppendImageSearchPath(...)
we need to cast our pointer to TargetProperties to a pointer to Target since there is no such property(manipulating with image search paths) in TargetProperties and then use Target's methods. I don't like this because of casting pointer from a parent class to a child one.
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).
We may store an additional pointer to Target, but, as I understood, it's not what we want from SBTargetSettings to be.
Moreover, it looks strange that image search path isn't a part of TargetProperties while executable and debug file search paths are.
I'm looking forward for your comments about this.
@jingham: do you have any opinion about the right SBAPI for manipulating settings like Alexander outlined?
We could end up moving anything that is set by a target property into the lldb_private target property class if it isn't already there and then that would fix things.
Another approach is to implement SBTargetSettings' functionality such as serialization inside lldb_private TargetProperties class and then just add an API to the SBTarget. For example:
void SBTarget::SerializeProperties(...)
What do you think about this way?
If you remember, we started this discussion from adding the AppendImageSearchPath(const char *from, const char *to) method. To do so, we need to modify Target's member variable
m_image_search_paths. It means that to move this option to TargetProperties we have to also move this member variable from Target to TargetProperties. We need to be sure that it won't break anything.
Abandoned since suggested functionality is already done in https://reviews.llvm.org/D49739.