This doesn't seem to be ToT-specific, it also affects non-ToT bots.

Looking at DevToolsProtocolTest.VirtualTimeTest first of all:

last good:

https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dll%29%20tester/builds/3226 (r429183)
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%20tester/builds/4820 (r429155)
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dbg%29%20tester/builds/3877 (r429184)
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dbg%29%20tester/builds/2919 (r429123)

first bad:

https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dll%29%20tester/builds/3227 (r429238)
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%20tester/builds/4821 (r429203)
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dbg%29%20tester/builds/3878 (r429212)
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dbg%29%20tester/builds/2920 (r429212)

So looks like failure range is 429184-429212.

To be more precise, 429184-429203.

These two seem like the only interesting commits in that range:

#429200: DevTools: bring hosted mode up-to-date with the new --custom-devtools-frontend
https://codereview.chromium.org/2469943002

#429195: [DevTools] migrate RenderingAgent and EmulationAgent to new style
https://codereview.chromium.org/2465343002

> These two seem like the only interesting commits in that range:

.. not that it makes much sense after looking at them :-/

Started a build of 429203 with 429195 reverted. Hoping it's that one as it seems to be the only relevant C++ change.

Reverting 429195 does appear to unbreak the test.

Copying obj/third_party/WebKit/Source/web/web/InspectorEmulationAgent.obj from my MSVC build tree to my Clang build tree makes the problem go away, and copying it from Clang into MSVC introduces the problem, so it looks like a problem with how we compile that object file.

It looks like we're miscompiling InspectorEmulationAgent::setVirtualTimePolicy(), putting that function in a clang-compiled file on its own causes the test to fail.

Specifically it looks like we're using the wrong calling convention for the Maybe<int> passed by value.

With this in i2.cpp:

template<typename T>
class MaybeBase {
public:
    MaybeBase() : m_isJust(false) { }
    MaybeBase(T value) : m_isJust(true), m_value(value) { }
    MaybeBase(MaybeBase&& other) : m_isJust(other.m_isJust), m_value(other.m_value) { }
    void operator=(T value) { m_value = value; m_isJust = true; }
    T fromJust() const { return m_value; }

protected:
    bool m_isJust;
    T m_value;
};

class Maybeint : public MaybeBase<int> {
public:
    Maybeint() { }
    Maybeint(int value) : MaybeBase(value) { }
    Maybeint(Maybeint&& other) : MaybeBase(other.m_value) { }
    using MaybeBase::operator=;
};

long long clangFromJust(Maybeint budget) {
  return static_cast<long long>(budget.fromJust());
}

run:

cl /c /Foi2cl.obj /O2 i2.cpp
..\..\third_party\llvm-build\Release+Asserts\bin\clang-cl /c /Foi2clang.obj /O2 i2.cpp

$ /home/pcc/h/llvm-build-rel-x64/bin/llvm-objdump -d -r i2cl.obj

i2cl.obj:       file format COFF-x86-64

Disassembly of section .text$mn:
?clangFromJust@@YA_JVMaybeint@@@Z:
       0:       48 63 41 04     movslq  4(%rcx), %rax
       4:       c3      retq
Disassembly of section .text$mn:
?fromJust@?$MaybeBase@H@@QEBAHXZ:
       0:       8b 41 04        movl    4(%rcx), %eax
       3:       c3      retq

$ /home/pcc/h/llvm-build-rel-x64/bin/llvm-objdump -d -r i2clang.obj

i2clang.obj:    file format COFF-x86-64

Disassembly of section .text:
?clangFromJust@@YA_JVMaybeint@@@Z:
       0:       48 c1 f9 20     sarq    $32, %rcx
       4:       48 89 c8        movq    %rcx, %rax
       7:       c3      retq

This more elaborate example shows that we get the caller side right but only for certain return value types (!):

template<typename T>
class MaybeBase {
public:
    MaybeBase() : m_isJust(false) { }
    MaybeBase(T value) : m_isJust(true), m_value(value) { }
    MaybeBase(MaybeBase&& other) : m_isJust(other.m_isJust), m_value(other.m_value) { }
    void operator=(T value) { m_value = value; m_isJust = true; }
    T fromJust() const { return m_value; }

protected:
    bool m_isJust;
    T m_value;
};

