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

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: dmajor, Unassigned)

Tracking

42 Branch
mozilla43
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed, firefox47 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8639423 [details] [diff] [review]
workaround

Ehsan, does my analysis in comment 0 seem reasonable to you?
Attachment #8639423 - Flags: feedback?(ehsan)

Comment 2

3 years ago
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?
Flags: needinfo?(dmajor)

Updated

3 years ago
Attachment #8639423 - Flags: feedback?(ehsan) → feedback-
(Reporter)

Comment 3

3 years ago
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.
(Reporter)

Comment 4

3 years ago
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)
Flags: needinfo?(dmajor)
(Reporter)

Comment 5

3 years ago
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)
(Reporter)

Comment 6

3 years ago
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.
(Reporter)

Comment 7

3 years ago
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.
(Reporter)

Comment 9

3 years ago
Created attachment 8657240 [details] [diff] [review]
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.
Attachment #8639423 - Attachment is obsolete: true
Attachment #8657240 - Flags: review?(mh+mozilla)
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.
Attachment #8657240 - Flags: review?(mh+mozilla) → review+
(Reporter)

Updated

3 years ago
Blocks: 1203096
https://hg.mozilla.org/mozilla-central/rev/71d29e7967e5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Comment 13

3 years ago
We need to work around this by making sandboxTarget::Instance() out of line...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 14

3 years ago
Created attachment 8717150 [details] [diff] [review]
Move the definition of sandboxTarget::Instance() out-of-line

This is required so that delay-loading xul.dll works with clang-cl.
Attachment #8717150 - Flags: review?(bobowen.code)

Comment 15

3 years ago
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?
Attachment #8717150 - Flags: review?(mh+mozilla)
Attachment #8717150 - Flags: review?(bobowen.code)
Attachment #8717150 - Flags: review+

Comment 16

3 years ago
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

3 years ago
(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

3 years ago
(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

3 years ago
Created attachment 8717596 [details] [diff] [review]
Move the definition of sandboxTarget::Instance() out-of-line

This is required so that delay-loading xul.dll works with clang-cl.
Attachment #8717596 - Flags: review?(mh+mozilla)

Updated

3 years ago
Attachment #8717150 - Attachment is obsolete: true
Attachment #8717150 - Flags: review?(mh+mozilla)

Comment 20

3 years ago
(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
Attachment #8717596 - Flags: review?(mh+mozilla) → review+

Updated

3 years ago
Duplicate of this bug: 1203096

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/625bfa7b1c88
https://hg.mozilla.org/mozilla-central/rev/3a72236db31e
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.