[Lldb-commits] [PATCH] Fix fetching the architecture of the target on process launch

[Greg Clayton]

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


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:942-956
@@ -941,13 +941,17 @@
             {
-                if (!m_target.GetArchitecture().IsValid()) 
+                if (m_gdb_comm.GetProcessArchitecture().IsValid())
+                {
+                    m_target.SetArchitecture(m_gdb_comm.GetProcessArchitecture());
+                }
+                else
                 {
-                    if (m_gdb_comm.GetProcessArchitecture().IsValid())
+                    if (m_target.GetArchitecture().IsValid())
                     {
-                        m_target.SetArchitecture(m_gdb_comm.GetProcessArchitecture());
+                        m_target.GetArchitecture().MergeFrom(m_gdb_comm.GetHostArchitecture());
                     }
                     else
                     {
                         m_target.SetArchitecture(m_gdb_comm.GetHostArchitecture());
                     }
                 }
 
----------------
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;
}
```

http://reviews.llvm.org/D8057

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/