[lldb-dev] PATCH for REVIEW: Implement Linux Host::FindProcesses()
Greg Clayton
gclayton at apple.com
Thu May 16 13:00:23 PDT 2013
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 <gclayton at apple.com> wrote:
>
> On May 16, 2013, at 11:32 AM, Michael Sartain <mikesart at valvesoftware.com> wrote:
>
>> On Thu, May 16, 2013 at 10:39 AM, Malea, Daniel <daniel.malea at intel.com> 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
> lldb-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
More information about the lldb-dev
mailing list