PATCH for REVIEW: Implement Linux Host::FindProcesses()

This should fix bug #14541: http://llvm.org/bugs/show_bug.cgi?id=14541

I haven’t been able to run the python test script mentioned in the bug report yet, but “platform process list” in lldb looks correct on my Ubuntu 12.04 64-bit machine.

Also should fix a leaked file descriptor, and null pointer dereference when ReadProcPseudoFile() fails.

Diff is down below and also here: https://gist.github.com/mikesartain/5580617

Please yell at me with any feedback. Thanks!

-Mike

Index: source/Host/common/Host.cpp

I haven't been able to run the python test script mentioned in the bug
report yet, but "platform process list" in lldb looks correct on my Ubuntu
12.04 64-bit machine.

We got "./dotest.py -C clang -v -t -f
PlatformCommandTestCase.test_process_list" to run and it all appears to
pass now except this line in TestPlatformCommand.py:

    @expectedFailureLinux # due to bugzilla 14806 -- "platform status"

prints more information on Mac OS X than on Linux
    def test_status(self):
        self.expect("platform status",
            substrs = ['Platform', 'Triple', 'OS Version', 'Kernel',
'Hostname'])

platform status returns this on my machine:

(lldb) platform status

Linux 3.5.0-28-generic #48~precise1-Ubuntu SMP Wed Apr 24 21:42:24 UTC 2013

Not sure how this test should be modified to expect something along these
lines and also handle all the other Linux distros? I'll leave this to
someone else... :slight_smile:

I see - this looks like it has a separate bug filed for it:

http://llvm.org/bugs/show_bug.cgi?id=14806

PlatformLinux::GetStatus() in PlatformLinux.cpp just needs a little bit of
love. We'll take a look at that fixing that...

Hey Mike, thanks for the patch — keep up the good work :slight_smile:

I verified the implementation works as expected on Linux (Ubuntu 12.10), but I notice you're not filling in the process architecture. I believe there's an existing comment about why it's better to determine the arch in a different place, so it's probably fine to omit for now, but unless someone has any qualms, I will file a bug to also list the process' architectures when the user does "platform process list"; currently that field just shows "0".

In addition to fixing the platform command bug (functionalities/platform) this patch also fixes the attach-by-name test cases (python_api/hello_world and functionalities/process_attach)!

Committed in r181904, tests re-enabled in r181905.

Cheers,
Dan

Thank you much for checking and getting this in Daniel. Appreciate it.

-Mike

Great! If you're already working on figuring out the process architecture, I won't bother filing a bug about it.

Cheers,
Dan

The new function is down below. Running "platform process list" results in
something like this now:

...
5919 5916 mikesart 6 x86_64-pc-linux tee
5920 5917 mikesart 6 x86_64-pc-linux tee
5923 1 mikesart 6 x86_64-pc-linux wineserver
5933 1 mikesart 6 x86_64-pc-linux wine64-preloader
5939 1 mikesart 6 x86_64-pc-linux wine64-preloader
5947 1 mikesart 6 x86_64-pc-linux wine64-preloader
6004 1 mikesart 6 x86_64-pc-linux wine64-preloader
6049 1 mikesart 4 i386-pc-linux wine-preloader
6181 2852 mikesart 6 x86_64-pc-linux dash

Please fire away with any feedback and if it looks ok I'll try to commit it
later today.
Thanks much.
-Mike

