Skip to content

Conversation

@hughbe
Copy link
Contributor

@hughbe hughbe commented Mar 25, 2017

The C++ standard states that a move constructor is called when returning a value from a method unless the compiler is able to elide the move,

For example, the following code invokes undefined behaviour:

#include <iostream>

class CallEmission {
public:
    bool assumedPlusZeroSelf;
    CallEmission(bool assumedPlusZeroSelf) : assumedPlusZeroSelf(assumedPlusZeroSelf) {}
    
    CallEmission(CallEmission &&e) {
        std::cout << "Called move constructor" << "\n";
    }
};

CallEmission method() {
    CallEmission x(false);
    return x;
}

int main() {
    CallEmission x = method();
    std::cout << x.assumedPlusZeroSelf;
}

MSVC:

Called move constructor
140

Clang:

0

As you can see, this causes us to access uninitialized memory. Later in some code, we have the following assertion:
assert(!assumedPlusZeroSelf)

This fails on Windows when compiling the standard library, as assumedPlusZeroSelf is true:

Assertion failed: !assumedPlusZeroSelf, file C:\Users\hughb\Documents\GitHub\swift\swift\lib\SILGen\SILGenApply.cpp, line 4314
Wrote crash dump file "C:\Users\hughb\AppData\Local\Temp\swiftc.exe-8e89c1.dmp"
0x00007FF624421C4C (0x0000006200000016 0x00007FFA791FADF0 0x00007FF61B97077D 0x0000000000000000), HandleAbort() + 0xC bytes(s), c:\users\hughb\documents\github\swift\llvm\lib\support\windows\signals.inc, line 405
0x00007FFA790D1C81 (0x00007FFA00000016 0x00007FFA791B56F0 0x00007FFA791FADF0 0x00007FF624B80850), raise() + 0x441 bytes(s)
0x00007FFA790D38A9 (0x00007FFA791FADF0 0x000000628E783CD0 0x000000628E783D00 0x000000628E783D08), abort() + 0x39 bytes(s)
0x00007FFA790D908F (0x00007FF624B808E0 0x00007FF624B80850 0x00000247000010DA 0x00007FF62450A178), _get_wide_winmain_command_line() + 0x211F bytes(s)
0x00007FFA790D7061 (0x00007FF624B808E0 0x00007FF624B80850 0x00000247000010DA 0x00007FF61D24F901), _get_wide_winmain_command_line() + 0xF1 bytes(s)
0x00007FFA790D9A3F (0x00007FF624B808E0 0x00007FF624B80850 0xCCCCCCCC000010DA 0xCCCCCCCCCCCCCCCC), _wassert() + 0x2F bytes(s)
0x00007FF61D24F901 (0x000000628E7843B0 0x000000628E7842C8 0x000000628E7841E8 0x000000628E784208), `anonymous namespace'::CallEmission::applyEnumElementConstructor() + 0x61 bytes(s), c:\users\hughb\documents\github\swift\swift\lib\silgen\silgenapply.cpp, line 4314 + 0x2D byte(s)
0x00007FF61D2505DE (0x000000628E7843B0 0x000000628E7842C8 0x000000628E7841E8 0x000000628E784208), `anonymous namespace'::CallEmission::applyFirstLevelCallee() + 0x21E bytes(s), c:\users\hughb\documents\github\swift\swift\lib\silgen\silgenapply.cpp, line 4253 + 0x64 byte(s)
0x00007FF61D2790D0 (0x000000628E7843B0 0x000000628E784628 0x0000000000000000 0xCCCCCCCCCCCCCCCC), `anonymous namespace'::CallEmission::apply() + 0x150 bytes(s), c:\users\hughb\documents\github\swift\swift\lib\silgen\silgenapply.cpp, line 4158
0x00007FF61D244137 (0x000000628E78A330 0x000000628E784628 0x00000247E54E2C50 0x0000000000000000), swift::Lowering::SILGenFunction::emitApplyExpr() + 0x77 bytes(s), c:\users\hughb\documents\github\swift\swift\lib\silgen\silgenapply.cpp, line 4694 + 0x32 byte(s)
0x00007FF61D1DD719 (0x000000628E7845D8 0x000000628E784628 0x00000247E54E2C50 0x0000000000000000), `anonymous namespace'::RValueEmitter::visitApplyExpr() + 0x49 bytes(s), c:\users\hughb\documents\github\swift\swift\lib\silgen\silgenexpr.cpp, line 470 + 0x1C byte(s)
...

@gottesmm this fixes the issue we discussed on email

@hughbe
Copy link
Contributor Author

hughbe commented Mar 25, 2017

@swift-ci please smoke test

@hughbe hughbe changed the title Fix undefined behaviour caused by accessing uninitialized field in CallEmission Fix undefined behaviour caused by accessing uninitialized field when moving CallEmission Mar 25, 2017
@hughbe hughbe changed the title Fix undefined behaviour caused by accessing uninitialized field when moving CallEmission Fix undefined behaviour caused by accessing uninitialized field after moving CallEmission Mar 25, 2017
@slavapestov
Copy link
Contributor

You're awesome.

@slavapestov slavapestov merged commit 23c0fea into swiftlang:master Mar 25, 2017
@hughbe hughbe deleted the silgenapply-assertion-fix branch March 25, 2017 07:44
@gottesmm
Copy link
Contributor

@hughbe Nice find! I wonder if there is a warning for this.

@hughbe
Copy link
Contributor Author

hughbe commented Mar 27, 2017

@gottesmm seemingly not with -Wall -Wextra or the static analyzer. I'll open a bug report to suggest this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants