File path comparisons should be case-insensitive on OS X

[stigger]

Summary

• File path comparisons should be case-insensitive on OS X
• Fixed TestBreakpointCaseSensitivity

[jasonmolenda]

I don't know if this is a safe change. Mac OS X filesystems like HFS Plus may be case-insensitive (but case preserving) or they may be case sensitive. Network filesystems mounted on a Mac may be case sensitive. Without doing some attribute check on the filesystem (I don't know what API can tell you this off-hand) where the FileSpec is, I don't think this is a good idea.

[stigger]

These are valid points, but they apply to Linux and Windows as well: nothing prevents you from mounting a case-insensitive FS on Linux or a case-sensitive network share on Windows. However, in most cases the file system is going to be case-sensitive on Linux and case-insensitive on Windows.

It is true, that HFS supports optional case-sensitivity, but by default it is not being used, and 99% of Mac users out there are on case-insensitive FS.

Of course, it would be nice to dynamically check case sensitivity for every path, but that should be done for all platforms and is kind of out of scope of the problem that I tried to solve here. Plus, it might affect the performance.

[jasonmolenda]

Agreed, filesystems are almost always case sensitive on mac/ios. I'd hate for anyone to make the assumption that it's always the case, though - I work with network filesystems that are case sensitive every day so I'm very aware of the possibility. :)

[stigger]

Friendly ping.

[stigger]

Ping.

[zturner]

I like the approach taken here in general.

One thing that I wonder about, if we have the Triple, why do we even need the PathSyntax at all? In light of the fact that not all Posix filesystems are case sensitive, this calls into question the usefulness of the Path Syntax since it is both unnecessary and insufficient.

I do think that FileSpec should be completely independent of Host. Any host detection code should be in FileSystem.h and any remote detection code should be in Platform somewhere. Both of those are independent of this patch though.

How difficult would be it be to eliminate the PathSyntax enumeration entirely and update every user of PathSyntax to use a triple instead?

[stigger]

In D20041#533725, @zturner wrote:

How difficult would be it be to eliminate the PathSyntax enumeration entirely and update every user of PathSyntax to use a triple instead?

I've attempted to do it and hit two issues which I'm not sure how to resolve:

1. SymbolFilePDB.cpp:358 uses FileSpec with explicit ePathSyntaxWindows. It's possible to do something like llvm::Triple("unknown", "unknown", "windows"), but that kind of smells.
2. HostInfoBase.cpp:63 has a bunch of FileSpecs with ePathSyntaxHostNative, but at that moment it is not yet possible to use HostInfo::GetArchitecture(), because it's being constructed.

[labath]

include/lldb/Host/FileSpec.h

712

I don't think the default-null parameter here is a good idea. This means some of your file-specs will be case-sensitive and some will not (depending on whether the the person who created the FileSpec remembered to pass in the triple).

If it were up to me, I wouldn't even make case-sensitivity a property of the file spec. I think it should be a property of the comparison between them. Otherwise, what are you going to do when someone asks you to compare a case-insensitive "/foo/bar" with a case-sensitive "/Foo/Bar" ?

[stigger]

include/lldb/Host/FileSpec.h

712

If we get rid of PathSyntax, then default-null is going to be an equivalent of ePathSyntaxHostNative, meaning "use host triple".

Otherwise, what are you going to do when someone asks you to compare a case-insensitive "/foo/bar" with a case-sensitive "/Foo/Bar" ?

That's already handled in FileSpec::Equal and FileSpec::Compare: the comparison is case-insensitive only when both operands are case-insensitive.

include/lldb/Host/FileSpec.h

 #include "lldb/Core/STLUtils.h"
 #include "lldb/Host/TimeValue.h"
 
