1188045 - clang-cl: error LNK1194: cannot delay-load 'xul.dll' due to import of data symbol mozilla::SandboxTarget::Instance(void)'::`2'::sb

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(Away)]

95:58.41 LINK : fatal error LNK1194: cannot delay-load 'xul.dll' due to import of data symbol '"__declspec(dllimport) class mozilla::SandboxTarget `public: static class mozilla::SandboxTarget * __cdecl mozilla::SandboxTarget::Instance(void)'::`2'::sb" (__imp_?sb@?1??Instance@SandboxTarget@mozilla@@SAPAV23@XZ@4V23@A)'; link without /DELAYLOAD:xul.dll

Apparently you can't delay-load a DLL if you import an object from it. I guess that's reasonable. So why isn't this a problem on MSVC?

I have a suspicion that clang-cl is propagating the class-wide TARGET_SANDBOX_EXPORT annotation down to the |sb| instance, and that MSVC doesn't. https://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/sandboxtarget/sandboxTarget.h#23

I guess it depends on your interpretation of whether these "::`2'::sb" variables are members of the class. You could argue that it's clang-cl's bug if they're not matching MSVC perfectly, but this seems like such an edge case that I don't mind just working around it.

Comment 1

[(Away)]

Attachment: workaround

Ehsan, does my analysis in comment 0 seem reasonable to you?

Comment 2

[(no longer active)]

No.  Clearly you're hitting a serious bug in how clang-cl handles exports, and they have had to fix a lot of things to match what MSVC does exactly, and this is another instance of the stuff that needs to be fixed there, so we shouldn't work around it in our code.  Do you mind creating a small test case please?

Comment 3

[(Away)]

Ok so I'm wrong about the _export_ side of this, because real-life xul.dll from MSVC _does_ export the 'sb' variable.

I guess the difference must be on the _import_ side. Attempting to create a test case.

Comment 4

[(Away)]

I haven't been able to find a minimal repro that shows a difference between compilers. On this toy example, either both compilers import the variable, or both don't, depending on whether the Instance() function gets inlined. It's starting to look like we just got lucky on real-world MSVC builds, with cl choosing not to inline SandboxTarget::Instance().

$ cat test.cpp
class __declspec(dllimport) Test
{
public:
  void Hello();

  static Test* Instance()
  {
    static Test singleton;
    return &singleton;
  }
};

int main()
{
  Test::Instance()->Hello();
  return 0;
}

$ (cl -nologo -c test.cpp && dumpbin -all test.obj) | grep singleton

$ (cl -O2 -nologo -c test.cpp && dumpbin -all test.obj) | grep singleton
 00000002  DIR32                      00000000         C  __imp_?singleton@?1??I
nstance@Test@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static
class Test * __cdecl Test::Instance(void)'::`2'::singleton)
00C 00000000 UNDEF  notype       External     | __imp_?singleton@?1??Instance@Te
st@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static class Test
 * __cdecl Test::Instance(void)'::`2'::singleton)

$ (clang-cl -nologo -c test.cpp && dumpbin -all test.obj) | grep singleton

$ (clang-cl -O2 -nologo -c test.cpp && dumpbin -all test.obj) | grep singleton
 00000002  DIR32                      00000000         A  __imp_?singleton@?1??I
nstance@Test@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static
class Test * __cdecl Test::Instance(void)'::`2'::singleton)
00A 00000000 UNDEF  notype       External     | __imp_?singleton@?1??Instance@Te
st@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static class Test
 * __cdecl Test::Instance(void)'::`2'::singleton)

Comment 5

[(Away)]

Marking Instance() as noinline fixes this on cl, but not on clang-cl.

$ cat test.cpp
class __declspec(dllimport) Test
{
public:
  void Hello();

  __declspec(noinline) static Test* Instance()
  {
    static Test singleton;
    return &singleton;
  }
};

int main()
{
  Test::Instance()->Hello();
  return 0;
}

$ (cl -nologo -c -O2 test.cpp && dumpbin -all test.obj) | grep singleton

$ (clang-cl -nologo -c -O2 test.cpp && dumpbin -all test.obj) | grep singleton
 00000002  DIR32                      00000000         A  __imp_?singleton@?1??I
nstance@Test@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static
class Test * __cdecl Test::Instance(void)'::`2'::singleton)
00A 00000000 UNDEF  notype       External     | __imp_?singleton@?1??Instance@Te
st@@SAPAV2@XZ@4V2@A (__declspec(dllimport) class Test `public: static class Test
 * __cdecl Test::Instance(void)'::`2'::singleton)

Comment 6

[(Away)]

I tried moving the definition outside the class, but clang wasn't too happy:

$ cat test.cpp
class __declspec(dllimport) Test
{
public:
  void Hello();

  static Test* Instance();
};

__declspec(noinline) Test* Test::Instance()
{
  static Test singleton;
  return &singleton;
}

int main()
{
  Test::Instance()->Hello();
  return 0;
}

$ clang-cl -c -O2 test.cpp
test.cpp(9,34) :  warning: 'Test::Instance' redeclared without 'dllimport'
      attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
__declspec(noinline) Test* Test::Instance()
                                 ^
test.cpp(6,16) :  note: previous declaration is here
  static Test* Instance();
               ^
test.cpp(1,18) :  note: previous attribute is here
class __declspec(dllimport) Test
                 ^
1 warning generated.

Comment 7

[(Away)]

So to appease it I tried to say __declspec(dllimport) a second time, but apparently that doesn't go with noinline. Maybe that explains why the noinline was ignored in comment 5.

$ cat test.cpp
class __declspec(dllimport) Test
{
public:
  void Hello();

  static Test* Instance();
};

__declspec(noinline) __declspec(dllimport) Test* Test::Instance()
{
  static Test singleton;
  return &singleton;
}

int main()
{
  Test::Instance()->Hello();
  return 0;
}

$ clang-cl -c -O2 test.cpp
test.cpp(9,56) :  error: dllimport cannot be applied to non-inline function
      definition
__declspec(noinline) __declspec(dllimport) Test* Test::Instance()
                                                       ^
1 error generated.

Comment 8

[(no longer active)]

Filed https://llvm.org/bugs/show_bug.cgi?id=24291 for comment 5.

Comment 9

[(Away)]

Attachment: workaround: don't delayload

Since the LLVM bug doesn't seem to be going anywhere, Ehsan said I could work around it as long as I file a bug to remove later. I'll file such a bug before pushing if you approve.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8657240 [details] [diff] [review]
workaround: don't delayload

Review of attachment 8657240 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/app/moz.build
@@ +57,5 @@
>          'rlz',
>          'sandbox_staticruntime_s',
>      ]
> +
> +    # clang-cl can't deal with this delay-load due to bug 1188045

Please also add a link to the clang bug.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71d29e7967e5

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71d29e7967e5

Comment 13

[(no longer active)]

We need to work around this by making sandboxTarget::Instance() out of line...

Comment 14

[(no longer active)]

Attachment: Move the definition of sandboxTarget::Instance() out-of-line

This is required so that delay-loading xul.dll works with clang-cl.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8717150 [details] [diff] [review]
Move the definition of sandboxTarget::Instance() out-of-line

Review of attachment 8717150 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of nits / questions, although I'll defer to glandium over the build stuff.

Do we need to get rid of the changes in ipc/app/moz.build from the first patch as well?

::: security/sandbox/win/src/sandboxtarget/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +include('/ipc/chromium/chromium-config.mozbuild')

Can't we just include mozilla/Assertions.h in sandboxTarget.h?

I'm a little uneasy over this, we've normally tried to keep the sandboxing and ipc stuff separate, because of conflicts.
Although it doesn't seem to cause problems here.

::: security/sandbox/win/src/sandboxtarget/sandboxTarget.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

