[Utility] Remove Triple{Environment,OS,Vendor}IsUnspecifiedUnknown from ArchSpec

[xiaobai]

Summary

These functions should always return the opposite of the Triple{Environment,OS,Vendor}WasSpecified functions. Unspecified unknown is the same as unspecified, which is why one set of functions should give us what we want. It's possible to have specified unknown, which is why we can't just rely on checking the enum values of vendor/os/environment. We must also ensure that the names of these are empty and not "unknown".

[JDevlieghere]

Do we actually need to check the string values in addition to the enum values? It looks like the llvm class tries pretty hard to make that the canonical way to query it. I know triples and arches are pretty tricky in lldb though, so maybe there's a reason that we can't.

[xiaobai]

In D58653#1409978, @JDevlieghere wrote:

Do we actually need to check the string values in addition to the enum values? It looks like the llvm class tries pretty hard to make that the canonical way to query it. I know triples and arches are pretty tricky in lldb though, so maybe there's a reason that we can't.

I wasn't sure when I was writing this, but I just tried and the tests seem to pass when we just check the enum values so I presume it's fine.

[xiaobai]

Remove redundant checks for triple components

[labath]

I think this basically defeats the purpose of the IsUnspecifiedUnknown functions, which was to differentiate between foo-unknown-bar and foo--bar triples. That in itself was a pretty big hack, but it seems that there is functionality which depends on this. There was a discussion about this back in december http://lists.llvm.org/pipermail/lldb-dev/2018-December/014437.html, and IIRC it converged to some sort of a conclusion, but I don't think there hasn't been any effort to try to implement it. I'm not sure what exactly it means about the future of this patch, but I'd be cautious about it.

[clayborg]

I seem to remember that we currently expect that there are two cases when it comes to unknowns:

• specified unknowns
• unspecific unknowns

A "specified unknown" is when the user actually typed "unknown" in their triple. IIRC there is no "none" to specify that there is no OS, so we use "specified unknowns" for this case. In this case we expect the enum _and_ the string accessor value for vendor and/or OS to be the string "unknown".

"unspecified unkowns" is when the user just types "arrmv7" as the triple. We need to know they didn't specify anything for vendor and OS. In this case the enums return unknown, but the string accessor return values are empty.

So we can't really change this code unless we add a "none" enum and string value to the LLVM triple system so we can tell the difference. Right now we rely on the above mechanism in a lot of places around the code.

[xiaobai]

In D58653#1410593, @clayborg wrote:

A "specified unknown" is when the user actually typed "unknown" in their triple. IIRC there is no "none" to specify that there is no OS, so we use "specified unknowns" for this case. In this case we expect the enum _and_ the string accessor value for vendor and/or OS to be the string "unknown".

This will work when the user types "armv7" as their triple, but when they type "armv7--linux" the vendor gets implicitly set to "unknown" even though the user didn't specify it.

In D58653#1410184, @labath wrote:

I think this basically defeats the purpose of the IsUnspecifiedUnknown functions, which was to differentiate between foo-unknown-bar and foo--bar triples. That in itself was a pretty big hack, but it seems that there is functionality which depends on this. There was a discussion about this back in december http://lists.llvm.org/pipermail/lldb-dev/2018-December/014437.html, and IIRC it converged to some sort of a conclusion, but I don't think there hasn't been any effort to try to implement it. I'm not sure what exactly it means about the future of this patch, but I'd be cautious about it.

Part of what you said is inconsistent with my understanding. Specifically, if you specify a Triple with the string "foo-unknown-bar" and another Triple with the string "foo--bar" you end up with the same triple. We rely on the llvm::Triple::normalize function to create triples from strings, and there is no way to indicate using this method that a triple has unspecified non-optional parts. We are relying on ArchSpec being created with a Triple object and not a StringRef for this behavior to work.

Yeah it does kind of defeat the purpose of some of these functions. I would like to change it to just IsUnknown (or just delete the functions entirely) because IsUnspecifiedUnknownshould always be the opposite of the WasSpecified functions. My understanding is that the llvm Triple class tries its best to make sure things are marked as the Unknown enum value if they are unspecified or marked as unknown, but there could be some strange edge case I'm not aware of.

Regardless, I would like to add these tests (or similar ones) to document the understanding of when vendors, OSes, and environments are considered unspecified or not.

