Implicit inclusion of checked header files. #998

sulekhark · 2021-03-02T18:50:16Z

This PR has previous discussion in PR #980.

mattmccutchen-cci · 2021-03-02T18:51:37Z

I'm referencing checkedc/checkedc#431, which this addresses.

…ay have clang-specific declarations.

…rations.

…of checked headers.

mgrang · 2021-03-11T18:28:30Z

clang/lib/Headers/inttypes.h

@@ -18,6 +18,11 @@
 #error MSVC does not have inttypes.h prior to Visual Studio 2013
 #endif

+#ifndef __cplusplus


Do we want this enabled even if the user compiles a C file with -fno-checkedc-extension?

Maybe we can create a macro like __checkedc which gets defined if Checked C is enabled. Then we can use that macro as guard here.

Yes, we need to resolve this issue. We also need to consider the use case when inttypes_checked.h is explicitly included.

+1 to creating a macro like __checkedc. That would make it possible for the public headers of third-party libraries to automatically generate Checked C declarations when loaded by the Checked C compiler while remaining compatible with plain C compilers as well: in effect, supporting implicit inclusion of checked headers for third-party libraries, just as you are doing for the system headers in this PR. (I was thinking about filing a separate issue for this.)

Thanks! I have created the __checkedc macro in the latest commit.

mgrang · 2021-03-11T18:38:26Z

clang/test/Driver/nostdlibinc.c

@@ -1,10 +1,8 @@
-// RUN: %clang -target x86_64-unknown-unknown \
-// RUN:   -nostdlibinc -ffreestanding -fsyntax-only %s
+// RUN: %clang -target x86_64-unknown-unknown -nostdlibinc -ffreestanding -MM -MG %s | FileCheck -check-prefix=CHECK %s


Could you please explain why we need to change community tests?

Does this mean we are changing existing behavior, and that users would need to update their flags/makefiles? Changing community tests could also cause merge conflicts when we upgrade.

This test case is ensuring that the system stdlib.h cannot be found if -nostdlibinc is specified on the compilation commandline.
This can be ensured in more than one way - the approach chosen in this test case assumes that stdlib.h is not present in <build-dir>/lib/clang/11.0.0/include directory. This is an assumption because of the following reason:
The mechanism used by clang/llvm to add clang-specific declarations is to create a header file of the same name (ex.: float.h, inttypes.h - these are present in <src-dir>/clang/lib/Headers), use #include_next <float.h> to include the system header file, and add clang-specific declarations below this. Then they copy the header file from <src-dir>/clang/lib/Headers to <build-dir>/lib/clang/11.0.0/include. The only reason that stdlib.h is not present in <build-dir/lib/clang/11.0.0/include in the llvm upstream repo is because there has not been a need to add clang-specific declarations to stdlib.h. Had there been, this test case would have to be modified anyway.

Now, that we have added stdlib.h to <build-dir>/lib/clang/11.0.0/include, we need to change the way of ensuring that the "system" stdlib.h is not found when -nostdlibinc is specified on the compilation command line.

Once we create a new macro for Checked C and use it as guard maybe we can simply add -fno-checkedc-extension to these tests to preserve community behavior without necessarily modifying the tests themselves.

We should create a new macro for Checked C and use it as a guard. But adding -fno-checkedc-extension to this test case will not stop it from failing. In it's original form, this test case will fail because of the fact that a file named stdlib.h exists in the <build-dir>/lib/clang/11.0.0/include directory, irrespective of what the file contains.

The above modification will be required to make this test case pass even if we create the new macro.

mattmccutchen-cci · 2021-03-11T20:08:15Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+  - For a system header file that contains clang-specific declarations and also
+    Checked-C-specific declarations (there is one such header file at present),
+    we will do the following:


If you're willing to change the default include path in the driver to add a Checked-C-specific include directory (say lib/clang/VERSION/include-checkedc) before the main one (lib/clang/VERSION/include), then you won't need to do this. You can use the solution in the previous bullet point for all Checked C headers, and the #include_next <foo.h> will automatically pick up either the system header or the Clang header (in which the #include_next <foo.h> would then include the system header) as appropriate. This would also cleanly separate the Checked C headers from the Clang headers in different subdirectories of the installation directory, making it clearer for users during troubleshooting.

