-
Notifications
You must be signed in to change notification settings - Fork 11.5k
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
Crash from Assertion `!Packs.empty() && "Pack expansion without unexpanded packs?"' #100095
Comments
@zygoloid I wonder if you remember this code and could help resolving this UB and the assertion. |
@llvm/issue-subscribers-clang-frontend Author: Ilya Biryukov (ilya-biryukov)
The following code crashes Clang with assertion at head, see https://gcc.godbolt.org/z/j6zY4as9h
```cpp
template <class... T>
struct Types {};
template <int& field>
using Forget = int;
template <int&... fields>
using SeqKey = Types<Forget<fields>...>;
template <typename Key, typename Value> template <int&... fields>
|
Not for submission yet. Will eventually aim to fix llvm#100095. The current approach should not be correct as it would have *too* much sugar in turn. We should also make sure the final substitution arguments do not contain any sugar. It does address the crash, though, so probably something along that diretion is needed, e.g. need to check if `decltype()` that's not type-dependent, but instantiation-dependent works correctly and follow the approach taken there.
I got to the bottom of it, we are loosing any mention of See draft #100144 for (an invalid) fix that bring some other problems with it, I'll need to explore something along those lines for a proper fix. |
Ping @mizvekov, our sugar expert :) |
I think in particular anything that relied on type deduction preserving sugar would be extremely fragile. The fix in #100144 is invalid. We can't use as-written template arguments where resolved arguments are expected, this will crash in a bunch of places. To fix that particular sugar loss in type deduction, we need sugared resolved arguments, which is implemented by https://reviews.llvm.org/D134113 This might be fine as is, but It just so always happens that google will have some internal thing where the impact is enormous. I have some ideas to reduce impact in that case. I intend to pick up that patch again soon after I restart my resugaring work, which is after my current thing is done. |
Yep, fully agree, the description of the PR says exactly that and that's why in draft state.
Could you elaborate on this? Do we have other similar cases for deduction that work with sugar in Clang?
I work at Google, and I am happy to check we're not seeing any compilation timeouts (i.e. enormous impact). Although our infrastructure does not allow to check how much slower the compilation will get until this actually gets released and we can stare at the graphs monitoring our build latencies, etc. That's still something, though, and given it's a correctness fix, we would be willing to take some regression and help reduce it. |
Until about a three years ago, type deduction in clang did not preserve any type sugar at all. But we missed preserving type sugar in type deduction through template specializations, mainly because After that is taken care of, I think we would be preserving it for practically all cases in type deduction itself. But in general, we have other issues, for one aspect, how we represent some semantic adjustments. So code in clang that relies on type sugar in order not to crash is highly suspect.
Thanks. |
By the way, the change which introduced the crash, 4a8f351, is much older than when we started preserving sugar in type deduction. So it's unlikely the change intended to rely on it. |
Any ideas on whether we could quick fix the UB in clang and just preserve the current behavior in non-assertions-enabled Clang build? |
Yes, I think it's a correctness issue that was there disregarding the sugar. If my reading of your previous comment is correct, I think the type sugar in itself is not the right way to solve this, I'll dig a bit deeper to understand what the standard says we should do here (technically we should substitute the type alias inside the template, but it's unclear whether deduction should still "see" the type alias in any way).
That's the part that I wanted to focus on in particular. It's also true that anything that relying on type sugar for correctness of deduction is also highly suspect, right?
I think I have an idea, but that revolves around removing the assumption that pack expansion type necessarily contains an unexpanded pack. From the looks of it, there is definitely quite some code that relies on it, so it may turn out to be non-trivial. |
This is not a proper fix, but enables Clang to continue working with more libc++ runtime checks enabled.
I mean a really quick fix that just retains current behavior with the libc++ runtime checks enabled, e.g. #100408 |
MSVC and GCC just tend to deduce empty template arguments from pack expansions that don't mention the template parameter name, resulting in "too many arguments" warning: Clang does something unreasonable and deduces a template pack with Btw, a simpler example that does not involve a type alias: template <class ...T> struct Bar {};
template <class ...T> void bar(typename Bar<T>::type...);
void test() {
bar();
bar(1,1,1); // GCC and MSVC deduce empty argument list, Clang deduces "<no value>, <no value>, <no value>".
} I think I'll check back with the standard to see what's expected. |
I would rather get a proper fix, but having a defined-but-broken is probably better than UB. We're still treating the symptoms and not the root cause. |
That's exactly what I need short term - to be able to turn on libc++ runtime checks (specifically, |
One idea is to ask libc++ folks if they would be able to work around this problem on their side. @ldionne
Yes, I don't think we are nowhere soon, if ever, going to get to the point where we can enforce sugar preservation everywhere in C++ from an architectural standpoint. So anything that relied on sugar, either not to crash or to produce semantic effect, would be inconsistent with clang architecture. Of course there were some inconsistent decisions here, like allowing Outside of C++, there are some sugar stuffs that are guaranteed to work and relied on semantically. Particularly in ObjC.
For deduction yes, since we started with no sugar preservation in deduction, and then improved this, but never got to the point where we could rely on it. Btw, I wouldn't rule out that the current pack implementation in clang may incorrectly rely on sugar in some places. |
I don't see anything wrong with libc++ - it just implements a hardening mode where an access to an unset optional aborts the program (or whatever |
I fully believe that. But I do also believe libc++ implements workarounds for cases clang is faulty. |
@mizvekov I'm not sure I understand what kind of a libc++ workaround would be possible here. We're talking about |
The code somehow exercises a code path we had never before seen a test case for. I spent some time in the past trying to reverse engineer a test case that would hit it. My gut feeling is that this is no ordinary code. |
To reiterate what Alex said, there is no actual code in libc++ that hits this, but the assertions inside libc++ from hardened mode uncovers a bug in Clang. It happens on real code (similar to the one I filed in this issue), but the code is outside of libc++. I think I have a good hold of what could be done here and plan to dig deeper into it and send a proper fix. |
The test is set to XFAIL with assertions enabled.
The test is set to XFAIL with assertions enabled.
This patch avoids accessing an unset `std::optional<>`, which is a part of the manifestation of #100095. The other part is an assertion failure that is not addressed here. This is not a proper fix, but enables Clang to continue working with more libc++ runtime checks enabled (specifically, `-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST`, which checks access to unset optionals among other things). A proper fix is being discussed on #100095.
The following code crashes Clang with assertion at head, see https://gcc.godbolt.org/z/j6zY4as9h
This should be relatively recent as it was found by our release testing.
The text was updated successfully, but these errors were encountered: