<div dir="ltr">OK, comment fixed and committed as 200939.<div><br></div><div>Thanks again,</div><div> Steve<br><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 5, 2014 at 1:42 PM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Looks good, except the comment in ObjectFileELF::SetLoadAddress(...) is out of date:<br>
<br>
+ // Iterate through the object file sections to find the<br>
+ // first section that starts of file offset zero and that<br>
+ // has bytes in the file...<br>
<br>
This should probably be changed to reflect the check for SHF_ALLOC...<br>
<div><div class="h5"><br>
<br>
On Feb 5, 2014, at 11:22 AM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
<br>
> Oops, sorry. Here it is...<br>
><br>
><br>
> On Wed, Feb 5, 2014 at 11:15 AM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> Can you attach the update patch file?<br>
><br>
> On Feb 5, 2014, at 10:23 AM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
><br>
> > OK, updated patch attached. Responses to the questions inline below.<br>
> ><br>
> > Thanks,<br>
> > Steve<br>
> ><br>
> ><br>
> > On Wed, Jan 29, 2014 at 4:16 PM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> > So a few questions:<br>
> ><br>
> > 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.<br>
> ><br>
> > 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.<br>
> ><br>
> > 2 - ReadInt() isn't correct for all systems:<br>
> ><br>
> > static int ReadInt(Process *process, addr_t addr)<br>
> > {<br>
> > Error error;<br>
> > int value = (int)process->ReadUnsignedIntegerFromMemory(addr, sizeof(uint32_t), 0, error);<br>
> ><br>
> > 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:<br>
> ><br>
> > ClangASTContext *ast = m_process->GetTarget().GetScratchClangASTContext();<br>
> > ClangASTType int_type = ast->GetBasicType (eBasicTypeInt);<br>
> > uint64_t int_size = int_type.GetByteSize();<br>
> ><br>
> > Or this function might be more useful if we pass in the size of the integer we need?<br>
> ><br>
> > 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.<br>
> ><br>
> > 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:<br>
> > int DynamicLoader::ReadInt(Process *process, addr_t addr);<br>
> > addr_t DynamicLoader::ReadPointer(Process *process, addr_t addr);<br>
> ><br>
> > I made these methods non-static and removed the process parameter as you suggested.<br>
> ><br>
> > On Jan 29, 2014, at 1:45 PM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
> ><br>
> > > OK, that seemed to work, at least on my simple shared-library testcase on Ubuntu, which invokes the new code in ObjectFileELF::SetLoadAddress().<br>
> > ><br>
> > > Updated full patch attached.<br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > On Tue, Jan 28, 2014 at 7:19 AM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
> > > OK, great, thanks Greg, I'll give it a go.<br>
> > ><br>
> > > - Steve<br>
> > ><br>
> > ><br>
> > > On Mon, Jan 27, 2014 at 4:31 PM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> > > 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.<br>
> > ><br>
> > > I think the bit you are looking for is SHF_ALLOC.<br>
> > ><br>
> > > The "sh_flags" from the ELF section are indeed placed in the lldb_private::Section flags, so you should be able to do:<br>
> > ><br>
> > > for (section : sections)<br>
> > > {<br>
> > > if (section->Test(SHF_ALLOC))<br>
> > > {<br>
> > > // Load this section<br>
> > > }<br>
> > > }<br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > On Jan 27, 2014, at 4:23 PM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
> > ><br>
> > > > 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.<br>
> > > ><br>
> > > > Thanks again,<br>
> > > > Steve<br>
> > > ><br>
> > > ><br>
> > > > On Mon, Jan 27, 2014 at 4:17 PM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> > > ><br>
> > > > On Jan 27, 2014, at 4:02 PM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
> > > ><br>
> > > > > Thanks, Greg.<br>
> > > > ><br>
> > > > > I think it all makes sense, except for one bit:<br>
> > > > ><br>
> > > > > 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?<br>
> > > ><br>
> > > > 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.<br>
> > > ><br>
> > > > We then will need to change the Module::SetLoadAddress() to call this new ObjectFile function.<br>
> > > ><br>
> > > > ><br>
> > > > > 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*).<br>
> > > ><br>
> > > > It should all be done in the ObjectFileELF::SetLoadAddress function.<br>
> > > ><br>
> > > > ><br>
> > > > > Thanks,<br>
> > > > > Steve<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > On Mon, Jan 27, 2014 at 3:14 PM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> > > > ><br>
> > > > > On Jan 27, 2014, at 3:05 PM, Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br>
> > > > ><br>
> > > > > > Looks ok except for:<br>
> > > > > ><br>
> > > > > > 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:<br>
> > > > > ><br>
> > > > > > +void<br>
> > > > > > +DynamicLoader::UpdateLoadedSectionsCommon(ModuleSP module, addr_t link_map_addr, addr_t base_addr)<br>
> > > > > > +{<br>
> > > > > > + Target &target = m_process->GetTarget();<br>
> > > > > > + const SectionList *sections = GetSectionListFromModule(module);<br>
> > > > > > +<br>
> > > > > > + assert(sections && "SectionList missing from loaded module.");<br>
> > > > > > +<br>
> > > > > > + const size_t num_sections = sections->GetSize();<br>
> > > > > > +<br>
> > > > > > + for (unsigned i = 0; i < num_sections; ++i)<br>
> > > > > > + {<br>
> > > > > > + SectionSP section_sp (sections->GetSectionAtIndex(i));<br>
> > > > > > + lldb::addr_t new_load_addr = section_sp->GetFileAddress() + base_addr;<br>
> > > > > > +<br>
> > > > > > + // If the file address of the section is zero then this is not an<br>
> > > > > > + // allocatable/loadable section (property of ELF sh_addr). Skip it.<br>
> > > > > > + if (new_load_addr == base_addr)<br>
> > > > > > + continue;<br>
> > > > > > +<br>
> > > > > > + target.SetSectionLoadAddress(section_sp, new_load_addr);<br>
> > > > > > + }<br>
> > > > > > +}<br>
> > > > > ><br>
> > > > > ><br>
> > > > > > There is also a module function that does something similar to this, without the looking for the zero address:<br>
> > > > > ><br>
> > > > > > bool<br>
> > > > > > Module::SetLoadAddress (Target &target, lldb::addr_t offset, bool &changed);<br>
> > > > > ><br>
> > > > > > So I would propose the following:<br>
> > > > > ><br>
> > > > > > Update DynamicLoader::UpdateLoadedSectionsCommon() to call into a new function that is a virtual function in ObjectFile:<br>
> > > > > ><br>
> > > > > > virtual bool SetLoadAddress (addr_t base_addr)<br>
> > > > > > {<br>
> > > > > > return false;<br>
> > > > > > }<br>
> > > > > ><br>
> > > > > > Then each object file (ObjectFileELF in your case) can choose to do the loading correctly given a single "base_addr":<br>
> > > > > ><br>
> > > > > > bool<br>
> > > > > > ObjectFileELF::SetLoadAddress (addr_t base_addr)<br>
> > > > > > {<br>
> > > > > > }<br>
> > > > > ><br>
> > > > > > 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.<br>
> > > > > ><br>
> > > > > > Does that make sense?<br>
> > > > ><br>
> > > > > 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:<br>
> > > > ><br>
> > > > > virtual bool SetLoadAddress (Target &target, addr_t base_addr)<br>
> > > > > {<br>
> > > > > return false;<br>
> > > > > }<br>
> > > > ><br>
> > > > > Then each object file (ObjectFileELF in your case) can choose to do the loading correctly given a single "base_addr":<br>
> > > > ><br>
> > > > > bool<br>
> > > > > ObjectFileELF::SetLoadAddress (Target &target, addr_t base_addr)<br>
> > > > > {<br>
> > > > > ModuleSP module_sp = GetModule();<br>
> > > > > if (module_sp)<br>
> > > > > {<br>
> > > > > ....<br>
> > > > > }<br>
> > > > > }<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > > Greg<br>
> > > > > ><br>
> > > > > ><br>
> > > > > ><br>
> > > > > > On Jan 27, 2014, at 2:32 PM, Steve Pucci <<a href="mailto:spucci@google.com">spucci@google.com</a>> wrote:<br>
> > > > > ><br>
> > > > > >> Hi,<br>
> > > > > >><br>
> > > > > >> 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.<br>
> > > > > >><br>
> > > > > >> 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.<br>
> > > > > >><br>
> > > > > >> This patch is intended to have no functional difference whatsoever. All 276 tests that are enabled for Ubuntu pass successfully.<br>
> > > > > >><br>
> > > > > >> Thanks,<br>
> > > > > >> Steve<br>
> > > > > >><br>
> > > > > >><br>
> > > > > >> <patch-FactorDynamicLibrary.txt>_______________________________________________<br>
> > > > > >> lldb-dev mailing list<br>
> > > > > >> <a href="mailto:lldb-dev@cs.uiuc.edu">lldb-dev@cs.uiuc.edu</a><br>
> > > > > >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
> > > > > ><br>
> > > > > > _______________________________________________<br>
> > > > > > lldb-dev mailing list<br>
> > > > > > <a href="mailto:lldb-dev@cs.uiuc.edu">lldb-dev@cs.uiuc.edu</a><br>
> > > > > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
> > > > ><br>
> > > > ><br>
> > > ><br>
> > > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > <patch-FactorDynamicLibrary-2.txt><br>
><br>
><br>
</div></div>> <patch-FactorDynamicLibrary-3.txt><br>
<br>
</blockquote></div><br></div>