Closed Bug 1378529 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file)

Bug 1378529 Use the MinGW libaom configuration for MinGW builds on Linux
59 bytes, text/x-review-board-request
rillian
: review+
Details 
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
Jacek, what do you think?
Flags: needinfo?(jacek)
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.
Flags: needinfo?(jacek)
Depends on: 1380118
Attachment #8885380 - Flags: review?(giles)
Blocks: 1381078
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.
Attachment #8885380 - Flags: review?(giles) → review+
I simplified the patch, can you re-review? If it looks good, I will land it once Bug 1380118 lands.
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
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!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4f05166b2be
Use the MinGW libaom configuration for MinGW builds on Linux r=rillian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4f05166b2be
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: