[clang] After 7d2d5a3a6d7, Assertion failed: (getContext().hasSameUnqualifiedType(E->getType(), E->getArg(0)->getType())), function EmitCXXConstructExpr, file clang/lib/CodeGen/CGExprCXX.cpp, line 616

[DimitryAndric]

Bugzilla Link	51862
Resolution	FIXED
Resolved on	Sep 30, 2021 18:17
Version	trunk
OS	All
Blocks	#50580
CC	@aaronpuchert,@Quuxplusone,@berolinux,@DougGregor,@DimitryAndric,@emaste,@mizvekov,@zygoloid,@rjmccall,@tstellar
Fixed by commit(s)	d9308aa d7b669b

Extended Description

While building the FreeBSD math/frobby port (see also https://doc.sagemath.org/html/en/reference/spkg/frobby.html ) with clang 13 as part of a large ports build, I encountered the following assertion:

Assertion failed: (getContext().hasSameUnqualifiedType(E->getType(), E->getArg(0)->getType())), function EmitCXXConstructExpr, file /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGExprCXX.cpp, line 616.

Program received signal SIGABRT, Aborted.
thr_kill () at thr_kill.S:4
4 RSYSCALL(thr_kill)
(gdb) bt
#​0 thr_kill () at thr_kill.S:4
#​1 0x00000000066c15af in __raise (s=s@entry=6) at /share/dim/src/freebsd/llvm-13-update/lib/libc/gen/raise.c:52
#​2 0x000000000672b0b9 in abort () at /share/dim/src/freebsd/llvm-13-update/lib/libc/stdlib/abort.c:67
#​3 0x00000000066ac7ba in __assert (func=, file=, line=, failedexpr=) at /share/dim/src/freebsd/llvm-13-update/lib/libc/gen/assert.c:51
#​4 0x0000000002ce4b94 in EmitCXXConstructExpr () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGExprCXX.cpp:615
#​5 0x0000000002ba8b43 in VisitCXXConstructExpr () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:1315
#​6 0x0000000002ba4eb7 in Visit () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:107
#​7 EmitAggExpr () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:1998
#​8 0x0000000002b12837 in EmitReturnStmt () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGStmt.cpp:1291
#​9 0x0000000002b0f0c9 in EmitStmt () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGStmt.cpp:152
#​10 0x0000000002b19edc in EmitCompoundStmtWithoutScope () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CGStmt.cpp:496
#​11 0x0000000002afe87d in EmitFunctionBody () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1199
#​12 0x0000000002aff3be in GenerateCode () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1373
#​13 0x000000000267cf49 in EmitGlobalFunctionDefinition () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:4861
#​14 0x0000000002676656 in EmitGlobalDefinition () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3220
#​15 0x000000000267a251 in EmitGlobal () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:2982
#​16 0x0000000002680332 in EmitTopLevelDecl () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:5698
#​17 0x00000000030f974f in HandleTopLevelDecl () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:170
#​18 0x00000000030f6d86 in HandleTopLevelDecl () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:215
#​19 0x00000000035cfce7 in ParseAST () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/Parse/ParseAST.cpp:162
#​20 0x00000000030378bf in Execute () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/Frontend/FrontendAction.cpp:951
#​21 0x0000000002fc10bf in ExecuteAction () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:974
#​22 0x00000000030f0d9b in ExecuteCompilerInvocation () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:278
#​23 0x0000000002615751 in cc1_main () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/tools/driver/cc1_main.cpp:246
#​24 0x0000000002622d92 in ExecuteCC1Tool () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/tools/driver/driver.cpp:338
#​25 0x0000000002622b80 in main () at /share/dim/src/freebsd/llvm-13-update/contrib/llvm-project/clang/tools/driver/driver.cpp:409

Bisection shows this to be a regression due to 7d2d5a3a6d7 ("[clang] Apply P1825 as Defect Report from C++11 up to C++20").

Reduced test case:

// clang -cc1 -triple x86_64-- -S IntersectFacade-min.cpp
struct a {
a();
a(a &);
a(int);
template operator b();
};
a c() {
if (0) {
a e;
return e;
}
a d;
return d;
}

[Quuxplusone]

Indeed. (The assertion fails, but in a release build nothing bad happens; that is, nothing obviously bad happens.)
https://godbolt.org/z/4n1sEazP5

I notice that your reduced example depends on a really grotesque set of implicit conversions:

struct a {
a(a&);
a(int);
operator int();
};
a c(a d) {
return d;
}

• Clang prefers operator int() followed by a(int), since Clang 13. (This conforms to C++20 because P1155; we seem to DR it into all C++ versions.)
• MSVC prefers operator int() followed by a(int), since MSVC 19.24. (This conforms to C++20 because P1155; they seem to DR it into all C++ versions.)
• GCC trunk still prefers a(a&). (This conforms to C++17's status quo ante, because the constructor found by the overload-resolution-as-rvalue isn't a move constructor, so we do a second overload-resolution-as-lvalue which finds a(a&) as the best match.)

It's actually surprising to me that the implicit conversion sequence here is allowed to stack two user-defined conversions in a row (operator int(), a(int)) -- I thought that was never allowed in C++ as a general rule!

Leaving aside the assert-fail (which Clang should certainly fix) -- Are you at all concerned about the change in overload resolution here between C++17 and C++20, and/or the difference between Clang 12/GCC and Clang 13/MSVC? You might want to do some cleanup in your code, which would end up working around the assert-fail anyway as a side effect.

[DimitryAndric]

Leaving aside the assert-fail (which Clang should certainly fix) -- Are you
at all concerned about the change in overload resolution here between C++17
and C++20, and/or the difference between Clang 12/GCC and Clang 13/MSVC? You
might want to do some cleanup in your code, which would end up working
around the assert-fail anyway as a side effect.

Frobby isn't a project I actively work on, but simply a port failure that was reported to assert with clang 13. :) It looks like the project is using a somewhat older C++ dialect, as it is littered with auto_ptr and such.

The original test case asserted on generating code for IntersectFacade::intersect():

Stack dump:
0. Program arguments: clang -cc1 -triple x86_64-unknown-freebsd14.0 -S --mrelax-relocations -disable-free -mrelocation-model static -mframe-pointer=all -relaxed-aliasing -fno-rounding-math -mconstructor-aliases -munwind-tables -target-cpu x86-64 -tune-cpu generic -O2 -Wno-parentheses-equality -Wno-deprecated-declarations -fdeprecated-macro -stack-protector 2 -fcxx-exceptions -fexceptions -vectorize-loops -vectorize-slp -faddrsig IntersectFacade-2324f5.ii

1.  <eof> parser at end of file
   

2.  src/IntersectFacade.cpp:32:37: LLVM IR generation of declaration 'IntersectFacade::intersect'
   

3.  src/IntersectFacade.cpp:32:37: Generating code for declaration 'IntersectFacade::intersect'
   

which comes from here:
https://github.com/Macaulay2/frobby/blob/master/src/IntersectFacade.cpp#L32

The parts which have been extracted by creduce correspond roughly to:

auto_ptr IntersectFacade::intersect(const vector<BigIdeal*>& ideals,
const VarNames& emptyNames) {
beginAction("Intersecting ideals.");

if (ideals.empty()) {
auto_ptr entireRing(new BigIdeal(emptyNames));
entireRing->newLastTerm();
return entireRing;
}
...
auto_ptr bigIdeal(new BigIdeal(names));
bigIdeal->insert(*intersection, translator);

endAction();
return bigIdeal;
}

Obviously creduce went through a lot of trouble to massage away the whole auto_ptr implementation from libc++, which is probably why the minimized test case looks a little weird.

[mizvekov]

It just never happened before that we would pick this conversion sequence when performing NRVO. The original implementation of P1155 from before the DR had a bug where that would abort the first overload resolution.

CC John McCall, who wrote the assert originally:
8ea46b6#diff-3c2b51085a493a8b9ff4a0140e4375e68e09012bb23aabefd511505c6d0359a9

I think in this case, the assert should just to be moved into the scope of the following condition.

[zygoloid]

If it's really correct that we perform two implicit conversions in a row here, I think we'll need to check all the consumers of CXXConstructExpr::isElidable and make sure they can cope with that. Both CodeGen and IgnoreElidableImplicitConstructorSingleStep (and probably other uses) will need to learn to step over not just a MaterializeTemporaryExpr but potentially also an implicit user-defined conversion when looking for the source class object in the elidable constructor call, in order to properly elide the constructor call (and its argument conversion).

The reason we perform two implicit conversions in a row here is that [dcl.init.general]/17.6.2 treats copy-initialization of a class from a glvalue of the same type as if it were direct-initialization, and as a result the special case in [over.best.ics.general]/4 doesn't apply at the top level, so we permit at most two user-defined conversions instead of at most one (exactly like we would for a direct-initialization).

If we want a short-term fix here, we could stop marking the CXXConstructExpr as elidable in this case. That's wrong (the copy is elidable) but it would let us go back to guaranteeing that the argument of an elidable construction is a glvalue of the same type. I guess no-one really cares too much if we elide the copy in a case like this?

But I wonder if perhaps the right answer here is to get the standard changed to disallow the two implicit conversions in a row, by expanding [over.best.ics.general]/4.3 to also cover the case where we're performing copy-initialization from an expression of the same or a derived class type.

[zygoloid]

We don't need two implicit conversions in a row to trigger this assertion. All we need is an elidable copy operation that calls something other than the copy constructor. For example:

struct b {};
struct a : b {
a();
a(a &);
a(const b &);
};
a c() {
if (0) { return a(); }

a x;
return x;
}

https://godbolt.org/z/T4zrxh5vd

So it seems like fixing the "two user-defined conversions in a row" issue in the standard won't be enough to resolve this.

[rjmccall]

Right, the assertion is totally correct except that, in this marvelous special case, the proper source expression is not necessarily getArg(0). Assuming we're going to make this work, we just need to be doing a better search for the elided object expression. Richard is right that we probably ought to be looking through qualification conversions and BindTemporaryExprs anyway.

CXXConstructExpr should just have a method to find this expression, and hopefully we can make Sema assert after building an elidable CXXConstructExpr that the method returns the expected expression.

I guess it makes sense that elided copy initializations can include not just a use of a non-copy/move-constructor but also even an invocation of a user-defined conversion, although this is well into that rather peculiar C++ sense of "making sense" where you've already accepted three absurdities so what's wrong with a fourth.

[mizvekov]

Okay, thanks John and Richard for the explanations :-)