static bool
GetELFProcessCPUType (const char *exe_path, ProcessInstanceInfo
&process_info)
{
    // First part of elf header structure copied from llvm/Support/ELF.h.
    // Read in just enough to get the machine type.
    struct Elf_Ehdr
    {
        unsigned char e_ident[llvm::elf::EI_NIDENT]; // ELF
Identification bytes
        uint16_t e_type; // Type of file
(see ET_* below)
        uint16_t e_machine; // Required
architecture for this file (see EM_*)

        bool checkMagic() const {
            return (memcmp(e_ident, llvm::elf::ElfMagic,
strlen(llvm::elf::ElfMagic))) == 0;
        }
        unsigned char getFileClass() const { return
e_ident[llvm::elf::EI_CLASS]; }
        unsigned char getDataEncoding() const { return
e_ident[llvm::elf::EI_DATA]; }
    } elfhdr;
    bool success = false;

    // Clear the architecture.
    process_info.GetArchitecture().Clear();

    // Open the binary and read the elf header.
    int fd = open(exe_path, O_RDONLY, 0);
    if (fd >= 0)
    {
        // Check that we read in enough bytes and the magic header lines up.
        int ret = read(fd, &elfhdr, sizeof(elfhdr));
        if (ret == sizeof(elfhdr) && elfhdr.checkMagic())
        {
            // Check elf version.
            int elfversion = elfhdr.e_ident[llvm::elf::EI_VERSION];
            if (elfversion == llvm::elf::EV_CURRENT)
            {
                // Check elf class.
                int elfclass = elfhdr.getFileClass();
                if (elfclass == llvm::elf::ELFCLASS32 || elfclass ==
llvm::elf::ELFCLASS64)
                {
                    // Check elf data encoding.
                    int elfdataencoding = elfhdr.getDataEncoding();
                    if (elfdataencoding == llvm::elf::ELFDATA2LSB ||
elfdataencoding == llvm::elf::ELFDATA2MSB)
                    {
                        int byte_size = (elfclass == llvm::elf::ELFCLASS32)
? 4 : 8;
                        lldb::ByteOrder byte_order = (elfdataencoding ==
llvm::elf::ELFDATA2LSB) ? eByteOrderLittle : eByteOrderBig;
                        lldb_private::DataExtractor data(&elfhdr,
sizeof(elfhdr), byte_order, byte_size);

                        uint16_t cpu;
                        lldb::offset_t offset = offsetof(Elf_Ehdr,
e_machine);
                        if (data.GetU16 (&offset, &cpu, 1))
                        {
                            // Set the machine type.
                            process_info.GetArchitecture ().SetArchitecture
(eArchTypeELF, cpu, LLDB_INVALID_CPUTYPE);
                            // 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);
                            success = true;
                        }
                    }
                }
            }
        }

        close (fd);
    }

    return success;
}

Hey Mike, looks good, but any reason to not put this implementation in ObjectFileELF (or re-use that class if possible)? I notice that ELFHeader::Parse() (see ELFHeader.cpp:108) seems to read some similar fields as your function below. If we can improve the ObjectFile plugin and re-use it instead of replicating its functionality, I think that would be preferable.

OTOH, if it is not possible to initialize an ObjectFile at the point where the CPU type is needed, or that is otherwise undesirable, I'm OK with having some minimal ELF functionality in the Linux host plugin.

Cheers,
Dan

The correct fix for this is to fill in the GetModuleSpecifications() in ObjectFileELF:

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)
{
    return 0;
}

Then, given a file, you can then call:

size_t
ObjectFile::GetModuleSpecifications (const FileSpec &file, lldb::offset_t file_offset, ModuleSpecList &specs);

With "file_offset" set to zero. It will return a ModuleSpecList with 1 entry and this will contain a ModuleSpec with the file and arch filled in. You can grab the arch from there, and also prepare for new executable types in the future.

Greg

FWIW, I was the one who put the comment in about getting the architecture elsewhere. I put it there when I implemented GetProcessInfo for the attach by pid case. In that situation, if I left it blank it was getting set later in the attach flow when ProcessPOSIX added the executable module to its module list. Since the executable image was already being parsed there, I didn't want to parse it again just before we got there.

I was also really hoping to avoid adding a third (or possibly fourth) bit of code to the LLVM/LLDB code base to read ELF headers, and I didn't have time to figure out a good way to get at one of the other bits of code that was doing so.

My point in all of this is to say, yes, go ahead and get the architecture there without worrying about what that comment meant.

