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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I believe I've addressed all suggestions provided so far. @dexonsmith & 
@Bigcheese, can you check again?




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'

jansvoboda11 wrote:
> dexonsmith wrote:
> > 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.
> Good point, I'll work on improving the coverage.
I tried using `-verify` together with `expected-remark-re` (to match 
**relative** paths) and `@*` (to match any/no line number), but those don't 
seem to work together.

This expectation:
```
// expected-remark-re @* {{search path used: '{{.*}}/search-path-usage/FwA'}}
```
produces:
```
error: 'error' diagnostics seen but not expected: 
  File 
/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/search-path-usage-headers.m
 Line 30: cannot find start ('{{') of expected regex
error: 'remark' diagnostics seen but not expected: 
  (frontend): search path used: 
'/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/Inputs/search-path-usage/FwA'
2 errors generated.
```

If I drop `@*`, stuff doesn't work (presumably because of line numbers):
```
error: 'remark' diagnostics expected but not seen: 
  File 
/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/search-path-usage-headers.m
 Line 30: search path used: '{{.*}}/search-path-usage/FwA'
error: 'remark' diagnostics seen but not expected: 
  (frontend): search path used: 
'/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/Inputs/search-path-usage/FwA'
2 errors generated.
```

Let me know if I'm doing something wrong. In the meantime, I made the FileCheck 
checks more thorough as you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 378222.
jansvoboda11 added a comment.

Rebase, handle module maps, add more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Lex/HeaderSearch.cpp
  
clang/test/Preprocessor/Inputs/search-path-usage/FwA/FrameworkA.framework/Headers/FrameworkA.h
  
clang/test/Preprocessor/Inputs/search-path-usage/FwA/FrameworkA.framework/Modules/module.modulemap
  
clang/test/Preprocessor/Inputs/search-path-usage/FwB/FrameworkB.framework/Headers/FrameworkB.h
  
clang/test/Preprocessor/Inputs/search-path-usage/FwB/FrameworkB.framework/Modules/module.modulemap
  clang/test/Preprocessor/Inputs/search-path-usage/a/a.h
  clang/test/Preprocessor/Inputs/search-path-usage/a_next/a.h
  clang/test/Preprocessor/Inputs/search-path-usage/b.hmap.json.template
  clang/test/Preprocessor/Inputs/search-path-usage/b/b.h
  clang/test/Preprocessor/Inputs/search-path-usage/d/d.h
  
clang/test/Preprocessor/Inputs/search-path-usage/modulemap_abs/module.modulemap.template
  clang/test/Preprocessor/search-path-usage-headers.m

