LLDB does not respect platform sysroot when loading core on Linux

[EugeneBi]

Summary

The situation arises when we are trying to load a core dump which was generated on a different machine
running a different version of Linux. The problem is that shared libraries differ on these machines and
LLDB either fails to load some libraries or loads wrong ones.

How it should work:

1. We copy the directory tree with libraries from target machine into some local directory
2. in LLDB, create a platform setting sysroot to that directory
3. Create target using that platform

The fix is to pass additional "sysroot" argument from lldb_private::Platform::GetSharedModule to
lldb_private::ModuleList::GetSharedModule

[labath]

Which version is this patch based on? The line numbers don't seem to match what I see on master.

Could you rebase the patch to master, and upload the patch with a full diff (e.g. via git diff -U9999, see https://llvm.org/docs/Phabricator.html#id3).

include/lldb/Core/ModuleList.h

545 ↗

(On Diff #156833)

Please make this an llvm::StringRef (and then change AsCString to GetStringRef below).

[clayborg]

I think doing this in the module list is not the right place. Why? Some platforms might have multiple sysroot to check. iOS for instance has a directory for each device that Xcode has connected to which can be checked. I am fine with adding this ability to lldb_private::Platform, but I would just do it in there. Try GetRemoteSharedModule with the spec, if it fails, try again after modifying the spec to prepend the sysroot path. Possible even just check the sysroot path + path first if m_sdk_sysroot is filled in. I don't see the need to change ModuleList itself.

include/lldb/Core/ModuleList.h

544–545 ↗

(On Diff #156833)

Revert this? See my main comment

source/Core/ModuleList.cpp

710–714 ↗

(On Diff #156833)

Revert

766–770 ↗

(On Diff #156833)

Revert

source/Target/Platform.cpp

228

Revert

230

Here just make a module spec that has m_sdk_sysroot prepended if m_sdk_sysroot is set, and check for that file first. If that succeeds, return that, else do the same code as before.

[lemo]

The problem is that shared libraries differ on these machines and
LLDB either fails to load some libraries *or loads wrong ones*.

Not finding the modules is not surprising but the latter (loading the wrong
modules) is a bit concerning. Do you know why the module build-id is not
used when searching for the local binary?

[clayborg]

We should never be loading the wrong shared libraries. The module spec we fill out must contain the UUID of the file are looking for. If it doesn't we have no chance of every really loading the right stuff.

[EugeneBi]

In D49685#1173640, @clayborg wrote:

We should never be loading the wrong shared libraries. The module spec we fill out must contain the UUID of the file are looking for. If it doesn't we have no chance of every really loading the right stuff.

Well, that's what it does on my machine. So prepending sysroot *after* trying to load module without it will cause problems to me. Also, I assume that if you specified sysroot for a platform, we should not try to load solibs without it - these paths are just not a part of the platform.

I think doing this in the module list is not the right place. Why? Some platforms might have multiple sysroot to check. iOS for instance has a directory for each device that Xcode has connected to which can be checked. I am fine with adding this ability to> lldb_private::Platform, but I would just do it in there. Try GetRemoteSharedModule with the spec, if it fails, try again after modifying the spec to prepend the sysroot path. Possible even just check the sysroot path + path first if m_sdk_sysroot is filled in. I don't see the need to change ModuleList itself.

I do not see how this prepend sysroot could be done in the platform... Essentially, my fix is doing what you suggest: the platform supplies sysroot to the module list, and two different platforms (for two XCode devices, etc.) would supply different sysroots. What do I miss?

[clayborg]

In D49685#1173720, @EugeneBi wrote:

In D49685#1173640, @clayborg wrote:

We should never be loading the wrong shared libraries. The module spec we fill out must contain the UUID of the file are looking for. If it doesn't we have no chance of every really loading the right stuff.

Well, that's what it does on my machine. So prepending sysroot *after* trying to load module without it will cause problems to me. Also, I assume that if you specified sysroot for a platform, we should not try to load solibs without it - these paths are just not a part of the platform.

I think doing this in the module list is not the right place. Why? Some platforms might have multiple sysroot to check. iOS for instance has a directory for each device that Xcode has connected to which can be checked. I am fine with adding this ability to> lldb_private::Platform, but I would just do it in there. Try GetRemoteSharedModule with the spec, if it fails, try again after modifying the spec to prepend the sysroot path. Possible even just check the sysroot path + path first if m_sdk_sysroot is filled in. I don't see the need to change ModuleList itself.

I do not see how this prepend sysroot could be done in the platform... Essentially, my fix is doing what you suggest: the platform supplies sysroot to the module list, and two different platforms (for two XCode devices, etc.) would supply different sysroots. What do I miss?

You would just move:

auto resolved_module_spec(module_spec);
  if (sysroot != nullptr)
    resolved_module_spec.GetFileSpec().PrependPathComponent(sysroot);

into the code in Platform.cpp and do it all there.

[EugeneBi]

Rebased to recent master.
Included the whole file in diff.

[EugeneBi]

This comment was removed by EugeneBi.

[EugeneBi]

You would just move:

auto resolved_module_spec(module_spec);
  if (sysroot != nullptr)
    resolved_module_spec.GetFileSpec().PrependPathComponent(sysroot);

into the code in Platform.cpp and do it all there.

Ah, I see. Will do, thanks.

[EugeneBi]

Code review followup:

• Restricted change to Platform.cpp
• Restricted change only to remote platforms.

[clayborg]

This will help, but not fix us loading incorrect versions of the shared library. I wonder if there is anything in the core file that could help use get the build ID/UUID of each binary. I doubt it, but might be worth checking. Linux core files are really useless unless they can be processed with the exact binaries and that is a shame. The windows minidump files that breakpad can create are much better than standard core files since they do contain the information we need (path + version + UUID) and they are very similar to core files but just don't always save the same amount of memory (just the thread stacks, not all mapped memory), though that can easily be fixed.

[EugeneBi]

source/Target/Platform.cpp

252

A bug here. must be resolved_spec if it succeeds.

[EugeneBi]

Fix a bug with resolved_spec path.

[EugeneBi]

source/Target/Platform.cpp

252

Now I have my doubts: what "platform file spec" really means? I mean, which code
is correct: current or previous one?

[labath]

Could you also add a test for this?

[EugeneBi]

In D49685#1174770, @labath wrote:

Could you also add a test for this?

I never ran LLDB tests, not sure where they are and what they are.
Also, how would you test that? I know now my open core dump works, but I cannot share it.

[labath]

In D49685#1175413, @EugeneBi wrote:

In D49685#1174770, @labath wrote:

Could you also add a test for this?

I never ran LLDB tests, not sure where they are and what they are.
Also, how would you test that? I know now my open core dump works, but I cannot share it.

Good question. Is this functionality specific to shared libraries, or could it be used to locate the main exe too?

If yes, then it should be sufficient to take one of the existing core&exe files we have, copy the exe into a path matching what the core file expects, set the sysroot, and make sure the exe gets found when we open up the core file. You can look at the existing tests in test/testcases/functionalities/postmortem/elf-core for examples.

If not, then it might get trickier because we'll need new core files, and it's hard to generate small ones with shared libraries.

[EugeneBi]

In D49685#1176399, @labath wrote:

In D49685#1175413, @EugeneBi wrote:

In D49685#1174770, @labath wrote:

Could you also add a test for this?

I never ran LLDB tests, not sure where they are and what they are.
Also, how would you test that? I know now my open core dump works, but I cannot share it.

Good question. Is this functionality specific to shared libraries, or could it be used to locate the main exe too?

If yes, then it should be sufficient to take one of the existing core&exe files we have, copy the exe into a path matching what the core file expects, set the sysroot, and make sure the exe gets found when we open up the core file. You can look at the existing tests in test/testcases/functionalities/postmortem/elf-core for examples.

If not, then it might get trickier because we'll need new core files, and it's hard to generate small ones with shared libraries.

It is specific to shared libraries. Opening the executable and core dump follows different code path.

[labath]

In D49685#1177046, @EugeneBi wrote:

It is specific to shared libraries. Opening the executable and core dump follows different code path.

Which path is that? Is the path different even when you don't specify an executable when opening the core file (target create --core /my/file.core). I believe that in this case we should end up here https://github.com/llvm-mirror/lldb/blob/7c07b8a9314eef118f95b57b49fa099be0808eac/source/Plugins/Process/elf-core/ProcessElfCore.cpp#L255, which should eventually call Platform::GetSharedModule. Is that not the case?

[EugeneBi]

In D49685#1178125, @labath wrote:

In D49685#1177046, @EugeneBi wrote:

It is specific to shared libraries. Opening the executable and core dump follows different code path.

Which path is that? Is the path different even when you don't specify an executable when opening the core file (target create --core /my/file.core). I believe that in this case we should end up here https://github.com/llvm-mirror/lldb/blob/7c07b8a9314eef118f95b57b49fa099be0808eac/source/Plugins/Process/elf-core/ProcessElfCore.cpp#L255, which should eventually call Platform::GetSharedModule. Is that not the case?

I did not try not to specify executable - what's the point in doing that? LLDB would fail to find symbols.

Here is the stack I captured when I debugged the problem. I believe that DynamicLoaderPOSIXDYLD::LoadAllCurrentModules did not try to load the executable.

(gdb) bt
#0 lldb_private::ModuleList::GetSharedModule (module_spec=..., module_sp=std::shared_ptr (empty) 0x0,

module_search_paths_ptr=0x83ad60, old_module_sp_ptr=0x7fffffffbb50, did_create_ptr=0x7fffffffbb07,
always_create=false) at /home/eugene/llvm/tools/lldb/source/Core/ModuleList.cpp:710

#1 0x00007fffedc2d130 in lldb_private::Platform::<lambda(const lldb_private::ModuleSpec&)>::operator()(const lldb_private::ModuleSpec &) const (__closure=0x8581b0, spec=...)

at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:234

#2 0x00007fffedc34ff2 in std::_Function_handler<lldb_private::Status(const lldb_private::ModuleSpec&), lldb_private::Platform::GetSharedModule(const lldb_private::ModuleSpec&, lldb_private::Process*, lldb::ModuleSP&, const lldb_private::FileSpecList*, lldb::ModuleSP*, bool*)::<lambda(const lldb_private::ModuleSpec&)> >::_M_invoke(const std::_Any_data &, const lldb_private::ModuleSpec &) (functor=..., args#0=...) at /usr/include/c++/5/functional:1857
#3 0x00007fffedc37978 in std::function<lldb_private::Status (lldb_private::ModuleSpec const&)>::operator()(lldb_private::ModuleSpec const&) const (this=0x7fffffffba80, __args#0=...) at /usr/include/c++/5/functional:2267
#4 0x00007fffedc3375a in lldb_private::Platform::GetRemoteSharedModule(lldb_private::ModuleSpec const&, lldb_private::Process*, std::shared_ptr<lldb_private::Module>&, std::function<lldb_private::Status (lldb_private::ModuleSpec const&)> const&, bool*) (this=0x839330, module_spec=..., process=0x84d310, module_sp=std::shared_ptr (empty) 0x0,

module_resolver=..., did_create_ptr=0x7fffffffbb07)
at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:1628

#5 0x00007fffedc2d2cd in lldb_private::Platform::GetSharedModule (this=0x839330, module_spec=...,

process=0x84d310, module_sp=std::shared_ptr (empty) 0x0, module_search_paths_ptr=0x83ad60,
old_module_sp_ptr=0x7fffffffbb50, did_create_ptr=0x7fffffffbb07)
at /home/eugene/llvm/tools/lldb/source/Target/Platform.cpp:240

#6 0x00007fffedc9957c in lldb_private::Target::GetSharedModule (this=0x846960, module_spec=..., error_ptr=0x0)

at /home/eugene/llvm/tools/lldb/source/Target/Target.cpp:1952

#7 0x00007fffef8e0d11 in lldb_private::DynamicLoader::LoadModuleAtAddress (this=0x9a0a70, file=...,

link_map_addr=139700267943784, base_addr=139700263510016, base_addr_is_offset=true)
at /home/eugene/llvm/tools/lldb/source/Core/DynamicLoader.cpp:171

#8 0x00007fffedd8fb55 in DynamicLoaderPOSIXDYLD::LoadAllCurrentModules (this=0x9a0a70)

at /home/eugene/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:537

#9 0x00007fffedd8de52 in DynamicLoaderPOSIXDYLD::DidAttach (this=0x9a0a70)

at /home/eugene/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:171

#10 0x00007fffedc476d9 in lldb_private::Process::LoadCore (this=0x84d310)

at /home/eugene/llvm/tools/lldb/source/Target/Process.cpp:2853

[EugeneBi]

Hmm... I never thought I can do that :)
Anyway, with the change as it is now, LLDB would try to load executable in the sysroot, fail that, then open it without the sysroot.

