[libcxx-commits] [PATCH] D158823: [libc++][hardening] Add back the safe mode.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/CMakeLists.txt:53
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
 option(LIBCXX_ENABLE_FILESYSTEM
   "Whether to include support for parts of the library that rely on a filesystem being
----------------
ldionne wrote:
> @thakis We brainstormed on some names here:
> 
> ```
> _LIBCPP_ENABLE_HARDENED_PLUS_MODE
> _LIBCPP_ENABLE_EXTENDED_HARDENED_MODE
> _LIBCPP_ENABLE_STRONG_HARDENED_MODE
> _LIBCPP_ENABLE_STRICT_MODE
> _LIBCPP_ENABLE_PARANOID_MODE
> _LIBCPP_ENABLE_FORTIFIED_MODE
> _LIBCPP_ENABLE_SAFE_MODE
> ```
> 
> Do you have any thoughts? Our thoughts so far:
> 
> ```
> HARDENED_PLUS, EXTENDED_HARDENED, STRONG_HARDENED // those are kind of heavyweight names
> PARANOID // has negative connotation and doesn't make it clear whether it is stronger than DEBUG
> STRICT // could be confused with the notion of not having non-standard extensions
> FORTIFIED // not clear whether it is stronger than HARDENED or not
> SAFE // our current preference
> ```
> 
> In fact, before LLVM 17 we had something called the `SAFE` mode, and I think what we discovered with Chromium's use case is that it still had its place. Hence, I think what we should do is call this the `SAFE` mode, backport this change, and rework the way we announced our 17 release notes not to say that we "replaced' the safe mode, but instead that we added new modes and that we changed how the safe mode is enabled. This is IMO a superior design and a superior way of rolling it out based on our experience so far.
@Mordante Those are the other names we considered.


================
Comment at: libcxx/utils/libcxx/test/params.py:301-303
                 AddCompileFlag("-D_LIBCPP_ENABLE_HARDENED_MODE=1") if hardening_mode == "hardened" else None,
+                AddCompileFlag("-D_LIBCPP_ENABLE_SAFE_MODE=1")     if hardening_mode == "safe" else None,
                 AddCompileFlag("-D_LIBCPP_ENABLE_DEBUG_MODE=1")    if hardening_mode == "debug" else None,
----------------
Mordante wrote:
> I start to feel slightly uncomfortable with these names. To me they are not very descriptive and they are now 4 options. I don't directly have better suggestions, but I think we should spend a bit of time on this.
> 
> For example, which is more expensive "safe" or "hardened"?
I agree, this is definitely not perfect. This is user facing too, so we need to find something good.

We thought that `safe` was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than `hardened`, but maybe it isn't. I'll CC you on the comment above that discusses naming.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158823