[libcxx-commits] [PATCH] D153902: [libc++][hardening][NFC] Add macros to enable hardened mode.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Since this is not a draft, I looked more in detail. So some additional remarks.



================
Comment at: libcxx/include/__config:213
+#ifndef _LIBCPP_ENABLE_HARDENED_MODE
+#  define _LIBCPP_ENABLE_HARDENED_MODE _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
+#endif
----------------
`_LIBCPP_ENABLE_HARDENED_MODE_DEFAULT` is undefined, or is that in one another patch?
The same for `_LIBCPP_ENABLE_HARDENED_DEBUG_MODE_DEFAULT`.


================
Comment at: libcxx/include/__config:227
+#if _LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
+#  error "Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_HARDENED_DEBUG_MODE can be defined."
+#endif
----------------
Maybe enabled isn't the greatest word too. However defined is wrong; you made sure both are defined.


================
Comment at: libcxx/include/__config:231
+// Hardened mode checks.
+#if _LIBCPP_ENABLE_HARDENED_MODE
+
----------------
The comment on line 238 `// TODO(hardening): more checks to be added here...` seem to suggest we get quite a bit of duplication in the `#if` and `#elif` branch. So I propose

```
#if _LIBCPP_ENABLE_HARDENED_MODE || _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
// do stuff for hardened and hardened debug
#endif // _LIBCPP_ENABLE_HARDENED_MODE || _LIBCPP_ENABLE_HARDENED_DEBUG_MODE

#if _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
// do stuff only for hardened debug
#endif // _LIBCPP_ENABLE_HARDENED_DEBUG_MODE
```


================
Comment at: libcxx/include/__config:258
+
+// TODO: more checks to be added here...
+
----------------
Do we really want to disable things? If I only want to define `_LIBCPP_ABI_BOUNDED_ITERATORS` is that prohibited?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153902/new/

https://reviews.llvm.org/D153902