Fix fetching the architecture of the target on process launch

[tberghammer]

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

[tberghammer]

Friendly ping.

[clayborg]

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

[tberghammer]

Updates based on suggestions

[tberghammer]

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]

Looks good.

lldb/trunk/include/lldb/Target/Target.h

public:
     ///     The new architecture.
     ///
     /// @return
     ///     \b true if the architecture was successfully set, \bfalse otherwise.
     //------------------------------------------------------------------
     bool
     SetArchitecture (const ArchSpec &arch_spec);
 
+    bool
+    MergeArchitecture (const ArchSpec &arch_spec);
+
     Debugger &
     GetDebugger ()
     {
         return m_debugger;
     }
 
     size_t
     ReadMemoryFromFileCache (const Address& addr, 

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (object_file)
                 if (log)
                     log->Printf("failed to connect to debugserver: %s", error.AsCString());
                 KillDebugserverProcess ();
                 return error;
             }
 
             if (m_gdb_comm.SendPacketAndWaitForResponse("?", 1, m_last_stop_packet, false) == GDBRemoteCommunication::PacketResult::Success)
             {
-                if (!m_target.GetArchitecture().IsValid()) 
-                {
-                    if (m_gdb_comm.GetProcessArchitecture().IsValid())
-                    {
-                        m_target.SetArchitecture(m_gdb_comm.GetProcessArchitecture());
-                    }
-                    else
-                    {
-                        m_target.SetArchitecture(m_gdb_comm.GetHostArchitecture());
-                    }
+                const ArchSpec &process_arch = m_gdb_comm.GetProcessArchitecture();
+
+                if (process_arch.IsValid())
+                    m_target.MergeArchitecture(process_arch);
+                    const ArchSpec &host_arch = m_gdb_comm.GetHostArchitecture();
+                    if (host_arch.IsValid())
+                        m_target.MergeArchitecture(host_arch);
                 }
 
                 SetPrivateState (SetThreadStopInfo (m_last_stop_packet));
                 
                 if (!disable_stdio)
                 {
                     if (pty.GetMasterFileDescriptor() != lldb_utility::PseudoTerminal::invalid_fd)
                         SetSTDIOFileDescriptor (pty.ReleaseMasterFileDescriptor());

lldb/trunk/source/Target/Target.cpp

else
                 SetExecutableModule (executable_sp, true);
                 return true;
             }
         }
     }
     return false;
 }
 
+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"
+
+            // 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;
+}
+
 void
 Target::WillClearList (const ModuleList& module_list)
 {
 }
 
 void
 Target::ModuleAdded (const ModuleList& module_list, const ModuleSP &module_sp)
 {