[FileSystem] Remove GetByteSize() from FileSpec

[JDevlieghere]

Summary

This patch removes the GetByteSize method from FileSpec and updates its uses with calls to the FileSystem.

[davide]

LGTM

[zturner]

I always wondered if we actually even need methods like this in FileSystem given that they already exist in llvm::sys::fs. Is it possible to just call the llvm methods directly, or is it still helpful to call the ones in FileSystem so we can more transparently interoperate with the VFS layer somehow?

[JDevlieghere]

In D53788#1278036, @zturner wrote:

I always wondered if we actually even need methods like this in FileSystem given that they already exist in llvm::sys::fs. Is it possible to just call the llvm methods directly, or is it still helpful to call the ones in FileSystem so we can more transparently interoperate with the VFS layer somehow?

This particular function uses the VFS so it has to go through the FileSystem class. Personally I feel we should do the same for functions that don't, because then it's clear and simple (i.e. everything uses FileSystem) and we eliminate potential bugs where someone uses the llvm::sys::fs while it should've actually gone through the VFS.

[shafik]

Are we refactoring in the right place? Why not just refactor FileSpec::GetByteSize() why is FileSpec the wrong place?

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

718 ↗

(On Diff #171383)

1024u just for consistency, I don't expect this is practically a problem.

[zturner]

In D53788#1279096, @shafik wrote:

Are we refactoring in the right place? Why not just refactor FileSpec::GetByteSize() why is FileSpec the wrong place?

There was another review thread where we discussed this just the other day. Basically, It took a herculean effort to get FileSpec into the Utility library, which helped greatly with layering and we don't want to undo this. In order to for GetByteSize to be able to use the VFS, it has to be able to use FileSystem, so either FileSpec has to move out of Utility, FileSystem has to move into Utility, or GetByteSize has to move out of FileSpec. FileSystem moving into Utility might actually be a worthwhile goal long term, but even putting that aside, I think FileSpec should just be a thing for manipulating paths. For example, it has the notion of manipulating paths which might not even make sense on the given host platform (e.g. it has a member variable which represents the path *syntax). You might be on a Windows host debugging a remove Linux target, for example, and your host needs to have some way to manipulate paths on the remote which use a non-host syntax. That's what FileSpec is supposed to be. It just deals with strings.

lldb/trunk/include/lldb/Core/ModuleSpec.h

 //===-- ModuleSpec.h --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_ModuleSpec_h_
 #define liblldb_ModuleSpec_h_
 
 // Project includes
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/UUID.h"
 
 // Other libraries and framework includes
 #include "llvm/Support/Chrono.h"
 
 // C Includes
 // C++ Includes
 #include <mutex>
 #include <vector>
 
 namespace lldb_private {
 
 class ModuleSpec {
 public:
   ModuleSpec()
       : m_file(), m_platform_file(), m_symbol_file(), m_arch(), m_uuid(),
         m_object_name(), m_object_offset(0), m_object_size(0),
         m_source_mappings() {}
 
-  ModuleSpec(const FileSpec &file_spec, const UUID& uuid = UUID())
+  ModuleSpec(const FileSpec &file_spec, const UUID &uuid = UUID())
       : m_file(file_spec), m_platform_file(), m_symbol_file(), m_arch(),
         m_uuid(uuid), m_object_name(), m_object_offset(0),
-        m_object_size(file_spec.GetByteSize()), m_source_mappings() {}
+        m_object_size(FileSystem::Instance().GetByteSize(file_spec)),
+        m_source_mappings() {}
 
   ModuleSpec(const FileSpec &file_spec, const ArchSpec &arch)
       : m_file(file_spec), m_platform_file(), m_symbol_file(), m_arch(arch),
         m_uuid(), m_object_name(), m_object_offset(0),
-        m_object_size(file_spec.GetByteSize()), m_source_mappings() {}
+        m_object_size(FileSystem::Instance().GetByteSize(file_spec)),
+        m_source_mappings() {}
 
   ModuleSpec(const ModuleSpec &rhs)
       : m_file(rhs.m_file), m_platform_file(rhs.m_platform_file),
         m_symbol_file(rhs.m_symbol_file), m_arch(rhs.m_arch),
         m_uuid(rhs.m_uuid), m_object_name(rhs.m_object_name),
         m_object_offset(rhs.m_object_offset), m_object_size(rhs.m_object_size),
         m_object_mod_time(rhs.m_object_mod_time),
         m_source_mappings(rhs.m_source_mappings) {}

lldb/trunk/include/lldb/Utility/FileSpec.h

public:
 
   //------------------------------------------------------------------
   /// Canonicalize this file path (basically running the static
   /// FileSpec::Resolve method on it). Useful if you asked us not to resolve
   /// the file path when you set the file.
   //------------------------------------------------------------------
   bool ResolvePath();
 
-  uint64_t GetByteSize() const;
-
   Style GetPathStyle() const;
 
   //------------------------------------------------------------------
   /// Directory string get accessor.
   ///
   /// @return
   ///     A reference to the directory string object.
   //------------------------------------------------------------------

lldb/trunk/source/Core/Module.cpp

ObjectFile *Module::GetObjectFile() {
   if (!m_did_load_objfile.load()) {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
     if (!m_did_load_objfile.load()) {
       static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
       Timer scoped_timer(func_cat, "Module::GetObjectFile () module = %s",
                          GetFileSpec().GetFilename().AsCString(""));
       DataBufferSP data_sp;
       lldb::offset_t data_offset = 0;
-      const lldb::offset_t file_size = m_file.GetByteSize();
+      const lldb::offset_t file_size =
+          FileSystem::Instance().GetByteSize(m_file);
       if (file_size > m_object_offset) {
         m_did_load_objfile = true;
         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 architecture since it might differ in vendor/os if some

lldb/trunk/source/Host/common/Host.cpp

 
 #if defined(__NetBSD__)
 #include <lwp.h>
 #endif
 
 // C++ Includes
 #include <csignal>
 
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/MonitoringProcessLauncher.h"
 #include "lldb/Host/ProcessLauncher.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Target/FileAction.h"
if (!shell_info_sp->process_reaped.WaitForValueEqualTo(true, timeout)) {
       if (status_ptr)
         *status_ptr = shell_info_sp->status;
 
       if (signo_ptr)
         *signo_ptr = shell_info_sp->signo;
 
       if (command_output_ptr) {
         command_output_ptr->clear();
-        uint64_t file_size = output_file_spec.GetByteSize();
+        uint64_t file_size =
+            FileSystem::Instance().GetByteSize(output_file_spec);
         if (file_size > 0) {
           if (file_size > command_output_ptr->max_size()) {
             error.SetErrorStringWithFormat(
                 "shell command output is too large to fit into a std::string");
           } else {
             auto Buffer =
                 DataBufferLLVM::CreateFromPath(output_file_spec.GetPath());
             if (error.Success())

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

 #include <cassert>
 #include <unordered_map>
 
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/DWARFCallFrameInfo.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Status.h"
if (header.Parse(data, &header_offset)) {
             uint32_t core_notes_crc = 0;
 
             if (!gnu_debuglink_crc) {
               static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
               lldb_private::Timer scoped_timer(
                   func_cat,
                   "Calculating module crc32 %s with size %" PRIu64 " KiB",
                   file.GetLastPathComponent().AsCString(),
-                  (file.GetByteSize() - file_offset) / 1024);
+                  (FileSystem::Instance().GetByteSize(file) - 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;
                 GetProgramHeaderInfo(program_headers, data, header);

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
   if (!dwo_file.Exists())
     return nullptr;
 
   const lldb::offset_t file_offset = 0;
   DataBufferSP dwo_file_data_sp;
   lldb::offset_t dwo_file_data_offset = 0;
   ObjectFileSP dwo_obj_file = ObjectFile::FindPlugin(
       GetObjectFile()->GetModule(), &dwo_file, file_offset,
-      dwo_file.GetByteSize(), dwo_file_data_sp, dwo_file_data_offset);
+      FileSystem::Instance().GetByteSize(dwo_file), dwo_file_data_sp,
+      dwo_file_data_offset);
   if (dwo_obj_file == nullptr)
     return nullptr;
 
   return llvm::make_unique<SymbolFileDWARFDwo>(dwo_obj_file, &dwarf_cu);
 }
 
 void SymbolFileDWARF::UpdateExternalModuleListIfNeeded() {
   if (m_fetched_external_modules)

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp

 
 std::unique_ptr<SymbolFileDWARFDwp>
 SymbolFileDWARFDwp::Create(lldb::ModuleSP module_sp,
                            const lldb_private::FileSpec &file_spec) {
   const lldb::offset_t file_offset = 0;
   lldb::DataBufferSP file_data_sp;
   lldb::offset_t file_data_offset = 0;
   lldb::ObjectFileSP obj_file = lldb_private::ObjectFile::FindPlugin(
-      module_sp, &file_spec, file_offset, file_spec.GetByteSize(), file_data_sp,
+      module_sp, &file_spec, file_offset,
+      lldb_private::FileSystem::Instance().GetByteSize(file_spec), file_data_sp,
       file_data_offset);
   if (obj_file == nullptr)
     return nullptr;
 
   std::unique_ptr<SymbolFileDWARFDwp> dwp_symfile(
       new SymbolFileDWARFDwp(module_sp, obj_file));
 
   lldb_private::DWARFDataExtractor debug_cu_index;

lldb/trunk/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp

for (size_t idx = 0; idx < file_spec_list.GetSize(); ++idx) {
     module_spec.GetFileSpec() = obj_file->GetFileSpec();
     module_spec.GetFileSpec().ResolvePath();
     module_spec.GetSymbolFileSpec() = fspec;
     module_spec.GetUUID() = uuid;
     FileSpec dsym_fspec = Symbols::LocateExecutableSymbolFile(module_spec);
     if (dsym_fspec) {
       DataBufferSP dsym_file_data_sp;
       lldb::offset_t dsym_file_data_offset = 0;
-      ObjectFileSP dsym_objfile_sp = ObjectFile::FindPlugin(
-          module_sp, &dsym_fspec, 0, dsym_fspec.GetByteSize(),
+      ObjectFileSP dsym_objfile_sp =
+          ObjectFile::FindPlugin(module_sp, &dsym_fspec, 0,
+                                 FileSystem::Instance().GetByteSize(dsym_fspec),
                                  dsym_file_data_sp, dsym_file_data_offset);
       if (dsym_objfile_sp) {
         // This objfile is for debugging purposes. Sadly, ObjectFileELF won't
         // be able to figure this out consistently as the symbol file may not
         // have stripped the code sections, etc.
         dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);
 
         SymbolVendorELF *symbol_vendor = new SymbolVendorELF(module_sp);
         if (symbol_vendor) {

lldb/trunk/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

if (!dsym_fspec) {
       if (module_spec.GetSourceMappingList().GetSize())
         module_sp->GetSourceMappingList().Append(
             module_spec.GetSourceMappingList(), true);
     }
 
     if (dsym_fspec) {
       DataBufferSP dsym_file_data_sp;
       lldb::offset_t dsym_file_data_offset = 0;
-      dsym_objfile_sp = ObjectFile::FindPlugin(
-          module_sp, &dsym_fspec, 0, dsym_fspec.GetByteSize(),
+      dsym_objfile_sp =
+          ObjectFile::FindPlugin(module_sp, &dsym_fspec, 0,
+                                 FileSystem::Instance().GetByteSize(dsym_fspec),
                                  dsym_file_data_sp, dsym_file_data_offset);
       if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) {
         // We need a XML parser if we hope to parse a plist...
         if (XMLDocument::XMLEnabled()) {
           char dsym_path[PATH_MAX];
           if (module_sp->GetSourceMappingList().IsEmpty() &&
               dsym_fspec.GetPath(dsym_path, sizeof(dsym_path))) {
             lldb_private::UUID dsym_uuid;
             if (dsym_objfile_sp->GetUUID(&dsym_uuid)) {

lldb/trunk/source/Symbol/ObjectFile.cpp

if (file) {
         char path_with_object[PATH_MAX * 2];
         module_sp->GetFileSpec().GetPath(path_with_object,
                                          sizeof(path_with_object));
 
         ConstString archive_object;
         const bool must_exist = true;
         if (ObjectFile::SplitArchivePathWithObject(
                 path_with_object, archive_file, archive_object, must_exist)) {
-          file_size = archive_file.GetByteSize();
+          file_size = FileSystem::Instance().GetByteSize(archive_file);
           if (file_size > 0) {
             file = &archive_file;
             module_sp->SetFileSpecAndObjectName(archive_file, archive_object);
             // Check if this is a object container by iterating through all
             // object container plugin instances and then trying to get an
             // object file from the container plugins since we had a name.
             // Also, don't read
             // ANY data in case there is data cached in the container plug-ins

 
 size_t ObjectFile::GetModuleSpecifications(const FileSpec &file,
                                            lldb::offset_t file_offset,
                                            lldb::offset_t file_size,
                                            ModuleSpecList &specs) {
   DataBufferSP data_sp = DataBufferLLVM::CreateSliceFromPath(file.GetPath(), 512, file_offset);
   if (data_sp) {
     if (file_size == 0) {
-      const lldb::offset_t actual_file_size = file.GetByteSize();
+      const lldb::offset_t actual_file_size =
+          FileSystem::Instance().GetByteSize(file);
       if (actual_file_size > file_offset)
         file_size = actual_file_size - file_offset;
     }
     return ObjectFile::GetModuleSpecifications(file,        // file spec
                                                data_sp,     // data bytes
                                                0,           // data offset
                                                file_offset, // file offset
                                                file_size,   // file length

lldb/trunk/source/Target/ModuleCache.cpp

Status ModuleCache::Get(const FileSpec &root_dir_spec, const char *hostname,
 
   const auto module_spec_dir =
       GetModuleDirectory(root_dir_spec, module_spec.GetUUID());
   const auto module_file_path = JoinPath(
       module_spec_dir, module_spec.GetFileSpec().GetFilename().AsCString());
 
   if (!module_file_path.Exists())
     return Status("Module %s not found", module_file_path.GetPath().c_str());
-  if (module_file_path.GetByteSize() != module_spec.GetObjectSize())
+  if (FileSystem::Instance().GetByteSize(module_file_path) !=
+      module_spec.GetObjectSize())
     return Status("Module %s has invalid file size",
                   module_file_path.GetPath().c_str());
 
   // We may have already cached module but downloaded from an another host - in
   // this case let's create a link to it.
   auto error = CreateHostSysRootModuleLink(root_dir_spec, hostname,
                                            module_spec.GetFileSpec(),
                                            module_file_path, false);

lldb/trunk/source/Utility/FileSpec.cpp

bool FileSpec::ResolvePath() {
   if (m_is_resolved)
     return true; // We have already resolved this path
 
   // SetFile(...) will set m_is_resolved correctly if it can resolve the path
   SetFile(GetPath(false), true);
   return m_is_resolved;
 }
 
-uint64_t FileSpec::GetByteSize() const {
-  uint64_t Size = 0;
-  if (llvm::sys::fs::file_size(GetPath(), Size))
-    return 0;
-  return Size;
-}
-
 FileSpec::Style FileSpec::GetPathStyle() const { return m_style; }
 
 uint32_t FileSpec::GetPermissions() const {
   namespace fs = llvm::sys::fs;
   fs::file_status st;
   if (fs::status(GetPath(), st, false))
     return fs::perms::perms_not_known;
 

lldb/trunk/unittests/Target/ModuleCacheTest.cpp

void ModuleCacheTest::TearDownTestCase() {
   ObjectFileELF::Terminate();
   HostInfo::Terminate();
   FileSystem::Terminate();
 }
 
 static void VerifyDiskState(const FileSpec &cache_dir, const char *hostname) {
   FileSpec uuid_view = GetUuidView(cache_dir);
   EXPECT_TRUE(uuid_view.Exists()) << "uuid_view is: " << uuid_view.GetCString();
-  EXPECT_EQ(module_size, uuid_view.GetByteSize());
+  EXPECT_EQ(module_size, FileSystem::Instance().GetByteSize(uuid_view));
 
   FileSpec sysroot_view = GetSysrootView(cache_dir, hostname);
   EXPECT_TRUE(sysroot_view.Exists()) << "sysroot_view is: "
                                      << sysroot_view.GetCString();
-  EXPECT_EQ(module_size, sysroot_view.GetByteSize());
+  EXPECT_EQ(module_size, FileSystem::Instance().GetByteSize(sysroot_view));
 }
 
 void ModuleCacheTest::TryGetAndPut(const FileSpec &cache_dir,
                                    const char *hostname, bool expect_download) {
   ModuleCache mc;
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = GetDummyRemotePath();
   module_spec.GetUUID().SetFromStringRef(module_uuid, uuid_bytes);