diff --git a/xls/common/BUILD b/xls/common/BUILD index 6155f11c29..987a0c0094 100644 --- a/xls/common/BUILD +++ b/xls/common/BUILD @@ -199,9 +199,7 @@ cc_test( cc_library( name = "source_location", hdrs = ["source_location.h"], - deps = [ - "@com_google_absl//absl/base:config", - ], + deps = ["@com_google_absl//absl/base:config"], ) cc_test( diff --git a/xls/common/source_location.h b/xls/common/source_location.h index 2371173c0e..a208b2601d 100644 --- a/xls/common/source_location.h +++ b/xls/common/source_location.h @@ -33,6 +33,20 @@ #include "absl/base/config.h" +#ifdef XLS_USE_ABSL_SOURCE_LOCATION +#include "absl/types/source_location.h" + +// Use absl::SourceLocation +namespace xabsl { +using SourceLocation = absl::SourceLocation; +#define XABSL_LOC ABSL_LOC +#define XABSL_LOC_CURRENT_DEFAULT_ARG ABSL_LOC_CURRENT_DEFAULT_ARG +} // namespace xabsl + +#else + +// OSS Version of source_location + #define XABSL_HAVE_SOURCE_LOCATION_CURRENT 1 // The xabsl namespace has types that are anticipated to become available in @@ -52,14 +66,12 @@ class SourceLocation { public: // Avoid this constructor; it populates the object with dummy values. - constexpr SourceLocation() - : line_(0), - file_name_(nullptr) {} + constexpr SourceLocation() : line_(0), file_name_(nullptr) {} // Wrapper to invoke the private constructor below. This should only be used // by the `XABSL_LOC` macro, hence the name. static constexpr SourceLocation DoNotInvokeDirectly(std::uint_least32_t line, - const char* file_name) { + const char* file_name) { return SourceLocation(line, file_name); } @@ -109,8 +121,7 @@ class SourceLocation { // `file_name` must outlive all copies of the `xabsl::SourceLocation` object, // so in practice it should be a string literal. constexpr SourceLocation(std::uint_least32_t line, const char* file_name) - : line_(line), - file_name_(file_name) {} + : line_(line), file_name_(file_name) {} friend constexpr int UseUnused() { static_assert(SourceLocation(0, nullptr).unused_column_ == 0, @@ -150,5 +161,6 @@ class SourceLocation { #else #define XABSL_LOC_CURRENT_DEFAULT_ARG #endif +#endif // XLS_USE_ABSL_SOURCE_LOCATION #endif // XLS_COMMON_SOURCE_LOCATION_H_ diff --git a/xls/common/status/BUILD b/xls/common/status/BUILD index 0ad93458c0..2d6f840799 100644 --- a/xls/common/status/BUILD +++ b/xls/common/status/BUILD @@ -77,7 +77,10 @@ cc_test( cc_library( name = "status_builder", - srcs = ["status_builder.cc"], + srcs = [ + "status_builder.cc", + "status_builder_oss.cc", + ], hdrs = ["status_builder.h"], deps = [ "@com_google_absl//absl/base:log_severity", diff --git a/xls/common/status/status_builder.h b/xls/common/status/status_builder.h index 1c2574a7fc..a757c5d7f6 100644 --- a/xls/common/status/status_builder.h +++ b/xls/common/status/status_builder.h @@ -15,6 +15,7 @@ #ifndef XLS_COMMON_STATUS_STATUS_BUILDER_H_ #define XLS_COMMON_STATUS_STATUS_BUILDER_H_ +#include #include #include #include @@ -142,10 +143,8 @@ class ABSL_MUST_USE_RESULT StatusBuilder { // logging mutator functions. Returns `*this` to allow method chaining. // If period is absl::ZeroDuration() or less, then this is equivalent to // calling the Log() method. - StatusBuilder& LogEvery(absl::LogSeverity level, - absl::Duration period) &; - StatusBuilder&& LogEvery(absl::LogSeverity level, - absl::Duration period) &&; + StatusBuilder& LogEvery(absl::LogSeverity level, absl::Duration period) &; + StatusBuilder&& LogEvery(absl::LogSeverity level, absl::Duration period) &&; // Mutates the builder so that the result status will be VLOGged (without a // stack trace) when this builder is converted to a Status. `verbose_level` @@ -268,13 +267,13 @@ class ABSL_MUST_USE_RESULT StatusBuilder { // Style guide exception for Ref qualified methods and rvalue refs. This // allows us to avoid a copy in the common case. template - auto With(Adaptor&& adaptor) & -> decltype( - std::forward(adaptor)(*this)) { + auto With( + Adaptor&& adaptor) & -> decltype(std::forward(adaptor)(*this)) { return std::forward(adaptor)(*this); } template - auto With(Adaptor&& adaptor) && -> decltype( - std::forward(adaptor)(std::move(*this))) { + auto With(Adaptor&& adaptor) && -> decltype(std::forward(adaptor)( + std::move(*this))) { return std::forward(adaptor)(std::move(*this)); } @@ -412,6 +411,13 @@ class ABSL_MUST_USE_RESULT StatusBuilder { // nullptr if the result status will be OK. Extra fields moved to the heap to // minimize stack space. std::unique_ptr rep_; + + private: + // Update this status to include the given source location (if supported by + // Status implementation). + // + // This is only for internal use by the StatusBuilder. + static void AddSourceLocation(absl::Status& status, SourceLocation loc); }; // Implicitly converts `builder` to `Status` and write it to `os`. @@ -658,13 +664,17 @@ inline absl::StatusCode StatusBuilder::code() const { return status_.code(); } inline StatusBuilder::operator absl::Status() const& { if (rep_ == nullptr) { - return status_; + absl::Status result = status_; + StatusBuilder::AddSourceLocation(result, loc_); + return result; } return StatusBuilder(*this).CreateStatusAndConditionallyLog(); } inline StatusBuilder::operator absl::Status() && { if (rep_ == nullptr) { - return std::move(status_); + absl::Status result = std::move(status_); + StatusBuilder::AddSourceLocation(result, loc_); + return result; } return std::move(*this).CreateStatusAndConditionallyLog(); } diff --git a/xls/common/status/status_builder_oss.cc b/xls/common/status/status_builder_oss.cc new file mode 100644 index 0000000000..2ebac24cfe --- /dev/null +++ b/xls/common/status/status_builder_oss.cc @@ -0,0 +1,25 @@ +// Copyright 2024 The XLS Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "xls/common/source_location.h" +#include "xls/common/status/status_builder.h" + +namespace xabsl { + +/* static */ void StatusBuilder::AddSourceLocation(absl::Status& status, + xabsl::SourceLocation loc) { + // Not supported in OSS absl::Status at the moment. +} + +} // namespace xabsl diff --git a/xls/common/status/status_macros_test.cc b/xls/common/status/status_macros_test.cc index f7d64ba615..f25195ca7c 100644 --- a/xls/common/status/status_macros_test.cc +++ b/xls/common/status/status_macros_test.cc @@ -30,6 +30,7 @@ namespace { using ::testing::AllOf; +using ::testing::ContainsRegex; using ::testing::Eq; using ::testing::HasSubstr; @@ -261,6 +262,33 @@ TEST(AssignOrReturn, UniquePtrWorksForExistingVariable) { EXPECT_THAT(func().message(), Eq("EXPECTED")); } +namespace assign_or_return { +absl::Status CallThree() { return absl::InternalError("foobar"); } +absl::StatusOr CallTwo() { + XLS_RETURN_IF_ERROR(CallThree()); + return 1; +} +absl::Status CallOne() { + XLS_ASSIGN_OR_RETURN(auto abc, CallTwo()); + (void)abc; + return absl::OkStatus(); +} +} // namespace assign_or_return +TEST(AssignOrReturn, KeepsBackTrace) { +#ifndef XLS_USE_ABSL_SOURCE_LOCATION + GTEST_SKIP() << "Back trace not recorded"; +#endif + auto result = assign_or_return::CallOne(); + RecordProperty("result", testing::PrintToString(result)); + EXPECT_THAT(result.message(), Eq("foobar")); + // Expect 3 lines in the stack trace with status_macros_test.cc in them for + // the three deep call stack. + EXPECT_THAT( + result.ToString(absl::StatusToStringMode::kWithEverything), + ContainsRegex( + "(.*xls/common/status/status_macros_test.cc:[0-9]+.*\n?){3}")); +} + TEST(ReturnIfError, Works) { auto func = []() -> absl::Status { XLS_RETURN_IF_ERROR(ReturnOk()); @@ -325,6 +353,33 @@ TEST(ReturnIfError, WorksWithVoidReturnAdaptor) { EXPECT_EQ(code, 2); } +namespace return_if_error { +absl::Status CallThree() { return absl::InternalError("foobar"); } +absl::Status CallTwo() { + XLS_RETURN_IF_ERROR(CallThree()); + return absl::OkStatus(); +} +absl::Status CallOne() { + XLS_RETURN_IF_ERROR(CallTwo()); + return absl::OkStatus(); +} +} // namespace return_if_error + +TEST(ReturnIfError, KeepsBackTrace) { +#ifndef XLS_USE_ABSL_SOURCE_LOCATION + GTEST_SKIP() << "Back trace not recorded"; +#endif + auto result = return_if_error::CallOne(); + RecordProperty("result", testing::PrintToString(result)); + EXPECT_THAT(result.message(), Eq("foobar")); + // Expect 3 lines in the stack trace with status_macros_test.cc in them for + // the three deep call stack. + EXPECT_THAT( + result.ToString(absl::StatusToStringMode::kWithEverything), + ContainsRegex( + "(.*xls/common/status/status_macros_test.cc:[0-9]+.*\n?){3}")); +} + // Basis for XLS_RETURN_IF_ERROR and XLS_ASSIGN_OR_RETURN benchmarks. Derived // classes override LoopAgain() with the macro invocation(s). template