nit: this doesn't quite match the one in the guidelines.

@@ +6,5 @@
> +
> +#define TARGET_SANDBOX_EXPORTS
> +#include "sandboxTarget.h"
> +
> +using namespace mozilla;

Don't we normally define functions explicitly within a namespace definition?

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8717150 [details] [diff] [review]
Move the definition of sandboxTarget::Instance() out-of-line

Review of attachment 8717150 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/win/src/sandboxtarget/sandboxTarget.cpp
@@ +11,5 @@
> +
> +// We need to define this function out of line so that clang-cl doesn't inline
> +// it.
> +SandboxTarget*
> +/* static */ SandboxTarget::Instance()

Just noticed this as well.
I think this should be on the line above or even a separate one above that.

Comment 17

[(no longer active)]

(In reply to Bob Owen (:bobowen) from comment #15)
> Do we need to get rid of the changes in ipc/app/moz.build from the first
> patch as well?

Yes, that's the goal.

> ::: security/sandbox/win/src/sandboxtarget/moz.build
> @@ +3,5 @@
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +include('/ipc/chromium/chromium-config.mozbuild')
> 
> Can't we just include mozilla/Assertions.h in sandboxTarget.h?

Not sure how that helps...

> I'm a little uneasy over this, we've normally tried to keep the sandboxing
> and ipc stuff separate, because of conflicts.
> Although it doesn't seem to cause problems here.

Hmm, actually it seems like I don't need that include line at all.  I did it out of habit mostly!

> ::: security/sandbox/win/src/sandboxtarget/sandboxTarget.cpp
> @@ +1,2 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=2 et sw=2 tw=80: */
> 
> nit: this doesn't quite match the one in the guidelines.

Which guidelines?  I copied this from the .h file IIRC.  Happy to change it to whatever you suggest.

> @@ +6,5 @@
> > +
> > +#define TARGET_SANDBOX_EXPORTS
> > +#include "sandboxTarget.h"
> > +
> > +using namespace mozilla;
> 
> Don't we normally define functions explicitly within a namespace definition?

I've seen both.  I can do that if you prefer.

(In reply to Bob Owen (:bobowen) from comment #16)
> Comment on attachment 8717150 [details] [diff] [review]
> Move the definition of sandboxTarget::Instance() out-of-line
> 
> Review of attachment 8717150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/win/src/sandboxtarget/sandboxTarget.cpp
> @@ +11,5 @@
> > +
> > +// We need to define this function out of line so that clang-cl doesn't inline
> > +// it.
> > +SandboxTarget*
> > +/* static */ SandboxTarget::Instance()
> 
> Just noticed this as well.
> I think this should be on the line above or even a separate one above that.

D'oh, of course!  Good catch.

Comment 18

[(no longer active)]

(In reply to (Away 2/10-2/19) from comment #17)
> > > +include('/ipc/chromium/chromium-config.mozbuild')
> > 
> > Can't we just include mozilla/Assertions.h in sandboxTarget.h?
> 
> Not sure how that helps...


I meant to delete this part before submitting!  New patch coming up.

Comment 19

[(no longer active)]

Attachment: Move the definition of sandboxTarget::Instance() out-of-line

This is required so that delay-loading xul.dll works with clang-cl.

Comment 20

[Bob Owen (:bobowen)]

(In reply to (Away 2/10-2/19) from comment #17)
> (In reply to Bob Owen (:bobowen) from comment #15)

> > > +/* vim: set ts=2 et sw=2 tw=80: */
> > 
> > nit: this doesn't quite match the one in the guidelines.
> 
> Which guidelines?  I copied this from the .h file IIRC.  Happy to change it
> to whatever you suggest.

Just fyi, but I'll not lose sleep over this or the namespace thing. :-)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/625bfa7b1c88
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a72236db31e

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/625bfa7b1c88
https://hg.mozilla.org/mozilla-central/rev/3a72236db31e