[lldb-dev] [PATCH] Linux JIT support

[Andrew MacPherson]

Hi Greg, thanks for the feedback. I've made the changes you requested here.
Regarding #5, there's no need right now for a generic iterator on the
JITLoadersList so I left out that part, iteration only happens internally
in the DidAttach() and DidLaunch() methods.

Let me know if there's anything else that should be updated.

Cheers!
Andrew


On Tue, Mar 4, 2014 at 12:04 AM, Greg Clayton <gclayton at apple.com> wrote:

> Looks pretty good. A few changes I would like to see:
>
> 1 - switch away from using std::unique_ptr<> to use std::shared_ptr<> for
> the JITLoader instances:
>
>     typedef lldb::JITLoaderSP (*JITLoaderCreateInstance) (Process
> *process, bool force);
>
> Newer plugins are adopting this method in case we ever want to inherit
> from std::shared_from_this()...
>
> 2 - make a JITLoaderList class that encapsulates the storage details of
> the JITLoader objects (std::vector<std::unique_ptr<JITLoader>> is what you
> had, we should probably switch to std::vector<JITLoaderSP>) and also
> protects access to the list with a recursive mutex. LLDB is multi-threaded
> and we should make sure we never can hose this collection by accessing it
> from two threads. The class should probably have simple functions like:
>
> class JITLoaderList
> {
>   JITLoaderList();
>   ~JITLoaderList();
>   void Append (const JITLoaderSP &jit_loader_sp);
>   void Remove (const JITLoaderSP &jit_loader_sp);
>   size_t GetSize() const;
>   JITLoaderSP GetLoaderAtIndex (size_t idx);
>   void DidLaunch();
>   void DidAttach();
> };
>
> 3 - switch any functions that were taking/returning
> "std::vector<std::unique_ptr<JITLoader>>" over to use the new JITLoaderList
> class.
> 4 - Process had these new members added:
>
> +    JITLoaderList m_jit_aps;
> +    bool m_did_load_jit_aps;
>
> You might be able to switch over to:
>     std::unique_ptr<JITLoaderList> m_jit_loaders_ap;
>
> Then you accessor switches to be:
>
> JITLoaderList &
> Process::GetJITLoaders ()
> {
>     if (!m_jit_loaders_ap) {
>         m_jit_loaders_ap.reset (new JITLoaderList());
>         JITLoader::LoadPlugins(this, *m_jit_loaders_ap);
>     }
>     return *m_jit_loaders_ap;
> }
>
> 5 - Any code that used to be using the iterators can just be replaced with
> the JITLoaderList::DidLaunch() and JITLoaderList::DidAttach() and the
> iteration can happen internally in the JITLoaderList class. If you need a
> generic iterator for the JITLoader objects inside a JITLoaderList class,
> you can make a:
>
>     void
>     ForEach (std::function <bool(JITLoader &)> const &callback);
>
> In the JITLoaderList class. This can work well because thee "ForEach"
> function can take the mutex to protect the collection's integrity during
> iteration, and the "bool" return value from the function can indicate if
> iteration should continue or stop...
>
>
>
> On Mar 3, 2014, at 2:32 PM, Andrew MacPherson <andrew.macp at gmail.com>
> wrote:
>
> > This patch adds support for loading debug info from JITed code on Linux
> through a new JITLoader plugin. The patch was largely written by Keno
> Fischer.
> >
> > The patch can be verified like this:
> >       * <Take any test.c program with a main()>
> >       * /build/bin/clang -O0 -g -emit-llvm -S test.ll test.c
> >       * /build/bin/lldb -- /build/bin/lli --use-mcjit test.ll
> >       * br set -f test.c -l <some line>
> >       * r
> >       * <hit breakpoint>
> >       * bt (to see stack trace with filenames and lines)
> > Let me know if there's anything that should be updated or if there is a
> better way to submit a larger patch like this.
> >
> > Thanks,
> >
> > Andrew
> >
> > <linux-jit.patch>_______________________________________________
> > lldb-dev mailing list
> > lldb-dev at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140304/2627bfc7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: linux-jit.2.patch
Type: text/x-diff
Size: 58117 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140304/2627bfc7/attachment.patch>