search

keyboardio / Kaleidoscope-Bundle-Keyboardio

A Kaleidoscope distribution for the Keyboardio Model 01 and other keyboards.
Other
17 stars 24 forks source link

Fix link order problems with inter-library/inter-plugin dependencies #15

Closed noseglasses closed 5 years ago

noseglasses commented 5 years ago

Before this change, when Arduino was linking the elf binary, it added all objects and archives (libraries/plugins) to a long linker command line. This would sometimes cause issues due to the linker's gargabe collection feature. When one library A referenced a symbol from a library B but library A appeared after B in the linker command line, the referenced symbol was in some cases garbage collected as to the linker it appeared to be unreferenced.

To understand this better it is important to know that the linker processes objects and archives one by one in the order of their appearance at the command line. For every object or archive encountered it runs garbage collection individually. That's why symbols might get discarded although an object or archives that is encountered later on while processing the linker command line might have referenced it.

To fix this problem this commit divides up the linker process into two steps.

First, all objects and archives are combined in a large .a archive. Then, this archive is passed to the original linker command line, thereby replacing the original set of objects and archives.

The final link now sees all symbols together. Thus, it is able to determine the true interdependencies and do a proper garbage collection without accidentally generating unresolved symbols.

With this fix, the order of appearance of include directives in the sketch file that formerly had an influence on the link order (as Arduino passes library archives to the linker in the order of the appearance of the matching includes in the sketch), can now be savely chosen arbitrarily.

Signed-off-by: Florian Fleissner florian.fleissner@inpartik.de

noseglasses commented 5 years ago

Travis testing seems somehow to be broken. Appears to be unrelated to this PR.

noseglasses commented 5 years ago

As a sidenote: Github events from Kaleidoscope-Bundle-Keyboardio are not (yet) forwarded to kaleidoscope-dev-notifications on discord.

obra commented 5 years ago

First up, this is amazing. Thank you.

I think it's worth some more explanatory comments in avr/platform.txt - this is a change from how Arduino does it, so we should be pretty loud about it, just to make sure nobody "fixes" it back to the way arduino did it two years from now.

Also, rather than commenting out the old version, just remove it. That's what we have version control history for :) ᐧ

On Thu, May 23, 2019 at 7:14 PM noseglasses notifications@github.com wrote:

Before this change, when Arduino was linking the elf binary, it added all objects and archives (libraries/plugins) to a long linker command line. This would sometimes cause issues due to the linker's gargabe collection feature. When one library A referenced a symbol from a library B but library A appeared after B in the linker command line, the referenced symbol was in some cases garbage collected as to the linker it appeared to be unreferenced.

To understand this better it is important to know that the linker processes objects and archives one by one in the order of their appearance at the command line. For every object or archive encountered it runs garbage collection individually. That's why symbols might get discarded although an object or archives that is encountered later on while processing the linker command line might have referenced it.

To fix this problem this commit divides up the linker process into two steps.

First, all objects and archives are combined in a large .a archive. Then, this archive is passed to the original linker command line, thereby replacing the original set of objects and archives.

The final link now sees all symbols together. Thus, it is able to determine the true interdependencies and do a proper garbage collection without accidentally generating unresolved symbols.

With this fix, the order of appearance of include directives in the sketch file that formerly had an influence on the link order (as Arduino passes library archives to the linker in the order of the appearance of the matching includes in the sketch), can now be savely chosen arbitrarily.

Signed-off-by: Florian Fleissner florian.fleissner@inpartik.de

You can view, comment on, or merge this pull request online at:

https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/15 Commit Summary

  • Fix link order problems with inter-library/inter-plugin dependencies

File Changes

Patch Links:

- https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/15.patch

https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/15.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/15?email_source=notifications&email_token=AAALC2CCBPJTTPXFBW65XK3PWZU7ZA5CNFSM4HO5IC32YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GVNK2QQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2GK3HNRGDUJWKCECWTPWZU7ZANCNFSM4HO5IC3Q .

noseglasses commented 5 years ago

i added more comments to platform.txt and removed the commented original line.

FWIW, here's the community discussion that led to this PR: https://community.keyboard.io/t/new-plugin-kaleidoscope-led-fire/2872/4?u=noseglasses

obra commented 5 years ago

Thanks so much for this. Merged

gedankenexperimenter commented 5 years ago

@noseglasses – I decided to try this out on Kaleidoglyph, and got a "Malformed archive" error from avr-ar when building code that compiles successfully with the old platform.txt file. I have not seen this with a Kaleidoscope build, but the two are still similar enough in many ways that it may come up in Kaleidoscope at some point. Can you make any guesses about the cause?

I can provide a link to a build tree if that would help.

noseglasses commented 5 years ago

Can you give me some hint how to reproduce this? Was this a fresh build or a rebuild?

noseglasses commented 5 years ago

@algernon and @obra, this did not show up when I tested it but @gedankenexperimenter is right. Whatever he encounters with his version of Kaleidoscope could also happen with the dev HEAD.

I recommend to take back this PR until we have some sort of fix.

gedankenexperimenter commented 5 years ago

Can you give me some hint how to reproduce this? Was this a fresh build or a rebuild?

Alas, no. In fact, I can't even reproduce it now. I think it was probably a rebuild, but I'm not 100% certain I know what that means. It was a build of a tree that had been built before, with some innocuous lines of code commented out (unused private member variables of a class), built with the same platform.txt file (with this change). It failed, then I tried it with a reverted platform.txt, and it succeeded. Then I re-introduced this change, built it again, and it works. I wish I could give you more details.

I do think what I saw is noteworthy, but I wouldn't be inclined to revert this PR just yet. As I said, Kaleidoglyph does have some substantial differences, particularly in the way it #includes headers from the sketch directory in other source files, so it may be triggering something that is very unlikely to occur in Kaleidoscope. In any case, I may take some time to see if I can reproduce the error, with more detail, but I'm all out of time today.

noseglasses commented 5 years ago

In any case, I may take some time to see if I can reproduce the error, with more detail

Alright, I'll be happy to work on everything you provide. As linking the elf-binary is such a vital part of the build process, we should take this very serious.

noseglasses commented 5 years ago

I just tried a fresh build of the bundle on my Ubuntu 18.04 machine with Arduino 1.8.8, then a rebuild of an old repo that I had lying around. The latter probably behaves as a fresh build even when i merge the bundle repos master. That's because my /tmp dir where the ccache stuff goes is purged on every restart. Now I am going to try a build with a bundle repo version just before this PR, then updating to this PR to see if a rebuild causes trouble...

noseglasses commented 5 years ago

... no problems even with this type of forced rebuild.

algernon commented 5 years ago

I'm a bit late to the party, but I wanted to express my gratitude for this. I've seen linker errors way back when I started working on Kaleidoscope (then still KeyboardioFirmware), and it haunted me ever since, biting me again and again every once in a while. It won't anymore.

Thank you!

gedankenexperimenter commented 5 years ago

I was able to reproduce the "Malformed archive" problem with Kaleidoscope, by simply adding a dummy class variable to Kaleidoscope.h. See #16.

noseglasses commented 5 years ago

This PR was temporarily reverted for reasons discussed in #16.

noseglasses commented 5 years ago

A follow up of this PR was submitted as https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/17.