Page MenuHomePhabricator

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

Authored by jankratochvil on Dec 18 2018, 3:22 PM.

Details

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.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.Dec 18 2018, 3:22 PM

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 added inline comments.Dec 19 2018, 12:56 AM
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)?

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 added a subscriber: friss.Dec 19 2018, 8:13 AM

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 added inline comments.Dec 19 2018, 8:40 AM
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 marked 4 inline comments as done.Dec 19 2018, 10:05 AM

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.

jankratochvil updated this revision to Diff 178911.
jankratochvil marked an inline comment as done.

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.

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

labath accepted this revision.Dec 21 2018, 2:26 AM
labath added inline comments.
source/Core/ModuleList.cpp
71–72

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).

This revision is now accepted and ready to land.Dec 21 2018, 2:26 AM
jankratochvil marked 2 inline comments as done.Dec 21 2018, 2:04 PM
jankratochvil added inline comments.
source/Core/ModuleList.cpp
71–72

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.

This revision was automatically updated to reflect the committed changes.
jankratochvil marked an inline comment as done.
aprantl added inline comments.Thu, Jan 3, 3:37 PM
source/Core/ModuleList.cpp
71–72

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 marked an inline comment as done.Thu, Jan 3, 3:53 PM
jankratochvil added inline comments.
source/Core/ModuleList.cpp
71–72

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 added inline comments.Thu, Jan 3, 11:29 PM
source/Core/ModuleList.cpp
71–72

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

jankratochvil marked an inline comment as done.Sat, Jan 5, 1:45 PM
jankratochvil added inline comments.
source/Core/ModuleList.cpp
71–72

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