Skip to content

Implicit inclusion of checked header files. #998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 1, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update to the approach to handle the presence of clang-specific decla…
…rations.
  • Loading branch information
sulekhark committed Mar 11, 2021
commit 0eabc20be56dc4bcf8f1c47d1fbf16df703bab03
84 changes: 46 additions & 38 deletions clang/docs/checkedc/rfc/Include-Checked-Hdrs.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,41 +139,49 @@ files.
## Proposed Solution:

The proposed solution is as follows:
- Based on feedback from CCI, we implicitly include checked headers by
default. That is, the compilation flag gives a way to opt out of this
implicit inclusion. In accordance with this, we rename the compilation
flag to NO_IMPLICIT_INCLUDE_CHECKED_HDRS.
- For each system header file, say `foo.h`, that does not have clang-specific
declarations (there are 13 such header files at present), we add a new file
also called `foo.h` that will contain the following:

#ifdef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
#include_next <foo.h>
#else
#include <foo_checked.h>
#endif

The file `foo_checked.h` will contain `#include_next <foo.h>` and the
Checked-C-specific declarations.

- In the case that a system header file, say `bar.h`, does have clang-specific
declarations (there is one such header file at present), the pre-existing
`bar.h` contains the following:

#include_next <bar.h>
<currently existing clang-specific declarations>

At the end of the pre-existing `bar.h`, we will add the following:

#ifndef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
#include <bar_checked_internal.h>
#endif

The file `bar_checked.h` will contain just the following:

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

All the Checked-C-specific declarations will be moved to
`bar_checked_internal.h`.
- It is a hybrid solution based on ideas in Solutions 3 and 1 that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

handles the presence of both clang-specific and Checked-C-specific
declarations in header files. It also incorporates the feedback from CCI to
make implicit inclusion of checked header files as default.
- We primarily adopt Solution 3 for header files that do not have
clang-specific declarations, but have Checked-C-specific declarations (there
are 13 such header files at present). To implicitly include checked headers
by default, the compilation flag (as proposed in Solution 3) now gives a way
to **opt out** of implicit inclusion. Accordingly, it is renamed to
NO_IMPLICIT_INCLUDE_CHECKED_HDRS.
- For each system header file, say `foo.h`, that does not have
clang-specific declarations but has Checked-C-specific declarations, we
will add a new file also called `foo.h` that will contain the following:

#ifdef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
#include_next <foo.h>
#else
#include <foo_checked.h>
#endif

- The file `foo_checked.h` will contain `#include_next <foo.h>` and the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Checked-C-specific declarations.

- 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:
Comment on lines +161 to +163
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- Let `bar.h` be such a header file. Then there already
exists a file called `bar.h` in the `clang/lib/Headers` directory,
which contains clang-specific declarations in the following format:

#include_next <bar.h>
<currently existing clang-specific declarations>

At the end of this pre-existing `bar.h`, we will add the following:

#ifndef NO_IMPLICIT_INCLUDE_CHECKED_HDRS
#include <bar_checked_internal.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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