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

[Greg Clayton]

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