Index: clang/test/Preprocessor/search-path-usage-headers.m
===
--- /dev/null
+++ clang/test/Preprocessor/search-path-usage-headers.m
@@ -0,0 +1,119 @@
+// RUN: rm -rf %t && mkdir %t
+
+// Check that search paths used by `#include` and `#include_next` are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage   \
+// RUN:   -I%S/Inputs/search-path-usage/a  \
+// RUN:   -I%S/Inputs/search-path-usage/a_next \
+// RUN:   -I%S/Inputs/search-path-usage/b  \
+// RUN:   -I%S/Inputs/search-path-usage/c  \
+// RUN:   -I%S/Inputs/search-path-usage/d  \
+// RUN:   -DINCLUDE 2>&1 | FileCheck %s --check-prefix=CHECK-INCLUDE
+#ifdef INCLUDE
+#include "a.h"
+#include "d.h"
+// CHECK-INCLUDE-NOT:  remark:
+// CHECK-INCLUDE:  remark: search path used: '{{.*}}/search-path-usage/a'
+// CHECK-INCLUDE-NEXT: remark: search path used: '{{.*}}/search-path-usage/a_next'
+// CHECK-INCLUDE-NEXT: remark: search path used: '{{.*}}/search-path-usage/d'
+// CHECK-INCLUDE-NOT:  remark:
+#endif
+
+// Check that framework search paths are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
+// RUN:   -F%S/Inputs/search-path-usage/FwA  \
+// RUN:   -F%S/Inputs/search-path-usage/FwB  \
+// RUN:   -DFRAMEWORK 2>&1 | FileCheck %s --check-prefix=CHECK-FRAMEWORK
+#ifdef FRAMEWORK
+#include "FrameworkA/FrameworkA.h"
+// CHECK-FRAMEWORK-NOT: remark:
+// CHECK-FRAMEWORK: remark: search path used: '{{.*}}/search-path-usage/FwA'
+// CHECK-FRAMEWORK-NOT: remark:
+#endif
+
+// Check that system search paths are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
+// RUN:   -isystem %S/Inputs/search-path-usage/b \
+// RUN:   -isystem %S/Inputs/search-path-usage/d \
+// RUN:   -DSYSTEM 2>&1 | FileCheck %s --check-prefix=CHECK-SYSTEM
+#ifdef SYSTEM
+#include "b.h"
+// CHECK-SYSTEM-NOT: remark:
+// CHECK-SYSTEM: remark: search path used: '{{.*}}/search-path-usage/b'
+// CHECK-SYSTEM-NOT: remark:
+#endif
+
+// Check that sysroot-based search paths are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
+// RUN:   -isysroot %S/Inputs/search-path-usage  \
+// RUN:   -iwithsysroot /b   \
+// RUN:   -iwithsysroot /d   \
+// RUN:   -DSYSROOT 2>&1 | FileCheck %s --check-prefix=CHECK-SYSROOT
+#ifdef SYSROOT
+#include "d.h"
+// CHECK-SYSROOT-NOT: remark:
+// CHECK-SYSROOT: remark: search path used: '/d'
+// CHECK-SYSROOT-NOT: remark:
+#endif
+
+// Check that search paths used by `__has_include()` are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
+// RUN:   -I%S/Inputs/search-path-usage/b\
+// RUN:   -I%S/Inputs/search-path-usage/d\
+// RUN:   -DHAS_INCLUDE 2>&1 | FileCheck %s --check-prefix CHECK-HAS-INCLUDE
+#ifdef HAS_INCLUDE
+#if __has_include("b.h")
+#endif
+#if __has_include("x.h")
+#endif
+// CHECK-HAS-INCLUDE-NOT: remark
+// CHECK-HAS-INCLUDE: remark: search path used: '{{.*}}/search-path-usage/b'
+// CHECK-HAS-INCLUDE-NOT: remark
+#endif
+
+// Check that search paths used by `#import` are reported.
+//
+// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
+// RUN:   -I%S/Inputs/search-path-usage/b\
+// RUN:   -I%S/Inputs/search-path-usage/d\
+// RUN:   -DIMPORT 2>&1 | FileCheck %s --check-prefix CHECK-IMPORT
+#ifdef IMPORT
+#import "d.h"
+// CHECK-IMPORT-NOT: remark
+// CHECK-IMPORT: remark: search path used: '{{.*}}/search-path-usage/d'
+// CHECK-IMPORT-NOT: remark
+#endif
+
+// Check that header maps are reported.
+//
+// RUN: sed "s|DIR|%/S/Inputs/search-path-usage|g" 

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

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

dexonsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:427
+def remark_pp_include_header_search_usage : Remark<
+  "user-provided search path used: '%0'">,
+  InGroup>;

jansvoboda11 wrote:
> dexonsmith wrote:
> > 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.).
> I chose this spelling to distinguish user-provided vs default include paths. 
> The default ones are not important for header search pruning in dependency 
> scanner, as they always get generated in 
> `InitHeaderSearch::AddDefaultIncludePaths`.
Ah, I thought that logic was all moved to the driver. Looking at 
AddDefaultIncludePaths, you're right that only some things have moved, and some 
haven't (yet)...

