jrfonseca / drmingw

Postmortem debugging tools for MinGW.
GNU Lesser General Public License v2.1
273 stars 53 forks source link

fix and simplify build #85

Closed oltolm closed 6 months ago

oltolm commented 1 year ago

I tried build drmingw with mingw-w64, but it failed because libwinpthread-1.dll was not getting linked statically. So I tried fixing the problem and in the course simplified the build system. Now libwinpthread-1.dll is getting linked statically.

jrfonseca commented 1 year ago

Thanks.

https://github.com/jrfonseca/drmingw/pull/85/commits/d42fe54745ad073b6a2f9a7ed63259191813f625 looks good and I've commited as a36c7dcb1a07ee206f0aa343882b174c116c7f75

https://github.com/jrfonseca/drmingw/pull/85/commits/8a6529a2924511951103a3431ef9a618ec13afb9 also has some good chunks modernizing CMake usage , but it goes too far in simplifing away important workarounds for GCC/Clang quirks, as can be seen by the failed builds/tests on https://github.com/jrfonseca/drmingw/actions/runs/4713642077

I'll try to get the non-controversial chunks committed, and the remaining will need to be address seperately.

oltolm commented 1 year ago

I will try to fix the failing tests.

jrfonseca commented 1 year ago

Thanks. I think the easier is to break the simplifications in smaller independent patches, starting with the uncontroversial chunks first, then slowly push those more suspect (the static linking, rewrite of import libraries)

codecov-commenter commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (33bbb6b) 50.62% compared to head (254c8d9) 50.57%.

:exclamation: Current head 254c8d9 differs from pull request most recent head add3815. Consider uploading reports for the commit add3815 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #85 +/- ## ========================================== - Coverage 50.62% 50.57% -0.05% ========================================== Files 15 15 Lines 1997 1993 -4 Branches 758 757 -1 ========================================== - Hits 1011 1008 -3 + Misses 761 759 -2 - Partials 225 226 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

oltolm commented 1 year ago

So I fixed all tests except for build-cross-clang. I don't know why it fails.

jrfonseca commented 1 year ago

Thanks for the update.

build-cross-clang is failing because libssp and libc++ are not being statically linked. Why, I'm not sure -- TBH, my surprise is not how Clang fails, but rather how GCC build is not broken after simplifying away so many workarounds I had and thought needed.

I depend upon Clang to build the Arm binaries, so I can't afford to break the Clang builds.

I also must insist upon the simplifications being broken up in smaller independent commits no matter what. Even if you fix all test regressions, the test suite I have is not 100% bulletproof, and these changes touch upon things that gave me lots of headaches in the past, taking many tries to get it working. Consequently I wouldn't be surprised if there were some unintended side effects here somehow that are escaping the test suite. Therefore I want to be able to easily bisect and revert parts of PR in the future, and for that I need it to be broken up in smaller independent changes.

I appreciate the attention you put on this so far -- this MR has been very educational for me -- CMake seems to have made quite a few things easier --, but I'm afraid it's not something I can commit as-is.

oltolm commented 1 year ago

I got the test to pass. I can try splitting the code into small commits.

jrfonseca commented 1 year ago

Thank you!

oltolm commented 1 year ago

So I tried splitting the code into 6 commits. This was more effort than I anticipated.

jrfonseca commented 1 year ago

Thanks for going the extra mile.

I'm afraid dependencies with ninja are broken when I manually run ci/clang.sh:

$ ci/clang.sh
[...]
[74/117] Linking C static library thirdparty/libdwarf.a
FAILED: thirdparty/libdwarf.a 
: && /usr/bin/cmake -E rm -f thirdparty/libdwarf.a && x86_64-w64-mingw32-ar qc thirdparty/libdwarf.a  thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_abbrev.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_alloc.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_arange.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_debuglink.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_debugnames.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_debug_sup.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_die_deliv.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_dsc.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_elf_load_headers.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_elfread.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_elf_rel_detector.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_error.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_find_sigref.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_fission_to_cu.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_form.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_form_class_names.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_frame2.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_frame.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_funcs.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_gdbindex.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_generic_init.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_global.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_gnu_index.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_groups.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_harmless.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_init_finish.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_leb.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_line.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_locationop_read.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_loc.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_loclists.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_machoread.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_macro5.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_macro.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_memcpy_swap.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_names.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_peread.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_print_lines.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_pubtypes.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_query.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_ranges.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_rnglists.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_safe_strcpy.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_string.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_stringsection.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_str_offsets.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_tied.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_tsearchhash.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_types.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_util.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_vars.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_weaks.c.obj thirdparty/CMakeFiles/dwarf.dir/libdwarf/src/lib/libdwarf/dwarf_xu_index.c.obj && x86_64-w64-mingw32-ranlib thirdparty/libdwarf.a && :
x86_64-w64-mingw32-ranlib: error: unable to load 'thirdparty/libdwarf.a': No such file or directory
[86/117] Building CXX object src/common/CMakeFiles/common.dir/debugger.cpp.obj
FAILED: CMakeFiles/check /home/jfonseca/projects/drmingw/build/x86_64-w64-mingw32-clang/CMakeFiles/check 
cd /home/jfonseca/projects/drmingw/build/x86_64-w64-mingw32-clang && /usr/bin/ctest --output-on-failure
ninja: build stopped: subcommand failed.

