Skip to content
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

<xkeycheck.h>: Fix circular inclusion for Header Units #1692

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

StephanTLavavej
Copy link
Member

Works towards #60.

Discovered by @olgaark - we've done something weird in the machinery that detects macroized keywords, and this is a problem for building Standard Library Header Units.

We had correctly marked xkeycheck.h as not being automatically importable as a header unit:

// "xkeycheck.h", // internal header, provides no machinery, scans for macroized keywords only

And correctly marked yvals_core.h (the STL's central internal header) as being importable (like the vast majority of STL headers):

"yvals_core.h"

Next, yvals_core.h defines _STL_COMPILER_PREPROCESSOR:

#ifndef _STL_COMPILER_PREPROCESSOR
// All STL headers avoid exposing their contents when included by various
// non-C++-compiler tools to avoid breaking builds when we use newer language
// features in the headers than such tools understand.
#if defined(RC_INVOKED) || defined(Q_MOC_RUN) || defined(__midl)
#define _STL_COMPILER_PREPROCESSOR 0
#else
#define _STL_COMPILER_PREPROCESSOR 1
#endif
#endif // _STL_COMPILER_PREPROCESSOR

And includes vcruntime.h for _HAS_CXX17 etc. before including xkeycheck.h:

STL/stl/inc/yvals_core.h

Lines 341 to 342 in 3c13e3e

#include <vcruntime.h>
#include <xkeycheck.h> // The _HAS_CXX tags must be defined before including this.

🪲 Finally, because xkeycheck.h wants _STL_COMPILER_PREPROCESSOR and _HAS_CXX17 etc. it includes yvals_core.h, which is circular inclusion!

STL/stl/inc/xkeycheck.h

Lines 6 to 10 in 3c13e3e

#pragma once
#ifndef _XKEYCHECK_H
#define _XKEYCHECK_H
#include <yvals_core.h>
#if _STL_COMPILER_PREPROCESSOR

This wasn't causing problems for ordinary compilation (because yvals_core.h's #pragma once and include guards prevent the recursion), but attempting to build STL Header Units in an optimal way involves "source dependencies" scanning, and this makes yvals_core.h look like it includes itself, which is a problem.

The fix is simple: remove the circular inclusion, and simply comment that xkeycheck.h is super special. Nothing else in the STL includes it, and users should not be including it directly. (I vaguely recall that at one time, every STL header tried to include xkeycheck.h itself, but we centralized this long ago.)

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Feb 24, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 24, 2021 01:54
@@ -6,7 +6,10 @@
#pragma once
#ifndef _XKEYCHECK_H
#define _XKEYCHECK_H
#include <yvals_core.h>

// xkeycheck.h assumes that it's being included by yvals_core.h in a specific order.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of assuming, should we #error if _YVALS_CORE_H_ isn't defined? I'm inclined to not check, since (1) as you say elsewhere no user should ever include this and there's no reason to include from elsewhere in the STL, and (2) if someone does somehow manage to include this before yvals_core.h the effect will simply be to disable the contents (_STL_COMPILER_PREPROCESSOR won't be defined). There's no danger of silent bad codegen.

The flip side of this coin is, if this file is only supposed to be included from one place ever, why not inline its contents and delete this file instead of forcing the preprocessor to inline it into yvals_core.h on every compile?

(This is an "approve with suggestions" comment: it's worth discussion, but I certainly won't be bothered if we merge this as is.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm inclined to not check

Same. I would add (3) this wouldn't enforce the particular ordering we care about (early enough to protect most of yvals_core.h from macros, but after the definitions of _STL_COMPILER_PREPROCESSOR and _HAS_CXXmeow).

The flip side of this coin is, if this file is only supposed to be included from one place ever, why not inline its contents and delete this file instead of forcing the preprocessor to inline it into yvals_core.h on every compile?

We could do that (as we recently did for <cmath>), but xkeycheck.h is rather large (almost 600 lines, 20 KB) and extremely boring (it should do nothing for builds that aren't doomed); yvals_core.h is already big enough (almost 1400 lines, 67 KB) and contains lots of important stuff.

What I really want is for the compilers to reward keyword macros with doom.

Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

Looks good (xkeycheck sounds like an NSA program doesn't it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants