1378529 - third_party/aom causes 'error: multiple storage classes in declaration specifiers' during MinGW compile

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tom Ritter [:tjr]]

Compiling with MinGW throws the following errors:

> /mingw-w64-build/firefox/third_party/aom/aom_ports/x86.h:121:1: error: multiple storage classes in declaration specifiers
>  static INLINE uint64_t xgetbv(void) {
>  ^
> /mingw-w64-build/firefox/third_party/aom/aom_ports/x86.h:169:1: error: multiple storage classes in declaration specifiers
>  static INLINE int x86_simd_caps(void) {
>  ^
> /mingw-w64-build/firefox/third_party/aom/aom_ports/x86.h:227:1: error: multiple storage classes in declaration specifiers
>  static INLINE unsigned int x86_readtsc(void) {
>  ^
> /mingw-w64-build/firefox/third_party/aom/aom_ports/x86.h:245:1: error: multiple storage classes in declaration specifiers
>  static INLINE uint64_t x86_readtsc64(void) {
>  ^
> /mingw-w64-build/firefox/third_party/aom/aom_ports/x86.h:310:1: error: multiple storage classes in declaration specifiers
>  static INLINE unsigned int x87_set_double_precision(void) {
>  ^

I was under the impression that the following were the only storage-class specifiers:

- typedef
- extern
- static
- auto
- register
- _Thread_local

INLINE gets defined to either 'inline' or '__forceinline'.  It appears[0] that mingw gives __forceinline an extern qualifier.

I suspect this is not going to be a project open to upstreaming, so my inclination is to do some sort of local or global undef or redefinition for forceinline...

[0] https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-git/0002-Undefine-FORCEINLINE-on-MinGW-w64-in-malloc.c.h.patch

Comment 1

[Tom Ritter [:tjr]]

Jacek, what do you think?

Comment 2

[Tom Ritter [:tjr]]

Thanks to :rillian, I figured out a better solution seems.

aom has a win64-gcc target that defines INLINE as 'inline' instead of '_forceinline'. That definition change fixes this. 

Unfortunately, aom does not have a win32-gcc target, so the goal is to upstream adding a win32-gcc target, then update and use that target for MinGW.

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

Review commit: https://reviewboard.mozilla.org/r/156234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156234/

Comment 4

[Tom Ritter [:tjr]]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156234/diff/1-2/

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

https://reviewboard.mozilla.org/r/156234/#review162598

::: media/libaom/config/win/mingw32/aom_config.asm:28
(Diff revision 2)
>  .equ HAVE_FEXCEPT ,  1
>  .equ HAVE_PTHREAD_H ,  0
>  .equ HAVE_WXWIDGETS ,  0
>  .equ CONFIG_DEPENDENCY_TRACKING ,  1
>  .equ CONFIG_EXTERNAL_BUILD ,  1
> -.equ CONFIG_INSTALL_DOCS ,  1
> +.equ CONFIG_INSTALL_DOCS ,  0

Is this spurious, maybe from importing a different revision?

::: media/libaom/generate_sources_mozbuild.sh:222
(Diff revision 2)
>  gen_rtcd_header linux/x64 x86_64
>  gen_rtcd_header linux/ia32 x86
>  gen_rtcd_header mac/x64 x86_64
>  gen_rtcd_header win/x64 x86_64
>  gen_rtcd_header win/ia32 x86
> +gen_rtcd_header win/mingw32 x86

Oops! This part should probably be rolled into the other import-script changes from bug 1380118.

::: media/libaom/moz.build:40
(Diff revision 2)
> +            CFLAGS += [ '-I%s/media/libaom/config/win/mingw32/' % TOPSRCDIR ]
> +            EXPORTS.aom += [ 'config/win/mingw32/aom_config.h' ]
> +        else:
> -        CFLAGS += [ '-I%s/media/libaom/config/win/ia32/' % TOPSRCDIR ]
> +            CFLAGS += [ '-I%s/media/libaom/config/win/ia32/' % TOPSRCDIR ]
> -        EXPORTS.aom += [ 'config/win/ia32/aom_config.h' ]
> +            EXPORTS.aom += [ 'config/win/ia32/aom_config.h' ]
> +        ASFLAGS += [ '-I%s/media/libaom/config/win/ia32/' % TOPSRCDIR ]

Even if using the same ASFLAGS work, the two branches here should probably have separate configs to make sure they match the CFLAGS settings.

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156234/diff/2-3/

Comment 7

[Tom Ritter [:tjr]]

I simplified the patch, can you re-review? If it looks good, I will land it once Bug 1380118 lands.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

https://reviewboard.mozilla.org/r/156234/#review174166

Thanks for the clean-up! Still r=me with the issues addressed.

::: media/libaom/moz.build:34
(Diff revision 3)
>  elif CONFIG['CPU_ARCH'] == 'x86':
>      EXPORTS.aom += files['IA32_EXPORTS']
>      SOURCES += files['IA32_SOURCES']
>      USE_YASM = True
>      if CONFIG['OS_TARGET'] == 'WINNT':
> +        if CONFIG['HOST_OS_ARCH'] == 'Linux':

Looking at this now, I think this branch will be triggered by linux-hosted clang-cl builds. I think you can avoid this by appending `and not CONFIG['CLANG_CL']` to the conditional here.

::: media/libaom/moz.build:35
(Diff revision 3)
>      EXPORTS.aom += files['IA32_EXPORTS']
>      SOURCES += files['IA32_SOURCES']
>      USE_YASM = True
>      if CONFIG['OS_TARGET'] == 'WINNT':
> +        if CONFIG['HOST_OS_ARCH'] == 'Linux':
> +            ASFLAGS += [ '-I%s/media/libaom/config/win/ia32/' % TOPSRCDIR ]

Again, please use the mingw32 ASFLAGS to make sure they match the CFLAGS.

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156234/diff/3-4/

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8885380 [details]
Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux

https://reviewboard.mozilla.org/r/156234/#review175092

::: media/libaom/moz.build:35
(Diff revisions 3 - 4)
>      EXPORTS.aom += files['IA32_EXPORTS']
>      SOURCES += files['IA32_SOURCES']
>      USE_YASM = True
>      if CONFIG['OS_TARGET'] == 'WINNT':
> -        if CONFIG['HOST_OS_ARCH'] == 'Linux':
> -            ASFLAGS += [ '-I%s/media/libaom/config/win/ia32/' % TOPSRCDIR ]
> +        if CONFIG['CC_TYPE'] == 'gcc':
> +            ASFLAGS += [ '-I%s/media/libaom/config/win/mingw32/' % TOPSRCDIR ]

Looks good. Thanks for fixing!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4f05166b2be
Use the MinGW libaom configuration for MinGW builds on Linux r=rillian

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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