Can you attach the update patch file? On Feb 5, 2014, at 10:23 AM, Steve Pucci <[email protected]> wrote:
> OK, updated patch attached. Responses to the questions inline below. > > Thanks, > Steve > > > On Wed, Jan 29, 2014 at 4:16 PM, Greg Clayton <[email protected]> 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 <[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
