Skip to content

Commit

Permalink
Associate source code locations with current fiber instead of current…
Browse files Browse the repository at this point in the history
… thread.

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.

Closes #1868.
  • Loading branch information
rsmmr committed Oct 14, 2024
1 parent f06bd93 commit 01dc123
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 16 deletions.
9 changes: 8 additions & 1 deletion hilti/runtime/include/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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 {
Expand Down
12 changes: 12 additions & 0 deletions hilti/runtime/include/fiber.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
34 changes: 22 additions & 12 deletions hilti/runtime/include/logging.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions hilti/runtime/src/logging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
34 changes: 33 additions & 1 deletion hilti/runtime/src/tests/fiber.cc
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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();

0 comments on commit 01dc123

Please sign in to comment.