+#include "llvm/ADT/Triple.h"
+
 namespace lldb_private {
 
 //----------------------------------------------------------------------

 
     FileSpec();
 
+    explicit FileSpec(bool case_sensitive);
+
     //------------------------------------------------------------------
     /// Constructor with path.
     ///

     ///   if case insensitive (Windows).
     //------------------------------------------------------------------
     bool
-    IsCaseSensitive() const
-    {
-        return m_syntax != ePathSyntaxWindows;
-    }
+    IsCaseSensitive() const;
 
     //------------------------------------------------------------------
     /// Dump this object to a Stream.

     /// @param[in] resolve_path
     ///     If \b true, then we will try to resolve links the path using
     ///     the static FileSpec::Resolve.
+    ///
+    /// @param[in] triple
+    ///     If \b provided, then used for a more precise case sensitivity detection.
     //------------------------------------------------------------------
     void
-    SetFile (const char *path, bool resolve_path, PathSyntax syntax = ePathSyntaxHostNative);
+    SetFile (const char *path,
+             bool resolve_path,
+             PathSyntax syntax = ePathSyntaxHostNative,
+             llvm::Triple *triple = nullptr);
+
+    void
+    SetFile(const char *path, bool resolve_path, bool case_sensitive, PathSyntax syntax = ePathSyntaxHostNative);
 
     void
     SetFile(const char *path, bool resolve_path, ArchSpec arch);

     ForEachItemInDirectory (const char *dir_path, DirectoryCallback const &callback);
 
 protected:
+    static bool
+    IsCaseSensitive(PathSyntax syntax, const llvm::Triple *triple = nullptr);
     //------------------------------------------------------------------
     // Member variables
     //------------------------------------------------------------------

     ConstString m_filename;     ///< The uniqued filename path
     mutable bool m_is_resolved; ///< True if this path has been resolved.
     PathSyntax m_syntax;        ///< The syntax that this path uses (e.g. Windows / Posix)
+    bool m_case_sensitive;
 };
 
 //----------------------------------------------------------------------

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py

         TestBase.setUp(self)
         self.line = line_number('main.c', self.BREAKPOINT_TEXT)
 
-    @skipIf(oslist=no_match(['windows']))  # Skip for non-windows platforms
+    @skipIf(oslist=['linux'])
     def test_breakpoint_matches_file_with_different_case(self):
         """Set breakpoint on file, should match files with different case on Windows"""
         self.build()
         self.case_sensitivity_breakpoint(True)
 
-    @skipIf(oslist=['windows']) # Skip for windows platforms
-    @expectedFailureAll() # Failing for unknown reason on non-Windows platforms.
+    @skipIf(oslist=no_match(['linux']))
     def test_breakpoint_doesnt_match_file_with_different_case(self):
         """Set breakpoint on file, shouldn't match files with different case on POSIX systems"""
         self.build()

                                                                    lldb.SBFileSpec(file))
         else:    
             breakpoint = self.target.BreakpointCreateByLocation(file, self.line)
-        
-        self.assertEqual(breakpoint and breakpoint.GetNumLocations() == 1,
+        self.assertEqual(breakpoint.GetNumLocations() == 1,
                     should_hit,
                     VALID_BREAKPOINT + desc)
 
         # Get the breakpoint location from breakpoint after we verified that,
         # indeed, it has one location.
         location = breakpoint.GetLocationAtIndex(0)
-        self.assertEqual(location and location.IsEnabled(),
+        self.assertEqual(location.IsEnabled(),
                     should_hit,
                     VALID_BREAKPOINT_LOCATION + desc)
         

             self.assertEqual(len(threads), 1, "There should be a thread stopped at breakpoint" + desc)
             # The hit count for the breakpoint should be 1.
             self.assertEqual(breakpoint.GetHitCount(), 1)
-            
+            # let process finish
+            process.Continue()
         else:
             # check the breakpoint was not hit
             self.assertEqual(lldb.eStateExited, process.GetState())
             self.assertEqual(breakpoint.GetHitCount(), 0)
 
-        # let process finish
-        process.Continue()
-        
         # cleanup
         self.target.BreakpointDelete(breakpoint.GetID())

source/Host/common/FileSpec.cpp

 #include "lldb/Host/FileSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/CleanUp.h"
 
 #include "llvm/ADT/StringRef.h"

 }
 
 FileSpec::FileSpec() : 
+    FileSpec{IsCaseSensitive(ePathSyntaxHostNative)}
+{
+}
+
+FileSpec::FileSpec(bool case_sensitive) :
     m_directory(), 
     m_filename(), 
-    m_syntax(FileSystem::GetNativePathSyntax())
+    m_syntax(FileSystem::GetNativePathSyntax()),
+    m_case_sensitive(case_sensitive)
 {
 }
 

     m_directory(),
     m_filename(),
     m_is_resolved(false),
-    m_syntax(syntax)
+    m_syntax(syntax),
+    m_case_sensitive(IsCaseSensitive(syntax))
 {
     if (pathname && pathname[0])
-        SetFile(pathname, resolve_path, syntax);
+        SetFile(pathname, resolve_path, m_case_sensitive, syntax);
 }
 
 FileSpec::FileSpec(const char *pathname, bool resolve_path, ArchSpec arch) :
     FileSpec{pathname, resolve_path, arch.GetTriple().isOSWindows() ? ePathSyntaxWindows : ePathSyntaxPosix}
 {
+    m_case_sensitive = IsCaseSensitive(m_syntax, &arch.GetTriple());
 }
 
 FileSpec::FileSpec(const std::string &path, bool resolve_path, PathSyntax syntax) :

     m_directory (rhs.m_directory),
     m_filename (rhs.m_filename),
     m_is_resolved (rhs.m_is_resolved),
-    m_syntax (rhs.m_syntax)
+    m_syntax (rhs.m_syntax),
+    m_case_sensitive (rhs.m_case_sensitive)
 {
 }
 

         m_filename = rhs.m_filename;
         m_is_resolved = rhs.m_is_resolved;
         m_syntax = rhs.m_syntax;
+        m_case_sensitive = rhs.m_case_sensitive;
     }
     return *this;
 }

 // string values for quick comparison and efficient memory usage.
 //------------------------------------------------------------------
 void
-FileSpec::SetFile (const char *pathname, bool resolve, PathSyntax syntax)
+FileSpec::SetFile(const char *pathname, bool resolve, bool case_sensitive, PathSyntax syntax)
 {
     m_filename.Clear();
     m_directory.Clear();
     m_is_resolved = false;
+    m_case_sensitive = case_sensitive;
     m_syntax = (syntax == ePathSyntaxHostNative) ? FileSystem::GetNativePathSyntax() : syntax;
 
     if (pathname == NULL || pathname[0] == '\0')

 }
 
 void
+FileSpec::SetFile (const char *pathname, bool resolve, PathSyntax syntax, llvm::Triple *triple)
+{
+    return SetFile(pathname, resolve, IsCaseSensitive(syntax, triple), syntax);
+}
+
+void
 FileSpec::SetFile(const char *pathname, bool resolve, ArchSpec arch)
 {
     return SetFile(pathname, resolve,
             arch.GetTriple().isOSWindows()
             ? ePathSyntaxWindows
-            : ePathSyntaxPosix);
+            : ePathSyntaxPosix,
+            &arch.GetTriple());
 }
 
 void

     if (!GetPath (path_buf, PATH_MAX, false))
         return false;
     // SetFile(...) will set m_is_resolved correctly if it can resolve the path
-    SetFile (path_buf, true);
+    SetFile(path_buf, true, m_case_sensitive, m_syntax);
     return m_is_resolved; 
 }
 

     const bool resolve = false;
     if (m_filename.IsEmpty() && m_directory.IsEmpty())
     {
-        SetFile(new_path, resolve);
+        SetFile(new_path, resolve, m_case_sensitive, m_syntax);
         return;
     }
     StreamString stream;

         stream.Printf("%s/%s", new_path, m_filename.GetCString());
     else
         stream.Printf("%s/%s/%s", new_path, m_directory.GetCString(), m_filename.GetCString());
