From 740f3f46119ced4128af5584d156bf4c06bbdcc6 Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Sat, 19 Jul 2025 16:05:39 -0300 Subject: [PATCH 1/6] Dump C++ and Status propagation stacktraces. --- test/cpp/test_status_common.h | 201 ++++++++++++++++++++++++---------- torch_xla/csrc/BUILD | 1 + torch_xla/csrc/status.cpp | 101 +++++++++++------ torch_xla/csrc/status.h | 53 +++++---- 4 files changed, 238 insertions(+), 118 deletions(-) diff --git a/test/cpp/test_status_common.h b/test/cpp/test_status_common.h index 7cb63d4f38a..fceabdf8395 100644 --- a/test/cpp/test_status_common.h +++ b/test/cpp/test_status_common.h @@ -18,9 +18,11 @@ #ifndef XLA_TEST_CPP_TEST_STATUS_COMMON_H_ #define XLA_TEST_CPP_TEST_STATUS_COMMON_H_ +#include #include #include +#include #include #include "absl/status/status.h" @@ -30,7 +32,7 @@ namespace torch_xla { -// Enum to control whether C++ error context is shown in status messages +// Enum to control whether C++ error context is shown in status kMessages enum class CppStacktracesMode { kShow, kHide, @@ -74,10 +76,21 @@ class StatusTest : public testing::TestWithParam { namespace testing { -constexpr inline char new_message[] = "New test error message"; -constexpr inline char message[] = "Test error message"; -constexpr inline char test_file[] = "test_file.cpp"; -constexpr inline int32_t line = 42; +constexpr inline char kNewMessage[] = "New test error message"; +constexpr inline char kMessage[] = "Test error message"; +constexpr inline char kFile[] = "test_file.cpp"; +constexpr inline char kEntryPrefix[] = "\n "; +constexpr inline int32_t kLine = 42; + +inline std::optional GetStacktrace(const absl::Status& status) { + if (status.ok()) { + return std::nullopt; + } else { + auto payload = status.GetPayload(kStacktraceKey); + return payload.has_value() ? std::optional(std::string(payload->Flatten())) + : std::nullopt; + } +} TEST_P(StatusTest, MaybeThrowWithOkStatus) { absl::Status ok_status = absl::OkStatus(); @@ -85,8 +98,19 @@ TEST_P(StatusTest, MaybeThrowWithOkStatus) { } TEST_P(StatusTest, MaybeThrowWithErrorStatus) { - absl::Status error_status = absl::InvalidArgumentError(message); - EXPECT_THROW(MaybeThrow(error_status), std::runtime_error); + auto excfn = [=]() { + absl::Status error_status = absl::InvalidArgumentError(kMessage); + MaybeThrow(error_status); + }; + if (IsShowCppStacktracesMode()) { + std::string expected_prefix = + absl::StrCat(kMessage, "\n\nC++ Stacktrace:\n"); + EXPECT_THAT(excfn, ::testing::ThrowsMessage( + ::testing::StartsWith(expected_prefix))); + } else { + EXPECT_THAT(excfn, ::testing::ThrowsMessage( + ::testing::Eq(kMessage))); + } } TEST_P(StatusTest, GetValueOrThrowWithOkStatusOr) { @@ -97,44 +121,57 @@ TEST_P(StatusTest, GetValueOrThrowWithOkStatusOr) { } TEST_P(StatusTest, GetValueOrThrowWithErrorStatusOr) { - absl::StatusOr status_or = absl::InvalidArgumentError(message); + absl::StatusOr status_or = absl::InvalidArgumentError(kMessage); EXPECT_THROW(GetValueOrThrow(std::move(status_or)), std::runtime_error); } TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { - absl::Status error_status = absl::InvalidArgumentError(message); - absl::Status result = MaybeWithLocation(error_status, test_file, line); + absl::Status error_status = absl::InvalidArgumentError(kMessage); + absl::Status result = + status_internal::MaybeWithLocation(error_status, kFile, kLine); if (IsShowCppStacktracesMode()) { ASSERT_NE(result, error_status); EXPECT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); - EXPECT_EQ(result.message(), "Test error message (at test_file.cpp:42)"); + EXPECT_EQ(result.message(), error_status.message()); + EXPECT_EQ(GetStacktrace(result), + absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine, + " (error: ", kMessage, ")")); } else { EXPECT_EQ(result, error_status); } } TEST_P(StatusTest, MaybeWithNewMessageEmptyNewMessage) { - absl::Status error_status = absl::InvalidArgumentError(message); - absl::Status result = MaybeWithNewMessage(error_status, test_file, line); - EXPECT_EQ(result, error_status); + absl::Status error_status = absl::InvalidArgumentError(kMessage); + absl::Status result = + status_internal::MaybeWithNewMessage(error_status, kFile, kLine); + if (IsShowCppStacktracesMode()) { + ASSERT_NE(result, error_status); + EXPECT_EQ(result.code(), error_status.code()); + EXPECT_EQ(result.message(), error_status.message()); + EXPECT_EQ(GetStacktrace(result), + absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine)); + } else { + ASSERT_EQ(result, error_status); + } } TEST_P(StatusTest, MaybeWithNewMessageNonEmptyNewMessage) { - absl::Status error_status = absl::InvalidArgumentError(message); - absl::Status result = - MaybeWithNewMessage(error_status, test_file, line, new_message); + absl::Status error_status = absl::InvalidArgumentError(kMessage); + absl::Status result = status_internal::MaybeWithNewMessage( + error_status, kFile, kLine, kNewMessage); ASSERT_NE(result, error_status); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); + EXPECT_EQ(result.message(), std::string_view(kNewMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(result.message(), - absl::StrCat("New test error message (at test_file.cpp:42)\n" - "From Error: Test error message")); - } else { - EXPECT_EQ(result.message(), std::string_view(new_message)); + auto stacktrace = GetStacktrace(result); + ASSERT_TRUE(stacktrace.has_value()); + EXPECT_EQ(*stacktrace, absl::StrCat(kEntryPrefix, "From: ", kFile, ":", + kLine, " (error: ", kNewMessage, ")")); } } @@ -154,7 +191,7 @@ TEST_P(StatusTest, MacroReturnIfError) { TEST_P(StatusTest, MacroReturnIfErrorWithError) { auto test_function = [=]() -> absl::Status { - absl::Status error_status = absl::InvalidArgumentError(message); + absl::Status error_status = absl::InvalidArgumentError(kMessage); XLA_RETURN_IF_ERROR(error_status); return absl::OkStatus(); }; @@ -162,21 +199,22 @@ TEST_P(StatusTest, MacroReturnIfErrorWithError) { absl::Status result = test_function(); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); - EXPECT_EQ(result.message(), std::string_view(message)); + EXPECT_EQ(result.message(), std::string_view(kMessage)); } TEST_P(StatusTest, MacroReturnIfErrorWithNestedError) { - int32_t errline = 0; - auto inner_test_function = [&errline]() -> absl::Status { - errline = __LINE__ + 1; - return XLA_ERROR_WITH_LOCATION(absl::InvalidArgumentError(message)); + int32_t errline0 = __LINE__ + 2; + auto inner_test_function = []() -> absl::Status { + return XLA_ERROR_WITH_LOCATION(absl::InvalidArgumentError(kMessage)); }; + int32_t errline1 = __LINE__ + 2; auto test_function = [&]() -> absl::Status { XLA_RETURN_IF_ERROR(inner_test_function()); return absl::OkStatus(); }; + int32_t errline2 = __LINE__ + 2; auto outer_test_function = [&]() -> absl::Status { XLA_RETURN_IF_ERROR(test_function()); return absl::OkStatus(); @@ -185,34 +223,34 @@ TEST_P(StatusTest, MacroReturnIfErrorWithNestedError) { absl::Status result = outer_test_function(); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.message(), std::string_view(kMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(result.message(), absl::StrCat("Test error message (at ", - __FILE__, ":", errline, ")")); - } else { - EXPECT_EQ(result.message(), std::string_view(message)); + auto stack0 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline0, + " (error: ", kMessage, ")"); + auto stack1 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline1); + auto stack2 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline2); + EXPECT_EQ(GetStacktrace(result), absl::StrCat(stack0, stack1, stack2)); } } TEST_P(StatusTest, MacroReturnIfErrorWithErrorWithNewMessage) { - int32_t errline = 0; - auto test_function = [&errline]() -> absl::Status { - absl::Status error_status = absl::InvalidArgumentError(message); - errline = __LINE__ + 1; - XLA_RETURN_IF_ERROR(error_status, new_message); + int32_t errline = __LINE__ + 3; + auto test_function = []() -> absl::Status { + absl::Status error_status = absl::InvalidArgumentError(kMessage); + XLA_RETURN_IF_ERROR(error_status, kNewMessage); return absl::OkStatus(); }; absl::Status result = test_function(); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.message(), std::string_view(kNewMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(result.message(), - absl::StrCat("New test error message (at ", __FILE__, ":", - errline, ")\nFrom Error: Test error message")); - } else { - EXPECT_EQ(result.message(), std::string_view(new_message)); + EXPECT_EQ(GetStacktrace(result), + absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, + " (error: ", kNewMessage, ")")); } } @@ -233,7 +271,7 @@ TEST_P(StatusTest, MacroAssignOrReturn) { TEST_P(StatusTest, MacroAssignOrReturnWithError) { auto test_function = []() -> absl::StatusOr { - absl::StatusOr status_or = absl::InvalidArgumentError(message); + absl::StatusOr status_or = absl::InvalidArgumentError(kMessage); XLA_ASSIGN_OR_RETURN(int value, status_or); return value * 2; }; @@ -241,43 +279,88 @@ TEST_P(StatusTest, MacroAssignOrReturnWithError) { absl::StatusOr result = test_function(); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_EQ(result.status().message(), std::string_view(message)); + EXPECT_EQ(result.status().message(), std::string_view(kMessage)); } TEST_P(StatusTest, MacroAssignOrReturnWithErrorWithNewMessage) { - int32_t errline = 0; - - auto test_function = [&errline]() -> absl::StatusOr { - absl::StatusOr status_or = absl::InvalidArgumentError(message); - errline = __LINE__ + 1; - XLA_ASSIGN_OR_RETURN(int value, status_or, new_message); + int32_t errline = __LINE__ + 3; + auto test_function = []() -> absl::StatusOr { + absl::StatusOr status_or = absl::InvalidArgumentError(kMessage); + XLA_ASSIGN_OR_RETURN(int value, status_or, kNewMessage); return value * 2; }; absl::StatusOr result = test_function(); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), std::string_view(kNewMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(result.status().message(), - absl::StrCat("New test error message (at ", __FILE__, ":", - errline, ")\nFrom Error: Test error message")); - } else { - EXPECT_EQ(result.status().message(), std::string_view(new_message)); + EXPECT_EQ(GetStacktrace(result.status()), + absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, + " (error: ", kNewMessage, ")")); } } TEST_P(StatusTest, MacroErrorWithLocation) { - absl::Status error_status = absl::InvalidArgumentError(message); + absl::Status error_status = absl::InvalidArgumentError(kMessage); int32_t errline = __LINE__ + 1; absl::Status result = XLA_ERROR_WITH_LOCATION(error_status); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.message(), std::string_view(kMessage)); + if (IsShowCppStacktracesMode()) { + EXPECT_EQ(GetStacktrace(result), + absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, + " (error: ", kMessage, ")")); + } +} + +TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { + int32_t errline0 = __LINE__ + 2; + auto innerfn = [&]() -> absl::Status { + return XLA_ERROR_WITH_LOCATION(absl::InvalidArgumentError(kMessage)); + }; + + int32_t errline1 = __LINE__ + 2; + auto midfn = [&]() -> absl::Status { + XLA_RETURN_IF_ERROR(innerfn(), kNewMessage); + return absl::OkStatus(); + }; + + int32_t errline2 = __LINE__ + 2; + auto outerfn = [&]() -> absl::Status { + XLA_RETURN_IF_ERROR(midfn()); + return absl::OkStatus(); + }; + + auto excfn = [&]() { MaybeThrow(outerfn()); }; + if (IsShowCppStacktracesMode()) { - EXPECT_EQ(result.message(), absl::StrCat("Test error message (at ", - __FILE__, ":", errline, ")")); + // Expected Error Message Prefix + // ============================= + // + // New test error kMessage + // + // Status Propagation Stacktrace: + // From: ./test/cpp/test_status_common.h:329 (error: Test error + // kMessage) From: ./test/cpp/test_status_common.h:335 (error: New test + // error kMessage) From: ./test/cpp/test_status_common.h:342 + // + // C++ Stacktrace: + // + std::string expected_prefix = absl::StrCat( + kNewMessage, "\n\nStatus Propagation Stacktrace:", kEntryPrefix, + "From: ", __FILE__, ":", errline0, " (error: ", kMessage, ")", + kEntryPrefix, "From: ", __FILE__, ":", errline1, + " (error: ", kNewMessage, ")", kEntryPrefix, "From: ", __FILE__, ":", + errline2, "\n\nC++ Stacktrace:\n"); + + EXPECT_THAT(excfn, ::testing::ThrowsMessage( + ::testing::StartsWith(expected_prefix))); } else { - EXPECT_EQ(result.message(), std::string_view(message)); + EXPECT_THAT(excfn, ::testing::ThrowsMessage( + ::testing::Eq(kNewMessage))); } } diff --git a/torch_xla/csrc/BUILD b/torch_xla/csrc/BUILD index 6c34eca1450..31ab65dbbca 100644 --- a/torch_xla/csrc/BUILD +++ b/torch_xla/csrc/BUILD @@ -377,6 +377,7 @@ cc_library( hdrs = ["status.h"], deps = [ "@torch//:headers", + "@tsl//tsl/platform:stacktrace", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/status:statusor", ], diff --git a/torch_xla/csrc/status.cpp b/torch_xla/csrc/status.cpp index 1eb3511cb33..d5338215a8a 100644 --- a/torch_xla/csrc/status.cpp +++ b/torch_xla/csrc/status.cpp @@ -2,18 +2,43 @@ #include +#include +#include +#include + #include "absl/log/absl_check.h" +#include "tsl/platform/stacktrace.h" namespace torch_xla { -// Common function for generating file location information with a space in the -// beginning. -static std::string LocationStrWithSpace(const char* file, const int32_t line) { - return absl::StrCat(" (at ", file, ":", line, ")"); +// Indent the stacktrace so that it's easier to see. +constexpr char kEntryPrefix[] = "\n "; + +// Creates an error propagation stacktrace entry. +// +// The resulting entry will be appended to the existing stacktrace of the status +// currently being processed. +// +// Example: +// From: : [(error: )] +// +static std::string GetStacktraceEntry(const char* file, const int32_t line, + const std::string_view new_message) { + auto error_suffix = + new_message.empty() ? "" : absl::StrCat(" (error: ", new_message, ")"); + return absl::StrCat(kEntryPrefix, "From: ", file, ":", line, error_suffix); } -absl::Status MaybeWithLocation(const absl::Status& status, const char* file, - const int32_t line) { +// Convenient function that retrieves the stacktrace payload if it exists. +// Otherwise, returns an empty absl::Cord. +static absl::Cord GetStacktraceOrEmptyCord(const absl::Status& status) { + auto opt = status.GetPayload(kStacktraceKey); + return opt.has_value() ? *opt : absl::Cord(); +} + +absl::Status status_internal::MaybeWithLocation(const absl::Status& status, + const char* file, + const int32_t line) { ABSL_CHECK(!status.ok()); // Return the same status if we don't need to add the C++ source location. @@ -21,14 +46,19 @@ absl::Status MaybeWithLocation(const absl::Status& status, const char* file, return status; } - return absl::Status( - status.code(), - absl::StrCat(status.message(), LocationStrWithSpace(file, line))); + // Make sure this is only called on fresh `status` instances. + ABSL_CHECK(GetStacktraceOrEmptyCord(status).empty()); + + // Adding source location to `status` has the same semantics as overwriting + // the status message: + // 1. An stacktrace entry will be added + // 2. The status' message will be the same + return MaybeWithNewMessage(status, file, line, status.message()); } -absl::Status MaybeWithNewMessage(const absl::Status& status, const char* file, - const int32_t line, - const std::string_view new_message) { +absl::Status status_internal::MaybeWithNewMessage( + const absl::Status& status, const char* file, const int32_t line, + const std::string_view new_message) { ABSL_CHECK(!status.ok()); // Return the same status if: @@ -38,38 +68,39 @@ absl::Status MaybeWithNewMessage(const absl::Status& status, const char* file, return status; } - std::string_view old_message = status.message(); - // Replace the old status message with `new_message`, if it's not empty. // // The idea is that whenever `new_message` is given, it should have more // context to give a better error message to the user. - std::string_view message = new_message.empty() ? old_message : new_message; + auto new_status = absl::Status( + status.code(), new_message.empty() ? status.message() : new_message); - // If `TORCH_SHOW_CPP_STACKTRACES` is set, show the context of this error. - // In other words, show: - // 1. The error location - // 2. The old messages that were replaced by `new_message`. - // - // This should give more context for developers. Showing the older error - // messages alongside their debug information. - // - // Note that we also condition showing source location information by (2) - // (i.e. `new_message` is not empty) because we don't really wish to show - // a stacktrace. Instead, we show only the history of error messages that - // has led to the current error. - const std::string context = - (torch::get_cpp_stacktraces_enabled() && !new_message.empty()) - ? absl::StrCat(LocationStrWithSpace(file, line), - "\nFrom Error: ", old_message) - : ""; - - return absl::Status(status.code(), absl::StrCat(message, context)); + // If `TORCH_SHOW_CPP_STACKTRACES` is set: + // 1. append the current source location to the stacktrace payload + // 2. append the new error message, if not empty + if (torch::get_cpp_stacktraces_enabled()) { + auto new_stacktrace = GetStacktraceOrEmptyCord(status); + new_stacktrace.Append(GetStacktraceEntry(file, line, new_message)); + new_status.SetPayload(kStacktraceKey, new_stacktrace); + } + + return new_status; } void MaybeThrow(const absl::Status& status) { if (!status.ok()) { - throw std::runtime_error(std::string(status.message())); + auto status_stacktrace = GetStacktraceOrEmptyCord(status); + auto status_stacktrace_str = + status_stacktrace.empty() + ? "" + : absl::StrCat("\n\nStatus Propagation Stacktrace:", + status_stacktrace.Flatten()); + auto cpp_stacktrace_str = + torch::get_cpp_stacktraces_enabled() + ? absl::StrCat("\n\nC++ Stacktrace:\n", tsl::CurrentStackTrace()) + : ""; + throw std::runtime_error(absl::StrCat( + status.message(), status_stacktrace_str, cpp_stacktrace_str)); } } diff --git a/torch_xla/csrc/status.h b/torch_xla/csrc/status.h index d64a78ba58d..2bb3570608b 100644 --- a/torch_xla/csrc/status.h +++ b/torch_xla/csrc/status.h @@ -14,6 +14,8 @@ namespace torch_xla { +constexpr char kStacktraceKey[] = "stacktrace"; + // If `TORCH_SHOW_CPP_STACKTRACES` is set, creates a new Status instance, // appending the current location (e.g. file and line information) to the // status message. @@ -28,10 +30,11 @@ namespace torch_xla { // // If `TORCH_SHOW_CPP_STACKTRACES` is set, the error shown will be: // -// Error message. (at :) +// RuntimeError: Error message. +// From: : (error: Error message.) // #define XLA_ERROR_WITH_LOCATION(status) \ - ::torch_xla::MaybeWithLocation(status, __FILE__, __LINE__) + ::torch_xla::status_internal::MaybeWithLocation(status, __FILE__, __LINE__) #define XLA_CONCAT_(a, b) XLA_CONCAT_IMPL_(a, b) #define XLA_CONCAT_IMPL_(a, b) a##b @@ -41,15 +44,15 @@ namespace torch_xla { // Provides a flexible way to handle error checking with optional message // modification. It evaluates `expr`, checks if it's OK, and either: -// 1. Returns early with an error status (potentially modified by the provided -// additional messages) -// 2. Proceeds with the given `then` block if successful -#define XLA_RETURN_IF_ERROR_IMPL_(expr, var, then, ...) \ - auto var = (expr); \ - if (!var.ok()) { \ - return ::torch_xla::MaybeWithNewMessage( \ - ::torch_xla::GetStatus(var), __FILE__, __LINE__, ##__VA_ARGS__); \ - } \ +// 1. Returns early with an error status +// 2. Proceeds with the given `then` block if successful +#define XLA_RETURN_IF_ERROR_IMPL_(expr, var, then, ...) \ + auto var = (expr); \ + if (!var.ok()) { \ + return ::torch_xla::status_internal::MaybeWithNewMessage( \ + ::torch_xla::status_internal::GetStatus(var), __FILE__, __LINE__, \ + ##__VA_ARGS__); \ + } \ then // Propagates `rexpr`, in case it's a non-ok status. @@ -65,9 +68,13 @@ namespace torch_xla { // we early return a non-ok status. Then, if `TORCH_SHOW_CPP_STACKTRACES` is // set, the error shown will be: // -// New error message. (at :) -// Previous error message. (at :) -// ... +// RuntimeError: New error message. +// +// Status Propagation Stacktrace: +// ... +// From: : (error: Previous error message.) +// ... +// From: : (error: New error message.) // #define XLA_RETURN_IF_ERROR(rexpr, ...) \ do { \ @@ -93,24 +100,20 @@ namespace torch_xla { // If the function call results in an ok status, execution continues with // `result` set to `ret.value()`, where `ret` is the returned value of the // function. Otherwise, we early return a non-ok status. Then, if -// `TORCH_SHOW_CPP_STACKTRACES` is set, the error shown will be: -// -// New error message. (at :) -// Previous error message. (at :) -// ... +// `TORCH_SHOW_CPP_STACKTRACES` is set, the error shown will be similar to +// the one above. // #define XLA_ASSIGN_OR_RETURN(lhs, rexpr, ...) \ XLA_RETURN_IF_ERROR_IMPL_(rexpr, XLA_STATUS_VAR_, \ lhs = std::move(XLA_STATUS_VAR_).value(), \ ##__VA_ARGS__) -// Maybe shows location information in the status message. +namespace status_internal { + +// Adds source location information to the status stacktrace if +// `TORCH_SHOW_CPP_STACKTRACES` is set. // // This function assumes that `status` is a non-ok status. -// -// If `TORCH_SHOW_CPP_STACKTRACES` is set, appends the current source -// location information to the status message. Otherwise, it simply returns -// `status`. absl::Status MaybeWithLocation(const absl::Status& status, const char* file, int32_t line); @@ -143,6 +146,8 @@ absl::Status MaybeWithNewMessage(const absl::Status& status, const char* file, int32_t line, std::string_view new_message = ""); +} // namespace status_internal + // Maybe throws an exception if `status` has a non-ok code. // // Ideally, this function should be used only used in the project's From 42e8b3db17fc8aebed3f1d66f55831b2329fa682 Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Wed, 23 Jul 2025 13:41:15 -0300 Subject: [PATCH 2/6] Address reviews. --- test/cpp/test_status_common.h | 85 ++++++++++++++++++----------------- torch_xla/csrc/status.cpp | 52 +++++++++++---------- torch_xla/csrc/status.h | 34 +++++++++++--- 3 files changed, 102 insertions(+), 69 deletions(-) diff --git a/test/cpp/test_status_common.h b/test/cpp/test_status_common.h index fceabdf8395..7b08baa5b5c 100644 --- a/test/cpp/test_status_common.h +++ b/test/cpp/test_status_common.h @@ -32,7 +32,7 @@ namespace torch_xla { -// Enum to control whether C++ error context is shown in status kMessages +// Enum to control whether C++ error context is shown in status messages. enum class CppStacktracesMode { kShow, kHide, @@ -82,14 +82,14 @@ constexpr inline char kFile[] = "test_file.cpp"; constexpr inline char kEntryPrefix[] = "\n "; constexpr inline int32_t kLine = 42; -inline std::optional GetStacktrace(const absl::Status& status) { +inline std::string GetStatusPropagationTrace(const absl::Status& status) { if (status.ok()) { - return std::nullopt; - } else { - auto payload = status.GetPayload(kStacktraceKey); - return payload.has_value() ? std::optional(std::string(payload->Flatten())) - : std::nullopt; + return ""; } + auto status_propagation_trace = status.GetPayload(kStatusPropagationTraceKey); + return status_propagation_trace.has_value() + ? std::string(status_propagation_trace->Flatten()) + : ""; } TEST_P(StatusTest, MaybeThrowWithOkStatus) { @@ -98,18 +98,18 @@ TEST_P(StatusTest, MaybeThrowWithOkStatus) { } TEST_P(StatusTest, MaybeThrowWithErrorStatus) { - auto excfn = [=]() { + auto throw_exception = [=]() { absl::Status error_status = absl::InvalidArgumentError(kMessage); MaybeThrow(error_status); }; if (IsShowCppStacktracesMode()) { std::string expected_prefix = absl::StrCat(kMessage, "\n\nC++ Stacktrace:\n"); - EXPECT_THAT(excfn, ::testing::ThrowsMessage( - ::testing::StartsWith(expected_prefix))); + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::StartsWith(expected_prefix))); } else { - EXPECT_THAT(excfn, ::testing::ThrowsMessage( - ::testing::Eq(kMessage))); + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::Eq(kMessage))); } } @@ -129,12 +129,14 @@ TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { absl::Status error_status = absl::InvalidArgumentError(kMessage); absl::Status result = status_internal::MaybeWithLocation(error_status, kFile, kLine); + + ASSERT_FALSE(result.ok()); + EXPECT_EQ(result.code(), error_status.code()); + EXPECT_EQ(result.message(), error_status.message()); + if (IsShowCppStacktracesMode()) { - ASSERT_NE(result, error_status); - EXPECT_FALSE(result.ok()); - EXPECT_EQ(result.code(), error_status.code()); - EXPECT_EQ(result.message(), error_status.message()); - EXPECT_EQ(GetStacktrace(result), + EXPECT_NE(result, error_status); + EXPECT_EQ(GetStatusPropagationTrace(result), absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine, " (error: ", kMessage, ")")); } else { @@ -146,14 +148,17 @@ TEST_P(StatusTest, MaybeWithNewMessageEmptyNewMessage) { absl::Status error_status = absl::InvalidArgumentError(kMessage); absl::Status result = status_internal::MaybeWithNewMessage(error_status, kFile, kLine); + + ASSERT_FALSE(result.ok()); + EXPECT_EQ(result.code(), error_status.code()); + EXPECT_EQ(result.message(), error_status.message()); + if (IsShowCppStacktracesMode()) { - ASSERT_NE(result, error_status); - EXPECT_EQ(result.code(), error_status.code()); - EXPECT_EQ(result.message(), error_status.message()); - EXPECT_EQ(GetStacktrace(result), + EXPECT_NE(result, error_status); + EXPECT_EQ(GetStatusPropagationTrace(result), absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine)); } else { - ASSERT_EQ(result, error_status); + EXPECT_EQ(result, error_status); } } @@ -162,16 +167,15 @@ TEST_P(StatusTest, MaybeWithNewMessageNonEmptyNewMessage) { absl::Status result = status_internal::MaybeWithNewMessage( error_status, kFile, kLine, kNewMessage); - ASSERT_NE(result, error_status); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); EXPECT_EQ(result.message(), std::string_view(kNewMessage)); + EXPECT_NE(result, error_status); if (IsShowCppStacktracesMode()) { - auto stacktrace = GetStacktrace(result); - ASSERT_TRUE(stacktrace.has_value()); - EXPECT_EQ(*stacktrace, absl::StrCat(kEntryPrefix, "From: ", kFile, ":", - kLine, " (error: ", kNewMessage, ")")); + EXPECT_EQ(GetStatusPropagationTrace(result), + absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine, + " (error: ", kNewMessage, ")")); } } @@ -226,11 +230,12 @@ TEST_P(StatusTest, MacroReturnIfErrorWithNestedError) { EXPECT_EQ(result.message(), std::string_view(kMessage)); if (IsShowCppStacktracesMode()) { - auto stack0 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline0, + auto frame0 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline0, " (error: ", kMessage, ")"); - auto stack1 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline1); - auto stack2 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline2); - EXPECT_EQ(GetStacktrace(result), absl::StrCat(stack0, stack1, stack2)); + auto frame1 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline1); + auto frame2 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline2); + EXPECT_EQ(GetStatusPropagationTrace(result), + absl::StrCat(frame0, frame1, frame2)); } } @@ -248,7 +253,7 @@ TEST_P(StatusTest, MacroReturnIfErrorWithErrorWithNewMessage) { EXPECT_EQ(result.message(), std::string_view(kNewMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(GetStacktrace(result), + EXPECT_EQ(GetStatusPropagationTrace(result), absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, " (error: ", kNewMessage, ")")); } @@ -296,7 +301,7 @@ TEST_P(StatusTest, MacroAssignOrReturnWithErrorWithNewMessage) { EXPECT_EQ(result.status().message(), std::string_view(kNewMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(GetStacktrace(result.status()), + EXPECT_EQ(GetStatusPropagationTrace(result.status()), absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, " (error: ", kNewMessage, ")")); } @@ -310,7 +315,7 @@ TEST_P(StatusTest, MacroErrorWithLocation) { EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(result.message(), std::string_view(kMessage)); if (IsShowCppStacktracesMode()) { - EXPECT_EQ(GetStacktrace(result), + EXPECT_EQ(GetStatusPropagationTrace(result), absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, " (error: ", kMessage, ")")); } @@ -334,7 +339,7 @@ TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { return absl::OkStatus(); }; - auto excfn = [&]() { MaybeThrow(outerfn()); }; + auto throw_exception = [&]() { MaybeThrow(outerfn()); }; if (IsShowCppStacktracesMode()) { // Expected Error Message Prefix @@ -350,17 +355,17 @@ TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { // C++ Stacktrace: // std::string expected_prefix = absl::StrCat( - kNewMessage, "\n\nStatus Propagation Stacktrace:", kEntryPrefix, + kNewMessage, "\n\nStatus Propagation Trace:", kEntryPrefix, "From: ", __FILE__, ":", errline0, " (error: ", kMessage, ")", kEntryPrefix, "From: ", __FILE__, ":", errline1, " (error: ", kNewMessage, ")", kEntryPrefix, "From: ", __FILE__, ":", errline2, "\n\nC++ Stacktrace:\n"); - EXPECT_THAT(excfn, ::testing::ThrowsMessage( - ::testing::StartsWith(expected_prefix))); + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::StartsWith(expected_prefix))); } else { - EXPECT_THAT(excfn, ::testing::ThrowsMessage( - ::testing::Eq(kNewMessage))); + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::Eq(kNewMessage))); } } diff --git a/torch_xla/csrc/status.cpp b/torch_xla/csrc/status.cpp index d5338215a8a..0b52409fb6a 100644 --- a/torch_xla/csrc/status.cpp +++ b/torch_xla/csrc/status.cpp @@ -11,28 +11,29 @@ namespace torch_xla { -// Indent the stacktrace so that it's easier to see. -constexpr char kEntryPrefix[] = "\n "; +// Indent the stack frame representation so that it's easier to see. +constexpr char kFramePrefix[] = "\n "; -// Creates an error propagation stacktrace entry. +// Creates the stack frame representation for the status propagation trace +// entry. // -// The resulting entry will be appended to the existing stacktrace of the status -// currently being processed. +// The resulting string will be appended to the existing status propagation +// trace of the status currently being processed. // // Example: -// From: : [(error: )] +// \n From: : [(error: )] // -static std::string GetStacktraceEntry(const char* file, const int32_t line, - const std::string_view new_message) { +static std::string GetStackFrame(const char* file, const int32_t line, + const std::string_view new_message) { auto error_suffix = new_message.empty() ? "" : absl::StrCat(" (error: ", new_message, ")"); return absl::StrCat(kEntryPrefix, "From: ", file, ":", line, error_suffix); } -// Convenient function that retrieves the stacktrace payload if it exists. -// Otherwise, returns an empty absl::Cord. -static absl::Cord GetStacktraceOrEmptyCord(const absl::Status& status) { - auto opt = status.GetPayload(kStacktraceKey); +// Convenient function that retrieves the status propagation trace payload +// if it exists. Otherwise, returns an empty absl::Cord. +static absl::Cord GetStatusPropagationTraceOrEmpty(const absl::Status& status) { + auto opt = status.GetPayload(kStatusPropagationTraceKey); return opt.has_value() ? *opt : absl::Cord(); } @@ -47,11 +48,11 @@ absl::Status status_internal::MaybeWithLocation(const absl::Status& status, } // Make sure this is only called on fresh `status` instances. - ABSL_CHECK(GetStacktraceOrEmptyCord(status).empty()); + ABSL_CHECK(GetStatusPropagationTraceOrEmpty(status).empty()); // Adding source location to `status` has the same semantics as overwriting // the status message: - // 1. An stacktrace entry will be added + // 1. An stack frame will be added to the status propagation trace // 2. The status' message will be the same return MaybeWithNewMessage(status, file, line, status.message()); } @@ -76,12 +77,15 @@ absl::Status status_internal::MaybeWithNewMessage( status.code(), new_message.empty() ? status.message() : new_message); // If `TORCH_SHOW_CPP_STACKTRACES` is set: - // 1. append the current source location to the stacktrace payload + // + // 1. append the current stack frame to the status propagation trace + // payload + // // 2. append the new error message, if not empty if (torch::get_cpp_stacktraces_enabled()) { - auto new_stacktrace = GetStacktraceOrEmptyCord(status); - new_stacktrace.Append(GetStacktraceEntry(file, line, new_message)); - new_status.SetPayload(kStacktraceKey, new_stacktrace); + auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); + status_propagation_trace.Append(GetStackFrame(file, line, new_message)); + new_status.SetPayload(kStatusPropagationTraceKey, status_propagation_trace); } return new_status; @@ -89,18 +93,18 @@ absl::Status status_internal::MaybeWithNewMessage( void MaybeThrow(const absl::Status& status) { if (!status.ok()) { - auto status_stacktrace = GetStacktraceOrEmptyCord(status); - auto status_stacktrace_str = - status_stacktrace.empty() + auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); + auto status_propagation_trace_str = + status_propagation_trace.empty() ? "" - : absl::StrCat("\n\nStatus Propagation Stacktrace:", - status_stacktrace.Flatten()); + : absl::StrCat("\n\nStatus Propagation Trace:", + status_propagation_trace.Flatten()); auto cpp_stacktrace_str = torch::get_cpp_stacktraces_enabled() ? absl::StrCat("\n\nC++ Stacktrace:\n", tsl::CurrentStackTrace()) : ""; throw std::runtime_error(absl::StrCat( - status.message(), status_stacktrace_str, cpp_stacktrace_str)); + status.message(), status_propagation_trace_str, cpp_stacktrace_str)); } } diff --git a/torch_xla/csrc/status.h b/torch_xla/csrc/status.h index 2bb3570608b..9cef353416a 100644 --- a/torch_xla/csrc/status.h +++ b/torch_xla/csrc/status.h @@ -14,7 +14,22 @@ namespace torch_xla { -constexpr char kStacktraceKey[] = "stacktrace"; +// `type_url` for retrieving the status propagation trace payload of a given +// status. +// +// The payload is composed of multiple lines, where each line represents a stack +// frame in the status propagation trace. Each line is in the following format: +// +// \n From: :[ErrorSuffix] +// | ---- | +// | | |_ error message produced in that source +// | | location (it might be overwritten later). +// | | +// | |_ leading 4 spaces for improved readability. +// | +// |_ start with a line break. +// +constexpr char kStatusPropagationTraceKey[] = "status-propagation-trace"; // If `TORCH_SHOW_CPP_STACKTRACES` is set, creates a new Status instance, // appending the current location (e.g. file and line information) to the @@ -110,10 +125,17 @@ constexpr char kStacktraceKey[] = "stacktrace"; namespace status_internal { -// Adds source location information to the status stacktrace if +// Adds source location information to the status propagation trace if // `TORCH_SHOW_CPP_STACKTRACES` is set. // -// This function assumes that `status` is a non-ok status. +// This function assumes that: +// +// 1. `status` is a non-ok status. +// 2. `status` doesn't have a status propagation trace payload +// +// If any of the above assumptions is false, this function crashes the +// whole program. +// absl::Status MaybeWithLocation(const absl::Status& status, const char* file, int32_t line); @@ -129,7 +151,8 @@ const absl::Status& GetStatus(const absl::StatusOr& status) { return status.status(); } -// Maybe replace the current `status` message with `new_message`. +// Maybe replace the current `status` message with `new_message`, and also +// add source location information if enabled. // // This function assumes that `status` is a non-ok status. // @@ -140,7 +163,8 @@ const absl::Status& GetStatus(const absl::StatusOr& status) { // Rationale: if given, `new_message` has more context, which makes it possible // to construct better error messages to the user. // -// This function also appends file location information to the error message, if +// This function also appends the source location information to the status +// propagation trace payload (creates a new one if needed), if // `TORCH_SHOW_CPP_STACKTRACES` is set. absl::Status MaybeWithNewMessage(const absl::Status& status, const char* file, int32_t line, From da72b575ea1826ef8abba8043c3909fa39e1aeea Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Wed, 23 Jul 2025 13:41:41 -0300 Subject: [PATCH 3/6] Add `__FUNCTION__` to the status propagation trace. --- test/cpp/test_status_common.h | 50 +++++++++++++++++++---------------- torch_xla/csrc/status.cpp | 15 ++++++----- torch_xla/csrc/status.h | 11 ++++---- 3 files changed, 42 insertions(+), 34 deletions(-) diff --git a/test/cpp/test_status_common.h b/test/cpp/test_status_common.h index 7b08baa5b5c..c568b4edb67 100644 --- a/test/cpp/test_status_common.h +++ b/test/cpp/test_status_common.h @@ -79,6 +79,7 @@ namespace testing { constexpr inline char kNewMessage[] = "New test error message"; constexpr inline char kMessage[] = "Test error message"; constexpr inline char kFile[] = "test_file.cpp"; +constexpr inline char kFunction[] = "foo"; constexpr inline char kEntryPrefix[] = "\n "; constexpr inline int32_t kLine = 42; @@ -128,7 +129,7 @@ TEST_P(StatusTest, GetValueOrThrowWithErrorStatusOr) { TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { absl::Status error_status = absl::InvalidArgumentError(kMessage); absl::Status result = - status_internal::MaybeWithLocation(error_status, kFile, kLine); + status_internal::MaybeWithLocation(error_status, kFile, kLine, kFunction); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); @@ -137,8 +138,8 @@ TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { if (IsShowCppStacktracesMode()) { EXPECT_NE(result, error_status); EXPECT_EQ(GetStatusPropagationTrace(result), - absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine, - " (error: ", kMessage, ")")); + absl::StrCat(kEntryPrefix, "From: ", kFunction, " at ", kFile, + ":", kLine, " (error: ", kMessage, ")")); } else { EXPECT_EQ(result, error_status); } @@ -146,8 +147,8 @@ TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { TEST_P(StatusTest, MaybeWithNewMessageEmptyNewMessage) { absl::Status error_status = absl::InvalidArgumentError(kMessage); - absl::Status result = - status_internal::MaybeWithNewMessage(error_status, kFile, kLine); + absl::Status result = status_internal::MaybeWithNewMessage( + error_status, kFile, kLine, kFunction); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); @@ -156,7 +157,8 @@ TEST_P(StatusTest, MaybeWithNewMessageEmptyNewMessage) { if (IsShowCppStacktracesMode()) { EXPECT_NE(result, error_status); EXPECT_EQ(GetStatusPropagationTrace(result), - absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine)); + absl::StrCat(kEntryPrefix, "From: ", kFunction, " at ", kFile, + ":", kLine)); } else { EXPECT_EQ(result, error_status); } @@ -165,7 +167,7 @@ TEST_P(StatusTest, MaybeWithNewMessageEmptyNewMessage) { TEST_P(StatusTest, MaybeWithNewMessageNonEmptyNewMessage) { absl::Status error_status = absl::InvalidArgumentError(kMessage); absl::Status result = status_internal::MaybeWithNewMessage( - error_status, kFile, kLine, kNewMessage); + error_status, kFile, kLine, kFunction, kNewMessage); ASSERT_FALSE(result.ok()); EXPECT_EQ(result.code(), error_status.code()); @@ -174,8 +176,8 @@ TEST_P(StatusTest, MaybeWithNewMessageNonEmptyNewMessage) { if (IsShowCppStacktracesMode()) { EXPECT_EQ(GetStatusPropagationTrace(result), - absl::StrCat(kEntryPrefix, "From: ", kFile, ":", kLine, - " (error: ", kNewMessage, ")")); + absl::StrCat(kEntryPrefix, "From: ", kFunction, " at ", kFile, + ":", kLine, " (error: ", kNewMessage, ")")); } } @@ -230,10 +232,12 @@ TEST_P(StatusTest, MacroReturnIfErrorWithNestedError) { EXPECT_EQ(result.message(), std::string_view(kMessage)); if (IsShowCppStacktracesMode()) { - auto frame0 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline0, - " (error: ", kMessage, ")"); - auto frame1 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline1); - auto frame2 = absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline2); + auto frame0 = absl::StrCat(kEntryPrefix, "From: operator() at ", __FILE__, + ":", errline0, " (error: ", kMessage, ")"); + auto frame1 = absl::StrCat(kEntryPrefix, "From: operator() at ", __FILE__, + ":", errline1); + auto frame2 = absl::StrCat(kEntryPrefix, "From: operator() at ", __FILE__, + ":", errline2); EXPECT_EQ(GetStatusPropagationTrace(result), absl::StrCat(frame0, frame1, frame2)); } @@ -254,8 +258,8 @@ TEST_P(StatusTest, MacroReturnIfErrorWithErrorWithNewMessage) { if (IsShowCppStacktracesMode()) { EXPECT_EQ(GetStatusPropagationTrace(result), - absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, - " (error: ", kNewMessage, ")")); + absl::StrCat(kEntryPrefix, "From: operator() at ", __FILE__, ":", + errline, " (error: ", kNewMessage, ")")); } } @@ -302,8 +306,8 @@ TEST_P(StatusTest, MacroAssignOrReturnWithErrorWithNewMessage) { if (IsShowCppStacktracesMode()) { EXPECT_EQ(GetStatusPropagationTrace(result.status()), - absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, - " (error: ", kNewMessage, ")")); + absl::StrCat(kEntryPrefix, "From: operator() at ", __FILE__, ":", + errline, " (error: ", kNewMessage, ")")); } } @@ -316,8 +320,8 @@ TEST_P(StatusTest, MacroErrorWithLocation) { EXPECT_EQ(result.message(), std::string_view(kMessage)); if (IsShowCppStacktracesMode()) { EXPECT_EQ(GetStatusPropagationTrace(result), - absl::StrCat(kEntryPrefix, "From: ", __FILE__, ":", errline, - " (error: ", kMessage, ")")); + absl::StrCat(kEntryPrefix, "From: ", __FUNCTION__, " at ", + __FILE__, ":", errline, " (error: ", kMessage, ")")); } } @@ -356,10 +360,10 @@ TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { // std::string expected_prefix = absl::StrCat( kNewMessage, "\n\nStatus Propagation Trace:", kEntryPrefix, - "From: ", __FILE__, ":", errline0, " (error: ", kMessage, ")", - kEntryPrefix, "From: ", __FILE__, ":", errline1, - " (error: ", kNewMessage, ")", kEntryPrefix, "From: ", __FILE__, ":", - errline2, "\n\nC++ Stacktrace:\n"); + "From: operator() at ", __FILE__, ":", errline0, " (error: ", kMessage, + ")", kEntryPrefix, "From: operator() at ", __FILE__, ":", errline1, + " (error: ", kNewMessage, ")", kEntryPrefix, "From: operator() at ", + __FILE__, ":", errline2, "\n\nC++ Stacktrace:\n"); EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( ::testing::StartsWith(expected_prefix))); diff --git a/torch_xla/csrc/status.cpp b/torch_xla/csrc/status.cpp index 0b52409fb6a..2006277ba39 100644 --- a/torch_xla/csrc/status.cpp +++ b/torch_xla/csrc/status.cpp @@ -21,13 +21,15 @@ constexpr char kFramePrefix[] = "\n "; // trace of the status currently being processed. // // Example: -// \n From: : [(error: )] +// \n From: at : [(error: )] // static std::string GetStackFrame(const char* file, const int32_t line, + const char* function, const std::string_view new_message) { auto error_suffix = new_message.empty() ? "" : absl::StrCat(" (error: ", new_message, ")"); - return absl::StrCat(kEntryPrefix, "From: ", file, ":", line, error_suffix); + return absl::StrCat(kFramePrefix, "From: ", function, " at ", file, ":", line, + error_suffix); } // Convenient function that retrieves the status propagation trace payload @@ -39,7 +41,8 @@ static absl::Cord GetStatusPropagationTraceOrEmpty(const absl::Status& status) { absl::Status status_internal::MaybeWithLocation(const absl::Status& status, const char* file, - const int32_t line) { + const int32_t line, + const char* function) { ABSL_CHECK(!status.ok()); // Return the same status if we don't need to add the C++ source location. @@ -54,12 +57,12 @@ absl::Status status_internal::MaybeWithLocation(const absl::Status& status, // the status message: // 1. An stack frame will be added to the status propagation trace // 2. The status' message will be the same - return MaybeWithNewMessage(status, file, line, status.message()); + return MaybeWithNewMessage(status, file, line, function, status.message()); } absl::Status status_internal::MaybeWithNewMessage( const absl::Status& status, const char* file, const int32_t line, - const std::string_view new_message) { + const char* function, const std::string_view new_message) { ABSL_CHECK(!status.ok()); // Return the same status if: @@ -84,7 +87,7 @@ absl::Status status_internal::MaybeWithNewMessage( // 2. append the new error message, if not empty if (torch::get_cpp_stacktraces_enabled()) { auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); - status_propagation_trace.Append(GetStackFrame(file, line, new_message)); + status_propagation_trace.Append(GetStackFrame(file, line, function, new_message)); new_status.SetPayload(kStatusPropagationTraceKey, status_propagation_trace); } diff --git a/torch_xla/csrc/status.h b/torch_xla/csrc/status.h index 9cef353416a..c86c6c70af4 100644 --- a/torch_xla/csrc/status.h +++ b/torch_xla/csrc/status.h @@ -48,8 +48,9 @@ constexpr char kStatusPropagationTraceKey[] = "status-propagation-trace"; // RuntimeError: Error message. // From: : (error: Error message.) // -#define XLA_ERROR_WITH_LOCATION(status) \ - ::torch_xla::status_internal::MaybeWithLocation(status, __FILE__, __LINE__) +#define XLA_ERROR_WITH_LOCATION(status) \ + ::torch_xla::status_internal::MaybeWithLocation(status, __FILE__, __LINE__, \ + __FUNCTION__) #define XLA_CONCAT_(a, b) XLA_CONCAT_IMPL_(a, b) #define XLA_CONCAT_IMPL_(a, b) a##b @@ -66,7 +67,7 @@ constexpr char kStatusPropagationTraceKey[] = "status-propagation-trace"; if (!var.ok()) { \ return ::torch_xla::status_internal::MaybeWithNewMessage( \ ::torch_xla::status_internal::GetStatus(var), __FILE__, __LINE__, \ - ##__VA_ARGS__); \ + __FUNCTION__, ##__VA_ARGS__); \ } \ then @@ -137,7 +138,7 @@ namespace status_internal { // whole program. // absl::Status MaybeWithLocation(const absl::Status& status, const char* file, - int32_t line); + int32_t line, const char* function); // Returns an `absl::Status` from an `absl::Status`. // In this case, this function is a no-op. It simply returns the argument. @@ -167,7 +168,7 @@ const absl::Status& GetStatus(const absl::StatusOr& status) { // propagation trace payload (creates a new one if needed), if // `TORCH_SHOW_CPP_STACKTRACES` is set. absl::Status MaybeWithNewMessage(const absl::Status& status, const char* file, - int32_t line, + int32_t line, const char* function, std::string_view new_message = ""); } // namespace status_internal From 9d82e72b56e6f3d5ce6842aa3bcd3adac7354046 Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Wed, 23 Jul 2025 13:51:29 -0300 Subject: [PATCH 4/6] Fix lint. --- torch_xla/csrc/status.cpp | 3 ++- torch_xla/csrc/status.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/torch_xla/csrc/status.cpp b/torch_xla/csrc/status.cpp index 2006277ba39..b8a97deda46 100644 --- a/torch_xla/csrc/status.cpp +++ b/torch_xla/csrc/status.cpp @@ -87,7 +87,8 @@ absl::Status status_internal::MaybeWithNewMessage( // 2. append the new error message, if not empty if (torch::get_cpp_stacktraces_enabled()) { auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); - status_propagation_trace.Append(GetStackFrame(file, line, function, new_message)); + status_propagation_trace.Append( + GetStackFrame(file, line, function, new_message)); new_status.SetPayload(kStatusPropagationTraceKey, status_propagation_trace); } diff --git a/torch_xla/csrc/status.h b/torch_xla/csrc/status.h index c86c6c70af4..d9e786a3311 100644 --- a/torch_xla/csrc/status.h +++ b/torch_xla/csrc/status.h @@ -132,7 +132,7 @@ namespace status_internal { // This function assumes that: // // 1. `status` is a non-ok status. -// 2. `status` doesn't have a status propagation trace payload +// 2. `status` doesn't have a status propagation trace payload // // If any of the above assumptions is false, this function crashes the // whole program. From e0427a2508104c470c5e024a6e8573dcd359f10c Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Mon, 28 Jul 2025 22:20:17 -0300 Subject: [PATCH 5/6] Use `TORCH_CHECK` for raising errors + dumping the stacktrace. --- test/cpp/test_status_common.h | 56 ++++++++++++++++++++++++++++++----- torch_xla/csrc/status.cpp | 40 ++++++++++++++++--------- 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/test/cpp/test_status_common.h b/test/cpp/test_status_common.h index c568b4edb67..4d4b173f643 100644 --- a/test/cpp/test_status_common.h +++ b/test/cpp/test_status_common.h @@ -18,6 +18,7 @@ #ifndef XLA_TEST_CPP_TEST_STATUS_COMMON_H_ #define XLA_TEST_CPP_TEST_STATUS_COMMON_H_ +#include #include #include @@ -83,6 +84,29 @@ constexpr inline char kFunction[] = "foo"; constexpr inline char kEntryPrefix[] = "\n "; constexpr inline int32_t kLine = 42; +// The PyTorch C++ stacktrace is ALWAYS appended to the error message. +// More specifically, when `what()` function is called. +// +// However, it's only when the raised `c10::Error` gets translated to a +// Python exception that PyTorch checks the value of the +// `TORCH_SHOW_CPP_STACKTRACES` environment variable, which actually +// controls whether the stacktrace will get shown or not by calling +// `what_without_backtraces()`, instead. +// +// Therefore, we need to mimic this behavior. +#define THROW_RUNTIME_ERROR_FROM_C10_ERROR(block) \ + try { \ + block; \ + } catch (const c10::Error& error) { \ + throw std::runtime_error(IsShowCppStacktracesMode() \ + ? error.what() \ + : error.what_without_backtrace()); \ + } + +// Prefix of the C++ stacktrace PyTorch adds to the error message. +constexpr inline char kTorchCppStacktracePrefix[] = + "Exception raised from MaybeThrow at torch_xla/csrc/status.cpp:"; + inline std::string GetStatusPropagationTrace(const absl::Status& status) { if (status.ok()) { return ""; @@ -100,12 +124,15 @@ TEST_P(StatusTest, MaybeThrowWithOkStatus) { TEST_P(StatusTest, MaybeThrowWithErrorStatus) { auto throw_exception = [=]() { - absl::Status error_status = absl::InvalidArgumentError(kMessage); - MaybeThrow(error_status); + THROW_RUNTIME_ERROR_FROM_C10_ERROR({ + absl::Status error_status = absl::InvalidArgumentError(kMessage); + MaybeThrow(error_status); + }); }; + if (IsShowCppStacktracesMode()) { std::string expected_prefix = - absl::StrCat(kMessage, "\n\nC++ Stacktrace:\n"); + absl::StrCat(kMessage, "\n\n", kTorchCppStacktracePrefix); EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( ::testing::StartsWith(expected_prefix))); } else { @@ -122,8 +149,21 @@ TEST_P(StatusTest, GetValueOrThrowWithOkStatusOr) { } TEST_P(StatusTest, GetValueOrThrowWithErrorStatusOr) { - absl::StatusOr status_or = absl::InvalidArgumentError(kMessage); - EXPECT_THROW(GetValueOrThrow(std::move(status_or)), std::runtime_error); + auto throw_exception = [=]() { + THROW_RUNTIME_ERROR_FROM_C10_ERROR({ + absl::StatusOr error_status = absl::InvalidArgumentError(kMessage); + int value = GetValueOrThrow(error_status); + }); + }; + if (IsShowCppStacktracesMode()) { + std::string expected_prefix = + absl::StrCat(kMessage, "\n\n", kTorchCppStacktracePrefix); + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::StartsWith(expected_prefix))); + } else { + EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( + ::testing::Eq(kMessage))); + } } TEST_P(StatusTest, MaybeWithLocationPropagatesErrorStatus) { @@ -343,7 +383,9 @@ TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { return absl::OkStatus(); }; - auto throw_exception = [&]() { MaybeThrow(outerfn()); }; + auto throw_exception = [&]() { + THROW_RUNTIME_ERROR_FROM_C10_ERROR(MaybeThrow(outerfn())); + }; if (IsShowCppStacktracesMode()) { // Expected Error Message Prefix @@ -363,7 +405,7 @@ TEST_P(StatusTest, MaybeThrowWithErrorPropagationWithNewMessage) { "From: operator() at ", __FILE__, ":", errline0, " (error: ", kMessage, ")", kEntryPrefix, "From: operator() at ", __FILE__, ":", errline1, " (error: ", kNewMessage, ")", kEntryPrefix, "From: operator() at ", - __FILE__, ":", errline2, "\n\nC++ Stacktrace:\n"); + __FILE__, ":", errline2, "\n\n", kTorchCppStacktracePrefix); EXPECT_THAT(throw_exception, ::testing::ThrowsMessage( ::testing::StartsWith(expected_prefix))); diff --git a/torch_xla/csrc/status.cpp b/torch_xla/csrc/status.cpp index b8a97deda46..270f3487867 100644 --- a/torch_xla/csrc/status.cpp +++ b/torch_xla/csrc/status.cpp @@ -1,5 +1,6 @@ #include "torch_xla/csrc/status.h" +#include #include #include @@ -95,21 +96,32 @@ absl::Status status_internal::MaybeWithNewMessage( return new_status; } +// Get a formatted string representation of the status propagation trace +// if it's not empty. +static std::string GetFormattedStatusPropagationTrace( + const absl::Status& status) { + auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); + return status_propagation_trace.empty() + ? "" + : absl::StrCat("\nStatus Propagation Trace:", + status_propagation_trace.Flatten(), "\n"); +} + +// Get the status message followed by a line break, if we are printing the +// C++ stacktraces. +// +// This is needed so we have a blank line in between the status message and +// the dumped C++ traces (either the status propagation one, or the C++ +// stacktrace). +static std::string MaybeGetMessageWithLineBreak(const absl::Status& status) { + return torch::get_cpp_stacktraces_enabled() + ? absl::StrCat(status.message(), "\n") + : std::string(status.message()); +} + void MaybeThrow(const absl::Status& status) { - if (!status.ok()) { - auto status_propagation_trace = GetStatusPropagationTraceOrEmpty(status); - auto status_propagation_trace_str = - status_propagation_trace.empty() - ? "" - : absl::StrCat("\n\nStatus Propagation Trace:", - status_propagation_trace.Flatten()); - auto cpp_stacktrace_str = - torch::get_cpp_stacktraces_enabled() - ? absl::StrCat("\n\nC++ Stacktrace:\n", tsl::CurrentStackTrace()) - : ""; - throw std::runtime_error(absl::StrCat( - status.message(), status_propagation_trace_str, cpp_stacktrace_str)); - } + TORCH_CHECK(status.ok(), MaybeGetMessageWithLineBreak(status), + GetFormattedStatusPropagationTrace(status)); } } // namespace torch_xla From 9f5984ef7c2d7f97e13a242ab40041843841dd54 Mon Sep 17 00:00:00 2001 From: Yukio Siraichi Date: Tue, 29 Jul 2025 13:26:26 -0300 Subject: [PATCH 6/6] Change payload key. --- torch_xla/csrc/status.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/torch_xla/csrc/status.h b/torch_xla/csrc/status.h index d9e786a3311..2f53b37381f 100644 --- a/torch_xla/csrc/status.h +++ b/torch_xla/csrc/status.h @@ -29,7 +29,8 @@ namespace torch_xla { // | // |_ start with a line break. // -constexpr char kStatusPropagationTraceKey[] = "status-propagation-trace"; +constexpr char kStatusPropagationTraceKey[] = + "type.googleapis.com/torch_xla.status_trace"; // If `TORCH_SHOW_CPP_STACKTRACES` is set, creates a new Status instance, // appending the current location (e.g. file and line information) to the