-Andy

I had looked at the ObjectFileELF class, but it seemed a bit heavyweight
for just getting the file architecture (2 bytes in the elf header). When
someone does a "platform process list" this can get called on just about
every process the user is running. It looks like calling
ObjectFile::GetModuleSpecifications() reads 512 bytes of the file and then
tries to callback all the ObjectFile plug-ins until a match is found.

I'll implement the ObjectFileELF::GetModuleSpecifications() and see how
that looks though.

Thanks.
-Mike

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;
}

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

If we only support one ModuleSpec on Linux, maybe add:

assert(num_specs == 1 && "Linux plugin supports only a single
architecture");

To keep things explicit if/when we run across multi-arch binaries.
Otherwise, the changes look great; feel free to commit at your leisure!

Cheers,
Dan

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?

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.
-Mike

Index: source/Host/linux/Host.cpp

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

One thing I forgot to mention: the the modifications to ArchSpec.cpp should also be removed prior to submission.

if (error.Success())
{
ModuleSpec module_spec (resolved_exe_file, exe_arch);
if (exe_arch.IsValid())
{
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)
{
exe_module_sp.reset();
error.SetErrorStringWithFormat ("’%s’ doesn’t contain the architecture %s",
exe_file.GetPath().c_str(),
exe_arch.GetArchitectureName());
}
}
else
{
// No valid architecture was specified, ask the platform for
// the architectures that we should be using (in the correct order)
// and see if we can find a match that way
StreamString arch_names;
for (uint32_t idx = 0; GetSupportedArchitectureAtIndex (idx, module_spec.GetArchitecture()); ++idx)
{
error = ModuleList::GetSharedModule (module_spec,
exe_module_sp,
NULL,
NULL,
NULL);
// Did we find an executable using one of the
if (error.Success())
{
if (exe_module_sp && exe_module_sp->GetObjectFile())
break;
else
error.SetErrorToGenericError();
}

if (idx > 0)
arch_names.PutCString (", ");
arch_names.PutCString (module_spec.GetArchitecture().GetArchitectureName());
}

if (error.Fail() || !exe_module_sp)
{
error.SetErrorStringWithFormat ("’%s’ doesn’t contain any ‘%s’ platform architectures: %s",
exe_file.GetPath().c_str(),
GetPluginName().GetCString(),
arch_names.GetString().c_str());
}
}
}

> 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().

Sorry I wasn't clear - this issue has nothing to do with the code in Linux/Host.cpp. It's showing up because we've now implemented ObjectFileELF::GetModuleSpecifications().

It's returning x86_64 / UnknownVendor / UnknownOS for this target I'm trying to debug where it used to return nothing. Ie, invalid arch.

Again, this arch for the target comes from the main executable in the target. The main executable's arch is filled in from getting the process info right? So if we fix the process info so that the arch info is correct (it should return "x86_64-pc-linux" or whatever the llvm target triple is supposed to be for linux).

So in PlatformLinux.cpp, we are now running the exe_arch.IsValid() block down below (used to take the else clause).

m_arch in module_spec is:
    Arch = llvm::Triple::x86_64,
    Vendor = llvm::Triple::UnknownVendor,
    OS = llvm::Triple::Linux,
    Environment = llvm::Triple::GNU
exe_arch is:
    Arch = llvm::Triple::x86_64,
    Vendor = llvm::Triple::UnknownVendor,
    OS = llvm::Triple::UnknownOS,
    Environment = llvm::Triple::UnknownEnvironment

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? So you can use this in your code to with

if (!arch.TripleVendorWasSpecified())
{
    // Match any vendor because it wasn't specified
}

if (!arch.TripleOSWasSpecified())
{
    // Match any OS because it wasn't specified
}

I believe the comparison functions in ArchSpec do this (see ArchSpec::IsEqualTo()).

Getting there. :slight_smile: 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?

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.

Thanks Greg.
-Mike

Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp