From 08c8147f549c582689bc9bf97b4fab10fefdaeaa Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Thu, 6 May 2021 15:30:46 -0700 Subject: [PATCH] Update abort() for AZURE_ASSERT (#2170) * remove abort from Core --- sdk/core/azure-core/CMakeLists.txt | 2 + .../inc/azure/core/azure_assert.hpp | 56 +++++++++++++++++++ .../azure-core/inc/azure/core/context.hpp | 9 ++- sdk/core/azure-core/inc/azure/core/etag.hpp | 9 ++- .../azure-core/inc/azure/core/nullable.hpp | 24 ++------ sdk/core/azure-core/src/azure_assert.cpp | 12 ++++ sdk/core/azure-core/test/ut/context.cpp | 20 +++++++ sdk/core/azure-core/test/ut/etag.cpp | 12 ++++ sdk/core/azure-core/test/ut/nullable.cpp | 36 ++++++++++++ 9 files changed, 151 insertions(+), 29 deletions(-) create mode 100644 sdk/core/azure-core/inc/azure/core/azure_assert.hpp create mode 100644 sdk/core/azure-core/src/azure_assert.cpp diff --git a/sdk/core/azure-core/CMakeLists.txt b/sdk/core/azure-core/CMakeLists.txt index f1bec922f..1ccd4cd3c 100644 --- a/sdk/core/azure-core/CMakeLists.txt +++ b/sdk/core/azure-core/CMakeLists.txt @@ -63,6 +63,7 @@ set( inc/azure/core/internal/json/json.hpp inc/azure/core/internal/strings.hpp inc/azure/core/io/body_stream.hpp + inc/azure/core/azure_assert.hpp inc/azure/core/base64.hpp inc/azure/core/case_insensitive_containers.hpp inc/azure/core/context.hpp @@ -87,6 +88,7 @@ set( AZURE_CORE_SOURCE ${CURL_TRANSPORT_ADAPTER_SRC} ${WIN_TRANSPORT_ADAPTER_SRC} + src/azure_assert.cpp src/cryptography/md5.cpp src/http/bearer_token_authentication_policy.cpp src/http/http.cpp diff --git a/sdk/core/azure-core/inc/azure/core/azure_assert.hpp b/sdk/core/azure-core/inc/azure/core/azure_assert.hpp new file mode 100644 index 000000000..d16ee8e42 --- /dev/null +++ b/sdk/core/azure-core/inc/azure/core/azure_assert.hpp @@ -0,0 +1,56 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +/** + * @file + * @brief Provide assert macros to use with pre-conditions. + * + * @remark Asserts are turned ON when `NDEBUG` is NOT defined (for Debug build). For Release build, + * `std::abort()` is directly called if the condition is false, without calling assert(). + * + */ + +#pragma once + +#include "platform.hpp" + +#include +#include + +#if defined(NDEBUG) + +/* + * NDEBUG = defined = Build is on Release + * Define AZURE_ASSERT to call abort directly on exp == false + */ + +#define AZURE_ASSERT(exp) \ + do \ + { \ + if (!(exp)) \ + { \ + std::abort(); \ + } \ + } while (0) + +#define AZURE_ASSERT_MSG(exp, msg) AZURE_ASSERT(exp) + +#else + +/* + * NDEBUG = NOT defined = Build is on Debug + * Define AZURE_ASSERT to call assert to provide better debug experience. + */ + +#include + +#define AZURE_ASSERT(exp) assert((exp)) +#define AZURE_ASSERT_MSG(exp, msg) assert(((void)msg, (exp))) + +#endif + +[[noreturn]] void AzureNoReturnPath(std::string const& msg); + +#define AZURE_ASSERT_FALSE(exp) AZURE_ASSERT(!(exp)) +#define AZURE_UNREACHABLE_CODE AzureNoReturnPath("unreachable code!") +#define AZURE_NOT_IMPLEMENTED AzureNoReturnPath("not implemented code!") diff --git a/sdk/core/azure-core/inc/azure/core/context.hpp b/sdk/core/azure-core/inc/azure/core/context.hpp index 76d6d2b7c..9f4c7fbe7 100644 --- a/sdk/core/azure-core/inc/azure/core/context.hpp +++ b/sdk/core/azure-core/inc/azure/core/context.hpp @@ -8,6 +8,7 @@ #pragma once +#include "azure/core/azure_assert.hpp" #include "azure/core/datetime.hpp" #include "azure/core/dll_import_export.hpp" @@ -175,11 +176,9 @@ namespace Azure { namespace Core { { if (ptr->Key == key) { - if (typeid(T) != ptr->ValueType) - { - // type mismatch - std::abort(); - } + AZURE_ASSERT_MSG( + typeid(T) == ptr->ValueType, "Type mismatch for Context::TryGetValue()."); + outputValue = *reinterpret_cast(ptr->Value.get()); return true; } diff --git a/sdk/core/azure-core/inc/azure/core/etag.hpp b/sdk/core/azure-core/inc/azure/core/etag.hpp index 74d645b86..4d2a9f397 100644 --- a/sdk/core/azure-core/inc/azure/core/etag.hpp +++ b/sdk/core/azure-core/inc/azure/core/etag.hpp @@ -8,6 +8,7 @@ #pragma once +#include "azure/core/azure_assert.hpp" #include "azure/core/nullable.hpp" #include @@ -111,7 +112,7 @@ public: break; } // Unknown comparison - abort(); + AZURE_UNREACHABLE_CODE; } /** @@ -137,10 +138,8 @@ public: */ const std::string& ToString() const { - if (!m_value.HasValue()) - { - abort(); - } + AZURE_ASSERT_MSG(m_value.HasValue(), "Empty ETag, check HasValue() before calling ToString()."); + return m_value.Value(); } diff --git a/sdk/core/azure-core/inc/azure/core/nullable.hpp b/sdk/core/azure-core/inc/azure/core/nullable.hpp index 556274512..e41289995 100644 --- a/sdk/core/azure-core/inc/azure/core/nullable.hpp +++ b/sdk/core/azure-core/inc/azure/core/nullable.hpp @@ -8,7 +8,8 @@ #pragma once -#include // for abort +#include + #include // for placement new #include #include // for swap and move @@ -220,12 +221,7 @@ public: */ const T& Value() const& noexcept { - if (!m_hasValue) - { - // throwing here prohibited by our guidelines - // https://azure.github.io/azure-sdk/cpp_design.html#pre-conditions - std::abort(); - } + AZURE_ASSERT_MSG(m_hasValue, "Empty Nullable, check HasValue() first."); return m_value; } @@ -235,12 +231,7 @@ public: */ T& Value() & noexcept { - if (!m_hasValue) - { - // throwing here prohibited by our guidelines - // https://azure.github.io/azure-sdk/cpp_design.html#pre-conditions - std::abort(); - } + AZURE_ASSERT_MSG(m_hasValue, "Empty Nullable, check HasValue() first."); return m_value; } @@ -250,12 +241,7 @@ public: */ T&& Value() && noexcept { - if (!m_hasValue) - { - // throwing here prohibited by our guidelines - // https://azure.github.io/azure-sdk/cpp_design.html#pre-conditions - std::abort(); - } + AZURE_ASSERT_MSG(m_hasValue, "Empty Nullable, check HasValue() first."); return std::move(m_value); } diff --git a/sdk/core/azure-core/src/azure_assert.cpp b/sdk/core/azure-core/src/azure_assert.cpp new file mode 100644 index 000000000..dae8a82dc --- /dev/null +++ b/sdk/core/azure-core/src/azure_assert.cpp @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +#include "azure/core/azure_assert.hpp" + +[[noreturn]] void AzureNoReturnPath(std::string const& msg) +{ + // void msg for Release build where Assert is ignored + (void)msg; + AZURE_ASSERT_MSG(false, msg); + std::abort(); +} diff --git a/sdk/core/azure-core/test/ut/context.cpp b/sdk/core/azure-core/test/ut/context.cpp index 8c6eb1d82..6240baf33 100644 --- a/sdk/core/azure-core/test/ut/context.cpp +++ b/sdk/core/azure-core/test/ut/context.cpp @@ -436,3 +436,23 @@ TEST(Context, Deadline) EXPECT_EQ(childCtx.GetDeadline(), Azure::DateTime::min()); } } + +TEST(Context, PreCondition) +{ + // Get a mismatch type from the context + std::string s("Test String"); + + Context context; + Context::Key const key; + + // New context from previous + auto c2 = context.WithValue(key, s); + int value; + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(c2.TryGetValue(key, value), ""); +#else + ASSERT_DEATH(c2.TryGetValue(key, value), "Type mismatch for Context::TryGetValue"); +#endif +} diff --git a/sdk/core/azure-core/test/ut/etag.cpp b/sdk/core/azure-core/test/ut/etag.cpp index 4ad80b9c8..bfe4f08ca 100644 --- a/sdk/core/azure-core/test/ut/etag.cpp +++ b/sdk/core/azure-core/test/ut/etag.cpp @@ -232,3 +232,15 @@ TEST(ETag, EqualsWeak) EXPECT_FALSE(ETag::Equals(weakTagTwo, weakTagtwo, ETag::ETagComparison::Weak)); EXPECT_FALSE(ETag::Equals(weakTagtwo, weakTagTwo, ETag::ETagComparison::Weak)); } + +TEST(ETag, PreCondition) +{ + ETag emptyTag; + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(emptyTag.ToString(), ""); +#else + ASSERT_DEATH(emptyTag.ToString(), "Empty ETag"); +#endif +} diff --git a/sdk/core/azure-core/test/ut/nullable.cpp b/sdk/core/azure-core/test/ut/nullable.cpp index a84f0f23f..8dc8535e3 100644 --- a/sdk/core/azure-core/test/ut/nullable.cpp +++ b/sdk/core/azure-core/test/ut/nullable.cpp @@ -171,3 +171,39 @@ TEST(Nullable, ValueOr) // Ensure val2 is still disengaged after call to ValueOr EXPECT_FALSE(val2); } + +void Foo(int&& rValue) { (void)rValue; } + +TEST(Nullable, PreCondition) +{ + Nullable emptyNullable; + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(auto a = emptyNullable.Value(); (void)a;, ""); +#else + ASSERT_DEATH(auto a = emptyNullable.Value(); (void)a;, "Empty Nullable"); +#endif +} + +TEST(Nullable, PreCondition2) +{ + Nullable emptyNullable; + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(auto& a = emptyNullable.Value(); (void)a;, ""); +#else + ASSERT_DEATH(auto& a = emptyNullable.Value(); (void)a;, "Empty Nullable"); +#endif +} + +TEST(Nullable, PreCondition3) +{ +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(Foo(Nullable().Value());, ""); +#else + ASSERT_DEATH(Foo(Nullable().Value());, "Empty Nullable"); +#endif +}