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: #200 (comment) 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