For example, it looks like the only logic remaining in `-cc1` on Darwin is:
```
  // All header search logic is handled in the Driver for Darwin.
  if (triple.isOSDarwin()) {
if (HSOpts.UseStandardSystemIncludes) {
  // Add the default framework include paths on Darwin.
  AddPath("/System/Library/Frameworks", System, true);
  AddPath("/Library/Frameworks", System, true);
}
return;
  }
```
(We should probably clean that up and move it to the driver as well...)

I think `-cc1` can be agnostic about the source of the search path (user, IDE, 
driver, etc.).



Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165
 
+  /// Mapping from SearchDir to HeaderSearchOptions::Entry indices.
+  llvm::DenseMap SearchDirToHSEntry;
+

jansvoboda11 wrote:
> dexonsmith wrote:
> > 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.
> I'm not sure what's our approach on early micro-optimizations like this. I 
> don't think it will have measurable impact on memory or runtime.
Heh, maybe with the principle that the small things add up (or maybe just out 
of habit), I probably tend too much toward making small things efficient when 
it's easy. I also personally find `Vector` just as natural a map data 
structure as `DenseMap`, but with more obvious allocation / etc. But yeah, 
probably not important.



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});

jansvoboda11 wrote:
> dexonsmith wrote:
> > 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.
> I'm not sure what you mean by system includes. 
> `HeaderSearchOptions::UserEntries` should contain all search paths provided 
> through the command-line including `-isystem` and `-isysroot`: 
> https://github.com/llvm/llvm-project/blob/97d234935f1514af128277943f30efc469525371/clang/lib/Frontend/CompilerInvocation.cpp#L2985-L3056.
> 
> "System header prefixes" only control whether a header found through 
> `DirectoryLookup` should be marked as system.
Thanks for explaining; you're right, I was confused. (Maybe this should be 
renamed from UserEntries to CommandLineEntries if it has everything on the 
command-line?)

As I mentioned above, the distinction between driver-generated and 
`-cc1`-generated options is a bit murky (and moving in the direction of 
removing the latter), so I'm not sure it's meaningful to filter out 
`-cc1`-generated options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D102923#2787702 , @dexonsmith 
wrote:

> 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?

I'll put a FIXME at an appropriate place in the next revision.




Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:427
+def remark_pp_include_header_search_usage : Remark<
+  "user-provided search path used: '%0'">,
+  InGroup>;

dexonsmith wrote:
> 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.).
I chose this spelling to distinguish user-provided vs default include paths. 
The default ones are not important for header search pruning in dependency 
scanner, as they always get generated in 
`InitHeaderSearch::AddDefaultIncludePaths`.



Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165
 
+  /// Mapping from SearchDir to HeaderSearchOptions::Entry indices.
+  llvm::DenseMap SearchDirToHSEntry;
+

dexonsmith wrote:
> 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.
I'm not sure what's our approach on early micro-optimizations like this. I 
don't think it will have measurable impact on memory or runtime.



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});

dexonsmith wrote:
> 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.
I'm not sure what you mean by system includes. 
`HeaderSearchOptions::UserEntries` should contain all search paths provided 
through the command-line including `-isystem` and `-isysroot`: 
https://github.com/llvm/llvm-project/blob/97d234935f1514af128277943f30efc469525371/clang/lib/Frontend/CompilerInvocation.cpp#L2985-L3056.

"System header prefixes" only control whether a header found through 
`DirectoryLookup` should be marked as system.



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'

dexonsmith wrote:
> 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.
Good point, I'll work on improving the coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @jansvoboda11 
wrote:

> In 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>;

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>;
 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 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
  

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 348518.
jansvoboda11 added a comment.

Fix naming of new functions/variables


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/a_next/a.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/b/b.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/d/d.h
  clang/test/Preprocessor/header-search-user-entries.c