[lemo]

I never *ran LLDB tests*, not sure where they are and what they are.

I hope you're planning to look into this before submitting the change :)

[labath]

In D49685#1178528, @EugeneBi wrote:

Hmm... I never thought I can do that :)
Anyway, with the change as it is now, LLDB would try to load executable in the sysroot, fail that, then open it without the sysroot.

Does that mean it is possible to exercise this code path (by placing the executable in the sysroot for lldb to find it)?

[EugeneBi]

In D49685#1178543, @lemo wrote:

I never *ran LLDB tests*, not sure where they are and what they are.

I hope you're planning to look into this before submitting the change :)

Good idea :)

Scanning dependencies of target check-lldb
[100%] Built target check-lldb

[EugeneBi]

In D49685#1178553, @labath wrote:

In D49685#1178528, @EugeneBi wrote:

Hmm... I never thought I can do that :)
Anyway, with the change as it is now, LLDB would try to load executable in the sysroot, fail that, then open it without the sysroot.

Does that mean it is possible to exercise this code path (by placing the executable in the sysroot for lldb to find it)?

I looked at the tests - is it all in Python? Not sure I have time to learn a new language... Is there anything in C++?

[labath]

In D49685#1178730, @EugeneBi wrote:

I looked at the tests - is it all in Python? Not sure I have time to learn a new language... Is there anything in C++?

