Opened 7 years ago

Closed 7 years ago

#10154 closed defect (fixed)

MSVC 2015 - SCI does not build, STATIC_ASSERT broken?

Reported by: quietust Owned by: csnover
Priority: blocker Component: Engine: SCI
Version: Keywords:
Cc: Game:

Description

When building ScummVM using Visual C++ 2015, the final link step to build scummvm.exe fails with 2 unresolved external symbol errors from sci.lib(message.obj) for "int * STATIC_ASSERT_uint16_can_only_be_read_from_int16_or_smaller_spans".

STATIC_ASSERT looks to be a macro (defined in common/scummsys.h) which tries to emulate C++11 "static_assert" (which works fine in MSVC 2015), but it's not working correctly here.

Oddly, if I try using the STATIC_ASSERT macro in a test program, it seems to work fine, aside from assertion failures showing up as "negative subscript" instead of the message provided.

Attachments (1)

10154.patch (1.1 KB ) - added by csnover 7 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by dreammaster, 7 years ago

That sounds weird. I build with Visual Studio 2015, and I've never had such issues with linking in the SCI engine. A few things I can suggest:
1) make sure you're using the dists/msvc14 folder to build the solution, not msvc15, since that's a VS2017 solution
2) Check if you're using Debug/Win32 as the target. I at least that that target works with the scummvm_libs zip file we provide. Not sure about the other targets off the top of my head.

If you still have problems, try changing all the lines calling create_project in the dists/msvc14/create_msvc14.bat file to add in a --disable-engine=sci
See if it compiles afterwards. Presuming that you're not building ScummVM specifically to poke around the SCI engine, that would at least hopefully let you bypass the problem you're having and run the executable.

Version 1, edited 7 years ago by dreammaster (previous) (next) (diff)

comment:2 by quietust, 7 years ago

Apparently, the problem is that I was trying to make a Release build - if I do a Debug build, it completes successfully.

comment:3 by dreammaster, 7 years ago

Owner: set to dreammaster
Resolution: worksforme
Status: newclosed

Ah, good then.

comment:4 by quietust, 7 years ago

I'm not quite sure how this is "not an issue" - create_project still offers a "Release" build configuration, but it clearly doesn't work with MSVC14.

Currently, the wiki page Compiling ScummVM/Visual Studio does not explicitly state that Debug is required, just that it's the default and that it's ideal for playing around with changes. If this is no longer the case, then it should be updated.

Last edited 7 years ago by quietust (previous) (diff)

comment:5 by wjp, 7 years ago

Resolution: worksforme
Status: closednew

comment:6 by wjp, 7 years ago

Owner: dreammaster removed

comment:7 by csnover, 7 years ago

Priority: normalblocker

At first glance, this seems like an MSVC bug where the symbols are getting optimised in a way that is confusing the linker. Does this problem also occur in Visual C++ 2017? If so, I think we could probably change the macro to use the real static_assert (which, actually, might not be a bad idea in any case) which will hopefully solve the problem since then that symbol would be a string literal for the message parameter of static_assert, and the compiler will know how to deal with its own static assertions, and so much less likely to get screwed up.

comment:8 by csnover, 7 years ago

Owner: set to csnover

by csnover, 7 years ago

Attachment: 10154.patch added

comment:9 by csnover, 7 years ago

quietust, could you please try applying this patch and let me know if it solves building in release mode? Thanks!

comment:10 by quietust, 7 years ago

That patch does indeed fix Release builds in VC++ 2015. Unfortunately, I don't have VC++ 2017 installed, so I can't test it to see if it works there too.

comment:11 by csnover, 7 years ago

Great! Thank you for testing. I imagine fewer and fewer problems with MSVC++ going forward since it is now apparently far more standards-compliant than it has been historically. I’ll land this tomorrow when I am fresh.

Last edited 7 years ago by csnover (previous) (diff)

comment:12 by csnover, 7 years ago

Resolution: fixed
Status: newclosed

Thanks for your report! A patch for this issue has been added in commit 2de83e09374abd10700e5f2bc4671318f182980b and will be available in daily builds 1.10.0git-5311 and later.

Note: See TracTickets for help on using tickets.