SCons: Add 'split_libmodules' option to workaround linker issue #34227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@akien-mga - try this change without your current changes and see if that resolves the issue: bdbaddog@ea7b293 |
|
@bdbaddog That seems to work well for I still get the same linking issue eventually as I do with this workaround PR, not sure yet why: |
|
Not sure what's causing that. Could it be the order of the built libraries isn't correct. .a's only have the .o's with undefined references to them pulled out and linked. and/or you may need a flag to tell the linker to always include all .o's from the .a's.. ? |
|
I did some testing but I don't understand what causes the issue. As you suggested, I tried to ensure that the linker will parse the diff --git a/modules/SCsub b/modules/SCsub
index 42d89d6ce2..50d75e2009 100644
--- a/modules/SCsub
+++ b/modules/SCsub
@@ -1,5 +1,7 @@
#!/usr/bin/env python
+import os
+
Import('env')
env_modules = env.Clone()
@@ -21,4 +23,5 @@ if env.split_modules:
else:
lib = env_modules.add_library("modules", env.modules_sources)
- env.Prepend(LIBS=[lib])
+ env.Append(LINKCOM=" -Wl,--start-group {} -Wl,--end-group".format(os.path.join("modules", env.GetBuildPath(lib)[0])))
+ print(env['LINKCOM'])
But it still fails with the same error. It's puzzling since as per the I tested using MinGW on Linux and there it does not have any issue linking, so it might be a bug in the Windows version of Edit: For the reference, I'm building with |
akien-mga
force-pushed
the
scons-mingw-split-libmodules
branch
from
7cd7dce
to
7bc80bf
Compare
akien-mga
changed the title
akien-mga
force-pushed
the
scons-mingw-split-libmodules
branch
from
7bc80bf
to
9cae81b
Compare
akien-mga
changed the title
akien-mga
commented
Dec 11, 2019
| @@ -294,7 +294,10 @@ def configure_mingw(env): | |||
| ## Compiler configuration | |||
|
|
|||
| if (os.name == "nt"): | |||
| env['ENV']['TMP'] = os.environ['TMP'] # way to go scons, you can be so stupid sometimes | |||
There was a problem hiding this comment.
This was redundant with a call already made in configure() (and we can do without the comment...).
The new 'split_libmodules=yes' option is useful to work around linker command line size limitations when linking a huge number of objects. We're currently over 64k chars when linking libmodules.a on Windows with MinGW, which triggers issues as seen in godotengine#30892. Even on Linux, we can also reach linker command line size limitations by adding more custom modules. We force this option to True for MinGW on Windows, which fixes godotengine#30892. Additional changes to lib splitting: - Fix linking of the split module libs with interdependent symbols, hacking our way into LINKCOM and SHLINKCOM to set the `--start-group` and `--end-group` flags. - Fix Python 3 compatibility in `methods.split_lib()`. - Drop seemingly obsolete condition for 'msys' on 'posix'. - Drop the unnecessary 'split_drivers' as the drivers lib is no longer too big since we moved all thirdparty builds to modules. Co-authored-by: Hein-Pieter van Braam-Stewart <hp@tmm.cx>
akien-mga
force-pushed
the
scons-mingw-split-libmodules
branch
from
9cae81b
to
c320a82
Compare
akien-mga
commented
Dec 11, 2019
| env.Replace(ARFLAGS=['rcsT']) | ||
| lib = env_lib.add_library(libname + "_collated", lib_list) | ||
| lib_list = [lib] | ||
|
|
There was a problem hiding this comment.
Removing this was not strictly necessary for the use cases I tried (MinGW from MSYS2 on Windows and MinGW from Scoop in Powershell on Windows (both nt and win32), and MinGW from distro repos on Linux), but I'm not sure any recent MinGW distribution would be posix and msys.
So better drop this old code and make sure that all platforms that may use this method use the same code. If an issue pops up on a specific platform, we can handle it and document it as such.
I still plan to look further into this change, which would have been cleaner than what this PR eventually implements. My current hypothesis is that since we have more than 64k chars on the command line, |
|
@akien-mga - you can double check your hypothesis by getting a list of files included in the archive.
I'm dubious this is the case. I've never seen a command line too long silently drop the arguments. You usually get an error and then SCons would exit with error. |
|
Cherry-picked for 3.1.3. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new 'split_libmodules=yes' option is useful to work around linker
command line size limitations when linking a huge number of objects.
We're currently over 64k chars when linking libmodules.a on Windows
with MinGW, which triggers issues as seen in #30892.
Even on Linux, we can also reach linker command line size limitations
by adding more custom modules.
We force this option to True for MinGW on Windows, which fixes #30892.
Additional changes:
Additional changes to lib splitting:
hacking our way into LINKCOM and SHLINKCOM to set the
--start-groupand
--end-groupflags.methods.split_lib().too big since we moved all thirdparty builds to modules.
Co-authored-by: @hpvb