[LLDB][GUI] Add Create Target form

[OmarEmaraDev]

Summary

This patch adds a Create Target form for the LLDB GUI. Additionally, an
Arch Field was introduced to input an arch and the file and directory
fields now have a required property.

[OmarEmaraDev]

This doesn't support remote files yet, I am still having trouble testing those. Also, there is also an unrelated clang-format change, not sure if I should revert it or keep it.

[clayborg]

Great start! Many inlined comments.

lldb/source/Core/IOHandlerCursesGUI.cpp

1333

Should this to into the FieldDelegate so we don't need to put it into both FileFieldDelegate and DirectoryFieldDelegate? Then the default FieldDelegate::FieldDelegateExitCallback() function could be:

virtual void FieldDelegate::FieldDelegateExitCallback() {
  if (!m_required)
    return;
  // Check value with another virtual function?
  FieldDelegateValueIsValid();
}

This seems like many fields would want to require being set and we don't want to re-invent this for each kind of field.

1335

It should be a property of the FileFieldDelegate if the file is for the current system. We might be specifying a file on a remote system, so it doesn't always have to exist, nor should it be resolved on the current system. When creating a target we would always want to specify this file should exist on the current system.

1378

It should be a property of the DirectoryFieldDelegate if the directory is for the current system. We might be specifying a working directory on a remote system, so it doesn't always have to exist, nor should it be resolved on the current system. So if this is for the current system, then it is ok to resolve, else, we need to just use the path as is.

1996

Should we add a "bool resolve_local_path" so we know if we should resolve the path on the local system as mentioned above?

2005

Should we add a "bool resolve_local_path" so we know if we should resolve the path on the local system as mentioned above?

2658

2662

we should add a "Platform" TextField that is populated with all of the platform names. The default value should be the currently selected platform. To get a list of all platform plug-in names you can do this like you did for the process plug-ins:

static const char *GetPlatformPluginNameAtIndex(uint32_t idx);

To get the current selected platform:

ConstString default_platform_name;
lldb::PlatformSP platform_sp = debugger.GetPlatformList()GetSelectedPlatform()
if (platform_sp)
  default_platform_name = platform_sp->GetName();

This will help with remote targets. The main reason we need this is many linux or android binaries have very minimal target triples encoded into the ELF files, so we often need to specify "remote-android" or "remote-linux" when creating targets. The user can of course set the "Architecture" with a fully specified triple, but they can also avoid setting the arch and just specify the platform.

2665

2698

Won't this be taken care of already by the File field? We should construct the FileDelegate so that:

• the file must exist
• the file gets resolved on the local system
• the file field is required

This way, the only way for the user to get to the "Create" button if is if they already were able to set a valid and required by the FileFieldDelegate class.

2703–2707

No need to do this here, we will get an error back from the target creation below in the "status". So this can be removed. Also, our FileFieldDelegate should have already shown an error if the file doesn't exist right?

2709

Set the platform name if one was selected and it wasn't the currently selected one

2713

Maybe add a GetPlatformOptions()?

2726–2728

Remove extra braces on single line if statements (LLVM coding guidelines).

2732–2741

Remove, these should have been verified by the FileFieldDelegate already.

2751–2753

Remove extra braces on single line if statements (LLVM coding guidelines).

2757–2766

Remove as FileFieldDelegate should have taken care of this already, or the process_sp->LoadCore() will return a valid error.

2775–2777

Remove extra braces on single line if statements (LLVM coding guidelines).

[DavidSpickett]

This doesn't support remote files yet, I am still having trouble testing those.

If you want to set that up we have some docs for it:
https://lldb.llvm.org/use/remote.html
https://lldb.llvm.org/use/qemu-testing.html

This is mostly how I work on AArch64 changes. That said there are some tricky bits getting the networking setup so I'm happy to try this out for you if you want.

I'll keep an eye on this change.

[OmarEmaraDev]

I am currently breaking this patch into smaller independent viable patches as suggested.

