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

ALL and VITA: Link with GNU Make recursive flag #2727

Merged
merged 3 commits into from Jan 17, 2021

Conversation

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jan 10, 2021

There are three changes in this PR which are dependent, thus the grouping.

First one is to use $(LD) instead of $(CXX) when linking.
I think it's more coherent and makes it easier to grep linking places in makefiles.
This change shouldn't have any impact as we set LD to ${CXX} in configure script

Second one is to make all link commands Make recursive by prepending them with a +.
We can then use -flto=jobserver to let GCC handle LTO threading properly.
This basically tells to make to open 2 more FDs for the child process so it shouldn't have any impact when linker doesn't use it.
(That's what GNU make documentation implies)

Third one is to make use of -flto=jobserver on Vita builds instead of hardcoding a 20 cores value which can have negative impacts on some servers.
Specifying number of cores for compilation is useless and is removed.

It will be more coherent.
LD is defined to CXX in configure script so it shouldn't change
behaviour.
With this make gives access to its jobserver for linker which will use
it when linking with -flto=jobserver
-flto=jobserver is present in GCC since version 7 at least
@lephilousophe lephilousophe requested review from sev-, criezy and ccawley2011 Jan 10, 2021
@henke37
Copy link
Contributor

@henke37 henke37 commented Jan 10, 2021

I presume that this has been tested as not to confuse create_project?

@lephilousophe
Copy link
Member Author

@lephilousophe lephilousophe commented Jan 10, 2021

I don't see why create_project would be impacted by this change, I don't think it parses Makefiles.
Do you have a precise idea about what would fail?

@henke37
Copy link
Contributor

@henke37 henke37 commented Jan 10, 2021

As a matter of fact, it does parse the make files. It happens in ProjectProvider::createModuleList in the create_project.cpp file.

@lephilousophe
Copy link
Member Author

@lephilousophe lephilousophe commented Jan 10, 2021

It seems it reads variables definitions (and ifeq, ...) and not recipes.

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jan 16, 2021

Looking good. +1 from me.

@sev-
Copy link
Member

@sev- sev- commented Jan 17, 2021

Thanks, merging

@sev- sev- merged commit cd34eaa into scummvm:master Jan 17, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.