[target] Change target create's behavior wrt loading dependent files.

[JDevlieghere]

Summary

When creating a target, lldb loads all dependent files (i.e. libs in LC_LOAD_DYLIB for Mach-O). This can be confusing, especially when two versions of the same library end up in the shared cache. It's possible to change this behavior, by specifying target create -d <target> these dependents are not loaded.

This patch changes the default behavior to only load dependent files only when the target is an executable. When creating a target for a library, it is now no longer necessary to pass -d. The user can still override this behavior by specifying the -d option. However, rather than a boolean you can now specify one of three values: default, yes, or no.

rdar://problem/43721382

[clayborg]

I would rather not change the argument to be required if we can help it. That might break existing scripts or command files. I think if -d is specified without a value it should default to "no". Can we make the value optional?

[clayborg]

Or add a new option that is --dependents that takes the enum and leave the --no-dependents there for compatability

[JDevlieghere]

I totally agree; I've left the argument name unchanged and made the argument optional. I had to change the semantics slightly (because of the negation in the argument name) but otherwise things will continue working as before (with the notable exception of the default case if no -d is specified of course).

[clayborg]

source/Commands/CommandObjectTarget.cpp

143–151

Maybe document which is the default value in one of these three strings?

[JDevlieghere]

• Document default behavior.

[shafik]

source/Commands/CommandObjectTarget.cpp

153

Should "no-dependents" be "default"?

181

Wouldn't an if/else make more sense here? I had to reread the description up top before this made sense that option_value is doing the work here.

[JDevlieghere]

• Address Shafik's feedback

[JDevlieghere]

source/Commands/CommandObjectTarget.cpp

153

Only if the target is not an executable, i.e. a shared library. This is what the eLoadDependentsDefault enum value is for.

[JDevlieghere]

ping

[clayborg]

I am fine with this, Jim or Jason should ok this too just to be sure

[jingham]

A typo and probably something copied from another test case, other than that this looks good.

packages/Python/lldbsuite/test/functionalities/target_create_deps/TestTargetCreateDeps.py

3

I don't think this is what the test actually does...

source/Commands/CommandObjectTarget.cpp

145

"executable" not "executables"

[stella.stamenova]

This test has been failing on Windows since it was added as it doesn't build correctly. I noticed some of the tests are also disabled on Linux. Is this supposed to pass on all platforms?

Error when building test subject.
Build Command:
make VPATH=E:\_work\60\s\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\target_create_deps -C E:\_work\60\b\LLVMBuild\lldb-test-build.noindex\functionalities\target_create_deps\TestTargetCreateDeps.test_dependents_explicit_default_lib -I E:\_work\60\s\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\target_create_deps -f E:\_work\60\s\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\target_create_deps\Makefile all ARCH=x86_64 CC="E:\_work\60\b\LLVMBuild\Release\bin\clang.exe" 

##[error]lld-link(0,0): Error : undefined symbol: "int __cdecl a_function(void)" (?a_function@@YAHXZ)
lld-link : error : undefined symbol: "int __cdecl a_function(void)" (?a_function@@YAHXZ) [e:\_work\60\b\LLVMBuild\check-all.vcxproj]
>>> referenced by main.o:(main)
##[error]clang(0,0): Error : linker command failed with exit code 1 (use -v to see invocation)
clang : error : linker command failed with exit code 1 (use -v to see invocation) [e:\_work\60\b\LLVMBuild\check-all.vcxproj]
make: *** [a.out] Error 1

[jingham]

IIRC, the test is disabled on Linux not because of problems with building the test executables, but because the Linux port doesn't currently load the dependencies of an binary when it loads the binary. So the test was irrelevant on Linux, as was the feature.

[stella.stamenova]

Thanks. I guess the more explicit question is: which platforms is this feature (and test) applicable to? The test should be disabled on other platforms. It is already disabled on Linux because it is not applicable and it is failing to even build on Windows in its current state.

@JDevlieghere , do you expect this to work on Windows?

[jingham]

I wouldn't be surprised if Jonas isn't familiar enough with the Windows port to answer the question. But if you have a running lldb you can tell easily. Just do:

lldb SomeBinaryThatLoadsSharedLibraries
(lldb) image list

Do you get only SomeBinaryThatLoadsSharedLibraries, or do you get a list of the dependencies of the binary as well? If the former, then this test won't test anything for you. If the latter, then this test will test something real, and we should figure out how to get the test to build on Windows.

packages/Python/lldbsuite/test/functionalities/target_create_deps/Makefile

+LEVEL := ../../make
+
+LIB_PREFIX := load_
+
+LD_EXTRAS := -L. -l$(LIB_PREFIX)a
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules
+
+a.out: lib_a
+
+lib_%:
+  $(MAKE) VPATH=$(SRCDIR) -I $(SRCDIR) -f $(SRCDIR)/$*.mk
+
+clean::
+  $(MAKE) -f $(SRCDIR)/a.mk clean

