Skip to content

Fix status source code location logic.#9440

Merged
ysiraichi merged 9 commits intomasterfrom
ysiraichi/fix-status-location-logic
Jul 10, 2025
Merged

Fix status source code location logic.#9440
ysiraichi merged 9 commits intomasterfrom
ysiraichi/fix-status-location-logic

Conversation

@ysiraichi
Copy link
Copy Markdown
Collaborator

@ysiraichi ysiraichi commented Jul 2, 2025

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)

Copy link
Copy Markdown
Collaborator

@zhanyong-wan zhanyong-wan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread test/cpp/test_status_dont_show_cpp_error_context.cpp Outdated
Comment thread torch_xla/csrc/status.cpp Outdated
Comment thread torch_xla/csrc/status.cpp Outdated
Comment thread torch_xla/csrc/status.cpp
Comment thread torch_xla/csrc/status.cpp Outdated
Comment thread test/cpp/test_status_dont_show_cpp_error_context.cpp Outdated
@ysiraichi ysiraichi requested a review from zhanyong-wan July 8, 2025 15:24
Copy link
Copy Markdown
Collaborator

@zhanyong-wan zhanyong-wan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread test/cpp/test_status_common.h
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/BUILD Outdated
@ysiraichi ysiraichi requested a review from zhanyong-wan July 9, 2025 13:15
Copy link
Copy Markdown
Collaborator

@zhanyong-wan zhanyong-wan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread test/cpp/test_status_common.h Outdated
Comment thread test/cpp/test_status_dont_show_cpp_error_context.cpp Outdated
Comment thread test/cpp/test_status_show_cpp_error_context.cpp Outdated
@ysiraichi ysiraichi force-pushed the ysiraichi/fix-status-location-logic branch from 22a420a to 0242960 Compare July 9, 2025 20:31
- 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
@ysiraichi ysiraichi force-pushed the ysiraichi/fix-status-location-logic branch from 0242960 to 620e4b6 Compare July 10, 2025 12:06
@ysiraichi ysiraichi merged commit 6acc8f3 into master Jul 10, 2025
39 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants