diff --git a/CHANGES b/CHANGES index e67502a43..bce582998 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index 916dcabd3..94a58942e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.12.0-dev.136 +1.12.0-dev.138 diff --git a/hilti/runtime/include/context.h b/hilti/runtime/include/context.h index a42b81386..7f695e091 100644 --- a/hilti/runtime/include/context.h +++ b/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 { diff --git a/hilti/runtime/include/fiber.h b/hilti/runtime/include/fiber.h index 094ae9748..44837734b 100644 --- a/hilti/runtime/include/fiber.h +++ b/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 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 { diff --git a/hilti/runtime/include/logging.h b/hilti/runtime/include/logging.h index f25f6b660..20732ffbb 100644 --- a/hilti/runtime/include/logging.h +++ b/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 diff --git a/hilti/runtime/src/logging.cc b/hilti/runtime/src/logging.cc index c37e5dbe1..7913617ec 100644 --- a/hilti/runtime/src/logging.cc +++ b/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(); diff --git a/hilti/runtime/src/tests/fiber.cc b/hilti/runtime/src/tests/fiber.cc index 728e6b75f..622ff0ada 100644 --- a/hilti/runtime/src/tests/fiber.cc +++ b/hilti/runtime/src/tests/fiber.cc @@ -1,7 +1,9 @@ // Copyright (c) 2020-2023 by the Zeek Project. See LICENSE for details. #include -#include +#include +#include +#include #include #include @@ -9,6 +11,7 @@ #include #include #include +#include #include 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();