Index: clang/test/Preprocessor/header-search-user-entries.c
===
--- /dev/null
+++ clang/test/Preprocessor/header-search-user-entries.c
@@ -0,0 +1,11 @@
+// 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
+
+#include "a.h"
+#include "d.h"
+
+// 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'
Index: clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
@@ -0,0 +1 @@
+#include_next "a.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -108,6 +108,20 @@
<< NumSubFrameworkLookups << " subframework lookups.\n";
 }
 
+std::vector HeaderSearch::computeUserEntryUsage() const {
+  std::vector UserEntryUsage(HSOpts->UserEntries.size());
+  for (unsigned I = 0, E = SearchDirsUsage.size(); I < E; ++I) {
+// Check whether this DirectoryLookup has been successfully used.
+if (SearchDirsUsage[I]) {
+  auto UserEntryIdxIt = SearchDirToHSEntry.find(I);
+  // Check whether this DirectoryLookup maps to a HeaderSearch::UserEntry.
+  if (UserEntryIdxIt != SearchDirToHSEntry.end())
+UserEntryUsage[UserEntryIdxIt->second] = true;
+}
+  }
+  return UserEntryUsage;
+}
+
 /// CreateHeaderMap - This method returns a HeaderMap for the specified
 /// FileEntry, uniquing them through the 'HeaderMaps' datastructure.
 const HeaderMap *HeaderSearch::CreateHeaderMap(const FileEntry *FE) {
@@ -649,6 +663,17 @@
   return None;
 }
 
+void HeaderSearch::cacheLookupSuccess(LookupFileCacheInfo ,
+  unsigned HitIdx) {
+  CacheLookup.HitIdx = HitIdx;
+  SearchDirsUsage[HitIdx] = true;
+
+  auto UserEntryIdxIt = SearchDirToHSEntry.find(HitIdx);
+  if (UserEntryIdxIt != SearchDirToHSEntry.end())
+Diags.Report(diag::remark_pp_include_header_search_usage)
+<< HSOpts->UserEntries[UserEntryIdxIt->second].Path;
+}
+
 void HeaderSearch::setTarget(const TargetInfo ) {
   ModMap.setTarget(Target);
 }
@@ -987,7 +1012,7 @@
   >getFileEntry(), isAngled, FoundByHeaderMap);
 
 // Remember this location for the next lookup we do.
-CacheLookup.HitIdx = i;
+cacheLookupSuccess(CacheLookup, i);
 return File;
   }
 
@@ -1017,8 +1042,8 @@
 return MSFE;
   }
 
-  LookupFileCacheInfo  = LookupFileCache[Filename];
-  CacheLookup.HitIdx = LookupFileCache[ScratchFilename].HitIdx;
+  cacheLookupSuccess(LookupFileCache[Filename],
+ LookupFileCache[ScratchFilename].HitIdx);
   // FIXME: SuggestedModule.
   return File;
 }
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -36,9 +36,11 @@
 struct DirectoryLookupInfo {
   IncludeDirGroup Group;
   DirectoryLookup Lookup;
+  Optional UserEntryIdx;
 
-  DirectoryLookupInfo(IncludeDirGroup Group, DirectoryLookup Lookup)
-  : Group(Group), Lookup(Lookup) {}
+  DirectoryLookupInfo(IncludeDirGroup Group, DirectoryLookup Lookup,
+  Optional UserEntryIdx)
+  : Group(Group), Lookup(Lookup), UserEntryIdx(UserEntryIdx) {}
 };
 
 /// InitHeaderSearch - This class makes it easier to set the search paths of
@@ -60,13 +62,15 @@
   /// AddPath - Add the specified path to the specified group list, prefixing
   /// the sysroot if used.
   /// Returns true if the path exists, false if it was ignored.
-  

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

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a subscriber: akyrtzi.
Bigcheese added a comment.

I think this is looking good, but would like either @dexonsmith, @akyrtzi, or 
someone else familiar with this area to take a look. Only other comment I have 
is that the new functions you add should use the current LLVM naming convention.

