Page MenuHomePhabricator

Fix SourceManager::SourceFileCache insertion
AcceptedPublic

Authored by emrekultursay on Wed, Mar 25, 2:55 PM.

Details

Summary

Lookup and subsequent insert was done using uninitialized
FileSpec object, which caused the cache to be a no-op.

Bug: llvm.org/PR45310

Depends on D76804.

Diff Detail

Event Timeline

emrekultursay created this revision.Wed, Mar 25, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 25, 2:55 PM

fix link warning

Could we add a unit test for this?

IIUC, you should be able to test the actual effect you are trying to achieve by having a large source file, running "source list" to get it into the File Manager, then try to open the source file for writing in a process in a subshell that won't have the same mmap. That should fail without the patch (or with the setting turned to cache-on) and succeed with it.

Add unit tests for SourceManager::SourceFileCache

labath accepted this revision.Thu, Mar 26, 1:13 AM
labath marked an inline comment as done.

Looks good. I'm just picking some nits in the test. I'm assuming that @jingham's comment refers to the other patch (and it more-or-less matches what I wrote there already).

Does this actually depend on the other patch? It looks like an independent fix we could commit separately.

lldb/source/Core/SourceManager.cpp
699

Woops :)

lldb/unittests/Core/SourceManagerTest.cpp
12

This doesn't appear to be needed.

25–32

A test without any assertion is weird. I'd recommend just deleting this since insertion is already tested in the other tests.

61–64

I'd recommend folding these two statements into one (ASSERT_EQ(cache.FindSourceFile(...), nullptr)). That way we'll have at least a semi-reasonable error message when this fails instead of a "0xdeadbeef != 0"

This revision is now accepted and ready to land.Thu, Mar 26, 1:13 AM
emrekultursay marked 3 inline comments as done.

Applied labath@'s suggestions on test

Does this actually depend on the other patch? It looks like an independent fix we could commit separately.

This bug seems to have existed forever. Fixing it means we will have source file cache enabled for the first time. If it causes any unforeseen issues, I'd like users to have the ability to disable the cache, which is why I made this change depend on the other change.

emrekultursay edited the summary of this revision. (Show Details)Thu, Mar 26, 9:39 AM

Does this actually depend on the other patch? It looks like an independent fix we could commit separately.

This bug seems to have existed forever. Fixing it means we will have source file cache enabled for the first time. If it causes any unforeseen issues, I'd like users to have the ability to disable the cache, which is why I made this change depend on the other change.

Ok, that makes kind of sense, though I am left wondering if we really need this feature, given that we have survived so long without noticing it is missing...

Am I understanding it correctly that without this patch, we would only cache the most recently accessed file (via m_last_file_sp member), and would always reload when switching to a new file?

Ok, that makes kind of sense, though I am left wondering if we really need this feature, given that we have survived so long without noticing it is missing...

Am I understanding it correctly that without this patch, we would only cache the most recently accessed file (via m_last_file_sp member), and would always reload when switching to a new file?

Yes, without this patch, only the most recently accessed file is cached inside that member, and switching to a new file replaces that entry. I guess this was designed for optimizing the case where the user hits "next line" while staying in the same file. Yet, if a breakpoint on a different file is hit during that "next line" command, we trigger a reload (twice).

This patch will increase memory usage, as mapped source files will stay in the cache, and thus, never be unmapped. The increase will be and proportional to the size of source files loaded by LLDB on-demand.