Fix "process load/unload" on android

[tberghammer]

Summary

Fix "process load/unload" on android

On android the symbols exposed by libdl (dlopen, dlclose, dlerror)
prefixed by "__dl_". This change moves the handling of process
load/unload to the platform object and override it for android to
handle the special prefix.

[srhines]

include/lldb/Target/Platform.h

872

Some platforms prefix the ... Let those platforms specify ...

[tberghammer]

Improve comment based on review

[tberghammer]

Friendly ping

[sivachandra]

I am not really familiar with this area. OK from my side, but you might want clayborg to take a look at this.

[clayborg]

We should add support to lldb::SBPlatform and lldb_private::Platform where we get the shared library extension from the platform and also fix the logic as noted in the inlined comments.

test/functionalities/load_unload/TestLoadUnload.py

201–204

It would be great to add something to lldb::SBPlatform and lldb_private::Platform that allows us to get the shared library extension from the platform:

class SBPlatform
{
    const char *
    GetSharedLibraryExtension();
};

Then this code can become:

dylibName = "libloadunload" + platform.GetSharedLibraryExtension()

206–209

This code should be:

if lldb.remote_platform:
    dylibPath = os.path.join(shlib_dir, dylibName)
else:
    dylibPath = dylibName

[clayborg]

The other alternative is to have lldb_private::Platform have a new function:

class Platform
{
    virtual uint32_t
    LoadImage (lldb_private::Process *process, const FileSpec &image_spec, Error &error);

    virtual Error
    UnLoadImage (lldb_private::Process *process, uint32_t image_token);
}

Then we move the current code from Process::LoadImage() and Process::UnLoadImage() into PlatformPOSIX, and have new versions for PlatformAndroid.

[clayborg]

The second platform route would probably be better because it would allow for Windows to load/unload images if we ever move to lldb-server on windows.

[tberghammer]

Address review comments

[tberghammer]

test/functionalities/load_unload/TestLoadUnload.py

201–204

It is a fairly unrelated change. Will create a separate CL for it

206–209

Done

[clayborg]

Looks fine.

[jingham]

It seems like an awful lot of logic is duplicated between the POSIX & Android versions of LoadImage when all that really changes is the name of the function you are calling. Is it possible to centralize this some more?

[labath]

In D11465#300581, @jingham wrote:

It seems like an awful lot of logic is duplicated between the POSIX & Android versions of LoadImage when all that really changes is the name of the function you are calling. Is it possible to centralize this some more?

We've been discussing this offline. I believe Tamas is working on resolving that.

[tberghammer]

In D11465#300606, @labath wrote:

In D11465#300581, @jingham wrote:

It seems like an awful lot of logic is duplicated between the POSIX & Android versions of LoadImage when all that really changes is the name of the function you are calling. Is it possible to centralize this some more?

We've been discussing this offline. I believe Tamas is working on resolving that.

I uploaded D15183 to fix the issue

[nitesh.jain]

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

389 ↗

