I agree with what you said, and the patches look good. A few inlined comments below...
On May 16, 2013, at 3:38 PM, Michael Sartain <[email protected]> wrote: > On Thu, May 16, 2013 at 1:28 PM, Greg Clayton <[email protected]> wrote: > > ModuleList::GetShareModule() fails and reports my exe doesn't contain > > "x86_64". Modifying the code in ArchSpec to return Linux isn't the correct > > solution, but what is? Something local here where if OS is Unknown we match > > any OS? > > Almost! There is "unknown unknown" (OS is unknown only because it wasn't > specified), and "specified unknown" (we use this to mean "no OS")). > > My patch used this feature: > > if (!process_info.GetArchitecture().TripleVendorWasSpecified()) > > > If you ask the ArchSpec::GetTriple().getOS(), it might return > "llvm::Triple::UnknownOS". If the string was actually specified as "unknown", > then "ArchSpec::GetTriple().getOSName()" will return a non-empty string > containing "unknown". > > Another way to fix this would be to have a "llvm::Triple::NoOS" enumeration. > There isn't one currently, so we currently test to see if the OS string is > empty to tell if the OS was specified. > > Does that make sense now? > > Getting there. :) I want to be really clear, so I'm going to break this up as > this issue involves the ObjectFileELF::GetModuleSpecifications() changes - we > can assume GetProcessCPUTypeFromExecutable() and friends don't even exist... > > Here is the trace of what is happening right now when it fails. > > calls TargetList::CreateTarget(), line 63 > - platform_arch defaults to unknown/unknown/unknown > calls ObjectFile::GetModuleSpecifications() > - only one arch, so platform_arch is set to it > (x86_64/unknownOS/UnknownEnv) > calls TargetList::CreateTarget, line 187 > - specified_arch == platform_arch == (x86_64/unknownOS/UnknownEnv) > calls Platform::ResolveExecutble() > - exe_arch == (x86_64/unknownOS/UnknownEnv) > - module_spec is intialized with exe_arch > if (exe_arch.IsValid()) is true so: > calls ModuleList::GetSharedModule(module_spec, ...) > > Given what you've said, I believe I need to add some code somewhere in that > call chain which checks for triple vendor not being specified. My guess is it > should be something like the below patch? Yep, the PlatformLinux needs to be able to deal with "unknown unknown" properly which your patch does do. > > Notes: > - I'm separating this from the Linux/Host.cpp changes. This will only be a > patch to implement ObjectFileELF::GetModuleSpecifications(). > - The TripleVendorWasSpecified() checks whether GetVendorName() is empty and > the string "unknown" isn't, so I'm explicitly checking for the Unknown enum > in the below code. The patch looks good. > Thanks Greg. > -Mike > > Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp > =================================================================== > --- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (revision 182038) > +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (working copy) > @@ -222,6 +222,18 @@ > return NULL; > } > > +bool > +ObjectFileELF::MagicBytesMatch (DataBufferSP& data_sp, > + lldb::addr_t data_offset, > + lldb::addr_t data_length) > +{ > + if (data_sp && data_sp->GetByteSize() > (llvm::ELF::EI_NIDENT + > data_offset)) > + { > + const uint8_t *magic = data_sp->GetBytes() + data_offset; > + return ELFHeader::MagicBytesMatch(magic); > + } > + return false; > +} > > size_t > ObjectFileELF::GetModuleSpecifications (const lldb_private::FileSpec& file, > @@ -231,7 +243,33 @@ > lldb::offset_t length, > lldb_private::ModuleSpecList &specs) > { > - return 0; > + const size_t initial_count = specs.GetSize(); > + > + if (ObjectFileELF::MagicBytesMatch(data_sp, 0, data_sp->GetByteSize())) > + { > + DataExtractor data; > + data.SetData(data_sp); > + elf::ELFHeader header; > + if (header.Parse(data, &data_offset)) > + { > + if (data_sp) > + { > + ModuleSpec spec; > + spec.GetFileSpec() = file; > + spec.GetArchitecture().SetArchitecture(eArchTypeELF, > + header.e_machine, > + LLDB_INVALID_CPUTYPE); > + if (spec.GetArchitecture().IsValid()) > + { > + // ObjectFileMachO adds the UUID here also, but that > isn't in the elf header > + // so we'd have to read the entire file in and calculate > the md5sum. > + // That'd be bad for this routine... > + specs.Append(spec); > + } > + } > + } > + } > + return specs.GetSize() - initial_count; > } > > //------------------------------------------------------------------ > Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.h > =================================================================== > --- source/Plugins/ObjectFile/ELF/ObjectFileELF.h (revision 182038) > +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.h (working copy) > @@ -65,6 +65,12 @@ > lldb::offset_t file_offset, > lldb::offset_t length, > lldb_private::ModuleSpecList &specs); > + > + static bool > + MagicBytesMatch (lldb::DataBufferSP& data_sp, > + lldb::addr_t offset, > + lldb::addr_t length); > + > //------------------------------------------------------------------ > // PluginInterface protocol > //------------------------------------------------------------------ > Index: source/Plugins/Platform/Linux/PlatformLinux.cpp > =================================================================== > --- source/Plugins/Platform/Linux/PlatformLinux.cpp (revision 182038) > +++ source/Plugins/Platform/Linux/PlatformLinux.cpp (working copy) > @@ -208,6 +208,29 @@ > NULL, > NULL, > NULL); > + if (error.Fail()) > + { > + // If we failed, it may be because the vendor and os aren't > known. If that is the > + // case, try setting them to the host architecture and give > it another try. > + llvm::Triple &module_triple = > module_spec.GetArchitecture().GetTriple(); > + bool is_vendor_specified = (module_triple.getVendor() != > llvm::Triple::UnknownVendor); > + bool is_os_specified = (module_triple.getOS() != > llvm::Triple::UnknownOS); > + if (!is_vendor_specified || !is_os_specified) > + { > + const llvm::Triple &host_triple = Host::GetArchitecture > (Host::eSystemDefaultArchitecture).GetTriple(); > + > + if (!is_vendor_specified) > + module_triple.setVendorName > (host_triple.getVendorName()); > + if (!is_os_specified) > + module_triple.setOSName (host_triple.getOSName()); > + > + error = ModuleList::GetSharedModule (module_spec, > + exe_module_sp, > + NULL, > + NULL, > + NULL); > + } > + } > > // TODO find out why exe_module_sp might be NULL > if (!exe_module_sp || exe_module_sp->GetObjectFile() == NULL) > _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