[clayborg]

In D58653#1410907, @xiaobai wrote:

In D58653#1410593, @clayborg wrote:

A "specified unknown" is when the user actually typed "unknown" in their triple. IIRC there is no "none" to specify that there is no OS, so we use "specified unknowns" for this case. In this case we expect the enum _and_ the string accessor value for vendor and/or OS to be the string "unknown".

This will work when the user types "armv7" as their triple, but when they type "armv7--linux" the vendor gets implicitly set to "unknown" even though the user didn't specify it.

In D58653#1410184, @labath wrote:

I think this basically defeats the purpose of the IsUnspecifiedUnknown functions, which was to differentiate between foo-unknown-bar and foo--bar triples. That in itself was a pretty big hack, but it seems that there is functionality which depends on this. There was a discussion about this back in december http://lists.llvm.org/pipermail/lldb-dev/2018-December/014437.html, and IIRC it converged to some sort of a conclusion, but I don't think there hasn't been any effort to try to implement it. I'm not sure what exactly it means about the future of this patch, but I'd be cautious about it.

Part of what you said is inconsistent with my understanding. Specifically, if you specify a Triple with the string "foo-unknown-bar" and another Triple with the string "foo--bar" you end up with the same triple. We rely on the llvm::Triple::normalize function to create triples from strings, and there is no way to indicate using this method that a triple has unspecified non-optional parts. We are relying on ArchSpec being created with a Triple object and not a StringRef for this behavior to work.

The "llvm::Triple::normalize" must have been added at some point and I missed it. That will mess up some assumptions in LLDB that have been around for a while.

Yeah it does kind of defeat the purpose of some of these functions. I would like to change it to just IsUnknown (or just delete the functions entirely) because IsUnspecifiedUnknownshould always be the opposite of the WasSpecified functions. My understanding is that the llvm Triple class tries its best to make sure things are marked as the Unknown enum value if they are unspecified or marked as unknown, but there could be some strange edge case I'm not aware of.

Regardless, I would like to add these tests (or similar ones) to document the understanding of when vendors, OSes, and environments are considered unspecified or not.

I agree testing is good. The main things I would like to stay consistent are:

• if any part of a triple isn't specified, we need to be able to tell so that MergeFrom can merge in any unspecified parts (arch, env, os, vendor)
• if any part of a triple was specified, including "unknown" (since there is no "none" for OS or vendor), then it should remain as is during a MergeFrom

Target triples are used by many plug-ins to determine if they should load or not. The DynamicLoaderStatic is used for bare board development and relies on someone being able to specify "armv7-unknown-unknown" so that it knows the triple is has a specified "unknown" for OS and vendor. Then it loads all images at the address that they are in the file (load address == file address).

[clayborg]

I would be all for adding a "none" as a valid vendor, os or environment to the llvm triple class. Then all of the unspecified unknown stuff goes away. But I am not sure how likely the LLVM community will like this change since it won't be useful for non LLDB code.

[xiaobai]

In D58653#1410964, @clayborg wrote:

I agree testing is good. The main things I would like to stay consistent are:

• if any part of a triple isn't specified, we need to be able to tell so that MergeFrom can merge in any unspecified parts (arch, env, os, vendor)
• if any part of a triple was specified, including "unknown" (since there is no "none" for OS or vendor), then it should remain as is during a MergeFrom

Target triples are used by many plug-ins to determine if they should load or not. The DynamicLoaderStatic is used for bare board development and relies on someone being able to specify "armv7-unknown-unknown" so that it knows the triple is has a specified "unknown" for OS and vendor. Then it loads all images at the address that they are in the file (load address == file address).

In that case, I plan on doing the following:

• Removing the IsUnspecifiedUnknown functions.
• Adding more tests cases so we know if triple parsing changes later in a way that breaks us in the future.

[xiaobai]

Add more tests and rethink approach

[aprantl]

@clayborg I recently ran into a similar issue and I think that perhaps adding explicit llvm::Triple::Any{Vendor|OS|...} enumerators to llvm::Triple to make this distinction explicit would be the cleanest solution.

[xiaobai]

Fix typo

[clayborg]

In D58653#1411285, @aprantl wrote:

@clayborg I recently ran into a similar issue and I think that perhaps adding explicit llvm::Triple::Any{Vendor|OS|...} enumerators to llvm::Triple to make this distinction explicit would be the cleanest solution.

