[lldb-dev] PATCH for REVIEW: Implement Linux Host::FindProcesses()

[Greg Clayton]

This looks good. The only thing I would change is change:

if (num_specs >= 1)

to be:

if (num_specs == 1)

in the GetELFProcessCPUType() function. If linux ever supports files that contain more than one architecture, we can't just select the first architecture slice, we would need to select the correct arch by inspecting the process a bit closer. On MacOSX, we have universal files that contain 32 and 64 bit copies of the executable, and we need to inspect the process to figure out which slice each process uses. On linux currently, all ELF files should contain only 1 spec, and the "if (num_specs == 1)" will ensure we select the one and only correct one.

Greg


On May 15, 2013, at 3:59 PM, Michael Sartain <mikesart at valvesoftware.com> wrote:

> On Wed, May 15, 2013 at 2:03 PM, Greg Clayton <gclayton at apple.com> wrote:
> The correct fix for this is to fill in the GetModuleSpecifications() in ObjectFileELF:
> 
> How does the below look? It's definitely cleaner. I stepped through all the code and performance seems more than acceptable as well.
> 
> Fire any feedback my way when you get a chance and I'll test it some more and hopefully get this in tomorrow assuming it's all ok.
> 
> Thanks!
>  -Mike
> 
> in linux/Host.cpp:
> 
> 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);
>     if (num_specs >= 1)
>     {
>         ModuleSpec module_spec;
>         if (specs.GetModuleSpecAtIndex (0, module_spec) && module_spec.GetArchitecture().IsValid())
>         {
>             process_info.GetArchitecture () = module_spec.GetArchitecture();
>             // SetArchitecture() in ArchSpec.cpp sets vendor and os to unknown. Reset them to PC and Linux.
>             process_info.GetArchitecture ().GetTriple().setVendor (llvm::Triple::PC);
>             process_info.GetArchitecture ().GetTriple().setOS (llvm::Triple::Linux);
>             return true;
>         }
>     }
>     return false;
> }
> 
> in ELF/ObjectFileELF.cpp:
> 
> 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,
>                                         lldb::DataBufferSP& data_sp,
>                                         lldb::offset_t data_offset,
>                                         lldb::offset_t file_offset,
>                                         lldb::offset_t length,
>                                         lldb_private::ModuleSpecList &specs)
> {
>     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;
> }