-    SetFile(stream.GetData(), resolve);
+    SetFile(stream.GetData(), resolve, m_case_sensitive, m_syntax);
 }
 
 void

     stream.PutCString(new_path);
 
     const bool resolve = false;
-    SetFile(stream.GetData(), resolve, m_syntax);
+    SetFile(stream.GetData(), resolve, m_case_sensitive, m_syntax);
 }
 
 void

     const bool resolve = false;
     if (m_filename.IsEmpty() && m_directory.IsEmpty())
     {
-        SetFile("",resolve);
+        SetFile("", resolve, m_case_sensitive, m_syntax);
         return;
     }
     if (m_directory.IsEmpty())
     {
-        SetFile("",resolve);
+        SetFile("", resolve, m_case_sensitive, m_syntax);
         return;
     }
     if (m_filename.IsEmpty())

         // check for obvious cases before doing the full thing
         if (!last_slash_ptr)
         {
-            SetFile("",resolve);
+            SetFile("", resolve, m_case_sensitive, m_syntax);
             return;
         }
         if (last_slash_ptr == dir_cstr)
         {
-            SetFile("/",resolve);
+            SetFile("/", resolve, m_case_sensitive, m_syntax);
             return;
         }        
         size_t last_slash_pos = last_slash_ptr - dir_cstr+1;
         ConstString new_path(dir_cstr,last_slash_pos);
-        SetFile(new_path.GetCString(),resolve);
+        SetFile(new_path.GetCString(), resolve, m_case_sensitive, m_syntax);
     }
     else
-        SetFile(m_directory.GetCString(),resolve);
+        SetFile(m_directory.GetCString(), resolve, m_case_sensitive, m_syntax);
 }
 //------------------------------------------------------------------
 /// Returns true if the filespec represents an implementation source

 {
     return !FileSpec::IsRelative();
 }
+
+bool
+FileSpec::IsCaseSensitive() const
+{
+    return m_case_sensitive;
+}
+
+bool
+FileSpec::IsCaseSensitive(PathSyntax syntax, const llvm::Triple *triple)
+{
+    switch (syntax)
+    {
+        case ePathSyntaxPosix:
+            return triple == nullptr || !triple->isMacOSX();
+        case ePathSyntaxWindows:
+            return false;
+        case ePathSyntaxHostNative:
+            return IsCaseSensitive(FileSystem::GetNativePathSyntax(), &HostInfo::GetArchitecture().GetTriple());
+        default:
+            return true;
+    }
+}

source/Host/common/HostInfoBase.cpp

         ArchSpec m_host_arch_32;
         ArchSpec m_host_arch_64;
 
-        FileSpec m_lldb_so_dir;
-        FileSpec m_lldb_support_exe_dir;
-        FileSpec m_lldb_headers_dir;
-        FileSpec m_lldb_python_dir;
-        FileSpec m_lldb_clang_resource_dir;
-        FileSpec m_lldb_system_plugin_dir;
-        FileSpec m_lldb_user_plugin_dir;
-        FileSpec m_lldb_process_tmp_dir;
-        FileSpec m_lldb_global_tmp_dir;
+        FileSpec m_lldb_so_dir = FileSpec(false);
+        FileSpec m_lldb_support_exe_dir = FileSpec(false);
+        FileSpec m_lldb_headers_dir = FileSpec(false);
+        FileSpec m_lldb_python_dir = FileSpec(false);
+        FileSpec m_lldb_clang_resource_dir = FileSpec(false);
+        FileSpec m_lldb_system_plugin_dir = FileSpec(false);
+        FileSpec m_lldb_user_plugin_dir = FileSpec(false);
+        FileSpec m_lldb_process_tmp_dir = FileSpec(false);
+        FileSpec m_lldb_global_tmp_dir = FileSpec(false);
     };
     
     HostInfoBaseFields *g_fields = nullptr;