Fix status source code location logic. #9440

ysiraichi · 2025-07-02T17:57:09Z

This PR refines the handling of status source code locations and overhauls related tests. The key changes improve the accuracy of source information and streamline our testing approach:

Resolved an undesirable behavior where the source location of every status propagation point (macro usage) was appended to the error message, effectively creating an unwanted backtrace. Now, source location information is only appended when the message is explicitly overwritten with a more informative one, providing relevant context without clutter.
Added nested error propagation test MacroReturnIfErrorWithNestedError that exemplifies the undesirable behavior.
Removed test_status.cpp, copying its tests to the other 2 existing tests: test_status_show_cpp_error_context.cpp and test_status_dont_show_cpp_error_context.cpp. Rationale: the tests should pass with both configurations.
Added a reminder on each test file, so that whenever one test is added to one of them, it's also added to the other.

Undesired Behavior

Below, we can see the results of the newly added test MacroReturnIfErrorWithNestedError when XLA_SHOW_CPP_ERROR_CONTEXT=1:

Expected: error message with source code location
Actual: error message with soruce code location of all the places we've propagated the status

TEST(StatusWithErrorContextTest, MacroReturnIfErrorWithNestedError) {
  auto test_function = []() -> Status {
    Status error_status = absl::InvalidArgumentError(message);
    XLA_RETURN_IF_ERROR(error_status);
    return absl::OkStatus();
  };

  auto outer_test_function = [&]() -> Status {
    XLA_RETURN_IF_ERROR(test_function());
    return absl::OkStatus();
  };

  Status result = outer_test_function();
}

[ RUN      ] StatusWithErrorContextTest.MacroReturnIfErrorWithNestedError
test/cpp/test_status_show_cpp_error_context.cpp:132: Failure
Expected equality of these values:
  result.message()
    Which is: "Test error message (at test/cpp/test_status_show_cpp_error_context.cpp:116) (at test/cpp/test_status_show_cpp_error_context.cpp:120) (at test/cpp/test_status_show_cpp_error_context.cpp:125)"
  StrCat("Test error message (at ", "test/cpp/test_status_show_cpp_error_context.cpp", ":", errline, ")")
    Which is: "Test error message (at test/cpp/test_status_show_cpp_error_context.cpp:116)"

[  FAILED  ] StatusWithErrorContextTest.MacroReturnIfErrorWithNestedError (0 ms)

- Refactor status tests - Remove test_status.cpp as its tests are now covered by specialized context tests - Both specialized tests now cover all status utility functions and macros

zhanyong-wan

Thanks!

zhanyong-wan · 2025-07-07T15:04:33Z

test/cpp/test_status_dont_show_cpp_error_context.cpp

+// This file specifically tests behavior when XLA_SHOW_CPP_ERROR_CONTEXT is
+// set to "false".
+//
+// If you add or delete a test in this file, please make the corresponding


This leads to a lot of code duplication and maintenance overhead.

We should write the test cases once and share them between the two configurations.

zhanyong-wan · 2025-07-07T15:05:50Z

torch_xla/csrc/status.cpp

-      context = absl::StrCat(context, "\nFrom Error: ", old_message);
-    }
-  }
+  std::string location = LocationStrWithSpace(file, line);


Don't create this unless it's actually needed.

zhanyong-wan · 2025-07-07T15:06:07Z

torch_xla/csrc/status.cpp

-    }
-  }
+  std::string location = LocationStrWithSpace(file, line);
+  std::string context =


torch_xla/csrc/status.cpp

zhanyong-wan · 2025-07-07T15:07:43Z

torch_xla/csrc/status.cpp

+  std::string location = LocationStrWithSpace(file, line);
+  std::string context =
+      (ShouldShowCppErrorContext() && !new_message.empty())
+          ? std::string(absl::StrCat(location, "\nFrom Error: ", old_message))


Nit: std::string() call is unnecessary.

ghpvnist · 2025-07-07T20:30:28Z

test/cpp/test_status_dont_show_cpp_error_context.cpp

+// Any new `TEST_P` test cases added to `test_status_common.h` will automatically
+// be run in this mode (without C++ error context).
+//
+INSTANTIATE_TEST_SUITE_WITH_MODE(HIDE);


Do we need these files if all the tests are unified under test_status_common.h?

Yes. The problem is that we set the mode there by setting the XLA_SHOW_CPP_ERROR_CONTEXT environment variable, which gets cached the first time it's used in a local static variable. This means that even if we reset that environment variable to another value afterwards, it won't affect execution. So, we need to test them in different processes. I think that the easiest and most obvious way to accomplish that is by having separate files.

zhanyong-wan

Thanks!

test/cpp/test_status_common.h

zhanyong-wan · 2025-07-08T15:33:59Z

test/cpp/test_status_common.h