(On Diff #41608)

Hi Tamas,

When I run "process load libloadunload_a.so --install" its fail with error Couldn't lookup symbols: dl_dlerror, dl_dlopen.

Symbol table contains 13 entries:

Num:    Value  Size Type    Bind   Vis      Ndx Name
  0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
  1: 00000304     5 FUNC    GLOBAL DEFAULT    5 android_dlopen_ext
  2: 00000309     5 FUNC    GLOBAL DEFAULT    5 android_get_LD_LIBRARY_PA
  3: 0000030e     5 FUNC    GLOBAL DEFAULT    5 android_update_LD_LIBRARY
  4: 00000313     5 FUNC    GLOBAL DEFAULT    5 dl_iterate_phdr
  5: 00000318     5 FUNC    GLOBAL DEFAULT    5 **dladdr**
  6: 0000031d     5 FUNC    GLOBAL DEFAULT    5 **dlclose**
  7: 00000322     5 FUNC    GLOBAL DEFAULT    5 **dlerror**
  8: 00000327     5 FUNC    GLOBAL DEFAULT    5 **dlopen**
  9: 0000032c     5 FUNC    GLOBAL DEFAULT    5 dlsym
 10: 00002000     0 NOTYPE  GLOBAL DEFAULT  ABS _edata
 11: 00002000     0 NOTYPE  GLOBAL DEFAULT  ABS __bss_start
 12: 00002000     0 NOTYPE  GLOBAL DEFAULT  ABS _end

The symbols are not prefix with "__dl_" . We are using SDK version 25 for MIP64r6 target.

generic_mips64:/ # getprop ro.build.version.sdk
25

Even we are not seeing prefix "__dl_" added to X86-64 dynamic symbol
Symbol table '.dynsym' contains 12 entries:

Num:    Value          Size Type    Bind   Vis      Ndx Name
  0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
  1: 0000000000000442     6 FUNC    GLOBAL DEFAULT    7 dl_iterate_phdr@@LIBC
  2: 000000000000043c     6 FUNC    GLOBAL DEFAULT    7 android_dlopen_ext@@LIBC
  3: 000000000000044e     6 FUNC    GLOBAL DEFAULT    7 dlclose@@LIBC
  4: 0000000000000448     6 FUNC    GLOBAL DEFAULT    7 dladdr@@LIBC
  5: 000000000000045a     6 FUNC    GLOBAL DEFAULT    7 dlopen@@LIBC
  6: 0000000000000454     6 FUNC    GLOBAL DEFAULT    7 dlerror@@LIBC
  7: 0000000000000460     6 FUNC    GLOBAL DEFAULT    7 dlsym@@LIBC
  8: 0000000000002000     0 NOTYPE  GLOBAL DEFAULT  ABS _edata
  9: 0000000000002000     0 NOTYPE  GLOBAL DEFAULT  ABS __bss_start
 10: 0000000000002000     0 NOTYPE  GLOBAL DEFAULT  ABS _end
 11: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LIBC

Symbol table '.symtab' contains 14 entries:

Num:    Value          Size Type    Bind   Vis      Ndx Name
  0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
  1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS tmp-platform.c
  2: 0000000000001ee0   288 OBJECT  LOCAL  HIDDEN    10 _DYNAMIC
  3: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS LIBC
  4: 0000000000000442     6 FUNC    GLOBAL DEFAULT    7 dl_iterate_phdr
  5: 000000000000043c     6 FUNC    GLOBAL DEFAULT    7 android_dlopen_ext
  6: 000000000000044e     6 FUNC    GLOBAL DEFAULT    7 dlclose
  7: 0000000000000448     6 FUNC    GLOBAL DEFAULT    7 dladdr
  8: 000000000000045a     6 FUNC    GLOBAL DEFAULT    7 dlopen

[tberghammer]

Hi Nitesh,

The way dlopen, dlclose and dl_phdr_iterate is implemented on android is pretty strange until (and including) SDK 25. The trick is that these functions are implemented inside "/system/bin/linker" with actual function names prefixed with "__dl_". The linker is doing some magic (creating a fake shared library entry at startup) to resolve these symbols when a library is loaded. This magic isn't understand by LLDB so it tries to lookup the symbols in the linker binary itself based on there actual function names. The actual libdl.so file located on the device isn't used for anything (it is there just for some compatibility reasons).

To help me investigate, can you do the followings and attach the output to this thread:

• Run "target modules list" in LLDB
• Run "target modules lookup -n dlopen" in LLDB
• Run "target modules lookup -n __dl_dlopen" in LLDB
• Download "/system/bin/linker" from the device and dump the ".symtab" section of it

Random guesses for what can cause the problem (will be able to more specific based on the above data):

• LLDB fails to detect or load "/system/bin/linker" in the shared library list
• /system/bin/linker doesn't contain a ".symtab" section (e.g. stripped)
• You are using a version of android what have an API 26 /system/bin/linker what had some changes breaking LLDB (Pavel fixed it but the fix only applies for API 26+)

[labath]

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

389 ↗

(On Diff #41608)

Is it possible you are using a preview version of android sdk? These had the new dlopen symbols even though the official android sdk's still used the __dl_dlopen symbols?

[clayborg]

It would be nice to auto detect the names correctly?

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

389 ↗

(On Diff #41608)

Maybe search for a symbol named "dl_open" first, then fall back to "dl_open" if that symbol is available, and supply as an argument for "expr.Printf(" by adding a "%s" in place of the "dl_dlopen? call? Then it can work for both?

const char *dl_open_func_name = nullptr;
SymbolContextList matching_symbols;
std::vector<const char *> dl_open_names = { "__dl_open", "dl_open" };
const char *dl_open_name = nullptr;
for (auto name: dl_open_names) {
  if (process->GetTarget().GetImages().FindFunctionSymbols(ConstString(name), eFunctionNameTypeFull, matching_symbols))
    dl_open_name = name;
    break;
  }
}

[nitesh.jain]

Hi Tamas,

Run "target modules list"

(lldb) target modules list
[ 0] E6A47987-0000-0000-0000-000000000000 /export/tmp/nin/LLVM/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/multiset/a.out
[ 1] C2298D9F-41E0-1ED4-C9BF-D5B416CBE38F /home/nin/.lldb/module_cache/remote-android/.cache/C2298D9F-41E0-1ED4-C9BF-D5B416CBE38F/libstdc++.so
[ 2] D178E966-9342-8A56-CE34-35C9359886A4 /home/nin/.lldb/module_cache/remote-android/.cache/D178E966-9342-8A56-CE34-35C9359886A4/libm.so
[ 3] 6F226DD0-EFC1-AC63-647D-374F43AB9CC6 /home/nin/.lldb/module_cache/remote-android/.cache/6F226DD0-EFC1-AC63-647D-374F43AB9CC6/libc.so
[ 4] DF63F23A-2C91-BA01-EC7D-7CCA4FA1A55D /home/nin/.lldb/module_cache/remote-android/.cache/DF63F23A-2C91-BA01-EC7D-7CCA4FA1A55D/libdl.so
[ 5] 0466B0A0-00B3-88DA-55F1-1DA6D42D2C36 /home/nin/.lldb/module_cache/remote-android/.cache/0466B0A0-00B3-88DA-55F1-1DA6D42D2C36/ld-android.so
[ 6] 3FD33902-2A3B-6F33-0890-CDF6A434045E /home/nin/.lldb/module_cache/remote-android/.cache/3FD33902-2A3B-6F33-0890-CDF6A434045E/linker64
[ 7] D738489C-460B-25A5-B3B1-0D9BBC7CB8D6 /home/nin/.lldb/module_cache/remote-android/.cache/D738489C-460B-25A5-B3B1-0D9BBC7CB8D6/libnetd_client.so
[ 8] 84239230-A38E-687A-7EC5-FA40CD52D25F /home/nin/.lldb/module_cache/remote-android/.cache/84239230-A38E-687A-7EC5-FA40CD52D25F/libcutils.so
[ 9] 38725D15-41E9-AB20-70A0-16012C5B9DAD /home/nin/.lldb/module_cache/remote-android/.cache/38725D15-41E9-AB20-70A0-16012C5B9DAD/libc++.so
[ 10] 72DDCA8E-D724-7089-17EF-0703F4CA12E4 /home/nin/.lldb/module_cache/remote-android/.cache/72DDCA8E-D724-7089-17EF-0703F4CA12E4/liblog.so

Run "target modules lookup -n dlopen"

(lldb) target modules lookup -n dlopen
1 match found in /home/nin/.lldb/module_cache/remote-android/.cache/DF63F23A-2C91-BA01-EC7D-7CCA4FA1A55D/libdl.so:

Address: libdl.so[0x00000000000010a0] (libdl.so..text + 352)
Summary: libdl.so`dlopen

target modules lookup -n __dl_dlopen

(lldb) target modules lookup -n __dl_dlopen
(lldb)

I have attach the output of /system/bin/linker64.

linker_64.log588 KBDownload

.

[nitesh.jain]

The android version am using for testing is "O".

generic_mips64:/ # getprop ro.build.version.release
O

[tberghammer]

Thanks for all of the data. Based on this I think you are using a preview version of android O what reports SDK 25 but the linker works the way an SDK 26 system linker would do.

I think the proper fix for the problem is to do something like what Greg suggested to detect the correct function name based on the list of available symbols instead of based on the SDK version. I will try to create a CL to fix it in the next couple of days (unless you are happy to do it), but not sure when I will have time for that and it is a bit tricky to test it.

Until then you can try to do either update to a newer version of the system image what reports SDK 26 (and make sure you are using a sufficiently new LLDB) or locally hack the "PlatformAndroid::LoadImage" function to use the SDK 26 version in case of SDK 25 as well (possibly with a check for ro.build.version.release)

[nitesh.jain]

In D11465#826132, @tberghammer wrote:

Thanks for all of the data. Based on this I think you are using a preview version of android O what reports SDK 25 but the linker works the way an SDK 26 system linker would do.

I think the proper fix for the problem is to do something like what Greg suggested to detect the correct function name based on the list of available symbols instead of based on the SDK version. I will try to create a CL to fix it in the next couple of days (unless you are happy to do it), but not sure when I will have time for that and it is a bit tricky to test it.

Until then you can try to do either update to a newer version of the system image what reports SDK 26 (and make sure you are using a sufficiently new LLDB) or locally hack the "PlatformAndroid::LoadImage" function to use the SDK 26 version in case of SDK 25 as well (possibly with a check for ro.build.version.release)

Thanks. As suggested by Greg will submit a patch for that

include/lldb/Target/Platform.h

//                Error &error) = 0;
             return 1;
         }
 
         virtual const lldb::UnixSignalsSP &
         GetRemoteUnixSignals();
 
         const lldb::UnixSignalsSP &
         GetUnixSignals();
         
+        // Android prefixes th functions in lindl.so. Let it specify the prefix used for those
+        // functions.
+        virtual const char*
+        GetDlFunctionPrefix()
+        {
+            return "";
+        }
+
         //------------------------------------------------------------------
         /// Locate a queue name given a thread's qaddr
         ///
         /// On a system using libdispatch ("Grand Central Dispatch") style
         /// queues, a thread may be associated with a GCD queue or not,
         /// and a queue may be associated with multiple threads.
         /// The process/thread must provide a way to find the "dispatch_qaddr" 
         /// for each thread, and from that dispatch_qaddr this Platform method

source/Plugins/Platform/Android/PlatformAndroid.h

public:
         GetFile (const FileSpec& source,
                  const FileSpec& destination) override;
 
         Error
         PutFile (const FileSpec& source,
                  const FileSpec& destination,
                  uint32_t uid = UINT32_MAX,
                  uint32_t gid = UINT32_MAX) override;
         
+        const char*
+        GetDlFunctionPrefix() override
+        {
+            return "__dl_";
+        }
+
      protected:
         const char *
         GetCacheHostname () override;
 
         Error
         DownloadModuleSlice (const FileSpec &src_file_spec,
                              const uint64_t src_offset,
                              const uint64_t src_size,

source/Target/Process.cpp

if (error.Success())
                 frame_sp->CalculateExecutionContext (exe_ctx);
                 EvaluateExpressionOptions expr_options;
                 expr_options.SetUnwindOnError(true);
                 expr_options.SetIgnoreBreakpoints(true);
                 expr_options.SetExecutionPolicy(eExecutionPolicyAlways);
                 expr_options.SetResultIsInternal(true);
                 
                 StreamString expr;
+                StreamString prefix;
+                const char* dl_prefix = m_target.GetPlatform()->GetDlFunctionPrefix();
                 expr.Printf(R"(
                                struct __lldb_dlopen_result { void *image_ptr; const char *error_str; } the_result;
-                               the_result.image_ptr = dlopen ("%s", 2);
+                               the_result.image_ptr = %sdlopen ("%s", 2);
                                if (the_result.image_ptr == (void *) 0x0)
                                {
-                                   the_result.error_str = dlerror();
+                                   the_result.error_str = %sdlerror();
                                }
                                else
                                {
                                    the_result.error_str = (const char *) 0x0;
                                }
                                the_result;
                               )",
-                              path);
-                const char *prefix = R"(
-                                        extern "C" void* dlopen (const char *path, int mode);
-                                        extern "C" const char *dlerror (void);
-                                        )";
+                              dl_prefix, path, dl_prefix);
+                prefix.Printf(R"(
+                                 extern "C" void* %sdlopen (const char *path, int mode);
+                                 extern "C" const char *%sdlerror (void);
+                                )",
+                                dl_prefix, dl_prefix);
                 lldb::ValueObjectSP result_valobj_sp;
                 Error expr_error;
                 ClangUserExpression::Evaluate (exe_ctx,
                                                expr_options,
                                                expr.GetData(),
-                                               prefix,
+                                               prefix.GetData(),
                                                result_valobj_sp,
                                                expr_error);
                 if (expr_error.Success())
                 {
                     error = result_valobj_sp->GetError();
                     if (error.Success())
                     {
                         Scalar scalar;
if (image_token < m_image_tokens.size())
                     if (frame_sp)
                     {
                         ExecutionContext exe_ctx;
                         frame_sp->CalculateExecutionContext (exe_ctx);
                         EvaluateExpressionOptions expr_options;
                         expr_options.SetUnwindOnError(true);
                         expr_options.SetIgnoreBreakpoints(true);
                         expr_options.SetExecutionPolicy(eExecutionPolicyAlways);
+                        
                         StreamString expr;
-                        expr.Printf("dlclose ((void *)0x%" PRIx64 ")", image_addr);
-                        const char *prefix = "extern \"C\" int dlclose(void* handle);\n";
+                        StreamString prefix;
+                        const char* dl_prefix = m_target.GetPlatform()->GetDlFunctionPrefix();
+                        expr.Printf("%sdlclose ((void *)0x%" PRIx64 ")", dl_prefix, image_addr);
+                        prefix.Printf("extern \"C\" int %sdlclose(void* handle);\n", dl_prefix);
+                        
                         lldb::ValueObjectSP result_valobj_sp;
                         Error expr_error;
                         ClangUserExpression::Evaluate (exe_ctx,
                                                        expr_options,
                                                        expr.GetData(),
-                                                       prefix,
+                                                       prefix.GetData(),
                                                        result_valobj_sp,
                                                        expr_error);
                         if (result_valobj_sp->GetError().Success())
                         {
                             Scalar scalar;
                             if (result_valobj_sp->ResolveValue (scalar))
                             {
                                 if (scalar.UInt(1))

test/functionalities/load_unload/TestLoadUnload.py

def test_dyld_library_path(self):
             env_cmd_string += ":" + wd
         self.runCmd(env_cmd_string)
 
         # This time, the hidden library should be picked up.
         self.expect("run", substrs=["return", "12345"])
 
     @skipIfFreeBSD # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @skipUnlessListedRemote(['android'])
-    @expectedFailureAndroid # dlopen and dlclose prefixed with "__dl_" on android causing JIT compilation issues
     def test_lldb_process_load_and_unload_commands(self):
         """Test that lldb process load/unload command work correctly."""
 
         # Invoke the default build rule.
         self.buildDefault()
         self.copy_shlibs_to_remote()
 
         exe = os.path.join(os.getcwd(), "a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Break at main.c before the call to dlopen().
         # Use lldb's process load command to load the dylib, instead.
 
         lldbutil.run_break_set_by_file_and_line (self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
 
         self.runCmd("run", RUN_SUCCEEDED)
 
         if lldb.remote_platform:
             shlib_dir = lldb.remote_platform.GetWorkingDirectory()
         else:
             shlib_dir = self.mydir
-        # Make sure that a_function does not exist at this point.
-        self.expect("image lookup -n a_function", "a_function should not exist yet",
-                    error=True, matching=False,
-            patterns = ["1 match found .* %s" % shlib_dir])
 
-        if lldb.remote_platform:
-            dylibName = os.path.join(shlib_dir, 'libloadunload_a.so')
-        elif self.platformIsDarwin():
+        if self.platformIsDarwin():
             dylibName = 'libloadunload_a.dylib'
         else:
             dylibName = 'libloadunload_a.so'
         
+        if lldb.remote_platform:
+            dylibPath = os.path.join(shlib_dir, 'libloadunload_a.so')
+        else:
+            dylibPath = dylibName
+
+        # Make sure that a_function does not exist at this point.
+        self.expect("image lookup -n a_function", "a_function should not exist yet",
+                    error=True, matching=False, patterns = ["1 match found"])
+
+
         # Use lldb 'process load' to load the dylib.
-        self.expect("process load %s" % dylibName, "%s loaded correctly" % dylibName,
-            patterns = ['Loading "%s".*ok' % dylibName,
+        self.expect("process load %s" % dylibPath, "%s loaded correctly" % dylibPath,
+            patterns = ['Loading "%s".*ok' % dylibPath,
                         'Image [0-9]+ loaded'])
 
         # Search for and match the "Image ([0-9]+) loaded" pattern.
         output = self.res.GetOutput()
         pattern = re.compile("Image ([0-9]+) loaded")
         for l in output.split(os.linesep):
             #print "l:", l
             match = pattern.search(l)
             if match:
                 break
         index = match.group(1)
 
         # Now we should have an entry for a_function.
         self.expect("image lookup -n a_function", "a_function should now exist",
-            patterns = ["1 match found .*%s" % shlib_dir])
+            patterns = ["1 match found .*%s" % dylibName])
 
         # Use lldb 'process unload' to unload the dylib.
         self.expect("process unload %s" % index, "%s unloaded correctly" % dylibName,
             patterns = ["Unloading .* with index %s.*ok" % index])
 
         self.runCmd("process continue")
 
     @skipIfFreeBSD # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support