[PATCH] D31178: [libcxxabi] Fix exception address alignment test for EHABI

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

For the most part this LGTM other than the nits mentioned.



================
Comment at: test/libcxxabi/test/config.py:34
+            'libunwind_src',
+            os.path.join(self.libcxxabi_src_root, '/../libunwind/src'))
 
----------------
I think the correct default here is `None`, not `../libunwind/src`, since we might not be using libunwind at all.


================
Comment at: test/libcxxabi/test/config.py:93
+           'libunwind_headers',
+           os.path.join(self.libunwind_src, '/../include'))
+        if self.get_lit_bool('llvm_unwinder', False):
----------------
I'm not sure if this is default is optimal. I think None might be less surprising.


================
Comment at: test/lit.site.cfg.in:9
 config.cxx_headers              = "@LIBCXXABI_LIBCXX_INCLUDES@"
+config.libunwind_src            = "@LIBCXXABI_LIBUNWIND_SOURCES@"
+config.libunwind_headers        = "@LIBCXXABI_LIBUNWIND_INCLUDES_INTERNAL@"
----------------
I don't think we need the unwind sources anywhere within the test suite. I would remove this option all together and simply depend on `libunwind_headers`.


================
Comment at: test/lit.site.cfg.in:10
+config.libunwind_src            = "@LIBCXXABI_LIBUNWIND_SOURCES@"
+config.libunwind_headers        = "@LIBCXXABI_LIBUNWIND_INCLUDES_INTERNAL@"
 config.cxx_library_root         = "@LIBCXXABI_LIBCXX_LIBRARY_PATH@"
----------------
I think `libunwind_headers` should be the empty string when LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL has not been found, instead of expanding to "LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL-NOTFOUND"


================
Comment at: test/test_exception_address_alignment.pass.cpp:30
+// Itanium: Largest supported alignment for the system
+#if _LIBUNWIND_ARM_EHABI
+#  define EXPECTED_ALIGNMENT 8
----------------
Shouldn't this be an `#ifdef`?


https://reviews.llvm.org/D31178