Improved LLDB GdbServer support and shared library only debugging

[ADodds]

Summary

This patch allows LLDB to parse the $qXfer:Libraries: packet supported by GDBServer. From adding this support, lldb will try to resolve all modules currently loaded by the target upon attach, and correctly set their load address. LLDB will also search the shared library search path for suitable images which is particuarly nice.

The PosixDynamicLoader has been modified (as per Gregs suggestions) to query the loaded modules from ProcessGdbRemote. The PosixDynamicLoader has also had its load address computation changed making it suitable for use when lldb has only shared librarys in its module list.

[clayborg]

The content of the fix is good, just needs a bit of re-org.

I would like to see the following changes:

lldb_private::Process should get a new virtual function:

class Process {

    // Sometimes processes know how to retrieve and load shared libraries.
    // This is normally done by DynamicLoader plug-ins, but sometimes the
    // connection to the process allows retrieving this information. The dynamic
    // loader plug-ins can use this function if they can't determine the current
    // shared library load state.
    // Returns the number of shared libraries that were loaded
    virtual size_t
    LoadModules ()
    {
        return 0;
    }
}

Then ProcessGDBRemote should implement this function

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

26–27

Remove this.

115–171

Move this to the ProcessGDBRemote::LoadModules() function.

185–186

call m_process->LoadModules(). You might also want to check if this returns a non-zero value and if so skip the code below?

193–219

You need to set the executable first _before_ setting and load locations. If the target clears the image list in Target::SetExecutableModule() it will clear the section load list. So you need to first set the executable, then load images.

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h

177 ↗

