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.
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?
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);
On Jan 29, 2014, at 1:45 PM, Steve Pucci <[email protected]> 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 <[email protected]> wrote:
> OK, great, thanks Greg, I'll give it a go.
>
> - Steve
>
>
> On Mon, Jan 27, 2014 at 4:31 PM, Greg Clayton <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >
> > On Jan 27, 2014, at 4:02 PM, Steve Pucci <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Jan 27, 2014, at 3:05 PM, Greg Clayton <[email protected]> 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 <[email protected]> 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
> > > >> [email protected]
> > > >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> > > >
> > > > _______________________________________________
> > > > lldb-dev mailing list
> > > > [email protected]
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> > >
> > >
> >
> >
>
>
>
> <patch-FactorDynamicLibrary-2.txt>
_______________________________________________
lldb-dev mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
