Merge remote-tracking branch 'origin/topic/robin/gh-1868-locations'

* origin/topic/robin/gh-1868-locations:
  Associate source code locations with current fiber instead of current thread.

CHANGES

@@ -1,3 +1,15 @@
+1.12.0-dev.138 | 2024-10-14 16:16:08 +0200
+
+  * GH-1868: Associate source code locations with current fiber instead of current thread. (Robin Sommer, Corelight)
+
+    This fixes potential location mix-ups when switching between fibers.
+    Note that we still need a context-wide fallback location as well
+    because we're not always running inside a fiber.
+
+    I ran a performance comparison before/after and couldn't measure a
+    difference. Looks like using TLS storage was a case of premature
+    optimization.
+
 1.12.0-dev.136 | 2024-10-14 14:20:16 +0200
 
   * Disable cpplint iwyu check. (Benjamin Bannier, Corelight)

VERSION

@@ -1 +1 @@
-1.12.0-dev.136
+1.12.0-dev.138

hilti/runtime/include/context.h

@@ -44,7 +44,7 @@ struct Context {
      */
     resumable::Handle* resumable = nullptr;
 
-    /**  Context-specific state for fiber management. */
+    /** Context-specific state for fiber management. */
     detail::FiberContext fiber;
 
     /**
@@ -59,6 +59,13 @@ struct Context {
 
     /** Current indent level for debug messages. */
     uint64_t debug_indent{};
+
+    /**
+     * Current location for user-visible diagnostic messages if we're running
+     * outside of a fiber; null if not set. Considered valid only if
+     * `resumable` is not set.
+     */
+    const char* location = nullptr;
 };
 
 namespace context {

hilti/runtime/include/fiber.h

@@ -208,6 +208,15 @@ class Fiber {
     auto&& result() { return std::move(_result); }
     std::exception_ptr exception() const { return _exception; }
 
+    /** Returns the current source code location if set, or null if not. */
+    const char* location() const { return _location; }
+
+    /**
+     * Sets the current source code location or unsets it if argument is null.
+     * @param l pointer to a statically allocated string that won't go out of scope.
+     */
+    void setLocation(const char* location = nullptr) { _location = location; }
+
     std::string tag() const;
 
     static std::unique_ptr<Fiber> create();
@@ -260,6 +269,9 @@ class Fiber {
     /** Buffer for the fiber's stack when swapped out. */
     StackBuffer _stack_buffer;
 
+    /** Current location for user-visible diagnostic messages; null if not set. */
+    const char* _location = nullptr;
+
 #ifdef HILTI_HAVE_ASAN
     /** Additional tracking state that ASAN needs. */
     struct {

hilti/runtime/include/logging.h

@@ -36,7 +36,7 @@ void warning(std::string_view msg);
     }
 
 /** Shortcut to `hilti::rt::debug::setLocation`. */
-#define __location__(x) ::hilti::rt::debug::detail::tls_location = x;
+#define __location__(x) ::hilti::rt::debug::setLocation(x);
 
 namespace debug {
 
@@ -79,23 +79,33 @@ inline void dedent(const std::string_view stream) {
         ::hilti::rt::detail::globalState()->debug_logger->dedent(stream);
 }
 
-namespace detail {
-// Stores pointer to a string containing the current HILTI-side source
-// location. This needs to be thread-local but also should also be as cheap as
-// possible for updating the value.
-extern HILTI_THREAD_LOCAL const char* tls_location;
-} // namespace detail
-
 /**
  * Returns the current source code location if set, or null if not.
  */
-inline const char* location() { return detail::tls_location; }
+inline const char* location() {
+    if ( auto* ctx = ::hilti::rt::context::detail::current() ) {
+        if ( auto* r = ctx->resumable )
+            return r->location();
+        else
+            return ctx->location;
+    }
+    else
+        return nullptr;
+}
 
 /**
- * Sets the current source code location or unsets it if no argument.
- * *loc* must point to a static string that won't go out of scope.
+ * Sets the current source code location, or unsets it if argument is null.
+ *
+ * @param l pointer to a statically allocated string that won't go out of scope.
  */
-inline void setLocation(const char* l = nullptr) { detail::tls_location = l; }
+inline void setLocation(const char* l = nullptr) {
+    if ( auto* ctx = ::hilti::rt::context::detail::current() ) {
+        if ( auto* r = ctx->resumable )
+            r->setLocation(l);
+        else
+            ctx->location = l;
+    }
+}
 
 /**
  * Prints a string, or a runtime value, to a specific debug stream. This is a

hilti/runtime/src/logging.cc

@@ -13,8 +13,6 @@ using namespace hilti::rt::detail;
 
 using namespace hilti::rt;
 
-HILTI_THREAD_LOCAL const char* debug::detail::tls_location = nullptr;
-
 void hilti::rt::internalError(std::string_view msg) {
     std::cerr << fmt("[libhilti] Internal error: %s", msg) << '\n';
     abort_with_backtrace();

hilti/runtime/src/tests/fiber.cc

@@ -1,14 +1,17 @@
 // Copyright (c) 2020-2023 by the Zeek Project. See LICENSE for details.
 
 #include <exception>
-#include <sstream>
+#include <memory>
+#include <string>
+#include <utility>
 
 #include <hilti/rt/configuration.h>
 #include <hilti/rt/doctest.h>
 #include <hilti/rt/exception.h>
 #include <hilti/rt/fiber-check-stack.h>
 #include <hilti/rt/fiber.h>
 #include <hilti/rt/init.h>
+#include <hilti/rt/logging.h>
 #include <hilti/rt/result.h>
 
 class TestDtor { //NOLINT
@@ -345,4 +348,33 @@ TEST_CASE("stack-size-check" * doctest::skip(isMacosAsan())) {
     CHECK_THROWS_AS(hilti::rt::fiber::execute(f), hilti::rt::StackSizeExceeded);
 }
 
+TEST_CASE("locations") {
+    hilti::rt::init();
+
+    __location__("global");
+
+    auto f1 = [&](hilti::rt::resumable::Handle* r) {
+        __location__("f1");
+        r->yield();
+        CHECK(strcmp(hilti::rt::debug::location(), "f1") == 0);
+        return hilti::rt::Nothing();
+    };
+
+    auto f2 = [&](hilti::rt::resumable::Handle* r) {
+        __location__("f2");
+        r->yield();
+        CHECK(strcmp(hilti::rt::debug::location(), "f2") == 0);
+        return hilti::rt::Nothing();
+    };
+
+    auto r1 = hilti::rt::fiber::execute(f1);
+    auto r2 = hilti::rt::fiber::execute(f2);
+    r2.resume();
+    r1.resume();
+    CHECK(r1);
+    CHECK(r2);
+
+    CHECK(strcmp(hilti::rt::debug::location(), "global") == 0);
+}
+
 TEST_SUITE_END();