Fix load address resolution on Windows

[zturner]

Summary

When loading a module as a result of the "target create" command, there is the potential for ObjectFilePECOFF to stomp the triple. This can confuse later code which depends on the triple. Ultimately this fixes load address resolution on Windows because the correct triple is plumbed all the way through, so DynamicLoaderWindows can now correctly recognize that it should be used.

[clayborg]

Close. A couple changed as annotated in the source. There are two categories of unknown:

• specified "unknown" for the vendor or OS where the user actually typed "unknown" in the triple
• unspecified "unknown" for the vendor or OS where the user didn't specify it.

Add the quick fixes I specify above and this patch is good to go.

source/Core/Module.cpp

1314 ↗

(On Diff #18586)

We need to make sure the vendor wasn't specified as "unknown" by also calling:

bool ArchSpec::TripleVendorWasSpecified(). So the above line should be:

if (m_arch.GetTriple().getVendor() == llvm::Triple::UnknownVendor && m_arch.TripleVendorWasSpecified())

1316 ↗

(On Diff #18586)

Save thing as the previous, make sure "unknown" wasn't specified:

if (m_arch.GetTriple().getOS() == llvm::Triple::UnknownOS && m_arch.TripleOSWasSpecified())

[zturner]

Just to be clear, we should be checking that the triple vendor was NOT specified right? The code you wrote checks that it was specified.

Would it be worthwhile to make a function called ArchSpec::MergeFrom(const ArchSpec &other), which basically does what I've done here (with a comment explaining how it works)?

[zturner]

Fixed suggested issues, and moved the logic into a function in ArchSpec, since I thought it might be useful.

I reversed your conditional though because it looked like a thinko.

LMK if this looks good.

[clayborg]

Looks good

lldb/trunk/include/lldb/Core/ArchSpec.h

public:
 
     bool
     TripleOSWasSpecified() const
     {
         return !m_triple.getOSName().empty();
     }
 
     //------------------------------------------------------------------
+    /// Merges fields from another ArchSpec into this ArchSpec.
+    ///
+    /// This will use the supplied ArchSpec to fill in any fields of
+    /// the triple in this ArchSpec which were unspecified.  This can
+    /// be used to refine a generic ArchSpec with a more specific one.
+    /// For example, if this ArchSpec's triple is something like
+    /// i386-unknown-unknown-unknown, and we have a triple which is
+    /// x64-pc-windows-msvc, then merging that triple into this one
+    /// will result in the triple i386-pc-windows-msvc.
+    ///
+    //------------------------------------------------------------------
+    void
+    MergeFrom(const ArchSpec &other);
+
+    //------------------------------------------------------------------
     /// Sets this ArchSpec according to the given architecture name.
     ///
     /// The architecture name can be one of the generic system default
     /// values:
     ///
     /// @li \c LLDB_ARCH_DEFAULT - The arch the current system defaults
     ///        to when a program is launched without any extra
     ///        attributes or settings.

lldb/trunk/source/Core/ArchSpec.cpp

if (triple_cstr && triple_cstr[0])
             SetTriple (normalized_triple);
         }
     }
     else
         Clear();
     return IsValid();
 }
 
+void
+ArchSpec::MergeFrom(const ArchSpec &other)
+{
+    if (GetTriple().getVendor() == llvm::Triple::UnknownVendor && !TripleVendorWasSpecified())
+        GetTriple().setVendor(other.GetTriple().getVendor());
+    if (GetTriple().getOS() == llvm::Triple::UnknownOS && !TripleOSWasSpecified())
+        GetTriple().setOS(other.GetTriple().getOS());
+    if (GetTriple().getArch() == llvm::Triple::UnknownArch)
+        GetTriple().setArch(other.GetTriple().getArch());
+    if (GetTriple().getEnvironment() == llvm::Triple::UnknownEnvironment)
+        GetTriple().setEnvironment(other.GetTriple().getEnvironment());
+}
+
 bool
 ArchSpec::SetArchitecture (ArchitectureType arch_type, uint32_t cpu, uint32_t sub)
 {
     m_core = kCore_invalid;
     bool update_triple = true;
     const ArchDefinition *arch_def = FindArchDefinition(arch_type);
     if (arch_def)
     {

lldb/trunk/source/Core/Module.cpp

if (m_did_load_objfile == false)
             m_objfile_sp = ObjectFile::FindPlugin (shared_from_this(),
                                                    &m_file,
                                                    m_object_offset,
                                                    file_size - m_object_offset,
                                                    data_sp,
                                                    data_offset);
             if (m_objfile_sp)
             {
-                // Once we get the object file, update our module with the object file's 
+                // Once we get the object file, update our module with the object file's
                 // architecture since it might differ in vendor/os if some parts were
-                // unknown.
-                m_objfile_sp->GetArchitecture (m_arch);
+                // unknown.  But since the matching arch might already be more specific
+                // than the generic COFF architecture, only merge in those values that
+                // overwrite unspecified unknown values.
+                ArchSpec new_arch;
+                m_objfile_sp->GetArchitecture(new_arch);
+                m_arch.MergeFrom(new_arch);
             }
             else
             {
                 ReportError ("failed to load objfile for %s", GetFileSpec().GetPath().c_str());
             }
         }
     }
     return m_objfile_sp.get();

lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

if (ObjectFilePECOFF::MagicBytesMatch(data_sp))
             lldb::offset_t offset = dos_header.e_lfanew;
             uint32_t pe_signature = data.GetU32(&offset);
             if (pe_signature != IMAGE_NT_SIGNATURE)
                 return false;
             if (ParseCOFFHeader(data, &offset, coff_header))
             {
                 ArchSpec spec;
                 if (coff_header.machine == MachineAmd64)
+                {
                     spec.SetTriple("x86_64-pc-windows");
+                    specs.Append(ModuleSpec(file, spec));
+                }
                 else if (coff_header.machine == MachineX86)
+                {
                     spec.SetTriple("i386-pc-windows");
                     specs.Append(ModuleSpec(file, spec));
+                    spec.SetTriple("i686-pc-windows");
+                    specs.Append(ModuleSpec(file, spec));
+                }
             }
         }
     }
 
     return specs.GetSize() - initial_count;
 }