[lldb-dev] [PATCH] factor methods in DynamicLoaderPOSIXDYLD into base class

[Steve Pucci]

Thanks, Greg.

I think it all makes sense, except for one bit:

In ObjectFileELF::SetLoadAddress(), are you proposing I simply call
Module::SetLoadAddress as it exists today?  That method walks through all
sections and checks only section_sp->IsThreadSpecific() to decide whether
to load the section, and there's no place to insert an ELF-specific check
of the section to see if it's loadable.  Is that what you meant, or are you
suggesting something else?

Instead of that I could have ObjectFileELF::SetLoadAddress iterate through
the sections as UpdateLoadedSectionsCommon does below, OR I could somehow
provide a callback to be called from Module::SetLoadAddress (perhaps by
passing in the ObjectFile*).

Thanks,
  Steve



On Mon, Jan 27, 2014 at 3:14 PM, Greg Clayton <gclayton at apple.com> wrote:

>
> On Jan 27, 2014, at 3:05 PM, Greg Clayton <gclayton at apple.com> wrote:
>
> > Looks ok except for:
> >
> > This is ELF specific with the file address of zero, and it probably
> should more be done via flags and asking the section if it is loadable:
> >
> > +void
> > +DynamicLoader::UpdateLoadedSectionsCommon(ModuleSP module, addr_t
> link_map_addr, addr_t base_addr)
> > +{
> > +    Target &target = m_process->GetTarget();
> > +    const SectionList *sections = GetSectionListFromModule(module);
> > +
> > +    assert(sections && "SectionList missing from loaded module.");
> > +
> > +    const size_t num_sections = sections->GetSize();
> > +
> > +    for (unsigned i = 0; i < num_sections; ++i)
> > +    {
> > +        SectionSP section_sp (sections->GetSectionAtIndex(i));
> > +        lldb::addr_t new_load_addr = section_sp->GetFileAddress() +
> base_addr;
> > +
> > +        // If the file address of the section is zero then this is not
> an
> > +        // allocatable/loadable section (property of ELF sh_addr).
>  Skip it.
> > +        if (new_load_addr == base_addr)
> > +            continue;
> > +
> > +        target.SetSectionLoadAddress(section_sp, new_load_addr);
> > +    }
> > +}
> >
> >
> > There is also a module function that does something similar to this,
> without the looking for the zero address:
> >
> > bool
> > Module::SetLoadAddress (Target &target, lldb::addr_t offset, bool
> &changed);
> >
> > So I would propose the following:
> >
> > Update DynamicLoader::UpdateLoadedSectionsCommon() to call into a new
> function that is a virtual function in ObjectFile:
> >
> > virtual bool SetLoadAddress (addr_t base_addr)
> > {
> >    return false;
> > }
> >
> > Then each object file (ObjectFileELF in your case) can choose to do the
> loading correctly given a single "base_addr":
> >
> > bool
> > ObjectFileELF::SetLoadAddress (addr_t base_addr)
> > {
> > }
> >
> > Then in ObjectFileELF::SetLoadAddress() you can use the section flags
> that were saved in the lldb_private::Section to properly determine which
> sections are loadable and which aren't. This function is for a rigid slide
> of all loadable sections.
> >
> > Does that make sense?
>
> I forgot the SetLoadAddress needs a target, and each object file already
> knows its module, so that doesn't need to be passed, it can be retrieved
> via the getter function:
>
> virtual bool SetLoadAddress (Target &target, addr_t base_addr)
> {
>    return false;
> }
>
> Then each object file (ObjectFileELF in your case) can choose to do the
> loading correctly given a single "base_addr":
>
> bool
> ObjectFileELF::SetLoadAddress (Target &target, addr_t base_addr)
> {
>      ModuleSP module_sp = GetModule();
>      if (module_sp)
>      {
>          ....
>      }
> }
>
>
>
> > Greg
> >
> >
> >
> > On Jan 27, 2014, at 2:32 PM, Steve Pucci <spucci at google.com> wrote:
> >
> >> Hi,
> >>
> >> I'd like to have access to a number of methods in
> DynamicLoaderPOSIXDYLD from the new class I'm working on,
> DynamicLoaderGDBServer.  These methods have no dependency on
> DynamicLoaderPOSIXDYLD, with two exceptions noted below, so I'm proposing
> to move them into the base class DynamicLoader.
> >>
> >> The two exceptions are the methods UpdateLoadedSections and
> UnloadSections; in each case there is one line of code that is special to
> the derived class, and the rest of the code in the method is generic to the
> base class.  In each case I created a XXXCommon() method on the base class
> with the common code, and created a virtual method XXX() on the base class,
> which in DynamicLoaderPOSIXDYLD will call the common code and then execute
> its one line of specialized code.
> >>
> >> This patch is intended to have no functional difference whatsoever.
>  All 276 tests that are enabled for Ubuntu pass successfully.
> >>
> >> Thanks,
> >>   Steve
> >>
> >>
> >>
> <patch-FactorDynamicLibrary.txt>_______________________________________________
> >> lldb-dev mailing list
> >> lldb-dev at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> >
> > _______________________________________________
> > 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/20140127/0f559b7c/attachment.html>