Fix SourceManager::SourceFileCache insertion

[emrekultursay]

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.

[emrekultursay]

fix link warning

[JDevlieghere]

Could we add a unit test for this?

[jingham]

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.

[emrekultursay]

Add unit tests for SourceManager::SourceFileCache

[labath]

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"

[emrekultursay]

Applied labath@'s suggestions on test

[emrekultursay]

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.

[labath]

In D76805#1944198, @emrekultursay wrote:

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?

[emrekultursay]

In D76805#1949642, @labath wrote:

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.

lldb/source/Core/SourceManager.cpp

bool SourceManager::File::GetLine(uint32_t line_no, std::string &buffer) {
   }
   buffer.assign((char *)m_data_sp->GetBytes() + start_offset,
                 end_offset - start_offset);
 
   return true;
 }
 
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
-  FileSpec file_spec;
+  FileSpec file_spec = file_sp->GetFileSpec();
   FileCache::iterator pos = m_file_cache.find(file_spec);
   if (pos == m_file_cache.end())
     m_file_cache[file_spec] = file_sp;
   else {
     if (file_sp != pos->second)
       m_file_cache[file_spec] = file_sp;
   }
 }

lldb/unittests/Core/CMakeLists.txt

 add_lldb_unittest(LLDBCoreTests
   CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
+  SourceManagerTest.cpp
   StreamCallbackTest.cpp
   UniqueCStringMapTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbSymbol
     lldbPluginObjectFileELF
     lldbPluginSymbolFileSymtab
     lldbUtilityHelpers
     LLVMTestingSupport
   LINK_COMPONENTS
     Support
   )

lldb/unittests/Core/SourceManagerTest.cpp

+//===-- SourceManagerTest.cpp ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Core/SourceManager.h"
+#include "lldb/Host/FileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class SourceFileCache : public ::testing::Test {
+public:
+  void SetUp() override { FileSystem::Initialize(); }
+  void TearDown() override { FileSystem::Terminate(); }
+};
+
+TEST_F(SourceFileCache, FindSourceFileFound) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp =
+      std::make_shared<SourceManager::File>(foo_file_spec, nullptr);
+  cache.AddSourceFile(foo_file_sp);
+
+  // Query: foo, expect found.
+  FileSpec another_foo_file_spec("foo");
+  ASSERT_EQ(cache.FindSourceFile(another_foo_file_spec), foo_file_sp);
+}
+
+TEST_F(SourceFileCache, FindSourceFileNotFound) {
+  SourceManager::SourceFileCache cache;
+
+  // Insert: foo
+  FileSpec foo_file_spec("foo");
+  auto foo_file_sp =
+      std::make_shared<SourceManager::File>(foo_file_spec, nullptr);
+  cache.AddSourceFile(foo_file_sp);
+
+  // Query: bar, expect not found.
+  FileSpec bar_file_spec("bar");
+  ASSERT_EQ(cache.FindSourceFile(bar_file_spec), nullptr);
+}