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 <[email protected]> wrote: > Oops, sorry. Here it is... > > > On Wed, Feb 5, 2014 at 11:15 AM, Greg Clayton <[email protected]> wrote: > 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> > > > <patch-FactorDynamicLibrary-3.txt> _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