We have decided not to change the default include path in the driver at present.

mattmccutchen-cci · 2021-03-11T20:13:07Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+            // Force the inclusion of Checked-C-specific declarations
+            #undef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+            #include <bar.h>   


If you do keep this approach rather than taking my suggestion above, might the following be safer?

Suggested change

// Force the inclusion of Checked-C-specific declarations

#undef NO_IMPLICIT_INCLUDE_CHECKED_HDRS

#include <bar.h>

#include <bar.h>

#include <bar_checked_internal.h>

bar_checked_internal.h might get scanned twice, so it would need a header guard and there might be a slight performance cost, but it would make it possible for a user to set NO_IMPLICIT_INCLUDE_CHECKED_HDRS for one #include <foo.h> directive in a translation unit without the risk of an #include <bar_checked.h> earlier in the translation unit clobbering NO_IMPLICIT_INCLUDE_CHECKED_HDRS (if that is a scenario you care about).

@mattmccutchen-cci Thank you for your suggestion - you are right! I have incorporated it in the latest commit.

mattmccutchen-cci · 2021-03-11T20:16:51Z

clang/lib/Headers/inttypes.h

@@ -18,6 +18,11 @@
 #error MSVC does not have inttypes.h prior to Visual Studio 2013
 #endif

+#ifndef __cplusplus


+1 to creating a macro like __checkedc. That would make it possible for the public headers of third-party libraries to automatically generate Checked C declarations when loaded by the Checked C compiler while remaining compatible with plain C compilers as well: in effect, supporting implicit inclusion of checked headers for third-party libraries, just as you are doing for the system headers in this PR. (I was thinking about filing a separate issue for this.)

dtarditi

Thanks for putting this together - I have some suggestions for the RFC.

dtarditi · 2021-03-11T22:05:53Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+## Problem Summary
+The existing approach for including checked header files, which is to explicitly
+replace an included header file with its checked counterpart, is inconvenient
+for automation by 3C. So we need a way to implicitly include checked header


I would talk about this in more general terms. 3C is representative of an approach to converting files. It is inconvenient for adoption of Checked C to have to alter include files throughout a project.

Thank you for your review! I have addressed all the review comments and made the requisite changes to the doc.

dtarditi · 2021-03-11T22:06:08Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+files.
+
+## Conditions that a Solution must Satisfy
+ - The solution must not violate the "opt-in" philosophy that we advocate for


The proposed solution isn't opt-in, though.

dtarditi · 2021-03-11T22:06:31Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+ clang picks it up from the above location.
+
+## Solution Space
+ - Solution 1: Proposed by CCI


Probably better to give this a more general name, not "proposed by CCI".

dtarditi · 2021-03-11T22:08:10Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+## Proposed Solution:
+
+The proposed solution is as follows:
+  - It is a hybrid solution based on ideas in Solutions 3 and 1 that


Shouldn't t his be classified as another possible solution? It is a little confusing for the proposed solution to be described as a hybrid of other solutions.

dtarditi · 2021-03-11T22:08:55Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+
+            // Force the inclusion of Checked-C-specific declarations
+            #undef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+            #include <bar.h>   


This approach of a slight alteration to the clang-specific header file looks good to me.

mgrang · 2021-03-18T19:41:03Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

 - The order in which the compiler driver searches the include paths must remain
 unaltered.
 - The solution must be able to accommodate any clang-specific declarations in
 system header files, if present (Ex.: inttypes.h).
- - The Checked-C-specific declarations must live in checkedc repository.
+ - The Checked-C-specific declarations must live in the checkedc repository.


Checked-C-specific --> Checked C-specific

Thank you for your review! I feel that Checked-C-specific is a bit more clear - so I have retained the same.

mgrang · 2021-03-18T19:53:34Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

@@ -53,14 +51,14 @@ files.
 clang picks it up from the above location.

 ## Solution Space
- - Solution 1: Proposed by CCI
+ - Solution 1:
     - Let `foo.h` be a system header file with a checked counterpart called
     `foo_checked.h`. We add a new file, also called `foo.h`, in the same
     directory that contains `foo_checked.h`. This new `foo.h` should contain
     `#include_next <foo.h>` plus all the Checked-C-specific declarations


