msys2 / MINGW-packages

Package scripts for MinGW-w64 targets to build under MSYS2.
https://packages.msys2.org
BSD 3-Clause "New" or "Revised" License
2.25k stars 1.21k forks source link

Multiple definitions of symbol PC when statically linking readline and termcap into a program #8105

Closed intvsteve closed 1 year ago

intvsteve commented 3 years ago

This used to work when using the package mingw-w64-i686-readline-7.0.005-1-any.pkg.tar.xz.

Now, when upgrading to the latest (mingw-w64-i686-readline-8.0.004-2-any.pkg.tar.zst), linking both of these libraries results in an error from ld:

D:/msys64/mingw32/lib\libtermcap.a(termcap.o):(.bss+0x8): multiple definition of `PC'; D:/msys64/mingw32/lib\libreadline.a(terminal.o):(.bss+0x5c): first defined here
collect2.exe: error: ld returned 1 exit status
Biswa96 commented 3 years ago

Can you provide a simple code to reproduce your issue?

revelator commented 3 years ago

does it pr chance also link to one of the curses libraries ? PC is defined in them as well.

intvsteve commented 3 years ago

The code I'm building does not appear to link to curses.

The code I'm building is the jzintv emulator. I have access to a slightly more recent version, but this aspect has not changed and the code here will likely reproduce the issue as well.

Some caveats

I think most of what is needed for the 32-bit build can be installed via:

pacman -S base-devel pacman -S gcc pacman -S mingw-w64-i686-gcc pacman -S mingw-w64-i686-SDL pacman -S mingw-w64-i686-SDL2 pacman -S mingw-w64-i686-readline

I had to reconstruct my msys2 when pacman stopped working completely. However, I could downgrade to the 32-bit version of the mingw-w64-i686-readline package, as mentioned, and get things to link. I initially filed this issue against the readline package.

UPDATE: I have confirmed the build failure with the above linked sources. Downgrading to my older package (mingw-w64-i686-readline-7.0.005-1-any.pkg.tar.xz) avoids the linker error.

intvsteve commented 3 years ago

@Biswa96 Unfortunately, the sample code I provided is not "simple". I have not created a "toy" program to reproduce.

revelator commented 3 years ago

hmm could have happened if say one of the libraries you link to include say ncurses as we only have a static version of that it might have exported the PC symbol to said library but it is hard to say.

can you try to build it with CFLAGS+=" -fcommon" CXXFLAGS+=" -fcommon" as i noticed this causes some packages that have previously linked without this error to suddenly throw a lot of multiple defines

EDIT: damn i had a lot to "say" here xD

intvsteve commented 3 years ago

Sadly, same exact error.

Everything's* statically linked, so it'd have to be something that gloms ncurses on, which I see no clear evidence for.

Here's a representative source compile w/ flags:

gcc -std=gnu11 -o jzintv.o -O3 -fomit-frame-pointer -msse2 -fcommon -ffunction-sections -flto -Wall -W -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-qual -Werror -I. -I.. -DDIRECT_INTV2PC -DUSE_SDL=2 -DJZINTV_VERSION_MAJOR=0x01 -DJZINTV_VERSION_MINOR=0x00 -DJZINTV_SVN_REV=2140 -DJZINTV_SVN_DTY=1 -c jzintv.c

Here's the link (all the .o files excluded for brevity) so you can see what's being fed to link:

g++ -std=c++14 -U__STRICT_ANSI__ -fvisibility=hidden -o ...<object files omitted for brevity> -O3 -fomit-frame-pointer -msse2 -ffunction-sections -flto -Wall -W -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-qual -Werror -I. -I.. -DDIRECT_INTV2PC -DUSE_SDL=2 -DJZINTV_VERSION_MAJOR=0x01 -DJZINTV_VERSION_MINOR=0x00 -DJZINTV_SVN_REV=2140 -DJZINTV_SVN_DTY=0 -ID:/msys64/mingw32/include/SDL2 -Dmain=SDL_main -L../lib -static-libgcc -static-libstdc++ -static -LD:/msys64/mingw32/lib -lmingw32 -lSDL2main -lSDL2 -mconsole -lmingw32 -ldinput8 -lshell32 -lsetupapi -ladvapi32 -luuid -lversion -loleaut32 -lole32 -limm32 -lwinmm -lgdi32 -luser32 -lm -mconsole -Wl,--no-undefined -pipe -Wl,--dynamicbase,--nxcompat,--no-seh -lreadline -ltermcap

*the exception being the SDL library

revelator commented 3 years ago

Hmm at this point it might be prudent to just use LDFLAGS+=" -Wl,--allow-multiple-definition" untill we figure out where this symbol is comming from.

intvsteve commented 3 years ago

Thanks, @revelator . I can chat w/ the code owner, but am more likely to use one of the other two workarounds available to me for now (the older build, or compiling out the usage of readline).

I did some basic snooping and found the termcap.h header has the extern char PC right out there for the world to see. So it would seem that termcap is not pulling any surprises here.

For readline that's not the case. PC does not appear in any of its headers. Based on the error, I'd expect a non-static, non-extern declaration to show up in a terminal.c file in the source for readline.

I'm not familiar w/ how msys2 packages are built, but it seems most likely to be a change in how readline was built from the 8.0 sources vs. the 7.0 sources.

FWIW, I found this repo and downloaded the 8.0 sources and checked terminal.c and found this:

#if !defined (__linux__) && !defined (NCURSES_VERSION)
#  if defined (__EMX__) || defined (NEED_EXTERN_PC)
extern 
#  endif /* __EMX__ || NEED_EXTERN_PC */
char PC, *BC, *UP;
#endif /* !__linux__ && !NCURSES_VERSION */

To me, it would seem it's entirely dependent upon how readline was built. The code for this particular bit seems identical in the 7.0 source from here - again indicating it seems to be a compile-time issue for readline.

In fact, if I were to throw out a wild guess... Did the definition of NEED_EXTERN_PC change?

revelator commented 3 years ago

termcap is kind of a minimal implementation of the curses library's terminal capabilities so that is not so odd :) and by itself it should pose no problem unless the exports are wrong as you also point out readline might be the culprit, the funny thing is that readline can use both curses or termcap. The one from us does not use ncurses but it does use termcap readline-symbols

intvsteve commented 3 years ago

Indeed it does! It may well be that for the DLL builds, this isn't an issue, but that in static library builds it is. IIRC it may take a little more effort to export a global from a DLL build, but it's all too easy for static libraries.

revelator commented 3 years ago

one experiment would be to try building readline without either ncurses or termcap :) it is entirely possible and depends only if you need the terminal capabilities in your program. If you dont then it should be an easy fix, though in the long run it would be best to find out why this happens so we dont run headlong into the same problem again.

revelator commented 3 years ago

static builds are nice when they work but as you noticed there are some pitfalls like the one here and also knowing exactly which system libraries need to be linked to (static builds are newer really truely static anyway as they link to several system dll's but the compiler only links to a small number of system libraries by default so sometimes we do get these "not so fun" problems), i work with an offshot of the compilers here using TDM's patches which by default links to the static gcc runtimes so it does give me some perspective.

revelator commented 3 years ago

dll builds load the symbols dynamically from the dll while static builds have the symbols linked into the application at runtime, so if say the symbol PC is not delared with the correct export for a static library then we get these kind of problems :). This is also the reason why static builds are sometimes several magnitudes bigger than the dll builds as all the symbols are linked into the resulting application. This was actually one of the major problems with TDM gcc as by default it linked in winpthreads statically but winpthreads also has problems with static exports so it caused me a huge headache trying to figure out why some stuff worked while several other projects failed miserably. the symbol in question is actually allmost correct for a static build but termcap has wrong exports for a dll build -> extern char PC; should have been __declspec(dllexport) char PC; and the static one should just be char PC:

intvsteve commented 3 years ago

Hehe yeah.... at a long-time prior employer I did a lot of work on a large multi-platform codebase with a pretty sophisticated (and bespoke) build toolchain. Exporting globals from Windows DLLs is almost a "dark art" involving secret incantations and rare reagents.

The fun part is the general difference in practices across platforms. Generally it seems the default *NIX approach is (or was) to statically link - it avoids "DLL Hell" and all sorts things, at the cost of larger executables and challenges to patch. E.g. if a security hole is found in termcap any app statically linked to it needs a rebuild to get the patch.

Windows tends to favor shared library linkages, inviting DLL Hell but getting smaller individual app sizes, easier patches outside every app needing to rebuild.

Mac apps seem to try to split the middle, but can't decide on .framework, .dylib, .so, or whatever lol.

So when you have portable code across compilers, platforms, and target linkages, things get pretty ugly pretty fast when you have to herd the cats. There's not a universal standard way to deal with all those combinations of sources and targets AFAIK.

Been a while since those things were the daily grind though! Maybe things are changing.

revelator commented 3 years ago

its becoming more standardized that is for sure :) gcc has had a lot of hardening lately so things which would normally build no problem no longer does. one such is fortran which added more strictness at about version 8 i think gcc itself also became more strict and now errors out on things that would previously just had been warnings. personally i like clang though im a bit partial with rust (a bytecode compiler dependant on a bytecode compiler just using a different syntax as if things were not advanced enough as is huh...).

revelator commented 3 years ago

btw i just noticed -Wl,--dynamicbase,--nxcompat,--no-seh if using binutils-2.36 these flags are no longer nessesary as it now defaults to having aslr and dep on. the --no-seh flag might still be needed though.

I guess msys2 needs an update ;)

flinco commented 3 years ago

I confirm the issue.

This is what I get :-(

cc -s -static xxxxxx.o xxxxxx-local.o xxxxxx-xml.o xxxxxx_utils.o vg_xxxxxx_support.o vg_xxxxxx_utils.o km_xxxxxx_template.o -o xxxxxx.exe -LC:/msys64/mingw64/lib -lxml2 -LC:/msys64/mingw64/lib -lz -LC:/msys64/mingw64/lib -lws2_32 -lssl -LC:/msys64/mingw64/lib -lws2_32 -lgdi32 -lcrypt32 -lcrypto -lws2_32 -lgdi32 -lcrypt32 -lintl -lws2_32 -lole32 -lwinmm -lshlwapi -lm -LC:/msys64/mingw64/lib -lpcre -lgthread-2.0 -pthread -lintl -lintl -lws2_32 -lole32 -lwinmm -lshlwapi -pthread -lm -LC:/msys64/mingw64/lib -lpcre -lgobject-2.0 -lglib-2.0 -lws2_32 -lole32 -lwinmm -lshlwapi -pthread -lm -LC:/msys64/mingw64/lib -LC:/msys64/mingw64/lib/../lib -lffi -lreadline -LC:/msys64/mingw64/lib -ltermcap -lregex -ltre -pipe -lintl -liconv -llzma -lpcre -lole32 -lws2_32 -lz -ldl -lm C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64/mingw64/lib\libtermcap.a(termcap.o):(.bss+0x8): multiple definition of 'PC'; C:/msys64/mingw64/lib\libreadline.a(terminal.o):terminal.c:(.bss+0xa8): first defined here collect2.exe: error: ld returned 1 exit status

raedrizqie commented 2 years ago

why not linking readline with ncurses instead?

revelator commented 2 years ago

at some point i think it did but it was reverted because of some other problems, try it out and see if everything works as expected :)

brisingraerowing commented 2 years ago

I got around this when building some statically linked utility programs by rebuilding Termcap with the PC variable renamed to TERMCAP_PC (there's 4 places, 2 in termcap.h and 2 in termcap.c). It doesn't seem to be used anywhere else, so it's probably safe.

mcuee commented 1 year ago

I got around this when building some statically linked utility programs by rebuilding Termcap with the PC variable renamed to TERMCAP_PC (there's 4 places, 2 in termcap.h and 2 in termcap.c). It doesn't seem to be used anywhere else, so it's probably safe.

This work well. Thanks.

mcuee commented 1 year ago

Hopefully this issue can be fixed so that avrdude can use readline properly with MSYS2 mingw32/64.

brisingraerowing commented 1 year ago

I'm working on my static utilities again, and unfortunately this is not fixed. I still get multiple definition errors refering to the PC variable when linking readline with anything (found when building PCRE2). This also still breaks when something links both readline and NCurses (like gettext does).