Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visual Studio 2022 fails to compile MAMe due to broken overload resolution #11316

Closed
Zhaojun-Liu opened this issue Jun 6, 2023 · 9 comments · Fixed by #12383
Closed

Visual Studio 2022 fails to compile MAMe due to broken overload resolution #11316

Zhaojun-Liu opened this issue Jun 6, 2023 · 9 comments · Fixed by #12383

Comments

@Zhaojun-Liu
Copy link

MAME version

5599230

System information

Windows Server 2022 Datacenter 21H2 amd64

INI configuration details

No response

Emulated system/software

Visual Studio 2022 17.5.5

Incorrect behaviour

F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,15): error C2039: 'empty': is not a member of 'emu_fatalerror' [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,15): error C2039: 		, m_end(fmt.empty() ? nullptr : (m_begin + std::distance(std::cbegin(fmt), std::cend(fmt)))) [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,15): error C2039: 		            ^ [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,65): error C2672: 'std::cbegin': no matching overloaded function found [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,65): error C2672: 		, m_end(fmt.empty() ? nullptr : (m_begin + std::distance(std::cbegin(fmt), std::cend(fmt)))) [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,65): error C2672: 		                                                              ^ [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]
F:\gitP\mamedev\mame\src\lib\util\strformat.h(1144,83): error C2672: 'std::cend': no matching overloaded function found [F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019\optional.vcxproj]

Expected behaviour

build successfully.

Steps to reproduce

  1. git clone https://github.com/mamedev/mame F:\gitP\mamedev\mame
  2. git -C "F:\gitP\mamedev\mame" submodule sync
  3. git -C "F:\gitP\mamedev\mame" submodule update --init --recursive
  4. cd F:\gitP\mamedev\mame
  5. set PATH=F:\gitP\mamedev\mame..\tools\msys64\usr\bin;F:\gitP\mamedev\mame..\tools\msys64\mingw64\bin;%PATH%
  6. make SUBTARGET=tiny PTR64=1 TOOLS=1 OPTIMIZE=0 NOWERROR=1 MODERN_WIN_API=1 NO_USE_PORTAUDIO=1 vs2019 -j4 2>&1
  7. cd F:\gitP\mamedev\mame\build\projects\windows\mametiny\vs2019
  8. devenv /upgrade mametiny.sln 2>&1
  9. msbuild /m /p:Platform=x64 /p:Configuration=Release /p:WindowsTargetPlatformVersion=10.0.22621.0 /p:PreferredToolArchitecture=x64 mametiny.sln /t:Rebuild

Additional details

detailed log: build.log

@pmackinlay
Copy link
Contributor

pmackinlay commented Jun 6, 2023

You can work around this problem with the following patch:

diff --git a/src/emu/emucore.h b/src/emu/emucore.h
index d87ea5ca3af..54660e54776 100644
--- a/src/emu/emucore.h
+++ b/src/emu/emucore.h
@@ -232,12 +232,12 @@ public:
        emu_fatalerror(int _exitcode, util::format_argument_pack<char> const &args);

        template <typename Format, typename... Params>
-       emu_fatalerror(Format &&fmt, Params &&... args)
+       explicit emu_fatalerror(Format &&fmt, Params &&... args)
                : emu_fatalerror(static_cast<util::format_argument_pack<char> const &>(util::make_format_argument_pack(std::forward<Format>(fmt), std::forward<Params>(args)...)))
        {
        }
        template <typename Format, typename... Params>
-       emu_fatalerror(int _exitcode, Format &&fmt, Params &&... args)
+       explicit emu_fatalerror(int _exitcode, Format &&fmt, Params &&... args)
                : emu_fatalerror(_exitcode, static_cast<util::format_argument_pack<char> const &>(util::make_format_argument_pack(std::forward<Format>(fmt), std::forward<Params>(args)...)))
        {
        }

I think this issue is caused by the different/non-standard Microsoft std::exception class, and the change should be harmless for non-MSVC builds, but I have not proposed upstreaming it yet.

@cuavas cuavas changed the title [MSVC] Mame failed to build due to error C2039 and error C2672 with MSVC on Windows Visual Studio 2022 fails to compile MAMe due to broken overload resolution Jun 6, 2023
@cuavas
Copy link
Member

cuavas commented Jun 6, 2023

The C++ compiler included with Visual Studio 2022 has serious regressions in overload resolution.

In this case, it is incorrectly selecting the template constructor rather than the implicitly-declared copy constructor. For better or worse, implicitly declared copy constructors are a fundamental feature of C++, and failure to support them correctly is a serious compiler bug.

