[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form
Greg Clayton via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Jul 16 15:47:38 PDT 2021
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Great start! Many inlined comments.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+ FileSpec file_spec(m_content);
+ FileSystem::Instance().Resolve(file_spec);
+ return file_spec;
----------------
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.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
bool m_need_to_exist;
+ bool m_required;
};
----------------
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.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1346
+ FileSpec file_spec(m_content);
+ FileSystem::Instance().Resolve(file_spec);
+ return file_spec;
----------------
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.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1912
FileFieldDelegate *AddFileField(const char *label, const char *content,
- bool need_to_exist = true) {
+ bool need_to_exist = true,
+ bool required = true) {
----------------
Should we add a "bool resolve_local_path" so we know if we should resolve the path on the local system as mentioned above?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1922
const char *content,
- bool need_to_exist = true) {
+ bool need_to_exist = true,
+ bool required = true) {
----------------
Should we add a "bool resolve_local_path" so we know if we should resolve the path on the local system as mentioned above?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2569
+ TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+ m_file_field = AddFileField("File", "");
+ m_core_file_field = AddFileField("Core File", "", true, false);
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2573
+ m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+ m_arch_field = AddArchField("Architecture", "");
+ std::vector<std::string> load_depentents_options;
----------------
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.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2576
+ load_depentents_options.push_back(
+ std::string("Only if the target is an executable."));
+ load_depentents_options.push_back(std::string("Yes."));
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+ if (!FileSystem::Instance().Exists(file_spec)) {
+ SetError("File doesn't exist!");
----------------
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.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2614-2618
+ if (!FileSystem::Instance().Open(file_spec,
+ lldb_private::File::eOpenOptionRead)) {
+ SetError("Can't open file!");
+ return nullptr;
+ }
----------------
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?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2620
+
+ OptionGroupPlatform platform_options(false);
+ TargetSP target_sp;
----------------
Set the platform name if one was selected and it wasn't the currently selected one
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2624
+ m_debugger, m_file_field->GetPath(), m_arch_field->GetArchString(),
+ GetLoadDependentFiles(), &platform_options, target_sp);
+
----------------
Maybe add a GetPlatformOptions()?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2637-2639
+ if (!m_symbol_file_field->IsSpecified()) {
+ return;
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2643-2652
+ 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)) {
----------------
Remove, these should have been verified by the FileFieldDelegate already.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2662-2664
+ if (!m_core_file_field->IsSpecified()) {
+ return;
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2668-2677
+ 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)) {
----------------
Remove as FileFieldDelegate should have taken care of this already, or the process_sp->LoadCore() will return a valid error.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2686-2688
+ if (!process_sp) {
+ SetError("Unable to find process plug-in for core file!");
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106192/new/
https://reviews.llvm.org/D106192
More information about the lldb-commits
mailing list