[PATCH] D102923: [clang][lex] Remark for used header search paths

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

Thanks for your patience! I thought I already looked at this last week.

This is looking close. Comments mostly inline, but a high level point: I think this should report system search paths.

In D102923#2787001 <https://reviews.llvm.org/D102923#2787001>, @jansvoboda11 wrote:

> In D102923#2781386 <https://reviews.llvm.org/D102923#2781386>, @Bigcheese wrote:
>
>> I also wonder how we should handle other things that are found via include paths such as module maps.
>
> That's a good point. The logic for module map lookup is more complex than header lookup, so I'd be keen to tackle that in a follow-up patch.

Yup, seems like something that could be added as a follow up (although probably using the same remark). Is there a good place to leave behind a FIXME?



================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:427
+def remark_pp_include_header_search_usage : Remark<
+  "user-provided search path used: '%0'">,
+  InGroup<DiagGroup<"include-header-search-usage">>;
----------------
I suggest, more simply, "search path used:" (drop the "user-provided"). If you think it's useful for information purposes to know whether it was a user or system search path, I'd suggest using a select (in that case, maybe you want to add an optional "framework" descriptor, and/or "header map" when applicable, etc.).


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:428
+  "user-provided search path used: '%0'">,
+  InGroup<DiagGroup<"include-header-search-usage">>;
 def err_pp_file_not_found_angled_include_not_fatal : Error<
----------------
I think it'd be better to define a DiagGroup in clang/include/clang/Basic/DiagnosticGroups.td and use it here.

It might be nice for it to be more general, and not refer to headers, so we can reuse it for module maps, and/or potentially other remarks. Maybe, `-Rsearch-paths`?


================
Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165
 
+  /// Mapping from SearchDir to HeaderSearchOptions::Entry indices.
+  llvm::DenseMap<unsigned, unsigned> SearchDirToHSEntry;
+
----------------
I think this can just be a vector of `unsigned`, since the key is densely packed and counting from 0. You can use `~0U` for a sentinel for the non-entries. Maybe it's not too important though.


================
Comment at: clang/lib/Frontend/InitHeaderSearch.cpp:581-583
+    // Check whether this DirectoryLookup maps to a HeaderSearch::UserEntry.
+    if (Infos[I].UserEntryIdx)
+      LookupsToUserEntries.insert({I, *Infos[I].UserEntryIdx});
----------------
I don't see why we'd want to filter out system includes.
- Users sometimes manually configure "system" search paths, and this remark is fairly special-purpose, so I'm not sure the distinction is interesting to preserve.
- This remark is already going to be "noisy" and hit a few times on essentially every compilation. Adding the system paths that get hit won't change that much.
- The motivation for the patch is to test the logic for detecting which search paths are used in isolation from the canonicalize-explicit-module-build-commands logic in clang-scan-deps. We need to know that the logic works for system search paths as well.


================
Comment at: clang/test/Preprocessor/header-search-user-entries.c:1-4
+// RUN: %clang_cc1 -fsyntax-only %s -Rinclude-header-search-usage \
+// RUN:   -I%S/Inputs/header-search-user-entries/a -I%S/Inputs/header-search-user-entries/a_next \
+// RUN:   -I%S/Inputs/header-search-user-entries/b -I%S/Inputs/header-search-user-entries/c \
+// RUN:   -I%S/Inputs/header-search-user-entries/d 2>&1 | FileCheck %s
----------------
Can we use `-verify` here? Or does that not work for remarks?


================
Comment at: clang/test/Preprocessor/header-search-user-entries.c:9-11
+// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/a'
+// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/a_next'
+// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/d'
----------------
If `-verify` doesn't work (hopefully it does), I think you'll need to add some `CHECK-NOT` / `CHECK-NEXT` / etc. or else you're not testing for the absence of other diagnostics.

Please also test:
- Framework search path (used vs. not used)
- System search path (used vs. not used)
- One search path described relative to `-isysroot` (used vs. not used)
- Header map (matched and used vs. matched but not used vs. not matched -- for the middle case, I'm not sure what result we actually want)
- A path only used via `#if __has_include()` (when it's not followed by an `#include` or `#import`)
- A path only used via ObjC's `#import`

If one of them doesn't work and it's complex enough to separate into a follow up, I think that'd be fine, but please find somewhere to leave a FIXME.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102923/new/

https://reviews.llvm.org/D102923