One thing I forgot to mention: the the modifications to ArchSpec.cpp should also be removed prior to submission.
On May 16, 2013, at 11:50 AM, Greg Clayton <[email protected]> wrote: > > On May 16, 2013, at 11:32 AM, Michael Sartain <[email protected]> > wrote: > >> On Thu, May 16, 2013 at 10:39 AM, Malea, Daniel <[email protected]> >> wrote: >> If we only support one ModuleSpec on Linux, maybe add: >> >> assert(num_specs == 1 && "Linux plugin supports only a single >> architecture"); >> >> Yeah, that makes me feel more comfortable with that "== 1" check. It's in. >> >> I ran into an issue where TargetList::CreateTarget() was failing because >> it's calling ObjectFile::GetModuleSpecifications() and getting back x86_64 / >> UnknownVendor / UnknownOS and is expecting to find the Linux for the OS when >> it calls IsCompatibleMatch(). >> >> So I modified ArchSpec::SetArchitecture() to set the Linux OS using an ifdef >> in ArchSpec.cpp. Is this the best way to do this? > > No, I would do this by modifying all the architectures in ModuleSpec objects > that are returned from ObjectFile::GetModuleSpecifications() inside > GetELFProcessCPUType(). All we know by looking at an ELF file and grabbing > its specifications is that is has an architecture, we don't know what OS the > ELF file is destined for. But in GetELFProcessCPUType(), which might be > better named GetProcessCPUTypeFromExecutable() since it isn't ELF specific, > we know we have an executable file for the current host. So I would make your > function now with something like: > > > static bool > GetProcessCPUTypeFromExecutable (const char *exe_path, ProcessInstanceInfo > &process_info) > { > // Clear the architecture. > process_info.GetArchitecture().Clear(); > > ModuleSpecList specs; > FileSpec filespec (exe_path, false); > const size_t num_specs = ObjectFile::GetModuleSpecifications (filespec, 0, > specs); > assert(num_specs == 1 && "Linux plugin supports only a single > architecture"); > if (num_specs == 1) > { > ModuleSpec module_spec; > if (specs.GetModuleSpecAtIndex (0, module_spec) && > module_spec.GetArchitecture().IsValid()) > { > const ArchSpec &host_arch = Host::GetArchitecture > (eSystemDefaultArchitecture); > process_info.GetArchitecture () = module_spec.GetArchitecture(); > > if (process_info.GetArchitecture().IsValid()) > { > if (!process_info.GetArchitecture().TripleVendorWasSpecified()) > process_info.GetArchitecture().GetTriple().setVendorName > (host_arch.GetTriple().getVendorName()); > if (!process_info.GetArchitecture().TripleOSWasSpecified()) > process_info.GetArchitecture().GetTriple().setOSName > (host_arch.GetTriple().getOSName()); > } > else > { > process_info.GetArchitecture() = host_arch; > } > return true; > } > } > return false; > } > > >> >> I also got rid of the llvm::Triple::PC for vendor. This just seemed wrong as >> Linux can run on all kinds of platforms. I did write some code to get the >> distributor name from /etc/lsb-release, but the llvm triple doesn't line up >> with anything there of course. So for now it spits out >> "x86_64-unknown-linux". >> >> The final patch is down below. Let me know if this is all ok and I'll submit. >> >> Thanks again for all the help / feedback on everything. > > I think if we replace GetELFProcessCPUType() with the > GetProcessCPUTypeFromExecutable() from above, the patch should be good to go > after testing it to make sure it does what you want. > > Greg > >> -Mike >> >> Index: source/Host/linux/Host.cpp >> =================================================================== >> --- source/Host/linux/Host.cpp (revision 182033) >> +++ source/Host/linux/Host.cpp (working copy) >> @@ -18,6 +18,8 @@ >> // C++ Includes >> // Other libraries and framework includes >> // Project includes >> +#include "llvm/Support/ELF.h" >> + >> #include "lldb/Core/Error.h" >> #include "lldb/Target/Process.h" >> >> @@ -25,6 +27,9 @@ >> #include "lldb/Core/DataBufferHeap.h" >> #include "lldb/Core/DataExtractor.h" >> >> +#include "lldb/Core/ModuleSpec.h" >> +#include "lldb/Symbol/ObjectFile.h" >> + >> using namespace lldb; >> using namespace lldb_private; >> >> @@ -292,6 +297,29 @@ >> } >> >> static bool >> +GetELFProcessCPUType (const char *exe_path, ProcessInstanceInfo >> &process_info) >> +{ >> + // Clear the architecture. >> + process_info.GetArchitecture().Clear(); >> + >> + ModuleSpecList specs; >> + FileSpec filespec (exe_path, false); >> + const size_t num_specs = ObjectFile::GetModuleSpecifications (filespec, >> 0, specs); >> + assert(num_specs == 1 && "Linux plugin supports only a single >> architecture"); >> + if (num_specs == 1) >> + { >> + ModuleSpec module_spec; >> + if (specs.GetModuleSpecAtIndex (0, module_spec) && >> module_spec.GetArchitecture().IsValid()) >> + { >> + process_info.GetArchitecture () = module_spec.GetArchitecture(); >> + return true; >> + } >> + } >> + return false; >> +} >> + >> + >> +static bool >> GetProcessAndStatInfo (lldb::pid_t pid, ProcessInstanceInfo &process_info, >> ProcessStatInfo &stat_info, lldb::pid_t &tracerpid) >> { >> tracerpid = 0; >> @@ -299,9 +327,6 @@ >> ::memset (&stat_info, 0, sizeof(stat_info)); >> stat_info.ppid = LLDB_INVALID_PROCESS_ID; >> >> - // Architecture is intentionally omitted because that's better resolved >> - // in other places (see ProcessPOSIX::DoAttachWithID(). >> - >> // Use special code here because proc/[pid]/exe is a symbolic link. >> char link_path[PATH_MAX]; >> char exe_path[PATH_MAX] = ""; >> @@ -323,6 +348,10 @@ >> { >> exe_path[len - deleted_len] = 0; >> } >> + else >> + { >> + GetELFProcessCPUType (exe_path, process_info); >> + } >> >> process_info.SetProcessID(pid); >> process_info.GetExecutableFile().SetFile(exe_path, false); >> Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.h >> =================================================================== >> --- source/Plugins/ObjectFile/ELF/ObjectFileELF.h (revision 182033) >> +++ 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/ObjectFile/ELF/ObjectFileELF.cpp >> =================================================================== >> --- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (revision 182033) >> +++ 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/Core/ArchSpec.cpp >> =================================================================== >> --- source/Core/ArchSpec.cpp (revision 182033) >> +++ source/Core/ArchSpec.cpp (working copy) >> @@ -692,7 +692,11 @@ >> else >> { >> m_triple.setVendor (llvm::Triple::UnknownVendor); >> +#if defined(__linux__) >> + m_triple.setOS (llvm::Triple::Linux); >> +#else >> m_triple.setOS (llvm::Triple::UnknownOS); >> +#endif >> } >> // Fall back onto setting the machine type if the arch by >> name failed... >> if (m_triple.getArch () == llvm::Triple::UnknownArch) >> > > _______________________________________________ > 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