I made a DR for the change where we just stop using this flag when copy eliding these initializations: https://reviews.llvm.org/D109800

If there was anything at all using this information, it looks like there was no test coverage for it anyway, so we might as well just remove the thing until we have time to implement this more complicated analysis of the CXXConstruct expression and then actually implement any uses of it and their related tests.

[rjmccall]

I suspect it's not a very difficult analysis; it just needs to look through ICEs, materializations, and (apparently) conversion operator calls until it finds a pr-value of the same type as the CXXConstructExpr.

[mizvekov]

I also think this is not hard to implement, but I believe this issue should block release-13.0.0, and it seems to me the straightforward thing is to just disable it for NRVO, since it's almost out the door.

[DimitryAndric]

I also think this is not hard to implement, but I believe this issue should
block release-13.0.0, and it seems to me the straightforward thing is to
just disable it for NRVO, since it's almost out the door.

Not sure how happy Tom Stellard will be since we're at 13.0.0-rc3 already, but shipping with this bug is going to result in quite a number of field reports, I guess. :)

In any case, I've confirmed that both the original test case (from https://github.com/Macaulay2/frobby) and the reduced test case compile successfully for me, with this patch added.

[tstellar]

Do we have a commit that I can backport (or revert) to fix this?

[mizvekov]

@​tstellar it's being worked on right now at https://reviews.llvm.org/D109800