+
+// Enum to control whether C++ error context is shown in status messages
+enum class CppErrorContextMode {
+  SHOW,


Per https://google.github.io/styleguide/cppguide.html#Enumerator_Names, this should be kShow.

Would be great to get familiar with the Google C++ style guide.

zhanyong-wan · 2025-07-08T15:35:03Z

test/cpp/test_status_common.h

+
+// Base test class for parameterized status tests with C++ error context control
+class StatusTest : public testing::TestWithParam<CppErrorContextMode> {
+  void SetUp() override {


Style: prefer using the StatusTest default ctor here.

zhanyong-wan · 2025-07-08T15:36:15Z

test/cpp/test_status_common.h

+    setenv(runtime::env::kEnvShowCppErrorContext, value, /* replace= */ 1);
+  }
+
+ public:


Per the style guide: public section should be before protected or private section.

Actually, this doesn't need to be public. It can be protected.

zhanyong-wan · 2025-07-08T15:37:03Z

test/cpp/test_status_common.h

+
+// Macro to instantiate parameterized status tests with specific mode
+#define INSTANTIATE_TEST_SUITE_WITH_MODE(mode)                            \
+  using torch_xla::CppErrorContextMode;                                   \


This has the side effect of defining some type aliases. Avoid it. Just spell out the types in the macro body.

zhanyong-wan · 2025-07-08T15:38:04Z

test/cpp/test_status_common.h

+};
+
+// Macro to instantiate parameterized status tests with specific mode
+#define INSTANTIATE_TEST_SUITE_WITH_MODE(mode)                            \


This name is too general. Also the connection with StatusTest is unobvious at the call site.

I'd let the caller provide StatusTest as an argument to make the code more readable.

zhanyong-wan · 2025-07-08T15:39:42Z

test/cpp/test_status_common.h

+
+namespace {
+
+using absl::Status;


Style:

avoid using declaration at namespace in headers unless we intend to define a type alias for all users of the header. Since we don't intend code that includes this header to be able to write StatusOr without absl::, these using declarations should be removed.

zhanyong-wan · 2025-07-08T15:40:09Z

test/cpp/test_status_common.h

+using absl::StatusOr;
+using absl::StrCat;
+
+constexpr char new_message[] = "New test error message";


Global variables defined in a header should be inline.

zhanyong-wan · 2025-07-08T15:42:29Z

test/cpp/BUILD

-    srcs = ["test_status_dont_show_cpp_error_context.cpp"],
+    srcs = [
+        "test_status_dont_show_cpp_error_context.cpp",
+        "test_status_common.h",


Style: it's better to define a header-only library for test_status_common.h and add the library to both cc_tests' deps. This ensures that the header is self-contained.

zhanyong-wan

Thanks!

zhanyong-wan · 2025-07-09T15:33:35Z

test/cpp/test_status_common.h

+// is being called.
+#define INSTANTIATE_WITH_CPP_ERROR_CONTEXT_MODE(suite, test, mode)  \
+  INSTANTIATE_TEST_SUITE_P(                                         \
+      suite, test, testing::Values(CppErrorContextMode::mode),      \


Style: in a macro body, always fully qualify the symbols (use absolute namespaces like ::testing::), so that we don't have to worry about changing the macro meaning if it's used in the wrong namespace.

zhanyong-wan · 2025-07-09T15:35:05Z

test/cpp/test_status_dont_show_cpp_error_context.cpp

@@ -1,5 +1,8 @@
 #include "test/cpp/test_status_common.h"

+using torch_xla::CppErrorContextMode;


Style: please make the macro body fully qualify the references of these names so that we don't have to do this here.

Ah, yes. Sorry about that. Since I couldn't do it with StatusTest (i.e. ::torch_xla::StatusTest), I incorrectly assumed I couldn't do it for CppErrorContextMode. It should be fixed, now.

zhanyong-wan · 2025-07-09T15:35:19Z

test/cpp/test_status_show_cpp_error_context.cpp

@@ -1,10 +1,14 @@
 #include "test/cpp/test_status_common.h"

+using torch_xla::CppErrorContextMode;


ysiraichi mentioned this pull request Jul 2, 2025

Add WITH_LOCATION macros for propagating external library errors. #9441

Draft

ysiraichi added 3 commits July 2, 2025 15:27

Fix status source code location logic.

0c5b199

- Refactor status tests - Remove test_status.cpp as its tests are now covered by specialized context tests - Both specialized tests now cover all status utility functions and macros

Modify nested error test.

19409cd

Add default capture.

d6ba41f

ysiraichi force-pushed the ysiraichi/fix-status-location-logic branch from 0427c0a to d6ba41f Compare July 2, 2025 18:27

ysiraichi requested review from zhanyong-wan and ghpvnist July 4, 2025 16:26

zhanyong-wan requested changes Jul 7, 2025

View reviewed changes

Address reviews.

e6b104e

ghpvnist reviewed Jul 7, 2025

View reviewed changes

Fix lint issues.

706d7dc

ysiraichi requested a review from zhanyong-wan July 8, 2025 15:24

zhanyong-wan requested changes Jul 8, 2025

View reviewed changes

ysiraichi added 3 commits July 8, 2025 15:08

Address reviews.

44630fc

Fix lint issue.

4d4e247

Fix compilation err.

2d3a217

ysiraichi requested a review from zhanyong-wan July 9, 2025 13:15

zhanyong-wan approved these changes Jul 9, 2025

View reviewed changes

Fully-qualify identifiers where possible.

22a420a

		@@ -1,5 +1,8 @@
		#include "test/cpp/test_status_common.h"

		using torch_xla::CppErrorContextMode;

		@@ -1,10 +1,14 @@
		#include "test/cpp/test_status_common.h"

		using torch_xla::CppErrorContextMode;

Fix status source code location logic. #9440

Are you sure you want to change the base?

Fix status source code location logic. #9440

Conversation

ysiraichi commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Undesired Behavior

Uh oh!

zhanyong-wan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhanyong-wan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhanyong-wan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ysiraichi commented Jul 2, 2025 •

edited

Loading