[libc++abi] Avoid raw calls to assert() in libc++abi #71121
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libcxxabi Author: Louis Dionne (ldionne) ChangesThe runtimes now have a principled way of doing assertions in relation to hardening, so we should use that instead of raw calls to assert() inside libc++abi. This patch aims to maintain the behavior of the demangler code when it is used from within LLVM by introducing a simple DEMANGLE_ASSERT(...) macro that is then defined to the appropriate assertion mechanism. Full diff: https://github.com/llvm/llvm-project/pull/71121.diff 8 Files Affected:
diff --git a/libcxxabi/src/cxa_demangle.cpp b/libcxxabi/src/cxa_demangle.cpp
index 6055b2a538de414..7ac86dab0104e06 100644
--- a/libcxxabi/src/cxa_demangle.cpp
+++ b/libcxxabi/src/cxa_demangle.cpp
@@ -10,10 +10,12 @@
// file does not yet support:
// - C++ modules TS
+#include <__assert>
+#define DEMANGLE_ASSERT(expr, msg) _LIBCPP_ASSERT_UNCATEGORIZED(expr, msg)
+
#include "demangle/DemangleConfig.h"
#include "demangle/ItaniumDemangle.h"
#include "__cxxabi_config.h"
-#include <cassert>
#include <cctype>
#include <cstdio>
#include <cstdlib>
@@ -395,7 +397,7 @@ __cxa_demangle(const char *MangledName, char *Buf, size_t *N, int *Status) {
InternalStatus = demangle_invalid_mangled_name;
else {
OutputBuffer O(Buf, N);
- assert(Parser.ForwardTemplateRefs.empty());
+ DEMANGLE_ASSERT(Parser.ForwardTemplateRefs.empty(), "");
AST->print(O);
O += '\0';
if (N != nullptr)
diff --git a/libcxxabi/src/demangle/DemangleConfig.h b/libcxxabi/src/demangle/DemangleConfig.h
index d5e11432d986764..dec382d0d38f8ef 100644
--- a/libcxxabi/src/demangle/DemangleConfig.h
+++ b/libcxxabi/src/demangle/DemangleConfig.h
@@ -99,6 +99,11 @@
#define DEMANGLE_FALLTHROUGH
#endif
+#ifndef DEMANGLE_ASSERT
+#include <cassert>
+#define DEMANGLE_ASSERT(__expr, __msg) assert((__expr) && (__msg))
+#endif
+
#define DEMANGLE_NAMESPACE_BEGIN namespace { namespace itanium_demangle {
#define DEMANGLE_NAMESPACE_END } }
diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 2336b84da293bcc..30f3ecf858743d6 100644
--- a/libcxxabi/src/demangle/ItaniumDemangle.h
+++ b/libcxxabi/src/demangle/ItaniumDemangle.h
@@ -21,7 +21,6 @@
#include "Utility.h"
#include <__cxxabi_config.h>
#include <algorithm>
-#include <cassert>
#include <cctype>
#include <cstdio>
#include <cstdlib>
@@ -129,12 +128,12 @@ template <class T, size_t N> class PODSmallVector {
// NOLINTNEXTLINE(readability-identifier-naming)
void pop_back() {
- assert(Last != First && "Popping empty vector!");
+ DEMANGLE_ASSERT(Last != First, "Popping empty vector!");
--Last;
}
void shrinkToSize(size_t Index) {
- assert(Index <= size() && "shrinkToSize() can't expand!");
+ DEMANGLE_ASSERT(Index <= size(), "shrinkToSize() can't expand!");
Last = First + Index;
}
@@ -144,11 +143,11 @@ template <class T, size_t N> class PODSmallVector {
bool empty() const { return First == Last; }
size_t size() const { return static_cast<size_t>(Last - First); }
T &back() {
- assert(Last != First && "Calling back() on empty vector!");
+ DEMANGLE_ASSERT(Last != First, "Calling back() on empty vector!");
return *(Last - 1);
}
T &operator[](size_t Index) {
- assert(Index < size() && "Invalid access!");
+ DEMANGLE_ASSERT(Index < size(), "Invalid access!");
return *(begin() + Index);
}
void clear() { Last = First; }
@@ -1678,7 +1677,7 @@ class SpecialSubstitution final : public ExpandedSpecialSubstitution {
std::string_view SV = ExpandedSpecialSubstitution::getBaseName();
if (isInstantiation()) {
// The instantiations are typedefs that drop the "basic_" prefix.
- assert(starts_with(SV, "basic_"));
+ DEMANGLE_ASSERT(starts_with(SV, "basic_"), "");
SV.remove_prefix(sizeof("basic_") - 1);
}
return SV;
@@ -2569,7 +2568,7 @@ void Node::visit(Fn F) const {
return F(static_cast<const X *>(this));
#include "ItaniumNodes.def"
}
- assert(0 && "unknown mangling node kind");
+ DEMANGLE_ASSERT(0, "unknown mangling node kind");
}
/// Determine the kind of a node from its type.
@@ -2611,7 +2610,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
Parser->TemplateParams.push_back(&Params);
}
~ScopedTemplateParamList() {
- assert(Parser->TemplateParams.size() >= OldNumTemplateParamLists);
+ DEMANGLE_ASSERT(Parser->TemplateParams.size() >= OldNumTemplateParamLists, "");
Parser->TemplateParams.shrinkToSize(OldNumTemplateParamLists);
}
TemplateParamList *params() { return &Params; }
@@ -2692,7 +2691,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
}
NodeArray popTrailingNodeArray(size_t FromPosition) {
- assert(FromPosition <= Names.size());
+ DEMANGLE_ASSERT(FromPosition <= Names.size(), "");
NodeArray res =
makeNodeArray(Names.begin() + (long)FromPosition, Names.end());
Names.shrinkToSize(FromPosition);
@@ -2859,7 +2858,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
std::string_view getSymbol() const {
std::string_view Res = Name;
if (Kind < Unnameable) {
- assert(starts_with(Res, "operator") &&
+ DEMANGLE_ASSERT(starts_with(Res, "operator"),
"operator name does not start with 'operator'");
Res.remove_prefix(sizeof("operator") - 1);
if (starts_with(Res, ' '))
@@ -3694,7 +3693,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseUnresolvedName(bool Global) {
}
}
- assert(SoFar != nullptr);
+ DEMANGLE_ASSERT(SoFar != nullptr, "");
Node *Base = getDerived().parseBaseUnresolvedName();
if (Base == nullptr)
@@ -5635,7 +5634,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParam() {
Node *ForwardRef = make<ForwardTemplateReference>(Index);
if (!ForwardRef)
return nullptr;
- assert(ForwardRef->getKind() == Node::KForwardTemplateReference);
+ DEMANGLE_ASSERT(ForwardRef->getKind() == Node::KForwardTemplateReference, "");
ForwardTemplateRefs.push_back(
static_cast<ForwardTemplateReference *>(ForwardRef));
return ForwardRef;
diff --git a/libcxxabi/src/demangle/Utility.h b/libcxxabi/src/demangle/Utility.h
index f8a36f88f8e7b07..f1fad35d60d98e4 100644
--- a/libcxxabi/src/demangle/Utility.h
+++ b/libcxxabi/src/demangle/Utility.h
@@ -19,7 +19,6 @@
#include "DemangleConfig.h"
#include <array>
-#include <cassert>
#include <cstdint>
#include <cstdlib>
#include <cstring>
@@ -159,7 +158,7 @@ class OutputBuffer {
}
void insert(size_t Pos, const char *S, size_t N) {
- assert(Pos <= CurrentPosition);
+ DEMANGLE_ASSERT(Pos <= CurrentPosition, "");
if (N == 0)
return;
grow(N);
@@ -172,7 +171,7 @@ class OutputBuffer {
void setCurrentPosition(size_t NewPos) { CurrentPosition = NewPos; }
char back() const {
- assert(CurrentPosition);
+ DEMANGLE_ASSERT(CurrentPosition, "");
return Buffer[CurrentPosition - 1];
}
diff --git a/libcxxabi/src/fallback_malloc.cpp b/libcxxabi/src/fallback_malloc.cpp
index f9fb1bc46302a9b..a499416f61c4bd2 100644
--- a/libcxxabi/src/fallback_malloc.cpp
+++ b/libcxxabi/src/fallback_malloc.cpp
@@ -16,7 +16,7 @@
#endif
#include <__memory/aligned_alloc.h>
-#include <assert.h>
+#include <__assert>
#include <stdlib.h> // for malloc, calloc, free
#include <string.h> // for memset
@@ -142,7 +142,7 @@ void* fallback_malloc(size_t len) {
// Check the invariant that all heap_nodes pointers 'p' are aligned
// so that 'p + 1' has an alignment of at least RequiredAlignment
- assert(reinterpret_cast<size_t>(p + 1) % RequiredAlignment == 0);
+ _LIBCPP_ASSERT_UNCATEGORIZED(reinterpret_cast<size_t>(p + 1) % RequiredAlignment == 0, "");
// Calculate the number of extra padding elements needed in order
// to split 'p' and create a properly aligned heap_node from the tail
@@ -163,7 +163,7 @@ void* fallback_malloc(size_t len) {
q->next_node = 0;
q->len = static_cast<heap_size>(aligned_nelems);
void* ptr = q + 1;
- assert(reinterpret_cast<size_t>(ptr) % RequiredAlignment == 0);
+ _LIBCPP_ASSERT_UNCATEGORIZED(reinterpret_cast<size_t>(ptr) % RequiredAlignment == 0, "");
return ptr;
}
@@ -176,7 +176,7 @@ void* fallback_malloc(size_t len) {
prev->next_node = p->next_node;
p->next_node = 0;
void* ptr = p + 1;
- assert(reinterpret_cast<size_t>(ptr) % RequiredAlignment == 0);
+ _LIBCPP_ASSERT_UNCATEGORIZED(reinterpret_cast<size_t>(ptr) % RequiredAlignment == 0, "");
return ptr;
}
}
diff --git a/llvm/include/llvm/Demangle/DemangleConfig.h b/llvm/include/llvm/Demangle/DemangleConfig.h
index 2ff95dd8d29e5e6..30f72ffe0d7efac 100644
--- a/llvm/include/llvm/Demangle/DemangleConfig.h
+++ b/llvm/include/llvm/Demangle/DemangleConfig.h
@@ -86,6 +86,11 @@
#define DEMANGLE_FALLTHROUGH
#endif
+#ifndef DEMANGLE_ASSERT
+#include <cassert>
+#define DEMANGLE_ASSERT(__expr, __msg) assert((__expr) && (__msg))
+#endif
+
#define DEMANGLE_NAMESPACE_BEGIN namespace llvm { namespace itanium_demangle {
#define DEMANGLE_NAMESPACE_END } }
diff --git a/llvm/include/llvm/Demangle/ItaniumDemangle.h b/llvm/include/llvm/Demangle/ItaniumDemangle.h
index f68c37258ce0992..4b2e18ec716367b 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -19,8 +19,8 @@
#include "DemangleConfig.h"
#include "StringViewExtras.h"
#include "Utility.h"
+#include <__cxxabi_config.h>
#include <algorithm>
-#include <cassert>
#include <cctype>
#include <cstdio>
#include <cstdlib>
@@ -128,12 +128,12 @@ template <class T, size_t N> class PODSmallVector {
// NOLINTNEXTLINE(readability-identifier-naming)
void pop_back() {
- assert(Last != First && "Popping empty vector!");
+ DEMANGLE_ASSERT(Last != First, "Popping empty vector!");
--Last;
}
void shrinkToSize(size_t Index) {
- assert(Index <= size() && "shrinkToSize() can't expand!");
+ DEMANGLE_ASSERT(Index <= size(), "shrinkToSize() can't expand!");
Last = First + Index;
}
@@ -143,11 +143,11 @@ template <class T, size_t N> class PODSmallVector {
bool empty() const { return First == Last; }
size_t size() const { return static_cast<size_t>(Last - First); }
T &back() {
- assert(Last != First && "Calling back() on empty vector!");
+ DEMANGLE_ASSERT(Last != First, "Calling back() on empty vector!");
return *(Last - 1);
}
T &operator[](size_t Index) {
- assert(Index < size() && "Invalid access!");
+ DEMANGLE_ASSERT(Index < size(), "Invalid access!");
return *(begin() + Index);
}
void clear() { Last = First; }
@@ -1677,7 +1677,7 @@ class SpecialSubstitution final : public ExpandedSpecialSubstitution {
std::string_view SV = ExpandedSpecialSubstitution::getBaseName();
if (isInstantiation()) {
// The instantiations are typedefs that drop the "basic_" prefix.
- assert(starts_with(SV, "basic_"));
+ DEMANGLE_ASSERT(starts_with(SV, "basic_"), "");
SV.remove_prefix(sizeof("basic_") - 1);
}
return SV;
@@ -2568,7 +2568,7 @@ void Node::visit(Fn F) const {
return F(static_cast<const X *>(this));
#include "ItaniumNodes.def"
}
- assert(0 && "unknown mangling node kind");
+ DEMANGLE_ASSERT(0, "unknown mangling node kind");
}
/// Determine the kind of a node from its type.
@@ -2610,7 +2610,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
Parser->TemplateParams.push_back(&Params);
}
~ScopedTemplateParamList() {
- assert(Parser->TemplateParams.size() >= OldNumTemplateParamLists);
+ DEMANGLE_ASSERT(Parser->TemplateParams.size() >= OldNumTemplateParamLists, "");
Parser->TemplateParams.shrinkToSize(OldNumTemplateParamLists);
}
TemplateParamList *params() { return &Params; }
@@ -2691,7 +2691,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
}
NodeArray popTrailingNodeArray(size_t FromPosition) {
- assert(FromPosition <= Names.size());
+ DEMANGLE_ASSERT(FromPosition <= Names.size(), "");
NodeArray res =
makeNodeArray(Names.begin() + (long)FromPosition, Names.end());
Names.shrinkToSize(FromPosition);
@@ -2858,7 +2858,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
std::string_view getSymbol() const {
std::string_view Res = Name;
if (Kind < Unnameable) {
- assert(starts_with(Res, "operator") &&
+ DEMANGLE_ASSERT(starts_with(Res, "operator"),
"operator name does not start with 'operator'");
Res.remove_prefix(sizeof("operator") - 1);
if (starts_with(Res, ' '))
@@ -3693,7 +3693,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseUnresolvedName(bool Global) {
}
}
- assert(SoFar != nullptr);
+ DEMANGLE_ASSERT(SoFar != nullptr, "");
Node *Base = getDerived().parseBaseUnresolvedName();
if (Base == nullptr)
@@ -5634,7 +5634,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParam() {
Node *ForwardRef = make<ForwardTemplateReference>(Index);
if (!ForwardRef)
return nullptr;
- assert(ForwardRef->getKind() == Node::KForwardTemplateReference);
+ DEMANGLE_ASSERT(ForwardRef->getKind() == Node::KForwardTemplateReference, "");
ForwardTemplateRefs.push_back(
static_cast<ForwardTemplateReference *>(ForwardRef));
return ForwardRef;
diff --git a/llvm/include/llvm/Demangle/Utility.h b/llvm/include/llvm/Demangle/Utility.h
index 99ed81461ce4138..e893cceea2cdc48 100644
--- a/llvm/include/llvm/Demangle/Utility.h
+++ b/llvm/include/llvm/Demangle/Utility.h
@@ -19,7 +19,6 @@
#include "DemangleConfig.h"
#include <array>
-#include <cassert>
#include <cstdint>
#include <cstdlib>
#include <cstring>
@@ -159,7 +158,7 @@ class OutputBuffer {
}
void insert(size_t Pos, const char *S, size_t N) {
- assert(Pos <= CurrentPosition);
+ DEMANGLE_ASSERT(Pos <= CurrentPosition, "");
if (N == 0)
return;
grow(N);
@@ -172,7 +171,7 @@ class OutputBuffer {
void setCurrentPosition(size_t NewPos) { CurrentPosition = NewPos; }
char back() const {
- assert(CurrentPosition);
+ DEMANGLE_ASSERT(CurrentPosition, "");
return Buffer[CurrentPosition - 1];
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
LGTM modulo the comment! |
var-const
approved these changes
Nov 2, 2023
nickdesaulniers
approved these changes
Nov 9, 2023
ldionne
force-pushed
the
review/no-raw-assert-cxxabi
branch
from
0d7e458
to
d5a436a
Compare
The runtimes now have a principled way of doing assertions in relation to hardening, so we should use that instead of raw calls to assert() inside libc++abi. This patch aims to maintain the behavior of the demangler code when it is used from within LLVM by introducing a simple DEMANGLE_ASSERT(...) macro that is then defined to the appropriate assertion mechanism.
…bi (circular deps)
ldionne
force-pushed
the
review/no-raw-assert-cxxabi
branch
from
d5a436a
to
2f96e0b
Compare
|
Hello @ldionne, Downstream, this patch caused a problem for the Zig project, because it changes the behavior to embed the result of To resolve this, we ended up patching libcxxabi like this: ziglang/zig@98a30ac There are two issues here being addressed:
Perhaps there is a way to resolve this in a way that does not require a downstream patch? Thanks for your help, |
|
Thanks for the heads up. We can apply that work around upstream. In the long term, we should actually merge libc++abi into libc++ so we can use the hardening mechanisms that are used by libc++ instead. |
|
@andrewrk Can you open a PR to respect |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The runtimes now have a principled way of doing assertions in relation to hardening, so we should use that instead of raw calls to assert() inside libc++abi. This patch aims to maintain the behavior of the demangler code when it is used from within LLVM by introducing a simple DEMANGLE_ASSERT(...) macro that is then defined to the appropriate assertion mechanism.