The failure started with https://chromium-review.googlesource.com/897174

+rnk, maybe you'd like to take a quick look at this?

There seems to be something fishy going on.

In the CL, yhanada had to add InputMethodEngine::ProcessKeyEvent() which just forwards to the base class in order to avoid this link error:

"error LNK2001: unresolved external symbol "[thunk]:public: virtual void __cdecl input_method::InputMethodEngineBase::ProcessKeyEvent`vtordisp{4294967292,56}' (class ui::KeyEvent const &,class base::OnceCallback<void __cdecl(bool)>)" (?ProcessKeyEvent@InputMethodEngineBase@input_method@@$4PPPPPPPM@DI@EAAXAEBVKeyEvent@ui@@V?$OnceCallback@$$A6AX_N@Z@base@@@Z)
./chrome.dll : fatal error LNK1120: 1 unresolved externals"

which smells like it might be a Clang problem. Adding the extra method fixed the link error, but I suspect something is still wrong, causing the ASan error which does not seem to make sense; it seems to me that something is going wrong with the call here.

Thanks for your help! I couldn't find any suspicious code so far.

The link error can be reproduced with this diff:

diff --git a/chrome/browser/ui/input_method/input_method_engine.cc b/chrome/browser/ui/input_method/input_method_engine.cc
index 6965dbdbc175..8f1a71902b58 100644
--- a/chrome/browser/ui/input_method/input_method_engine.cc
+++ b/chrome/browser/ui/input_method/input_method_engine.cc
@@ -223,12 +223,6 @@ bool InputMethodEngine::SendKeyEvent(ui::KeyEvent* event,
   return true;
 }

-void InputMethodEngine::ProcessKeyEvent(const ui::KeyEvent& key_event,
-                                        KeyEventDoneCallback callback) {
-  // This override is neeeded to prevent a link failure on Windows build.
-  InputMethodEngineBase::ProcessKeyEvent(key_event, std::move(callback));
-}
-
 bool InputMethodEngine::IsSpecialPage(ui::InputMethod* input_method) {
   Browser* browser = chrome::FindLastActive();
   DCHECK(browser);
diff --git a/chrome/browser/ui/input_method/input_method_engine.h b/chrome/browser/ui/input_method/input_method_engine.hindex 27648ee77985..2b3de7c51070 100644
--- a/chrome/browser/ui/input_method/input_method_engine.h
+++ b/chrome/browser/ui/input_method/input_method_engine.h
@@ -50,8 +50,6 @@ class InputMethodEngine : public InputMethodEngineBase,
   void CommitTextToInputContext(int context_id,
                                 const std::string& text) override;
   bool SendKeyEvent(ui::KeyEvent* ui_event, const std::string& code) override;
-  void ProcessKeyEvent(const ui::KeyEvent& key_event,
-                       KeyEventDoneCallback callback) override;

  private:
   // ui::ImeWindowObserver:



args.gn:

is_clang = true
is_component_build = false
is_debug = false
symbol_level = 0
target_cpu = "x86"

ninja -C out\release unit_tests


However, the link error does not occur when building with MSVC.

Uh-oh, sounds like a this-adjustment bug... Looks like inalloca is involved too judging by the std::move. Good times.

Here is a reduction for the link error:

```
// t.h
struct Incomplete;
struct A {
  A() {}
  virtual ~A() {}
  virtual void asdf() = 0;
  virtual void foo(Incomplete p) = 0;
  int a;
};
struct B : virtual A {
  B();
  ~B() override;
  void foo(Incomplete p) override;
  virtual void bar();
  int b;
};
struct C {
  C() {}
  virtual ~C() {}
  virtual void baz() = 0;
  int c;
};
struct D : B, C {
  D();
  ~D() override;
  void asdf() override {}
  //void foo(Incomplete p) override;
  void bar() override;
  void baz() override;
  int d;
};

// a.cpp
#include "t.h"
D::D() {}
D::~D() {}
//void D::foo(Incomplete p) {}
void D::bar() {}
void D::baz() {}

// b.cpp
#include "t.h"
struct Incomplete { int i; };
B::B() {}
B::~B() {}
void B::bar() {}
void B::foo(Incomplete p) {}
int main() {}

$ clang-cl -c a.cpp b.cpp && link a.obj b.obj -out:t.exe -nologo
a.obj : error LNK2001: unresolved external symbol "[thunk]:public: virtual void __cdecl B::foo`vtordisp{4294967292,24}' (struct SmartPtr<int>)" (?foo@B@@$4PPPPPPPM@BI@EAAXU?$SmartPtr@H@@@Z)
t.exe : fatal error LNK1120: 1 unresolved externals
```

The record layouts for D and B are interesting:

*** Dumping AST Record Layout
         0 | struct B
         0 |   (B vftable pointer)
         8 |   (B vbtable pointer)
        16 |   int b
        28 |   (vtordisp for vbase A)
        32 |   struct A (virtual base)
        32 |     (A vftable pointer)
        40 |     int a
           | [sizeof=48, align=8,
           |  nvsize=24, nvalign=8]

*** Dumping AST Record Layout
         0 | struct D
         0 |   struct B (primary base)
         0 |     (B vftable pointer)
         8 |     (B vbtable pointer)
        16 |     int b
        24 |   struct C (base)
        24 |     (C vftable pointer)
        32 |     int c
        40 |   int d
        52 |   (vtordisp for vbase A)
        56 |   struct A (virtual base)
        56 |     (A vftable pointer)
        64 |     int a
           | [sizeof=72, align=8,
           |  nvsize=48, nvalign=8]

'foo' here corresponds to ProcessKeyEvent. The primary definition of 'B::foo' hardcodes a -32 this adjustment, which adjusts from A-in-C to the beginning of A.

A is a virtual base, so its location changes relative to B in D's layout. In D's layout, the offset from A to B is -56, not -32.

D's vftable slot for 'foo' needs to use a thunk to do an additional 24 byte adjustment so that -24 - 32 = -56, and B::foo's 'this' argument is correctly positioned. That's where the 24 in "vtordisp{*,24}" comes from.

So far, Clang implements all that stuff correctly.

The link error comes from the fact we don't have a definition for the Incomplete struct, so we give up when attempting to emit the thunk that does the adjustment. We don't emit an error because this code was originally written to support available_externally vtables for Itanium, so it was just an optimization, not a correctness issue.

The fix is probably to abuse musttail to emit a thunk without knowing the prototype for the virtual method.

---

As far as the ASan error, I have no idea what's up yet. Adding the method as a workaround should've worked.

I filed that upstream: https://bugs.llvm.org/show_bug.cgi?id=36952

I'll look into the ASan error after fixing it.

Looks like that thunk issue was filed a long time ago and we never fixed it: https://bugs.llvm.org/show_bug.cgi?id=25641 :(

Anyway, it's fixed now in r329009. We can remove the workaround after the next clang roll:  https://crbug.com/828582 

I still don't know what's up with the ASan report yet.

This test is still red, with the same stack. Started

https://luci-milo.appspot.com/buildbot/chromium.clang/CrWinAsan/378

Still red today:

https://luci-milo.appspot.com/buildbot/chromium.clang/CrWinAsan/524


Is it expected that this still fails? Sounds like the asan bit wasn't investigated.

It looks like the thunk workaround is still in but could be removed now, but doing that probably won't help with the test? (Probably still should still remove it -- Hans, want to give it a shot since you know what exactly to remove?)

I'll send a patch to remove the thunk.

That may end up fixing the asan error too, but only by masking the underlying asan problem :-/

https://chromium-review.googlesource.com/c/chromium/src/+/1037343
It also makes the asan error go away.

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

commit fcc2bf3d910207fe591a81e17463461be4287304
Author: Hans Wennborg <hans@chromium.org>
Date: Tue May 01 16:21:20 2018

Remove the InputMethodEngine::ProcessKeyEvent thunk

It was there to work around a link error caused by a compiler bug,
which has since been fixed.

Bug:  822202 
Change-Id: Iaeaa061fcd5018efc11d4132f64f5d1d6d446ddb
Reviewed-on: https://chromium-review.googlesource.com/1037343
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555063}
[modify] https://crrev.com/fcc2bf3d910207fe591a81e17463461be4287304/chrome/browser/ui/input_method/input_method_engine.cc
[modify] https://crrev.com/fcc2bf3d910207fe591a81e17463461be4287304/chrome/browser/ui/input_method/input_method_engine.h

Looks like removing the manual thunk made the bot go green:
https://luci-milo.appspot.com/buildbot/chromium.clang/CrWinAsan/526

This may be papering over a bug, but I think it's more likely to be in the virtual adjustment code than it is to be in ASan. I think we should close this anyway. If we want to find bugs in MS ABI this adjustment code, there are easier ways to do that with targeted fuzzing. Building Chromium unit_tests with ASan is a very roundabout way to find such bugs.