[Lldb-commits] [lldb] 4add853 - [lldb] Improve platform handling in CreateTargetInternal
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Wed Jul 29 10:30:27 PDT 2020
Author: Jonas Devlieghere
Date: 2020-07-29T10:30:20-07:00
New Revision: 4add853647b358d9bb7f162682e806725888848b
URL: https://github.com/llvm/llvm-project/commit/4add853647b358d9bb7f162682e806725888848b
DIFF: https://github.com/llvm/llvm-project/commit/4add853647b358d9bb7f162682e806725888848b.diff
LOG: [lldb] Improve platform handling in CreateTargetInternal
Currently, `target create` has no --platform option. However,
TargetList::CreateTargetInternal which is called under the hood, will
return an error when either no platform or multiple matching platforms
are found, saying that a platform should be specified with --platform.
This patch adds the platform option, but that doesn't solve either of
these errors.
- If more than one platform matches, specifying the platform isn't
going to fix that. The current code will only look at the
architecture instead. I've updated the error message to ask the user
to specify an architecture.
- If no architecture is found, specifying a new one via platform isn't
going to change that either because we already try to find one that
matches the given architecture.
Differential revision: https://reviews.llvm.org/D84809
Added:
lldb/test/API/commands/target/basic/bogus.yaml
Modified:
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Target/TargetList.cpp
lldb/test/API/commands/target/basic/TestTargetCommand.py
Removed:
################################################################################
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index e975d99ee024..84b7c988353a 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -23,6 +23,7 @@
#include "lldb/Interpreter/OptionGroupBoolean.h"
#include "lldb/Interpreter/OptionGroupFile.h"
#include "lldb/Interpreter/OptionGroupFormat.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
#include "lldb/Interpreter/OptionGroupString.h"
#include "lldb/Interpreter/OptionGroupUInt64.h"
#include "lldb/Interpreter/OptionGroupUUID.h"
@@ -214,6 +215,7 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
"Create a target using the argument as the main executable.",
nullptr),
m_option_group(), m_arch_option(),
+ m_platform_options(true), // Include the --platform option.
m_core_file(LLDB_OPT_SET_1, false, "core", 'c', 0, eArgTypeFilename,
"Fullpath to a core file to use for this target."),
m_symbol_file(LLDB_OPT_SET_1, false, "symfile", 's', 0,
@@ -240,6 +242,7 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
m_arguments.push_back(arg);
m_option_group.Append(&m_arch_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
+ m_option_group.Append(&m_platform_options, LLDB_OPT_SET_ALL, 1);
m_option_group.Append(&m_core_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
m_option_group.Append(&m_symbol_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
m_option_group.Append(&m_remote_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
@@ -311,7 +314,8 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
llvm::StringRef arch_cstr = m_arch_option.GetArchitectureName();
Status error(debugger.GetTargetList().CreateTarget(
debugger, file_path, arch_cstr,
- m_add_dependents.m_load_dependent_files, nullptr, target_sp));
+ m_add_dependents.m_load_dependent_files, &m_platform_options,
+ target_sp));
if (target_sp) {
// Only get the platform after we create the target because we might
@@ -442,6 +446,7 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
private:
OptionGroupOptions m_option_group;
OptionGroupArchitecture m_arch_option;
+ OptionGroupPlatform m_platform_options;
OptionGroupFile m_core_file;
OptionGroupFile m_symbol_file;
OptionGroupFile m_remote_file;
diff --git a/lldb/source/Target/TargetList.cpp b/lldb/source/Target/TargetList.cpp
index 3974cb5de419..75a022d2c463 100644
--- a/lldb/source/Target/TargetList.cpp
+++ b/lldb/source/Target/TargetList.cpp
@@ -75,55 +75,49 @@ Status TargetList::CreateTargetInternal(
const OptionGroupPlatform *platform_options, TargetSP &target_sp,
bool is_dummy_target) {
Status error;
- PlatformSP platform_sp;
- // This is purposely left empty unless it is specified by triple_cstr. If not
- // initialized via triple_cstr, then the currently selected platform will set
- // the architecture correctly.
+ // Let's start by looking at the selected platform.
+ PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+
+ // This variable corresponds to the architecture specified by the triple
+ // string. If that string was empty the currently selected platform will
+ // determine the architecture.
const ArchSpec arch(triple_str);
- if (!triple_str.empty()) {
- if (!arch.IsValid()) {
- error.SetErrorStringWithFormat("invalid triple '%s'",
- triple_str.str().c_str());
- return error;
- }
+ if (!triple_str.empty() && !arch.IsValid()) {
+ error.SetErrorStringWithFormat("invalid triple '%s'",
+ triple_str.str().c_str());
+ return error;
}
ArchSpec platform_arch(arch);
- bool prefer_platform_arch = false;
-
- CommandInterpreter &interpreter = debugger.GetCommandInterpreter();
-
- // let's see if there is already an existing platform before we go creating
- // another...
- platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
-
- if (platform_options && platform_options->PlatformWasSpecified()) {
- // Create a new platform if it doesn't match the selected platform
- if (!platform_options->PlatformMatches(platform_sp)) {
- const bool select_platform = true;
- platform_sp = platform_options->CreatePlatformWithOptions(
- interpreter, arch, select_platform, error, platform_arch);
- if (!platform_sp)
- return error;
- }
+ // Create a new platform if a platform was specified in the platform options
+ // and doesn't match the selected platform.
+ if (platform_options && platform_options->PlatformWasSpecified() &&
+ !platform_options->PlatformMatches(platform_sp)) {
+ const bool select_platform = true;
+ platform_sp = platform_options->CreatePlatformWithOptions(
+ debugger.GetCommandInterpreter(), arch, select_platform, error,
+ platform_arch);
+ if (!platform_sp)
+ return error;
}
+ bool prefer_platform_arch = false;
+
if (!user_exe_path.empty()) {
- ModuleSpecList module_specs;
- ModuleSpec module_spec;
- module_spec.GetFileSpec().SetFile(user_exe_path, FileSpec::Style::native);
+ ModuleSpec module_spec(FileSpec(user_exe_path, FileSpec::Style::native));
FileSystem::Instance().Resolve(module_spec.GetFileSpec());
-
// Resolve the executable in case we are given a path to a application
- // bundle like a .app bundle on MacOSX
+ // bundle like a .app bundle on MacOSX.
Host::ResolveExecutableInBundle(module_spec.GetFileSpec());
lldb::offset_t file_offset = 0;
lldb::offset_t file_size = 0;
+ ModuleSpecList module_specs;
const size_t num_specs = ObjectFile::GetModuleSpecifications(
module_spec.GetFileSpec(), file_offset, file_size, module_specs);
+
if (num_specs > 0) {
ModuleSpec matching_module_spec;
@@ -134,7 +128,7 @@ Status TargetList::CreateTargetInternal(
matching_module_spec.GetArchitecture())) {
// If the OS or vendor weren't specified, then adopt the module's
// architecture so that the platform matching can be more
- // accurate
+ // accurate.
if (!platform_arch.TripleOSWasSpecified() ||
!platform_arch.TripleVendorWasSpecified()) {
prefer_platform_arch = true;
@@ -155,113 +149,107 @@ Status TargetList::CreateTargetInternal(
return error;
}
} else {
- // Only one arch and none was specified
+ // Only one arch and none was specified.
prefer_platform_arch = true;
platform_arch = matching_module_spec.GetArchitecture();
}
}
+ } else if (arch.IsValid()) {
+ // A (valid) architecture was specified.
+ module_spec.GetArchitecture() = arch;
+ if (module_specs.FindMatchingModuleSpec(module_spec,
+ matching_module_spec)) {
+ prefer_platform_arch = true;
+ platform_arch = matching_module_spec.GetArchitecture();
+ }
} else {
- if (arch.IsValid()) {
- module_spec.GetArchitecture() = arch;
- if (module_specs.FindMatchingModuleSpec(module_spec,
- matching_module_spec)) {
- prefer_platform_arch = true;
- platform_arch = matching_module_spec.GetArchitecture();
- }
- } else {
- // No architecture specified, check if there is only one platform for
- // all of the architectures.
-
- typedef std::vector<PlatformSP> PlatformList;
- PlatformList platforms;
- PlatformSP host_platform_sp = Platform::GetHostPlatform();
- for (size_t i = 0; i < num_specs; ++i) {
- ModuleSpec module_spec;
- if (module_specs.GetModuleSpecAtIndex(i, module_spec)) {
- // See if there was a selected platform and check that first
- // since the user may have specified it.
- if (platform_sp) {
- if (platform_sp->IsCompatibleArchitecture(
- module_spec.GetArchitecture(), false, nullptr)) {
- platforms.push_back(platform_sp);
- continue;
- }
- }
-
- // Next check the host platform it if wasn't already checked
- // above
- if (host_platform_sp &&
- (!platform_sp ||
- host_platform_sp->GetName() != platform_sp->GetName())) {
- if (host_platform_sp->IsCompatibleArchitecture(
- module_spec.GetArchitecture(), false, nullptr)) {
- platforms.push_back(host_platform_sp);
- continue;
- }
+ // No architecture specified, check if there is only one platform for
+ // all of the architectures.
+ PlatformSP host_platform_sp = Platform::GetHostPlatform();
+ std::vector<PlatformSP> platforms;
+ for (size_t i = 0; i < num_specs; ++i) {
+ ModuleSpec module_spec;
+ if (module_specs.GetModuleSpecAtIndex(i, module_spec)) {
+ // First consider the platform specified by the user, if any, and
+ // the selected platform otherwise.
+ if (platform_sp) {
+ if (platform_sp->IsCompatibleArchitecture(
+ module_spec.GetArchitecture(), false, nullptr)) {
+ platforms.push_back(platform_sp);
+ continue;
}
+ }
- // Just find a platform that matches the architecture in the
- // executable file
- PlatformSP fallback_platform_sp(
- Platform::GetPlatformForArchitecture(
- module_spec.GetArchitecture(), nullptr));
- if (fallback_platform_sp) {
- platforms.push_back(fallback_platform_sp);
+ // Now consider the host platform if it is
diff erent from the
+ // specified/selected platform.
+ if (host_platform_sp &&
+ (!platform_sp ||
+ host_platform_sp->GetName() != platform_sp->GetName())) {
+ if (host_platform_sp->IsCompatibleArchitecture(
+ module_spec.GetArchitecture(), false, nullptr)) {
+ platforms.push_back(host_platform_sp);
+ continue;
}
}
- }
- Platform *platform_ptr = nullptr;
- bool more_than_one_platforms = false;
- for (const auto &the_platform_sp : platforms) {
- if (platform_ptr) {
- if (platform_ptr->GetName() != the_platform_sp->GetName()) {
- more_than_one_platforms = true;
- platform_ptr = nullptr;
- break;
- }
- } else {
- platform_ptr = the_platform_sp.get();
+ // Finally find a platform that matches the architecture in the
+ // executable file.
+ PlatformSP fallback_platform_sp(
+ Platform::GetPlatformForArchitecture(
+ module_spec.GetArchitecture(), nullptr));
+ if (fallback_platform_sp) {
+ platforms.push_back(fallback_platform_sp);
}
}
+ }
+ Platform *platform_ptr = nullptr;
+ bool more_than_one_platforms = false;
+ for (const auto &the_platform_sp : platforms) {
if (platform_ptr) {
- // All platforms for all modules in the executable match, so we can
- // select this platform
- platform_sp = platforms.front();
- } else if (!more_than_one_platforms) {
- // No platforms claim to support this file
- error.SetErrorString("No matching platforms found for this file, "
- "specify one with the --platform option");
- return error;
+ if (platform_ptr->GetName() != the_platform_sp->GetName()) {
+ more_than_one_platforms = true;
+ platform_ptr = nullptr;
+ break;
+ }
} else {
- // More than one platform claims to support this file, so the
- // --platform option must be specified
- StreamString error_strm;
- std::set<Platform *> platform_set;
- error_strm.Printf(
- "more than one platform supports this executable (");
- for (const auto &the_platform_sp : platforms) {
- if (platform_set.find(the_platform_sp.get()) ==
- platform_set.end()) {
- if (!platform_set.empty())
- error_strm.PutCString(", ");
- error_strm.PutCString(the_platform_sp->GetName().GetCString());
- platform_set.insert(the_platform_sp.get());
- }
+ platform_ptr = the_platform_sp.get();
+ }
+ }
+
+ if (platform_ptr) {
+ // All platforms for all modules in the executable match, so we can
+ // select this platform.
+ platform_sp = platforms.front();
+ } else if (!more_than_one_platforms) {
+ // No platforms claim to support this file.
+ error.SetErrorString("no matching platforms found for this file");
+ return error;
+ } else {
+ // More than one platform claims to support this file.
+ StreamString error_strm;
+ std::set<Platform *> platform_set;
+ error_strm.Printf(
+ "more than one platform supports this executable (");
+ for (const auto &the_platform_sp : platforms) {
+ if (platform_set.find(the_platform_sp.get()) ==
+ platform_set.end()) {
+ if (!platform_set.empty())
+ error_strm.PutCString(", ");
+ error_strm.PutCString(the_platform_sp->GetName().GetCString());
+ platform_set.insert(the_platform_sp.get());
}
- error_strm.Printf(
- "), use the --platform option to specify a platform");
- error.SetErrorString(error_strm.GetString());
- return error;
}
+ error_strm.Printf("), specify an architecture to disambiguate");
+ error.SetErrorString(error_strm.GetString());
+ return error;
}
}
}
}
// If we have a valid architecture, make sure the current platform is
- // compatible with that architecture
+ // compatible with that architecture.
if (!prefer_platform_arch && arch.IsValid()) {
if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch)) {
platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
@@ -269,8 +257,8 @@ Status TargetList::CreateTargetInternal(
debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
}
} else if (platform_arch.IsValid()) {
- // if "arch" isn't valid, yet "platform_arch" is, it means we have an
- // executable file with a single architecture which should be used
+ // If "arch" isn't valid, yet "platform_arch" is, it means we have an
+ // executable file with a single architecture which should be used.
ArchSpec fixed_platform_arch;
if (!platform_sp->IsCompatibleArchitecture(platform_arch, false,
&fixed_platform_arch)) {
@@ -284,10 +272,9 @@ Status TargetList::CreateTargetInternal(
if (!platform_arch.IsValid())
platform_arch = arch;
- error = TargetList::CreateTargetInternal(
+ return TargetList::CreateTargetInternal(
debugger, user_exe_path, platform_arch, load_dependent_files, platform_sp,
target_sp, is_dummy_target);
- return error;
}
lldb::TargetSP TargetList::GetDummyTarget(lldb_private::Debugger &debugger) {
diff --git a/lldb/test/API/commands/target/basic/TestTargetCommand.py b/lldb/test/API/commands/target/basic/TestTargetCommand.py
index 83e27e272464..be6eeb938ab8 100644
--- a/lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ b/lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -115,6 +115,33 @@ def do_target_command(self):
self.runCmd("target list")
+ @no_debug_info_test
+ def test_target_create_invalid_arch(self):
+ exe = self.getBuildArtifact("a.out")
+ self.expect("target create {} --arch doesntexist".format(exe), error=True,
+ patterns=["error: invalid triple 'doesntexist'"])
+
+ @no_debug_info_test
+ def test_target_create_platform(self):
+ self.buildB()
+ exe = self.getBuildArtifact("b.out")
+ self.expect("target create {} --platform host".format(exe))
+
+ @no_debug_info_test
+ def test_target_create_unsupported_platform(self):
+ yaml = os.path.join(self.getSourceDir(), "bogus.yaml")
+ exe = self.getBuildArtifact("bogus")
+ self.yaml2obj(yaml, exe)
+ self.expect("target create {}".format(exe), error=True,
+ patterns=['error: no matching platforms found for this file'])
+
+ @no_debug_info_test
+ def test_target_create_invalid_platform(self):
+ self.buildB()
+ exe = self.getBuildArtifact("b.out")
+ self.expect("target create {} --platform doesntexist".format(exe), error=True,
+ patterns=['error: unable to find a plug-in for the platform named "doesntexist"'])
+
def do_target_variable_command(self, exe_name):
"""Exercise 'target variable' command before and after starting the inferior."""
self.runCmd("file " + self.getBuildArtifact(exe_name),
diff --git a/lldb/test/API/commands/target/basic/bogus.yaml b/lldb/test/API/commands/target/basic/bogus.yaml
new file mode 100644
index 000000000000..d36695cdedd6
--- /dev/null
+++ b/lldb/test/API/commands/target/basic/bogus.yaml
@@ -0,0 +1,194 @@
+--- !fat-mach-o
+FatHeader:
+ magic: 0xCAFEBABE
+ nfat_arch: 3
+FatArchs:
+ - cputype: 0x0000000C
+ cpusubtype: 0x00000009
+ offset: 0x0000000000004000
+ size: 200
+ align: 14
+ - cputype: 0x0000000C
+ cpusubtype: 0x0000000B
+ offset: 0x0000000000008000
+ size: 200
+ align: 14
+ - cputype: 0x0100000C
+ cpusubtype: 0x00000000
+ offset: 0x000000000000C000
+ size: 332
+ align: 14
+Slices:
+ - !mach-o
+ FileHeader:
+ magic: 0xFEEDFACE
+ # Bogus
+ cputype: 0x00000003
+ cpusubtype: 0x00000009
+ filetype: 0x00000001
+ ncmds: 4
+ sizeofcmds: 112
+ flags: 0x00002000
+ LoadCommands:
+ - cmd: LC_SEGMENT
+ cmdsize: 56
+ segname: ''
+ vmaddr: 0
+ vmsize: 0
+ fileoff: 0
+ filesize: 0
+ maxprot: 7
+ initprot: 7
+ nsects: 0
+ flags: 0
+ - cmd: LC_SYMTAB
+ cmdsize: 24
+ symoff: 172
+ nsyms: 1
+ stroff: 184
+ strsize: 16
+ - cmd: LC_VERSION_MIN_IPHONEOS
+ cmdsize: 16
+ version: 327680
+ sdk: 0
+ - cmd: LC_DATA_IN_CODE
+ cmdsize: 16
+ dataoff: 172
+ datasize: 0
+ LinkEditData:
+ NameList:
+ - n_strx: 4
+ n_type: 0x01
+ n_sect: 0
+ n_desc: 512
+ n_value: 4
+ StringTable:
+ - ''
+ - ''
+ - ''
+ - ''
+ - _armv7_var
+ - ''
+ - !mach-o
+ FileHeader:
+ magic: 0xFEEDFACE
+ # Bogus
+ cputype: 0x00000002
+ cpusubtype: 0x0000000B
+ filetype: 0x00000001
+ ncmds: 4
+ sizeofcmds: 112
+ flags: 0x00002000
+ LoadCommands:
+ - cmd: LC_SEGMENT
+ cmdsize: 56
+ segname: ''
+ vmaddr: 0
+ vmsize: 0
+ fileoff: 0
+ filesize: 0
+ maxprot: 7
+ initprot: 7
+ nsects: 0
+ flags: 0
+ - cmd: LC_SYMTAB
+ cmdsize: 24
+ symoff: 172
+ nsyms: 1
+ stroff: 184
+ strsize: 16
+ - cmd: LC_VERSION_MIN_IPHONEOS
+ cmdsize: 16
+ version: 327680
+ sdk: 0
+ - cmd: LC_DATA_IN_CODE
+ cmdsize: 16
+ dataoff: 172
+ datasize: 0
+ LinkEditData:
+ NameList:
+ - n_strx: 4
+ n_type: 0x01
+ n_sect: 0
+ n_desc: 512
+ n_value: 4
+ StringTable:
+ - ''
+ - ''
+ - ''
+ - ''
+ - _armv7s_var
+ - !mach-o
+ FileHeader:
+ magic: 0xFEEDFACF
+ # Bogus
+ cputype: 0x00000001
+ cpusubtype: 0x00000000
+ filetype: 0x00000001
+ ncmds: 4
+ sizeofcmds: 208
+ flags: 0x00002000
+ reserved: 0x00000000
+ LoadCommands:
+ - cmd: LC_SEGMENT_64
+ cmdsize: 152
+ segname: ''
+ vmaddr: 0
+ vmsize: 0
+ fileoff: 272
+ filesize: 0
+ maxprot: 7
+ initprot: 7
+ nsects: 1
+ flags: 0
+ Sections:
+ - sectname: __text
+ segname: __TEXT
+ addr: 0x0000000000000000
+ size: 0
+ offset: 0x00000110
+ align: 0
+ reloff: 0x00000000
+ nreloc: 0
+ flags: 0x80000400
+ reserved1: 0x00000000
+ reserved2: 0x00000000
+ reserved3: 0x00000000
+ content: ''
+ - cmd: LC_SYMTAB
+ cmdsize: 24
+ symoff: 276
+ nsyms: 2
+ stroff: 308
+ strsize: 24
+ - cmd: LC_VERSION_MIN_IPHONEOS
+ cmdsize: 16
+ version: 327680
+ sdk: 0
+ - cmd: LC_DATA_IN_CODE
+ cmdsize: 16
+ dataoff: 276
+ datasize: 0
+ LinkEditData:
+ NameList:
+ - n_strx: 15
+ n_type: 0x0E
+ n_sect: 1
+ n_desc: 0
+ n_value: 0
+ - n_strx: 4
+ n_type: 0x01
+ n_sect: 0
+ n_desc: 512
+ n_value: 4
+ StringTable:
+ - ''
+ - ''
+ - ''
+ - ''
+ - _arm64_var
+ - ltmp0
+ - ''
+ - ''
+ - ''
+...
More information about the lldb-commits
mailing list