838803 - nsRunnableMethod's specialization for __stdcall should only be declared on Win32 and OS2

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Seth Fowler [:seth] [:s2h]]

The __stdcall check used in the configure script currently passes on the version of clang provided by Apple on OS X Mountain Lion. Clang doesn't signal any kind of error, even though __stdcall is not supported on the platform. This is due to an upstream bug which has been fixed. Fortunately, quite possibly due to the same bug, the resulting redefinition in nsThreadUtils.h doesn't cause a compilation error. Unfortunately, it _does_ cause an error when using static analysis tools based upon more recent versions of the clang source code.

It would be preferable to eliminate this issue entirely by getting rid of the __stdcall check in configure. We don't even run this check on Win32, one of the two platforms that support __stdcall. (The other being OS2; not sure what happens there.) We already have perfectly good checks to identify the platform we're building on; I propose simply switching the single use of HAVE_STDCALL in the codebase to check if XP_WIN32 or XP_OS2 are defined.

Comment 1

[Seth Fowler [:seth] [:s2h]]

I had a patch for this but it seems that someone has already fixed this issue. Closing.

Comment 2

[Benoit Girard (:BenWa)]

Still seeing this with YouCompleteMe

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think you have the wrong bug ...

Comment 4

[Seth Fowler [:seth] [:s2h]]

Actually YouCompleteMe was where I noticed this problem myself. I don't think there's any reason to assume that Benoit has the wrong bug here...

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok apparently I just have no clue what YouCompleteMe is ...

Comment 6

[Benoit Girard (:BenWa)]

Yes, I'm reopening. As I understand it YouCompleteMe uses an extra recent version of Clang to generate proper completion information. This is what it reports on nearly all files because nsThreadUtils is everyone:

  1 nsThreadUtils.h|326 col 34 warning| calling convention '__stdcall' ignored for this target                                                       
  2 nsThreadUtils.h|326 col 8 error| redefinition of 'nsRunnableMethodTraits<type-parameter-0-1 (type-parameter-0-0::*)(), Owning>'

I think it aborts the parsing of nsThreadUtils.h after this error so a few threading functions don't complete. The rest works fine luckly.

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Ok apparently I just have no clue what YouCompleteMe is ...

With a proper configuration file (which I have a script to generate), it's gives nearly perfect completion suggestions for mozilla on-the-fly.

Comment 7

[Benoit Girard (:BenWa)]

is everywhere*

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Only check stdcall for winnt && os2

This fixes the remaining YouCompleteMe error.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 776631 [details] [diff] [review]
Only check stdcall for winnt && os2

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

::: configure.in
@@ -2641,5 @@
> -if test "$ac_cv___stdcall" = true ; then
> -  AC_DEFINE(HAVE_STDCALL)
> -  AC_MSG_RESULT(yes)
> -else
> -  AC_MSG_RESULT(no)

Just remove this and replace the #ifdef HAVE_STDCALL in nsThreadUtils.h with #if NS_STDCALL, and __stdcall with NS_STDCALL.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=a9dc3119435f

Comment 11

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecfcb0796854

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ecfcb0796854

Comment 13

[Jacek Caban]

Attachment: win64 fix

The check was there for a reason. On win64, __stdcall is no-op (there is no separated calling convention), so this results in an error on mingw:

 4:02.74 /home/jacek/mozilla/mozilla-central/xpcom/glue/nsThreadUtils.h:326:8: error: redefinition of 'struct nsRunnableMethodTraits<R (C::*)(), Owning>'
 4:02.74  struct nsRunnableMethodTraits<R (NS_STDCALL C::*)(), Owning> {
 4:02.74         ^
 4:02.74 /home/jacek/mozilla/mozilla-central/xpcom/glue/nsThreadUtils.h:318:8: error: previous definition of 'struct nsRunnableMethodTraits<R (C::*)(), Owning>'
 4:02.74  struct nsRunnableMethodTraits<R (C::*)(), Owning> {
 4:02.74         ^

I expect MSVC to give similar error. AFAIR, that was the original reason for this configure check. The attached patch should help.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 777645 [details] [diff] [review]
win64 fix

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

This is tracked in bug 895083