Leave OS type and vendor as unspecified unknowns.

[ovyalov]

Summary

Leave OS type and vendor as unspecified unknowns - so it can filled in by merging with target or host architecture.

[tberghammer]

I don't really like the way we force some fix value into the triple on android. Do you know who is depending on this information to be filled in? I think it would be a better idea to change that function to accept unknown value both for OS and for Environment in case it isn't specified in the ELF file.

[ovyalov]

In D10080#180084, @tberghammer wrote:

I don't really like the way we force some fix value into the triple on android. Do you know who is depending on this information to be filled in? I think it would be a better idea to change that function to accept unknown value both for OS and for Environment in case it isn't specified in the ELF file.

The problem that I stumbled upon is qProcessInfo - response field ostype is set to 'unknown' and eventually host side cannot find an appropriate dynamic loader for such process, no modules are loaded.

[tberghammer]

I checked your change again and I don't understand why it have any effect at all. The "process_info.GetArchitecture().MergeFrom(HostInfo::GetArchitecture());" call should fill in all of these details from the HostInfo even without your change assuming HostInfo::GetArchitecture() returns the correct value.

If it isn't filled by the MergeFrom call then we should fix that one instead of adding one more merge layer with hard coded values.

[ovyalov]

In D10080#180220, @tberghammer wrote:

I checked your change again and I don't understand why it have any effect at all. The "process_info.GetArchitecture().MergeFrom(HostInfo::GetArchitecture());" call should fill in all of these details from the HostInfo even without your change assuming HostInfo::GetArchitecture() returns the correct value.

If it isn't filled by the MergeFrom call then we should fix that one instead of adding one more merge layer with hard coded values.

Presumably problem is in specified unknowns for os and environment set by ObjectFileELF::GetModuleSpecifications.
ArchSpec::MergeFrom updates os and environment only if corresponding item wasn't set in destination spec.
We may modify ObjectFileELF::GetModuleSpecifications to leave os and environment as unspecified unknowns if there is no explicit values for them.

[tberghammer]

In D10080#180231, @ovyalov wrote:

In D10080#180220, @tberghammer wrote:

I checked your change again and I don't understand why it have any effect at all. The "process_info.GetArchitecture().MergeFrom(HostInfo::GetArchitecture());" call should fill in all of these details from the HostInfo even without your change assuming HostInfo::GetArchitecture() returns the correct value.

If it isn't filled by the MergeFrom call then we should fix that one instead of adding one more merge layer with hard coded values.

Presumably problem is in specified unknowns for os and environment set by ObjectFileELF::GetModuleSpecifications.
ArchSpec::MergeFrom updates os and environment only if corresponding item wasn't set in destination spec.
We may modify ObjectFileELF::GetModuleSpecifications to leave os and environment as unspecified unknowns if there is no explicit values for them.

I would prefer that solution especially as the merge was added to handle this type of problems. For the first look I don't see any place where GetModuleSpecifications set os/environment to specified unknown but I might miss something.

[ovyalov]

Leave OS type as unspecified unknown for ELF binaries - so it can be filled in by merging with target or host architecture.

[clayborg]

Looks good.

[tberghammer]

LGTM (Do you know why we set the vendor to "defined unknown"?)

[ovyalov]

In D10080#180836, @tberghammer wrote:

LGTM (Do you know why we set the vendor to "defined unknown"?)

Hard to say - there are might be some historical reasons...
Potentially, we may leave it as unspecified unknown as well.

[clayborg]

We should prefer unspecified unknown for now for anything that isn't known. We really want this triple to be "<arch>-*-<ostype>" or "<arch>-*-*. We should, at some point, try to get a wildcard patch into the LLVM triple where we get AnyOS added to llvm::Triple::OSType and AnyVendor added to the and llvm::Triple::VendorType. This would then allow the triples to print nicely ( x86_64-*-* instead of "x86_64-unknown-unknown" and not being able to tell if the unknowns are specified or not.

[ovyalov]

According to Greg's comment removed setting os type and vendor type to unknown altogether - if this will cause problem on some architectures, we can narrow down the scope, let's say apply only to ELF.

[ovyalov]

AFFECTED FILES

/lldb/trunk/source/Core/ArchSpec.cpp

USERS

ovyalov (Author)

http://reviews.llvm.org/rL238623

source/Core/ArchSpec.cpp

if (arch_def)
                 m_core = core_def->core;
                 update_triple = false;
                 // Always use the architecture name because it might be more descriptive
                 // than the architecture enum ("armv7" -> llvm::Triple::arm).
                 m_triple.setArchName(llvm::StringRef(core_def->name));
                 if (arch_type == eArchTypeMachO)
                 {
                     m_triple.setVendor (llvm::Triple::Apple);
-
                     switch (core_def->machine)
                     {
                         case llvm::Triple::aarch64:
                         case llvm::Triple::arm:
                         case llvm::Triple::thumb:
                             m_triple.setOS (llvm::Triple::IOS);
                             break;
                             
                         case llvm::Triple::x86:
                         case llvm::Triple::x86_64:
                             // Don't set the OS for x86_64 or for x86 as we want to leave it as an "unspecified unknown"
                             // which means if we ask for the OS from the llvm::Triple we get back llvm::Triple::UnknownOS, but
                             // if we ask for the string value for the OS it will come back empty (unspecified).
                             // We do this because we now have iOS and MacOSX as the OS values for x86 and x86_64 for
                             // normal desktop and simulator binaries. And if we compare a "x86_64-apple-ios" to a "x86_64-apple-"
                             // triple, it will say it is compatible (because the OS is unspecified in the second one and will match
                             // anything in the first
                             break;
 
                         default:
                             m_triple.setOS (llvm::Triple::MacOSX);
                             break;
                     }
                 }
-                else
-                {
-                    m_triple.setVendor (llvm::Triple::UnknownVendor);
-                    m_triple.setOS (llvm::Triple::UnknownOS);
-                }
                 // Fall back onto setting the machine type if the arch by name failed...
                 if (m_triple.getArch () == llvm::Triple::UnknownArch)
                     m_triple.setArch (core_def->machine);
             }
         }
     }
     CoreUpdated(update_triple);
     return IsValid();