[lldb] Remove FileSpec::GetLastPathComponent

[bulbazord]

Summary

As far as I can tell, this just computes the filename of the FileSpec,
which is already conveniently stored in m_filename. We can use
FileSpec::GetFilename() instead.

[JDevlieghere]

LGTM

[mib]

@bulbazord What if the FileSpec is pointing to a directory instead of a file ? What would GetFilename return in that case compared to GetLastPathComponent ?

[bulbazord]

In D149663#4313112, @mib wrote:

@bulbazord What if the FileSpec is pointing to a directory instead of a file ? What would GetFilename return in that case compared to GetLastPathComponent ?

FileSpec chops everything up into a directory and a filename, even if it's pointing to a directory. For example:

fspec = lldb.SBFileSpec("/foo/bar/baz/")
print(fspec.GetDirectory())
print(fspec.GetFilename())

This will print "/foo/bar" followed by "baz". baz is clearly a directory, but FileSpec will treat it as the filename. GetLastPathComponent uses llvm::sys::path::filename to get the last element of the path, which is the exact same mechanism we use when constructing FileSpec's internal m_directory and m_filename in the first place.

[mib]

In D149663#4313123, @bulbazord wrote:

In D149663#4313112, @mib wrote:

@bulbazord What if the FileSpec is pointing to a directory instead of a file ? What would GetFilename return in that case compared to GetLastPathComponent ?

FileSpec chops everything up into a directory and a filename, even if it's pointing to a directory. For example:

fspec = lldb.SBFileSpec("/foo/bar/baz/")
print(fspec.GetDirectory())
print(fspec.GetFilename())

This will print "/foo/bar" followed by "baz". baz is clearly a directory, but FileSpec will treat it as the filename. GetLastPathComponent uses llvm::sys::path::filename to get the last element of the path, which is the exact same mechanism we use when constructing FileSpec's internal m_directory and m_filename in the first place.

Here's some more bike-shedding: This is not related to this patch specifically, but I don't think GetFilename()should return baz since it's not a file. Similarly, GetDirectory() should /foo/bar/baz/. Then in order to get the pointed file or directory I'd use GetLastPathComponent(). And to get the parent (/foo/bar), I think we should expose a new GetParent method. TBH, I find the current behavior very confusing.

[mib]

Given the current behavior, LGTM

lldb/include/lldb/Utility/FileSpec.h

public:
 
   /// Removes the last path component by replacing the current path with its
   /// parent. When the current path has no parent, this is a no-op.
   ///
   /// \return
   ///     A boolean value indicating whether the path was updated.
   bool RemoveLastPathComponent();
 
