Happy new year! If this change is acceptable, could someone please merge it for me? (And if not, could someone point out what needs more work?) Thanks for your help!

Well, I'm hoping it doesn't prove to be too much work. 🙂 With this fix and D112049, most things are working quite well, though I'm sure I'll uncover more interesting things as I go.

Ping. Is there anyone more suited to reviewing this patch?

The test is failing for a different reason (that I'd forgotten I worked around locally). I'll look into fixing that first. Sorry for noise.

In D95664#2660051, @arsenm wrote:

I just realized I've been reading "frame pointer elimination" as "frame index elimination" but this change still doesn't make sense to me. Why do you want to treat these stores as frame instructions when they don't modify the stack pointer?

Do we happen to know what change(s) made this pass work correctly for AVR?

In D107684#2936033, @MaskRay wrote:

--sysroot %S/Inputs/basic_avr_tree/usr/lib seems incorrect.

sysroot is usually a directory which contains usr/lib and lib/. somewhere/usr/lib is not usually used as a sysroot.

Rebase atop recent changes; use separate check lines and string substitution for SYSROOT.

In D107682#2932685, @benshi001 wrote:

In D107682#2932607, @mhjacobson wrote:

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

Thanks for your help. I have updated my patch. Now it should work for both your platform and mine. It search GCCRoot.getParentLibPath()/avr first, otherwise GCCRoot.getParentLibPath()/../avr.

Please help me check.

In D107684#2932609, @benshi001 wrote:

In D107684#2932605, @mhjacobson wrote:

In D107684#2932603, @benshi001 wrote:

We need not add another basic_avr_tree_opt_local, since you added /avr to possible avr-libc pathes, you can test your change by specifying --sysroot %S/Inputs/basic_avr_tree/usr/lib/ .

Oh, good idea.

Sorry, only spefifying --sysroot %S/Inputs/basic_avr_tree/usr/lib/ does not work, since the search for avr-ld also relies on --sysroot.

Simplify.

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

In D107684#2932603, @benshi001 wrote:

We need not add another basic_avr_tree_opt_local, since you added /avr to possible avr-libc pathes, you can test your change by specifying --sysroot %S/Inputs/basic_avr_tree/usr/lib/ .

In D107682#2932599, @benshi001 wrote:

@mhjacobson Could you please check if my modification also works as expected on your platform ?

In D107672#2932423, @MaskRay wrote:

@benshi001 This was prematurely committed. It has no test and has apparent unaddressed issue. (I revamped many parts in the Linux/Gnu search paths so am quite confident about my observation.)

In D107672#2932413, @mhjacobson wrote:

At first glance, I think you're right: avr-gcc does expect avr-libc to be relative to itself. That seems slightly weird to me (since avr-libc is technically separate from GCC), but OK.

In D107672#2932406, @MaskRay wrote:

I know nearly nothing about AVR, but from avr-gcc a.c '-###' I doubt this is the GCC behavior. GCC's library path is relative to the GCC installation. /usr/lib/gcc/avr/5.4.0/../../../avr/lib

There is a difference between sysroot inferred path and GCC installation inferred path. https://maskray.me/blog/2021-03-28-compiler-driver-and-cross-compilation

In D107672#2932190, @mhjacobson wrote:

I'm also tempted to remove /usr/lib/avr, because these paths make no sense to me: $SYSROOT/usr/lib/avr/lib and $SYSROOT/usr/lib/avr/include.

I'm also tempted to remove /usr/lib/avr, because these paths make no sense to me: $SYSROOT/usr/lib/avr/lib and $SYSROOT/usr/lib/avr/include.

Thanks!

Thanks for your help, @benshi001 and @MaskRay!

Appeased the linter.

Move emission of __do_copy_data and __do_clear_bss into AVRAsmPrinter. (Leave AVRTargetStreamer as base class of AVRELFStreamer.)

@MaskRay, do you have an opinion on whether I should move the other undefined-symbols definitions into AVRAsmPrinter.cpp to match?

Removed incorrect use of setExternal().

In D107133#2924267, @MaskRay wrote:

Defining __do_global_ctors looks like ld's work (in GNU ld), not the assembler's.

Appeasing linter.

In D107133#2919033, @mhjacobson wrote:

Just brainstorming. Another option might be to move the code in AVRTargetStreamer::finish() into a new override AVRAsmPrinter::doFinalization(). That would make it so that both bits of .globl code are in AVRAsmPrinter.

Just brainstorming. Another option might be to move the code in AVRTargetStreamer::finish() into a new override AVRAsmPrinter::doFinalization(). That would make it so that both bits of .globl code are in AVRAsmPrinter.

Sure, I'm open to that.

Conform to source formatting convention.

You're right, AVR does have a section (.ctors) for the *table* of constructors. But there has to be some code that actually calls the functions in that table. That's what __do_global_ctors does. When present, it gets linked into the boot path (after things like __do_copy_data and __do_clear_bss) to do its work. But if nothing requests it, it won't be linked, and the constructor table will just sit around, never to be consulted.

Add assembly comment.

Fix the ordering of the directives in the test, so that it actually passes! :)

Argh, the test fails now, because the ordering is wrong. Fixing it now.

Fixed alphabetization of includes.

Updated test with update_llc_test_checks.py. Note that update_llc_test_checks.py doesn't actually support AVR at the moment! I had to apply this fix first:

Added a test.

I roughly followed the example of how __do_copy_data and __do_clear_bss are linked, in AVRTargetStreamer::finish().

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

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

Updated the diff to add more context.

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