This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Create a new folder for config-related headers.
AbandonedPublic

Authored by var-const on Jun 1 2023, 9:43 AM.

Details

Reviewers
ldionne
jdoerfert
Group Reviewers
Restricted Project
Summary

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

Event Timeline

var-const created this revision.Jun 1 2023, 9:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
var-const requested review of this revision.Jun 1 2023, 9:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Could you add some information in the commit message why this is a good idea?

var-const updated this revision to Diff 528989.Jun 6 2023, 12:23 PM

Missing files

ldionne added inline comments.Jun 6 2023, 12:30 PM
libcxx/include/__config_dir/debug.h
10

I think I would avoid touching this one since you can't include if from` __config`. Also per the RFC this is mostly going to go away.

libcxx/include/module.modulemap.in
1811–1813

I think removing this should get rid of your circular dependency issue with modules.

ldionne added inline comments.Jun 6 2023, 12:47 PM
libcxx/utils/generate_iwyu_mapping.py
39–40

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.

var-const edited the summary of this revision. (Show Details)Jun 7 2023, 2:49 AM

Could you add some information in the commit message why this is a good idea?

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.

var-const marked 3 inline comments as done.Jun 7 2023, 10:59 AM
var-const added inline comments.
libcxx/utils/generate_iwyu_mapping.py
39–40

Done (added internal_directories).

var-const updated this revision to Diff 529368.Jun 7 2023, 10:59 AM
var-const marked an inline comment as done.
var-const edited the summary of this revision. (Show Details)

Address feedback.

var-const edited the summary of this revision. (Show Details)Jun 7 2023, 11:00 AM

Could you add some information in the commit message why this is a good idea?

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.

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.
So I think we need generated tests verify tests that give an error when I write code like

#include <__config/constexpr.h>

From a header like <format>.

ldionne added inline comments.Jun 7 2023, 1:32 PM
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.

var-const updated this revision to Diff 529505.Jun 7 2023, 10:30 PM

Granularize most of <__config>.

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

var-const added inline comments.Jun 8 2023, 3:23 PM
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?

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?
If we need it we need to think of a better name, umbrella is not descriptive.

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.
(I've no real suggestions for improvements, parts are just in the eye of the beholder.)

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.
(I assume that Clang modules also only care about named declarations.)

ldionne added inline comments.Jun 12 2023, 11:52 AM
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.

var-const abandoned this revision.Sep 1 2023, 2:23 PM

At this point, this patch is stale and hard to rebase. I might want to revisit this in the future, but abandoning for now.