-  ConstString GetLastPathComponent() const;
-
 protected:
   // Convenience method for setting the file without changing the style.
   void SetFile(llvm::StringRef path);
 
   /// Called anytime m_directory or m_filename is changed to clear any cached
   /// state in this object.
   void PathWasModified() {
     m_is_resolved = false;

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (header.Parse(data, &header_offset)) {
                     spec.GetArchitecture().GetArchitectureName());
 
           if (!uuid.IsValid()) {
             uint32_t core_notes_crc = 0;
 
             if (!gnu_debuglink_crc) {
               LLDB_SCOPED_TIMERF(
                   "Calculating module crc32 %s with size %" PRIu64 " KiB",
-                  file.GetLastPathComponent().AsCString(),
+                  file.GetFilename().AsCString(),
                   (length - file_offset) / 1024);
 
               // For core files - which usually don't happen to have a
               // gnu_debuglink, and are pretty bulky - calculating whole
               // contents crc32 would be too much of luxury.  Thus we will need
               // to fallback to something simpler.
               if (header.e_type == llvm::ELF::ET_CORE) {
                 ProgramHeaderColl program_headers;

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

static UUID GetCoffUUID(llvm::object::COFFObjectFile &coff_obj) {
   // from .gnu_debuglink if it exists, just like how ObjectFileELF does it.
   if (!GetDebugLinkContents(coff_obj, gnu_debuglink_file, gnu_debuglink_crc)) {
     // If there is no .gnu_debuglink section, then this may be an object
     // containing DWARF debug info for .gnu_debuglink, so calculate the crc of
     // the object itself.
     auto raw_data = coff_obj.getData();
     LLDB_SCOPED_TIMERF(
         "Calculating module crc32 %s with size %" PRIu64 " KiB",
-        FileSpec(coff_obj.getFileName()).GetLastPathComponent().AsCString(),
+        FileSpec(coff_obj.getFileName()).GetFilename().AsCString(),
         static_cast<lldb::offset_t>(raw_data.size()) / 1024);
     gnu_debuglink_crc = llvm::crc32(0, llvm::arrayRefFromStringRef(raw_data));
   }
   // Use 4 bytes of crc from the .gnu_debuglink section.
   llvm::support::ulittle32_t data(gnu_debuglink_crc);
   return UUID(&data, sizeof(data));
 }
 
static llvm::Triple::EnvironmentType default_env = [] {
     return llvm::Triple::MSVC;
   }();
 
   // Check for a module-specific override.
   OptionValueSP module_env_option;
   const auto *map = GetGlobalPluginProperties().ModuleABIMap();
   if (map->GetNumValues() > 0) {
     // Step 1: Try with the exact file name.
-    auto name = file.GetLastPathComponent();
+    auto name = file.GetFilename();
     module_env_option = map->GetValueForKey(name);
     if (!module_env_option) {
       // Step 2: Try with the file name in lowercase.
       auto name_lower = name.GetStringRef().lower();
       module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
     }
     if (!module_env_option) {
       // Step 3: Try with the file name with ".debug" suffix stripped.

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

if (!module_sp && module_search_paths_ptr && platform_file) {
     // /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
     //
     // We'll need to look in the module_search_paths_ptr directories for both
     // "UIFoundation" and "UIFoundation.framework" -- most likely the latter
     // will be the one we find there.
 
     FileSpec platform_pull_upart(platform_file);
     std::vector<std::string> path_parts;
-    path_parts.push_back(
-        platform_pull_upart.GetLastPathComponent().AsCString());
+    path_parts.push_back(platform_pull_upart.GetFilename().AsCString());
     while (platform_pull_upart.RemoveLastPathComponent()) {
-      ConstString part = platform_pull_upart.GetLastPathComponent();
+      ConstString part = platform_pull_upart.GetFilename();
       path_parts.push_back(part.AsCString());
     }
     const size_t path_parts_size = path_parts.size();
 
     size_t num_module_search_paths = module_search_paths_ptr->GetSize();
     for (size_t i = 0; i < num_module_search_paths; ++i) {
       Log *log_verbose = GetLog(LLDBLog::Host);
       LLDB_LOGF(

lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp

void StructuredDataDarwinLog::ModulesDidLoad(Process &process,
   bool found_logging_support_module = false;
   for (size_t i = 0; i < module_list.GetSize(); ++i) {
     auto module_sp = module_list.GetModuleAtIndex(i);
     if (!module_sp)
       continue;
 
     auto &file_spec = module_sp->GetFileSpec();
     found_logging_support_module =
-        (file_spec.GetLastPathComponent() == logging_module_name);
+        (file_spec.GetFilename() == logging_module_name);
     if (found_logging_support_module)
       break;
   }
 
   if (!found_logging_support_module) {
     LLDB_LOGF(log,
               "StructuredDataDarwinLog::%s logging module %s "
               "has not yet been loaded, can't set a breakpoint "

lldb/source/Target/Platform.cpp

case fs::file_type::socket_file:
     // we have no way to copy pipes and sockets - ignore them and continue
     return FileSystem::eEnumerateDirectoryResultNext;
     break;
 
   case fs::file_type::directory_file: {
     // make the new directory and get in there
     FileSpec dst_dir = rc_baton->dst;
     if (!dst_dir.GetFilename())
-      dst_dir.SetFilename(src.GetLastPathComponent());
+      dst_dir.SetFilename(src.GetFilename());
     Status error = rc_baton->platform_ptr->MakeDirectory(
         dst_dir, lldb::eFilePermissionsDirectoryDefault);
     if (error.Fail()) {
       rc_baton->error.SetErrorStringWithFormat(
           "unable to setup directory %s on remote end",
           dst_dir.GetPath().c_str());
       return FileSystem::eEnumerateDirectoryResultQuit; // got an error, bail out
     }

lldb/source/Utility/FileSpec.cpp

FileSpec FileSpec::CopyByRemovingLastPathComponent() const {
   llvm::SmallString<64> current_path;
   GetPath(current_path, false);
   if (llvm::sys::path::has_parent_path(current_path, m_style))
     return FileSpec(llvm::sys::path::parent_path(current_path, m_style),
                     m_style);
   return *this;
 }
 
-ConstString FileSpec::GetLastPathComponent() const {
-  llvm::SmallString<64> current_path;
-  GetPath(current_path, false);
-  return ConstString(llvm::sys::path::filename(current_path, m_style));
-}
-
 void FileSpec::PrependPathComponent(llvm::StringRef component) {
   llvm::SmallString<64> new_path(component);
   llvm::SmallString<64> current_path;
   GetPath(current_path, false);
   llvm::sys::path::append(new_path,
                           llvm::sys::path::begin(current_path, m_style),
                           llvm::sys::path::end(current_path), m_style);
   SetFile(new_path, m_style);

lldb/source/Utility/XcodeSDK.cpp

case Type::Linux:
     return true;
   default:
     return false;
   }
 }
 
 bool XcodeSDK::SDKSupportsModules(XcodeSDK::Type desired_type,
                                   const FileSpec &sdk_path) {
-  ConstString last_path_component = sdk_path.GetLastPathComponent();
+  ConstString last_path_component = sdk_path.GetFilename();
 
   if (!last_path_component)
     return false;
 
   XcodeSDK sdk(last_path_component.GetStringRef().str());
   if (sdk.GetType() != desired_type)
     return false;
   return SDKSupportsModules(sdk.GetType(), sdk.GetVersion());