Checked-C-specific --> Checked C-specific

mgrang · 2021-03-18T19:53:52Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+       4) It is convenient for automation.
+     - **Cons**: 1) The solution does not provide a way to opt out of including
+       checked headers. 2) While it is easy to accommodate clang-specific
+       declarations in system header files (Checked-C-specific declarations can


Checked-C-specific --> Checked C-specific

mgrang · 2021-03-18T19:54:05Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+       drivers are needed to compile the sources. This may happen in a scenario
+       where most source files want implicit inclusion but a few source files
+       want explicit inclusion because they want to directly include some system
+       header files in order to avoid Checked-C-specific declarations.


Checked-C-specific --> Checked C-specific

mgrang · 2021-03-18T19:54:11Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+    compilation flag NO_IMPLICIT_INCLUDE_CHECKED_HDRS to opt out of the implicit
+    inclusion.
+  - For header files that do not have clang-specific declarations, but have
+    Checked-C-specific declarations (there are 13 such header files at present),


Checked-C-specific --> Checked C-specific

mgrang · 2021-03-18T19:55:37Z

clang/lib/Headers/inttypes.h

 #pragma CHECKED_SCOPE pop
 #endif

-#ifndef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+#if defined __checkedc && !defined NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+// Compiling for Checked C and implicit inclusion of checked headers is enabled


Period needed at the end of comment.

mgrang · 2021-03-18T19:58:47Z

clang/test/CheckedC/headers/nostdlibinc.c

+//      includes (via #include_next), is not found.
+// The -MM -MG preprocessor options list the header files not found.
+//
+// RUN: %clang -target x86_64-unknown-unknown -nostdlibinc -ffreestanding -MM -MG %s | FileCheck -check-prefix=CHECK %s


By default, FileCheck will always check the prefix CHECK. So you can omit the --check-prefix=CHECK part in this RUN command.
See https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-filecheck-check-prefix

…cket.h may not be found in all compilation environments (Ex. Windows). Therefore the includes in the wrapper header files need to be guarded by __has_include_next. This may impact the expected output of a testcase.

mattmccutchen-cci · 2021-03-23T16:27:11Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+            #if !defined __checkedc || defined NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+            #include_next <foo.h>
+            #else
+            #include <foo_checked.h>
+            #endif


I realize this can be substantially simplified if you don't mind some header files being read multiple times. They probably are or have to be guarded anyway, so the only cost I see is performance, unless there is a concern about a header file indirectly including itself recursively even if the recursion is immediately stopped by a guard.

Suggested change

#if !defined __checkedc || defined NO_IMPLICIT_INCLUDE_CHECKED_HDRS

#include_next <foo.h>

#else

#include <foo_checked.h>

#endif

#include_next <foo.h>

#if defined __checkedc && !defined NO_IMPLICIT_INCLUDE_CHECKED_HDRS

#include <foo_checked.h>

#endif

I understand that this suggestion has the advantage of a uniform approach for all header files but it relies on guard flags to avoid recursion. In order to keep the solution simple and maintainable, we do prefer to avoid a header file including itself recursively without relying on guard flags.
Additionally, in the implementation, there are more flags entering into the fray in order to handle the absence of some posix header files like unistd.h, arpa/inet.h and sys/socket.h in the Windows environment. This makes it even more necessary to keep the solution simple and also comprehensible.

When I think about this now, I think I got a little carried away with my proposal and you were right to decline it. The resulting code looks simpler on the surface, but it's significantly more complicated to verify that it works correctly.

FWIW, an alternative way to achieve a uniform approach is to have each header foo.h with no Clang-specific declarations use your existing design for headers with Clang-specific declarations, effectively as if Clang already had a foo.h containing nothing but #include_next <foo.h> followed by an empty set of Clang-specific declarations. You could decide whether the resulting short, boilerplate foo.h goes in the checkedc-clang or checkedc repository; the former would achieve total uniformity. But this would increase the number of header files you have to maintain by about 50% (in addition to foo.h and foo_checked.h, you would have foo_checked_internal.h) for minimal practical benefit, so I imagine it's not worth it. (The only practical benefit I see is that if the user had some problem with the system foo.h but had another way to get the plain-C declarations they want, they could then get the Checked C declarations by including foo_checked_internal.h with no risk of pulling in the system foo.h. But I think this would be very unusual, and the user could use the hacky workaround of manually defining the header guard for foo.h before including foo_checked.h.)

Yes, we did earlier consider having the uniform approach of creating *_checked_internal.h for all header files that have Checked-C-specific declarations. At present, there are 14 such header files of which inttypes.h is the only file with clang-specific declarations. We decided not to create *_checked_internal.h for all because 13 of these files will be redundant at present.
If a user wants custom plain-C declarations and also Checked C declarations for, say, foo.h, under the present system he/she can 1) include either foo.h (for implicit inclusion) or foo_checked.h (for explicit inclusion) and 2) add -nostdlibinc and the include directory containing custom plain-C declarations on the compilation command line.

mattmccutchen-cci · 2021-03-23T16:30:51Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+        At the end of this pre-existing `bar.h`, we will add the following:
+
+            #if defined __checkedc && !defined NO_IMPLICIT_INCLUDE_CHECKED_HDRS
+            #include <bar_checked_internal.h>


Based on the same idea as my previous comment, this could be changed to just:

Suggested change

#include <bar_checked_internal.h>

#include <bar_checked.h>

Then the format of bar.h would be exactly the same as in the case of no Clang-specific declarations, except for the presence of the declarations between the #include_next <bar.h> and the Checked-C-specific block at the bottom.

mattmccutchen-cci · 2021-03-23T16:32:23Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+      - All the Checked-C-specific declarations (that are currently present in
+        `bar_checked.h`) will be moved to `bar_checked_internal.h`. The file
+        `bar_checked.h` will be modified to contain just the following:
+
+            #include <bar.h>
+            #include <bar_checked_internal.h>


And then:

Suggested change

- All the Checked-C-specific declarations (that are currently present in

`bar_checked.h`) will be moved to `bar_checked_internal.h`. The file

`bar_checked.h` will be modified to contain just the following:

#include <bar.h>

#include <bar_checked_internal.h>

- The file `bar_checked.h` will contain `#include <bar.h>` and the

Checked-C-specific declarations.

mattmccutchen-cci · 2021-03-23T16:39:10Z

clang/docs/checkedc/rfc/Include-Checked-Hdrs.md

+            #include <foo_checked.h>
+            #endif
+
+      - The file `foo_checked.h` will contain `#include_next <foo.h>` and the


You could change the #include_next <foo.h> to #include <foo.h> (making it consistent with the case where Clang-specific declarations are present) if you add a guard to foo.h to avoid direct recursion between the two files. (Otherwise, foo.h does not need a guard because all it does is include two other files that presumably have their own guards. But if it's considered best practice for foo.h to have a guard anyway, then you could adopt this suggestion.)

are crashing, in order to test all the other test cases on the ADO build machines.

sulekhark · 2021-03-23T17:06:13Z

@mattmccutchen-cci Thanks for you review! I will think about your suggestions. Meanwhile, there are five 3C test cases that are crashing on Windows - they all include stdio.h and stdlib.h - so the root cause is most likely the same (3d-allocation.c, basic.c, graphs.c, graphs2.c, linkedlist.c). It will be great if you could take a look at them.

mattmccutchen-cci · 2021-03-23T17:19:42Z

Meanwhile, there are five 3C test cases that are crashing on Windows - they all include stdio.h and stdlib.h - so the root cause is most likely the same (3d-allocation.c, basic.c, graphs.c, graphs2.c, linkedlist.c). It will be great if you could take a look at them.

I'll look into this. In the meantime, maybe you could work on #1009 since that's probably a blocker for turning on implicit checked header inclusion by default (it would break existing C code).

mattmccutchen-cci · 2021-03-23T20:49:18Z

