[libcxx-commits] [PATCH] D125242: [libc++abi] Use from-scratch testing configs for libc++abi by default

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 4 inline comments as done.
ldionne added a comment.

In D125242#3534908 <https://reviews.llvm.org/D125242#3534908>, @EricWF wrote:

> I'm not sure I understand what "from scratch" means given this patch, but I don't need to understand to see that to LGTM it. LGTM once the inline comments are addressed.

Yeah, it's not a great name. It's how I've been calling the new style of configuration file where you define only the base Lit substitutions required for your environment instead of using the logic in `config.py` that tried to handle all configurations (and you'd add various knobs to tweak its behavior instead).



================
Comment at: libcxxabi/test/libcxxabi/test/config.py:52
         if not self.get_lit_bool('enable_threads', True):
             self.cxx.compile_flags += ['-D_LIBCXXABI_HAS_NO_THREADS']
+            self.config.available_features.add('libcpp-has-no-threads')
----------------
EricWF wrote:
> If we're using the same feature name for libc++ and libc++abi, I assume that means that they cannot vary independently. 
> 
> If so, should we just use `_LIBCPP_HAS_NO_THREADS` instead, and kill the `LIBCXXABI_HAS_NO_THREADS` macro?
This is actually an intermediate state -- after this patch lands, I plan to rename `libcpp-has-no-threads` to just `no-threads`, like I've done for `no-exceptions`.

> If so, should we just use `_LIBCPP_HAS_NO_THREADS` instead, and kill the `LIBCXXABI_HAS_NO_THREADS` macro?

I think this probably makes sense. I'm adding it to my TODO list for an upcoming patch.



================
Comment at: libcxxabi/test/test_exception_storage.pass.cpp:17
 
+#include "test_macros.h"
+
----------------
EricWF wrote:
> Is this the same `test_macros.h` header as libc++, or a different one?
> 
> 
It's the same. It does mean that `TEST_HAS_NO_THREADS` is defined based on `_LIBCPP_HAS_NO_THREADS`, if that's where you were driving. But in accordance with your comment above, libc++abi will be using the libc++ macro anyway in the future.

The underlying problem is that libc++abi doesn't have a way to persist its configuration state like libc++ (via `__config_site`). So we have to either pass it explicitly with `-DLIBCXXABI_FOO`, or piggy-back on the same state saved in libc++'s `__config_site`.


================
Comment at: libcxxabi/test/test_fallback_malloc.pass.cpp:18
+#ifdef TEST_HAS_NO_THREADS
+# define _LIBCXXABI_HAS_NO_THREADS
+#endif
----------------
EricWF wrote:
> Doesn't the LIT config handle defining this macro?
It doesn't, and I don't think it should, since this is effectively a macro that *should* only matter when compiling the libc++abi sources.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125242