This is an archive of the discontinued LLVM Phabricator instance.

Differential D106854

Pass `--start-group` and `--end-group` to the linker when using the AVR toolchain
ClosedPublic

Authored by mhjacobson on Jul 27 2021, 2:10 AM.

Details

Reviewers
dylanmckay
benshi001
Commits
rG1e6a93f15c7e: [AVR][clang] Pass '--start-group' and '--end-group' options to avr-ld
Summary

The way the GNU linker deals with static archives is that it only loads objects
from the archive that are needed by *previously seen objects*. That is, in this
case, if libm or libc depend on symbols from libgcc, they might be out of luck.

As it happens, it's common for avr-libc to depend on symbols (specifically, the
deduplicated function prologue/epilogue) from libgcc. The linker, as invoked by
clang currently, fails to link the program in this case because of the argument
ordering.

In general, libraries should be supplied to the linker in a dependents-then-
dependencies order.

However, to more perfectly match GCC's behavior here--and since we don't control
the libraries in question--simply use --start-group and --end-group. These
flags tell the linker to repeatedly search all the intervening libraries until
no new undefined symbol references are created.

Diff Detail

Unit TestsFailed

TimeTest
220 msx64 debian > Clang.Driver::avr-ld.c
3,090 msx64 debian > libarcher.critical::critical.c
3,220 msx64 debian > libarcher.critical::lock.c
3,520 msx64 debian > libarcher.races::critical-unrelated.c
3,440 msx64 debian > libarcher.races::lock-nested-unrelated.c
View Full Test Results (18 Failed)

Event Timeline

mhjacobson created this revision.Jul 27 2021, 2:10 AM
Herald added a subscriber: Jim. · View Herald TranscriptJul 27 2021, 2:10 AM
mhjacobson requested review of this revision.Jul 27 2021, 2:10 AM
Herald added a subscriber: cfe-commits. · View Herald TranscriptJul 27 2021, 2:10 AM
Harbormaster completed remote builds in B116371: Diff 361931.Jul 27 2021, 3:00 AM
mhjacobson updated this revision to Diff 362185.Jul 27 2021, 2:22 PM

Modified regression test Driver/avr-ld.c to account for the new arguments.

mhjacobson added a reviewer: benshi001.Jul 27 2021, 2:24 PM
Harbormaster completed remote builds in B116541: Diff 362185.Jul 27 2021, 4:58 PM
benshi001 added inline comments.Jul 27 2021, 7:56 PM
clang/lib/Driver/ToolChains/AVR.cpp
430

You are appreciated to use 'git show -U999999' or 'git diff -U999999' for better context display.

clang/test/Driver/avr-ld.c
23 ↗(On Diff #362185)

could you add explict options (which you added) to these tests ?

mhjacobson updated this revision to Diff 362275.Jul 27 2021, 9:26 PM

Updated the diff to add more context.

mhjacobson marked an inline comment as done.Jul 27 2021, 9:26 PM
mhjacobson added inline comments.
clang/lib/Driver/ToolChains/AVR.cpp
430

Sorry! Fixed, I think.

mhjacobson updated this revision to Diff 362277.Jul 27 2021, 9:39 PM
mhjacobson marked an inline comment as done.

Explicitly added --start-group and --end-group to the expectation in the test.

mhjacobson marked an inline comment as done.Jul 27 2021, 9:40 PM
Harbormaster completed remote builds in B116614: Diff 362277.Jul 27 2021, 10:35 PM
benshi001 accepted this revision.Jul 29 2021, 6:33 AM

Do you need I land your patch for you?

What name and email do you expected to see in the commit message ?

This revision is now accepted and ready to land.Jul 29 2021, 6:33 AM
benshi001 added a comment.EditedJul 29 2021, 6:42 AM

Do you mind if I change the title to "[AVR][clang] Pass --start-group and --end-group options to avr-ld" ?

mhjacobson added a comment.Jul 29 2021, 9:50 AM

Thanks, Ben. Yes, would you mind to land the patch for me? Name and e-mail are

Matt Jacobson <mhjacobson@me.com>

And yes, please do change the title as appropriate. I wasn't sure what the proper naming convention was.

Thanks again!

Closed by commit rG1e6a93f15c7e: [AVR][clang] Pass '--start-group' and '--end-group' options to avr-ld (authored by mhjacobson, committed by benshi001). · Explain WhyJul 29 2021, 5:26 PM
This revision was automatically updated to reflect the committed changes.
benshi001 added a commit: rG1e6a93f15c7e: [AVR][clang] Pass '--start-group' and '--end-group' options to avr-ld.

Revision Contents

PathSize
clang/
lib/
Driver/
ToolChains/
4 lines

Diff 361931

clang/lib/Driver/ToolChains/AVR.cpp

Context not available.
if (LinkStdlib) { if (LinkStdlib) {
assert(!CPU.empty() && "CPU name must be known in order to link stdlibs"); assert(!CPU.empty() && "CPU name must be known in order to link stdlibs");
CmdArgs.push_back("--start-group");
// Add the object file for the CRT. // Add the object file for the CRT.
std::string CrtFileName = std::string("-l:crt") + CPU + std::string(".o"); std::string CrtFileName = std::string("-l:crt") + CPU + std::string(".o");
CmdArgs.push_back(Args.MakeArgString(CrtFileName)); CmdArgs.push_back(Args.MakeArgString(CrtFileName));
Context not available.
// Add the link library specific to the MCU. // Add the link library specific to the MCU.
CmdArgs.push_back(Args.MakeArgString(std::string("-l") + CPU)); CmdArgs.push_back(Args.MakeArgString(std::string("-l") + CPU));
CmdArgs.push_back("--end-group");
benshi001Unsubmitted
Done
ReplyInline Actions

You are appreciated to use 'git show -U999999' or 'git diff -U999999' for better context display.

benshi001: You are appreciated to use 'git show -U999999' or 'git diff -U999999' for better context…
mhjacobsonAuthorUnsubmitted
Done
ReplyInline Actions

Sorry! Fixed, I think.

mhjacobson: Sorry! Fixed, I think.
// Specify the family name as the emulation mode to use. // Specify the family name as the emulation mode to use.
// This is almost always required because otherwise avr-ld // This is almost always required because otherwise avr-ld
// will assume 'avr2' and warn about the program being larger // will assume 'avr2' and warn about the program being larger
Context not available.