We have unit tests in c++, but it's going to be quite hard to tickle this code path from there.

FWIW, I don't think you really need to *know* python to write a test like this. You should be able to fudge it by cargo-culting some code from existing tests and some basic python examples. I expect the test should be something like:

# copy core file an .exe into an appropriate directory tree
self.runCmd("platform select remote-linux --sysroot '%s'" % sysroot)
target = self.dbg.CreateTarget(None)
process = target.LoadCode(core)
self.assertEquals(1, target.GetNumModules())
self.assertEquals(exe, target.GetModuleAtIndex(0).GetFileSpec())

[EugeneBi]

In D49685#1179837, @labath wrote:

In D49685#1178730, @EugeneBi wrote:

I looked at the tests - is it all in Python? Not sure I have time to learn a new language... Is there anything in C++?

We have unit tests in c++, but it's going to be quite hard to tickle this code path from there.

FWIW, I don't think you really need to *know* python to write a test like this. You should be able to fudge it by cargo-culting some code from existing tests and some basic python examples. I expect the test should be something like:

# copy core file an .exe into an appropriate directory tree
self.runCmd("platform select remote-linux --sysroot '%s'" % sysroot)
target = self.dbg.CreateTarget(None)
process = target.LoadCode(core)
self.assertEquals(1, target.GetNumModules())
self.assertEquals(exe, target.GetModuleAtIndex(0).GetFileSpec())

