Skip to content

Conversation

@dnadlinger
Copy link
Member

@dnadlinger dnadlinger commented Jun 28, 2016

Currently, we are translating the linker flags for LLVM/libconfig and from CMAKE_EXE_LINKER_FLAGS into -L options. Since DMD/LDC prefix -L options by -Xlinker before passing them to the GCC driver (causing the latter to forward it directly to the underlying ld), this does not work in the general case, causing issues such as #1494.

In #1468, a few driver-only flags were special-cased in LDC so that -Xlinker is not applied to them. However, I don't think this is a viable strategy in the long run. First of all, there are quite a few more options which are supposed to be passed to the linker driver instead of the linker. But rather more importantly, this only works with an LDC version that contains this fix, and not DMD.

Thus, we need a solution that works on the build-system level. One possibility would be to white-list flags we know how to handle in the CMake scripts (e.g. all of them starting with -Xlinker or -Wl,…, where we can just strip off the prefix and pass them to the D compiler using -L), and just ignore all the others. However, for some common use cases, we do want to pass flags to the linker driver, such as to link against a static C++ standard library when building binary releases.

Since there is no DMD flag for passing flags to the linker driver (we could of course add one in the next LDC version, but this wouldn't help with DMD or older versions), this PR changes the build system to again use the system linker directly from CMake instead of going through the D compiler for linking. Compared to @JohanEngelen's earlier version (sorry!), this now parses the -v output to determine the default linker flags used internally by the D compiler instead of trying to guess them.

Fixes #1494, #1544.

@JohanEngelen
Copy link
Member

This could actually speed-up re-builds quite a bit when the D-source hasn't changed! :-)

@dnadlinger
Copy link
Member Author

Yep. I'm also happy to receive suggestions on how to make this more robust, but doing this in CMake is a bit of a pain.

@kinke
Copy link
Member

kinke commented Jun 28, 2016

a bit of a pain

That's an understatement. :P

@dnadlinger dnadlinger changed the title [WIP] cmake: Directly use (g)cc for linking on Unix-like systems cmake: Directly use (g)cc for linking on Unix-like systems Jun 28, 2016
@dnadlinger
Copy link
Member Author

@redstar: Assuming we can get this to work reliably, this should probably go into both 1.0 and 1.1, as it should conclusively solve the various build issues people were having after the switch to the D frontend. Of course, we need to make sure that it is reasonably robust still.

@dnadlinger dnadlinger force-pushed the direct-system-linker branch 2 times, most recently from bb59eac to eee3d1c Compare July 4, 2016 17:31
@kinke
Copy link
Member

kinke commented Jul 6, 2016

Wrt. the AppVeyor failure:

c:\projects\ldc2-1.0.0-win64-msvc\bin\ldmd2.exe "-wi -g -O -inline -release -JC:/projects/ldc/ddmd -IC:/projects/ldc -IC:/projects/ninja-ldc -version=IN_LLVM_MSVC -version=IN_LLVM" "-L/NODEFAULTLIB:libcmt -L/NODEFAULTLIB:libvcruntime" -L"C:/projects/libconfig/lib/x64/ReleaseStatic/libconfig.lib"...

I guess the problem are the quotes in the first 2 arguments:

  1. "-wi -g -O -inline -release -JC:/projects/ldc/ddmd -IC:/projects/ldc -IC:/projects/ninja-ldc -version=IN_LLVM_MSVC -version=IN_LLVM"
  2. "-L/NODEFAULTLIB:libcmt -L/NODEFAULTLIB:libvcruntime"

The other args are okay.

@dnadlinger
Copy link
Member Author

dnadlinger commented Jul 15, 2016

Could somebody on Windows give this a quick try? I'd rather we don't ship another release that breaks on various distros, but I can't seem to make sense of the way the separate_arguments business is supposed to work. (Away from my machine with the Windows VMs, unfortunately.)

@kinke
Copy link
Member

kinke commented Jul 17, 2016

@klickverbot: This works for me:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -581,14 +581,21 @@ function(build_d_executable output_exe compiler_args linker_args dependencies)
             DEPENDS ${object_file}
         )
     else()
+        set(dflags "${D_COMPILER_FLAGS} ${DDMD_DFLAGS} ${DDMD_LFLAGS}")
         set(lflags ${LDC_TRANSLATED_LINKER_FLAGS})
-        separate_arguments(lflags)
+        if(UNIX)
+          separate_arguments(dflags UNIX_COMMAND "${dflags}")
+          separate_arguments(lflags UNIX_COMMAND "${lflags}")
+        else()
+          separate_arguments(dflags WINDOWS_COMMAND "${dflags}")
+          separate_arguments(lflags WINDOWS_COMMAND "${lflags}")
+        endif()
         foreach(f ${linker_args})
             append("-L\"${f}\"" lflags)
         endforeach()
         add_custom_command(
             OUTPUT ${output_exe}
-            COMMAND ${D_COMPILER} ${D_COMPILER_FLAGS} ${DDMD_DFLAGS} ${DDMD_LFLAGS} ${lflags} -I${PROJECT_SOURCE_DIR}/${DDMDFE_PATH} -of${output_exe} ${compiler_args} ${linker_args}
+            COMMAND ${D_COMPILER} ${dflags} ${lflags} -I${PROJECT_SOURCE_DIR}/${DDMDFE_PATH} -of${output_exe} ${compiler_args} ${linker_args}
             WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
             DEPENDS ${dependencies}
         )

@kinke
Copy link
Member

kinke commented Jul 24, 2016

We need this in 1.1 too. ;)

@dnadlinger dnadlinger force-pushed the direct-system-linker branch from eee3d1c to 87ab648 Compare July 24, 2016 15:45
@dnadlinger
Copy link
Member Author

@kinke: Added your patch, thanks. Let's see what the CI systems have to say.

@kinke
Copy link
Member

kinke commented Jul 24, 2016

Interesting, LDC 1.1 alpha as front-end compiler now doesn't work anymore for Travis due to multiple definitions of pragma(LDC_extern_weak) void* _d_execBssBegAddr...

@JohanEngelen
Copy link
Member

JohanEngelen commented Jul 24, 2016

@kinke Is it a build with -O3? Cross-module inlining gone bad can result in multiple definition problems maybe. What happens when you build with -disable-cross-module-inlining ?

@dnadlinger
Copy link
Member Author

@JohanEngelen: Somehow the build seems to ends up with __main.o and ldmd2.o, and _d_execBss{Beg, End}Addr being emitted into both of them (the weak declarations that @kinke mentioned are most probably not the issue, but the actual definition by the compiler code).

@kinke
Copy link
Member

kinke commented Jul 25, 2016

@JohanEngelen: I was just trying to summarize the Travis logs. The only difference wrt. the previous successful Travis runs (before the latest commit) is/should be that the arguments in D_COMPILER_FLAGS, DDMD_DFLAGS and DDMD_LFLAGS are now separated too (I thought that was unnecessary for Unixoids and only relevant for Windows...). That could be easily checked by diffing the generated makefiles.

I don't get why there's a ldmd2.o and a ldc2.o. There are no corresponding .cpp modules, so they seem to be dummy object files for the 2 executables (with that name)...

@dnadlinger dnadlinger force-pushed the direct-system-linker branch from 87ab648 to e67412d Compare July 26, 2016 17:49
@dnadlinger
Copy link
Member Author

I don't get why there's a ldmd2.o and a ldc2.o.

They are the object files for the D parts of LDMD and LDC, or at least supposed to be. The idea is to then use the system linker with the flags extracted from -v to link in Phobos/…, but also the C++ parts as usual with the flags extracted from llvm-config, any environment variables, etc.

@kinke
Copy link
Member

kinke commented Jul 26, 2016

Thanks, so how did you just fix it? :)

dnadlinger and others added 4 commits July 27, 2016 00:25
This allows us to properly take CMAKE_EXE_LINKER_FLAGS into
account without running into gcc/ld layering issues.

GitHub: Fixes ldc-developers#1494, alternative fix for ldc-developers#1460.
It is somewhat unclear whether the variable can actually be set
in a valid way at this point, though.
@dnadlinger dnadlinger force-pushed the direct-system-linker branch from e67412d to 557a256 Compare July 26, 2016 23:25
@dnadlinger
Copy link
Member Author

dnadlinger commented Jul 26, 2016

@kinke: Apparently, we were emitting __main.o when using the -main switch (as previously done in ExtractDMDSystemLinker.cmake), and that then didn't get stripped off the command line. The CMake build should be using LDMD though, and hence -singleobj, so I'm not sure what the deal was there. I've replaced the use of -main by writing void main() {} to the test file.

@dnadlinger
Copy link
Member Author

@JohanEngelen: Okay to merge?

@JohanEngelen
Copy link
Member

Looks good to me.

Things for future work:

  • source dependencies are a little off somehow: if I change a .cpp file, the D source is still recompiled too.
  • compile the D source per file, instead of all at once (with cross-module inlining in 1.1.0, the built compiler performance should be the same; we must not forget to check!). That should help the ARM buildbots that timeout on the current long compile time.

@JohanEngelen
Copy link
Member

(it works without issue on my machine too btw)

@JohanEngelen JohanEngelen added this to the 1.1.0 milestone Jul 29, 2016
@dnadlinger dnadlinger merged commit 7974fdd into ldc-developers:master Jul 29, 2016
@dnadlinger dnadlinger deleted the direct-system-linker branch July 29, 2016 14:40
@dnadlinger
Copy link
Member Author

Okay, finally merging, then. ;)

This should make our build process fundamentally more robust, since we are no longer trying to pass GCC linker flags directly to ld. There might be some more implementation issues to iron out initially, though, since CMake isn't exactly amenable to defensive programming and testing.

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.

unrecognized option '-Wl,--as-needed'

3 participants