Re the 3C tests crashing on Windows: just looking at the state at the time of the crash, it seems we are using a Clang API incorrectly and this change fixes the tests for me. We need to investigate the problem further (correctcomputation#509), but you can go ahead and use this fix as a basis for further development of this PR and (if need be) merge it even if we haven't completed our investigation yet.

…hdrfiles.fix-windows-crash isFunctionProtoType should be used with getAs, not dyn_cast.

dtarditi

Looks good, thank you! Please commit the changes in the other repos before this change, so automated testing does not fail.

mattmccutchen-cci · 2021-04-01T11:37:20Z

@sulekhark I guess it's time for me to test this with the 3C benchmark test suite then? (I had been waiting for your confirmation.) I can do that later this morning.

sulekhark · 2021-04-01T14:00:15Z

@mattmccutchen-cci Yes, please go ahead and test.

default now that we've merged Microsoft's change for implicit checked header inclusion (checkedc#998). I filed #520 for follow-ups.

default now that we've merged Microsoft's change for implicit checked header inclusion (checkedc#998). I filed #520 for follow-ups. We don't have to change the benchmark workflow right now: it still passes --includeDir, but that option is ignored when #include updating is off.

Final version of the rfc for implicit inclusion of checked header files.

a9e1476

sulekhark requested a review from dtarditi March 2, 2021 18:50

sulekhark added 3 commits March 4, 2021 10:37

Modified the solution to handle the case where a system header file m…

71daaa2

…ay have clang-specific declarations.

Update to the approach to handle the presence of clang-specific decla…

0eabc20

…rations.

Changes in the checkedc-clang repo for supporting implicit inclusion …

82df474

…of checked headers.

mgrang reviewed Mar 11, 2021

View reviewed changes

sulekhark changed the title ~~Final version of the rfc for implicit inclusion of checked header files.~~ Rfc for implicit inclusion of checked header files. Mar 11, 2021

mattmccutchen-cci reviewed Mar 11, 2021

View reviewed changes

dtarditi reviewed Mar 11, 2021

View reviewed changes

Added __checkedc flag, added test cases and addressed review comments.

b5d7c36

mattmccutchen-cci mentioned this pull request Mar 18, 2021

Implicit include of checked header files checkedc/checkedc#440

Merged

mgrang reviewed Mar 18, 2021

View reviewed changes

sulekhark added 2 commits March 19, 2021 18:01

Incorporated review comments. Also added another test case.

8310aae

mattmccutchen-cci reviewed Mar 23, 2021

View reviewed changes

Temporarily marking five of 3C tests as unsupported on Windows as they

e0b5ed0

are crashing, in order to test all the other test cases on the ADO build machines.

isFunctionProtoType should be used with getAs, not dyn_cast.

18b735b

mattmccutchen-cci mentioned this pull request Mar 23, 2021

Investigate and add regression test for dyn_cast<FunctionProtoType>(OrigType) crash on Windows correctcomputation/checkedc-clang#509

Open

sulekhark added 3 commits March 23, 2021 18:24

Merge pull request #1013 from correctcomputation/implicit-include-of-…

fecf7d2

…hdrfiles.fix-windows-crash isFunctionProtoType should be used with getAs, not dyn_cast.

Enabling the 3C test cases.

2e7f5e5

Merge branch 'master' into implicit-include-of-hdrfiles

0091379

sulekhark requested a review from kkjeer March 30, 2021 20:21

dtarditi approved these changes Apr 1, 2021

View reviewed changes

sulekhark merged commit 2023d30 into master Apr 1, 2021

sulekhark deleted the implicit-include-of-hdrfiles branch April 1, 2021 14:11

mattmccutchen-cci mentioned this pull request Apr 1, 2021

Follow-ups after implicit checked header inclusion correctcomputation/checkedc-clang#520

Closed

5 tasks

This was referenced Apr 2, 2021

Use #include_next to avoid the need for user code to modify includes to point to Checked C headers checkedc/checkedc#431

Closed

Problems caused by __SOCKADDR_ARG transparent union in sys/socket.h when _GNU_SOURCE is on checkedc/checkedc#441

Open

sulekhark changed the title ~~Rfc for implicit inclusion of checked header files.~~ Implicit inclusion of checked header files. May 7, 2021

Implicit inclusion of checked header files. #998

Implicit inclusion of checked header files. #998

Uh oh!

Conversation

sulekhark commented Mar 2, 2021

Uh oh!

mattmccutchen-cci commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtarditi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mattmccutchen-cci commented Mar 23, 2021 •

edited

Loading