packages/Python/lldbsuite/test/functionalities/target_create_deps/TestTargetCreateDeps.py

+"""
+Test that loading of dependents works correctly for all the potential
+combinations.
+"""
+
+from __future__ import print_function
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetDependentsTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        TestBase.setUp(self)
+        self.build()
+
+    def has_exactly_one_image(self, matching, msg=""):
+        self.expect(
+            "image list",
+            "image list should contain at least one image",
+            substrs=['[  0]'])
+        should_match = not matching
+        self.expect(
+            "image list", msg, matching=should_match, substrs=['[  1]'])
+
+    def test_dependents_implicit_default_exe(self):
+        """Test default behavior"""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create  " + exe, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(False)
+
+    def test_dependents_explicit_default_exe(self):
+        """Test default behavior"""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create -ddefault " + exe, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(False)
+
+    def test_dependents_explicit_true_exe(self):
+        """Test default behavior"""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create -dtrue " + exe, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)
+
+    def test_dependents_explicit_false_exe(self):
+        """Test default behavior"""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create -dfalse " + exe, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(False)
+
+    def test_dependents_implicit_false_exe(self):
+        """Test default behavior"""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("target create  -d " + exe, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)
+
+    def test_dependents_implicit_default_lib(self):
+        ctx = self.platformContext
+        dylibName = ctx.shlib_prefix + 'load_a.' + ctx.shlib_extension
+        lib = self.getBuildArtifact(dylibName)
+        self.runCmd("target create " + lib, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)
+
+    def test_dependents_explicit_default_lib(self):
+        ctx = self.platformContext
+        dylibName = ctx.shlib_prefix + 'load_a.' + ctx.shlib_extension
+        lib = self.getBuildArtifact(dylibName)
+        self.runCmd("target create -ddefault " + lib, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)
+
+    def test_dependents_explicit_true_lib(self):
+        ctx = self.platformContext
+        dylibName = ctx.shlib_prefix + 'load_a.' + ctx.shlib_extension
+        lib = self.getBuildArtifact(dylibName)
+        self.runCmd("target create -dtrue " + lib, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)
+
+    def test_dependents_explicit_false_lib(self):
+        ctx = self.platformContext
+        dylibName = ctx.shlib_prefix + 'load_a.' + ctx.shlib_extension
+        lib = self.getBuildArtifact(dylibName)
+        self.runCmd("target create -dfalse " + lib, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(False)
+
+    def test_dependents_implicit_false_lib(self):
+        ctx = self.platformContext
+        dylibName = ctx.shlib_prefix + 'load_a.' + ctx.shlib_extension
+        lib = self.getBuildArtifact(dylibName)
+        self.runCmd("target create -d " + lib, CURRENT_EXECUTABLE_SET)
+        self.has_exactly_one_image(True)

packages/Python/lldbsuite/test/functionalities/target_create_deps/a.cpp

+//===-- b.c -----------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+int a_function ()
+{
+    return 500;
+}

packages/Python/lldbsuite/test/functionalities/target_create_deps/a.mk

+LEVEL := ../../make
+
+LIB_PREFIX := load_
+
+DYLIB_NAME := $(LIB_PREFIX)a
+DYLIB_CXX_SOURCES := a.cpp
+DYLIB_ONLY := YES
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/functionalities/target_create_deps/main.cpp

+//===-- main.c --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+extern int a_function ();
+extern int b_function ();
+
+int
+main (int argc, char const *argv[])
+{
+    return a_function();
+}

source/Commands/CommandObjectTarget.cpp

for (uint32_t i = 0; i < num_targets; ++i) {
         DumpTargetInfo(i, target_sp.get(), is_selected ? "* " : "  ",
                        show_stopped_process_status, strm);
       }
     }
   }
   return num_targets;
 }
 
