This is an archive of the discontinued LLVM Phabricator instance.

Fix fetching the architecture of the target on process launch
ClosedPublic

Authored by tberghammer on Mar 4 2015, 6:17 AM.

Details

Summary

Fix fetching the architecture of the target on process launch

Previously it was fetched only if the architecture isn't valid, but the architecture can be valid without containing all information about the current target (e.g. missing os).

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer updated this revision to Diff 21195.Mar 4 2015, 6:17 AM
tberghammer retitled this revision from to Fix fetching the architecture of the target on process launch.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added reviewers: vharron, clayborg.
tberghammer added a subscriber: Unknown Object (MLST).

Friendly ping.

clayborg requested changes to this revision.Mar 11 2015, 10:24 AM
clayborg edited edge metadata.

This one worries me. Every time I have played with this in the past something has gone wrong. See my inline code suggestions above.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
942–956 ↗(On Diff #21195)

We need to change Target::GetArchitecture() to return a "const ArchSpec &" so no one can modify it like this. If you change the target's architecture you must call Target::SetArchitecture() so that the target can correctly manage its Modules. The target may have loaded incorrect files with the previous triple. For example on MacOSX, you can specify:

(lldb) target create --arch=x86_64 /path/to/a.out

The target now has many shared libraries loaded that are all x86_64. Now we attach to our process:

(lldb) process attach -n a.out

And lo and behold a.out is a universal file that has both x86_64 and i386 slices in it and the i386 slice is what we attached to. If we call Target::SetArchitecture("i386-apple-macosx") it will need to remove all old x86_64 files. So currently this is a bug that the target architecture has an accessor that is non-const and we should fix that.

Keeping this in mind we also want to listen to the user if the user fully specified a triple. So if the use says:

(lldb) target create --arch x86_64-unknown-unknown /path/to/a.out

We should honor this. So with this in mind this code should be:

const ArchSpec &target_arch = m_target.GetArchitecture();
const ArchSpec &process_arch = m_gdb_comm.GetProcessArchitecture();

if (process_arch.IsValid())
{
    m_target.MergeArchitecture(process_arch);
}
else
{
    const ArchSpec &host_arch = m_gdb_comm.GetHostArchitecture();
    if (host_arch.IsValid())
        m_target.MergeArchitecture(host_arch);
}

Then we need a new method in Target:

bool
Target::MergeArchitecture (const ArchSpec &arch_spec)
{
    if (arch_spec.IsValid())
    {
        if (m_arch.IsCompatibleMatch(arch_spec))
        {
            // The current target arch is compatible with "arch_spec", see if we
            // can improve our current architecture using bits from "arch_spec"
            if (m_arch.TripleOSWasSpecified() && m_arch.TripleVendorWasSpecified())
            {
                // Leave the target architecture alone it was fully specified (arch + vendor + OS)
                // by someone and we should trust the user
                return false; 
            }
            else
            {
                // Merge bits from arch_spec into "merged_arch" and set our architecture
                ArchSpec merged_arch (m_arch);
                merged_arch.MergeFrom (arch_spec);
                return SetArchitecture(merged_arch);
            }
        }
        else
        {
            // The new architecture is different, we just need to replace it
            return SetArchitecture(arch_spec);
        }
    }
    return false;
}
This revision now requires changes to proceed.Mar 11 2015, 10:24 AM
tberghammer edited edge metadata.

Updates based on suggestions

I updated the code based on your suggestions with one difference. In Target::MergeArchitecture I always do a merge if the current architecture and the specified architecture is compatible because if the architecture was fully specified then the MergeFrom is a no op and the check you suggested (checking for OS and Vendor) isn't sufficient because in some case (e.g. on android) we still have to update the environment part of the triple.

clayborg accepted this revision.Mar 12 2015, 9:43 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 12 2015, 9:43 AM
This revision was automatically updated to reflect the committed changes.