Assertion failed: Result.isUninit() && "temporary created multiple times", file ExprConstant.cpp, line 1107

[Quuxplusone]

Bugzilla Link	PR33140
Status	NEW
Importance	P normal
Reported by	Russell Gallop (russell_gallop@sn.scee.net)
Reported on	2017-05-23 06:51:27 -0700
Last modified on	2017-06-14 10:40:58 -0700
Version	trunk
Hardware	PC Linux
CC	arichardson.kde@gmail.com, bcahoon@codeaurora.org, dgregor@apple.com, efriedma@quicinc.com, greg.bedwell@sony.com, llvm-bugs@lists.llvm.org, nejati@gmail.com, nicholas@mxc.ca, nlewycky@google.com, paul_robinson@playstation.sony.com, richard-llvm@metafoo.co.uk, v.reichelt@netcologne.de
Fixed by commit(s)
Attachments	llvm-bug.txt (11201 bytes, text/plain)
Blocks
Blocked by
See also

These are two cases found in internal testing. They hit the same assertion but
were introduced at different times, presumably by related commits.

$ cat testa.cpp
// ========================
struct S {
  constexpr S(const int &a = 0) {}
};
void foo(void) {
  S s[2] = {};
}
// =======================
$ clang -c testa.cpp -std=c++11
Assertion failed: Result.isUninit() && "temporary created multiple times", file
<...>\ExprConstant.cpp, line 1107

Bisects to:
commit f83586caa9df7ebae5226227614513df4af4b402
Author: Nick Lewycky <nicholas@mxc.ca>
Date:   Sat Apr 29 09:33:46 2017 +0000

    Remove Sema::CheckForIntOverflow, and instead check all full-expressions.

    CheckForIntOverflow used to implement a whitelist of top-level expressions to
    send to the constant expression evaluator, which handled many more expressions
    than the CheckForIntOverflow whitelist did.

    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@301742 91177308-0d34-0410-b5e6-96231b3b80d8

$ cat testb.cpp
// ==============================================================
bool bar(float const &f = 0);
bool foo() { return bar() && bar(); }
// ==============================================================
$ clang -c testb.cpp
Assertion failed: Result.isUninit() && "temporary created multiple times", file
<...>\ExprConstant.cpp, line 1107

Bisects to:
commit 750306071663247152ab611f63e1d97ec175786d
Author: Nick Lewycky <nicholas@mxc.ca>
Date:   Wed May 17 23:56:54 2017 +0000

    The constant expression evaluator should examine function arguments for non-constexpr function calls unless the EvalInfo says to stop.

    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@303317 91177308-0d34-0410-b5e6-96231b3b80d8

[Quuxplusone]

testa.cpp has PR32864 again, InitListExpr->isTransparent() is false. I think
there are other bugs here.

