Page MenuHomePhabricator

Case sensitive path compare on Windows breaks breakpoints
ClosedPublic

Authored by Honsik on Feb 21 2016, 3:20 PM.

Details

Summary

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

Diff Detail

Event Timeline

Honsik updated this revision to Diff 48640.Feb 21 2016, 3:20 PM
Honsik retitled this revision from to Case sensitive path compare on Windows breaks breakpoints.
Honsik updated this object.
Honsik added a reviewer: zturner.
Honsik updated this revision to Diff 48643.Feb 21 2016, 3:57 PM

tabs -> spaces

zturner edited edge metadata.Feb 22 2016, 11:16 AM

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 added a subscriber: labath.Feb 23 2016, 3:23 AM
labath added inline comments.
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 added inline comments.Feb 23 2016, 8:40 AM
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 updated this revision to Diff 48871.Feb 23 2016, 6:07 PM
Honsik updated this object.
Honsik edited edge metadata.

I added requested changes.

Honsik added inline comments.Feb 23 2016, 6:11 PM
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 added inline comments.Feb 23 2016, 7:37 PM
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 accepted this revision.Feb 23 2016, 7:41 PM
zturner edited edge metadata.

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.

This revision is now accepted and ready to land.Feb 23 2016, 7:41 PM

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.

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.

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.

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

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)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 7:11 AM