[flang] Unittests for runtime terminator

[ashermancinelli]

Summary

Create test fixture for runtime tests which enables verification
of failure cases. Test some runtime IO APIs for failure cases.
Support testing efforts in D98303. Expand on effort discussed
in D98601.

[awarzynski]

Thank you for working on this!

This is a very neat validation of the crash handling API. It also demonstrates that the GTest API in no way obfuscates that crash output. In other words, this shows that we can rely on Fortran::runtime::Terminator::RegisterCrashHandler in other GTest unit tests.

I've left a few comments inline.

[nit] Could you clarify somewhere _why_ testing these particular APIs?

Otherwise, LGTM!

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

31–32

This is not needed, is it? AFAIK, for every unit tests a new test fixture would be generated anyway.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp

65–66

This would be a bit more idiomatic (helps, a bit, to understand what the magic number is):

ASSERT_DEATH(IONAME(OutputInteger64)(cookie, /*x=*/0xfeedface),
    "Unknown 'C' edit descriptor in FORMAT");

Similar comment in other places.

129–138

Any chance to make it a bit more visible what the difference between the 2 blocks is? Same elsewhere.

[ashermancinelli]

More thoroughly comment runtime crash tests.

[ashermancinelli]

Fix header guards to conform to LLVM style guide

[ashermancinelli]

Remove superfluous comments

[ashermancinelli]

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

31–32

The terminator's crash handler is static, so I think we only want to register the crash handler once in a translation unit, unless I'm mistaken.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp

65–66

IMO, /*x=*/ does not give any additional information. However I am a fan of the /*param=*/ style - do you have a suggestion for a more descriptive parameter value to use? Also, in this case, I think the name OutputInteger64 is clear enough that we don't need the parameter name comment that badly.

129–138

I left a comment at the top outlining my rationale for performing each IO operation twice - is this sufficient in your opinion?

[awarzynski]

LGTM, thank you!

As this patch addresses some concerns raised by @klausler in the review for D98303, let's wait at least 24hr before landing this. In case there are some additional comments.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp

65–66

I agree and I don't have any good suggestion. Let's leave it as is.

129–138

I was referring to the fact that in e.g. OverwriteBufferComplexTest you are testing 2 APIs, whereas in OverwriteBufferComplexTest you are testing only one. That wasn't immediately obvious.

This was a nit. If you can improve (e.g. through a comment), that would be great. It's not a blocker.

flang/unittests/RuntimeGTest/CMakeLists.txt

 add_flang_unittest(FlangRuntimeTests
   CharacterTest.cpp
+  RuntimeCrashTest.cpp
+  CrashHandlerFixture.cpp
 )
 
 target_link_libraries(FlangRuntimeTests
   PRIVATE
   FortranRuntime
 )

flang/unittests/RuntimeGTest/CrashHandlerFixture.h

+//===-- flang/unittests/RuntimeGTest/CrashHandlerFixture.h ------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+/// Test fixture registers a custom crash handler to ensure death tests fail
+/// with expected message.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_FLANG_UNITTESTS_RUNTIMEGTEST_CRASHHANDLERFIXTURE_H
+#define LLVM_FLANG_UNITTESTS_RUNTIMEGTEST_CRASHHANDLERFIXTURE_H
+#include <gtest/gtest.h>
+
+struct CrashHandlerFixture : testing::Test {
+  void SetUp();
+};
+
+#endif

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

+//===-- flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "CrashHandlerFixture.h"
+#include "../../runtime/terminator.h"
+#include <cstdarg>
+#include <cstdlib>
+
+// Replaces Fortran runtime's crash handler so we can verify the crash message
+[[noreturn]] static void CatchCrash(
+    const char *sourceFile, int sourceLine, const char *message, va_list &ap) {
+  char buffer[1000];
+  std::vsnprintf(buffer, sizeof buffer, message, ap);
+  va_end(ap);
+  llvm::errs()
+      << "Test "
+      << ::testing::UnitTest::GetInstance()->current_test_info()->name()
+      << " crashed in file "
+      << (sourceFile ? sourceFile : "unknown source file") << '(' << sourceLine
+      << "): " << buffer << '\n';
+  std::exit(EXIT_FAILURE);
+}
+
+// Register the crash handler above when creating each unit test in this suite
+void CrashHandlerFixture::SetUp() {
+  static bool isCrashHanlderRegistered{false};
+  if (not isCrashHanlderRegistered)
+    Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
+  isCrashHanlderRegistered = true;
+}

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp

+//===-- flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+/// Selected APIs are tested here to support development of unit tests for other
+/// runtime components and ensure the test fixture handles crashes as we expect.
+//
+//===----------------------------------------------------------------------===//
+#include "CrashHandlerFixture.h"
+#include "../../runtime/io-api.h"
+#include "../../runtime/terminator.h"
+#include <gtest/gtest.h>
+
+using namespace Fortran::runtime;
+using namespace Fortran::runtime::io;
+
+//------------------------------------------------------------------------------
+/// Test crashes through direct calls to terminator methods
+//------------------------------------------------------------------------------
+struct TestTerminator : CrashHandlerFixture {};
+
+#define TEST_CRASH_HANDLER_MESSAGE \
+  "Intentionally crashing runtime for unit test"
+
+TEST(TestTerminator, CrashTest) {
+  static Fortran::runtime::Terminator t;
+  ASSERT_DEATH(t.Crash(TEST_CRASH_HANDLER_MESSAGE), TEST_CRASH_HANDLER_MESSAGE);
+}
+
+#undef TEST_CRASH_HANDLER_MESSAGE
+
+TEST(TestTerminator, CheckFailedLocationTest) {
+  static Fortran::runtime::Terminator t;
+  ASSERT_DEATH(t.CheckFailed("predicate", "someFileName", 789),
+      "RUNTIME_CHECK\\(predicate\\) failed at someFileName\\(789\\)");
+}
+
+TEST(TestTerminator, CheckFailedTest) {
+  static Fortran::runtime::Terminator t;
+  ASSERT_DEATH(t.CheckFailed("predicate"),
+      "RUNTIME_CHECK\\(predicate\\) failed at \\(null\\)\\(0\\)");
+}
+
+//------------------------------------------------------------------------------
+/// Test misuse of io api
+//------------------------------------------------------------------------------
+struct TestIOCrash : CrashHandlerFixture {};
+
+TEST(TestIOCrash, FormatDescriptorWriteMismatchTest) {
+  static constexpr int bufferSize{4};
+  static char buffer[bufferSize];
+  static const char *format{"(A4)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  ASSERT_DEATH(IONAME(OutputInteger64)(cookie, 0xfeedface),
+      "Data edit descriptor 'A' may not be used with an INTEGER data item");
+}
+
+TEST(TestIOCrash, InvalidFormatCharacterTest) {
+  static constexpr int bufferSize{1};
+  static char buffer[bufferSize];
+  static const char *format{"(C1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  ASSERT_DEATH(IONAME(OutputInteger64)(cookie, 0xfeedface),
+      "Unknown 'C' edit descriptor in FORMAT");
+}
+
+//------------------------------------------------------------------------------
+/// Test buffer overwrites with Output* functions
+/// Each test performs the tested IO operation correctly first, before causing
+/// an overwrite to demonstrate that the failure is caused by the overwrite and
+/// not a misuse of the API.
+//------------------------------------------------------------------------------
+TEST(TestIOCrash, OverwriteBufferAsciiTest) {
+  static constexpr int bufferSize{4};
+  static char buffer[bufferSize];
+  static const char *format{"(A4)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputAscii)(cookie, "four", bufferSize);
+  ASSERT_DEATH(IONAME(OutputAscii)(cookie, "Too many characters!", 20),
+      "Internal write overran available records");
+}
+
+TEST(TestIOCrash, OverwriteBufferCharacterTest) {
+  static constexpr int bufferSize{1};
+  static char buffer[bufferSize];
+  static const char *format{"(A1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputCharacter)(cookie, "a", 1);
+  ASSERT_DEATH(IONAME(OutputCharacter)(cookie, "a", 1),
+      "Internal write overran available records");
+}
+
+TEST(TestIOCrash, OverwriteBufferLogicalTest) {
+  static constexpr int bufferSize{1};
+  static char buffer[bufferSize];
+  static const char *format{"(L1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputLogical)(cookie, true);
+  ASSERT_DEATH(IONAME(OutputLogical)(cookie, true),
+      "Internal write overran available records");
+}
+
+TEST(TestIOCrash, OverwriteBufferRealTest) {
+  static constexpr int bufferSize{1};
+  static char buffer[bufferSize];
+  static const char *format{"(F1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputReal32)(cookie, 1.);
+  EXPECT_DEATH(IONAME(OutputReal32)(cookie, 1.),
+      "Internal write overran available records");
+
+  std::memset(buffer, '\0', bufferSize);
+  cookie = IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format));
+  IONAME(OutputReal64)(cookie, 1.);
+  EXPECT_DEATH(IONAME(OutputReal64)(cookie, 1.),
+      "Internal write overran available records");
+}
+
+TEST(TestIOCrash, OverwriteBufferComplexTest) {
+  static constexpr int bufferSize{8};
+  static char buffer[bufferSize];
+  static const char *format{"(Z1,Z1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputComplex32)(cookie, 1., 1.);
+  EXPECT_DEATH(IONAME(OutputComplex32)(cookie, 1., 1.),
+      "Internal write overran available records");
+
+  std::memset(buffer, '\0', bufferSize);
+  cookie = IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format));
+  IONAME(OutputComplex64)(cookie, 1., 1.);
+  EXPECT_DEATH(IONAME(OutputComplex64)(cookie, 1., 1.),
+      "Internal write overran available records");
+}
+
+TEST(TestIOCrash, OverwriteBufferIntegerTest) {
+  static constexpr int bufferSize{1};
+  static char buffer[bufferSize];
+  static const char *format{"(I1)"};
+  auto *cookie{IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format))};
+  IONAME(OutputInteger64)(cookie, 0xdeadbeef);
+  ASSERT_DEATH(IONAME(OutputInteger64)(cookie, 0xdeadbeef),
+      "Internal write overran available records");
+}