[Git][ghc/ghc][wip/T19078] 2 commits: rts/eventlog: Fix erroneous marker on restarting logging

[David Eichmann]

David Eichmann pushed to branch wip/T19078 at Glasgow Haskell Compiler / GHC


Commits:
0515b269 by Ben Gamari at 2020-12-16T20:43:51+00:00
rts/eventlog: Fix erroneous marker on restarting logging

Previously calling endEventLogging() followed by startEventLogging()
would produce an invalid eventlog due to an erroneous block markers left
over in the header used to push the headers.

Here we fix this by clarifying the invariants surrounding the eventlog
buffers and ensuring that the buffers are cleared when we finish
logging.

Fixes #19078.

- - - - -
3bbaf0e1 by David Eichmann at 2020-12-16T20:43:58+00:00
Test start/endEventlogging: first header must be EVENT_HEADER_BEGIN

- - - - -


5 changed files:

- rts/eventlog/EventLog.c
- + testsuite/tests/rts/RestartEventLogging.hs
- + testsuite/tests/rts/RestartEventLogging.stdout
- + testsuite/tests/rts/RestartEventLogging_c.c
- testsuite/tests/rts/all.T


Changes:

=====================================
rts/eventlog/EventLog.c
=====================================
@@ -579,11 +579,19 @@ eventLogStatus(void)
     }
 }
 
+/*
+ * Preconditions:
+ *  - initEventLogging has been called
+ *  - all buffers are empty
+ *
+ * N.B. the eventlog format requires that the header not be preceded by a block marker.
+ * c.f. #19078.
+ */
 static bool
 startEventLogging_(void)
 {
     initEventLogWriter();
-
+    ASSERT(eventBuf.begin == eventBuf.pos);
     postHeaderEvents();
 
     // Flush capEventBuf with header.
@@ -594,7 +602,9 @@ startEventLogging_(void)
     printAndClearEventBuf(&eventBuf);
 
     for (uint32_t c = 0; c < get_n_capabilities(); ++c) {
-        postBlockMarker(&capEventBuf[c]);
+        EventsBuf *eb = &capEventBuf[c];
+        ASSERT(eb->begin == eb->pos); // buffers must be empty
+        postBlockMarker(eb);
     }
     return true;
 }
@@ -632,6 +642,7 @@ endEventLogging(void)
     // Flush all events remaining in the buffers.
     for (uint32_t c = 0; c < n_capabilities; ++c) {
         printAndClearEventBuf(&capEventBuf[c]);
+        resetEventsBuf(&capEventBuf[c]);
     }
     printAndClearEventBuf(&eventBuf);
     resetEventsBuf(&eventBuf); // we don't want the block marker
@@ -642,6 +653,9 @@ endEventLogging(void)
     // Flush the end of data marker.
     printAndClearEventBuf(&eventBuf);
 
+    // Ensure the buffer is clear in case we later restart logging
+    resetEventsBuf(&eventBuf);
+
     stopEventLogWriter();
     event_log_writer = NULL;
     eventlog_enabled = false;


=====================================
testsuite/tests/rts/RestartEventLogging.hs
=====================================
@@ -0,0 +1,13 @@
+{-# LANGUAGE ForeignFunctionInterface #-}
+
+import System.IO
+
+-- Test that the start/end/restartEventLog interface works as expected.
+main :: IO ()
+main = do
+  putStrLn "Restarting eventlog..."
+  hFlush stdout
+  c_restart_eventlog
+
+foreign import ccall unsafe "c_restart_eventlog"
+  c_restart_eventlog :: IO ()


=====================================
testsuite/tests/rts/RestartEventLogging.stdout
=====================================
@@ -0,0 +1,22 @@
+Restarting eventlog...
+init
+Event log started with EVENT_HEADER_BEGIN
+write
+write
+write
+write
+stop
+init
+Event log started with EVENT_HEADER_BEGIN
+write
+write
+write
+write
+stop
+init
+Event log started with EVENT_HEADER_BEGIN
+write
+write
+write
+write
+stop


=====================================
testsuite/tests/rts/RestartEventLogging_c.c
=====================================
@@ -0,0 +1,69 @@
+#include <stdio.h>
+#include <Rts.h>
+#include <rts/EventLogFormat.h>
+
+#define STOPPED       0
+#define STARTED       1
+#define WRITTEN       2
+
+static int32_t state = STOPPED;
+
+void test_init(void) {
+  if (state != STOPPED) {
+    printf("test_init was not called first or directly after test_stop\n");
+  }
+
+  state = STARTED;
+  printf("init\n");
+  fflush(stdout);
+}
+
+bool test_write(void *eventlog, size_t eventlog_size) {
+  if (state == STOPPED) {
+    printf("test_init was not called\n");
+  }
+  if (state == STARTED) {
+    // Note that the order of bytes is reversed compared with EVENT_HEADER_BEGIN
+    int32_t header = *((int32_t *)eventlog);
+    if (header != 0x62726468) {
+      printf("ERROR: event does not start with EVENT_HEADER_BEGIN\n");
+    }
+    else {
+      printf("Event log started with EVENT_HEADER_BEGIN\n");
+    }
+  }
+
+  state = WRITTEN;
+
+  printf("write\n");
+  fflush(stdout);
+  return true;
+}
+
+void test_flush(void) {
+  printf("flush\n");
+  fflush(stdout);
+}
+
+void test_stop(void) {
+  state = STOPPED;
+  printf("stop\n");
+  fflush(stdout);
+}
+
+const EventLogWriter writer = {
+  .initEventLogWriter = test_init,
+  .writeEventLog = test_write,
+  .flushEventLog = test_flush,
+  .stopEventLogWriter = test_stop
+};
+
+void c_restart_eventlog(void) {
+  for (int i = 0; i < 3; i++) {
+    if (!startEventLogging(&writer)) {
+      printf("failed to start eventlog\n");
+    }
+    endEventLogging();
+  }
+}
+


=====================================
testsuite/tests/rts/all.T
=====================================
@@ -419,6 +419,9 @@ test('T13676',
 test('InitEventLogging',
      [only_ways(['normal']), extra_run_opts('+RTS -RTS')],
      compile_and_run, ['-eventlog InitEventLogging_c.c'])
+test('RestartEventLogging',
+     [only_ways(['normal']), extra_run_opts('+RTS -RTS')],
+     compile_and_run, ['-eventlog RestartEventLogging_c.c'])
 
 test('T17088',
      [only_ways(['normal']), extra_run_opts('+RTS -c -A256k -RTS')],



View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/d2dfb7b918cc49df021c56fffce79c372045b862...3bbaf0e1b0bac4f9074fc5fbdc9e42f15fa6d984

-- 
View it on GitLab: https://gitlab.haskell.org/ghc/ghc/-/compare/d2dfb7b918cc49df021c56fffce79c372045b862...3bbaf0e1b0bac4f9074fc5fbdc9e42f15fa6d984
You're receiving this email because of your account on gitlab.haskell.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-commits/attachments/20201216/de5025fd/attachment-0001.html>