This is in preparation for adding more configuration macros for hardening purposes. __config is a large monolithic header that can be hard to follow because it contains many unrelated groups of macros. The idea is to split it into several headers each focused on a different aspect (e.g. ABI, support for older language versions, hardening modes in the future, etc.). The split headers are placed in a directory to reduce clutter at the top level.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/utils/generate_iwyu_mapping.py | ||
|---|---|---|
| 40–41 | I wouldn't special case for __config_dir here. The libcxx.imp file we generate right now is already incorrect in a few instances (for example we generate a line with { include: [ "@<__debug_utils/.*>", "private", "<debug_utils>", "public" ] },) -- let's figure out how to handle it correctly for all our internal directories at once, not here. We probably want to do something like this: internal_directories = ['__config_dir', '__debug_utils', etc...]
for i in detail_directories:
if i in internal_direcotries:
continue
...Or if you want, I guess you could actually do this in this patch and handle the following directories: __debug_utils, __fwd, __pstl, __support. | |
Comment Actions
Added some comments to the patch description, please let me know what you think. This came out of my first attempts to add macros for configuring hardening modes. I find __config hard to navigate due to its size, and it seems to have a few "themes" that seem like a good fit to be split into separate, more focused headers. I thought to refactor __config first before adding even more stuff to it. I also hope that having more focused headers (e.g. __config_dir/language_support.h, __config_dir/abi.h) would make finding macros easier.
| libcxx/utils/generate_iwyu_mapping.py | ||
|---|---|---|
| 40–41 | Done (added internal_directories). | |
Comment Actions
Thanks! I agree the file is somewhat large and hard to navigate.
I think granularizing this header has some pitfalls as mention in my comment, but I think we can find ways to guard against them.
| libcxx/include/__config_dir/base.h | ||
|---|---|---|
| 12 | I actually would rather like to see how you envision the final split, this is the normal way how we granularize headers. I also think we should never include one of the granularized headers from headers outside of __config/, except from config. Here I feel that including the granularized headers could easily result in bugs. A define not set might be on purpose or a bug due to forgetting an include. #include <__config/constexpr.h> From a header like <format>. | |
| libcxx/include/__config_dir/base.h | ||
|---|---|---|
| 12 | +1, this makes sense to me. Here's a few categories we spotted easily during live review: environment.h or versions.h // detects compiler and language version abi.h // all the ABI flags visibility.h // all the macros related to visibility annotations, DLL_VIS, etc. compiler_shims.h // maybe for things like `#define __has_attribute(x) 0` depending on what's the use of those random.h // we have stuff related to random device endian.h // endianness detection ????.h // detection of stuff like _LIBCPP_HAS_OBJC_ARC deprecation.h // we have a bunch of deprecation macros I still suggest we keep the uncategorized rest into base.h or we might end up with really small headers, but I'd be fine with that too. | |
Comment Actions
@Mordante I gave granularizing a shot. There's still feedback to address, but I uploaded the split headers in case you'd like to take a look. Some notes:
- for splitting, some headers had to be split because they are essentially dependencies for other headers (e.g. that's why I split object_format.h even though it's very small). For others, I went mostly by size of the category, but sometimes created really small headers if the macros seemed focused on something very specific (e.g. objc.h). There definitely is some overlap between different categories, so I'm very open to suggestions on how to rearrange / combine / etc.;
- base.h probably can be renamed now (it feels more like "misc").
One important thing was to make sure that new small headers don't rely on macros from other headers that they don't include. Unfortunately since we rely on #if defined, forgetting to include a necessary header will often result in a silent error (I presume, I didn't test that yet). To make it easier, I created the base_umbrella.h header that contains the most commonly used "core" headers and decided to include it everywhere (except the core headers themselves). There are still a few places that include stuff like abi.h and c_std_library.h.
This problem existed before (it was quite possible to add new macros somewhere in the middle of the file and have them rely on something that wasn't defined yet). I wonder if moving to #if checks instead of #if defined would be a good idea.
| libcxx/include/__config_dir/base_umbrella.h | ||
|---|---|---|
| 17 ↗ | (On Diff #529505) | Perhaps features.h, object_format.h and versions.h should be merged into e.g. environment.h? |
Comment Actions
I like the direction this is going, still somewhat hesitant since we don't have good guards against directly including subheaders. I want to think a bit about that.
| libcxx/include/__config_dir/attributes.h | ||
|---|---|---|
| 21–33 ↗ | (On Diff #529505) | Maybe in a followup remove the blank lines. |
| libcxx/include/__config_dir/base_umbrella.h | ||
| 13 ↗ | (On Diff #529505) | Why do we need this header? |
| libcxx/include/__config_dir/c_std_library.h | ||
| 11 ↗ | (On Diff #529505) | How about libc_support.h, when I saw c std library I thought about the c headers in the standard. |
| libcxx/include/__config_dir/compiler_shims.h | ||
| 11 ↗ | (On Diff #529505) | Likewise would I call this compiler_support.h |
| libcxx/include/__config_dir/platform_msvc.h | ||
| 11 ↗ | (On Diff #529505) | Is this MSVC or Windows in general? |
| libcxx/include/__config_dir/versions.h | ||
| 30 ↗ | (On Diff #529505) | Why not at the top? |
| 62–67 ↗ | (On Diff #529505) | Maybe in a followup move C++03 in the NOLINT block. |
| 73–82 ↗ | (On Diff #529505) | In general I sometimes feel some pats can be placed in other places, the borders are not very strong. This I feel stronger about, I would like this in platform_msvc.h |
| libcxx/include/module.modulemap.in | ||
| 4 | We should document that __config_dir is excluded since it only contains macros and no named declarations. | |
| libcxx/include/__config_dir/base.h | ||
|---|---|---|
| 21–49 | This could move to aligned_allocation.h or features.h. | |
| 51–58 | Let's move this to abi.h. | |
| 100–110 | __config_dir/locale.h? | |
| 112–116 | We can move this to objc.h, I think. | |
| libcxx/include/__config_dir/base_umbrella.h | ||
| 13 ↗ | (On Diff #529505) | I think the intent is to avoid the issue where you're using a macro but forget to include the header that defines it. I think we should instead find a way to catch that systematically and not introduce this header, just IWYU from all the config headers. |
| 17 ↗ | (On Diff #529505) | IMO this is fine as-is, at least to begin with. |
| libcxx/include/__config_dir/c_std_library.h | ||
| 40–43 ↗ | (On Diff #529505) | I think this would belong to __config_dir/locale.h. |
| libcxx/include/__config_dir/versions.h | ||
| 30 ↗ | (On Diff #529505) | I think we need the compiler detection to be enabled even when we get included in C mode. That was for <stdatomic.h> IIRC. Honestly my preference would be for all of __config_dir to be include-able in C mode, but I'd do that in a follow-up. |
Comment Actions
At this point, this patch is stale and hard to rebase. I might want to revisit this in the future, but abandoning for now.
I actually would rather like to see how you envision the final split, this is the normal way how we granularize headers.
I also think we should never include one of the granularized headers from headers outside of __config/, except from config. Here I feel that including the granularized headers could easily result in bugs. A define not set might be on purpose or a bug due to forgetting an include.
So I think we need generated tests verify tests that give an error when I write code like
From a header like <format>.