[Lldb-commits] [PATCH] D17492: Case sensitive path compare on Windows breaks breakpoints

[Zachary Turner via lldb-commits]

zturner added a comment.

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?


================
Comment at: packages/Python/lldbsuite/test/dotest.py:288-294
@@ -288,1 +287,9 @@
+        
+        if sys.platform.startswith('win32'):
+            import ctypes, time
+            while not ctypes.windll.kernel32.IsDebuggerPresent():
+                time.sleep(0.5)
+            ctypes.windll.kernel32.DebugBreak()
+        else:
+            os.kill(os.getpid(), signal.SIGSTOP)
 
----------------
See if you can get [[ https://github.com/Microsoft/PTVS/releases | 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.

 

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:20
@@ +19,3 @@
+
+    def test_case_sensitivity_breakpoint(self):
+        """Set breakpoint on file, should match files with different case on Windows"""
----------------
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)
```

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:25-35
@@ +24,13 @@
+
+    def case_sensitivity_breakpoint(self):
+        """Set breakpoint on file, should match files with different case on Windows"""
+        
+        # if breakpoint should hit when file path is case modified 
+        should_hit_modified_case = lldbplatformutil.getHostPlatform() == "windows"
+        
+        # use different case on Windows
+        if should_hit_modified_case:
+            exe = os.path.join(os.getcwd(), "A.OUT")
+        else:
+            exe = os.path.join(os.getcwd(), "a.out")
+
----------------
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)
```

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:92-95
@@ +91,6 @@
+        
+        # not needed to spawn the process when break is not set
+        if not should_hit:
+            self.target.BreakpointDelete(breakpoint.GetID())
+            return
+
----------------
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())



================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:103
@@ +102,3 @@
+        threads = get_threads_stopped_at_breakpoint (process, breakpoint)
+        self.assertTrue(len(threads) == 1, "There should be a thread stopped at breakpoint" + desc)
+        # The hit count for the breakpoint should be 1.
----------------
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.

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:108
@@ +107,3 @@
+        # clean after ourself
+        process.Kill()
+        self.target.BreakpointDelete(breakpoint.GetID())
----------------
`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)

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:109
@@ +108,2 @@
+        process.Kill()
+        self.target.BreakpointDelete(breakpoint.GetID())
----------------
This line is harmless, but not necessary.  Because the test runner should end up deleting the target later anyway.

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py:110
@@ +109,1 @@
+        self.target.BreakpointDelete(breakpoint.GetID())
\ No newline at end of file

----------------
Need newline at end of file.

================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/main.c:6
@@ +5,3 @@
+{
+    printf ("Set a breakpoint here.\n");
+    return 0;
----------------
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.

================
Comment at: source/Core/ConstString.cpp:269
@@ +268,3 @@
+bool
+ConstString::Equals (const ConstString& lhs, const ConstString& rhs, const bool case_sensitive)
+{
----------------
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.

================
Comment at: source/Core/ConstString.cpp:274-275
@@ +273,4 @@
+
+    if (case_sensitive)
+        return false;
+
----------------
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.
```

================
Comment at: source/Host/common/FileSpec.cpp:400
@@ +399,3 @@
+    // case sensitivity of file check
+    const bool case_sensitive = m_syntax != ePathSyntaxWindows;
+
----------------
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`.

================
Comment at: source/Host/common/FileSpec.cpp:402
@@ -400,1 +401,3 @@
+
+    if (ConstString::Equals(m_filename, rhs.m_filename, case_sensitive))
     {
----------------
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.

================
Comment at: source/Host/common/FileSpec.cpp:522
@@ -518,3 +521,3 @@
     {
-        result = ConstString::Compare(a.m_directory, b.m_directory);
+        result = ConstString::Compare(a.m_directory, b.m_directory, a.m_syntax != ePathSyntaxWindows);
         if (result)
----------------
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.


http://reviews.llvm.org/D17492