Handle internal headers guarded by macro and x-macros (fix issue #109).

I've made a few trade-offs in implementation which I'd like to explain more.

First, code doesn't distinguish between guarded internal headers and
headers with x-macros. They are handled the same way. But in tests both
patterns are tested. It is done not to cover all code paths but to test
include-what-you-use from user's perspective.

Also I check if file defining macro is immediate includer. I decided not
to check if it includes file using the macro transitively until we have
such real-life use cases. Current implementation is strict in order to avoid
unexpected results.

For some cases I am reusing mechanism that keeps files included with the
"IWYU pragma: keep" comment. The downside is that it keeps all lines
including this file which might be not entirely correct for x-macros.
Though x-macros are close to pure textual includes and we cannot reason
about textual includes.

I also didn't include a few test cases because I think they don't
represent real-life use cases. These test cases are:
* when associated header is included by non-associated header;
* when file defining macro and file using macro are both included by the
  third file.
This commit is contained in:
Volodymyr Sapsai 2016-08-14 17:28:46 -07:00
parent 2f826a3323
commit 04f1c92537
20 changed files with 367 additions and 37 deletions

View File

@ -598,6 +598,10 @@ void IwyuFileInfo::ReportMacroUse(clang::SourceLocation use_loc,
LogSymbolUse("Marked full-info use of macro", symbol_uses_.back());
}
void IwyuFileInfo::ReportDefinedMacroUse(const clang::FileEntry* used_in) {
macro_users_.insert(used_in);
}
void IwyuFileInfo::ReportIncludeFileUse(const clang::FileEntry* included_file,
const string& quoted_include) {
symbol_uses_.push_back(OneUse("", included_file, quoted_include,
@ -605,7 +609,7 @@ void IwyuFileInfo::ReportIncludeFileUse(const clang::FileEntry* included_file,
LogSymbolUse("Marked use of include-file", symbol_uses_.back());
}
void IwyuFileInfo::ReportPragmaKeep(const clang::FileEntry* included_file) {
void IwyuFileInfo::ReportKnownDesiredFile(const FileEntry* included_file) {
kept_includes_.insert(included_file);
}
@ -1887,6 +1891,41 @@ size_t PrintableDiffs(const string& filename,
} // namespace internal
void IwyuFileInfo::HandlePreprocessingDone() {
// Check macros defined by includer. Requires file preprocessing to be
// finished to know all direct includes and all macro usages.
bool should_report_violations = ShouldReportIWYUViolationsFor(file_);
std::list<const FileEntry*> direct_macro_use_includees;
std::set_intersection(macro_users_.begin(), macro_users_.end(),
direct_includes_as_fileentries_.begin(),
direct_includes_as_fileentries_.end(),
std::inserter(direct_macro_use_includees,
direct_macro_use_includees.end()));
for (const FileEntry* macro_use_includee : direct_macro_use_includees) {
if (should_report_violations) {
ERRSYM(file_) << "Keep #include " << macro_use_includee->getName()
<< " in " << file_->getName()
<< " because used macro is defined by includer.\n";
ReportKnownDesiredFile(macro_use_includee);
} else {
string private_include =
ConvertToQuotedInclude(GetFilePath(macro_use_includee));
if (GlobalIncludePicker().IsPublic(macro_use_includee)) {
ERRSYM(file_) << "Skip marking " << quoted_file_
<< " as public header for " << private_include
<< " because latter is already marked as public,"
<< " though uses macro defined by includer.\n";
} else {
ERRSYM(file_) << "Mark " << quoted_file_
<< " as public header for " << private_include
<< " because used macro is defined by includer.\n";
MutableGlobalIncludePicker()->AddMapping(private_include, quoted_file_);
MutableGlobalIncludePicker()->MarkIncludeAsPrivate(private_include);
}
}
}
}
void IwyuFileInfo::ResolvePendingAnalysis() {
// Resolve using declarations: This handles the case where there's a using
// declaration in the file but no code is actually using it. If that

View File

@ -235,10 +235,14 @@ class IwyuFileInfo {
const string& symbol);
// TODO(dsturtevant): Can we determine in_cxx_method_body? Do we care?
// Called when using a macro in this file.
void ReportMacroUse(clang::SourceLocation use_loc,
clang::SourceLocation dfn_loc,
const string& symbol);
// Called when somebody uses a macro defined in this file.
void ReportDefinedMacroUse(const clang::FileEntry* used_in);
// We only allow forward-declaring of decls, not arbitrary symbols.
void ReportForwardDeclareUse(clang::SourceLocation use_loc,
const clang::NamedDecl* decl,
@ -256,15 +260,19 @@ class IwyuFileInfo {
void ReportIncludeFileUse(const clang::FileEntry* included_file,
const string& quoted_include);
// This is used when we see an "IWYU pragma: keep" comment
// on an include line.
void ReportPragmaKeep(const clang::FileEntry* included_file);
// This is used when we see a file we want to keep not due to symbol-use
// reasons. For example, it can be #included with an "IWYU pragma: keep"
// comment or it can be an x-macro.
void ReportKnownDesiredFile(const clang::FileEntry* included_file);
// This is used only in iwyu_preprocessor.cc. TODO(csilvers): revamp?
const set<const clang::FileEntry*>& direct_includes_as_fileentries() const {
return direct_includes_as_fileentries_;
}
// Called when all macros in the file are processed.
void HandlePreprocessingDone();
// Resolve and pending analysis that needs to occur between AST traversal
// and CalculateAndReportIwyuViolations.
void ResolvePendingAnalysis();
@ -345,9 +353,13 @@ class IwyuFileInfo {
set<const clang::FileEntry*> direct_includes_as_fileentries_;
set<const clang::NamedDecl*> direct_forward_declares_;
// Holds any files included with the "IWYU pragma: keep" comment.
// Holds files forced to be kept. For example, files included with the
// "IWYU pragma: keep" comment and x-macros.
set<const clang::FileEntry*> kept_includes_;
// Holds files using macros defined in this file.
set<const clang::FileEntry*> macro_users_;
// What we will recommend the #includes to be.
set<string> desired_includes_;
bool desired_includes_have_been_calculated_;

View File

@ -11,6 +11,8 @@
#include <cstddef> // for size_t
#include <cstring>
#include <algorithm>
#include <iterator>
#include <string> // for string, basic_string, etc
#include <utility> // for pair, make_pair
@ -47,13 +49,6 @@ using std::string;
namespace SrcMgr = clang::SrcMgr;
// Prints to errs() if the verbose level is at a high enough level to
// print symbols that occur in the given file. This is only valid
// when used inside a class, such as IwyuAstConsumer, that defines a
// method named ShouldPrintSymbolFromFile().
#define ERRSYM(file_entry) \
if (!ShouldPrintSymbolFromFile(file_entry)) ; else ::llvm::errs()
namespace include_what_you_use {
namespace {
@ -393,7 +388,7 @@ void IwyuPreprocessorInfo::MaybeProtectInclude(
if (IncludeLineHasText(includer_loc, "// IWYU pragma: keep") ||
IncludeLineHasText(includer_loc, "/* IWYU pragma: keep")) {
protect_reason = "pragma_keep";
FileInfoFor(includer)->ReportPragmaKeep(includee);
FileInfoFor(includer)->ReportKnownDesiredFile(includee);
} else if (IncludeLineHasText(includer_loc, "// IWYU pragma: export") ||
IncludeLineHasText(includer_loc, "/* IWYU pragma: export") ||
@ -640,13 +635,14 @@ void IwyuPreprocessorInfo::InclusionDirective(
void IwyuPreprocessorInfo::FileChanged(SourceLocation loc,
FileChangeReason reason,
SrcMgr::CharacteristicKind file_type,
FileID exiting_from) {
FileID exiting_from_id) {
switch (reason) {
case EnterFile:
FileChanged_EnterFile(loc);
return;
case ExitFile:
FileChanged_ExitToFile(loc, exiting_from);
FileChanged_ExitToFile(
loc, GlobalSourceManager()->getFileEntryForID(exiting_from_id));
return;
case RenameFile:
FileChanged_RenameFile(loc);
@ -737,12 +733,10 @@ void IwyuPreprocessorInfo::FileChanged_EnterFile(
}
// Called when done with an #included file and returning to the parent file.
void IwyuPreprocessorInfo::FileChanged_ExitToFile(SourceLocation include_loc,
FileID exiting_from_id) {
void IwyuPreprocessorInfo::FileChanged_ExitToFile(
SourceLocation include_loc, const FileEntry* exiting_from) {
ERRSYM(GetFileEntry(include_loc)) << "[ Exiting to ] "
<< PrintableLoc(include_loc) << "\n";
const FileEntry* exiting_from = GlobalSourceManager()->getFileEntryForID(
exiting_from_id);
if (HasOpenBeginExports(exiting_from)) {
Warn(begin_exports_location_stack_.top(),
"begin_exports without an end_exports");
@ -767,27 +761,29 @@ void IwyuPreprocessorInfo::FileChanged_SystemHeaderPragma(SourceLocation loc) {
void IwyuPreprocessorInfo::ReportMacroUse(const string& name,
SourceLocation usage_location,
SourceLocation dfn_location) {
const FileEntry* used_in = GetFileEntry(usage_location);
if (!ShouldReportIWYUViolationsFor(used_in))
return; // ignore symbols used outside foo.{h,cc}
// Don't report macro uses that aren't actually in a file somewhere.
if (!dfn_location.isValid() || GetFilePath(dfn_location) == "<built-in>")
return;
const FileEntry* used_in = GetFileEntry(usage_location);
if (ShouldReportIWYUViolationsFor(used_in)) {
// ignore symbols used outside foo.{h,cc}
// TODO(csilvers): this isn't really a symbol use -- it may be ok
// that the symbol isn't defined. For instance:
// foo.h: #define FOO
// bar.h: #ifdef FOO ... #else ... #endif
// baz.cc: #include "foo.h"
// #include "bar.h"
// bang.cc: #include "bar.h"
// We don't want to say that bar.h 'uses' FOO, and thus needs to
// #include foo.h -- adding that #include could break bang.cc.
// I think the solution is to have a 'soft' use -- don't remove it
// if it's there, but don't add it if it's not. Or something.
GetFromFileInfoMap(used_in)->ReportMacroUse(usage_location, dfn_location,
name);
// TODO(csilvers): this isn't really a symbol use -- it may be ok
// that the symbol isn't defined. For instance:
// foo.h: #define FOO
// bar.h: #ifdef FOO ... #else ... #endif
// baz.cc: #include "foo.h"
// #include "bar.h"
// bang.cc: #include "bar.h"
// We don't want to say that bar.h 'uses' FOO, and thus needs to
// #include foo.h -- adding that #include could break bang.cc.
// I think the solution is to have a 'soft' use -- don't remove it
// if it's there, but don't add it if it's not. Or something.
GetFromFileInfoMap(used_in)->ReportMacroUse(usage_location, dfn_location,
name);
}
const FileEntry* defined_in = GetFileEntry(dfn_location);
GetFromFileInfoMap(defined_in)->ReportDefinedMacroUse(used_in);
}
// As above, but get the definition location from macros_definition_loc_.
@ -934,6 +930,9 @@ void IwyuPreprocessorInfo::PopulateTransitiveIncludeMap() {
// The public API.
void IwyuPreprocessorInfo::HandlePreprocessingDone() {
CHECK_(main_file_ && "Main file should be present");
FileChanged_ExitToFile(SourceLocation(), main_file_);
// In some cases, macros can refer to macros in files that are
// defined later in other files. In those cases, we can't
// do an iwyu check until all header files have been read.
@ -945,6 +944,9 @@ void IwyuPreprocessorInfo::HandlePreprocessingDone() {
}
// Other post-processing steps.
for (auto& file_info_map_entry : iwyu_file_info_map_) {
file_info_map_entry.second.HandlePreprocessingDone();
}
MutableGlobalIncludePicker()->FinalizeAddedIncludes();
ProtectReexportIncludes(&iwyu_file_info_map_);
PopulateIntendsToProvideMap();

View File

@ -208,7 +208,7 @@ class IwyuPreprocessorInfo : public clang::PPCallbacks,
// FileChanged is actually a multi-plexer for 4 different callbacks.
void FileChanged_EnterFile(clang::SourceLocation file_beginning);
void FileChanged_ExitToFile(clang::SourceLocation include_loc,
clang::FileID exiting_from);
const clang::FileEntry* exiting_from);
void FileChanged_RenameFile(clang::SourceLocation new_file);
void FileChanged_SystemHeaderPragma(clang::SourceLocation loc);

View File

@ -40,6 +40,13 @@ bool ShouldPrintSymbolFromFile(const clang::FileEntry* file);
if (!::include_what_you_use::ShouldPrint( \
verbose_level)) ; else ::llvm::errs()
// Prints to errs() if the verbose level is at a high enough level to
// print symbols that occur in the given file. This is only valid
// when used inside a class, such as IwyuAstConsumer, that defines a
// method named ShouldPrintSymbolFromFile().
#define ERRSYM(file_entry) \
if (!ShouldPrintSymbolFromFile(file_entry)) ; else ::llvm::errs()
} // namespace include_what_you_use
#endif // INCLUDE_WHAT_YOU_USE_IWYU_VERRS_H_

View File

@ -98,6 +98,8 @@ class OneIwyuTest(unittest.TestCase):
'deleted_implicit.cc' : ['-std=c++11'],
'lambda_fwd_decl.cc': ['-std=c++11'],
'lateparsed_template.cc': ['-fdelayed-template-parsing'],
'macro_defined_by_includer.cc': [
'-std=c++11', '-DCOMMAND_LINE_TYPE=double'],
'ms_inline_asm.cc': ['-fms-extensions'],
'prefix_header_attribution.cc': [self.Include('prefix_header_attribution-d1.h')],
'prefix_header_includes_add.cc': prefix_headers,
@ -140,6 +142,7 @@ class OneIwyuTest(unittest.TestCase):
'iwyu_stricter_than_cpp.cc': ['.'],
'keep_mapping.cc': ['.'],
'lateparsed_template.cc': ['.'],
'macro_defined_by_includer.cc': ['.'],
'macro_location.cc': ['.'],
'member_expr.cc': ['.'],
'multiple_include_paths.cc': ['.'],

View File

@ -0,0 +1,12 @@
//===--- macro_defined_by_includer-d1.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Header file to provide extra level of inclusion indirection.
#include "tests/cxx/macro_defined_by_includer-i1.h"

View File

@ -0,0 +1,15 @@
//===--- macro_defined_by_includer-d2.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Tests case when file using macro is included from file defining macro but
// the macro user is first encountered as include in another file.
#define DIRECT_INCLUDE_GUARD_2
#include "tests/cxx/macro_defined_by_includer-i2.h"
#include "tests/cxx/macro_defined_by_includer-g2.h"

View File

@ -0,0 +1,14 @@
//===--- macro_defined_by_includer-d3.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Tests nested guarded includes. Covers the case when "public" mapping for
// some private include can be in fact private.
#define DIRECT_INCLUDE_GUARD_3
#include "tests/cxx/macro_defined_by_includer-g3.h"

View File

@ -0,0 +1,12 @@
//===--- macro_defined_by_includer-d4.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Header file to provide extra level of inclusion indirection.
#include "tests/cxx/macro_defined_by_includer-i3.h"

View File

@ -0,0 +1,14 @@
//===--- macro_defined_by_includer-g1.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#ifndef DIRECT_INCLUDE_GUARD_1
#error Do not include directly
#else
class GuardedInclude1 {};
#endif

View File

@ -0,0 +1,19 @@
//===--- macro_defined_by_includer-g2.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#ifndef INCLUDE_WHAT_YOU_USE_TESTS_CXX_MACRO_DEFINED_BY_INCLUDER_G2_H_
#define INCLUDE_WHAT_YOU_USE_TESTS_CXX_MACRO_DEFINED_BY_INCLUDER_G2_H_
#ifndef DIRECT_INCLUDE_GUARD_2
#error Do not include directly
#else
class GuardedInclude2 {};
#endif
#endif // INCLUDE_WHAT_YOU_USE_TESTS_CXX_MACRO_DEFINED_BY_INCLUDER_G2_H_

View File

@ -0,0 +1,17 @@
//===--- macro_defined_by_includer-g3.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#define DIRECT_INCLUDE_GUARD_4
#include "tests/cxx/macro_defined_by_includer-g4.h"
#ifndef DIRECT_INCLUDE_GUARD_3
#error Do not include directly
#else
class GuardedInclude3 {};
#endif

View File

@ -0,0 +1,14 @@
//===--- macro_defined_by_includer-g4.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#ifndef DIRECT_INCLUDE_GUARD_4
#error Do not include directly
#else
class GuardedInclude4 {};
#endif

View File

@ -0,0 +1,14 @@
//===--- macro_defined_by_includer-g5.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#ifndef DIRECT_INCLUDE_GUARD_5
#error Do not include directly
#else
class GuardedInclude5 {};
#endif

View File

@ -0,0 +1,11 @@
//===--- macro_defined_by_includer-i1.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#define DIRECT_INCLUDE_GUARD_1
#include "tests/cxx/macro_defined_by_includer-g1.h"

View File

@ -0,0 +1,10 @@
//===--- macro_defined_by_includer-i2.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#include "tests/cxx/macro_defined_by_includer-g2.h"

View File

@ -0,0 +1,14 @@
//===--- macro_defined_by_includer-i3.h - test input file for iwyu --------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Header file to test including file that uses x-macro.
#define TYPE char
#include "tests/cxx/macro_defined_by_includer-xmacro.h"
#undef TYPE

View File

@ -0,0 +1,12 @@
//===--- macro_defined_by_includer-xmacro.h - test input file for iwyu ----===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// File containing X-macro templates.
void f(TYPE const t) {}

View File

@ -0,0 +1,89 @@
//===--- macro_defined_by_includer.cc - test input file for iwyu ----------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
// Tests a few macro patterns:
// * internal headers guarded by macro defined in designated header;
// * x-macros.
//
// Usually you include a file with a macro and use that macro. But in these
// macro patterns macro is defined by includer and includee doesn't know where
// the macro comes from.
// Test guarded includes.
#include "tests/cxx/macro_defined_by_includer-d1.h"
// IWYU: GuardedInclude1 is...*macro_defined_by_includer-i1.h
GuardedInclude1 g1;
#include "tests/cxx/macro_defined_by_includer-d2.h"
GuardedInclude2 g2;
#include "tests/cxx/macro_defined_by_includer-d3.h"
GuardedInclude3 g3;
GuardedInclude4 g4;
#define DIRECT_INCLUDE_GUARD_5
#include "tests/cxx/macro_defined_by_includer-g5.h"
GuardedInclude5 g5;
// Test x-macros.
#include "tests/cxx/macro_defined_by_includer-d4.h"
#define TYPE int
#include "tests/cxx/macro_defined_by_includer-xmacro.h"
#undef TYPE
// For x-macros we keep all includes even if its content isn't used.
#define TYPE double
#include "tests/cxx/macro_defined_by_includer-xmacro.h"
#undef TYPE
int main() {
// IWYU: f is...*macro_defined_by_includer-i3.h
f(3);
// IWYU: f is...*macro_defined_by_includer-i3.h
f('a');
}
// Test macro defined on command line. Make sure that detecting file defining
// macro works without actual file on disk.
COMMAND_LINE_TYPE x;
// Clang internal <limits.h> defines LLONG_MIN and #include_next system
// <limits.h> which on Mac OS X uses LLONG_MIN.
//
// Test that we don't create a mapping between those 2 <limits.h> and don't try
// to mark system <limits.h> as private.
#include <limits.h>
/**** IWYU_SUMMARY
tests/cxx/macro_defined_by_includer.cc should add these lines:
#include "tests/cxx/macro_defined_by_includer-i1.h"
#include "tests/cxx/macro_defined_by_includer-i3.h"
tests/cxx/macro_defined_by_includer.cc should remove these lines:
- #include <limits.h> // lines XX-XX
- #include "tests/cxx/macro_defined_by_includer-d1.h" // lines XX-XX
- #include "tests/cxx/macro_defined_by_includer-d4.h" // lines XX-XX
The full include-list for tests/cxx/macro_defined_by_includer.cc:
#include "tests/cxx/macro_defined_by_includer-d2.h" // for GuardedInclude2
#include "tests/cxx/macro_defined_by_includer-d3.h" // for GuardedInclude3, GuardedInclude4
#include "tests/cxx/macro_defined_by_includer-g5.h" // for GuardedInclude5
#include "tests/cxx/macro_defined_by_includer-i1.h" // for GuardedInclude1
#include "tests/cxx/macro_defined_by_includer-i3.h" // for f
#include "tests/cxx/macro_defined_by_includer-xmacro.h" // lines XX-XX
#include "tests/cxx/macro_defined_by_includer-xmacro.h" // lines XX-XX
***** IWYU_SUMMARY */