New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memory leak in std::string comparison after libc++ aaef3b82f4f0 #60709
Comments
It causes mysterious memory leaks when comparing std::string, see GitHub Issue #60709 and the code review. > All supported compilers support `consteval`, so there is no more need for the macro. > > Reviewed By: ldionne, Mordante, #libc > > Spies: libcxx-commits > > Differential Revision: https://reviews.llvm.org/D143489 This reverts commit aaef3b8.
I'm pretty sure this is a compiler problem. Changing |
@llvm/issue-subscribers-clang-frontend |
It's also worth noting that Clang doesn't currently define |
Sorry, I meant the other way around. Making the function
I am aware of that. The memory leak is still very surprising. |
Oh yeah that IS surprising!
Agreed, and I'm not suggesting this isn't necessarily a frontend issue in Clang. |
Removing the libc++ tag since this was fixed in libc++ by using |
I wonder if that could be related to #46711 - IE, code gen bug |
Still reproducible with trunk and the diff diff --git a/libcxx/include/__compare/ordering.h b/libcxx/include/__compare/ordering.h
index c348f0433a32..e0104c018879 100644
--- a/libcxx/include/__compare/ordering.h
+++ b/libcxx/include/__compare/ordering.h
@@ -40,7 +40,7 @@ template<class _Tp, class... _Args>
inline constexpr bool __one_of_v = (is_same_v<_Tp, _Args> || ...);
struct _CmpUnspecifiedParam {
- _LIBCPP_HIDE_FROM_ABI constexpr
+ _LIBCPP_HIDE_FROM_ABI consteval
_CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
template<class _Tp, class = enable_if_t<!__one_of_v<_Tp, int, partial_ordering, weak_ordering, strong_ordering>>> I got the exact same error.
|
Reduced: struct _CmpUnspecifiedParam {
consteval _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {} // Change here to constexpr to make leak go away
};
struct ordering {
int value;
friend constexpr bool operator<(ordering lhs, _CmpUnspecifiedParam) { return lhs.value > 0; }
};
class string {
public:
string(const char* data, unsigned int size) {Data = new char[size];}
friend ordering operator<=>(string lhs, const char* rhs) { return ordering{1}; }
~string() { delete[] Data;}
private:
char *Data;
};
struct S {
string ToString() { return string(data, sizeof(data)); }
char data[10] = {'a'};
};
int main() {
S s;
int i = 0;
do {
++i;
} while (i < 2 && s.ToString() < "whatev");
return 0;
} It seems changing constructor of
Looking further. |
The problematic bit seems to be this: llvm-project/clang/lib/Sema/SemaExpr.cpp Line 18173 in e829eb9
I'm not quite sure why immediate invocation would need a cleanup, but calling |
CC @zygoloid for any insights he might have based on the information you gathered so far |
For the record, I'm also looking into it. I can confirm @Fznamznon's conclusions and I also think the line mentioned above is probably wrong. I would assume that immediate invocations never need to run the cleanups themselves (there are no Here are a few more details. A smaller repro is: struct iparam {
consteval iparam() {}
};
struct string {
string(int v) { this->data = new int(v); }
~string() { delete data; }
private:
int *data;
};
int main() {
for (;string(1), iparam(), false;);
return 0;
} See godbolt.
If constructor is not
Note that for (;iparam(), string(1), false;); Or if the temporary is created both before and after the immediate invocation: for (;string(1), iparam(), string(1), false;); In this case, we get actually get two
|
I also have another guess, that even though the line I mentioned above may not be correct, it should bring no harm. I suspect that @ilya-biryukov given that your findings confirm my earlier conclusion, what about I post a patch removing the problematic bit? |
IIUC the struct param {
consteval param() { val = nullptr; allocated = false; }
~param() { delete val; }
param& assign(int v) {
if (!allocated) {
val = new int;
allocated = true;
}
*val = v;
return *this;
}
int get() { return *val; }
private:
int *val;
bool allocated;
};
int main() {
// param() is an immediate invocation,
// but we need to run the destructor for a temporary produced by it.
for (; param().assign(10).get() < 5;) {}
}
@Fznamznon this sounds good, let's try this out. I think it would also be valuable to add tests that check AST and codegen is doing the right thing here. We could continue the discussion in Phabricator. |
I've created https://reviews.llvm.org/D153294
I see! Thank you for the explanation. |
It's mysterious, but bisection clearly points to aaef3b8. Here is a reduced reproducer:
Dropping the
-std=c++20
flag makes the leak go away.The diff below also makes it go away.
The text was updated successfully, but these errors were encountered: