[lldb] Fix and re-enable TestUseSourceCache.py

[igorkudrin]

The decorators caused the test_set_use_source_cache_true() test to be skipped in most scenarios. It was only run on a Windows host targeting a non-Windows remote platform. The source file is opened with the FILE_SHARE_DELETE sharing mode, which allows the file to be removed even though it is also memory-mapped; at least, this behavior is observed on Windows 11.

The patch replaces the operation with an attempt to overwrite the file, which still fails for such files on Windows 11.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

The decorators caused the main test, i.e. test_set_use_source_cache_true(), to be skipped in most scenarios. It was only run on a Windows host targeting a non-Windows remote platform, and it failed in this case because the source file is opened with the FILE_SHARE_DELETE share mode, which allows the file to be removed, see llvm::sys::fs::openNativeFileInternal() in llvm/lib/Support/Windows/Path.inc.

This patch changes the test to check that the content can still be displayed, even after deleting the file, when the source cache is enabled. The updated test is expected to pass on any platform, so the decorators are removed.

---

Full diff: https://github.com/llvm/llvm-project/pull/111237.diff

1 Files Affected:

• (modified) lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py (+4-7)

diff --git a/lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py b/lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
index 421599080a9e51..421b13f253f05b 100644
--- a/lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
+++ b/lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py
@@ -17,8 +17,6 @@ def test_set_use_source_cache_false(self):
         """Test that after 'set use-source-cache false', files are not locked."""
         self.set_use_source_cache_and_test(False)
 
-    @skipIf(hostoslist=no_match(["windows"]))
-    @skipIf(oslist=["windows"])  # Fails on windows 11
     def test_set_use_source_cache_true(self):
         """Test that after 'set use-source-cache false', files are locked."""
         self.set_use_source_cache_and_test(True)
@@ -43,16 +41,15 @@ def set_use_source_cache_and_test(self, is_cache_enabled):
             self, "calc"
         )
 
-        # Show the source file contents to make sure LLDB loads src file.
-        self.runCmd("source list")
+        # Ensure that the source file is loaded.
+        self.expect("step", patterns=["-> .+ int x4 ="])
 
         # Try deleting the source file.
         is_file_removed = self.removeFile(src)
 
         if is_cache_enabled:
-            self.assertFalse(
-                is_file_removed, "Source cache is enabled, but delete file succeeded"
-            )
+            # Regardless of whether the file is removed, its contents should be displayed.
+            self.expect("step", patterns=["-> .+ int x5 ="])
 
         if not is_cache_enabled:
             self.assertTrue(

[labath]

Looks good. Tagging @emrekultursay, as this might be interesting for him. As I understand it, Android Studio was setting use-source-cache:=false so that users can continue to edit files on windows while lldb has them open.

Judging by this patch, that may no longer be necessary. Or does that only apply for certain situations (certain windows versions, files which are not large enough to be mmapped, etc.)?

[igorkudrin]

As I understand it, Android Studio was setting use-source-cache:=false so that users can continue to edit files on windows while lldb has them open.

Judging by this patch, that may no longer be necessary. Or does that only apply for certain situations (certain windows versions, files which are not large enough to be mmapped, etc.)?

Based on the change history of TestUseSourceCache.py, the test probably passes on Windows 10 in its original form, so the opened file is probably locked there, despite the FILE_SHARE_DELETE mode. I don't have this system to properly debug the case, though.

[emrekultursay]

it failed in this case because the source file is opened with the FILE_SHARE_DELETE share mode, which allows the file to be removed, see llvm::sys::fs::openNativeFileInternal() in llvm/lib/Support/Windows/Path.inc.

Was there an LLDB change about how it opens a source file? When this test was written, LLDB called:

liblldb.dll!shouldUseMmap(...) Line 323
	at c:\src\llvm-project\llvm\lib\support\memorybuffer.cpp(327)
liblldb.dll!getOpenFileImpl<llvm::WritableMemoryBuffer>(...) Line 446
	at c:\src\llvm-project\llvm\lib\support\memorybuffer.cpp(446)
liblldb.dll!getFileAux<llvm::WritableMemoryBuffer>(...) Line 254
	at c:\src\llvm-project\llvm\lib\support\memorybuffer.cpp(254)
liblldb.dll!llvm::WritableMemoryBuffer::getFile(...) Line 261
	at c:\src\llvm-project\llvm\lib\support\memorybuffer.cpp(261)
liblldb.dll!lldb_private::FileSystem::CreateDataBuffer(...) Line 294
	at c:\src\llvm-project\lldb\source\host\common\filesystem.cpp(294)
liblldb.dll!lldb_private::FileSystem::CreateDataBuffer(...) Line 310
	at c:\src\llvm-project\lldb\source\host\common\filesystem.cpp(310)
liblldb.dll!lldb_private::SourceManager::File::CommonInitializer(...) Line 459
	at c:\src\llvm-project\lldb\source\core\sourcemanager.cpp(459)
liblldb.dll!lldb_private::SourceManager::File::File(...) Line 397
	at c:\src\llvm-project\lldb\source\core\sourcemanager.cpp(397)

...and that shouldUseMmap was returning true for large files, which resulted in a MemoryBufferMMapFile. I don't see any relevant changes on those files, so I don't know the where openNativeFileInternal() comes into the picture.

Judging by this patch, that may no longer be necessary. Or does that only apply for certain situations (certain windows versions, files which are not large enough to be mmapped, etc.)?

LLDB used memory mapping for very large files. Maybe the test main.cpp doesn't satisfy the "memory-mapped file" criteria any more? I couldn't find an explanation based on Windows 10 vs. Windows 11 file system locking.

[igorkudrin]

I don't see any relevant changes on those files, so I don't know the where openNativeFileInternal() comes into the picture.

getFileAux() calls openNativeFileForRead() first and then passes the file descriptor to getOpenFileImpl(). openNativeFileForRead() eventually calls openNativeFileInternal(), which passes FILE_SHARE_DELETE to ::CreateFileW(), and that was already like that in times of D76806, "Remove m_last_file_sp from SourceManager", so I don't know why the opened file was locked for deletion back then.

The only change in the sequence I can see was replacing WritableMemoryBuffer with MemoryBuffer, caused by D122856, "[lldb] Refactor DataBuffer so we can map files as read-only", but the file was still opened with the FILE_SHARE_DELETE mode.

[labath]

At least back when I was looking at this problem (5+ years ago), memory mapping was the key -- I was not able to delete the process if the file was mapped -- regardless of the open flags. That said, this comment seems to indicate this might be possible (though it may refer to FILE_FLAG_DELETE_ON_CLOSE rather then an separate delete operation), so 🤷

Either way, I think this change is fine, as we don't really want to enforce that the file can't be deleted if the source cache setting is "true".

[emrekultursay]

OK, I figured the problem. In the test, we call os.removeFile() which does not actually represent the real end-user use-case (which is to edit the file and save it).

Empirically, on Windows 11, I can verify that when LLDB mmaps these large files, then:

1. I can append to the file from Python (mode="a")
2. I can delete the file from Python (what the current test is doing)
3. I cannot delete the file from File Explorer => says lldb.exe is using the file
4. I cannot overwrite the file from Python (mode="w")
5. I cannot overwrite/save the file from Notepad++
6. I can overwrite/save the file from vi.

I am guessing that there are different mechanisms for deleting/editing/saving, and different editors/tools use different approaches.

To make the test more representative, I suggeest replacing the removeFile() method with the following:

    def overwriteFile(self, src):
        """Write to file and return true iff file was successfully written."""
        try:
            f = open(src, "w") 
            f.writelines(["// hello world\n"])
            f.close()
            return True
        except Exception:
            return False

...and then the test should start passing.

[labath]

• I can append to the file from Python (mode="a")

• I can delete the file from Python (what the current test is doing)

• I cannot delete the file from File Explorer => says lldb.exe is using the file

• I cannot overwrite the file from Python (mode="w")

• I cannot overwrite/save the file from Notepad++

• I can overwrite/save the file from vi.

Well, that's... interesting. Thanks for gathering these. I suspect that this is due to different strategies for saving a file. I know of basically two: you either delete the file and create a new one in its place (if you want a backup you can rename instead of deleting), or you overwrite the file in place (and make a copy if you want a backup). In vim, this is controlled by the backupcopy setting so it would be interesting (but not really necessary) to see if this affects vim's ability to save the file.

With this in mind, deleting the file is representative of an editor wanting to save the file using the deletion strategy, but I still think it's not our goal here to ensure the editor cannot save the file if the setting is true.

[emrekultursay]

With this in mind, deleting the file is representative of an editor wanting to save the file using the deletion strategy,...
Yes. I had meant it is not representative of what Android Studio (or some editors) does.

think it's not our goal here to ensure the editor cannot save the file if the setting is true.

I agree that having a test that sets use_source_cache=true and expects failure behavior is not a goal. But, it is how we observe that the test input cpp source file we use satisfies the condition to use mmap. If the condition to use mmap changes (e.g., from filesize=16K to filesize=32K), then the test input source file will no longer trigger mmap, yet the use_source_cache=false test will keep passing. So, I think having that negative test case, despite being weird, still serves some purpose.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[igorkudrin]

So the test basically shows that it makes sense to set use-source-cache to false on Windows, otherwise the source file will be locked for editing. Windows 11 seems to allow deleting mmaped files, so we need to try opening the file for writing to re-enable the test. Thanks, @emrekultursay.

[emrekultursay]

LGTM. Thanks.

[labath]

With this in mind, deleting the file is representative of an editor wanting to save the file using the deletion strategy,...
Yes. I had meant it is not representative of what Android Studio (or some editors) does.

think it's not our goal here to ensure the editor cannot save the file if the setting is true.

I agree that having a test that sets use_source_cache=true and expects failure behavior is not a goal. But, it is how we observe that the test input cpp source file we use satisfies the condition to use mmap. If the condition to use mmap changes (e.g., from filesize=16K to filesize=32K), then the test input source file will no longer trigger mmap, yet the use_source_cache=false test will keep passing. So, I think having that negative test case, despite being weird, still serves some purpose.

Makes sense (and now that I think about it, I think I used some line of reasoning like this on the original patch that introduced this).