In testb.cpp we've got this situation:
(gdb) call E->dump()
BinaryOperator 0xb7e2668 '_Bool' '&&'
|-CallExpr 0xb7e2520 '_Bool'
| |-ImplicitCastExpr 0xb7e2508 '_Bool (*)(const float &)'
<FunctionToPointerDecay>
| | `-DeclRefExpr 0xb7e24b0 '_Bool (const float &)' lvalue Function 0xb7e2290
'bar' '_Bool (const float &)'
| `-CXXDefaultArgExpr 0xb7e2558 'const float' lvalue
`-CallExpr 0xb7e2610 '_Bool'
  |-ImplicitCastExpr 0xb7e25f8 '_Bool (*)(const float &)' <FunctionToPointerDecay>
  | `-DeclRefExpr 0xb7e25d0 '_Bool (const float &)' lvalue Function 0xb7e2290 'bar' '_Bool (const float &)'
  `-CXXDefaultArgExpr 0xb7e2648 'const float' lvalue
1.
(gdb) call E->dump()
CXXDefaultArgExpr 0xb7e2558 'const float' lvalue
(gdb) call E->getExpr()->dump()
MaterializeTemporaryExpr 0xb7e21f8 'const float' lvalue
`-ImplicitCastExpr 0xb7e21e0 'const float' <IntegralToFloating>
  `-IntegerLiteral 0xb7e21c0 'int' 0
2.
(gdb) call E->dump()
CXXDefaultArgExpr 0xb7e2648 'const float' lvalue
(gdb) call E->getExpr()->dump()
MaterializeTemporaryExpr 0xb7e21f8 'const float' lvalue
`-ImplicitCastExpr 0xb7e21e0 'const float' <IntegralToFloating>
  `-IntegerLiteral 0xb7e21c0 'int' 0

Either we shouldn't be sharing MTE's across multiple CXXDefaultArgExpr's, or we
should key the Temporaries off the CXXDefaultArgExpr when there is one, or we
should enter a new CallStackFrame when evaluating the arguments to a callee.

If we do the last one we could try to put it in handleCallExpr to solve
testb.cc and EvaluateArgs to handle testa.cpp. Or we could try to put it in
CXXDefaultArgExpr itself somehow, the stack trace from CXXDefaultArgExpr to
MaterializeTemporaryExpr runs right through the LValueVisitor / StmtVisitorBase:

#10 0x0000000006a5e154 in Evaluate (Result=..., Info=..., E=0xb7e2648)
    at /usr/local/google/home/nlewycky/llvm/src/tools/clang/lib/AST/ExprConstant.cpp:9831
9831        if (!EvaluateLValue(E, LV, Info))
(gdb)
#9  0x0000000006a46eb9 in EvaluateLValue (E=0xb7e2648, Result=..., Info=...,
InvalidBaseOK=false)
    at /usr/local/google/home/nlewycky/llvm/src/tools/clang/lib/AST/ExprConstant.cpp:5084
5084      return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
(gdb)
#8  0x0000000006a672cd in clang::StmtVisitorBase<clang::make_const_ptr,
(anonymous namespace)::LValueExprEvaluator, bool>::Visit (this=0x7fffffff9000,
S=0xb7e2648)
    at tools/clang/include/clang/AST/StmtNodes.inc:227
227 CXXDEFAULTARGEXPR(CXXDefaultArgExpr, Expr)
(gdb)
#7  0x0000000006a7e3ee in (anonymous namespace)::ExprEvaluatorBase<(anonymous
namespace)::LValueExprEvaluator>::VisitCXXDefaultArgExpr (this=0x7fffffff9000,
E=0xb7e2648)
    at /usr/local/google/home/nlewycky/llvm/src/tools/clang/lib/AST/ExprConstant.cpp:4482
4482        { return StmtVisitorTy::Visit(E->getExpr()); }
(gdb)
#6  0x0000000006a677f5 in clang::StmtVisitorBase<clang::make_const_ptr,
(anonymous namespace)::LValueExprEvaluator, bool>::Visit (this=0x7fffffff9000,
S=0xb7e21f8)
    at tools/clang/include/clang/AST/StmtNodes.inc:591
591 MATERIALIZETEMPORARYEXPR(MaterializeTemporaryExpr, Expr)
(gdb)
#5  0x0000000006a4787c in (anonymous
namespace)::LValueExprEvaluator::VisitMaterializeTemporaryExpr
    (this=0x7fffffff9000, E=0xb7e21f8)
    at /usr/local/google/home/nlewycky/llvm/src/tools/clang/lib/AST/ExprConstant.cpp:5182
5182            createTemporary(E, E->getStorageDuration() == SD_Automatic);

Richard, thoughts?

[Quuxplusone]

Looks like we're going to need to track more information about an LValue that
denotes (a subobject of) a materialized temporary -- the Expr* and call frame
index are insufficient in the case where a single MaterializeTemporaryExpr can
correspond to multiple live objects in the same stack frame (through
CXXDefaultArgExprs or CXXDefaultInitExprs).

We could consider creating separate instances of MaterializeTemporaryExprs for
each time the default argument / default init is *used*, with some kind of
mapping scheme to go from an MTE within the initializer for a CXXDefault*Expr
to the per-usage version of it. It looks like that's the cleanest way to
properly support lifetime extension of MTEs within CXXDefaultInitExprs in
nested aggregate initialization [1], but it might be more expensive than we'd
like if we also apply that to non-lifetime-extended temporaries.

I think we also have this problem for ArrayLoopInitExpr and probably for the
array filler in an InitListExpr, but in those cases we should use a different
solution: temporaries created in an array init loop should be destroyed at the
end of the loop iteration.

 [1]: Example of this:

struct A { int &&r = 0; };
struct B { A x, y; };
B b = {};

... should lifetime-extend two int temporaries (nested within
CXXDefaultInitExprs with the same underlying expression), but Clang currently
gets this wrong and actually lifetime-extends no temporaries.

[Quuxplusone]

We're seeing the same bug show up when compiling the Linux kernel. The assert
occurs when compiling linux/kernel/irq/irqdesc.c (in version 3.19 and 4.2). The
code causing the assert is:

struct irq_desc irq_desc[NR_IRQS] __cacheline_aligned_in_smp = {
        [0 ... NR_IRQS-1] = {
                .handle_irq     = handle_bad_irq,
                .depth          = 1,
                .lock           = __RAW_SPIN_LOCK_UNLOCKED(irq_desc->lock),
        }
}

I have a smaller test case as well, if needed.

typedef struct Y {
  unsigned int c;
} Y_t;
struct X {
  Y_t a;
};
struct X foo[2] = { [0 ... 1] = { .a = (Y_t) { .c = 0 } } };

[Quuxplusone]

Attached llvm-bug.txt (11201 bytes, text/plain): stack trace

[Quuxplusone]

I see the same assertion when compiling the Linux kernel 4.4 at cpufeature.c

[Quuxplusone]

(In reply to Brendon Cahoon from comment #3)
> typedef struct Y {
>   unsigned int c;
> } Y_t;
> struct X {
>   Y_t a;
> };
> struct X foo[2] = { [0 ... 1] = { .a = (Y_t) { .c = 0 } } };

This looks like a different bug from the one in comment#0. In this case, our
GNU array range designator support has ended up copying the same Expr* into
multiple slots in the initializer of foo, and we attempt to create the same
compound literal temporary object multiple times when evaluating the
initializer.

But this code is skating on *very* thin ice: minor changes to it result in code
that both GCC and Clang miscompile in the same way, for the same reason that
we're asserting here. Example:

struct Y { unsigned int c; };
struct X { struct Y *p; };
struct X foo[2] = { [0 ... 1] = { .p = &(struct Y){0} } };

Per normal C rules, the above compound literal(s) has/have static storage
duration. The question is, how many such static storage duration objects exist
here? The code assumes that each compound literal is a distinct object. But
both implementations only create a *single* object here.

If indeed the semantics of the original code are that there is only a single
object, then it violates the sequencing rules and thus has undefined behavior
(we still shouldn't be crashing on it though). If the semantics of the original
code are "as if the same initializer were repeated N times" then we're going to
need to do something altogether more horrible to get the expression evaluator
and IR generation to create multiple compound literal globals.

[Quuxplusone]

Reverted r303316 in r305223. Sorry for the long delay time.

Please leave this bug open to track the issues that were uncovered, those bugs are still bugs and haven't gone away just because the patch which exposed them was reverted.

[Quuxplusone]

(In reply to Richard Smith from comment #6)
> Per normal C rules, the above compound literal(s) has/have static storage
> duration. The question is, how many such static storage duration objects
> exist here? The code assumes that each compound literal is a distinct
> object. But both implementations only create a *single* object here.

From the gcc manual: "If the value in it has side-effects, the side-effects
happen only once, not for each initialized field by the range initializer."
(See also http://reviews.llvm.org/rL63327.)

(In reply to Nick Lewycky from comment #7)
> Reverted r303316 in r305223. Sorry for the long delay time.
>
> Please leave this bug open to track the issues that were uncovered, those
> bugs are still bugs and haven't gone away just because the patch which
> exposed them was reverted.

r305233 doesn't fix the regression for the testcase in comment 3.

[Quuxplusone]

Thanks Eli.

Continued rolling back with r305239.

[Quuxplusone]

I also hit this bug while compiling Qt with a version of clang that was just
before the latest reverts (cherry-picking them fixes the crash).
It reduces to the following test case which crashes with the same assertion
even though there is no constexpr in it:

// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -o - -emit-llvm %s |
FileCheck %s
int a(const int &i = int()) {
  return i;
}

bool b(void) {
  return a() == a();
}