Why do we have a dependency on msvcr90 if compiling with MINGW?

cowo78 commented 1 year ago

I see from 146e2edaac9fad6910e4c337b540cab01d1a072f that msvcr90 is brought in as a dependency when compiling with MINGW.

Could it be clarified what are the dependencies that we have from msvcr?

Why is msvcr90 (Visual Studio 2008) hardcoded? Shouldn't we link to whatever is available on the system?

mariuszmaximus commented 1 year ago

in my opinion this is a mistake,

I use MSYS2 clang 15.0.5 and MINGW is True I remove lines

    if(MINGW)
        set(MINGW_MSVCR_LIBRARY "msvcr90$<$<CONFIG:DEBUG>>" CACHE STRING "Mingw MSVC runtime import library")
        list(APPEND _BACKWARD_LIBRARIES ${MINGW_MSVCR_LIBRARY})
    endif()

then: [ctest] 100% tests passed, 0 tests failed out of 5

bombela commented 1 year ago

The pull request #234 states:

msvcr90 was needed because backward-cpp uses _set_abort_behavior.

bombela commented 1 year ago

It is used there: https://github.com/bombela/backward-cpp/blob/dc8b8c76822dcbc8918f032171050ad90e11eb7f/backward.hpp#L4338

Maybe MinGW didn't implements this before? Or it behaves differently? Clearly it seems to compile fine for you.

mariuszmaximus commented 1 year ago

