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

[Steve Pucci]

OK, I understand, though I may need some help from someone with
interpreting Section headers for Elf.  I'll let this group know if I have
questions.

Thanks again,
   Steve


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

>
> On Jan 27, 2014, at 4:02 PM, Steve Pucci <spucci at google.com> wrote:
>
> > 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?
>
> Something else. When the ObjectFileELF parser parses the section headers,
> it places the flags (or it should if it isn't) into the flags field of the
> lldb_private::Section. So it should be able to use the flags from its
> sections to correctly in each lldb_private::Section, and correctly
> interpret them to know which sections need to be loaded and which don't. So
> let the ELF plugin that created the sections correctly interpret the flags
> it put into the sections.
>
> We then will need to change the Module::SetLoadAddress() to call this new
> ObjectFile function.
>
> >
> > 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*).
>
> It should all be done in the ObjectFileELF::SetLoadAddress function.
>
> >
> > 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/206a78f8/attachment.html>