We had green runs on that bot with clang=1 before: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/430268
(from May 18).

Actually, we had a roll on May 18 too.. I wonder if that build is before that one.

That green May 18 tryjob was against Chromium #472709

The May 18 roll landed in #473087 (https://codereview.chromium.org/2897453002)

But reverting that locally doesn't seem to help.

I'll start bisecting...

Still running bisect, but I expect it'll end up at https://codereview.chromium.org/2707903002 which added the dcheck.

Is that the right link? I don't see a new line with a Scheck in there.

correction, it didn't add the dcheck, but the if-statement before it.


And actually, bisection points to this V8 roll: https://codereview.chromium.org/2887193003

Which doesn't include the V8 change mentioned in #3.

Let me try something else..

It's this one: https://chromium-review.googlesource.com/c/506016/

It adds a new entry to the ExternalReference table.

I guess somehow that ends up being a duplicate? Not sure where win64-clang comes in here though..

One theory would be that clang generates identical code for internal::libc_memcpy and memmove and that the linker then ICF's them to the same address, but that doesn't seem like the case:

From obj\v8\v8_base\assembler.obj:

?libc_memmove@internal@v8@@YAPEAXPEAXPEBX_K@Z (void * __cdecl v8::internal::libc_memmove(void *,void const *,unsigned __int64)):
  0000000000000000: 56                 push        rsi
  0000000000000001: 48 83 EC 20        sub         rsp,20h
  0000000000000005: 48 89 CE           mov         rsi,rcx
  0000000000000008: E8 00 00 00 00     call        memmove
  000000000000000D: 48 89 F0           mov         rax,rsi
  0000000000000010: 48 83 C4 20        add         rsp,20h
  0000000000000014: 5E                 pop         rsi
  0000000000000015: C3                 ret

?libc_memcpy@internal@v8@@YAPEAXPEAXPEBX_K@Z (void * __cdecl v8::internal::libc_memcpy(void *,void const *,unsigned __int64)):
  0000000000000000: 56                 push        rsi
  0000000000000001: 48 83 EC 20        sub         rsp,20h
  0000000000000005: 48 89 CE           mov         rsi,rcx
  0000000000000008: E8 00 00 00 00     call        memcpy
  000000000000000D: 48 89 F0           mov         rax,rsi
  0000000000000010: 48 83 C4 20        add         rsp,20h
  0000000000000014: 5E                 pop         rsi
  0000000000000015: C3                 ret


Unless of course on windows memcpy and memmove were to be resolved to the same symbol, causing the functions to end up identical after all and get ICF'd...

Ha! It must be something like that:

diff --git a/src/assembler.cc b/src/assembler.cc
index 20a7b6c..0eb5a26 100644
--- a/src/assembler.cc
+++ b/src/assembler.cc
@@ -1552,6 +1552,7 @@ void* libc_memcpy(void* dest, const void* src, size_t n) {
 }

 ExternalReference ExternalReference::libc_memcpy_function(Isolate* isolate) {
+  fprintf(stderr, "libc_memcpy: %p\n", &libc_memcpy);
   return ExternalReference(Redirect(isolate, FUNCTION_ADDR(libc_memcpy)));
 }

@@ -1560,6 +1561,7 @@ void* libc_memmove(void* dest, const void* src, size_t n) {
 }

 ExternalReference ExternalReference::libc_memmove_function(Isolate* isolate) {
+  fprintf(stderr, "libc_memmove: %p\n", &libc_memmove);
   return ExternalReference(Redirect(isolate, FUNCTION_ADDR(libc_memmove)));
 }

C:\src\chromium\src>ninja -C out\release gen\v8\snapshot.cc
ninja: Entering directory `out\release'
[5/5] ACTION //v8:run_mksnapshot(//build/toolchain/win:clang_x64)
FAILED: gen/v8/snapshot.cc snapshot_blob.bin
c:/src/depot_tools/python276_bin/python.exe ../../v8/tools/run.py ./mksnapshot --startup_src gen/v8/snapshot.cc --random-seed 314159265 --startup_blob snapshot_blob.bin
libc_memmove: 00007FF721BBD800
libc_memmove: 00007FF721BBD800
libc_memcpy: 00007FF721BBD800
libc_memcpy: 00007FF721BBD800
libc_memmove: 00007FF721BBD800


#
# Fatal error in ../../v8/src/snapshot/serializer-common.cc, line 27
# Check failed: map_->Get(addr).IsNothing().
#

So, isn't this exactly what the CL from #3 is supposed to make work -- allowing duplicate external references?

No, that's only for functions added by ExternalReferenceTable::AddApiReferences

Probably time to cc a few v8 folks and let them figure it out.

Strange that the msvc build doesn't hit this, it has the same libc and linker.

Why isn't this a problem with msvc? Because they beat us at tail call optimization:

libc_memcpy:

  0000000000000000: 40 53              push        rbx
  0000000000000002: 48 83 EC 20        sub         rsp,20h
  0000000000000006: 48 8B D9           mov         rbx,rcx
  0000000000000009: E8 00 00 00 00     call        memcpy
  000000000000000E: 48 8B C3           mov         rax,rbx
  0000000000000011: 48 83 C4 20        add         rsp,20h
  0000000000000015: 5B                 pop         rbx
  0000000000000016: C3                 ret

libc_memove:

  0000000000000000: E9 00 00 00 00     jmp         memmove

+yangguo who fixed a similar problem with https://codereview.chromium.org/2707903002

and +tebbi who landed https://chromium-review.googlesource.com/c/506016/ which introduced this duplicate external function

I'm not sure what the correct fix would be, but since memcpy and memmove resolve to the same symbol in MS's runtime, perhaps the code should just always use one of them?

+jochen might find this interesting too :-)

I suspected something like this. We expect external references to be unique. Let's hide this dcheck behind a ifdef for MS and add a comment. And a TODO for me. I'll think about this when I'm back from leave.

yangguo: I've prepared a patch. Would you mind stamping if you're around?
 https://codereview.chromium.org/2906193002

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e

commit c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e
Author: hans <hans@chromium.org>
Date: Sat May 27 02:34:27 2017

Disable DCHECK for external reference address uniqueness on Windows

The memcpy and memmove externals can end up at the same address; see bug for
details.

BUG=chromium:726896

Review-Url: https://codereview.chromium.org/2906193002
Cr-Commit-Position: refs/heads/master@{#45545}

[modify] https://crrev.com/c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e/src/snapshot/serializer-common.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e

commit c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e
Author: hans <hans@chromium.org>
Date: Sat May 27 02:34:27 2017

Disable DCHECK for external reference address uniqueness on Windows

The memcpy and memmove externals can end up at the same address; see bug for
details.

BUG=chromium:726896

Review-Url: https://codereview.chromium.org/2906193002
Cr-Commit-Position: refs/heads/master@{#45545}

[modify] https://crrev.com/c05ca9d7b8ef74d73959a3d582c26dbdbf5f0c7e/src/snapshot/serializer-common.cc

+machenbach: looking at https://v8-roll.appspot.com/, it seems the roller is currently closed?

Maybe related to https://chromium-review.googlesource.com/515462 (now resolved I think)? But maybe something else.

Our roll into chromium is closed doe to branch time and some branching difficulties. Will reopen latest tomorrow I think.

This was rolled as:
https://chromium.googlesource.com/chromium/src/+/2a97a5d29f8b0518066e152d9f87ba5700954796

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51f8742b06870bb6e3b79ca3dca2663dbb7c4131

commit 51f8742b06870bb6e3b79ca3dca2663dbb7c4131
Author: Hans Wennborg <hans@chromium.org>
Date: Tue May 30 21:58:46 2017

mb: Enable dcheck_always_on for Clang(ToT)Win release bots

This would have caught the recent dcheck we hit in mksnapshot (see bug) and
gives us more coverage in general.

BUG=726896
R=thakis@chromium.org

Review-Url: https://codereview.chromium.org/2915633002 .
Cr-Commit-Position: refs/heads/master@{#475683}

[modify] https://crrev.com/51f8742b06870bb6e3b79ca3dca2663dbb7c4131/tools/mb/mb_config.pyl

Filed https://bugs.llvm.org/show_bug.cgi?id=33233 to figure out why LLVM isn't tail-calling both of these (not that it would change the outcome, but it would be nice if it did).

We're unblocked on this from Clang's point of view now. I'll reassign to yang if he wants to look at the bigger problem of external references at the same address eventually.

Removing blocker. yangguo, it'd be nice if this got fixed for real eventually.

I think we should simply allow external references that have same targets. So far the check has only been there so that we don't accidentally introduce duplicates, but I don't think we have a high risk of doing that anyways.