`riscv-rt`: Cannot be built with `lto = true` if the user defines replacement for `.weak` symbols

[rslawson]

With the current design choice for some functions to be written in Assembly and be marked as .weak, this can remove the ability for downstream users to build with lto = true. I initially filed a bug with rustc, however, bjorn3 there suggested that this crate could replace .weak with #[linkage = "weak"] behind some sort of nightly feature. I have a locally patched version, however it at minimum would require #![feature(linkage)] and possibly even #![feature(linkage, naked_functions)]. I don't know where this project stands on nightly usage, but I think this could fairly easily be hidden behind a feature flag.

[MabezDev]

Maybe you can try this: https://github.com/rust-embedded/riscv/pull/200#discussion_r1601316656 and see if it resolves it. It should discard asm syms in LTO consideration.

[rslawson]

Ah, sorry, I completely forgot to link the issue I filed in the original post. It's #133974. I already have a working solution using #[feature(linkage, naked_functions)], but I'll see if I can adjust it to use what you shared instead.

[romancardenas]

I prefer @MabezDev 's suggestion, as it would fit stable Rust too. In any case, while these crates are mainly focused on stable Rust, it should be harmless to provide some nightly functionalities under feature gates.

[rslawson]

I've been trying that for a couple hours today and I'm either not clever enough to get it to work or it just doesn't work. If I put the .lto_discard directives later in the file, then I get duplicate definition errors again. If I put it too early in the file, I get absolutely blasted with symbol not found: ... errors. If I tag each definition individually with them, I get the same. I dug through the esp-hal crate and it seems to use the same method that was used initially in this crate - provide a DefaultExceptionHandler of some sort, and then link with PROVIDE(ExceptionHandler = DefaultExceptionHandler). The only uses of .weak I can see are on _start_trap*, _vector_table, and interrupt*.

[romancardenas]

Let me think a bit on this issue. Maybe we can try to reduce the number of weak symbols by making the build script more sophisticated.

[rslawson]

I did find the PR that introduced weak symbols was marked as closing #155. Is there a mechanism for pruning the default definitions if they're not overridden by the end user? I thought maybe --gc-sections would do that, but perhaps not?

[romancardenas]

Looks like Rust already does that (link), but it didn't work?

Maybe we can make abort a global symbol and rearrange the .trap section to fit abort there. Then, use PROVIDE(DefaultHandler = abort) and PROVIDE(DefaultExceptionHandler = abort).

If we want to keep abort weak, then we can do the same trick with a global _abort symbol and PROVIDE(DefaultHandler = _abort).

What do you think? Am I missing something important?

[romancardenas]

In fact, abort was initially a global symbol (here)

[romancardenas]

The cause of the change in is #197 . So... perhaps the _abort thing is the way to go?

[rslawson]

I think that sounds about right to me. Just to clarify my understanding of this so I can work on it - do you mean to remove the busy loop-defined default functions from the Rust code and instead replace them with a PROVIDE(DefaultThing = _abort) (along with renaming abort to _abort)?

[romancardenas]

So, abort is renamed to _abort and is a global symbol. Then, in the linker file, we can add something like:

PROVIDE(abort = _abort)
PROVIDE(DefaultExceptionHandler = abort)
PROVIDE(DefaultHandler = abort)

I would like @MabezDev and @jessebraham to share their opinions about this move. They work a lot in the esp-rs ecosystem, which shares a lot of aspects with riscv. Maybe they have some concerns with this change.

[rslawson]

I think that could be a workable solution. Frankly I'm entirely unsure of how .weak and .lto_discard interact, so maybe there's a solution to be found in that direction. I just couldn't find much with my limited knowledge of what search terms to use for that sort of thing.

[romancardenas]

OK, you can open a PR with these changes and we move the discussion there

[rslawson]

Sorry for the long delay - I've been on holiday with very limited capability to work on this. I should be able to open up a PR sometime this upcoming week.

[rslawson]

Got some time to try and start on this today and I quickly discovered I have absolutely no idea how to linker script. I had assumed that just defining _abort in global_asm! and then doing EXTERN(_abort) would be enough to get started, but immediately found I was wrong. If I could get some guidance in this direction it would be much appreciated, since this is one part of the embedded world that I've never touched previously and I don't know where to start.

[romancardenas]

Let us assume that you rename the abort symbol in the asm.rs file:

#[rustfmt::skip]
global_asm!(
    ".section .text.abort
.global _abort
_abort: 
    j _abort"
);

riscv-rt still expects an abort symbol, which, by default, should map to _abort. To achieve this, we can do the following in link.x.in

EXTERN(_abort); /* _abort is defined globally in asm.rs */
PROVIDE(abort = _abort); /* If no abort symbol is provided, then abort maps to _abort */

We can now use abort as a "weak" symbol for DefaultExceptionHandler and DefaultHandler:

PROVIDE(DefaultExceptionHandler = abort); /* If no DefaultExceptionHandler symbol is provided, then map to abort */
PROVIDE(DefaultHandler = abort);  /* If no DefaultHandler symbol is provided, then map to abort */

I think that should do the trick.

[rslawson]

Nice, okay, so I think then I was most of the way there except for missing the .global _abort.

[romancardenas]

Weak symbols currently used

• __pre_init: code to be run before RAM initialization. The default implementation does nothing. It can be overridden using the pre_init macro. However, this macro must be deprecated, as running Rust code at this point may lead to undefined behavior. The only "legal" way must be with inline assembly.
• _mp_hook: only in MP. It should return true only in one HART so it is in charge of initializing RAM. The others should wait for an event before continuing. Default implementation returns true for HART ID 0, whereas the others never return. This default implementation does not allow MP systems, and PACs (presumably) must override it.
• _setup_interrupts: sets the trap to jump to either _start_trap or loads the mtvec for vectored mode.
• ExceptionHandler, DefaultHandler, and _pre_init_trap: default implementations are infinite loops. They could be weakly assigned to abort by the linker.
• abort: default implementation is an infinite loop. As done in #254, we can define a global _abort symbol and let the linker to weakly assign abort to _abort

If we leave the _abort symbol as global, abort, _pre_init_trap , DefaultHandler, and ExceptionHandler can be handled in the linker script. However, the other weak symbols are not that easily replaceable.

[romancardenas]

Using feature gates

While this approach is not desirable, we kind of already have that with _dispatch_core_interrupt and _dispatch_exception via the no-interrupts and no-exceptions features. However, these functions are implemented on PACs, and PACs can activate the features when required without users getting noticed.

This is the easy way. Of course using weak symbols is more elegant, but maybe also harder to debug problems?

[MabezDev]

Sorry I didn't have much time over the holidays to look at this. @rslawson do you have a min repro for the original issue? I'd like to see if I can get it working without removing all the .weak symbols. .weak is a tool we should be able to use, and we (Espressif) are using it in esp-riscv-rt. We want to switch back to riscv-rt pretty soon, so we need to iron this out before we do that.

[rslawson]

@MabezDev I do have a min repro for the issue, yes, it's in some code blocks in the issue I opened up on rustc about this: #133974.

[ia0]

Maybe you can try this: #200 (comment) and see if it resolves it. It should discard asm syms in LTO consideration.

I tried it and it fixes the issue, see #273. Not sure if that's the fix we want, but at least it's a quick and easy fix.

[rslawson]

Huh, I tried almost exactly that and it didn't fix the issue. Wonder what I did differently.

[ia0]

Huh, I tried almost exactly that and it didn't fix the issue. Wonder what I did differently.

Turns out it doesn't always work, so it's not a very reliable solution. So #254 is probably the best bet so far.