Extend 'target symbols add' to set symbol file for a given module

[eugene]

Summary

Now -shlib flag can be provided alongside with names of symbols files:

(lldb) target symbols add --shlib stripper-lib.so unstripper-lib.so

This is helpful when default matching mechanisms by name and UUID
can't find a module, and the user needs to explicitly specify
which module the given symbol file belongs to.

[labath]

I think Jim (or maybe Greg?) should take a look at this.

Right now I'll just comment on the test.

packages/Python/lldbsuite/test/linux/add-symbols/Makefile

1

This is a very non-standard Makefile. As it is now, it will fail if running the test against android.

I think you should be able to replace the strip command with a LD_EXTRAS += -Wl,--build-id=none. This should allow you to use Makefile.rules and be more portable. Also, I'd recommend setting this up in a way that you don't overwrite the a.out compiler output, as that will make the rules simpler. I'm thinking of something like this (untested) snippet:

LEVEL= ???
C_SOURCES := a.c
LD_EXTRAS += -Wl,--build-id=none

include $(LEVEL)/Makefile.rules

b.out: a.out
        $(OBJCOPY) --strip-debug a.out b.out #Have the test use b.out as the main executable

all: b.out
clean::
  rm -rf b.out

[labath]

Btw, will this work correctly if we are cross-debugging from a mac? (because then we will use the fancy DownloadObjectAndSymbolFile implementation)

[eugene]

In D35607#814560, @labath wrote:

Btw, will this work correctly if we are cross-debugging from a mac? (because then we will use the fancy DownloadObjectAndSymbolFile implementation)

Yeah, you're right. I'll move this logic to the CommandObjectTarget.cpp itself.

[jingham]

This seems like an okay change. You want to make sure that passing the file name and UUID into AddModuleSymbols doesn't override a UUID mismatch against the symbol file however you find it. It looks like that function does the right thing, but can you add a test for this?

You also need to fix up the error messages (around line 4270.) At present they assume that the uuid & file option's are mutually exclusive. It's disconcerting to specify two things and have the error mention only one.

This is more Greg's area than mine, so he should also give it the okay.

[clayborg]

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does? In that case, there is no need to specify the --uuid option. If the UUID do match, then you could just do:

(lldb) target symbols add debug_binaries/a.out

It would extract the UUID from "debug_binaries/a.out" and then match it to the file. So the "--shlib" option is there to point you in the right direction. When the UUIDs don't match, we should make sure that "target symbols add --shlib a.out debug_binaries/a.out" by recognizing that "a.out" doesn't have a valid UUID so the UUID doesn't need to match in order for "debug_binaries/a.out" to be an acceptable match for it as a symbol file. This might make some of the changes above go away and move them to the ModuleSpec matching code.

[labath]

In D35607#814982, @clayborg wrote:

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does?

In this case, neither of the files has a build id/UUID. However, the tricky part here is that in this case we "invent" a UUID by doing a checksum of the whole file. This is done to support the .gnu_debuglink https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html method of finding symbols. It works like this:

• if a file has a gnu_debuglink section, we set the UUID to the crc inside that section
• if a file does not have a gnu_debuglink section, we *assume* it will be used as a gnu_debuglink target, and also set the UUID to the crc we compute from the whole file contents.

This works fine for the gnu_debuglink case, as then the rest of the machinery will match the files based on their UUIDs. However, it does not work for the general case without the debug link, as then the files will end up with two different "UUID"s (because they are based on the file content), and everything will consider them as different.

The solution that would seem best to me here would be to leave the UUID of files which don't have a build-id as empty, as the gnu_debuglink is not really a UUID -- it's main function is to contain a file path, and the crc just acts as a sanity check. This should make the logic for matching two uuid-less files saner, but it would require us to implement the gnu_debuglink method in a different way (which I think is fine, as the current way seems pretty hacky, and causes us to often compute crcs of large files needlessly). I have no idea how much of our existing code would blow up if we started having files without a uuid though.

What do you think about that?

[clayborg]

In D35607#815769, @labath wrote:

In D35607#814982, @clayborg wrote:

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does?

In this case, neither of the files has a build id/UUID. However, the tricky part here is that in this case we "invent" a UUID by doing a checksum of the whole file. This is done to support the .gnu_debuglink https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html method of finding symbols. It works like this:

• if a file has a gnu_debuglink section, we set the UUID to the crc inside that section
• if a file does not have a gnu_debuglink section, we *assume* it will be used as a gnu_debuglink target, and also set the UUID to the crc we compute from the whole file contents.

This works fine for the gnu_debuglink case, as then the rest of the machinery will match the files based on their UUIDs. However, it does not work for the general case without the debug link, as then the files will end up with two different "UUID"s (because they are based on the file content), and everything will consider them as different.

The solution that would seem best to me here would be to leave the UUID of files which don't have a build-id as empty, as the gnu_debuglink is not really a UUID -- it's main function is to contain a file path, and the crc just acts as a sanity check. This should make the logic for matching two uuid-less files saner, but it would require us to implement the gnu_debuglink method in a different way (which I think is fine, as the current way seems pretty hacky, and causes us to often compute crcs of large files needlessly). I have no idea how much of our existing code would blow up if we started having files without a uuid though.

What do you think about that?

The UUID is assumed to be a unique identifier that isn't based on the entire file contents, but on the content the the important bits (.text, .data, etc). On Mac, the mach-o files have a LC_UUID which is generated by carefully doing and MD5 checksum on only the bits that don't change due to debug info or strings from debug info. This means a binary that is built in /tmp/a or in /tmp/b with debug info will result in the same UUID. When we create a stand alone debug info file, this UUID gets copied into it.

One other thing we could try is to add a type to the lldb_private::UUID class that specifies the type of the UUID:

namespace lldb_private {
class UUID {
public:
  enum Type {
    eTypeBuildID,
    eTypeFileCRC, // CRC of the entire file contents
    eTypeDebugInfoCRC, // CRC of the debug info
  };
};

If I understood your comments above, one file would have a UUID::eTypeBuildID type and the other would have either a UUID::eTypeFileCRC or UUID::eTypeDebugInfoCRC. Then we teach the ObjectFile subclasses to properly set the UUID type. When comparing UUID values, we then could note that the types are not the same and do something more intelligent. Like when adding a symbol file, we could ignore the UUID mismatch if the UUID::Type doesn't match and just go on filename. But we would honor the mismatch if the UUID::Type was the same.

[eugene]

In D35607#814982, @clayborg wrote:

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

I like this, and I'm going to implement it. Pavel is right and the way UUID is set for modules without build-id is kinda strange, but changing it seems like a big and risky change.
Actually "target symbols add" already has logic that if UUID doesn't match it solely uses filenames to match debug symbols, so being able to separately set name for both executable and debug modules seems very logical.

[clayborg]

One other idea if we go with the UUID::Type solution: if we have UUIDs that are different, we could ask the ObjectFile (ObjectFileELF) to see if it thinks the files go together. Not sure if there is anything in the ELF file that would allow us to do that. In Mach-O we have all definitions for the .text and .data sections, just not the bytes. So it would be easy to compare the file address and file sizes of the sections that are in both to see if they match. Something like:

bool ObjectFile::FilesAreRelated(ObjectFile &rhs);

In Mach-O it could run through the segments and compare the ones in common to ensure things match.

[eugene]

Found a way to implement what I need much easily and switched to using file names instead of UUID.

Please take another look.

[clayborg]

Looks fine, just add an error that checks that when the --file options is used, that only one argument (symbol file) is specified.

source/Commands/CommandObjectTarget.cpp

4308–4311

We should verify that if the file option is set, then there is only one symbol file argument as it wouldn't make sense to do:

(lldb) target symbols add -- file a.out a.out.symbols b.out.symbols

[eugene]

Added error message when more than one symbol file is provided.

[eugene]

Greg, are you with me checking this in?

[clayborg]

Greg, are you with me checking this in?

Sure thing

packages/Python/lldbsuite/test/linux/add-symbols/Makefile

+all: clean
+  mkdir debug_binaries
+  $(CC) -g main.c
+  cp a.out debug_binaries
+  strip --strip-debug --remove-section=.note.gnu.build-id a.out
+
+clean:
+  rm -rf a.out debug_binaries

packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py

+""" Testing explicit symbol loading via target symbols add. """
+import os
+import time
+import lldb
+import sys
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetSymbolsAddCommand(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+        self.source = 'main.c'
+
+    @no_debug_info_test  # Prevent the genaration of the dwarf version of this test
+    @skipUnlessPlatform(['linux'])
+    def test_target_symbols_add(self):
+        """Test that 'target symbols add' can load the symbols
+        even if gnu.build-id and gnu_debuglink are not present in the module.
+        Similar to test_add_dsym_mid_execution test for macos."""
+        self.build(clean=True)
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        self.target = self.dbg.CreateTarget(exe)
+        self.assertTrue(self.target, VALID_TARGET)
+
+        main_bp = self.target.BreakpointCreateByName("main", "a.out")
+        self.assertTrue(main_bp, VALID_BREAKPOINT)
+
+        self.process = self.target.LaunchSimple(
+            None, None, self.get_process_working_directory())
+        self.assertTrue(self.process, PROCESS_IS_VALID)
+
+        # The stop reason of the thread should be breakpoint.
+        self.assertTrue(self.process.GetState() == lldb.eStateStopped,
+                        STOPPED_DUE_TO_BREAKPOINT)
+
+        exe_module = self.target.GetModuleAtIndex(0)
+        uuid = exe_module.GetUUIDString()
+
+        # Check that symbols are not loaded and main.c is not know to be
+        # the source file.
+        self.expect("frame select", substrs=['main.c'], matching=False)
+
+        self.runCmd("target symbols add -u %s -s debug_binaries/a.out" % uuid)
+
+        # Check that symbols are now loaded and main.c is in the output.
+        self.expect("frame select", substrs=['main.c'])

packages/Python/lldbsuite/test/linux/add-symbols/main.c

+#include <stdio.h>
+static int var = 5;
+int main() {
+  printf("%p is %d\n", &var, var);
+  return ++var;
+}

source/Commands/CommandObjectTarget.cpp

if (argc == 0) {
               error_set = true;
             }
           } else {
             result.AppendError(
                 "a process must exist in order to use the --frame option");
             error_set = true;
           }
         } else {
+          if (file_option_set) {
+            module_spec.GetFileSpec() =
+                m_file_option.GetOptionValue().GetCurrentValue();
             if (uuid_option_set) {
               module_spec.GetUUID() =
                   m_uuid_option_group.GetOptionValue().GetCurrentValue();
-            success |= module_spec.GetUUID().IsValid();
-          } else if (file_option_set) {
-            module_spec.GetFileSpec() =
-                m_file_option.GetOptionValue().GetCurrentValue();
+              module_spec.GetArchitecture() = target->GetArchitecture();
+            } else {
               ModuleSP module_sp(
                   target->GetImages().FindFirstModule(module_spec));
               if (module_sp) {
                 module_spec.GetFileSpec() = module_sp->GetFileSpec();
                 module_spec.GetPlatformFileSpec() =
                     module_sp->GetPlatformFileSpec();
                 module_spec.GetUUID() = module_sp->GetUUID();
                 module_spec.GetArchitecture() = module_sp->GetArchitecture();
-            } else {
-              module_spec.GetArchitecture() = target->GetArchitecture();
+              }
             }
             success |= module_spec.GetUUID().IsValid() ||
                        module_spec.GetFileSpec().Exists();
+          } else if (uuid_option_set) {
+            module_spec.GetUUID() =
+                m_uuid_option_group.GetOptionValue().GetCurrentValue();
+            success |= module_spec.GetUUID().IsValid();
           }
         }
 
         if (success) {
           if (Symbols::DownloadObjectAndSymbolFile(module_spec)) {
             if (module_spec.GetSymbolFileSpec())
               success = AddModuleSymbols(target, module_spec, flush, result);
           }
if (argc == 0) {
         result.AppendError("specify either one or more paths to symbol files "
                            "or use the --frame option without arguments");
       } else {
         PlatformSP platform_sp(target->GetPlatform());
 
         for (auto &entry : args.entries()) {
           if (!entry.ref.empty()) {
             module_spec.GetSymbolFileSpec().SetFile(entry.ref, true);
             if (platform_sp) {
               FileSpec symfile_spec;
               if (platform_sp
                       ->ResolveSymbolFile(*target, module_spec, symfile_spec)
                       .Success())
                 module_spec.GetSymbolFileSpec() = symfile_spec;
             }
 
             ArchSpec arch;
             bool symfile_exists = module_spec.GetSymbolFileSpec().Exists();
 
             if (symfile_exists) {

source/Host/common/Symbols.cpp

FileSpec Symbols::FindSymbolFileInBundle(const FileSpec &symfile_bundle,
                                          const lldb_private::UUID *uuid,
                                          const ArchSpec *arch) {
   // FIXME
   return FileSpec();
 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
                                           bool force_lookup) {
-  // Fill in the module_spec.GetFileSpec() for the object file and/or the
-  // module_spec.GetSymbolFileSpec() for the debug symbols file.
+  // Unlike on Apple there is no smart way to download symbol files.
+  // Simply try using module file spec if it is specified and exists.
+  if (module_spec.GetFileSpec() && module_spec.GetFileSpec().Exists()) {
+    module_spec.GetSymbolFileSpec() = module_spec.GetFileSpec();
+    return true;
+  }
   return false;
 }
 
 #endif