class Maybeint : public MaybeBase<int> {
public:
    Maybeint() { }
    Maybeint(int value) : MaybeBase(value) { }
    Maybeint(Maybeint&& other) : MaybeBase(other.m_value) { }
    using MaybeBase::operator=;
};

class foo {
public:
 long long clangFromJust(char *p, Maybeint budget);
 void clangFromJust2(char *p, Maybeint budget);
 long long clangFromJust3(char *p, Maybeint budget);
};

long long foo::clangFromJust(char *p, Maybeint budget) {
  return static_cast<long long>(budget.fromJust());
}

void call2(foo *f) {
  f->clangFromJust2(nullptr, 42);
}

void call3(foo *f) {
  f->clangFromJust3(nullptr, 42);
}

$ /home/pcc/h/llvm-build-rel-x64/bin/llvm-objdump -d -r i2clang.obj

i2clang.obj:    file format COFF-x86-64

Disassembly of section .text:
?clangFromJust@foo@@QEAA_JPEADVMaybeint@@@Z:
       0:       49 c1 f8 20     sarq    $32, %r8
       4:       4c 89 c0        movq    %r8, %rax
       7:       c3      retq
Disassembly of section .text:
?call2@@YAXPEAVfoo@@@Z:
       0:       48 83 ec 38     subq    $56, %rsp
       4:       48 8b 05 00 00 00 00    movq    (%rip), %rax
                0000000000000007:  IMAGE_REL_AMD64_REL32        __security_cookie
       b:       48 89 44 24 30  movq    %rax, 48(%rsp)
      10:       c6 44 24 28 01  movb    $1, 40(%rsp)
      15:       c7 44 24 2c 2a 00 00 00         movl    $42, 44(%rsp)
      1d:       4c 8d 44 24 28  leaq    40(%rsp), %r8
      22:       31 d2   xorl    %edx, %edx
      24:       e8 00 00 00 00  callq   0 <?call2@@YAXPEAVfoo@@@Z+0x29>
                0000000000000025:  IMAGE_REL_AMD64_REL32        ?clangFromJust2@foo@@QEAAXPEADVMaybeint@@@Z
      29:       48 8b 4c 24 30  movq    48(%rsp), %rcx
      2e:       e8 00 00 00 00  callq   0 <?call2@@YAXPEAVfoo@@@Z+0x33>
                000000000000002f:  IMAGE_REL_AMD64_REL32        __security_check_cookie
      33:       90      nop
      34:       48 83 c4 38     addq    $56, %rsp
      38:       c3      retq
Disassembly of section .text:
?call3@@YAXPEAVfoo@@@Z:
       0:       49 b8 01 00 00 00 2a 00 00 00   movabsq $180388626433, %r8
       a:       31 d2   xorl    %edx, %edx
       c:       e9 00 00 00 00  jmp     0 <?call3@@YAXPEAVfoo@@@Z+0x11>
                000000000000000d:  IMAGE_REL_AMD64_REL32        ?clangFromJust3@foo@@QEAA_JPEADVMaybeint@@@Z

$ /home/pcc/h/llvm-build-rel-x64/bin/llvm-objdump -d -r i2cl.obj

i2cl.obj:       file format COFF-x86-64

Disassembly of section .text$mn:
??0?$MaybeBase@H@@QEAA@H@Z:
       0:       c6 01 01        movb    $1, (%rcx)
       3:       48 8b c1        movq    %rcx, %rax
       6:       89 51 04        movl    %edx, 4(%rcx)
       9:       c3      retq
Disassembly of section .text$mn:
??0Maybeint@@QEAA@H@Z:
       0:       c6 01 01        movb    $1, (%rcx)
       3:       48 8b c1        movq    %rcx, %rax
       6:       89 51 04        movl    %edx, 4(%rcx)
       9:       c3      retq
Disassembly of section .text$mn:
?call2@@YAXPEAVfoo@@@Z:
       0:       48 83 ec 38     subq    $56, %rsp
       4:       4c 8d 44 24 20  leaq    32(%rsp), %r8
       9:       c6 44 24 20 01  movb    $1, 32(%rsp)
       e:       33 d2   xorl    %edx, %edx
      10:       c7 44 24 24 2a 00 00 00         movl    $42, 36(%rsp)
      18:       e8 00 00 00 00  callq   0 <?call2@@YAXPEAVfoo@@@Z+0x1D>
                0000000000000019:  IMAGE_REL_AMD64_REL32        ?clangFromJust2@foo@@QEAAXPEADVMaybeint@@@Z
      1d:       48 83 c4 38     addq    $56, %rsp
      21:       c3      retq
Disassembly of section .text$mn:
?call3@@YAXPEAVfoo@@@Z:
       0:       48 83 ec 38     subq    $56, %rsp
       4:       4c 8d 44 24 20  leaq    32(%rsp), %r8
       9:       c6 44 24 20 01  movb    $1, 32(%rsp)
       e:       33 d2   xorl    %edx, %edx
      10:       c7 44 24 24 2a 00 00 00         movl    $42, 36(%rsp)
      18:       e8 00 00 00 00  callq   0 <?call3@@YAXPEAVfoo@@@Z+0x1D>
                0000000000000019:  IMAGE_REL_AMD64_REL32        ?clangFromJust3@foo@@QEAA_JPEADVMaybeint@@@Z
      1d:       48 83 c4 38     addq    $56, %rsp
      21:       c3      retq
Disassembly of section .text$mn:
?clangFromJust@foo@@QEAA_JPEADVMaybeint@@@Z:
       0:       49 63 40 04     movslq  4(%r8), %rax
       4:       c3      retq
Disassembly of section .text$mn:
?fromJust@?$MaybeBase@H@@QEBAHXZ:
       0:       8b 41 04        movl    4(%rcx), %eax
       3:       c3      retq

The bug is probably in this code:
https://github.com/llvm-mirror/clang/blob/70c1976f0a6115c56ae72cd4bcdefa94dc390703/lib/CodeGen/MicrosoftCXXABI.cpp#L817

Maybe it produces different answers for MaybeBase<int> at different points in the TU.

I think the differing calling conventions in your second example arise because we cache CGFunctionInfo (clang's notion of the calling convention) keyed on the function type.

Thanks. I think you're right about the caching, if I remove the definition of foo::clangFromJust I get the right calling convention in call3, which would seem to indicate that we're caching the wrong answer if we need it for a definition.

It looks like this is what's going on here:

If we see a call first:
- Sema will try to find a constructor to convert the int to a Maybeint by calling Sema::LookupConstructors which will in turn call Sema::DeclareImplicitCopyConstructor.
- This will cause us to create an implicit deleted copy constructor for Maybeint because it has a user-declared move constructor.
- That later causes MicrosoftCXXABI::getRecordArgABI() to set CopyDeleted to true and return RAA_Indirect.

If we never needed to call DeclareImplicitCopyConstructor for Maybeint (which is the case if we see a definition first), MicrosoftCXXABI::getRecordArgABI() will not find the deleted copy constructor and we end up returning RAA_Default.

We probably need to make sure we call DeclareImplicitCopyConstructor more often in Microsoft mode.

This patch fixes my test case, but feels a bit hackish to me.

diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 06c6af1a..242c8fe 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7388,7 +7388,8 @@ void Sema::AddImplicitlyDeclaredMembersToClass(CXXRecordDecl *ClassDecl) {
     // determined while the class was being declared, force a declaration
     // of it now.
     if (ClassDecl->needsOverloadResolutionForCopyConstructor() ||
-        ClassDecl->hasInheritedConstructor())
+        ClassDecl->hasInheritedConstructor() ||
+        Context.getTargetInfo().getCXXABI().isMicrosoft())
       DeclareImplicitCopyConstructor(ClassDecl);
   }
 