I am going on vacation till the end of August and I still have something to do at work. Most likely I will not have a chance to look at it until I am back.

[magardne]

@labath Eugene asked me to help add a unit test for this. I have the updated diff, but I can't seem to attach it to this code review -- it must be because I'm not the original author? I'll attach the diff to this comment and maybe you can try to update the review. Otherwise we can wait until Eugene gets back on Monday.

The diff makes the following changes:

• Add an executable: test/testcases/functionalities/postmortem/elf-core/mock_sysroot/home/labath/test/a.out
• This is a copy of linux-i386.out (which is why it's in mock_sysroot/home/labath...). Let me know if this is ok, or if I should try to create a new binary/core
• Implement the test you suggested: set platform and sysroot, open the core, check threads/stacks are valid

I verified that the test fails without the changes to Platform.cpp and passes after the changes.

sysroot_cr.diff84 KBDownload

[EugeneBi]

In D49685#1201351, @magardne wrote:

@labath Eugene asked me to help add a unit test for this. I have the updated diff, but I can't seem to attach it to this code review -- it must be because I'm not the original author? I'll attach the diff to this comment and maybe you can try to update the review. Otherwise we can wait until Eugene gets back on Monday.

The diff makes the following changes:

• Add an executable: test/testcases/functionalities/postmortem/elf-core/mock_sysroot/home/labath/test/a.out
• This is a copy of linux-i386.out (which is why it's in mock_sysroot/home/labath...). Let me know if this is ok, or if I should try to create a new binary/core
• Implement the test you suggested: set platform and sysroot, open the core, check threads/stacks are valid

I verified that the test fails without the changes to Platform.cpp and passes after the changes.

sysroot_cr.diff84 KBDownload

Mark, thanks a lot for doing this. I have one request though: let's not add new binaries to the repo. Instead, the test should create a temporary directory somewhere (in /tmp, I guess) and copy necessary files there. After the test is done, wipe it out.

[clayborg]

You can always run a.out through obj2yaml and check that in. There is test suite support for using a yaml file that converts it to a binary and debugs it

functionalities/gdb_remote_client/gdbclientutils.py
429:    def createTarget(self, yaml_path):
431:        Create a target by auto-generating the object based on the given yaml
437:        yaml_base, ext = os.path.splitext(yaml_path)
438:        obj_path = self.getBuildArtifact(yaml_base)
439:        self.yaml2obj(yaml_path, obj_path)

[EugeneBi]

Mark added the test. Please let us know if this is OK.

[labath]

Thank you for writing the test. This is more-or-less what I had in mind. Just make sure we don't put the test files in /tmp (e.g., because windows doesn't have it).

It's unfortunate that the core file has my username embedded in it (I should have build this in /tmp or somewhere), but if that doesn't bother you then I'm fine with it too (you can find leftover usernames in some llvm tests too, although, with this test, this becomes a bit more apparent). Alternatively, we can regenerate the core file and the matching executable. (We could use yaml2obj on the executable, but it unfortunately doesn't support (elf) core files at the moment).

packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py

215 ↗

(On Diff #161520)

Please put this in the build folder (self.getBuildDir) instead of /tmp.

231–232 ↗

(On Diff #161520)

Then, this will not be necessary.

[EugeneBi]

Do not use /tmp directory in the test

[EugeneBi]

packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py

215 ↗

(On Diff #161520)

I uploaded new diff and closed revision.
Do I need to do anything else?

[labath]

packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py

215 ↗

(On Diff #161520)

Normally, the revision would be automatically closed when the patch is committed. Do you have commit access or you need someone to commit this for you?

[EugeneBi]

packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/TestLinuxCore.py

215 ↗

(On Diff #161520)

I do not have commit access.
Could you commit it, please?

source/Target/Platform.cpp

 
 Status Platform::GetSharedModule(const ModuleSpec &module_spec,
                                  Process *process, ModuleSP &module_sp,
                                  const FileSpecList *module_search_paths_ptr,
                                  ModuleSP *old_module_sp_ptr,
                                  bool *did_create_ptr) {
   if (IsHost())
     return ModuleList::GetSharedModule(
         module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
         did_create_ptr, false);
 
-  return GetRemoteSharedModule(module_spec, process, module_sp,
-                               [&](const ModuleSpec &spec) {
-                                 Status error = ModuleList::GetSharedModule(
-                                     spec, module_sp, module_search_paths_ptr,
-                                     old_module_sp_ptr, did_create_ptr, false);
-                                 if (error.Success() && module_sp)
-                                   module_sp->SetPlatformFileSpec(
-                                       spec.GetFileSpec());
-                                 return error;
-                               },
-                               did_create_ptr);
+  // Module resolver lambda.
+  auto resolver = [&](const ModuleSpec &spec) {
+    Status error(eErrorTypeGeneric);
+    ModuleSpec resolved_spec;
+    // Check if we have sysroot set.
+    if (m_sdk_sysroot) {
+      // Prepend sysroot to module spec.
+      resolved_spec = spec;
+      resolved_spec.GetFileSpec().PrependPathComponent(
+        m_sdk_sysroot.GetStringRef());
+      // Try to get shared module with resolved spec.
+      error = ModuleList::GetSharedModule( 
+        resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+        did_create_ptr, false);
+    }
+    // If we don't have sysroot or it didn't work then 
+    // try original module spec.
+    if (!error.Success()) {
+      resolved_spec = spec;
+      error = ModuleList::GetSharedModule(
+        resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr, 
+        did_create_ptr, false);
+    }
+      module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());
+  };
+
+  return GetRemoteSharedModule(module_spec, process, module_sp,
+                               resolver, did_create_ptr);
 }
 
 bool Platform::GetModuleSpec(const FileSpec &module_file_spec,
                              const ArchSpec &arch, ModuleSpec &module_spec) {
   ModuleSpecList module_specs;
   if (ObjectFile::GetModuleSpecifications(module_file_spec, 0, 0,
                                           module_specs) == 0)
     return false;