[lldb-dev] [PATCH] factor methods in DynamicLoaderPOSIXDYLD into base class
Steve Pucci
spucci at google.com
Thu Feb 6 11:08:43 PST 2014
OK, comment fixed and committed as 200939.
Thanks again,
Steve
On Wed, Feb 5, 2014 at 1:42 PM, Greg Clayton <gclayton at apple.com> wrote:
> Looks good, except the comment in ObjectFileELF::SetLoadAddress(...) is
> out of date:
>
> + // Iterate through the object file sections to find the
> + // first section that starts of file offset zero and that
> + // has bytes in the file...
>
> This should probably be changed to reflect the check for SHF_ALLOC...
>
>
> On Feb 5, 2014, at 11:22 AM, Steve Pucci <spucci at google.com> wrote:
>
> > Oops, sorry. Here it is...
> >
> >
> > On Wed, Feb 5, 2014 at 11:15 AM, Greg Clayton <gclayton at apple.com>
> wrote:
> > Can you attach the update patch file?
> >
> > On Feb 5, 2014, at 10:23 AM, Steve Pucci <spucci at google.com> wrote:
> >
> > > OK, updated patch attached. Responses to the questions inline below.
> > >
> > > Thanks,
> > > Steve
> > >
> > >
> > > On Wed, Jan 29, 2014 at 4:16 PM, Greg Clayton <gclayton at apple.com>
> wrote:
> > > So a few questions:
> > >
> > > 1 - Does anyone use the "link_map_addr" parameter that is being sent
> to many of the functions that were moved into DynamicLoader? I didn't see
> any. Please remove this argument if possible.
> > >
> > > It turns out this *is* used in
> DynamicLoaderPOSIXDYLD::UpdateLoadedSections(), so I left it in there and
> in the routine that calls it (LoadModuleAtAddress), but it was possible to
> remove it from UpdateLoadedSectionsCommon so I did that.
> > >
> > > 2 - ReadInt() isn't correct for all systems:
> > >
> > > static int ReadInt(Process *process, addr_t addr)
> > > {
> > > Error error;
> > > int value = (int)process->ReadUnsignedIntegerFromMemory(addr,
> sizeof(uint32_t), 0, error);
> > >
> > > See the "sizeof(uint32_t)"? We will want to get the size of an "int"
> for the process that is being run if this function really does need to get
> a "int" from the debugger. So this sizeof() needs to be changed to get the
> actual size of a type "int" via:
> > >
> > > ClangASTContext *ast =
> m_process->GetTarget().GetScratchClangASTContext();
> > > ClangASTType int_type = ast->GetBasicType (eBasicTypeInt);
> > > uint64_t int_size = int_type.GetByteSize();
> > >
> > > Or this function might be more useful if we pass in the size of the
> integer we need?
> > >
> > > Investigating this revealed what appears to be a bug on big-endian
> 64-bit Linux platforms, but I'm decoupling that bug fix from this patch, as
> this patch is supposed to be functionally a no-op. I've taken your
> suggestion that the size of the int should be passed in, but for now the
> caller passes in "4" as that's equivalent to the code that was there before.
> > >
> > > 3 - The DynamicLoader class has a m_process member variable so the
> "Process *process" argument doesn't need to be passed into the following
> functions:
> > > int DynamicLoader::ReadInt(Process *process, addr_t addr);
> > > addr_t DynamicLoader::ReadPointer(Process *process, addr_t
> addr);
> > >
> > > I made these methods non-static and removed the process parameter as
> you suggested.
> > >
> > > On Jan 29, 2014, at 1:45 PM, Steve Pucci <spucci at google.com> wrote:
> > >
> > > > OK, that seemed to work, at least on my simple shared-library
> testcase on Ubuntu, which invokes the new code in
> ObjectFileELF::SetLoadAddress().
> > > >
> > > > Updated full patch attached.
> > > >
> > > >
> > > >
> > > >
> > > > On Tue, Jan 28, 2014 at 7:19 AM, Steve Pucci <spucci at google.com>
> wrote:
> > > > OK, great, thanks Greg, I'll give it a go.
> > > >
> > > > - Steve
> > > >
> > > >
> > > > On Mon, Jan 27, 2014 at 4:31 PM, Greg Clayton <gclayton at apple.com>
> wrote:
> > > > The first thing to do is just look at the section that has address
> of zero and see if it has any bits that the other don't or vice versa.
> > > >
> > > > I think the bit you are looking for is SHF_ALLOC.
> > > >
> > > > The "sh_flags" from the ELF section are indeed placed in the
> lldb_private::Section flags, so you should be able to do:
> > > >
> > > > for (section : sections)
> > > > {
> > > > if (section->Test(SHF_ALLOC))
> > > > {
> > > > // Load this section
> > > > }
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > On Jan 27, 2014, at 4:23 PM, Steve Pucci <spucci at google.com> wrote:
> > > >
> > > > > 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
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > <patch-FactorDynamicLibrary-2.txt>
> >
> >
> > <patch-FactorDynamicLibrary-3.txt>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140206/267e71bc/attachment.html>
More information about the lldb-dev
mailing list