[clang] Implement -Wmissing-include-dirs #94827

bradenhelmer · 2024-06-08T01:45:32Z

Implements -Wmissing-include-dirs #92015

This is my first contribution and would love some feedback. Thanks!

github-actions · 2024-06-08T01:45:51Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-08T01:46:20Z

@llvm/pr-subscribers-clang

Author: Braden Helmer (bradenhelmer)

Changes

Implements -Wmissing-include-dirs #92015

This is my first contribution and would love some feedback. Thanks

Full diff: https://github.com/llvm/llvm-project/pull/94827.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
(modified) clang/include/clang/Basic/DiagnosticGroups.td (+1-1)
(modified) clang/lib/Driver/Driver.cpp (+6)

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 773b234cd68fe..b7d50e22cc0d0 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -809,4 +809,7 @@ def warn_android_unversioned_fallback : Warning<
 
 def err_drv_triple_version_invalid : Error<
   "version '%0' in target triple '%1' is invalid">;
+
+def warn_missing_include_dirs : Warning <
+	"the included directory %0 is missing">, InGroup<MissingIncludeDirs>;
 }
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 7d5ba7869ec34..9b37d4bd3205b 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -506,7 +506,7 @@ def MaxUnsignedZero : DiagGroup<"max-unsigned-zero">;
 def MissingBraces : DiagGroup<"missing-braces">;
 def MissingDeclarations: DiagGroup<"missing-declarations">;
 def : DiagGroup<"missing-format-attribute">;
-def : DiagGroup<"missing-include-dirs">;
+def MissingIncludeDirs : DiagGroup<"missing-include-dirs">;
 def MissingNoreturn : DiagGroup<"missing-noreturn">;
 def MultiChar : DiagGroup<"multichar">;
 def : DiagGroup<"nested-externs">;
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index f5ea73a04ae5c..5bc737a43338e 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1271,6 +1271,12 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
     if (VFS->setCurrentWorkingDirectory(WD->getValue()))
       Diag(diag::err_drv_unable_to_set_working_directory) << WD->getValue();
 
+  // Check for missing include directories
+  for (auto IncludeDir : Args.getAllArgValues(options::OPT_I_Group)) {
+    if (!llvm::sys::fs::is_directory(IncludeDir))
+      Diag(diag::warn_missing_include_dirs) << IncludeDir;
+  }
+
   // FIXME: This stuff needs to go into the Compilation, not the driver.
   bool CCCPrintPhases;

llvmbot · 2024-06-08T01:46:20Z

@llvm/pr-subscribers-clang-driver

Author: Braden Helmer (bradenhelmer)

Changes

Implements -Wmissing-include-dirs #92015

This is my first contribution and would love some feedback. Thanks

Full diff: https://github.com/llvm/llvm-project/pull/94827.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
(modified) clang/include/clang/Basic/DiagnosticGroups.td (+1-1)
(modified) clang/lib/Driver/Driver.cpp (+6)

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 773b234cd68fe..b7d50e22cc0d0 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -809,4 +809,7 @@ def warn_android_unversioned_fallback : Warning<
 
 def err_drv_triple_version_invalid : Error<
   "version '%0' in target triple '%1' is invalid">;
+
+def warn_missing_include_dirs : Warning <
+	"the included directory %0 is missing">, InGroup<MissingIncludeDirs>;
 }
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 7d5ba7869ec34..9b37d4bd3205b 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -506,7 +506,7 @@ def MaxUnsignedZero : DiagGroup<"max-unsigned-zero">;
 def MissingBraces : DiagGroup<"missing-braces">;
 def MissingDeclarations: DiagGroup<"missing-declarations">;
 def : DiagGroup<"missing-format-attribute">;
-def : DiagGroup<"missing-include-dirs">;
+def MissingIncludeDirs : DiagGroup<"missing-include-dirs">;
 def MissingNoreturn : DiagGroup<"missing-noreturn">;
 def MultiChar : DiagGroup<"multichar">;
 def : DiagGroup<"nested-externs">;
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index f5ea73a04ae5c..5bc737a43338e 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1271,6 +1271,12 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
     if (VFS->setCurrentWorkingDirectory(WD->getValue()))
       Diag(diag::err_drv_unable_to_set_working_directory) << WD->getValue();
 
+  // Check for missing include directories
+  for (auto IncludeDir : Args.getAllArgValues(options::OPT_I_Group)) {
+    if (!llvm::sys::fs::is_directory(IncludeDir))
+      Diag(diag::warn_missing_include_dirs) << IncludeDir;
+  }
+
   // FIXME: This stuff needs to go into the Compilation, not the driver.
   bool CCCPrintPhases;

In a real-world case with functions that have many, many R_RISCV_CALL_PLT relocations due to asan and ubsan instrumentation, all these can be relaxed by an instruction and the net result is more than 65536 bytes of reduction in the output .text section that totals about 1.2MiB in final size. This changes InputSection to use a 32-bit field for bytesDropped. The RISCV relaxation keeps track in a 64-bit field and detects 32-bit overflow as it previously detected 16-bit overflow. It doesn't seem likely that 32-bit overflow will arise, but it's not inconceivable and it's cheap enough to detect it. This unfortunately increases the size of InputSection on 64-bit hosts by a word, but that seems hard to avoid. Reviewed By: MaskRay Differential Revision: https://reviews.llvm.org/D150722

zmodem

Thanks! The main missing thing is a test for this. Have a look in clang/test/Driver/ to see what the tests can look like.