(On Diff #24879)

Remove this and call new Process::LoadModules() functions described in comments.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3931–3932

Chage this function to use a local copy of GDBLoadedModuleInfoList and do the loading all within this function. You will probably need to set the executable in the target if needed, then add all other modules, then update the load location _after_ changing the executable. When calling Target::SetExecutableModule() it will clear the target module list and if any modules were loaded before this, they will be unloaded (because they are being removed from the target, so we don't want to have section load info for a module that the target doesn't have anymore...).

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

42–132

Move this to the ProcessGDBRemote.cpp file and use in ProcessGDBRemote::LoadModules().

335–337

Remove this and add the:

size_t
LoadModules() override;

to the ProcessGDBRemote.h header file and implement the functionality in there.

[ADodds]

Hi greg, I have pushed the loading code back inside of ProcessGDB remote as you suggested.
I wasnt quite following your suggestion regarding SetExecutable in the PosixDYLD, could you elaborate more on that please.

Thanks,
Aidan

[ADodds]

Hi Greg,
I forgot to remove two changes which accidentally made it threw in this patch. Could you please ignore the changes to PlatformWindows.cpp involving the software breakpoint opcode, and the change to GDBRemoteCommunicationClient.cpp which was committed this morning.
Sorry for the confusion.

[clayborg]

See inlined comments for what needs to be fixed

source/Plugins/Platform/Windows/PlatformWindows.cpp

352–377 ↗

(On Diff #24946)

Not part of this patch.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

294

Don't need this as a member variable right? We will call Process::LoadModules() from the dynamic loader when needed.

4111–4112

We might need to call Target::SetExecutable(..) in here if the executable changes. Take care of that here _before_ you load anything in the target. If you call Target:SetExecutable() after this, all of your load info will get blown away.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

42–144

Move to .cpp file in anonymous namespace.

462

Shouldn't need this as a member variable right? Just use one locally in the ProcessGDBRemote::LoadModules()?

[ADodds]

Hi Greg,

Thanks again for looking over my previous patches. Here's another revision which I hope manages to take on board all of the things you have suggested.

Thanks,
Aidan

[clayborg]

See inlined comments.

A few things to fix:
1 - remove spaces in if statements after the '(' and before the ')' to keep things consistent
2 - check the comments about setting the target correctly by creating a target with one executable (a.out) then attach to another (b.out) and make sure that works.

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

124

Do we need to check the return value of this and not do stuff below if non-zero is returned?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4209

Please add "_sp" suffix to shared pointer variables. So this should be "executable_sp"

4211

You don't need he ".get()" you can just write:

if (!executable_sp)

4214

Should this be negated? Why would we check if the file doesn't exist and return it?

4217–4235

Doesn't this code need to get the executable from the GDB info? How can we continue to use the wrong executable? What if we do:

(lldb) target create a.out

Then we attach and the program is "b.out"? We would have a valid executable, but it needs to be changed to "b.out".

4245–4250

We need to get the executable from the GDBLoadedModuleInfoList and verify our target has the right executable. Is this being done somewhere that I am missing? See my example above where we create a target with "a.out" then we attach to a program that is "b.out". Make sure that works.

[ADodds]

Hi greg,

I have changed the SetExecutable() code, so that it will now search the library list for the first executable, set it, and then try to load all of the modules into the target. Will this have the behaviour you are wanting or have I still miss understood something. I hope I have addresses your other concerns too.

Thanks,
Aidan

[clayborg]

Looks good.

include/lldb/Target/Process.h

     virtual const lldb::DataBufferSP
     GetAuxvData();
 
+    //------------------------------------------------------------------
+    /// Sometimes processes know how to retrieve and load shared libraries.
+    /// This is normally done by DynamicLoader plug-ins, but sometimes the
+    /// connection to the process allows retrieving this information. The
+    /// dynamic loader plug-ins can use this function if they can't 
+    /// determine the current shared library load state.
+    ///
+    /// @return
+    ///    The number of shared libraries that were loaded
+    //------------------------------------------------------------------
+    virtual size_t
+    LoadModules ()
+    {
+        return 0;
+    }
+
 protected:
     virtual JITLoaderList &
     GetJITLoaders ();

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

     if (log)
         log->Printf ("DynamicLoaderPOSIXDYLD::%s pid %" PRIu64 " reloaded auxv data", __FUNCTION__, m_process ? m_process->GetID () : LLDB_INVALID_PROCESS_ID);
 
-    ModuleSP executable_sp = GetTargetExecutable();
-    ResolveExecutableModule(executable_sp);
+    // ask the process if it can load any of its own modules
+    m_process->LoadModules ();
 
-    addr_t load_offset = ComputeLoadOffset();
+    ModuleSP executable_sp = GetTargetExecutable ();
+    ResolveExecutableModule (executable_sp);
+
+    // find the main process load offset
+    addr_t load_offset = ComputeLoadOffset ();
     if (log)
         log->Printf ("DynamicLoaderPOSIXDYLD::%s pid %" PRIu64 " executable '%s', load_offset 0x%" PRIx64, __FUNCTION__, m_process ? m_process->GetID () : LLDB_INVALID_PROCESS_ID, executable_sp ? executable_sp->GetFileSpec().GetPath().c_str () : "<null executable>", load_offset);
 
+    // if we dont have a load address we cant re-base
+    bool rebase_exec = ( load_offset == LLDB_INVALID_ADDRESS ) ? false : true;
 
-    if (executable_sp && load_offset != LLDB_INVALID_ADDRESS)
+    // if we have a valid executable
+    if (executable_sp.get())
+    {
+        lldb_private::ObjectFile * obj = executable_sp->GetObjectFile( );
+        if (obj)
+        {
+            // don't rebase if the module is not an executable
+            if (obj->GetType() != ObjectFile::Type::eTypeExecutable)
+                rebase_exec = false;
+
+            // don't rebase if the module already has a load address
+            Target & target = m_process->GetTarget ();
+            Address addr = obj->GetImageInfoAddress (&target);
+            if (addr.GetLoadAddress (&target) != LLDB_INVALID_ADDRESS)
+                rebase_exec = false;
+        }
+    }
+    else
+    {
+        // no executable, nothing to re-base
+        rebase_exec = false;
+    }
+
+    // if the target executable should be re-based
+    if (rebase_exec)
     {
         ModuleList module_list;
 

     if (!exe)
         return LLDB_INVALID_ADDRESS;
 
+    if ( exe->GetType( ) != ObjectFile::Type::eTypeExecutable )
+        return LLDB_INVALID_ADDRESS;
+
     Address file_entry = exe->GetEntryPointAddress();
 
     if (!file_entry.IsValid())

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

                   const ArchSpec& arch,
                   ModuleSpec &module_spec) override;
 
-    // query remote gdbserver for information
-    bool
-    GetGDBServerInfo ( );
+    virtual size_t
+    LoadModules( ) override;
 
 protected:
     friend class ThreadGDBRemote;
     friend class GDBRemoteCommunicationClient;
     friend class GDBRemoteRegisterContext;
 
+    class GDBLoadedModuleInfoList;
+
     //----------------------------------------------------------------------
     // Accessors
     //----------------------------------------------------------------------

     DynamicLoader *
     GetDynamicLoader () override;
 
+    // Query remote GDBServer for register information
+    bool
+    GetGDBServerRegisterInfo ();
+
+    // Query remote GDBServer for a detailed loaded library list
+    Error
+    GetLoadedModuleList (GDBLoadedModuleInfoList &);
+
+    lldb::ModuleSP
+    LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr);
+
+    lldb::ModuleSP
+    GetTargetExecutable ();
+
 private:
     //------------------------------------------------------------------
     // For ProcessGDBRemote only

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

     
 } // anonymous namespace end
 