Any is fine, we just need a way to say "no os" or "no vendor" or "no environment". The way I am thinking about the way things would be:

If we had a "None":

• "Any" would be "unknown" (specified or unspecified)
• "None" would be the new enum/string

If we add "Any":

• "Any" would be the new enum/string
• "None" would be "unknown" (specified or unspecified)

Seems like less work to add the "None". Otherwise we end up having to change all much more triple stuff in LLVM because if we init a triple with "armv7", it currently defaults to unknown for os, vendor and env and that would need to change. Or all constructors in LLDB would need to change to call a different constructor that would force any unspecified parts to become "Any". Thoughts?
Not sure which is easier.

[clayborg]

Still issues from what I see. We need to know if something was specified or not and with your change we don't know the difference between the "none" state (which is current enum = unknown and string = "unknown") and the just not specified case (which is current enum = unknown and string = <empty>)

[clayborg]

Actually on re-reading the patch, we are detecting if anything wasn't set and setting it. Makes sense.

include/lldb/Utility/ArchSpec.h

bool IsValid() const {
     return m_core >= eCore_arm_generic && m_core < kNumCores;
   }
   explicit operator bool() const { return IsValid(); }
 
   bool TripleVendorWasSpecified() const {
     return !m_triple.getVendorName().empty();
   }
 
-  bool TripleVendorIsUnspecifiedUnknown() const {
-    return m_triple.getVendor() == llvm::Triple::UnknownVendor &&
-           m_triple.getVendorName().empty();
-  }
-
   bool TripleOSWasSpecified() const { return !m_triple.getOSName().empty(); }
 
   bool TripleEnvironmentWasSpecified() const {
-    return !m_triple.getEnvironmentName().empty();
-  }
-
-  bool TripleOSIsUnspecifiedUnknown() const {
-    return m_triple.getOS() == llvm::Triple::UnknownOS &&
-           m_triple.getOSName().empty();
+    return m_triple.hasEnvironment();
   }
 
   //------------------------------------------------------------------
   /// 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

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (!ParseHeader())
     return ArchSpec();
 
   if (m_section_headers.empty()) {
     // Allow elf notes to be parsed which may affect the detected architecture.
     ParseSectionHeaders();
   }
 
   if (CalculateType() == eTypeCoreFile &&
-      m_arch_spec.TripleOSIsUnspecifiedUnknown()) {
+      !m_arch_spec.TripleOSWasSpecified()) {
     // Core files don't have section headers yet they have PT_NOTE program
     // headers that might shed more light on the architecture
     for (const elf::ELFProgramHeader &H : ProgramHeaders()) {
       if (H.p_type != PT_NOTE || H.p_offset == 0 || H.p_filesz == 0)
         continue;
       DataExtractor data;
       if (data.SetData(m_data, H.p_offset, H.p_filesz) == H.p_filesz) {
         UUID uuid;

source/Utility/ArchSpec.cpp

 bool ArchSpec::ContainsOnlyArch(const llvm::Triple &normalized_triple) {
   return !normalized_triple.getArchName().empty() &&
          normalized_triple.getOSName().empty() &&
          normalized_triple.getVendorName().empty() &&
          normalized_triple.getEnvironmentName().empty();
 }
 
 void ArchSpec::MergeFrom(const ArchSpec &other) {
-  if (TripleVendorIsUnspecifiedUnknown() &&
-      !other.TripleVendorIsUnspecifiedUnknown())
+  if (!TripleVendorWasSpecified() && other.TripleVendorWasSpecified())
     GetTriple().setVendor(other.GetTriple().getVendor());
-  if (TripleOSIsUnspecifiedUnknown() && !other.TripleOSIsUnspecifiedUnknown())
+  if (!TripleOSWasSpecified() && other.TripleVendorWasSpecified())
     GetTriple().setOS(other.GetTriple().getOS());
   if (GetTriple().getArch() == llvm::Triple::UnknownArch) {
     GetTriple().setArch(other.GetTriple().getArch());
 
     // MachO unknown64 isn't really invalid as the debugger can still obtain
     // information from the binary, e.g. line tables. As such, we don't update
     // the core here.
     if (other.GetCore() != eCore_uknownMach64)
       UpdateCore();
   }
-  if (GetTriple().getEnvironment() == llvm::Triple::UnknownEnvironment &&
-      !TripleVendorWasSpecified()) {
+  if (!TripleEnvironmentWasSpecified() &&
+      other.TripleEnvironmentWasSpecified() && !TripleVenderWasSpecified()) {
     if (other.TripleVendorWasSpecified())
       GetTriple().setEnvironment(other.GetTriple().getEnvironment());
   }
   // If this and other are both arm ArchSpecs and this ArchSpec is a generic
   // "some kind of arm" spec but the other ArchSpec is a specific arm core,
   // adopt the specific arm core.
   if (GetTriple().getArch() == llvm::Triple::arm &&
       other.GetTriple().getArch() == llvm::Triple::arm &&

unittests/Utility/ArchSpecTest.cpp

TEST(ArchSpecTest, Compatibility) {
     ASSERT_TRUE(A.IsCompatibleMatch(B));
   }
 }
 
 TEST(ArchSpecTest, OperatorBool) {
   EXPECT_FALSE(ArchSpec());
   EXPECT_TRUE(ArchSpec("x86_64-pc-linux"));
 }
+
+TEST(ArchSpecTest, TripleComponentsWereSpecified) {
+  {
+    ArchSpec A("");
+    ArchSpec B("-");
+    ArchSpec C("--");
+    ArchSpec D("---");
+
+    ASSERT_FALSE(A.TripleVendorWasSpecified());
+    ASSERT_FALSE(A.TripleOSWasSpecified());
+    ASSERT_FALSE(A.TripleEnvironmentWasSpecified());
+
+    ASSERT_FALSE(B.TripleVendorWasSpecified());
+    ASSERT_FALSE(B.TripleOSWasSpecified());
+    ASSERT_FALSE(B.TripleEnvironmentWasSpecified());
+
+    ASSERT_FALSE(C.TripleVendorWasSpecified());
+    ASSERT_FALSE(C.TripleOSWasSpecified());
+    ASSERT_FALSE(C.TripleEnvironmentWasSpecified());
+
+    ASSERT_FALSE(D.TripleVendorWasSpecified());
+    ASSERT_FALSE(D.TripleOSWasSpecified());
+    ASSERT_FALSE(D.TripleEnvironmentWasSpecified());
+  }
+  {
+    // TODO: llvm::Triple::normalize treats the missing components from these
+    // triples as specified unknown components instead of unspecified
+    // components. We need to either change the behavior in llvm or work around
+    // this in lldb.
+    ArchSpec A("armv7");
+    ArchSpec B("armv7-");
+    ArchSpec C("armv7--");
+    ArchSpec D("armv7---");
+
+    ASSERT_FALSE(A.TripleVendorWasSpecified());
+    ASSERT_FALSE(A.TripleOSWasSpecified());
+    ASSERT_FALSE(A.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(B.TripleVendorWasSpecified());
+    ASSERT_FALSE(B.TripleOSWasSpecified());
+    ASSERT_FALSE(B.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(C.TripleVendorWasSpecified());
+    ASSERT_TRUE(C.TripleOSWasSpecified());
+    ASSERT_FALSE(C.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(D.TripleVendorWasSpecified());
+    ASSERT_TRUE(D.TripleOSWasSpecified());
+    ASSERT_TRUE(D.TripleEnvironmentWasSpecified());
+  }
+  {
+    ArchSpec A("x86_64-unknown");
+    ArchSpec B("powerpc-unknown-linux");
+    ArchSpec C("i386-pc-windows-msvc");
+    ArchSpec D("aarch64-unknown-linux-android");
+
+    ASSERT_TRUE(A.TripleVendorWasSpecified());
+    ASSERT_FALSE(A.TripleOSWasSpecified());
+    ASSERT_FALSE(A.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(B.TripleVendorWasSpecified());
+    ASSERT_TRUE(B.TripleOSWasSpecified());
+    ASSERT_FALSE(B.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(C.TripleVendorWasSpecified());
+    ASSERT_TRUE(C.TripleOSWasSpecified());
+    ASSERT_TRUE(C.TripleEnvironmentWasSpecified());
+
+    ASSERT_TRUE(D.TripleVendorWasSpecified());
+    ASSERT_TRUE(D.TripleOSWasSpecified());
+    ASSERT_TRUE(D.TripleEnvironmentWasSpecified());
+  }
+}