Skip to content

Commit

Permalink
Merge pull request #2079 from hzeller/20240127-fix-ls-server-crash
Browse files Browse the repository at this point in the history 
Fix string view index in UpdateFileContents()
  • Loading branch information
@hzeller
hzeller authored Jan 28, 2024
2 parents a4d61b1 + 3afdb92 commit d3e3ea6
Show file tree
Hide file tree
Showing 17 changed files with 449 additions and 160 deletions.
3 changes: 3 additions & 0 deletions common/strings/string_memory_map.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ class StringViewSuperRangeMap {
return string_map_.find({substring.begin(), substring.end()});
}

// Erase given range.
const_iterator erase(const_iterator pos) { return string_map_.erase(pos); }

// Similar to find(), but asserts that a superstring range is found,
// and converts the result directly to a string_view.
absl::string_view must_find(absl::string_view substring) const {
Expand Down
22 changes: 22 additions & 0 deletions common/strings/string_memory_map_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,28 @@ TEST(StringViewSuperRangeMapTest, TwoStrings) {
});
}

TEST(StringViewSuperRangeMapTest, EraseString) {
constexpr absl::string_view text1("onestring");
constexpr absl::string_view text2("another");
StringViewSuperRangeMap svmap;
svmap.must_emplace(text1);
svmap.must_emplace(text2);

auto found = svmap.find(text1);
EXPECT_NE(found, svmap.end());
svmap.erase(found);

// should be gone now
found = svmap.find(text1);
EXPECT_EQ(found, svmap.end());

found = svmap.find(text2);
EXPECT_NE(found, svmap.end());
svmap.erase(found);

EXPECT_TRUE(svmap.empty());
}

// Function to get the owned address range of the underlying string.
static absl::string_view StringViewKey(
const std::unique_ptr<const std::string> &owned) {
Expand Down
3 changes: 3 additions & 0 deletions common/util/interval_set.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,9 @@ class DisjointIntervalSet : private internal::IntervalSetImpl {
return p;
}

// Erase given interval.
const_iterator erase(const_iterator pos) { return intervals_.erase(pos); }

// Same as emplace(), but fails fatally if emplacement fails,
// and only returns the iterator to the new map entry (which should have
// consumed 'value').
Expand Down
20 changes: 20 additions & 0 deletions common/util/interval_set_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,26 @@ TEST(DisjointIntervalSetTest, MustEmplaceOverlapsUpper) {
EXPECT_DEATH(iset.must_emplace(45, 55), "Failed to emplace");
}

TEST(DisjointIntervalSetTest, EraseRange) {
IntIntervalSet iset;
iset.must_emplace(30, 40);
iset.must_emplace(50, 60);

auto found = iset.find(35);
EXPECT_NE(found, iset.end());
iset.erase(found);

// ... should be gone now.
found = iset.find(35);
EXPECT_EQ(found, iset.end());

found = iset.find(55);
EXPECT_NE(found, iset.end());
iset.erase(found);

EXPECT_TRUE(iset.empty());
}

TEST(DisjointIntervalMapTest, FindInterval) {
IntIntervalSet iset;
iset.must_emplace(20, 25);
Expand Down
156 changes: 80 additions & 76 deletions verilog/analysis/verilog_project.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ static constexpr verilog::VerilogPreprocess::Config kPreprocessConfig{
};

VerilogSourceFile::VerilogSourceFile(absl::string_view referenced_path,
const absl::Status& status)
const absl::Status &status)
: referenced_path_(referenced_path), status_(status) {}

absl::Status VerilogSourceFile::Open() {
Expand Down Expand Up @@ -89,7 +89,7 @@ absl::Status VerilogSourceFile::Parse() {
return status_;
}

const verible::TextStructureView* VerilogSourceFile::GetTextStructure() const {
const verible::TextStructureView *VerilogSourceFile::GetTextStructure() const {
if (analyzed_structure_ == nullptr) return nullptr;
return &analyzed_structure_->Data();
}
Expand All @@ -101,90 +101,95 @@ std::vector<std::string> VerilogSourceFile::ErrorMessages() const {
return result;
}

std::ostream& operator<<(std::ostream& stream,
const VerilogSourceFile& source) {
std::ostream &operator<<(std::ostream &stream,
const VerilogSourceFile &source) {
stream << "referenced path: " << source.ReferencedPath() << std::endl;
stream << "resolved path: " << source.ResolvedPath() << std::endl;
stream << "corpus: " << source.Corpus() << std::endl;
const absl::Status status = source.Status();
stream << "status: " << (status.ok() ? "ok" : status.message()) << std::endl;
const auto content = source.GetContent();
stream << "have content? " << (!content.empty() ? "yes" : "no") << std::endl;
const auto* text_structure = source.GetTextStructure();
const auto *text_structure = source.GetTextStructure();
stream << "have text structure? " << (text_structure ? "yes" : "no")
<< std::endl;
return stream;
}

absl::Status InMemoryVerilogSourceFile::Open() {
processing_state_ = ProcessingState::kOpened;
status_ = absl::OkStatus();
return status_;
void VerilogProject::ContentToFileIndex::Register(
const VerilogSourceFile *file) {
CHECK(file);
const absl::string_view content = file->GetContent();
string_view_map_.must_emplace(content);
const auto map_inserted =
buffer_to_analyzer_map_.emplace(content.begin(), file);
CHECK(map_inserted.second);
}

absl::Status ParsedVerilogSourceFile::Open() {
processing_state_ = ProcessingState::kOpened;
status_ = absl::OkStatus();
return status_;
void VerilogProject::ContentToFileIndex::Unregister(
const VerilogSourceFile *file) {
CHECK(file);
const absl::string_view content = file->GetContent();
auto full_content_found = string_view_map_.find(content);
CHECK(full_content_found != string_view_map_.end());
string_view_map_.erase(full_content_found);
buffer_to_analyzer_map_.erase(content.begin());
}

absl::Status ParsedVerilogSourceFile::Parse() {
processing_state_ = ProcessingState::kParsed;
status_ = absl::OkStatus();
return status_;
}
const VerilogSourceFile *VerilogProject::ContentToFileIndex::Lookup(
absl::string_view content_substring) const {
// Look for corresponding source text (superstring) buffer start.
const auto found_superstring = string_view_map_.find(content_substring);
if (found_superstring == string_view_map_.end()) return nullptr;
const absl::string_view::const_iterator buffer_start =
found_superstring->first;

const verible::TextStructureView* ParsedVerilogSourceFile::GetTextStructure()
const {
return text_structure_;
// Reverse-lookup originating file based on buffer start.
const auto found_file = buffer_to_analyzer_map_.find(buffer_start);
if (found_file == buffer_to_analyzer_map_.end()) return nullptr;

return found_file->second;
}

absl::StatusOr<VerilogSourceFile*> VerilogProject::OpenFile(
absl::StatusOr<VerilogSourceFile *> VerilogProject::OpenFile(
absl::string_view referenced_filename, absl::string_view resolved_filename,
absl::string_view corpus) {
const auto inserted = files_.emplace(
referenced_filename, std::make_unique<VerilogSourceFile>(
referenced_filename, resolved_filename, corpus));
CHECK(inserted.second); // otherwise, would have already returned above
const auto file_iter = inserted.first;
VerilogSourceFile& file(*file_iter->second);
VerilogSourceFile &file(*file_iter->second);

// Read the file's contents.
if (absl::Status status = file.Open(); !status.ok()) {
return status;
}

// NOTE: string view maps don't support removal operation. The following block
// is valid only if files won't be removed from the project.
if (populate_string_maps_) {
const absl::string_view contents(file.GetContent());

// Register the file's contents range in string_view_map_.
string_view_map_.must_emplace(contents);

// Map the start of the file's contents to its corresponding owner
// VerilogSourceFile.
const auto map_inserted =
buffer_to_analyzer_map_.emplace(contents.begin(), file_iter);
CHECK(map_inserted.second);
}
if (content_index_) content_index_->Register(&file);

return &file;
}

bool VerilogProject::RemoveByName(const std::string &filename) {
NameToFileMap::const_iterator found = files_.find(filename);
if (found == files_.end()) return false;
if (content_index_) content_index_->Unregister(found->second.get());
files_.erase(found);
return true;
}

// TODO: explain better in the header what happens with includes.
bool VerilogProject::RemoveRegisteredFile(
absl::string_view referenced_filename) {
CHECK(!populate_string_maps_)
<< "Removing of files added to string maps is not supported! Disable "
"populating string maps.";
if (files_.erase(std::string(referenced_filename)) == 1) {
if (RemoveByName(std::string(referenced_filename))) {
LOG(INFO) << "Removed " << referenced_filename << " from the project.";
return true;
}
for (const auto& include_path : include_paths_) {
for (const auto &include_path : include_paths_) {
const std::string resolved_filename =
verible::file::JoinPath(include_path, referenced_filename);
if (files_.erase(resolved_filename) == 1) {
if (RemoveByName(resolved_filename)) {
LOG(INFO) << "Removed " << resolved_filename << " from the project.";
return true;
}
Expand All @@ -204,28 +209,39 @@ std::string VerilogProject::GetRelativePathToSource(
}

void VerilogProject::UpdateFileContents(
absl::string_view path, const verible::TextStructureView* updatedtext) {
std::string projectpath = GetRelativePathToSource(path);
absl::string_view path, const verilog::VerilogAnalyzer *parsed) {
constexpr absl::string_view kCorpus = "";
const std::string projectpath = GetRelativePathToSource(path);

// If we get a non-null parsed file, use that, otherwise fall back to
// file based loading.
std::unique_ptr<VerilogSourceFile> contents = nullptr;
if (updatedtext) {
contents = std::make_unique<ParsedVerilogSourceFile>(
projectpath, path, updatedtext, /*corpus=*/"");
std::unique_ptr<VerilogSourceFile> source_file;
bool do_register_content = true;
if (parsed) {
source_file = std::make_unique<ParsedVerilogSourceFile>(projectpath, path,
*parsed, kCorpus);
} else {
contents = std::make_unique<VerilogSourceFile>(projectpath, path, "");
source_file =
std::make_unique<VerilogSourceFile>(projectpath, path, kCorpus);
do_register_content = source_file->Open().ok();
}

auto fileptr = files_.find(projectpath);
if (fileptr == files_.end()) {
files_.emplace(projectpath, std::move(contents));
if (content_index_ && do_register_content) {
content_index_->Register(source_file.get());
}

auto previously_existing = files_.find(projectpath);
if (previously_existing == files_.end()) {
files_.emplace(projectpath, std::move(source_file));
} else {
fileptr->second = std::move(contents);
if (content_index_) {
content_index_->Unregister(previously_existing->second.get());
}
previously_existing->second = std::move(source_file);
}
}

VerilogSourceFile* VerilogProject::LookupRegisteredFileInternal(
VerilogSourceFile *VerilogProject::LookupRegisteredFileInternal(
absl::string_view referenced_filename) const {
const auto opened_file = FindOpenedFile(referenced_filename);
if (opened_file) {
Expand All @@ -236,7 +252,7 @@ VerilogSourceFile* VerilogProject::LookupRegisteredFileInternal(
}

// Check if this is already opened include file
for (const auto& include_path : include_paths_) {
for (const auto &include_path : include_paths_) {
const std::string resolved_filename =
verible::file::JoinPath(include_path, referenced_filename);
const auto opened_file = FindOpenedFile(resolved_filename);
Expand All @@ -247,7 +263,7 @@ VerilogSourceFile* VerilogProject::LookupRegisteredFileInternal(
return nullptr;
}

absl::optional<absl::StatusOr<VerilogSourceFile*>>
absl::optional<absl::StatusOr<VerilogSourceFile *>>
VerilogProject::FindOpenedFile(absl::string_view filename) const {
const auto found = files_.find(filename);
if (found != files_.end()) {
Expand All @@ -259,7 +275,7 @@ VerilogProject::FindOpenedFile(absl::string_view filename) const {
return absl::nullopt;
}

absl::StatusOr<VerilogSourceFile*> VerilogProject::OpenTranslationUnit(
absl::StatusOr<VerilogSourceFile *> VerilogProject::OpenTranslationUnit(
absl::string_view referenced_filename) {
// Check for a pre-existing entry to avoid duplicate files.
{
Expand Down Expand Up @@ -290,7 +306,7 @@ absl::Status VerilogProject::IncludeFileNotFoundError(
"' among the included paths: ", absl::StrJoin(include_paths_, ", ")));
}

absl::StatusOr<VerilogSourceFile*> VerilogProject::OpenIncludedFile(
absl::StatusOr<VerilogSourceFile *> VerilogProject::OpenIncludedFile(
absl::string_view referenced_filename) {
VLOG(1) << __FUNCTION__ << ", referenced: " << referenced_filename;
// Check for a pre-existing entry to avoid duplicate files.
Expand All @@ -302,7 +318,7 @@ absl::StatusOr<VerilogSourceFile*> VerilogProject::OpenIncludedFile(
}

// Check if this is already opened include file
for (const auto& include_path : include_paths_) {
for (const auto &include_path : include_paths_) {
const std::string resolved_filename =
verible::file::JoinPath(include_path, referenced_filename);
const auto opened_file = FindOpenedFile(resolved_filename);
Expand All @@ -312,7 +328,7 @@ absl::StatusOr<VerilogSourceFile*> VerilogProject::OpenIncludedFile(
}

// Locate the file among the base paths.
for (const auto& include_path : include_paths_) {
for (const auto &include_path : include_paths_) {
const std::string resolved_filename =
verible::file::JoinPath(include_path, referenced_filename);
if (verible::file::FileExists(resolved_filename).ok()) {
Expand Down Expand Up @@ -349,22 +365,10 @@ void VerilogProject::AddVirtualFile(absl::string_view resolved_filename,
}
}

const VerilogSourceFile* VerilogProject::LookupFileOrigin(
const VerilogSourceFile *VerilogProject::LookupFileOrigin(
absl::string_view content_substring) const {
CHECK(populate_string_maps_)
<< "Populating string maps must be enabled for LookupFileOrigin!";
// Look for corresponding source text (superstring) buffer start.
const auto found_superstring = string_view_map_.find(content_substring);
if (found_superstring == string_view_map_.end()) return nullptr;
const absl::string_view::const_iterator buffer_start =
found_superstring->first;

// Reverse-lookup originating file based on buffer start.
const auto found_file = buffer_to_analyzer_map_.find(buffer_start);
if (found_file == buffer_to_analyzer_map_.end()) return nullptr;

const VerilogSourceFile* file = found_file->second->second.get();
return file;
CHECK(content_index_) << "LookupFileOrigin() not enabled in constructor";
return content_index_->Lookup(content_substring);
}

} // namespace verilog
Loading

0 comments on commit d3e3ea6

Please sign in to comment.