Alternatively, I think I've been able to convince myself that this patch is correct:

diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 2722985..5c7eebb 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -831,8 +831,9 @@ MicrosoftCXXABI::getRecordArgABI(const CXXRecordDecl *RD) const {
       return RAA_Indirect;
 
     // We have a trivial copy constructor or no copy constructors, but we have
-    // to make sure it isn't deleted.
-    bool CopyDeleted = false;
+    // to make sure it isn't deleted. If we have a user-declared move
+    // constructor, the copy constructor is deleted unless explicitly defaulted.
+    bool CopyDeleted = RD->hasUserDeclaredMoveConstructor();
     for (const CXXConstructorDecl *CD : RD->ctors()) {
       if (CD->isCopyConstructor()) {
         assert(CD->isTrivial());

That's a reasonable work around, but from talking with Richard, there are other reasons why the copy might be deleted. It's really complicated, though. I'm in favor of applying the workaround.

Another test case that shows how confused clang is:

struct A {
  A() {}
  A(A &&o) = default;
  int val;
};
int getit(A a) { return a.val; }
A a; // constructing an A object declares the implicit copy ctor
int g;
void tryagain(A a) { g = a.val; }

We pass a directly in getit and indirectly in tryagain.

This test case still does the wrong thing with the patch in c#13, because the move constructor comes from A's base class:

struct B {
  B();
  B(B &&o) : b(o.b) {}
  int b;
};
struct A {
  A();
  B b;
  int val;
};
int getit(A a) { return a.val; }

Thanks. It sounds like we probably want something closer to c#15 then. I think I'll start with the below and see if I can push the call to DeclareImplicitCopyConstructor down to where it is needed.

diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 2722985..1be39c2 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -830,25 +830,20 @@ MicrosoftCXXABI::getRecordArgABI(const CXXRecordDecl *RD) const {
         getContext().getTypeSize(RD->getTypeForDecl()) > 64)
       return RAA_Indirect;
 
+    assert(!RD->needsImplicitCopyConstructor());
+
     // We have a trivial copy constructor or no copy constructors, but we have
     // to make sure it isn't deleted.
-    bool CopyDeleted = false;
     for (const CXXConstructorDecl *CD : RD->ctors()) {
       if (CD->isCopyConstructor()) {
         assert(CD->isTrivial());
         // We had at least one undeleted trivial copy ctor.  Return directly.
         if (!CD->isDeleted())
           return RAA_Default;
-        CopyDeleted = true;
       }
     }
 
-    // The trivial copy constructor was deleted.  Return indirectly.
-    if (CopyDeleted)
-      return RAA_Indirect;
-
-    // There were no copy ctors.  Return in RAX.
-    return RAA_Default;
+    return RAA_Indirect;
   }
 
   llvm_unreachable("invalid enum");
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 06c6af1a..242c8fe 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7388,7 +7388,8 @@ void Sema::AddImplicitlyDeclaredMembersToClass(CXXRecordDecl *ClassDecl) {
     // determined while the class was being declared, force a declaration
     // of it now.
     if (ClassDecl->needsOverloadResolutionForCopyConstructor() ||
-        ClassDecl->hasInheritedConstructor())
+        ClassDecl->hasInheritedConstructor() ||
+        Context.getTargetInfo().getCXXABI().isMicrosoft())
       DeclareImplicitCopyConstructor(ClassDecl);
   }
 

Filed upstream bug: https://llvm.org/bugs/show_bug.cgi?id=31049

Moving the call to DeclareImplicitCopyConstructor seems to be difficult because we need the property not only for calls and definitions but also for casts between function types (at least while pointers still have element types).

We could just do c#20 I suppose with a fixme?

It looks like DevToolsAgentTest.RuntimeCallFunctionOnRunMicrotasks is also passing with my patch applied, so it was probably the same root cause.

The fix landed upstream in r287600.

The ToT bots have turned green, so this is now blocked on the next Clang roll.

https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dbg%29%20tester/builds/3011
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%28dll%29%20tester/builds/3319
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin64%20tester/builds/6889

The roll landed a while ago.