Revert "Make: use gcc as LD" #3900

yamt · 2021-06-10T23:30:17Z

Summary

This reverts commit 45672c2.

Because:

It's very confusing to have cc as LD.
I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are
supposed to do when we use LD directly. It would be simpler to
remove them from our LDFLAGS.

Impact

Testing

xiaoxiang781216 · 2021-06-11T12:39:58Z

Because:

It's very confusing to have cc as LD.

Yes, it's a little bit confusing, but if you want to enable LTO, you must use gcc to drive the link process as far as I know. If we can't find a method to let ld work with LTO, it's better to keep the change as it.

I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are
supposed to do when we use LD directly. It would be simpler to
remove them from our LDFLAGS.

hartmannathan · 2021-06-11T15:11:49Z

Because:

It's very confusing to have cc as LD.

Yes, it's a little bit confusing, but if you want to enable LTO, you must use gcc to drive the link process as far as I know. If we can't find a method to let ld work with LTO, it's better to keep the change as it.

I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are
supposed to do when we use LD directly. It would be simpler to
remove them from our LDFLAGS.

Agreed that Link Time Optimization would be a good thing to make a reality, if possible. We may run into some additional fallout from the 45672c2 change, like what happened with clang (fixed in #3904). But so far it looks good.

yamt · 2021-06-13T23:53:28Z

Because:

It's very confusing to have cc as LD.

Yes, it's a little bit confusing, but if you want to enable LTO, you must use gcc to drive the link process as far as I know. If we can't find a method to let ld work with LTO, it's better to keep the change as it.

I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are
supposed to do when we use LD directly. It would be simpler to
remove them from our LDFLAGS.

iirc, linux supports LTO with ld. can you explain why it doesn't work for us?
anyway, LTO thing is a bit off-topic. the change in question was to fix arm-none-eabi-ld: Error: unable to disambiguate: -nostartfiles #3826 and my point is that it doesn't seem like an appropriate fix.

https://github.com/torvalds/linux/blob/009c9aa5be652675a06d5211e1640e02bbb1c33d/Makefile#L920
https://github.com/torvalds/linux/blob/009c9aa5be652675a06d5211e1640e02bbb1c33d/Makefile#L438

yamt · 2021-06-14T04:47:26Z

i played a bit with LTO.
https://github.com/yamt/garbage/tree/master/lto
for me, it seems working with ld.

also, it seems gcc folks are thinking ld should work.
http://gcc.gnu.org/wiki/whopr/driver

my impression is that it isn't a good idea to use LD=cc for the LTO purpose either.

gustavonihei · 2021-06-14T14:02:44Z

iirc, linux supports LTO with ld. can you explain why it doesn't work for us?

Linux Kernel support LTO with Clang-only so far, and using the LLD, which is the LLVM linker.

also, it seems gcc folks are thinking ld should work.
http://gcc.gnu.org/wiki/whopr/driver

I believe that wiki page might not be up-to-date, since it is already a bit old. Is there a specific detail from this guide you'd like to bring to the discussion?
If you look at the documentation for the -flto flag, it is clearly stated that:

The important thing to keep in mind is that to enable link-time optimizations you need to use the GCC driver to perform the link step

https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#:~:text=the%20important%20thing%20to%20keep%20in%20mind%20is%20that%20to%20enable%20link-time%20optimizations%20you%20need%20to%20use%20the%20gcc%20driver%20to%20perform%20the%20link%20step.

anyway, LTO thing is a bit off-topic. the change in question was to fix arm-none-eabi-ld: Error: unable to disambiguate: -nostartfiles #3826 and my point is that it doesn't seem like an appropriate fix.

I understand that a future support for LTO should not be the sole reason for that decision, and I agree that removing -nostarfiles and -nodefaultlibs is also a solution for the issue #3826.
But I don't see why the implemented solution is not appropriate. It indeed fixes the issue and it has the advantage of setting up the build system for a future improvement.

yamt · 2021-06-14T22:53:15Z

iirc, linux supports LTO with ld. can you explain why it doesn't work for us?

Linux Kernel support LTO with Clang-only so far, and using the LLD, which is the LLVM linker.

yes.

also, it seems gcc folks are thinking ld should work.
http://gcc.gnu.org/wiki/whopr/driver

I believe that wiki page might not be up-to-date, since it is already a bit old. Is there a specific detail from this guide you'd like to bring to the discussion?
If you look at the documentation for the -flto flag, it is clearly stated that:

The important thing to keep in mind is that to enable link-time optimizations you need to use the GCC driver to perform the link step

https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#:~:text=the%20important%20thing%20to%20keep%20in%20mind%20is%20that%20to%20enable%20link-time%20optimizations%20you%20need%20to%20use%20the%20gcc%20driver%20to%20perform%20the%20link%20step.

have you looked at the example i provided?
https://github.com/yamt/garbage/tree/master/lto
it includes gcc. to me it looks working as documented in the wiki.

anyway, LTO thing is a bit off-topic. the change in question was to fix arm-none-eabi-ld: Error: unable to disambiguate: -nostartfiles #3826 and my point is that it doesn't seem like an appropriate fix.

I understand that a future support for LTO should not be the sole reason for that decision, and I agree that removing -nostarfiles and -nodefaultlibs is also a solution for the issue #3826.
But I don't see why the implemented solution is not appropriate. It indeed fixes the issue and it has the advantage of setting up the build system for a future improvement.

as i stated, using cc as LD is very confusing. if you really want to use cc for linking, it's better to use CC. in some places we already do that. (grep CCLINKFLAG)
do you have a POC patch to enable LTO that way? otherwise the "setting up the build system for a future improvement" sounds moot.

juniskane · 2021-06-15T11:16:29Z

I have no opinion if this should be reverted or not, but doing changes like this repeatedly is hard for commercial users because all out-of-tree board Make.defs files needs to be modified every single time when something like this is either applied or reverted.

At least the -Wl, should have been expanded from make variable, say $(LDFLAGPREFIX), so that changing between gcc and ld does not need to touch so many files again.

hartmannathan · 2021-06-16T15:06:30Z

I have no opinion if this should be reverted or not, but doing changes like this repeatedly is hard for commercial users because all out-of-tree board Make.defs files needs to be modified every single time when something like this is either applied or reverted.

I agree that we should not cause a lot of additional work to downstream users.

At least the -Wl, should have been expanded from make variable, say $(LDFLAGPREFIX), so that changing between gcc and ld does not need to touch so many files again.

I think that is a good idea.

The good news is: although #3836 is on master, it has not been released. A make variable could be added now. Commercial users who work with official releases will only have to make 1 change, not 2, if we do it now, before the next release.

(Users who track master are sure to experience a bumpy road no matter what, so if they've updated their boards already, they will have to do it again; but this could be easier than the first time because the diff of the 1st change will show them exactly where to make the 2nd change.)

yamt · 2021-06-16T23:24:02Z

my suggestion is

revert the change on master for now
do a LTO POC for both of gcc and clang, to confirm if using cc for linking is the way to go for us.
if it is, use CC (or a new var like CCLINK), not "cc as LD".

yamt · 2021-07-05T03:55:05Z

@xiaoxiang781216 @gustavonihei
the discussion seemed settled. can you consider merging this? or are you working on alternatives?

xiaoxiang781216 · 2021-07-05T06:43:46Z

I am fine with either approach, but could you incorporate your LTO demo into mainline? Otherwise, the same change may be created again for LTO.

yamt · 2021-07-06T04:07:57Z

I am fine with either approach, but could you incorporate your LTO demo into mainline? Otherwise, the same change may be created again for LTO.

maybe i will try.
however, honestly, i feel it's very unfair to require me to do the lto work while it was not and will not required for "gcc as ld" supporters. after all, lto is their reasoning, not mine.

xiaoxiang781216 · 2021-07-07T08:56:26Z

@gustavonihei was your opinion?

gustavonihei · 2021-07-07T17:49:22Z

@gustavonihei was your opinion?

My opinion is somewhat like yours, I am fine either way.
I am more inclined to simply drop this PR. I don’t agree with the “confuse” reasoning. But I won’t oppose to the decision of merging it also.

yamt · 2021-07-19T13:01:55Z

@xiaoxiang781216 @gustavonihei

i haven't found a chance to investigate the lto things.
you haven't either, it guess.
on the other hand, the "cc as LD" thing seems still causing issues. #4173
how about merging this revert for now?

yamt · 2021-07-19T13:33:33Z

btw, i updated my demo to use a library. it seems working for both of gcc and clang so far.
yamt/garbage@506efb8

Ouss4 · 2021-07-21T08:15:52Z

@xiaoxiang781216 @gustavonihei @AlexanderVasiljev are we going to have more PRs like #4173? Otherwise we should merge this one, it's been sitting here for a while.

… Clang" This reverts commit ba1f730.

…ld to gcc": This reverts commit 6545b95.

This reverts commit 45672c2. Because: * It's very confusing to have cc as LD. * I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are supposed to do when we use LD directly. It would be simpler to remove them from our LDFLAGS.

Remove -Wl, in LDFLAGS.

These options are not ld options. The recent versions of ld complain on them. https://sourceware.org/pipermail/binutils/2021-June/116826.html

…ilds" This reverts commit 6c2c70c.

xiaoxiang781216 · 2021-09-07T02:59:48Z

@gustavonihei and @Ouss4 how about we merge this PR? because using gcc as ld doesn't work well with clang toolchain. I can provide lto demo with ld after this PR merge.

acassis · 2021-09-11T20:31:02Z

@yamt @xiaoxiang781216 please take a look, this Revert brought back an old issue, now all ARM and MIPS boards are breaking because of it:

LDFLAGS += -nostartfiles ...

"arm-none-eabi-ld: Error: unable to disambiguate: -nostartfiles (did you mean --nostartfiles ?)"

As @hartmannathan reported here #3209 it will happen if "Binutils is updated to 2.36.x". So probably our CI is using an old version and the problem doesn't show up.

acassis · 2021-09-11T20:48:13Z

I thought about the idea of going ahead and removing "-nostartfiles -nodefaultlibs" from all LDFLAGS, but we cannot do it without double check each toolchain. For example:

# Pinguino toolchain under Linux

ifeq ($(CONFIG_MIPS32_TOOLCHAIN),PINGUINOL)

This is a toolchain that use to be very outdated and removing those flags could bring unexpected results.

hartmannathan · 2021-09-12T01:49:04Z

On Sat, Sep 11, 2021 at 4:31 PM Alan Carvalho de Assis < ***@***.***> wrote: @yamt <https://github.com/yamt> @xiaoxiang781216 <https://github.com/xiaoxiang781216> please take a look, this Revert brought back an old issue, now all ARM and MIPS boards are breaking because of it: LDFLAGS += -nostartfiles ... "arm-none-eabi-ld: Error: unable to disambiguate: -nostartfiles (did you mean --nostartfiles ?)" As @hartmannathan <https://github.com/hartmannathan> reported here #3209 <#3209> it will happen if "Binutils is updated to 2.36.x". So probably our CI is using an old version and the problem doesn't show up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3900 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOD4O5ZXWNJNAKYGDKDMUW3UBO4BDANCNFSM46PSFL7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

In the CWIKI, I wrote a more detailed explanation: https://cwiki.apache.org/confluence/display/NUTTX/NuttX+10.2?desktop=true ¯oName=markdown#ld-now-called-through-gcc

yamt mentioned this pull request Jun 14, 2021

Make: -nostartfiles -nodefaultlibs are not flags of LD but flags of GCC #3836

Merged

yamt force-pushed the revert5 branch from 91fd5cd to 51a0fe5 Compare June 16, 2021 23:16

yamt force-pushed the revert5 branch from 6e722ce to bd14fb9 Compare July 19, 2021 12:55

yamt added 6 commits July 30, 2021 12:38

Revert "sim: Use cc as linker, which usually aliases to either GCC or…

04537a9

… Clang" This reverts commit ba1f730.

Revert "boards/sim/sim/sim: correct LDLINKFLAGS after migrating from …

92215ed

…ld to gcc": This reverts commit 6545b95.

Revert "Make: use gcc as LD"

44d9938

This reverts commit 45672c2. Because: * It's very confusing to have cc as LD. * I don't see what "-nostartfiles -nodefaultlibs" in LDFLAGS are supposed to do when we use LD directly. It would be simpler to remove them from our LDFLAGS.

nucleo-g431kb: Update after the revert of "Make: use gcc as LD"

9bbdc80

Remove -Wl, in LDFLAGS.

arch/avr: Remove -nostartfiles -nodefaultlibs from LDFLAGS

f24823b

These options are not ld options. The recent versions of ld complain on them. https://sourceware.org/pipermail/binutils/2021-June/116826.html

Revert "Make: fix use of gcc as LD for modules and ELF executables bu…

711bbe0

…ilds" This reverts commit 6c2c70c.

yamt force-pushed the revert5 branch from bd14fb9 to 711bbe0 Compare July 30, 2021 03:41

xiaoxiang781216 approved these changes Sep 7, 2021

View reviewed changes

jerpelea self-requested a review September 7, 2021 06:32

jerpelea approved these changes Sep 7, 2021

View reviewed changes

Ouss4 approved these changes Sep 7, 2021

View reviewed changes

xiaoxiang781216 merged commit c4216d0 into apache:master Sep 8, 2021

Ouss4 mentioned this pull request Sep 10, 2021

nucleo-g431kb build error #4510

Closed

Ouss4 added this to To-Add in Release Notes - 10.2 Oct 11, 2021

jerpelea moved this from To-Add to Minor in Release Notes - 10.2 Oct 13, 2021

xiaoxiang781216 mentioned this pull request Jan 26, 2022

arch/Toolchain.defs: Simplify addition builtin library to EXTRA_LIBS #5332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Make: use gcc as LD" #3900

Revert "Make: use gcc as LD" #3900

yamt commented Jun 10, 2021

xiaoxiang781216 commented Jun 11, 2021 •

edited

Loading

hartmannathan commented Jun 11, 2021

yamt commented Jun 13, 2021

yamt commented Jun 14, 2021 •

edited

Loading

gustavonihei commented Jun 14, 2021 •

edited

Loading

yamt commented Jun 14, 2021

juniskane commented Jun 15, 2021

hartmannathan commented Jun 16, 2021

yamt commented Jun 16, 2021

yamt commented Jul 5, 2021

xiaoxiang781216 commented Jul 5, 2021

yamt commented Jul 6, 2021

xiaoxiang781216 commented Jul 7, 2021

gustavonihei commented Jul 7, 2021

yamt commented Jul 19, 2021 •

edited

Loading

yamt commented Jul 19, 2021

Ouss4 commented Jul 21, 2021

xiaoxiang781216 commented Sep 7, 2021 •

edited

Loading

acassis commented Sep 11, 2021

acassis commented Sep 11, 2021 •

edited

Loading

hartmannathan commented Sep 12, 2021 via email

Revert "Make: use gcc as LD" #3900

Revert "Make: use gcc as LD" #3900

Conversation

yamt commented Jun 10, 2021

Summary

Impact

Testing

xiaoxiang781216 commented Jun 11, 2021 • edited Loading

hartmannathan commented Jun 11, 2021

yamt commented Jun 13, 2021

yamt commented Jun 14, 2021 • edited Loading

gustavonihei commented Jun 14, 2021 • edited Loading

yamt commented Jun 14, 2021

juniskane commented Jun 15, 2021

hartmannathan commented Jun 16, 2021

yamt commented Jun 16, 2021

yamt commented Jul 5, 2021

xiaoxiang781216 commented Jul 5, 2021

yamt commented Jul 6, 2021

xiaoxiang781216 commented Jul 7, 2021

gustavonihei commented Jul 7, 2021

yamt commented Jul 19, 2021 • edited Loading

yamt commented Jul 19, 2021

Ouss4 commented Jul 21, 2021

xiaoxiang781216 commented Sep 7, 2021 • edited Loading

acassis commented Sep 11, 2021

acassis commented Sep 11, 2021 • edited Loading

hartmannathan commented Sep 12, 2021 via email

xiaoxiang781216 commented Jun 11, 2021 •

edited

Loading

yamt commented Jun 14, 2021 •

edited

Loading

gustavonihei commented Jun 14, 2021 •

edited

Loading

yamt commented Jul 19, 2021 •

edited

Loading

xiaoxiang781216 commented Sep 7, 2021 •

edited

Loading

acassis commented Sep 11, 2021 •

edited

Loading