Attempting a complete MAME build with Visual Studio 2022 results in numerous errors due to bugs in the C++ compiler’s overload resolution logic. See issue #10488 for more examples. This is a serious enough compiler regression that it simply isn’t worthwhile to attempt to support building MAME with Visual Studio 2022. Microsoft needs to fix their compiler, or give up and switch to using clang exclusively.

@Zhaojun-Liu
Copy link
Author

The C++ compiler included with Visual Studio 2022 has serious regressions in overload resolution.

In this case, it is incorrectly selecting the template constructor rather than the implicitly-declared copy constructor. For better or worse, implicitly declared copy constructors are a fundamental feature of C++, and failure to support them correctly is a serious compiler bug.

Attempting a complete MAME build with Visual Studio 2022 results in numerous errors due to bugs in the C++ compiler’s overload resolution logic. See issue #10488 for more examples. This is a serious enough compiler regression that it simply isn’t worthwhile to attempt to support building MAME with Visual Studio 2022. Microsoft needs to fix their compiler, or give up and switch to using clang exclusively.

Thanks for your reply. I submitted a bug to MSVC to confirm.

@Zhaojun-Liu
Copy link
Author

You can work around this problem with the following patch:

diff --git a/src/emu/emucore.h b/src/emu/emucore.h
index d87ea5ca3af..54660e54776 100644
--- a/src/emu/emucore.h
+++ b/src/emu/emucore.h
@@ -232,12 +232,12 @@ public:
        emu_fatalerror(int _exitcode, util::format_argument_pack<char> const &args);

        template <typename Format, typename... Params>
-       emu_fatalerror(Format &&fmt, Params &&... args)
+       explicit emu_fatalerror(Format &&fmt, Params &&... args)
                : emu_fatalerror(static_cast<util::format_argument_pack<char> const &>(util::make_format_argument_pack(std::forward<Format>(fmt), std::forward<Params>(args)...)))
        {
        }
        template <typename Format, typename... Params>
-       emu_fatalerror(int _exitcode, Format &&fmt, Params &&... args)
+       explicit emu_fatalerror(int _exitcode, Format &&fmt, Params &&... args)
                : emu_fatalerror(_exitcode, static_cast<util::format_argument_pack<char> const &>(util::make_format_argument_pack(std::forward<Format>(fmt), std::forward<Params>(args)...)))
        {
        }

I think this issue is caused by the different/non-standard Microsoft std::exception class, and the change should be harmless for non-MSVC builds, but I have not proposed upstreaming it yet.

Thanks for your quickly reply, I tried your patch, it worked, thank you~

@yz70s
Copy link
Contributor

yz70s commented Jun 6, 2023

Thanks for your reply. I submitted a bug to MSVC to confirm.

Can you share the link to the bug report, for future reference ?

@Zhaojun-Liu
Copy link
Author

Thanks for your reply. I submitted a bug to MSVC to confirm.

Can you share the link to the bug report, for future reference ?

What a pity, it is an internal bug item, can't be accessed with external website. I will synchronize progress in time.

@joemmett
Copy link

Hi, sorry for the long turnaround but I am a MSVC dev and investigated this issue. This is not an overload resolution bug, in fact all compilers will choose the templated constructor when copying an instance of this type (e.g. when throwing) as it is a better candidate than the usual copy constructor. All compilers will also elide that copy whenever possible, though, so it usually is not visible. You can provoke the same error from gcc or clang to see the same overload resolution result by forcing a copy, e.g. by catching an instance of this type by value or actually copying an instance in a way that can't be elided. The templated function could be constrained to avoid this situation if you expect the type to be copyable (and non-copyable types cannot conformingly be thrown).

The reason for the error with MSVC is because it uses a different exception handling implementation strategy from gcc/clang. MSVC requires the function to be used to copy an exception to be available at runtime and so will instantiate it always as the exception handling runtime may need to call the function to make a copy lazily; gcc and clang will always use a heap allocation at the throw site instead of a potential runtime copy and only instantiate the copy function if it is known to be needed at compile time.

@backrock
Copy link

Hi, when I compile by vs2019-clang , the same errors reported..
and I can't fix it by " explicit emu_fatalerror(Format &&fmt, Params &&... args)".
who know how to fix finally

@rb6502
Copy link
Contributor

rb6502 commented Aug 30, 2023

That's irrelevant to what's being discussed here, @backrock. This is about using the actual MSVC compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants