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

Respect linking order of libraries #8263

Merged
merged 4 commits into from
Sep 1, 2021

Conversation

maxgerhardt
Copy link
Contributor

@maxgerhardt maxgerhardt commented Aug 9, 2021

Fixes #8262.

Arduino IDE link order:

-lhal -lphy -lpp -lnet80211 -llwip2-536-feat -lwpa -lcrypto -lmain -lwps -lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 -lstdc++ -lm -lc -lgcc

PlatformIO link order before this fix:

-lhal -lphy -lpp -lnet80211 -lwpa -lcrypto -lmain -lwps -lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 -lm -lc -lgcc -llwip2-536-feat -lstdc++

(^ which causes the linking failure in regards to the math library in the referenced issue)

PlatformIO link order after this fix:

-lhal -lphy -lpp -lnet80211 -llwip2-536-feat -lwpa -lcrypto -lmain -lwps -lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 -lstdc++ -lm -lc -lgcc

Aka, now equivalent to the Arduino IDE linking order.

The code refactors setting env["LIBS"] further down the script where it knows the system libraries to be built, and then creates the array with the correct linking order.

The previous env.Append(LIBS = ...) is wrong here because it appends to the back and hence does not respect the original linker order laid out in the platform.txt

compiler.c.elf.libs=-lhal -lphy -lpp -lnet80211 {build.lwip_lib} -lwpa -lcrypto -lmain -lwps -lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 {build.stdcpp_lib} -lm -lc -lgcc

Now has the same order as the Arduino IDE does with its platform.txt
Instead of injecting at magic indices, which might break when some other extra-scripts inject other libraries, let's create the LIBS array at the bottom in easy to understand and correct order.
@mcspr
Copy link
Collaborator

mcspr commented Aug 9, 2021

Initial LIBS could be left empty:

if EXCEPTIONS:
    CXX_LIBS = ["std++"]
else:
    CXX_LIBS = ["std++-noexc"]

SDK_LIBS = [
    "hal", "phy", "pp", "net80211", "wpa", "crypto", "main",
    "wps", "bearssl", "espnow", "smartconfig", "airkiss", "wpa2"
]

C_LIBS = [
    "m", "c", "gcc"
]

if BLAH:
    LWIP_LIBS = [ ... ]
# ...

env.Replace(LIBS=SDK_LIBS + LWIP_LIBS + CXX_LIBS + C_LIBS)

?

Hardcoded offsets are hard to parse at a glance

@maxgerhardt
Copy link
Contributor Author

I'm not quite sure if I follow? The hardcoded offsets have been removed with the latest commit because I didn't like that style either. The code at the bottom with env.Append(LIBS=<correct array>) is always reached (not inside an if), so it can't be left empty after the script.

@mcspr
Copy link
Collaborator

mcspr commented Aug 9, 2021

Github interface caching is weird, looking at the Files changes does not always update to the latest version when new commits come in. Latest update pretty much does what I proposed, so lgtm :)

@maxgerhardt
Copy link
Contributor Author

I think this is a straight-forward fix & improvement PR. Any thoughts on getting this merged?

@mcspr
Copy link
Collaborator

mcspr commented Aug 29, 2021

@ivankravets

@earlephilhower earlephilhower merged commit f7951e6 into esp8266:master Sep 1, 2021
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.

Sketch with math function fails to compile in PlatformIO
3 participants