Case sensitive path compare on Windows breaks breakpoints

[Honsik]

Summary

On Windows allows setting breakpoint at path with different case (file main.c, breakpoint at Main.c).
fixes http://llvm.org/pr22667

[Honsik]

tabs -> spaces

[zturner]

This probably seems like a lot of comments, but most of them are pretty minor. Overall this looks like a good patch. When running the test suite with this patch, did any tests start showing up as Unexpected Success?

packages/Python/lldbsuite/test/dotest.py

288–294 ↗

(On Diff #48643)

See if you can get PTVS to work. If so, you won't need any of this code and it will be a much better debugging experience anyway. Assuming PTVS works for you, I'd rather just delete this code. I'm planning to add info about using PTVS to the website this week.

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

20 ↗

(On Diff #48643)

When we run the test suite, it's going to report this test as test_case_sensitivity_breakpoint. So looking at it, you will have to think for a minute about whether the test was expecting it to match or to not match. It would be nice if the test was called something like test_breakpoint_matches_file_with_different_case. So then you would need 2 test methods. One for matching with different case, and one for not matching with different case. Then you could use a decorator to enable them on windows or on not windows. Like this:

@skipIf(oslist=no_match(['windows']))  # Skip for non-windows platforms
def test_breakpoint_matches_file_with_different_case(self):
    self.build()
    self.case_sensitivity_breakpoint(true)

@skipIf(oslist=['windows']) # Skip for windows platforms
def test_breakpoint_doesnt_match_file_with_different_case(self):
    self.build()
    self.case_sensitivity_breakpoint(false)

25–35 ↗

(On Diff #48643)

Then this whole thing becomes:

def case_sensitivity_breakpoint(self, case_insensitive):
    exe = "a.out"
    if case_insensitive:
        exe = strupper(exe)
    exe = os.path.join(os.getcwd(), exe)

92–95 ↗

(On Diff #48643)

If should_hit is false, then I think we should launch the process anyway, but verify that it DOES NOT get hit. It would look something like this (not tested, maybe some errors here):

process = self.target.LaunchSimple(None, None, self.get_process_working_directory())
self.assertTrue(process, PROCESS_IS_VALID + desc)

if should_hit:
    # Did we hit our breakpoint?
    from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint 
    threads = get_threads_stopped_at_breakpoint (process, breakpoint)
    self.assertEqual(1, len(threads), "There should be a thread stopped at breakpoint" + desc)

    # The hit count for the breakpoint should be 1.
    self.assertEqual(1, breakpoint.GetHitCount())
else:
    self.assertEqual(lldb.eStateExited, process.GetState())

103 ↗

(On Diff #48643)

Minor nitpick, but can you use assertEqual here? Same goes for following test. This will provide a better error message, since as the code is written here it will just say "Expected True, got False", but with assertEqual, it will say "expected 1, got 0" or something like that.

108 ↗

(On Diff #48643)

process.Kill() is kind of harsh. I know some tests do it, but usually we just do process.Continue() to let it exit gracefully. (It shouldn't matter in practice, so whatever you prefer)

109 ↗

(On Diff #48643)

This line is harmless, but not necessary. Because the test runner should end up deleting the target later anyway.

110 ↗

(On Diff #48643)

Need newline at end of file.

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/main.c

6 ↗

(On Diff #48643)

Doesn't look like this is clang-formatted. We don't use a space between function name and open parentheses. You'll see a lot of code that does this, but after that code was written we agreed to remove the parentheses. clang-format should remove it for you automatically.

source/Core/ConstString.cpp

269 ↗

(On Diff #48643)

Looks like this code also isn't clang-formatted. If you build clang-format (with ninja clang-format) and add its folder to your PATH environment variable, then when your changes are staged in git, just run git clang-format.

If you've already committed you can uncommit with git reset --soft HEAD. Then they will be staged again, then you can run git clang-format, then re-add all the modified files, and then recommit.

274–275 ↗

(On Diff #48643)

I thought this was wrong at first, but then I realized it's right. Can you add a comment above this line:

// Since the pointers weren't equal, and identical ConstStrings always have identical pointers, the result must be false.

source/Host/common/FileSpec.cpp

400 ↗

(On Diff #48643)

There is a minor edge case that isn't handled here, when *this and rhs have different syntaxes. Granted if that's ever the case, it's very unlikely that the FileSpecs would be otherwise the same, but it's possible (especially if m_directory is not set).

So I think we should add a method to FileSpec

bool IsCaseSensitive() const { return m_syntax != ePathSyntaxWindows; }

Then this line becomes:

const bool case_sensitive = IsCaseSensitive() || rhs.IsCaseSensitive();

Here the comparison should be case sensitive if *either* FileSpec requires a case sensitive comparison, not just *this.

402 ↗

(On Diff #48643)

Can you reverse this conditional and just early out if they don't match (I know that wasn't your code, but seems to fit more with LLVM style.

522 ↗

(On Diff #48643)

If you add the IsCaseSensitive() method, then this would change here too (and also you should check a and b just like I mentioned in the last comment.

Same goes for the rest of the places.

[labath]

source/Core/ConstString.cpp

269 ↗

(On Diff #48643)

Or, you can use git clang-format rev to format all changes since rev (you probably want rev=HEAD^).

</drive-by>

[zturner]

source/Core/ConstString.cpp

269 ↗

(On Diff #48643)

Neat, didn't know about this. (although on Windows you have to use either HEAD~1 or HEAD^^, since ^ needs to be escaped. This makes it particularly annoying if you want HEAD~2 for example, because then you need HEAD^^^^, etc. So most people on windows use the ~n syntax.

[Honsik]

I added requested changes.

[Honsik]

packages/Python/lldbsuite/test/dotest.py

288–294 ↗

(On Diff #48643)

Ok I removed this part, it is working using PTVS. But maybe in future we will still need something similar if we would like to debug lldb testsuite using lldb, because I don't know whether lldb will support Python in near future. We can always return this part back.

[zturner]

source/Host/common/FileSpec.cpp

519 ↗

(On Diff #48643)

Did the case sensitive check get removed here? I thought this would need to be

result = ConstString::Compare(a.m_directory, b.m_directory, a.IsCaseSensitive() || b.IsCaseSensitive());

The same later in this function, and also for FileSpec::Equal

[zturner]

Looks good after the fix of the misspelling. If you want I can fix it for you and commit tomorrow.

I assume you ran the test suite and did not see any new errors?

source/Host/common/FileSpec.cpp

535 ↗

(On Diff #48871)

Minor nitpick, you misspelled case_sensitive as case_sentitive.

[Honsik]

The testsuite passed the same way as master, but no UNEXPECTED SUCCESS.
Please fix the miss spelling directly.
Thanks.

source/Host/common/FileSpec.cpp

517 ↗

(On Diff #48871)

I don't know what check you mean. bool case_sensitive is the check here.

[zturner]

Sorry that comment was a mistake. When you uploaded the most recent diff,
did you generate the diff against your first patch? Or against tip of
trunk? I think you may have generated it against your first patch, but
Phabricator gets confused and shows the wrong thing in that case.

It should be fine for me to download it and apply this way, but it makes it
harder to review. So in the future always generated all versions of your
diff against the same base.

[Honsik]

I have generated it against master branch. I used git reset HEAD~1, modified, commited and created patch.

Maybe the problem was I used the Arcanist for the first diff and then I used web interface, as you have suggested.

[zturner]

Ahh that could be. In any case when i view the full diff it looks ok. I'll
commit later today, thanks!

[zturner]

The non-Windows version of the test actually fails for some reason. I think it's more likely to be a probelm in the test and not in the C++ side of the patch, but I'm not sure. Do you by any chance have a Linux box to test on? (It's not a big problem if you don't, I've xfail'ed the test on Linux in the meantime)

[zturner]

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/11757/steps/test1/logs/stdio

There's the logs for the test that failed.

lldb/include/lldb/Core/ConstString.h

public:
     //------------------------------------------------------------------
     void
     Clear ()
     {
         m_string = nullptr;
     }
 
     //------------------------------------------------------------------
+    /// Equal to operator
+    ///
+    /// Returns true if this string is equal to the string in \a rhs.
+    /// If case sensitive equality is tested, this operation is very
+    /// fast as it results in a pointer comparison since all strings
+    /// are in a uniqued in a global string pool.
+    ///
+    /// @param[in] rhs
+    ///     The Left Hand Side const ConstString object reference.
+    ///
+    /// @param[in] rhs
+    ///     The Right Hand Side const ConstString object reference.
+    ///
+    /// @param[in] case_sensitive
+    ///     Case sensitivity. If true, case sensitive equality
+    ///     will be tested, otherwise character case will be ignored
+    ///
+    /// @return
+    ///     @li \b true if this object is equal to \a rhs.
+    ///     @li \b false if this object is not equal to \a rhs.
+    //------------------------------------------------------------------
+    static bool
+    Equals(const ConstString &lhs, const ConstString &rhs, const bool case_sensitive = true);
+
+    //------------------------------------------------------------------
     /// Compare two string objects.
     ///
     /// Compares the C string values contained in \a lhs and \a rhs and
     /// returns an integer result.
     ///
-    /// NOTE: only call this function when you want a true string 
+    /// NOTE: only call this function when you want a true string
     /// comparison. If you want string equality use the, use the ==
     /// operator as it is much more efficient. Also if you want string
     /// inequality, use the != operator for the same reasons.
     ///
     /// @param[in] lhs
     ///     The Left Hand Side const ConstString object reference.
     ///
     /// @param[in] rhs
     ///     The Right Hand Side const ConstString object reference.
     ///
+    /// @param[in] case_sensitive
+    ///     Case sensitivity of compare. If true, case sensitive compare
+    ///     will be performed, otherwise character case will be ignored
+    ///
     /// @return
     ///     @li -1 if lhs < rhs
     ///     @li 0 if lhs == rhs
     ///     @li 1 if lhs > rhs
     //------------------------------------------------------------------
     static int
-    Compare (const ConstString& lhs, const ConstString& rhs);
+    Compare(const ConstString &lhs, const ConstString &rhs, const bool case_sensitive = true);
 
     //------------------------------------------------------------------
     /// Dump the object description to a stream.
     ///
     /// Dump the string value to the stream \a s. If the contained string
     /// is empty, print \a value_if_empty to the stream instead. If
     /// \a value_if_empty is nullptr, then nothing will be dumped to the
     /// stream.

lldb/include/lldb/Host/FileSpec.h

public:
     //------------------------------------------------------------------
     static int
     Compare (const FileSpec& lhs, const FileSpec& rhs, bool full);
 
     static bool
     Equal (const FileSpec& a, const FileSpec& b, bool full, bool remove_backups = false);
 
     //------------------------------------------------------------------
+    /// Case sensitivity of path.
+    ///
+    /// @return
+    ///     \b true if the file path is case sensitive (POSIX), false
+    ///   if case insensitive (Windows).
+    //------------------------------------------------------------------
+    bool
+    IsCaseSensitive() const
+    {
+        return m_syntax != ePathSyntaxWindows;
+    }
+
+    //------------------------------------------------------------------
     /// Dump this object to a Stream.
     ///
     /// Dump the object to the supplied stream \a s. If the object
     /// contains a valid directory name, it will be displayed followed
     /// by a directory delimiter, and the filename.
     ///
     /// @param[in] s
     ///     The stream to which to dump the object description.

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS += -std=c99
+
+include $(LEVEL)/Makefile.rules

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

+"""
+Test case sensitivity of paths on Windows / POSIX
+llvm.org/pr22667
+"""
+
+import os
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbplatform, lldbplatformutil
+
+class BreakpointCaseSensitivityTestCase(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+    BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        self.line = line_number('main.c', self.BREAKPOINT_TEXT)
+
+    @skipIf(oslist=no_match(['windows']))  # Skip for non-windows platforms
+    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
+    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()
+        self.case_sensitivity_breakpoint(False)
+
+    def case_sensitivity_breakpoint(self, case_insensitive):
+        """Set breakpoint on file, should match files with different case if case_insensitive is True"""
+        
+        # use different case to check CreateTarget
+        exe = 'a.out'
+        if case_insensitive:
+            exe = exe.upper()
+            
+        exe = os.path.join(os.getcwd(), exe)
+
+        # Create a target by the debugger.
+        self.target = self.dbg.CreateTarget(exe)
+        self.assertTrue(self.target, VALID_TARGET)
+        cwd = self.get_process_working_directory();
+                
+        # try both BreakpointCreateByLocation and BreakpointCreateBySourceRegex
+        for regex in [False, True]:
+            # should always hit
+            self.check_breakpoint('main.c', regex, True)
+            # should always hit
+            self.check_breakpoint(os.path.join(cwd, 'main.c'), regex, True)
+            # different case for directory
+            self.check_breakpoint(os.path.join(cwd.upper(), 'main.c'),
+                                  regex,
+                                  case_insensitive)
+            # different case for file
+            self.check_breakpoint('Main.c',
+                                  regex,
+                                  case_insensitive)
+            # different case for both
+            self.check_breakpoint(os.path.join(cwd.upper(), 'Main.c'),
+                                  regex,
+                                  case_insensitive)
+
+    def check_breakpoint(self, file, source_regex, should_hit):
+        """
+        Check breakpoint hit at given file set by given method
+        
+        file:
+            File where insert the breakpoint
+        
+        source_regex:
+            True for testing using BreakpointCreateBySourceRegex,
+            False for  BreakpointCreateByLocation
+            
+        should_hit:
+            True if the breakpoint should hit, False otherwise
+        """
+        
+        desc = ' file %s set by %s' % (file, 'regex' if source_regex else 'location')
+        if source_regex:
+            breakpoint = self.target.BreakpointCreateBySourceRegex(self.BREAKPOINT_TEXT,
+                                                                   lldb.SBFileSpec(file))
+        else:    
+            breakpoint = self.target.BreakpointCreateByLocation(file, self.line)
+        
+        self.assertEqual(breakpoint and 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(),
+                    should_hit,
+                    VALID_BREAKPOINT_LOCATION + desc)
+        
+        process = self.target.LaunchSimple(None, None, self.get_process_working_directory())
+        self.assertTrue(process, PROCESS_IS_VALID + desc)
+
+        if should_hit:
+            # Did we hit our breakpoint?
+            from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint 
+            threads = get_threads_stopped_at_breakpoint (process, breakpoint)
+            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)
+            
+        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())

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/main.c

+#include <stdio.h>
+
+int
+main()
+{
+    printf("Set a breakpoint here.\n");
+    return 0;
+}

lldb/source/Core/ConstString.cpp

 }
 
 size_t
 ConstString::GetLength () const
 {
     return StringPool().GetConstCStringLength (m_string);
 }
 
+bool
+ConstString::Equals(const ConstString &lhs, const ConstString &rhs, const bool case_sensitive)
+{
+    if (lhs.m_string == rhs.m_string)
+        return true;
+
+    // Since the pointers weren't equal, and identical ConstStrings always have identical pointers,
+    // the result must be false for case sensitive equality test.
+    if (case_sensitive)
+        return false;
+
+    // perform case insensitive equality test
+    llvm::StringRef lhs_string_ref(lhs.m_string, StringPool().GetConstCStringLength(lhs.m_string));
+    llvm::StringRef rhs_string_ref(rhs.m_string, StringPool().GetConstCStringLength(rhs.m_string));
+    return lhs_string_ref.equals_lower(rhs_string_ref);
+}
+
 int
-ConstString::Compare (const ConstString& lhs, const ConstString& rhs)
+ConstString::Compare(const ConstString &lhs, const ConstString &rhs, const bool case_sensitive)
 {
     // If the iterators are the same, this is the same string
     const char *lhs_cstr = lhs.m_string;
     const char *rhs_cstr = rhs.m_string;
     if (lhs_cstr == rhs_cstr)
         return 0;
     if (lhs_cstr && rhs_cstr)
     {
         llvm::StringRef lhs_string_ref (lhs_cstr, StringPool().GetConstCStringLength (lhs_cstr));
         llvm::StringRef rhs_string_ref (rhs_cstr, StringPool().GetConstCStringLength (rhs_cstr));
+
+        if (case_sensitive)
+        {
             return lhs_string_ref.compare(rhs_string_ref);
         }
+        else
+        {
+            return lhs_string_ref.compare_lower(rhs_string_ref);
+        }
+    }
 
     if (lhs_cstr)
         return +1;  // LHS isn't NULL but RHS is
     else
         return -1;  // LHS is NULL but RHS isn't
 }
 
 void

lldb/source/Core/FileSpecList.cpp

FileSpecList::FindFileIndex (size_t start_idx, const FileSpec &file_spec, bool full, bool remove_dots) const
     // When looking for files, we will compare only the filename if the
     // FILE_SPEC argument is empty
     bool compare_filename_only = file_spec.GetDirectory().IsEmpty();
 
     for (size_t idx = start_idx; idx < num_files; ++idx)
     {
         if (compare_filename_only)
         {
-            if (m_files[idx].GetFilename() == file_spec.GetFilename())
+            if (ConstString::Equals(m_files[idx].GetFilename(), file_spec.GetFilename(),
+                                    file_spec.IsCaseSensitive() || m_files[idx].IsCaseSensitive()))
                 return idx;
         }
         else
         {
             if (FileSpec::Equal (m_files[idx], file_spec, full, remove_dots))
                 return idx;
         }
     }

lldb/source/Host/common/FileSpec.cpp

 }
 
 //------------------------------------------------------------------
 // Equal to operator
 //------------------------------------------------------------------
 bool
 FileSpec::operator== (const FileSpec& rhs) const
 {
-    if (m_filename == rhs.m_filename)
-    {
-        if (m_directory == rhs.m_directory)
-            return true;
-        
-        // TODO: determine if we want to keep this code in here.
-        // The code below was added to handle a case where we were
-        // trying to set a file and line breakpoint and one path
-        // was resolved, and the other not and the directory was
-        // in a mount point that resolved to a more complete path:
-        // "/tmp/a.c" == "/private/tmp/a.c". I might end up pulling
-        // this out...
-        if (IsResolved() && rhs.IsResolved())
-        {
-            // Both paths are resolved, no need to look further...
-            return false;
-        }
-        
-        FileSpec resolved_lhs(*this);
+    // case sensitivity of equality test
+    const bool case_sensitive = IsCaseSensitive() || rhs.IsCaseSensitive();
+
+    if (!ConstString::Equals(m_filename, rhs.m_filename, case_sensitive))
+        return false;
+
+    if (ConstString::Equals(m_directory, rhs.m_directory, case_sensitive))
 
-        // If "this" isn't resolved, resolve it
-        if (!IsResolved())
-        {
-            if (resolved_lhs.ResolvePath())
-            {
-                // This path wasn't resolved but now it is. Check if the resolved
-                // directory is the same as our unresolved directory, and if so, 
-                // we can mark this object as resolved to avoid more future resolves
-                m_is_resolved = (m_directory == resolved_lhs.m_directory);
-            }
-            else
-                return false;
-        }
-        
-        FileSpec resolved_rhs(rhs);
-        if (!rhs.IsResolved())
-        {
-            if (resolved_rhs.ResolvePath())
-            {
-                // rhs's path wasn't resolved but now it is. Check if the resolved
-                // directory is the same as rhs's unresolved directory, and if so, 
-                // we can mark this object as resolved to avoid more future resolves
-                rhs.m_is_resolved = (rhs.m_directory == resolved_rhs.m_directory);
-            }
-            else
-                return false;
-        }
+            // directory is the same as our unresolved directory, and if so,
+            // directory is the same as rhs's unresolved directory, and if so,
 
-        // If we reach this point in the code we were able to resolve both paths
-        // and since we only resolve the paths if the basenames are equal, then
-        // we can just check if both directories are equal...
-        return resolved_lhs.GetDirectory() == resolved_rhs.GetDirectory();
-    }
-    return false;
+    return ConstString::Equals(m_directory, rhs.m_directory, case_sensitive);
 }
 
 //------------------------------------------------------------------
 // Not equal to operator
 //------------------------------------------------------------------
 bool
 FileSpec::operator!= (const FileSpec& rhs) const
 {

 // Return -1 if the "a" is less than "b", 0 if "a" is equal to "b"
 // and "1" if "a" is greater than "b".
 //------------------------------------------------------------------
 int
 FileSpec::Compare(const FileSpec& a, const FileSpec& b, bool full)
 {
     int result = 0;
 
+    // case sensitivity of compare
+    const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();
+
     // If full is true, then we must compare both the directory and filename.
 
     // If full is false, then if either directory is empty, then we match on
     // the basename only, and if both directories have valid values, we still
     // do a full compare. This allows for matching when we just have a filename
     // in one of the FileSpec objects.
 
     if (full || (a.m_directory && b.m_directory))
     {
-        result = ConstString::Compare(a.m_directory, b.m_directory);
+        result = ConstString::Compare(a.m_directory, b.m_directory, case_sensitive);
         if (result)
             return result;
     }
-    return ConstString::Compare (a.m_filename, b.m_filename);
+    return ConstString::Compare(a.m_filename, b.m_filename, case_sensitive);
 }
 
 bool
 FileSpec::Equal (const FileSpec& a, const FileSpec& b, bool full, bool remove_backups)
 {
+    // case sensitivity of equality test
+    const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();
+
     if (!full && (a.GetDirectory().IsEmpty() || b.GetDirectory().IsEmpty()))
-        return a.m_filename == b.m_filename;
+        return ConstString::Equals(a.m_filename, b.m_filename, case_sensitive);
     else if (remove_backups == false)
         return a == b;
     else
     {
-        if (a.m_filename != b.m_filename)
+        if (!ConstString::Equals(a.m_filename, b.m_filename, case_sensitive))
             return false;
-        if (a.m_directory == b.m_directory)
+        if (ConstString::Equals(a.m_directory, b.m_directory, case_sensitive))
             return true;
         ConstString a_without_dots;
         ConstString b_without_dots;
 
         RemoveBackupDots (a.m_directory, a_without_dots);
         RemoveBackupDots (b.m_directory, b_without_dots);
-        return a_without_dots == b_without_dots;
+        return ConstString::Equals(a_without_dots, b_without_dots, case_sensitive);
     }
 }
 
 void
 FileSpec::NormalizePath ()
 {
     ConstString normalized_directory;
     FileSpec::RemoveBackupDots(m_directory, normalized_directory);