I'm programming in C++ very little, I'm not an expert(I've used Delphi all my life) , but MinGW is broad concept. It is included in it:

MinGW (I don't think anyone uses it anymore)
Cygwin
Mingw-w64
MSYS2-mingw32
MSYS2-mingw64
MSYS2-clang32
MSYS2-clang64
other ?

In MSYS2-mingw64 and MSYS2-clang64 _set_abort_behavior exists and msvcr90 is unnecessary

cowo78 commented 1 year ago

As far as I understand _set_abort_behaviour is available in MSYS import libraries that in turn refer to native DLLs (i.e. ucrtbase, msvcr90 ...). Such import libraries are distributed inside mingw-*-crt*.

In our case the problem is that I don't see why it's necessary to hardcode a specific redistributable dependency. The same should be obtained by finding whatever is available on the system.

As far as I know there's no import library for debug MSVC libraries, so I would suggest to change set(MINGW_MSVCR_LIBRARY "msvcr90$<$<CONFIG:DEBUG>:d>" CACHE STRING "Mingw MSVC runtime import library") to find_library(MINGW_MSVCR_LIBRARY ucrtbase msvcr140 msvcr120 msvcr110 msvcr100 msvcr90 REQUIRED DOC "Mingw MSVC runtime import library").

I can provide a PR for that if you see the point.

cowo78 commented 1 year ago

In MSYS2-mingw64 and MSYS2-clang64 _set_abort_behavior exists and msvcr90 is unnecessary

AFAIK import libraries exist but the target EXE is dynamically linked to msvcr* or ucrtbase where the symbol is really defined.

madebr commented 10 months ago

I see the missing __imp__set_abort_behavior symbol error again in current master when I build for mingw with -DBUILD_SHARED_LIBS=ON. The following fixes it for me:

--- a/BackwardConfig.cmake
+++ b/BackwardConfig.cmake
@@ -215,10 +215,9 @@ if(WIN32)
                if(SUPPORT_WINDOWS_DEBUG_INFO)
                        set(CMAKE_EXE_LINKER_FLAGS "-Wl,--pdb= ")
                        add_compile_options(-gcodeview)
-               else()
-                       set(MINGW_MSVCR_LIBRARY "msvcr90$<$<CONFIG:DEBUG>:d>" CACHE STRING "Mingw MSVC runtime import library")
-                       list(APPEND _BACKWARD_LIBRARIES ${MINGW_MSVCR_LIBRARY})
                endif()
+               set(MINGW_MSVCR_LIBRARY "msvcr90$<$<CONFIG:DEBUG>:d>" CACHE STRING "Mingw MSVC runtime import library")
+               list(APPEND _BACKWARD_LIBRARIES ${MINGW_MSVCR_LIBRARY})
        endif() 
 endif()

mariuszmaximus commented 10 months ago

@madebr which mingw ?

https://sourceforge.net/projects/mingw/ here is gcc 6.x
https://www.mingw-w64.org/downloads/ I do not know what to choose, too much choice
https://sourceforge.net/projects/mingw-w64/ maybe this one?

could you send link to your mingw compiler, version and CMakeCache.txt from build ?

on my msys2 when I turn on BACKWARD_SHARED=YES then I must add -lDbghelp

I will look at the problem globally

madebr commented 10 months ago

When I build with -DBACKWARD_SHARED=YES, I additionally get a __imp_SymGetModuleBase64 missing symbol. Looks like the order of libraries is wrong: -ldbghelp must be after backward.cpp.obj, not before.

Here's my CMakeCache.txt, configured on top of current main (without any patch applied).

I'm using a mingw toolchain, provided by my Linux distribution (Fedora 38). It provides a gcc toolchain with version 12.2.1, using mingw64 10.0.0.

I will look at the problem globally

It might be interesting to link with MSVC using -nodefaultlib (while debugging, don't push this upstream). This will force you to explicitly add every library as a link argument (including the c, c++ and windows libraries).

mariuszmaximus commented 10 months ago

In my opinion better solution, compatible with Linux and msys2

if(WIN32)
    list(APPEND _BACKWARD_LIBRARIES dbghelp psapi)
    if(MINGW)
        if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")
            # mingw on Linux platform
            set(MINGW_MSVCR_LIBRARY "msvcr90$<$<CONFIG:DEBUG>:d>" CACHE STRING "Mingw MSVC runtime import library")
            list(APPEND _BACKWARD_LIBRARIES ${MINGW_MSVCR_LIBRARY})
        else()
            # mingw on msys2
            include(CheckCXXCompilerFlag)
            check_cxx_compiler_flag(-gcodeview SUPPORT_WINDOWS_DEBUG_INFO)  
            if(SUPPORT_WINDOWS_DEBUG_INFO)
                set(CMAKE_EXE_LINKER_FLAGS "-Wl,--pdb= ")
                add_compile_options(-gcodeview)
            endif()
        endif()
    endif() 
endif()

madebr commented 10 months ago

Or do a check_c_source_compiles test on all mingw/win32 platforms?

Also, the --codeview test should run on all mingw toolchains. I don't see why the __imp__set_abort_behavior symbol issue and the -gcodeview check are mutually exclusive.

cowo78 commented 9 months ago

Please note also #307 regarding the usage of fixed msvcr90. Better use whatever is available on the system IMO.

cowo78 commented 5 months ago

As of 51f0700452cf71c57d43c2d028277b24cde32502 there are no unresolved symbols when building with ninja/-DBACKWARD_SHARED=YES/gcc 12.2

cowo78 commented 5 months ago

Or do a check_c_source_compiles test on all mingw/win32 platforms?

Also, the --codeview test should run on all mingw toolchains. I don't see why the __imp__set_abort_behavior symbol issue and the -gcodeview check are mutually exclusive.

_set_abort_behavior sets if we want to show a windows error report message or print some text (on the console if any I guess). -gcodeview produces debug info in CodeView format (compatible with visual c++).

I don't understand why we should use -gcodeview option. If we build on windows with MINGW we will not be able to use the binary with MSVC anyway so I don't understand why we should produce a MSVC debug info. I see this was introduced in af4436fcdaaf994f68d33eb279673548075b6787 by @mariuszmaximus so maybe you want to shed some light?

mariuszmaximus commented 5 months ago

@cowo78 various compilers are included in the word "mingw" (on Windows and Linux) , I am using msys2 and clang , and only CodeView format enable beautiful stack trace CodeView also has some drawbacks, in vscode I must use cppvsdbg :D Please tell us what compiler you use, preferably provide a download link

cowo78 commented 5 months ago

@cowo78 various compilers are included in the word "mingw" (on Windows and Linux) , I am using msys2 and clang , and only CodeView format enable beautiful stack trace CodeView also has some drawbacks, in vscode I must use cppvsdbg :D Please tell us what compiler you use, preferably provide a download link

I am using GCC 12.2.0/mingw64/msys2; didn't know that CodeView provided better looking traces. I understand that -gcodeview should be enabled on GCC/CLANG for windows target if possible, regardless of the host. Correct?

mariuszmaximus commented 5 months ago

In my opinion backward-cpp doesn't work in msys2:mingw64 not exists function _set_abort_behavior and not exists BACKTRACE_SYMBOL

STACK_DETAILS_AUTO_DETECT = ON

msys2:mingw64

BACKWARD_DEFINITIONS:
BACKWARD_HAS_UNWIND=1;
BACKWARD_HAS_LIBUNWIND=0;
BACKWARD_HAS_BACKTRACE=0;
BACKWARD_HAS_BACKTRACE_SYMBOL=0;
BACKWARD_HAS_DW=0;
BACKWARD_HAS_BFD=1;
BACKWARD_HAS_DWARF=0

-- running test case: smalltrace
Stack trace (most recent call last):
#11   Object "", at 0x7ffe8f8b257d, in BaseThreadInitThunk
Attempt to access invalid address.

#10   Object "", at 0x7ff6d3ec1406, in  ?? 
Attempt to access invalid address.

#9    Object "", at 0x7ff6d3ec12ee, in  ?? 
Attempt to access invalid address.

#8    Object "", at 0x7ff6d3ec1b3b, in  ?? 
Attempt to access invalid address.

#7    Object "", at 0x7ff6d3ec178b, in  ?? 
Attempt to access invalid address.

#6    Object "", at 0x7ff6d3ec89d6, in  ??
Attempt to access invalid address.

#5    Object "", at 0x7ff6d3ec1624, in  ?? 
Attempt to access invalid address.

#4    Object "", at 0x7ff6d3ec15e0, in  ??
Attempt to access invalid address.

msys2:clang64

BACKWARD_DEFINITIONS:
BACKWARD_HAS_UNWIND=1;
BACKWARD_HAS_LIBUNWIND=0;
BACKWARD_HAS_BACKTRACE=0;
BACKWARD_HAS_BACKTRACE_SYMBOL=1;
BACKWARD_HAS_DW=0;
BACKWARD_HAS_BFD=0;
BACKWARD_HAS_DWARF=0

running test case: smalltrace
Stack trace (most recent call last):
#11   Object "", at 00007FFE8F8B257D, in BaseThreadInitThunk
#10   Object "", at 00007FF70DE81366, in mainCRTStartup
#9    Object "", at 00007FF70DE81315, in WinMainCRTStartup
#8    Source "D:\mariusz\pulpit\gity\_kasuj\backward-cpp_2024\test\_test_main.cpp", line 203, in main
        200:       if (strcasecmp(argv[2], test.name) == 0) {
        201:         run_test(test, false);
        202:
      > 203:         return 0;
        204:       }
        205:     }
        206:     return -1;
#7    Source "D:\mariusz\pulpit\gity\_kasuj\backward-cpp_2024\test\_test_main.cpp", line 77, in run_test
         75: bool run_test(TestBase &test, bool use_child_process = true) {
         76:   if (!use_child_process) {
      >  77:     exit(static_cast<int>(test.run()));
         78:   }
         79:
         80:   printf("-- running test case: %s\n", test.name);
#6    Source "D:\mariusz\pulpit\gity\_kasuj\backward-cpp_2024\test\test.hpp", line 92, in test::TestBase::run
         90:   TestStatus run() {
         91:     try {
      >  92:       do_test();
         93:       return SUCCESS;
         94:     } catch (const AssertFailedError &e) {
         95:       printf("!! %s\n", e.what());
#5    Source "D:\mariusz\pulpit\gity\_kasuj\backward-cpp_2024\test\stacktrace.cpp", line 54, in TEST_smalltrace::do_test

-gcodeview only works on msys2:clang !!!

mariuszmaximus commented 5 months ago

@cowo78 in my opinion backward-cpp use many libraries (libdw,libbfd,libdwarf etc...) default is STACK_DETAILS_AUTO_DETECT = ON on my platform I don't have any extra libs then backward-cpp use StackWalk64 for walk and we need codeview for beautiful stack trace

codeview is broken in GCC :( https://github.com/msys2/MINGW-packages/issues/17599

bombela / backward-cpp

Why do we have a dependency on msvcr90 if compiling with MINGW? #291