[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

1309

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.

1315

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.

1346

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.

1912

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

1922

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

2569

2573

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.

2576

2609

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.

2614–2618

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?

2620

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

2624

Maybe add a GetPlatformOptions()?

2637–2639

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

2643–2652

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

2662–2664

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

2668–2677

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

2686–2688

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

1309

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?

1315

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.

2609

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

1309

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

1315

Sounds good!

2609

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

2573

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

2569

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);

2570–2573

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

2574

inlined comment for "false" above

2601

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

2602

remove the '.'

2603

remote the '.'

2609

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.

2611

remove the '.'

2681–2696

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

 //===-- IOHandlerCursesGUI.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/IOHandlerCursesGUI.h"
 #include "lldb/Host/Config.h"
 
 #if LLDB_ENABLE_CURSES
 #if CURSES_HAVE_NCURSES_CURSES_H
 #include <ncurses/curses.h>
 #include <ncurses/panel.h>
 #else
 #include <curses.h>

 #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"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/State.h"
 #endif
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #ifdef _WIN32
 #include "lldb/Host/windows/windows.h"
 #endif
 
 #include <memory>
 #include <mutex>
public:
 
   // Returns the integer content of the field.
   int GetInteger() { return std::stoi(m_content); }
 };
 
 class FileFieldDelegate : public TextFieldDelegate {
 public:
   FileFieldDelegate(const char *label, const char *content,
-                    bool need_to_exist = true)
-      : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+                    bool need_to_exist = true, bool required = true)
+      : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+        m_required(required) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
-    FileSpec file(GetPath());
+    if (m_content.empty() && !m_required)
+      return;
+
+    FileSpec file = GetFileSpec();
     if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
       SetError("File doesn't exist!");
       return;
     }
     if (FileSystem::Instance().IsDirectory(file)) {
       SetError("Not a file!");
       return;
     }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+    FileSpec file_spec(m_content);
+    FileSystem::Instance().Resolve(file_spec);
+    return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
 };
 
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
-                         bool need_to_exist = true)
-      : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+                         bool need_to_exist = true, bool required = true)
+      : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+        m_required(required) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
-    FileSpec file(GetPath());
+    if (m_content.empty() && !m_required)
+      return;
+
+    FileSpec file = GetFileSpec();
     if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
       SetError("Directory doesn't exist!");
       return;
     }
     if (!FileSystem::Instance().IsDirectory(file)) {
       SetError("Not a directory!");
       return;
     }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+    FileSpec file_spec(m_content);
+    FileSystem::Instance().Resolve(file_spec);
+    return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
+};
+
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content,
+                    bool required = false)
+      : TextFieldDelegate(label, content), m_required(required) {}
+
+  void FieldDelegateExitCallback() override {
+    if (m_content.empty() && !m_required)
+      return;
+
+    if (GetArchSpec().IsValid())
+      return;
+
+    SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+
+protected:
+  bool m_required;
 };
 
 class BooleanFieldDelegate : public FieldDelegate {
 public:
   BooleanFieldDelegate(const char *label, bool content)
       : m_label(label), m_content(content) {}
 
   // Boolean fields are drawn as checkboxes.
public:
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
     TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-                                  bool need_to_exist = true) {
+                                  bool need_to_exist = true,
+                                  bool required = true) {
     FileFieldDelegate *delegate =
-        new FileFieldDelegate(label, content, need_to_exist);
+        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 = true) {
+                                            bool need_to_exist = true,
+                                            bool required = true) {
     DirectoryFieldDelegate *delegate =
-        new DirectoryFieldDelegate(label, content, need_to_exist);
+        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 = false) {
+    ArchFieldDelegate *delegate =
+        new ArchFieldDelegate(label, content, required);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
     IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
     m_fields.push_back(FieldDelegateUP(delegate));
     return delegate;
protected:
   TextFieldDelegate *m_name_field;
   BooleanFieldDelegate *m_continue_field;
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
   ChoicesFieldDelegate *m_plugin_field;
 };
 
+class TargetCreateFormDelegate : public FormDelegate {
+public:
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+    m_file_field = AddFileField("File", "");
+    m_core_file_field = AddFileField("Core File", "", true, false);
+    m_symbol_file_field = AddFileField("Symbol File", "", true, false);
+    m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+    m_arch_field = AddArchField("Architecture", "");
+    std::vector<std::string> load_depentents_options;
+    load_depentents_options.push_back(
+        std::string("Only if the target is an executable."));
+    load_depentents_options.push_back(std::string("Yes."));
+    load_depentents_options.push_back(std::string("No."));
+    m_load_dependent_files_field =
+        AddChoicesField("Load Dependents", 3, load_depentents_options);
+
+    AddAction("Create", [this](Window &window) { CreateTarget(window); });
+  }
+
+  std::string GetName() override { return "Create Target"; }
+
+  void UpdateFieldsVisibility() override {
+    if (m_show_advanced_field->GetBoolean()) {
+      m_arch_field->FieldDelegateShow();
+      m_load_dependent_files_field->FieldDelegateShow();
+    } else {
+      m_arch_field->FieldDelegateHide();
+      m_load_dependent_files_field->FieldDelegateHide();
+    }
+  }
+
+  LoadDependentFiles GetLoadDependentFiles() {
+    std::string choice = m_load_dependent_files_field->GetChoiceContent();
+    if (choice == "No.")
+      return eLoadDependentsNo;
+    if (choice == "Yes.")
+      return eLoadDependentsYes;
+    return eLoadDependentsDefault;
+  }
+
+  TargetSP GetTarget() {
+    FileSpec file_spec = m_file_field->GetFileSpec();
+
+    if (!FileSystem::Instance().Exists(file_spec)) {
+      SetError("File doesn't exist!");
+      return nullptr;
+    }
+
+    if (!FileSystem::Instance().Open(file_spec,
+                                     lldb_private::File::eOpenOptionRead)) {
+      SetError("Can't open file!");
+      return nullptr;
+    }
+
+    OptionGroupPlatform platform_options(false);
+    TargetSP target_sp;
+    Status status = m_debugger.GetTargetList().CreateTarget(
+        m_debugger, m_file_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;
+    }
+
+    FileSpec symbol_file_spec = m_symbol_file_field->GetFileSpec();
+
+    if (!FileSystem::Instance().Exists(symbol_file_spec)) {
+      SetError("Symbol file doesn't exist!");
+      return;
+    }
+
+    if (!FileSystem::Instance().Open(symbol_file_spec,
+                                     lldb_private::File::eOpenOptionRead)) {
+      SetError("Can't open symbol file!");
+      return;
+    }
+
+    ModuleSP module_sp(target_sp->GetExecutableModule());
+    if (!module_sp)
+      return;
+
+    module_sp->SetSymbolFileFileSpec(symbol_file_spec);
+  }
+
+  void SetCoreFile(TargetSP target_sp) {
+    if (!m_core_file_field->IsSpecified()) {
+      return;
+    }
+
+    FileSpec core_file_spec = m_core_file_field->GetFileSpec();
+
+    if (!FileSystem::Instance().Exists(core_file_spec)) {
+      SetError("Core file doesn't exist!");
+      return;
+    }
+
+    if (!FileSystem::Instance().Open(core_file_spec,
+                                     lldb_private::File::eOpenOptionRead)) {
+      SetError("Can't open core file!");
+      return;
+    }
+
+    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!");
+    }
+
+    Status status = process_sp->LoadCore();
+    if (status.Fail()) {
+      SetError("Can't find plug-in for core file!");
+      return;
+    }
+  }
+
+  void RemoveTarget(TargetSP target_sp) {
+    m_debugger.GetTargetList().DeleteTarget(target_sp);
+  }
+
+  void CreateTarget(Window &window) {
+    ClearError();
+
+    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;
+    }
+
+    window.GetParent()->RemoveSubWindow(&window);
+  }
+
+protected:
+  Debugger &m_debugger;
+
+  FileFieldDelegate *m_file_field;
+  FileFieldDelegate *m_core_file_field;
+  FileFieldDelegate *m_symbol_file_field;
+  BooleanFieldDelegate *m_show_advanced_field;
+  ArchFieldDelegate *m_arch_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);