+// Note that the negation in the argument name causes a slightly confusing
+// mapping of the enum values,
+static OptionEnumValueElement g_dependents_enumaration[4] = {
+    {eLoadDependentsDefault, "default",
+     "Only load dependents when the target is an executable."},
+    {eLoadDependentsNo, "true",
+     "Don't load dependents, even if the target is an executable."},
+    {eLoadDependentsYes, "false",
+     "Load dependents, even if the target is not an executable."},
+    {0, nullptr, nullptr}};
+
+static OptionDefinition g_dependents_options[1] = {
+    {LLDB_OPT_SET_1, false, "no-dependents", 'd',
+     OptionParser::eOptionalArgument, nullptr, g_dependents_enumaration, 0,
+     eArgTypeValue,
+     "Whether or not to load dependents when creating a target. If the option "
+     "is not specified, the value is implicitly 'default'. If the option is "
+     "specified but without a value, the value is implicitly 'true'."}};
+
+class OptionGroupDependents : public OptionGroup {
+public:
+  OptionGroupDependents() {}
+
+  ~OptionGroupDependents() override {}
+
+  llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+    return llvm::makeArrayRef(g_dependents_options);
+  }
+
+  Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
+                        ExecutionContext *execution_context) override {
+    Status error;
+
+    // For compatibility no value means don't load dependents.
+    if (option_value.empty()) {
+      m_load_dependent_files = eLoadDependentsNo;
+      return error;
+    }
+
+    const char short_option = g_dependents_options[option_idx].short_option;
+    if (short_option == 'd') {
+      LoadDependentFiles tmp_load_dependents;
+      tmp_load_dependents = (LoadDependentFiles)OptionArgParser::ToOptionEnum(
+          option_value, g_dependents_options[option_idx].enum_values, 0, error);
+      if (error.Success())
+        m_load_dependent_files = tmp_load_dependents;
+    } else {
+      error.SetErrorStringWithFormat("unrecognized short option '%c'",
+                                     short_option);
+    }
+
+    return error;
+  }
+
+  Status SetOptionValue(uint32_t, const char *, ExecutionContext *) = delete;
+
+  void OptionParsingStarting(ExecutionContext *execution_context) override {
+    m_load_dependent_files = eLoadDependentsDefault;
+  }
+
+  LoadDependentFiles m_load_dependent_files;
+
+private:
+  DISALLOW_COPY_AND_ASSIGN(OptionGroupDependents);
+};
+
 #pragma mark CommandObjectTargetCreate
 
 //-------------------------------------------------------------------------
 // "target create"
 //-------------------------------------------------------------------------
 
 class CommandObjectTargetCreate : public CommandObjectParsed {
 public:
   CommandObjectTargetCreate(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "target create",
             "Create a target using the argument as the main executable.",
             nullptr),
         m_option_group(), m_arch_option(),
         m_core_file(LLDB_OPT_SET_1, false, "core", 'c', 0, eArgTypeFilename,
                     "Fullpath to a core file to use for this target."),
         m_platform_path(LLDB_OPT_SET_1, false, "platform-path", 'P', 0,
                         eArgTypePath,
                         "Path to the remote file to use for this target."),
         m_symbol_file(LLDB_OPT_SET_1, false, "symfile", 's', 0,
-                      eArgTypeFilename, "Fullpath to a stand alone debug "
-                                        "symbols file for when debug symbols "
-                                        "are not in the executable."),
+                      eArgTypeFilename,
+                      "Fullpath to a stand alone debug "
         m_remote_file(
             LLDB_OPT_SET_1, false, "remote-file", 'r', 0, eArgTypeFilename,
             "Fullpath to the file on the remote host if debugging remotely."),
-        m_add_dependents(LLDB_OPT_SET_1, false, "no-dependents", 'd',
-                         "Don't load dependent files when creating the target, "
-                         "just add the specified executable.",
-                         true, true) {
+        m_add_dependents() {
     CommandArgumentEntry arg;
     CommandArgumentData file_arg;
 
     // Define the first (and only) variant of this arg.
     file_arg.arg_type = eArgTypeFilename;
     file_arg.arg_repetition = eArgRepeatPlain;
 
     // There is only one variant this argument could be; put it into the
if (argc == 1 || core_file || remote_file) {
         file_spec.SetFile(file_path, true, FileSpec::Style::native);
 
       bool must_set_platform_path = false;
 
       Debugger &debugger = m_interpreter.GetDebugger();
 
       TargetSP target_sp;
       llvm::StringRef arch_cstr = m_arch_option.GetArchitectureName();
-      const bool get_dependent_files =
-          m_add_dependents.GetOptionValue().GetCurrentValue();
       Status error(debugger.GetTargetList().CreateTarget(
           debugger, file_path, arch_cstr,
-          get_dependent_files ? eLoadDependentsYes : eLoadDependentsNo, nullptr,
-          target_sp));
+          m_add_dependents.m_load_dependent_files, nullptr, target_sp));
 
       if (target_sp) {
         // Only get the platform after we create the target because we might
         // have switched platforms depending on what the arguments were to
         // CreateTarget() we can't rely on the selected platform.
 
         PlatformSP platform_sp = target_sp->GetPlatform();
 

 
 private:
   OptionGroupOptions m_option_group;
   OptionGroupArchitecture m_arch_option;
   OptionGroupFile m_core_file;
   OptionGroupFile m_platform_path;
   OptionGroupFile m_symbol_file;
   OptionGroupFile m_remote_file;
-  OptionGroupBoolean m_add_dependents;
+  OptionGroupDependents m_add_dependents;
 };
 
 #pragma mark CommandObjectTargetList
 
 //----------------------------------------------------------------------
 // "target list"
 //----------------------------------------------------------------------