Sometimes its libcommon.a or test_addr2line.exe that's missing.

I also see warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument]

This is what I'm going to do: I'm going to go every commit one by one, keep the good bits, remove any troublesome bits. Hopefully this will push things forward without breaking anything.

oltolm commented 1 year ago

Sure. I don't see a file ci/clang.sh in the repo or I could fix it too.

jrfonseca commented 1 year ago

Sorry, ci/clang.sh was a typo. I meant to write ci/build-clang.sh -- the script used by the build-cross-clang job.

I'm not sure why it always fails when locally, but not on GitHub Actions -- perhaps because it's a race condition and I have more CPU threads than GitHub VMs. For me nproc outputs 12.

jrfonseca commented 1 year ago

I merged the a good chunk of the changes.

The mgwhelp import lib and test dependencies seem to be more problematic. I'll leave those alone for now.

jrfonseca commented 6 months ago

Hi @oltolm,

The build started failing on master due to some weird race condition on the import libs.

I tried these changes again, and they do avoid the race condition.

However I feared regressions, so I wrote this script to make sure mgwhelp.dll is properly linked, and alas your change does break MinGW build as I feared.

See https://github.com/jrfonseca/drmingw/actions/runs/7151760733/job/19476380076:

> python tests\check_dynamic_linkage.py --objdump=objdump --validate $buildDir\bin\*.dll $buildDir\bin\*.exe
error: build\mingw32\bin\exchndl.dll: imports dbghelp.dll instead of mgwhelp.dll
info: build\mingw32\bin\exchndl.dll: NOT OK
info: build\mingw32\bin\mgwhelp.dll: OK
error: build\mingw32\bin\addr2line.exe: imports dbghelp.dll instead of mgwhelp.dll
info: build\mingw32\bin\addr2line.exe: NOT OK
error: build\mingw32\bin\catchsegv.exe: imports dbghelp.dll instead of mgwhelp.dll
info: build\mingw32\bin\catchsegv.exe: NOT OK
error: build\mingw32\bin\drmingw.exe: imports dbghelp.dll instead of mgwhelp.dll
info: build\mingw32\bin\drmingw.exe: NOT OK

So your changes cause all EXEs to link against dbghelp.dll instead of mgwhelp.dll.

The issue is the PUBLIC in mgwhelp, but the build will fail with missing symbols if this is removed:

diff --git a/src/mgwhelp/CMakeLists.txt b/src/mgwhelp/CMakeLists.txt
index f38339e..ea1702f 100644
--- a/src/mgwhelp/CMakeLists.txt
+++ b/src/mgwhelp/CMakeLists.txt
@@ -14,9 +14,8 @@ target_sources (mgwhelp PRIVATE
 target_link_libraries (mgwhelp PRIVATE
     common
     dwarf
     libiberty
-    PUBLIC
     dbghelp
 )

 set_target_properties (mgwhelp PROPERTIES

In short, I'm afraid that change a07674ce37158d46314af5868b610b9a5efd8e2f is a false simplification: it simplifies the build by removing (the admittedly complex!) workarounds necessary to get this to work...

I wished there was a simple way to workaround MinGW 32-bits DLL import library issue, but I don't see a solution besides using separate DEF for imports/exports...

oltolm commented 6 months ago

I tried another fix.

jrfonseca commented 6 months ago

You need to revert f9c837ede6987b4c14798606c4c8e6a7e24a3f9d on your branch.

oltolm commented 5 months ago

I think it is a bug in ld. I fixed it in https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d544a1dca23469d212e7e9edc754487352c5f042.

jrfonseca commented 5 months ago

I think it is a bug in ld. I fixed it in https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d544a1dca23469d212e7e9edc754487352c5f042.

Awesome! Thank you.

I'm not sure when that fix will trickle through prebuild mingw binaries like https://github.com/niXman/mingw-builds-binaries but I'll reopen this PR so I don't forget to revisit this in the future.

jrfonseca commented 5 months ago

Reopening...