Page MenuHomePhabricator

Improved LLDB GdbServer support and shared library only debugging
ClosedPublic

Authored by ADodds on May 4 2015, 8:01 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ADodds updated this revision to Diff 24879.May 4 2015, 8:01 AM
ADodds retitled this revision from to Improved LLDB GdbServer support and shared library only debugging.
ADodds updated this object.
ADodds edited the test plan for this revision. (Show Details)
ADodds added a reviewer: clayborg.
ADodds set the repository for this revision to rL LLVM.
ADodds added a subscriber: Unknown Object (MLST).
clayborg edited edge metadata.May 4 2015, 11:10 AM

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 ↗(On Diff #24879)

Remove this.

115–171 ↗(On Diff #24879)

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

185–186 ↗(On Diff #24879)

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 ↗(On Diff #24879)

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 ↗(On Diff #24879)

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 ↗(On Diff #24879)

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

335–337 ↗(On Diff #24879)

Remove this and add the:

size_t
LoadModules() override;

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

ADodds updated this revision to Diff 24946.May 5 2015, 7:36 AM
ADodds edited edge metadata.

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 added a comment.May 5 2015, 9:59 AM

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 requested changes to this revision.May 5 2015, 10:56 AM
clayborg edited edge metadata.

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 ↗(On Diff #24946)

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

4111–4112 ↗(On Diff #24946)

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 ↗(On Diff #24946)

Move to .cpp file in anonymous namespace.

462 ↗(On Diff #24946)

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

This revision now requires changes to proceed.May 5 2015, 10:56 AM
ADodds updated this revision to Diff 25067.May 6 2015, 10:48 AM
ADodds edited edge metadata.

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 requested changes to this revision.May 6 2015, 11:39 AM
clayborg edited edge metadata.

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 ↗(On Diff #25067)

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 ↗(On Diff #25067)

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

4211 ↗(On Diff #25067)

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

if (!executable_sp)
4214 ↗(On Diff #25067)

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

4217–4235 ↗(On Diff #25067)

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 ↗(On Diff #25067)

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.

This revision now requires changes to proceed.May 6 2015, 11:39 AM
ADodds updated this revision to Diff 25203.May 7 2015, 10:04 AM
ADodds edited edge metadata.

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 accepted this revision.May 7 2015, 10:16 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.May 7 2015, 10:16 AM
This revision was automatically updated to reflect the committed changes.