Skip to content

Commit

Permalink
fix(cxx_extractor): don't crash when modules are on (#5554)
Browse files Browse the repository at this point in the history
* fix(cxx_extractor): don't crash when modules are on

In service of #5543.

This PR should only affect the behavior of the extractor
where it would have either crashed because of a null
dereference or due to a CHECK-fail. The conditions
leading to these cases occur when modules are turned on.

* fix: address comments

* fix: add a test case

* fix: address comments
  • Loading branch information
zrlk committed Apr 5, 2023
1 parent ac48a01 commit a18df6c
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 14 deletions.
1 change: 1 addition & 0 deletions kythe/cxx/extractor/BUILD
Expand Up @@ -117,6 +117,7 @@ cc_library(
"//kythe/cxx/common:path_utils",
"//kythe/cxx/common:sha256_hasher",
"//kythe/cxx/indexer/cxx:clang_utils",
"//kythe/cxx/indexer/cxx:stream_adapter",
"//kythe/proto:analysis_cc_proto",
"//kythe/proto:buildinfo_cc_proto",
"//kythe/proto:cxx_cc_proto",
Expand Down
60 changes: 46 additions & 14 deletions kythe/cxx/extractor/cxx_extractor.cc
Expand Up @@ -35,6 +35,7 @@
#include "absl/strings/string_view.h"
#include "absl/strings/strip.h"
#include "absl/types/optional.h"
#include "clang/Basic/Module.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/FrontendAction.h"
#include "clang/Lex/MacroArgs.h"
Expand All @@ -50,6 +51,7 @@
#include "kythe/cxx/extractor/CommandLineUtils.h"
#include "kythe/cxx/extractor/language.h"
#include "kythe/cxx/extractor/path_utils.h"
#include "kythe/cxx/indexer/cxx/stream_adapter.h"
#include "kythe/proto/analysis.pb.h"
#include "kythe/proto/buildinfo.pb.h"
#include "kythe/proto/cxx.pb.h"
Expand Down Expand Up @@ -83,6 +85,11 @@ constexpr absl::string_view kStableRootDirectories[] = {
"/kythe_cxx_extractor_root",
};

bool IsSpecialBufferName(llvm::StringRef id) {
return id == clang::Module::getModuleInputBufferName() ||
id == "<built-in>" || id == "<command line>";
}

absl::string_view GetPathForProto(
const proto::CxxCompilationUnitDetails::SystemHeaderPrefix& prefix) {
return prefix.prefix();
Expand Down Expand Up @@ -538,12 +545,33 @@ ExtractorPPCallbacks::ExtractorPPCallbacks(ExtractorState state)
}

void ExtractorPPCallbacks::FileChanged(
clang::SourceLocation /*Loc*/, FileChangeReason Reason,
clang::SourceLocation Loc, FileChangeReason Reason,
clang::SrcMgr::CharacteristicKind /*FileType*/, clang::FileID /*PrevFID*/) {
if (Reason == EnterFile) {
if (last_inclusion_directive_path_.empty()) {
current_files_.push(FileState{std::string(GetMainFile()->getName()),
ClaimDirective::AlwaysClaim});
if (auto* mfile = GetMainFile()) {
current_files_.push(FileState{std::string(mfile->getName()),
ClaimDirective::AlwaysClaim});
} else {
// For some compilations with modules enabled, there may be no main
// source file set. Previously we would segfault
// (`GetMainFile()->getName()`) above instead of `mfile`, so CHECK-
// failing below is no more unpleasant.
LOG(WARNING) << "unusual EnterFile @"
<< Loc.printToString(*source_manager_);
auto fid = source_manager_->getFileID(Loc);
CHECK(fid.isValid());
auto buffer = source_manager_->getBufferOrNone(fid);
CHECK(buffer.has_value());
auto id = buffer->getBufferIdentifier();
CHECK(IsSpecialBufferName(id))
<< "unknown buffer " << StreamAdapter::Stream(id);
// TODO(zarko): we need a more appropriate path for the synthesized
// <module-includes> buffer.
current_files_.push(
FileState{std::string(buffer->getBufferIdentifier()),
ClaimDirective::AlwaysClaim});
}
} else {
CHECK(!current_files_.empty());
current_files_.top().last_include_offset = last_inclusion_offset_;
Expand Down Expand Up @@ -591,8 +619,10 @@ PreprocessorTranscript ExtractorPPCallbacks::PopFile() {
}

void ExtractorPPCallbacks::EndOfMainFile() {
AddFile(GetMainFile(), std::string(GetMainFile()->getName()));
*main_source_file_transcript_ = PopFile();
if (auto* mfile = GetMainFile()) {
AddFile(mfile, std::string(mfile->getName()));
*main_source_file_transcript_ = PopFile();
}
}

std::string ExtractorPPCallbacks::FixStdinPath(const clang::FileEntry* file,
Expand Down Expand Up @@ -846,24 +876,24 @@ std::string ExtractorPPCallbacks::AddFile(const clang::FileEntry* file,
llvm::StringRef file_name,
llvm::StringRef search_path,
llvm::StringRef relative_path) {
CHECK(!current_files_.top().file_path.empty());
const auto& top_path = current_files_.top().file_path;
CHECK(!top_path.empty());
const auto search_path_entry =
source_manager_->getFileManager().getDirectory(search_path);
llvm::ErrorOr<const clang::FileEntry*> file_or =
source_manager_->getFileManager().getFile(top_path);
const auto current_file_parent_entry =
(*source_manager_->getFileManager().getFile(
current_files_.top().file_path.c_str()))
->getDir();
file_or.getError() ? nullptr : (*file_or)->getDir();
// If the include file was found relatively to the current file's parent
// directory or a search path, we need to normalize it. This is necessary
// because llvm internalizes the path by which an inode was first accessed,
// and always returns that path afterwards. If we do not normalize this
// we will get an error when we replay the compilation, as the virtual
// file system is not aware of inodes.
llvm::SmallString<1024> out_name;
if (*search_path_entry == current_file_parent_entry) {
auto parent =
llvm::sys::path::parent_path(current_files_.top().file_path.c_str())
.str();
if (!search_path_entry.getError() &&
*search_path_entry == current_file_parent_entry) {
auto parent = llvm::sys::path::parent_path(top_path).str();

// If the file is a top level file ("file.cc"), we normalize to a path
// relative to "./".
Expand All @@ -879,7 +909,9 @@ std::string ExtractorPPCallbacks::AddFile(const clang::FileEntry* file,
out_name = search_path;
llvm::sys::path::append(out_name, relative_path);
} else {
CHECK(llvm::sys::path::is_absolute(file_name)) << file_name.str();
CHECK(IsSpecialBufferName(top_path) ||
llvm::sys::path::is_absolute(file_name))
<< StreamAdapter::Stream(file_name);
out_name = file_name;
}
std::string out_name_string(out_name.str());
Expand Down
18 changes: 18 additions & 0 deletions kythe/cxx/extractor/testdata/BUILD
Expand Up @@ -82,6 +82,24 @@ cc_test(
],
)

cc_test(
name = "header_only_module_test",
srcs = ["header_only_module_test.cc"],
data = [
"header_only_module.cppmap",
"header_only_module.h",
],
deps = [
"//kythe/cxx/extractor:testlib",
"//kythe/proto:analysis_cc_proto",
"//third_party:gmock_main",
"//third_party:gtest",
"@com_github_inazarenko_protobuf_matchers//protobuf-matchers",
"@com_google_absl//absl/algorithm:container",
"@com_google_absl//absl/strings",
],
)

cc_test(
name = "extract_transcript_test",
srcs = ["extract_transcript_test.cc"],
Expand Down
3 changes: 3 additions & 0 deletions kythe/cxx/extractor/testdata/header_only_module.cppmap
@@ -0,0 +1,3 @@
module "//kythe/cxx/extractor/testdata:header_only_module" {
header "kythe/cxx/extractor/testdata/header_only_module.h"
}
1 change: 1 addition & 0 deletions kythe/cxx/extractor/testdata/header_only_module.h
@@ -0,0 +1 @@
int f();
112 changes: 112 additions & 0 deletions kythe/cxx/extractor/testdata/header_only_module_test.cc
@@ -0,0 +1,112 @@
/*
* Copyright 2019 The Kythe Authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "absl/algorithm/container.h"
#include "absl/strings/string_view.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "kythe/cxx/extractor/testlib.h"
#include "kythe/proto/analysis.pb.h"
#include "protobuf-matchers/protocol-buffer-matchers.h"

namespace kythe {
namespace {
using ::kythe::proto::CompilationUnit;
using ::protobuf_matchers::EquivToProto;

proto::CompilationUnit ExpectedCompilation() {
return ParseTextCompilationUnitOrDie(R"pb(
v_name { language: "c++" }
required_input {
v_name { path: "kythe/cxx/extractor/testdata/header_only_module.h" }
info {
path: "kythe/cxx/extractor/testdata/header_only_module.h"
digest: "1c1216acd9ba7e73f6e66b5174c9445b191ccefa3fc3a3df3c20cbd931755ca7"
}
details {
[type.googleapis.com/kythe.proto.ContextDependentVersion] {
row { source_context: "hash1" always_process: false }
}
}
}
argument: "/dummy/bin/clang"
argument: "-target"
argument: "dummy-target"
argument: "-DKYTHE_IS_RUNNING=1"
argument: "-resource-dir"
argument: "/kythe_builtins"
argument: "-I./kythe/cxx/extractor/testdata"
argument: "-xc++"
argument: "-Xclang=-emit-module"
argument: "-Xclang=-fmodules-embed-all-files"
argument: "-Xclang=-fmodules-local-submodule-visibility"
argument: "-fmodules"
argument: "-fmodule-file-deps"
argument: "-fno-implicit-modules"
argument: "-fno-implicit-module-maps"
argument: "-fmodules-strict-decluse"
argument: "-fmodule-name=//kythe/cxx/extractor/testdata:header_only_module"
argument: "-fmodule-map-file=./kythe/cxx/extractor/testdata/header_only_module.cppmap"
argument: "-Xclang=-fmodule-map-file-home-is-cwd"
argument: "-fPIE"
argument: "-fPIC"
argument: "./kythe/cxx/extractor/testdata/header_only_module.cppmap"
argument: "-o"
argument: "header_only_module.pic.pcm"
argument: "-fsyntax-only"
source_file: "./kythe/cxx/extractor/testdata/header_only_module.cppmap"
working_directory: "/root"
entry_context: "hash0"
output_key: "header_only_module.pic.pcm"
)pb");
}

TEST(CxxExtractorTest, TestHeaderOnlyModule) {
CompilationUnit unit = ExtractSingleCompilationOrDie(
{{"--with_executable",
"/dummy/bin/clang",
"-I./kythe/cxx/extractor/testdata",
"-xc++",
"-Xclang=-emit-module",
"-Xclang=-fmodules-embed-all-files",
"-Xclang=-fmodules-local-submodule-visibility",
"-fmodules",
"-fmodule-file-deps",
"-fno-implicit-modules",
"-fno-implicit-module-maps",
"-fmodules-strict-decluse",
"-fmodule-name=//kythe/cxx/extractor/testdata:header_only_module",
"-fmodule-map-file=./kythe/cxx/extractor/testdata/"
"header_only_module.cppmap",
"-Xclang=-fmodule-map-file-home-is-cwd",
"-fPIE",
"-fPIC",
"-c",
"./kythe/cxx/extractor/testdata/header_only_module.cppmap",
"-o",
"header_only_module.pic.pcm"}});

// Fix up things which may legitmately vary between runs.
// TODO(shahms): use gMock protobuf matchers when available.
CanonicalizeHashes(&unit);
unit.set_argument(2, "dummy-target");
unit.clear_details();

EXPECT_THAT(unit, EquivToProto(ExpectedCompilation()));
}

} // namespace
} // namespace kythe

0 comments on commit a18df6c

Please sign in to comment.