lldb/source/Core/IOHandlerCursesGUI.cpp

1333

The fields that can have a required property is the TextField and its derivatives (File, Directory, and Number), so we can can add this logic there. This is implemented in D106458.

1335

I was trying to add properties that indicates if the file is local or not and if it should be resolved or not. But they all seems identical to the need_to_exist property. If m_need_to_exist is false, then the file will not be resolved and will not be checked with the host file system (Which is what we want to remote files). If it is true, then it will be resolved and checked with the host file system (Which is what we want for local files). So adding more properties might be redundant? What do you think?

2698

D106459 implements the necessary bits to allow for this.

[OmarEmaraDev]

Thanks @DavidSpickett! I will look into this and let you know how it goes.

[clayborg]

In D106192#2893862, @OmarEmaraDev wrote:

I am currently breaking this patch into smaller independent viable patches as suggested.

That will make reviews much easier. Are you going to land the other smaller diffs first and then update this one after hey have landed?

lldb/source/Core/IOHandlerCursesGUI.cpp

1333

Sounds good!

1335

That sounds fine. No need to add more fields that don't make sense

2698

Sounds good!

[OmarEmaraDev]

Are you going to land the other smaller diffs first and then update this one after hey have landed?

Yes. There is much dependency between patches, so the smaller ones will have to land first then we will rebase this one on main. Also, notice that the smaller patches might conflict with each other, so I will have to rebase some of them once other land.

lldb/source/Core/IOHandlerCursesGUI.cpp

2662

The required field is implemented in D106483.

[clayborg]

In D106192#2894392, @OmarEmaraDev wrote:

Are you going to land the other smaller diffs first and then update this one after hey have landed?

Yes. There is much dependency between patches, so the smaller ones will have to land first then we will rebase this one on main. Also, notice that the smaller patches might conflict with each other, so I will have to rebase some of them once other land.

Indeed! I applied two patches already, and the last one https://reviews.llvm.org/D106483 is conflicting. Ping me back when you get the conflicts resolved and we can move forward.

[OmarEmaraDev]

• Rebase on main.
• Add basic remote debugging support.

[OmarEmaraDev]

I still can't get remote debugging to work unfortunately, or maybe I don't understand it really. The way I understand it is as follows:

• If the remote file is specified, then that means we are creating a target for remote debugging.
• If the remote file doesn't exist, we upload the executable to the location specified by the remote file.
• If the remote file exists but the executable doesn't exist, then the remote file is downloaded to the local file. (Why? And does that mean the executable needn't exist if the remote file is specified? And also, does that mean the executable isn't a required field?)
• Everything happens using the platform of the created target, which is specified in the platform field. (When/Where does the connection with the server get established? platform connect connects the currently selected platform, but the target have its own platform which may not necessary be the currently selected one.)

[clayborg]

Many inline comments with simple fixes! Very close.

lldb/source/Core/IOHandlerCursesGUI.cpp

2658

Lots of "true, true" and "true, false" in the file field parameters. Add comments with parameter names:

m_executable_field = AddFileField("Executable", "", *need_to_exist=*/true, /*required=*/true);

2659–2662

Ditto for all these: add comment variable names as suggested above.

2663

inlined comment for "false" above

2690

I would vote to shorten this string with something like "Executable only" with no trailing '.' character.

2691

remove the '.'

2692

remote the '.'

2698

remove the '.'. You might want to create a constexpr string value as a static outside of the functions and use them in GetLoadDependentFilesChoices() and GetLoadDependentFiles(). The static would be:

static constexpr const char *kLoadDependentFilesNo = "No";
static constexpr const char *kLoadDependentFilesYes = "Yes";
static constexpr const char *kLoadDependentFilesExecOnly = "Only for executable files";

Then use these in the GetLoadDependentFilesChoices() and GetLoadDependentFiles() functions. That way if you change the strings, you only have to change one location.

2700

remove the '.'

2770–2785

Remove all of this (getting the platform, putting the file), no need to do anything other than module_sp->SetPlatformFileSpec(...) that you are doing below. This information will be used in Process::Launch(...) to do the right thing.

[OmarEmaraDev]

• Address review comments

[clayborg]

Looks good!

lldb/source/Core/IOHandlerCursesGUI.cpp

 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
FileSpec GetResolvedFileSpec() {
     FileSystem::Instance().Resolve(file_spec);
     return file_spec;
   }
 
   const std::string &GetPath() { return m_content; }
 
 protected:
   bool m_need_to_exist;
 };
 
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
                          bool need_to_exist, bool required)
       : TextFieldDelegate(label, content, required),
         m_need_to_exist(need_to_exist) {}
 
   void FieldDelegateExitCallback() override {
     TextFieldDelegate::FieldDelegateExitCallback();
public:
   }
 
   const std::string &GetPath() { return m_content; }
 
 protected:
   bool m_need_to_exist;
 };
 
 class ArchFieldDelegate : public TextFieldDelegate {
 public:
   ArchFieldDelegate(const char *label, const char *content, bool required)
       : TextFieldDelegate(label, content, required) {}
 
   void FieldDelegateExitCallback() override {
     TextFieldDelegate::FieldDelegateExitCallback();
     if (!IsSpecified())
       return;
TextFieldDelegate *AddTextField(const char *label, const char *content,
                                   bool required) {
     TextFieldDelegate *delegate =
         new TextFieldDelegate(label, content, required);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
                                   bool need_to_exist, bool required) {
     FileFieldDelegate *delegate =
         new FileFieldDelegate(label, content, need_to_exist, required);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
                                             const char *content,
                                             bool need_to_exist, bool required) {
     DirectoryFieldDelegate *delegate =
         new DirectoryFieldDelegate(label, content, need_to_exist, required);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
   }
 
   ArchFieldDelegate *AddArchField(const char *label, const char *content,
                                   bool required) {
protected:
   TextFieldDelegate *m_name_field;
   BooleanFieldDelegate *m_continue_field;
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
   ProcessPluginFieldDelegate *m_plugin_field;
 };
 
+class TargetCreateFormDelegate : public FormDelegate {
+public:
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+    m_executable_field = AddFileField("Executable", "", /*need_to_exist=*/true,
+                                      /*required=*/true);
+    m_core_file_field = AddFileField("Core File", "", /*need_to_exist=*/true,
+                                     /*required=*/false);
+    m_symbol_file_field = AddFileField(
+        "Symbol File", "", /*need_to_exist=*/true, /*required=*/false);
+    m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+    m_remote_file_field = AddFileField(
+        "Remote File", "", /*need_to_exist=*/false, /*required=*/false);
+    m_arch_field = AddArchField("Architecture", "", /*required=*/false);
+    m_platform_field = AddPlatformPluginField(debugger);
+    m_load_dependent_files_field =
+        AddChoicesField("Load Dependents", 3, GetLoadDependentFilesChoices());
+
+    AddAction("Create", [this](Window &window) { CreateTarget(window); });
+  }
+
+  std::string GetName() override { return "Create Target"; }
+
+  void UpdateFieldsVisibility() override {
+    if (m_show_advanced_field->GetBoolean()) {
+      m_remote_file_field->FieldDelegateShow();
+      m_arch_field->FieldDelegateShow();
+      m_platform_field->FieldDelegateShow();
+      m_load_dependent_files_field->FieldDelegateShow();
+    } else {
+      m_remote_file_field->FieldDelegateHide();
+      m_arch_field->FieldDelegateHide();
+      m_platform_field->FieldDelegateHide();
+      m_load_dependent_files_field->FieldDelegateHide();
+    }
+  }
+
+  static constexpr const char *kLoadDependentFilesNo = "No";
+  static constexpr const char *kLoadDependentFilesYes = "Yes";
+  static constexpr const char *kLoadDependentFilesExecOnly = "Executable only";
+
+  std::vector<std::string> GetLoadDependentFilesChoices() {
+    std::vector<std::string> load_depentents_options;
+    load_depentents_options.push_back(kLoadDependentFilesExecOnly);
+    load_depentents_options.push_back(kLoadDependentFilesYes);
+    load_depentents_options.push_back(kLoadDependentFilesNo);
+    return load_depentents_options;
+  }
+
+  LoadDependentFiles GetLoadDependentFiles() {
+    std::string choice = m_load_dependent_files_field->GetChoiceContent();
+    if (choice == kLoadDependentFilesNo)
+      return eLoadDependentsNo;
+    if (choice == kLoadDependentFilesYes)
+      return eLoadDependentsYes;
+    return eLoadDependentsDefault;
+  }
+
+  OptionGroupPlatform GetPlatformOptions() {
+    OptionGroupPlatform platform_options(false);
+    platform_options.SetPlatformName(m_platform_field->GetPluginName().c_str());
+    return platform_options;
+  }
+
+  TargetSP GetTarget() {
+    OptionGroupPlatform platform_options = GetPlatformOptions();
+    TargetSP target_sp;
+    Status status = m_debugger.GetTargetList().CreateTarget(
+        m_debugger, m_executable_field->GetPath(),
+        m_arch_field->GetArchString(), GetLoadDependentFiles(),
+        &platform_options, target_sp);
+
+    if (status.Fail()) {
+      SetError(status.AsCString());
+      return nullptr;
+    }
+
+    m_debugger.GetTargetList().SetSelectedTarget(target_sp);
+
+    return target_sp;
+  }
+
+  void SetSymbolFile(TargetSP target_sp) {
+    if (!m_symbol_file_field->IsSpecified())
+      return;
+
+    ModuleSP module_sp(target_sp->GetExecutableModule());
+    if (!module_sp)
+      return;
+
+    module_sp->SetSymbolFileFileSpec(
+        m_symbol_file_field->GetResolvedFileSpec());
+  }
+
+  void SetCoreFile(TargetSP target_sp) {
+    if (!m_core_file_field->IsSpecified())
+      return;
+
+    FileSpec core_file_spec = m_core_file_field->GetResolvedFileSpec();
+
+    FileSpec core_file_directory_spec;
+    core_file_directory_spec.GetDirectory() = core_file_spec.GetDirectory();
+    target_sp->AppendExecutableSearchPaths(core_file_directory_spec);
+
+    ProcessSP process_sp(target_sp->CreateProcess(
+        m_debugger.GetListener(), llvm::StringRef(), &core_file_spec, false));
+
+    if (!process_sp) {
+      SetError("Unable to find process plug-in for core file!");
+      return;
+    }
+
+    Status status = process_sp->LoadCore();
+    if (status.Fail()) {
+      SetError("Can't find plug-in for core file!");
+      return;
+    }
+  }
+
+  void SetRemoteFile(TargetSP target_sp) {
+    if (!m_remote_file_field->IsSpecified())
+      return;
+
+    ModuleSP module_sp(target_sp->GetExecutableModule());
+    if (!module_sp)
+      return;
+
+    FileSpec remote_file_spec = m_remote_file_field->GetFileSpec();
+    module_sp->SetPlatformFileSpec(remote_file_spec);
+  }
+
+  void RemoveTarget(TargetSP target_sp) {
+    m_debugger.GetTargetList().DeleteTarget(target_sp);
+  }
+
+  void CreateTarget(Window &window) {
+    ClearError();
+
+    bool all_fields_are_valid = CheckFieldsValidity();
+    if (!all_fields_are_valid)
+      return;
+
+    TargetSP target_sp = GetTarget();
+    if (HasError())
+      return;
+
+    SetSymbolFile(target_sp);
+    if (HasError()) {
+      RemoveTarget(target_sp);
+      return;
+    }
+
+    SetCoreFile(target_sp);
+    if (HasError()) {
+      RemoveTarget(target_sp);
+      return;
+    }
+
+    SetRemoteFile(target_sp);
+    if (HasError()) {
+      RemoveTarget(target_sp);
+      return;
+    }
+
+    window.GetParent()->RemoveSubWindow(&window);
+  }
+
+protected:
+  Debugger &m_debugger;
+
+  FileFieldDelegate *m_executable_field;
+  FileFieldDelegate *m_core_file_field;
+  FileFieldDelegate *m_symbol_file_field;
+  BooleanFieldDelegate *m_show_advanced_field;
+  FileFieldDelegate *m_remote_file_field;
+  ArchFieldDelegate *m_arch_field;
+  PlatformPluginFieldDelegate *m_platform_field;
+  ChoicesFieldDelegate *m_load_dependent_files_field;
+};
+
 class MenuDelegate {
 public:
   virtual ~MenuDelegate() = default;
 
   virtual MenuActionResult MenuDelegateAction(Menu &menu) = 0;
 };
 
 class Menu : public WindowDelegate {
public:
   }
 
   void Terminate() { ::endwin(); }
 
   void Run(Debugger &debugger) {
     bool done = false;
     int delay_in_tenths_of_a_second = 1;
 
-    // Alas the threading model in curses is a bit lame so we need to resort to
-    // polling every 0.5 seconds. We could poll for stdin ourselves and then
-    // pass the keys down but then we need to translate all of the escape
+    // Alas the threading model in curses is a bit lame so we need to resort
+    // to polling every 0.5 seconds. We could poll for stdin ourselves and
+    // then pass the keys down but then we need to translate all of the escape
     // sequences ourselves. So we resort to polling for input because we need
     // to receive async process events while in this loop.
 
-    halfdelay(delay_in_tenths_of_a_second); // Poll using some number of tenths
-                                            // of seconds seconds when calling
-                                            // Window::GetChar()
+    halfdelay(delay_in_tenths_of_a_second); // Poll using some number of
+                                            // tenths of seconds seconds when
+                                            // calling Window::GetChar()
 
     ListenerSP listener_sp(
         Listener::MakeListener("lldb.IOHandler.curses.Application"));
     ConstString broadcaster_class_process(Process::GetStaticBroadcasterClass());
     debugger.EnableForwardEvents(listener_sp);
 
     m_update_screen = true;
 #if defined(__APPLE__)
static curses::KeyHelp g_source_view_key_help[] = {
         {KEY_PPAGE, "Page up"},
         {KEY_NPAGE, "Page down"},
         {'\0', nullptr}};
     return g_source_view_key_help;
   }
 
   MenuActionResult MenuDelegateAction(Menu &menu) override {
     switch (menu.GetIdentifier()) {
+    case eMenuID_TargetCreate: {
+      WindowSP main_window_sp = m_app.GetMainWindow();
+      FormDelegateSP form_delegate_sp =
+          FormDelegateSP(new TargetCreateFormDelegate(m_debugger));
+      Rect bounds = main_window_sp->GetCenteredRect(80, 19);
+      WindowSP form_window_sp = main_window_sp->CreateSubWindow(
+          form_delegate_sp->GetName().c_str(), bounds, true);
+      WindowDelegateSP window_delegate_sp =
+          WindowDelegateSP(new FormWindowDelegate(form_delegate_sp));
+      form_window_sp->SetDelegate(window_delegate_sp);
+      return MenuActionResult::Handled;
+    }
     case eMenuID_ThreadStepIn: {
       ExecutionContext exe_ctx =
           m_debugger.GetCommandInterpreter().GetExecutionContext();
       if (exe_ctx.HasThreadScope()) {
         Process *process = exe_ctx.GetProcessPtr();
         if (process && process->IsAlive() &&
             StateIsStoppedState(process->GetState(), true))
           exe_ctx.GetThreadRef().StepIn(true);