Some existing tests are failing, see https://buildkite.com/llvm-project/github-pull-requests/builds/70953#018ff782-9ba1-426b-bb3e-7c0e93533ba4

For example it seems clang/test/Frontend/warning-poison-system-directories.c is failing because it builds with -Werror and passes -I flags with directories that don't exist on Windows.

clang/test/Driver/cl-pch.c also failed, but it's not clear to me why.

Maybe if the flag is not enabled by default, these test failures will go away.

zmodem · 2024-06-10T09:17:11Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -809,4 +809,7 @@ def warn_android_unversioned_fallback : Warning<

 def err_drv_triple_version_invalid : Error<
  "version '%0' in target triple '%1' is invalid">;
+
+def warn_missing_include_dirs : Warning <


nit: no space between Warning and <, and two spaces for indentation on the next line

This makes the warning enabled by default. I'm not sure whether that's desirable. Maybe it should be under -Wall or -Wextra (what does gcc do?)

To achieve that, add the DefaultIgnore mark to this definition, and add InGroup<All> or Extra to the DiagGroup definition.

Tested with GCC, its not enabled in -Wall or -Wextra, leaving as DefaultIgnore for now.

clang/lib/Driver/Driver.cpp

zmodem

This basically looks good to me. Just a few nits.

zmodem · 2024-06-11T13:38:23Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -809,4 +809,7 @@ def warn_android_unversioned_fallback : Warning<

 def err_drv_triple_version_invalid : Error<
  "version '%0' in target triple '%1' is invalid">;
+
+def warn_missing_include_dirs : Warning<
+  "the included directory %0 is missing">, InGroup<MissingIncludeDirs>, DefaultIgnore;


nit: let's make this a little shorter and more precise by copying the sysroot warning from the test, how about: "no such include directory: ..."

zmodem · 2024-06-11T13:40:16Z

clang/test/Driver/warning-options.cpp

+
+// Check for proper warning with -Wmissing-include-dirs
+// RUN: %clang -### -Wmissing-include-dirs -I %t/warning-options %s 2>&1 | FileCheck --check-prefix=CHECK-MISSING-INCLUDE-DIRS %s
+// CHECK-MISSING-INCLUDE-DIRS: warning: the included directory {{.*}}/warning-options is missing [-Wmissing-include-dirs]


nit: we usually don't include the "[-Warning-flag-name]" in the check (you can just drop that part, the check doesn't have to cover the whole line)

zmodem

lgtm

zmodem · 2024-06-12T08:41:37Z

Do you have access to press the "Squash and merge" button, or should I do it?

bradenhelmer · 2024-06-12T09:18:14Z

Do you have access to press the "Squash and merge" button, or should I do it?

You'll have to, i dont have access yet, thx

github-actions · 2024-06-12T09:30:30Z

@bradenhelmer Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Implement -Wmissing-include-dirs

317e4c2

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 8, 2024

bradenhelmer mentioned this pull request Jun 8, 2024

[clang] -Wmissing-include-dirs not implemented #92015

Closed

llvm deleted a comment Jun 8, 2024

Merge branch 'main' into Wmissing-include-dirs-impl

090dff8

zmodem reviewed Jun 10, 2024

View reviewed changes

Add test and address PR comments

6bd7085

bradenhelmer requested a review from zmodem June 11, 2024 12:31

zmodem reviewed Jun 11, 2024

View reviewed changes

bradenhelmer added 2 commits June 11, 2024 09:56

Update DiagnosticDriverKinds.td

9476cd3

Update warning-options.cpp

7b3f38b

zmodem approved these changes Jun 11, 2024

View reviewed changes

zmodem merged commit 83a54e7 into llvm:main Jun 12, 2024
7 checks passed

bradenhelmer deleted the Wmissing-include-dirs-impl branch June 12, 2024 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Implement -Wmissing-include-dirs #94827

[clang] Implement -Wmissing-include-dirs #94827

Uh oh!

bradenhelmer commented Jun 8, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jun 8, 2024

Uh oh!

llvmbot commented Jun 8, 2024

Uh oh!

llvmbot commented Jun 8, 2024

Uh oh!

zmodem left a comment

Uh oh!

zmodem Jun 10, 2024

Uh oh!

bradenhelmer Jun 10, 2024

Uh oh!

Uh oh!

Uh oh!

zmodem left a comment

Uh oh!

zmodem Jun 11, 2024

Uh oh!

zmodem Jun 11, 2024

Uh oh!

zmodem left a comment

Uh oh!

zmodem commented Jun 12, 2024

Uh oh!

bradenhelmer commented Jun 12, 2024

Uh oh!

Uh oh!

github-actions bot commented Jun 12, 2024

Uh oh!

Uh oh!

[clang] Implement -Wmissing-include-dirs #94827

[clang] Implement -Wmissing-include-dirs #94827

Uh oh!

Conversation

bradenhelmer commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 8, 2024

Uh oh!

llvmbot commented Jun 8, 2024

Uh oh!

llvmbot commented Jun 8, 2024

Uh oh!

zmodem left a comment

Choose a reason for hiding this comment

Uh oh!

zmodem Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

bradenhelmer Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zmodem left a comment

Choose a reason for hiding this comment

Uh oh!

zmodem Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

zmodem Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

zmodem left a comment

Choose a reason for hiding this comment

Uh oh!

zmodem commented Jun 12, 2024

Uh oh!

bradenhelmer commented Jun 12, 2024

Uh oh!

Uh oh!

github-actions bot commented Jun 12, 2024

Uh oh!

Uh oh!

bradenhelmer commented Jun 8, 2024 •

edited

Loading