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

LTO configuration and workarounds touchup and two new workarounds (take two) #464

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

elsandosgrande
Copy link
Contributor

@elsandosgrande elsandosgrande commented Jan 11, 2020

#452, but clean.

@InBetweenNames, @wolfwood, here I am, hoping that I do not end up having to do this song and dance every single time.

Edit

  1. Well, if I do end up having to cherry-pick every single time, I think that I can plan ahead in the future, making this not so difficult.
  2. My question from earlier still stands.
  3. Thank you!!!

While this basically makes `-fwhole-program` unusable by default, it's
not a bad default to have. As there is a mixture of strings and direct
flags being used in ltoworkarounds.conf (one line might have `*FLAGS-=-flto*`,
while another might have `*FLAGS-="${FLTO}"`), this can kind of be
remedied by using only direct flags. That way, if somebody were to wish
to use `-fwhole-program`, they would still be able to benifit from
ltoworkarounds.conf, since the file would now definitely work with all
setups, including those that might set some flags manually in make.conf, that is without using the strings from make.conf.lto.defines. Also, ${FLTO} was the only odd name out, so now it's ${LTO}.
So... Regarding my previous commit message, LTO is the one flag that is
never mentioned as a string. I got confused, but I was still correct
about mixed usage.
To think that **this** project would ever have the word "legacy" used anywhere...
@InBetweenNames
Copy link
Owner

Regarding your question, the reason why is that I am the one that is committing to the central GentooLTO repository. The committer is not necessarily the same as the author.

@InBetweenNames
Copy link
Owner

Is this ready to be merged?

@elsandosgrande
Copy link
Contributor Author

  1. Yes, it is ready.
  2. What is Matthew possibly doing then? According to what you said, he merges pull requests in a different way somehow. If so, is that related to the Olive repository being managed by a GitHub organization which he is the only member of (he created it)?

@elsandosgrande
Copy link
Contributor Author

@InBetweenNames

@InBetweenNames
Copy link
Owner

OK -- I'd like to see this PR broken up into a few smaller PRs. Namely, I think the FLTO => LTO change should be a separate thing, as conceptually it's different than the other changes. That includes both the changes in make.conf.lto and make.conf.lto.defines.

Second, if we're going to be getting rid of IPAPTA, we should probably remove the variable too (but keep the corresponding comments in the .defines file. The same should apply to SEMINTERPOS.

Third, the new workarounds you're adding should be in a separate PR than these other two, because we need to be able to track those in the git log. I can't count the number of times I've needed to track down when something was added via git blame.

In principle these changes look good -- with these few small adjustments they should get merged in no time.

@InBetweenNames
Copy link
Owner

Regarding your question about what Matthew is doing over in the Olive project, he probably isn't merging directly on GitHub but doing it manually via the command line. He may be using git am or even just manually applying the diffs and committing directly. Your best bet is to ask him directly.

@elsandosgrande
Copy link
Contributor Author

Hm... The confirmation message looks to be the same as when you accept pull requests.

merged commit f07dd69 into olive-editor:master

I will ask him then (it could take a while...).

Wonders why this request is still open instead of accepted

@elsandosgrande
Copy link
Contributor Author

@InBetweenNames

@InBetweenNames
Copy link
Owner

Okay, I reviewed your individual commits and I'm satisfied -- but please in the future don't do mega PRs. There should be, in general, be one change per PR, whether that's adding or removing a workaround (or related set of workarounds), renaming a variable, or clean-up work. Anything that makes me spend more than 10 seconds reviewing is not likely to get accepted right away.

@InBetweenNames
Copy link
Owner

Okay, I spoke too soon. There is a problematic part of this patch series:

# BEGIN: GNU linker workarounds
<=media-gfx/blender-2.79b-r1 *FLAGS-="-fuse-ld=bfd" *FLAGS+="-fuse-ld=gold" #381
# END: GNU linker workarounds

So ltoize should depend on binutils[gold] for sure if we're going to do this. I have noticed that some packages build with gold that don't build with ld.bfd. I'm fine with this kind of workaround but we do need to make the dependency on gold explicit. I would like to see this change in particular moved to its own PR.

Also, I can't make changes directly on this myself because I would be committing to master on your repository, and I don't want to break things on your end.

So, my advice is the following:

  • Do a git rebase -i and remove commit: 1a837aa -- consider saving the diff to a file somewhere.
  • Force push your changes to your github repository. This will automatically update the PR here. As long as there are no other showstoppers I haven't noticed, I'll accept it.
  • Then, create a new commit for the -fno-semantic-interposition workaround you added. Make a PR for this.
  • Then, create a new commit with the GNU linker workarounds. Make a PR for this -- version bump sys-config/ltoize and make it depend on binutils[gold].

If you do this, your stuff should be accepted in no time.

@elsandosgrande
Copy link
Contributor Author

  1. I will be closing this pull request and making sequential ones (rebasing has always given me headaches and I am not very experienced with Git, so the situation is as discussed on Gitter).
  2. Um... I do not yet know how to write an ebuild.

Realizes that, due to how ebuilds work, some of what I said on Gitter is unnecessary

All right, sequential requests it is. Now, off to very late sleep. Oh, and I ended up just making all of the changes at once, so it all ended up in one request. I shall be much more careful in the future.

Prays for forgiveness

@InBetweenNames
Copy link
Owner

No harm, no foul -- this is how learning is done!

@nivedita76
Copy link

fuse-linker-plugin option is not necessary: it is enabled by default if you have a linker newer than 2.21 which is ages ago (not even available in gentoo).

@elsandosgrande
Copy link
Contributor Author

@InBetweenNames What do you say to that?

Also, the power went out twice. I am still recompiling everything with GCC 10 pre-release, just using a Xubuntu LiveUSB (Kubuntu is now over two gigabytes). As you might guess, I am currently on my phone.

@InBetweenNames
Copy link
Owner

On my system, GCC 9.3.0:

> gcc -Q  --help=common | grep fuse-linker                                                                                                                                                                      
  -fuse-linker-plugin                   [disabled]

@InBetweenNames
Copy link
Owner

Okay, I did some more reading and went into the GCC sources. Apparently it really is used as @nivedita76 said -- the only time it isn't used is if you explicitly pass -fno-use-linker-plugin or -fno-lto to GCC. Curiously, this does not seem to be equivalent to omitting -fuse-linker-plugin in the first place, even if doing so makes gcc -Q --help=common indicate that it is disabled. It's as if -fno-* is even stronger than a "disabled" default. Weird. We should probably look at removing -fuse-linker-plugin as it is implied.

@elsandosgrande
Copy link
Contributor Author

elsandosgrande commented Apr 14, 2020

@InBetweenNames

Great! 😃

I got a new laptop, but can't unscrew the last few stubborn screws to insert the HDD from my old one to get back on Gentoo (currently on Ubuntu Eoan). In the mean time, somebody in here could try to compile a package with and without the explicit invocation of the linker plugin to triple check that it remains implied.

Edit

I am currently caught up in this, so it might even be longer than the time that it takes for the screws to naturally loosen in a hot laptop.

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.

3 participants