+class ProcessGDBRemote::GDBLoadedModuleInfoList
+{
+public:
+
+    class LoadedModuleInfo
+    {
+    public:
+
+        enum e_data_point
+        {
+            e_has_name      = 0,
+            e_has_base      ,
+            e_has_dynamic   ,
+            e_has_link_map  ,
+            e_num
+        };
+
+        LoadedModuleInfo ()
+        {
+            for ( uint32_t i = 0; i < e_num; ++i )
+                m_has[i] = false;
+        };
+
+        void set_name (const std::string & name)
+        {
+            m_name = name;
+            m_has[e_has_name] = true;
+        }
+        bool get_name (std::string & out) const
+        {
+            out = m_name;
+            return m_has[e_has_name];
+        }
+
+        void set_base (const lldb::addr_t base)
+        {
+            m_base = base;
+            m_has[e_has_base] = true;
+        }
+        bool get_base (lldb::addr_t & out) const
+        {
+            out = m_base;
+            return m_has[e_has_base];
+        }
+
+        void set_link_map (const lldb::addr_t addr)
+        {
+            m_link_map = addr;
+            m_has[e_has_link_map] = true;
+        }
+        bool get_link_map (lldb::addr_t & out) const
+        {
+            out = m_link_map;
+            return m_has[e_has_link_map];
+        }
+
+        void set_dynamic (const lldb::addr_t addr)
+        {
+            m_dynamic = addr;
+            m_has[e_has_dynamic] = true;
+        }
+        bool get_dynamic (lldb::addr_t & out) const
+        {
+            out = m_dynamic;
+            return m_has[e_has_dynamic];
+        }
+
+        bool has_info (e_data_point datum)
+        {
+            assert( datum < e_num );
+            return m_has[datum];
+        }
+
+    protected:
+
+        bool m_has[e_num];
+        std::string m_name;
+        lldb::addr_t m_link_map;
+        lldb::addr_t m_base;
+        lldb::addr_t m_dynamic;
+    };
+
+    GDBLoadedModuleInfoList ()
+        : m_list ()
+        , m_link_map (LLDB_INVALID_ADDRESS)
+    {}
+
+    void add (const LoadedModuleInfo & mod)
+    {
+        m_list.push_back (mod);
+    }
+
+    void clear ()
+    {
+        m_list.clear ();
+    }
+
+    std::vector<LoadedModuleInfo> m_list;
+    lldb::addr_t m_link_map;
+};
+
 // TODO Randomly assigning a port is unsafe.  We should get an unused
 // ephemeral port from the kernel and make sure we reserve it before passing
 // it to debugserver.

     if (reg_num == 0)
     {
         // try to extract information from servers target.xml
-        if ( GetGDBServerInfo( ) )
+        if ( GetGDBServerRegisterInfo () )
             return;
 
         FileSpec target_definition_fspec = GetGlobalPluginProperties()->GetTargetDefinitionFile ();

 addr_t
 ProcessGDBRemote::GetImageInfoAddress()
 {
-    return m_gdb_comm.GetShlibInfoAddr();
+    // request the link map address via the $qShlibInfoAddr packet
+    lldb::addr_t addr = m_gdb_comm.GetShlibInfoAddr();
+
+    // the loaded module list can also provides a link map address
+    if ( addr == LLDB_INVALID_ADDRESS ) {
+        GDBLoadedModuleInfoList list;
+        if (GetLoadedModuleList(list).Success())
+            addr = list.m_link_map;
+    }
+
+    return addr;
 }
 
 //------------------------------------------------------------------

 // return:  'true'  on success
 //          'false' on failure
 bool
-ProcessGDBRemote::GetGDBServerInfo ()
+ProcessGDBRemote::GetGDBServerRegisterInfo ()
 {
 
     // redirect libxml2's error handler since the default prints to stdout

     return true;
 }
 
+Error
+ProcessGDBRemote::GetLoadedModuleList (GDBLoadedModuleInfoList & list)
+{
+    Log *log = GetLogIfAnyCategoriesSet (LIBLLDB_LOG_PROCESS);
+    if (log)
+        log->Printf ("ProcessGDBRemote::%s", __FUNCTION__);
+
+    // redirect libxml2's error handler since the default prints to stdout
+    xmlGenericErrorFunc func = libxml2NullErrorFunc;
+    initGenericErrorDefaultFunc (&func);
+
+    GDBRemoteCommunicationClient & comm = m_gdb_comm;
+    GDBRemoteDynamicRegisterInfo & regInfo = m_register_info;
+
+    // check that we have extended feature read support
+    if (!comm.GetQXferLibrariesSVR4ReadSupported ())
+        return Error (0,ErrorType::eErrorTypeGeneric);
+
+    list.clear ();
+
+    // request the loaded library list
+    std::string raw;
+    lldb_private::Error lldberr;
+    if (!comm.ReadExtFeature (ConstString ("libraries-svr4"), ConstString (""), raw, lldberr))
+        return Error (0,ErrorType::eErrorTypeGeneric);
+
+    // parse the xml file in memory
+    if (log)
+        log->Printf( "parsing: %s", raw.c_str() );
+    xmlDocPtr doc = xmlReadMemory (raw.c_str(), raw.size(), "noname.xml", nullptr, 0);
+    if (doc == nullptr)
+        return Error (0,ErrorType::eErrorTypeGeneric);
+
+    xmlNodePtr elm = xmlExFindElement (doc->children, {"library-list-svr4"});
+    if (!elm)
+        return Error();
+
+    // main link map structure
+    xmlAttr * attr = xmlExFindAttribute (elm, "main-lm");
+    if (attr)
+    {
+        std::string val = xmlExGetTextContent (attr);
+        if (val.length() > 2)
+        {
+            uint32_t process_lm = std::stoul (val.c_str()+2, 0, 16);
+            list.m_link_map = process_lm;
+        }
+    }
+
+    // parse individual library entries
+    for (xmlNode * child = elm->children; child; child=child->next)
+    {
+        if (!child->name)
+            continue;
+
+        if (strcmp ((char*)child->name, "library") != 0)
+            continue;
+
+        GDBLoadedModuleInfoList::LoadedModuleInfo module;
+
+        for (xmlAttrPtr prop = child->properties; prop; prop=prop->next)
+        {
+            if (strcmp ((char*)prop->name, "name") == 0 )
+                module.set_name (xmlExGetTextContent (prop));
+
+            // the address of the link_map struct.
+            if (strcmp ((char*)prop->name, "lm") == 0 )
+            {
+                std::string val = xmlExGetTextContent (prop);
+                if (val.length() > 2)
+                {
+                    uint32_t module_lm = std::stoul (val.c_str()+2, 0, 16);
+                    module.set_link_map (module_lm);
+                }
+            }
+
+            // the displacement as read from the field 'l_addr' of the link_map struct.
+            if (strcmp ((char*)prop->name, "l_addr") == 0 )
+            {
+                std::string val = xmlExGetTextContent (prop);
+                if (val.length() > 2)
+                {
+                    uint32_t module_base = std::stoul (val.c_str()+2, 0, 16);
+                    module.set_base (module_base);
+                }
+            }
+
+            // the memory address of the libraries PT_DYAMIC section.
+            if (strcmp ((char*)prop->name, "l_ld") == 0 )
+            {
+                std::string val = xmlExGetTextContent (prop);
+                if (val.length() > 2)
+                {
+                    uint32_t module_dyn = std::stoul (val.c_str()+2, 0, 16);
+                    module.set_dynamic (module_dyn);
+                }
+            }
+        }
+
+        if (log)
+        {
+            std::string name ("");
+            lldb::addr_t lm=0, base=0, ld=0;
+
+            module.get_name (name);
+            module.get_link_map (lm);
+            module.get_base (base);
+            module.get_dynamic (ld);
+
+            log->Printf ("found (link_map:0x08%" PRIx64 ", base:0x08%" PRIx64 ", ld:0x08%" PRIx64 ", name:'%s')", lm, base, ld, name.c_str());
+        }
+
+        list.add (module);
+    }
+
+    if (log)
+        log->Printf ("found %" PRId32 " modules in total", (int) list.m_list.size());
+
+    return Error();
+}
+
 #else // if defined( LIBXML2_DEFINED )
 
-using namespace lldb_private::process_gdb_remote;
+Error
+ProcessGDBRemote::GetLoadedModuleList (GDBLoadedModuleInfoList &)
+{
+    // stub (libxml2 not present)
+    Error err;
+    err.SetError( 0, ErrorType::eErrorTypeGeneric );
+    return err;
+}
 
 bool
-ProcessGDBRemote::GetGDBServerInfo ()
+ProcessGDBRemote::GetGDBServerRegisterInfo ()
 {
     // stub (libxml2 not present)
     return false;

 
 #endif // if defined( LIBXML2_DEFINED )
 
+lldb::ModuleSP
+ProcessGDBRemote::LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr)
+{
+    Target &target = m_process->GetTarget();
+    ModuleList &modules = target.GetImages();
+    ModuleSP module_sp;
+
+    bool changed = false;
+
+    ModuleSpec module_spec (file, target.GetArchitecture());
+    if ((module_sp = modules.FindFirstModule (module_spec)))
+    {
+        module_sp->SetLoadAddress( target, base_addr, true, changed );
+    }
+    else if ((module_sp = target.GetSharedModule(module_spec)))
+    {
+        module_sp->SetLoadAddress( target, base_addr, true, changed );
+    }
+
+    return module_sp;
+}
+
+ModuleSP
+ProcessGDBRemote::GetTargetExecutable()
+{
+    Target &target = m_process->GetTarget();
+    ModuleSP executable = target.GetExecutableModule();
+
+    if ( !executable.get( ) )
+        return ModuleSP();
+
+    if ( !executable->GetFileSpec( ).Exists( ) )
+        return executable;
+
+    ModuleSpec module_spec (executable->GetFileSpec(), executable->GetArchitecture());
+    ModuleSP module_sp (new Module (module_spec));
+
+    // Check if the executable has changed and set it to the target executable if they differ.
+    if (module_sp.get() && module_sp->GetUUID().IsValid() && executable->GetUUID().IsValid())
+    {
+        if (module_sp->GetUUID() != executable->GetUUID())
+            executable.reset();
+    }
+    else if (executable->FileHasChanged())
+        executable.reset();
+
+    if (!executable.get())
+    {
+        executable = target.GetSharedModule(module_spec);
+        if (executable.get() != target.GetExecutableModulePointer())
+            target.SetExecutableModule(executable, false);
+    }
+
+    return executable;
+}
+
+size_t
+ProcessGDBRemote::LoadModules ()
+{
+    using lldb_private::process_gdb_remote::ProcessGDBRemote;
+
+    // request a list of loaded libraries from GDBServer
+    GDBLoadedModuleInfoList module_list;
+    if (GetLoadedModuleList(module_list).Fail())
+        return 0;
+
+    // if we have a target executable make sure its set before we pull in any loaded modules
+    GetTargetExecutable();
+
+    // get a list of all the modules
+    ModuleList &loaded_modules = m_process->GetTarget().GetImages();
+    ModuleList new_modules;
+
+    for (GDBLoadedModuleInfoList::LoadedModuleInfo & modInfo : module_list.m_list)
+    {
+        std::string  mod_name;
+        lldb::addr_t mod_base;
+
+        bool valid = true;
+        valid &= modInfo.get_name (mod_name);
+        valid &= modInfo.get_base (mod_base);
+        if ( !valid )
+            continue;
+
+        // hack (cleaner way to get file name only?) (win/unix compat?)
+        int marker = mod_name.rfind ('/');
+        if (marker == std::string::npos)
+            marker = 0;
+        else
+            marker += 1;
+
+        FileSpec file (mod_name.c_str()+marker, true);
+        lldb::ModuleSP module_sp = LoadModuleAtAddress (file, mod_base);
+
+        if (module_sp.get())
+        {
+            loaded_modules.AppendIfNeeded (module_sp);
+            new_modules.Append (module_sp);
+        }
+    }
+
+    if (new_modules.GetSize() > 0)
+        m_process->GetTarget().ModulesDidLoad (new_modules);
+
+    return new_modules.GetSize();
+}
 
 class CommandObjectProcessGDBRemotePacketHistory : public CommandObjectParsed
 {

source/Target/Target.cpp

 ModuleSP
 Target::GetExecutableModule ()
 {
-    return m_images.GetModuleAtIndex(0);
+    // search for the first executable in the module list
+    for ( size_t i = 0; i < m_images.GetSize( ); ++i )
+    {
+        ModuleSP modsp = m_images.GetModuleAtIndex( i );
+        lldb_private::ObjectFile * obj = modsp->GetObjectFile( );
+        if ( obj == nullptr )
+            continue;
+        ObjectFile::Type type = obj->GetType( );
+        if ( type == ObjectFile::Type::eTypeExecutable )
+            return modsp;
+    }
+    // as fall back return the first module loaded
+    return m_images.GetModuleAtIndex( 0 );
 }
 
 Module*
 Target::GetExecutableModulePointer ()
 {
-    return m_images.GetModulePointerAtIndex(0);
+    return GetExecutableModule().get();
 }
 
 static void