I also wonder how we should handle other things that are found via include 
paths such as module maps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For dependency scanning, it would be useful to collect header search paths 
(provided on command-line via `-I` and friends) that were actually used during 
preprocessing. This patch adds that feature to `HeaderSearch` along with a new 
remark that reports such paths as they get used.

Previous version of this patch tried to use the existing `LookupFileCache` to 
report used paths via `HitIdx`. That doesn't work for `ComputeUserEntryUsage` 
(which is intended to be called *after* preprocessing), because it indexes used 
search paths by the file name. This means the values get overwritten when the 
code contains `#include_next`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102923

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/a_next/a.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/b/b.h
  clang/test/Preprocessor/Inputs/header-search-user-entries/d/d.h
  clang/test/Preprocessor/header-search-user-entries.c

Index: clang/test/Preprocessor/header-search-user-entries.c
===
--- /dev/null
+++ clang/test/Preprocessor/header-search-user-entries.c
@@ -0,0 +1,11 @@
+// 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
+
+#include "a.h"
+#include "d.h"
+
+// 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'
Index: clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/header-search-user-entries/a/a.h
@@ -0,0 +1 @@
+#include_next "a.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -108,6 +108,18 @@
<< NumSubFrameworkLookups << " subframework lookups.\n";
 }
 
+std::vector HeaderSearch::ComputeUserEntryUsage() const {
+  std::vector UserEntryUsage(HSOpts->UserEntries.size());
+  for (unsigned i = 0, e = SearchDirsUsage.size(); i < e; ++i) {
+if (SearchDirsUsage[i]) {
+  auto UserEntryIdxIt = SearchDirToHSEntry.find(i);
+  if (UserEntryIdxIt != SearchDirToHSEntry.end())
+UserEntryUsage[UserEntryIdxIt->second] = true;
+}
+  }
+  return UserEntryUsage;
+}
+
 /// CreateHeaderMap - This method returns a HeaderMap for the specified
 /// FileEntry, uniquing them through the 'HeaderMaps' datastructure.
 const HeaderMap *HeaderSearch::CreateHeaderMap(const FileEntry *FE) {
@@ -649,6 +661,17 @@
   return None;
 }
 
+void HeaderSearch::CacheLookupSuccess(LookupFileCacheInfo ,
+  unsigned HitIdx) {
+  CacheLookup.HitIdx = HitIdx;
+  SearchDirsUsage[HitIdx] = true;
+
+  auto UserEntryIdxIt = SearchDirToHSEntry.find(HitIdx);
+  if (UserEntryIdxIt != SearchDirToHSEntry.end())
+Diags.Report(diag::remark_pp_include_header_search_usage)
+<< HSOpts->UserEntries[UserEntryIdxIt->second].Path;
+}
+
 void HeaderSearch::setTarget(const TargetInfo ) {
   ModMap.setTarget(Target);
 }
@@ -987,7 +1010,7 @@
   >getFileEntry(), isAngled, FoundByHeaderMap);
 
 // Remember this location for the next lookup we do.
-CacheLookup.HitIdx = i;
+CacheLookupSuccess(CacheLookup, i);
 return File;
   }
 
@@ -1017,8 +1040,8 @@
 return MSFE;
   }
 
-  LookupFileCacheInfo  = LookupFileCache[Filename];
-  CacheLookup.HitIdx = LookupFileCache[ScratchFilename].HitIdx;
+  CacheLookupSuccess(LookupFileCache[Filename],
+ LookupFileCache[ScratchFilename].HitIdx);
   // FIXME: SuggestedModule.
   return File;
 }
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -36,9 +36,11 @@
 struct IncludePathInfo {
   IncludeDirGroup Group;
   DirectoryLookup Path;
+  Optional UserEntryIdx;
 
-  IncludePathInfo(IncludeDirGroup Group, DirectoryLookup Path)
-  :