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

Fix issue #3795 - Fix LDC linked dynamically with Phobos and DRuntime when built with GDMD. #3810

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbankov-vmware
Copy link
Contributor

@dbankov-vmware dbankov-vmware commented Aug 11, 2021

Prepend arguments starting with "-B" in D_LINKER_ARGS with "-Wl," when
using GDMD. The latter is needed because the arguments reported by GDMD
are what is passed to COLLECT2 while D_LINKER_ARGS are later passed to
CXX. The problem with that is that without "-Wl," prefix the -Bstatic
and -Bdynamic arguments before and after -lgphobos and -lgdruntime are
dropped. And that is a problem because without these the produced LDC is
linked dynamically to libgphobos and libgdruntime so these have to be
available whereever LDC is used.

Once libgphobos and libgdruntime are linked statically symbol conflicts
for _d_allocmemory, _d_newclass, _d_newitemiT and _d_newitemT symbols
were revealed which are fixed by checking if these symbols are marked as
weak in libgdruntime.a and if not "-Wl,-allow-multiple-definition" link
option is added to avoid link failure.

@dbankov-vmware dbankov-vmware changed the title Fix for 3795 Fix for #3795 Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix for #3795 Fix issue https://github.com/ldc-developers/ldc/issues/3795 Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix issue https://github.com/ldc-developers/ldc/issues/3795 Fix issue 3795 Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix issue 3795 Fix issue #3795 - Fix LDC linked dynamically to Phobos and DRuntime when built with GDMD. Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix issue #3795 - Fix LDC linked dynamically to Phobos and DRuntime when built with GDMD. Fix issue #3795 - Fix LDC linked dynamically with Phobos and DRuntime when built with GDMD. Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix issue #3795 - Fix LDC linked dynamically with Phobos and DRuntime when built with GDMD. Fix issue #3795 - Fix LDC linked dynamicallywith Phobos and DRuntime when built with GDMD. Aug 11, 2021
@dbankov-vmware dbankov-vmware changed the title Fix issue #3795 - Fix LDC linked dynamicallywith Phobos and DRuntime when built with GDMD. Fix issue #3795 - Fix LDC linked dynamically with Phobos and DRuntime when built with GDMD. Aug 11, 2021
dmd/root/rmem.d Outdated
@@ -220,9 +220,12 @@ static if (OVERRIDE_MEMALLOC)
// That scheme is faster and comes with less memory overhead than using a
// disabled GC alone.

extern (C) void* _d_allocmemory(size_t m_size) nothrow
version (LDC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too restrictive if it only meant to fix building with GDC. (e.g. this now also changes the binary when compiling with DMD)
If GDC does not support this code, then lines 204-209 should be changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as I already said in #3795 (comment). But that's upstream code, i.e., DMD would be affected by the same problem. Overriding the symbols seems to work fine with shared GDC druntime, only the static one seems problematic (no @weak for GDC @ibuclaw?). So I'm clearly against patching LDC here in this respect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the reviews!

If I understand correctly newer versions of GDC (I'm using GDC from GCC 9.3) should not have the duplicated symbols problem. So is there a way to figure out if GDC would work (e.g. check its version or something else) and only disable OVERRIDE_MEMALLOC for versions that are known not to work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__VERSION__ may be of help here. https://dlang.org/spec/lex.html#specialtokens

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @kinke says, please file the patch upstream (with dmd) https://github.com/dlang/dmd/blob/master/src/dmd/root/rmem.d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just able to bootstrap LDC with -DLDC_LINK_MANUALLY=OFF in our build infrastructure with this (dbankov-vmware/gdmd@3ae6155) patch on top of GDMD.

As I mentioned earlier it seems LDC build scripts expect that when -DLDC_LINK_MANUALLY=OFF the D compiler will link stdc++ automatically. Because of that expectation I've implemented -Xcc support in GDMD to automatically link stdc++ and also to honour -static-libstdc++ option so static linking to work as well (which is what is needed in my case).

@ibuclaw Please let me know if this patch looks acceptable for GDMD so to post a PR if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to mention that with GDMD from dbankov-vmware/gdmd@3ae6155 and -DLDC_LINK_MANUALLY=OFF I again hit the symbol conflicts when I use unmodified version of libgphobos (where these are not marked as weak).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned earlier it seems LDC build scripts expect that when -DLDC_LINK_MANUALLY=OFF the D compiler will link stdc++ automatically.

Now that's really something that's LDC-specific and should be fixed. This (LDC_LINK_MANUALLY=OFF on Posix) currently only works with ldmd2 host compilers, by instructing it to invoke the C++ compiler for linking, not the C compiler:

# We need to link against the C++ runtime library.
if(NOT MSVC AND "${D_COMPILER_ID}" STREQUAL "LDMD")
set(translated_linker_args "-gcc=${CMAKE_CXX_COMPILER}" ${translated_linker_args})
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I saw that and also for LDC_LINK_MANUALLY=ON it is the same:

set_target_properties(${target_name} PROPERTIES
RUNTIME_OUTPUT_DIRECTORY ${output_dir}
LINKER_LANGUAGE CXX
)

After fiddling so much with this I think there is a legitimate problem when linking C/C++ and D code which is that neither of the compilers knows how to link both languages correctly with their runtime/standard libraries. I haven't thought through this yet but it seems to me that what LDMD/LDC does is a good approach as the other options that I could think of (what I did in GDMD and generating the link line from scratch) seem worse to me. What I'm saying is that probably this doesn't need fixing in LDC build scripts but that maybe all D compilers should allow specifying the linking agent and should be able to pass through options from the customer to it but also include their own based on parameters that they recognise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented what I mentioned above for GDMD in D-Programming-GDC/gdmd#13 and with this version of GDMD am able to achieve what I need without touching LDC build scripts. Unfortunately I once again need a version of GCC with GDC which has _d_allocmemory, _d_newclass, _d_newitemiT and _d_newitemT symbols marked as weak so I'll post a PR with this update shortly.

Trying to figure out why bootstrapping LDC with both DMD and LDMD2 works without failures even though both link their versions of DRuntime and Phobos statically with LDC revealed that the duplicated symbols in both libdruntime.a (as well as in libdruntime-ldc.a) and libphobos.a (as well as in libphobos-ldc.a) are already marked as weak. Now for the LDC versions of the libraries this is achieved with the @weak attribute but for DMD ones it seems that all symbols are marked as weak by default and it was interesting to me (if somebody knows) wether this is the desired behaviour or has happened by accident.

@dbankov-vmware dbankov-vmware force-pushed the fix-for-3795 branch 2 times, most recently from adde711 to 87bb447 Compare August 17, 2021 09:50
Comment on lines +72 to +76
# CXX. The problem with that is that without "-Wl," prefix the -Bstatic
# and -Bdynamic arguments before and after -lgphobos and -lgdruntime are
# dropped. And that is a problem because without these the produced LDC is
# linked dynamically to libgphobos and libgdruntime so these have to be
# available whereever LDC is used.
Copy link
Contributor

@ibuclaw ibuclaw Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what you are describing is something specific to Debian and all its derivatives (Ubuntu, etc...)

https://salsa.debian.org/toolchain-team/gcc/-/blob/gcc-9-debian/debian/patches/gdc-dynamic-link-phobos.diff

There exists the driver option -static-libphobos which covers what looks to be your intent for messing around with -Bdynamic and -Bstatic.

So you can simplify this as just -static-libphobos -Wl,--allow-multiple-definition.

Copy link
Contributor Author

@dbankov-vmware dbankov-vmware Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibuclaw I'm building GDC from the source in GCC 9.3 and if used directly my GDC version links both Phobos and DRuntime statically by default.

The problem occurs only when building LDC because when -DLDC_LINK_MANUALLY=ON (which is the default for GDMD) instead of calling GDMD for linking LDC build script calls CXX for linking. Now to make sure that the correct libraries (phobos and druntime) are linked by CXX, LDC build script extracts the link line from a call to GDMD with -v and then copies all arguments which start with -L, -l and -B in a list which is later passed to CXX. The problem here is that for GDMD the link line reported by -v is what is passed to COLLECT2 (which are practically parameters passed to LD) and then when LDC build script passes these directly to CXX only parameters starting with -L and -l are honoured while these starting with -B are simply dropped. The result is that although the link line reported by GDMD contains -Bstatic before -lgphobos and -lgdruntime and -Bdynamic after these when CXX is linking LDC it drops the parameters starting -B and LDC ends up being linked dynamically with Phobos and DRuntime. What was needed was for parameters starting with -B from the link line reported by GDMD to be prepended with -Wl, in LDC build script so when these are passed to CXX it to correctly redirect them to LD.

with Phobos and DRuntime when built with GDMD.

Prepend arguments starting with "-B" in D_LINKER_ARGS with "-Wl," when
using GDMD. The latter is needed because the arguments reported by GDMD
are what is passed to COLLECT2 while D_LINKER_ARGS are later passed to
CXX. The problem with that is that without "-Wl," prefix the -Bstatic
and -Bdynamic arguments before and after -lgphobos and -lgdruntime are
dropped. And that is a problem because without these the produced LDC is
linked dynamically to libgphobos and libgdruntime so these have to be
available whereever LDC is used.

Once libgphobos and libgdruntime are linked statically symbol conflicts
for _d_allocmemory, _d_newclass, _d_newitemiT and _d_newitemT symbols
were revealed which are fixed by checking if these symbols are marked as
weak in libgdruntime.a and if not "-Wl,-allow-multiple-definition" link
option is added to avoid link failure.
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 this pull request may close these issues.

None yet

4 participants