Refactor ObjectFile::GetSDKVersion

[teemperor]

Summary

This patch modernizes the GetSDKVersion API and hopefully prevents problems such as the ones discovered in D61218.

[teemperor]

• Added documentation on the return value.

[teemperor]

I decided to go for a vector because otherwise the implementation in ObjectFileMachO gets a bit awkward. But I have no problem with doing a VersionTuple instead and adding some manual translation to version tuple in ObjectFileMachO.

[labath]

In D61233#1481527, @teemperor wrote:

I decided to go for a vector because otherwise the implementation in ObjectFileMachO gets a bit awkward. But I have no problem with doing a VersionTuple instead and adding some manual translation to version tuple in ObjectFileMachO.

I think it would be better to use a VersionTuple, for consistency with other version uses. I don't see why the MachO implementation should be awkward because of that. You'd just need to replace the three push_back lines with m_sdk_version = VersionTuple(xxxx, yy, zz);. In fact, if at the same time you rewrite the GetSDKVersion into something like:

if (m_sdk_version.empty())
  m_sdk_version = GetVersionFromLC_VERSION());
if (m_sdk_version.empty())
  m_sdk_version = GetVersionFromLC_BUILD_VERSION());
return m_sdk_version;

that code will become at least five times more readable than it is now. :)

[aprantl]

Awesome. I should have done that right away. This isn't super important, but I feel like just returning a vector (with potentially zero elements would be just as good) and the Optional doesn't add much value.

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp

211–212

For my own education, why is this preferred over versions[0]? Because of Optional's operator->?

[teemperor]

• Rewrote patch to use llvm::VersionTuple (thanks Pavel)

[teemperor]

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp

211–212

Yeah, just preferred that style to (*versions)[0] :)

[clayborg]

lgtm

[labath]

looks good to me too...

lldb/include/lldb/Symbol/ObjectFile.h

public:
   ///     llvm::VersionTuple. In case of error an empty VersionTuple is
   ///     returned.
   virtual llvm::VersionTuple GetMinimumOSVersion() {
     return llvm::VersionTuple();
   }
 
   /// Get the SDK OS version this object file was built with.
   ///
-  /// The versions arguments and returns values are the same as the
-  /// GetMinimumOSVersion()
-  virtual uint32_t GetSDKVersion(uint32_t *versions, uint32_t num_versions) {
-    if (versions && num_versions) {
-      for (uint32_t i = 0; i < num_versions; ++i)
-        versions[i] = UINT32_MAX;
-    }
-    return 0;
-  }
+  /// \return
+  ///     This function returns extracted version numbers as a
+  ///     llvm::VersionTuple. In case of error an empty VersionTuple is
+  ///     returned.
+  virtual llvm::VersionTuple GetSDKVersion() { return llvm::VersionTuple(); }
 
   /// Return true if this file is a dynamic link editor (dyld)
   ///
   /// Often times dyld has symbols that mirror symbols in libc and other
   /// shared libraries (like "malloc" and "free") and the user does _not_ want
   /// to stop in these shared libraries by default. We can ask the ObjectFile
   /// if it is such a file and should be avoided for things like settings
   /// breakpoints and doing function lookups for expressions.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

public:
   ObjectFile::Type CalculateType() override;
 
   ObjectFile::Strata CalculateStrata() override;
 
   llvm::VersionTuple GetVersion() override;
 
   llvm::VersionTuple GetMinimumOSVersion() override;
 
-  uint32_t GetSDKVersion(uint32_t *versions, uint32_t num_versions) override;
+  llvm::VersionTuple GetSDKVersion() override;
 
   bool GetIsDynamicLinkEditor() override;
 
   static bool ParseHeader(lldb_private::DataExtractor &data,
                           lldb::offset_t *data_offset_ptr,
                           llvm::MachO::mach_header &header);
 
   bool AllowAssemblyEmulationUnwindPlans() override;
protected:
   static lldb_private::ConstString GetSegmentNameLINKEDIT();
   static lldb_private::ConstString GetSegmentNameDWARF();
   static lldb_private::ConstString GetSectionNameEHFrame();
 
   llvm::MachO::dysymtab_command m_dysymtab;
   std::vector<llvm::MachO::segment_command_64> m_mach_segments;
   std::vector<llvm::MachO::section_64> m_mach_sections;
   llvm::Optional<llvm::VersionTuple> m_min_os_version;
-  std::vector<uint32_t> m_sdk_versions;
+  llvm::Optional<llvm::VersionTuple> m_sdk_versions;
   typedef lldb_private::RangeVector<uint32_t, uint32_t> FileRangeArray;
   lldb_private::Address m_entry_point_address;
   FileRangeArray m_thread_context_offsets;
   bool m_thread_context_offsets_valid;
   lldb_private::FileSpecList m_reexported_dylibs;
   bool m_allow_assembly_emulation_unwind_plans;
 };
 
 #endif // liblldb_ObjectFileMachO_h_

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

if (!m_min_os_version) {
       // Set version to an empty value so we don't keep trying to
       m_min_os_version = llvm::VersionTuple();
     }
   }
 
   return *m_min_os_version;
 }
 
-uint32_t ObjectFileMachO::GetSDKVersion(uint32_t *versions,
-                                        uint32_t num_versions) {
-  if (m_sdk_versions.empty()) {
+llvm::VersionTuple ObjectFileMachO::GetSDKVersion() {
+  if (!m_sdk_versions.hasValue()) {
     lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
-    bool success = false;
-    for (uint32_t i = 0; !success && i < m_header.ncmds; ++i) {
+    for (uint32_t i = 0; i < m_header.ncmds; ++i) {
       const lldb::offset_t load_cmd_offset = offset;
 
       version_min_command lc;
       if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
         break;
       if (lc.cmd == llvm::MachO::LC_VERSION_MIN_MACOSX ||
           lc.cmd == llvm::MachO::LC_VERSION_MIN_IPHONEOS ||
           lc.cmd == llvm::MachO::LC_VERSION_MIN_TVOS ||
           lc.cmd == llvm::MachO::LC_VERSION_MIN_WATCHOS) {
         if (m_data.GetU32(&offset, &lc.version,
                           (sizeof(lc) / sizeof(uint32_t)) - 2)) {
           const uint32_t xxxx = lc.sdk >> 16;
           const uint32_t yy = (lc.sdk >> 8) & 0xffu;
           const uint32_t zz = lc.sdk & 0xffu;
           if (xxxx) {
-            m_sdk_versions.push_back(xxxx);
-            m_sdk_versions.push_back(yy);
-            m_sdk_versions.push_back(zz);
-            success = true;
+            m_sdk_versions = llvm::VersionTuple(xxxx, yy, zz);
+            break;
           } else {
             GetModule()->ReportWarning(
                 "minimum OS version load command with invalid (0) version found.");
           }
         }
       }
       offset = load_cmd_offset + lc.cmdsize;
     }
 
-    if (!success) {
+    if (!m_sdk_versions.hasValue()) {
       offset = MachHeaderSizeFromMagic(m_header.magic);
-      for (uint32_t i = 0; !success && i < m_header.ncmds; ++i) {
+      for (uint32_t i = 0; i < m_header.ncmds; ++i) {
         const lldb::offset_t load_cmd_offset = offset;
 
         version_min_command lc;
         if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
           break;
         if (lc.cmd == llvm::MachO::LC_BUILD_VERSION) {
           // struct build_version_command {
           //     uint32_t    cmd;            /* LC_BUILD_VERSION */
if (!m_sdk_versions.hasValue()) {
 
           offset += 4; // skip platform
           uint32_t minos = m_data.GetU32(&offset);
 
           const uint32_t xxxx = minos >> 16;
           const uint32_t yy = (minos >> 8) & 0xffu;
           const uint32_t zz = minos & 0xffu;
           if (xxxx) {
-            m_sdk_versions.push_back(xxxx);
-            m_sdk_versions.push_back(yy);
-            m_sdk_versions.push_back(zz);
-            success = true;
+            m_sdk_versions = llvm::VersionTuple(xxxx, yy, zz);
+            break;
           }
         }
         offset = load_cmd_offset + lc.cmdsize;
       }
     }
 
-    if (!success) {
-      // Push an invalid value so we don't try to find
-      // the version # again on the next call to this
-      // method.
-      m_sdk_versions.push_back(UINT32_MAX);
-    }
+    if (!m_sdk_versions.hasValue())
+      m_sdk_versions = llvm::VersionTuple();
   }
 
-  // Legitimate version numbers will have 3 entries pushed
-  // on to m_sdk_versions.  If we only have one value, it's
-  // the sentinel value indicating that this object file
-  // does not have a valid minimum os version #.
-  if (m_sdk_versions.size() > 1) {
-    if (versions != nullptr && num_versions > 0) {
-      for (size_t i = 0; i < num_versions; ++i) {
-        if (i < m_sdk_versions.size())
-          versions[i] = m_sdk_versions[i];
-        else
-          versions[i] = 0;
-      }
-    }
-    return m_sdk_versions.size();
-  }
-  // Call the superclasses version that will empty out the data
-  return ObjectFile::GetSDKVersion(versions, num_versions);
+  return m_sdk_versions.getValue();
 }
 
 bool ObjectFileMachO::GetIsDynamicLinkEditor() {
   return m_header.filetype == llvm::MachO::MH_DYLINKER;
 }
 
 bool ObjectFileMachO::AllowAssemblyEmulationUnwindPlans() {
   return m_allow_assembly_emulation_unwind_plans;

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp

 ConstString PlatformMacOSX::GetSDKDirectory(lldb_private::Target &target) {
   ModuleSP exe_module_sp(target.GetExecutableModule());
   if (exe_module_sp) {
     ObjectFile *objfile = exe_module_sp->GetObjectFile();
     if (objfile) {
       std::string xcode_contents_path;
       std::string default_xcode_sdk;
       FileSpec fspec;
-      uint32_t versions[2];
-      if (objfile->GetSDKVersion(versions, 2)) {
+      llvm::VersionTuple version = objfile->GetSDKVersion();
+      if (!version.empty()) {
         fspec = HostInfo::GetShlibDir();
         if (fspec) {
           std::string path;
           xcode_contents_path = fspec.GetPath();
           size_t pos = xcode_contents_path.find("/Xcode.app/Contents/");
           if (pos != std::string::npos) {
             // LLDB.framework is inside an Xcode app bundle, we can locate the
             // SDK from here
if (objfile) {
             }
           }
         }
 
         if (!xcode_contents_path.empty()) {
           StreamString sdk_path;
           sdk_path.Printf("%sDeveloper/Platforms/MacOSX.platform/Developer/"
                           "SDKs/MacOSX%u.%u.sdk",
-                          xcode_contents_path.c_str(), versions[0],
-                          versions[1]);
+                          xcode_contents_path.c_str(), version.getMajor(),
+                          version.getMinor().getValue());
           fspec.SetFile(sdk_path.GetString(), FileSpec::Style::native);
           if (FileSystem::Instance().Exists(fspec))
             return ConstString(sdk_path.GetString());
         }
 
         if (!default_xcode_sdk.empty()) {
           fspec.SetFile(default_xcode_sdk, FileSpec::Style::native);
           if (FileSystem::Instance().Exists(fspec))