I suspect the implementation itself is ready, and we are just going to adjust the tests and/or some comments perhaps.

[Quuxplusone]

FYI, I've put some thoughts on the wider issue (more CWG than Clang) at
https://quuxplusone.github.io/blog/2021/09/17/a-class-without-a-copy-constructor/

[tstellar]

I can hold a few more days for this if you think there is a large impact from this bug, but I don't think we can hold the release indefinitely for this. We always have 13.0.1 to land this fix.

Any ETA on when the patch will be reviewed?

[aaronpuchert]

Let me reopen because the backport is still missing as far as I can tell.

[DimitryAndric]

Let me reopen because the backport is still missing as far as I can tell.

Yes, this should be in 13.0.0. If Tom's OK with it, it can be merged right away?

[aaronpuchert]

He'll just pick it up, don't worry.

[tstellar]

How important is it to get this into 13.0.0 as opposed to waiting until 13.0.1? Richard mentioned on Discord that the case its fixing seems fairly obscure, and it hasn't had much testing in main.

[DimitryAndric]

How important is it to get this into 13.0.0 as opposed to waiting until
13.0.1? Richard mentioned on Discord that the case its fixing seems fairly
obscure, and it hasn't had much testing in main.

In FreeBSD's base system I'll just import the commit separately, but indeed the problem is (as far as I am aware) only encountered when building this particular math program. Since having P1825 support is rather important we'd better not attempt to revert 7d2d5a3, and postpone merging d9308aa to 13.0.1.

[tstellar]

I've decided to merge this one for 13.0.0. Mainly because it introduces an API change, so we may not be able to include it in 13.0.1.

I'm OK taking the risk of it being in trunk for only a few days. We still have about a week until GA. If there are major issues, we can always revert it in 13.0.1.

[mizvekov]

*** Bug llvm/llvm-bugzilla-archive#51987 has been marked as a duplicate of this bug. ***

[tstellar]

Merged: d7b669b

[mizvekov]

mentioned in issue llvm/llvm-bugzilla-archive#51987