noexternal 2/2: symbols.enable-external-lookup=false on all hosts (not just OSX)

[jankratochvil]

Summary

There is already in use:

lit/lit-lldb-init:settings set symbols.enable-external-lookup false
packages/Python/lldbsuite/test/lldbtest.py:        self.runCmd('settings set symbols.enable-external-lookup false')

But those are not in effect during MI part of the testsuite. Another problem is that symbols.enable-external-lookup (read by GetEnableExternalLookup) has been currently read only by LocateMacOSXFilesUsingDebugSymbols and therefore it had no effect on Linux.
On Red Hat platforms (Fedoras, RHEL-7) there is DWZ in use and so MiSyntaxTestCase-test_lldbmi_output_grammar FAILs due to:

AssertionError: error: inconsistent pattern ''^.+?\n'' for state 0x5f (matched string: warning: (x86_64) /lib64/libstdc++.so.6 unsupported DW_FORM values: 0x1f20 0x1f21

It is the only testcase with this error. It happens due to:

(lldb) target create "/lib64/libstdc++.so.6"
Current executable set to '/lib64/libstdc++.so.6' (x86_64).
(lldb) b main
warning: (x86_64) /lib64/libstdc++.so.6 unsupported DW_FORM values: 0x1f20 0x1f21
Breakpoint 1: no locations (pending).
WARNING:  Unable to resolve breakpoint to any actual locations.

which happens only with gcc-base-debuginfo rpm installed (similarly for other packages).
It should also speed up the testsuite as it no longer needs to read /usr/lib/debug symbols which have no effect (and should not have any effect) on the testsuite results.

[labath]

I think this fits well in the spirit in which the enable-external-lookup setting was introduced (i.e., to reduce test environment dependencies on mac), but the letter is somewhat questionable. The setting is currently described as "Control the use of external tools or libraries to locate symbol files.", which doesn't sound quite like what is happening here. I think this description rephrased to something more generic so that it covers this use case too. I'm not sure exactly how though. Maybe some blurb about not using "external resources" for symbols and then explaining that we consider /usr/lib/debug to be an external resource? Or maybe just outright confess that this does whatever it takes to make the lldb symbol lookup more predictable/hermetic?

+Adrian and Greg to see what they think about this.

source/Host/common/Symbols.cpp

365

This doesn't sound right to me. It looks like this will prevent LocateDSYMInVincinityOfExecutable, which (I would expect) is necessary to find dsym bundles for all of our dsym tests.

[jankratochvil]

source/Host/common/Symbols.cpp

365

I have regression tested it only on Fedora 29 x86_64. I see now it may regress OSX but I have no idea what really dsym is. I do not have OSX available, is it accessible somewhere remotely for LLVM development (such as is GCC Compile Farm)?

[labath]

I have regression tested it only on Fedora 29 x86_64. I see now it may regress OSX but I have no idea what really dsym is.

dsym is a apple-specific format for external storage of debug info. I think the closest standard equivalent would be a DWP bundle, but there some differences too (and dsym is much older).

I do not have OSX available, is it accessible somewhere remotely for LLVM development (such as is GCC Compile Farm)?

Unfortunately, I don't think llvm has anything like that, though I think it would be extremely useful (/me looks at apple folks). If you try hard enough, you should be able to get clang to produce a dsym bundle for you even on linux. This did the trick for me:

$ cat /tmp/a.c 
void start() asm("start");
void dyld_stub_binder() asm("dyld_stub_binder");

void start() {}
void dyld_stub_binder() {}
$ bin/clang --target=x86_64-apple-darwin --debug /tmp/a.c -o /tmp/a.out -fuse-ld=lld -nostdlib
ld64.lld: warning: -sdk_version is required when emitting min version load command.  Setting sdk version to match provided min version
$ ls -l /tmp/a.out.dSYM/Contents/Resources/DWARF
total 12
-rw-rw---- 1 pavel pavel 8852 Dec 19 10:24 a.out

[friss]

Unfortunately, I don't think llvm has anything like that, though I think it would be extremely useful (/me looks at apple folks). If you try hard enough, you should be able to get clang to produce a dsym bundle for you even on linux. This did the trick for me:

$ cat /tmp/a.c 
void start() asm("start");
void dyld_stub_binder() asm("dyld_stub_binder");

void start() {}
void dyld_stub_binder() {}
$ bin/clang --target=x86_64-apple-darwin --debug /tmp/a.c -o /tmp/a.out -fuse-ld=lld -nostdlib
ld64.lld: warning: -sdk_version is required when emitting min version load command.  Setting sdk version to match provided min version
$ ls -l /tmp/a.out.dSYM/Contents/Resources/DWARF
total 12
-rw-rw---- 1 pavel pavel 8852 Dec 19 10:24 a.out

Pretty sure this won't work. It might generate a dSYM, but an empty one. dsymutil relies on the linker leaving breadcrumbs in the executable to be able to link the DWARF and AFAIK lld doesn't do this.

[friss]

source/Host/common/Symbols.cpp

365

I would just remove that early exit. The rest of the code shouldn't impact the systems you care about.

[jankratochvil]

In D55859#1336351, @friss wrote:

Pretty sure this won't work. It might generate a dSYM, but an empty one.

I agree LLDB did not parse such DSYM symbols on Linux.

Is the patch OK for check-in now? Thanks.

source/Host/common/Symbols.cpp

365

OK, yes, I have removed it.

[labath]

In D55859#1336470, @jankratochvil wrote:

Is the patch OK for check-in now? Thanks.

I'd still like the update the description of the setting controlling this. As it stands now, I don't think it accurately describes what's happening here.

[jankratochvil]

Updated symbols.enable-external-lookup description in detail.

[labath]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

My main issue here was with the "tools or libraries" part of this description -- we're not using any special tools or libraries when looking in /usr/lib/debug.

Maybe just say that this is controls whether we use "external sources" when searching for symbols. The rest of the description is fine (though I would just drop the ifdefs and print everything everywhere).

[jankratochvil]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

The Spotlight on OSX was a bit unclear to me what it really does. Doesn't it use some those "tools or libraries" to access the dSYM files downloaded from internet? But I have put there the "sources" word now as you wish.
Also rather added See also symbols.enable-external-lookup. to target.debug-file-search-paths.
I will check it in if there are no more replies in some time. Thanks for the approval.

[aprantl]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

The original wording is meant for tools that download debug symbols from centralized repositories like the dsym download scripts mentioned on http://lldb.llvm.org/symbols.html + the Spotlight metadata search engine.

The wording "sources" is problematic because it could be confused with source code. Does "/usr/lib..." count as an external library? Alternatively, what about "tools and repositories"?

[jankratochvil]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

GDB calls "/usr/lib/debug" as "debug-file-directory". I definitely would not call it a "library". Would be OK for everyone to use "external tools or directories"?

[labath]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

Both "repositories" and "directories" sound fine to me. Another option might be to use something really generic like "facilities"?

[jankratochvil]

source/Core/ModuleList.cpp

72 ↗

(On Diff #179028)

It is now changed to "external tools and repositories" which should satisfy everyone by: rL350479

packages/Python/lldbsuite/test/tools/lldb-mi/lldbmi_testcase.py

def tearDown(self):
         if self.TraceOn():
             print("\n\nContents of %s:" % self.mylog)
             try:
                 print(open(self.mylog, "r").read())
             except IOError:
                 pass
         Base.tearDown(self)
 
-    def spawnLldbMi(self, exe=None, args=None):
+    def spawnLldbMi(self, exe=None, args=None, preconfig=True):
         import pexpect
         self.child = pexpect.spawn("%s --interpreter %s" % (
             self.lldbMiExec, args if args else ""), cwd=self.getBuildDir())
         self.child.setecho(True)
         self.mylog = self.getBuildArtifact("child.log")
         self.child.logfile_read = open(self.mylog, "w")
         # wait until lldb-mi has started up and is ready to go
         self.expect(self.child_prompt, exactly=True)
+        if preconfig:
+            self.runCmd("settings set symbols.enable-external-lookup false")
+            self.expect("\^done")
+            self.expect(self.child_prompt, exactly=True)
         if exe:
             self.runCmd("-file-exec-and-symbols \"%s\"" % exe)
             # Testcases expect to be able to match output of this command,
             # see test_lldbmi_specialchars.
 
     def runCmd(self, cmd):
         self.child.sendline(cmd)
 
     def expect(self, pattern, exactly=False, *args, **kwargs):
         if exactly:
             return self.child.expect_exact(pattern, *args, **kwargs)
         return self.child.expect(pattern, *args, **kwargs)

packages/Python/lldbsuite/test/tools/lldb-mi/startup_options/TestMiStartupOptions.py

class MiStartupOptionsTestCase(lldbmi_testcase.MiTestCaseBase):
     @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
     @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races
     @skipIfDarwin
     def test_lldbmi_source_option_start_script_error(self):
         """Test that 'lldb-mi --interpreter' stops execution of initial commands in case of error."""
 
         # Prepared source file
         sourceFile = self.copyScript("start_script_error")
-        self.spawnLldbMi(args="--source %s" % sourceFile)
+        self.spawnLldbMi(args="--source %s" % sourceFile, preconfig=False)
+
+        # After 'settings set symbols.enable-external-lookup false'
+        self.expect("settings set symbols.enable-external-lookup false")
+        self.expect("\^done")
 
         # After '-file-exec-and-symbols a.out'
         self.expect("-file-exec-and-symbols %s" % self.myexe)
         self.expect("\^done")
 
         # After '-break-ins -f main'
         self.expect("-break-ins -f main")
         self.expect("\^error")

packages/Python/lldbsuite/test/tools/lldb-mi/startup_options/start_script_error

+settings set symbols.enable-external-lookup false
 -file-exec-and-symbols a.out
 -break-ins -f main

source/Host/common/Symbols.cpp

 }
 
 FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec) {
   FileSpec symbol_file_spec = module_spec.GetSymbolFileSpec();
   if (symbol_file_spec.IsAbsolute() &&
       FileSystem::Instance().Exists(symbol_file_spec))
     return symbol_file_spec;
 
+  bool external_lookup =
+      ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup();
+
   const char *symbol_filename = symbol_file_spec.GetFilename().AsCString();
   if (symbol_filename && symbol_filename[0]) {
     FileSpecList debug_file_search_paths(
         Target::GetDefaultDebugFileSearchPaths());
 
     // Add module directory.
     FileSpec module_file_spec = module_spec.GetFileSpec();
     // We keep the unresolved pathname if it fails.
     FileSystem::Instance().ResolveSymbolicLink(module_file_spec, module_file_spec);
 
     const ConstString &file_dir = module_file_spec.GetDirectory();
     {
       FileSpec file_spec(file_dir.AsCString("."));
       FileSystem::Instance().Resolve(file_spec);
       debug_file_search_paths.AppendIfUnique(file_spec);
     }
 
+    if (external_lookup) {
       // Add current working directory.
       {
         FileSpec file_spec(".");
         FileSystem::Instance().Resolve(file_spec);
         debug_file_search_paths.AppendIfUnique(file_spec);
       }
 
 #ifndef _WIN32
 #if defined(__NetBSD__)
       // Add /usr/libdata/debug directory.
       {
         FileSpec file_spec("/usr/libdata/debug");
         FileSystem::Instance().Resolve(file_spec);
         debug_file_search_paths.AppendIfUnique(file_spec);
       }
 #else
       // Add /usr/lib/debug directory.
       {
         FileSpec file_spec("/usr/lib/debug");
         FileSystem::Instance().Resolve(file_spec);
         debug_file_search_paths.AppendIfUnique(file_spec);
       }
 #endif
 #endif // _WIN32
+    }
 
     std::string uuid_str;
     const UUID &module_uuid = module_spec.GetUUID();
     if (module_uuid.IsValid()) {
       // Some debug files are stored in the .build-id directory like this:
       //   /usr/lib/debug/.build-id/ff/e7fe727889ad82bb153de2ad065b2189693315.debug
       uuid_str = module_uuid.GetAsString("");
       std::transform(uuid_str.begin(), uuid_str.end(), uuid_str.begin(),
for (size_t idx = 0; idx < num_directories; ++idx) {
                 return file_spec;
             }
           }
         }
       }
     }
   }
 
   return LocateExecutableSymbolFileDsym(module_spec);
 }
 
 #if !defined(__APPLE__)
 
 FileSpec Symbols::FindSymbolFileInBundle(const FileSpec &symfile_bundle,
                                          const lldb_private::UUID *uuid,
                                          const ArchSpec *arch) {
   // FIXME