This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add a new test matcher for tests raising floating point exceptions.
ClosedPublic

Authored by sivachandra on Jul 15 2021, 11:22 AM.

Details

Summary

This new matcher does not use death tests to check if SIGFPE is raised.
Instead, that a SIGFPE was raised is checked using a SIGFPE signal handler.

Diff Detail

Event Timeline

sivachandra created this revision.Jul 15 2021, 11:22 AM
sivachandra requested review of this revision.Jul 15 2021, 11:22 AM

I am not sure how this will play with Fuchsia's zxtest framework.

Do you anticipate this will be used later to match against if any exception was raised? I imagine in that case the current logic would be moved from the constructor into match.

If not, I think this would be a lot simpler if the logic inside of FPExceptMatcher::FPExceptMatcher was just a function called testSigfpeRaised which returned caughtExcept and then RAISES_FP_EXCEPT would just be ASSERT_TRUE(testSigfpeRaised(func))

Also, what does this solve that the death tests do not, Windows support?

libc/utils/FPUtil/TestHelpers.cpp
19

Not used

86

siglongjmp takes a sigjmp_buf not jmp_buf. I'm guessing [sig]jmp_buf are macros to the same type but we should still use sigjmp_buf when not in Windows.

libc/utils/FPUtil/TestHelpers.h
86

Not used

134

Should this be called ASSERT_RAISES_FP_EXCEPT?

138

Can this be __llvm_libc::testing::Test::createCallable(func)

sivachandra marked 3 inline comments as done.

Address comments.

sivachandra marked an inline comment as done.Jul 15 2021, 1:47 PM

Do you anticipate this will be used later to match against if any exception was raised? I imagine in that case the current logic would be moved from the constructor into match.

Yes, I would like to extend this to match the actual raised exception. So, some kind of reordering might be required.

If not, I think this would be a lot simpler if the logic inside of FPExceptMatcher::FPExceptMatcher was just a function called testSigfpeRaised which returned caughtExcept and then RAISES_FP_EXCEPT would just be ASSERT_TRUE(testSigfpeRaised(func))

The appeal of the matcher over an ASSERT_TRUE(...) kind of check is the custom error message.

Also, what does this solve that the death tests do not, Windows support?

That is the primary motivation. However, I am not yet sure how this will fit with Fuchsia's scheme of things.

libc/utils/FPUtil/TestHelpers.h
138

Yes, we could. I am trying not to grow a dependency from testutils/ExecuteFunction.h. I want to move testutils in to UnitTest as soon as I can. So, we can do that cleanup at that time.

For Fuchsia you should just define the new macro to continue to use zxtest's ASSERT_DEATH macro.

libc/utils/FPUtil/TestHelpers.cpp
77

It seems like it would be cleaner to just do

#define sigjmp_buf jmp_buf
#define sigsetjmp(buf, save) setjmp(buf)
#define siglongjmp(buf) longjmp(buf)

and not have any more conditionals below.

86

Your handler is only called for the signals you've set it on, so this will never be entered with a different value and you don't need to test it.

98

Just make unique_ptr the argument type so callers construct it in place.

105

It's probably wise to use fegetenv / fesetenv here.

libc/utils/FPUtil/TestHelpers.h
79

This ctor is superfluous if you just construct with Callable{func}.

sivachandra marked 3 inline comments as done.

Address comments.

libc/utils/FPUtil/TestHelpers.cpp
98

We cannot use std::unique_ptr in the API as we will then have to include <memory> in the header file which will lead to header-mixups in the unit test. That is why I have arranged it this way. Added a comment now in the header file about "ownership transfer".

libc/utils/FPUtil/TestHelpers.h
79

My understanding about brace initializers could be wrong or incomplete here but since the class is dynamic (because of the virtual methods), its objects cannot be initialized with {...}? Clang infact complains that there is no appropriate constructor for it when I use the brace initializer.

For Fuchsia you should just define the new macro to continue to use zxtest's ASSERT_DEATH macro.

I have added an alternate definition in TestHelpers.h.

#ifdef LLVM_LIBC_TEST_USE_FUCHSIA
#define ASSERT_RAISES_FP_EXCEPT(func) ASSERT_DEATH(func, WITH_SIGNAL(SIGFPE))
#else
#define ASSERT_RAISES_FP_EXCEPT(func)
...
#endif // LLVM_LIBC_TEST_USE_FUCHSIA
This revision is now accepted and ready to land.Jul 20 2021, 10:23 AM
This revision was landed with ongoing or failed builds.Jul 20 2021, 9:59 PM
This revision was automatically updated to reflect the committed changes.