riscv-rt: organize trap section

[romancardenas]

This PR adds a better organization of the .trap section:

1. _trap_vector (if v-trap is enabled).
2. _start_trap (defaults to _default_start_trap).
3. _start_INTERRUPT_trap routines (if v-trap is enabled).
4. _start_DefaultHandler_trap and _continue_trap (if v-trap is enabled).
5. _start_trap_rust.
6. Other code in .trap section (usually, none)

This ensures that symbols in the binary are sorted as the expected execution flow both in trap mode and in direct mode.

[romancardenas]

@gibbz00 @MabezDev maybe a quick review from the ESP team would be very handy!

The next PR I plan to open is going back to _start_rust, as you do in esp-rs

[gibbz00]

Looks nice! I lack the experience to comment too much on the alignment changes. Trust that you're right there, it's just that they stood out a bit for me 😅

[MabezDev]

Should these symbols really be 2byte aligned? What pushed this change? I don't suppose you meant .palign 2?

[romancardenas]

I was trying to remove some restrictions, but I'm not 100% sure about the required alignment... In fact, I suspect no special alignment is required.

I got back to an alignment of 4 for _start_DefaultHandler_trap, because it is part of the vector table and might be somewhat delicate, but removed the alignment requirements for _continue_trap, as I think it is not necessary. Note that we reach that function from the rest of _start_INTERRUPT_trap routines with a regular j instruction, so I don't think we need a specific alignment.

In any case, I am not 100% sure of what I said, so any expert knowledge is more than welcome :)

[romancardenas]

Just for homogeneity, I also modified the alignment of _start_INTERRUPT_trap routines in riscv-rt-macros to 4, matching _start_DefaultHandler_trap. If we finally decide to drop alignment constraints, we must remove them from all the symbols

[gibbz00]

I've done some digging now that should hopefully clear somethings up.

.align 2 means, apparently, 4-byte alignment. n is given in powers of two.

https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_7.html#SEC70

Some assemblers alias .align with .palign. Even so, it won't work in Rust's asm! macros, but .p2align will.

https://doc.rust-lang.org/reference/inline-assembly.html#directives-support

Mabez and I were probably confusing .align with .balign.

https://ftp.gnu.org/old-gnu/Manuals/gas-2.9.1/html_chapter/as_7.html#SEC74

In fact, I suspect no special alignment is required.

Both yes and no, think the devil lies in the details.

It's true that the spec does not mandate natural alignment requirements for
data access, leaving it up to processor implementations to decide if it is
allowed or not. But aren't we're talking about instruction alignment?

The RISC-V Instruction Set Manual Volume I Section 2.2:

The base ISA has IALIGN=32, meaning that instructions must be aligned on a
four-byte boundary in memory. An instruction-address-misaligned exception is
generated on a taken branch or unconditional jump if the target address is not
IALIGN-bit aligned.

As such, my take is that .align 2 (or .balign 4) should be for all sections.
Which means that I think you were right all along @romancardenas :)

[romancardenas]

I believe in LLVM .align refers to Byte alignment, not power of two (in contrast with GCC). So @mabez is right pointing out the .palign.

Regarding instruction alignment, is this alignment mandatory even in targets with the C ISA extension? Also, as this alignment is strongly required, I think LLVM already applies it. .align is necessary for those symbols with extra alignment requirements (e.g., trap vectors).

Again, this is what I inferred after a few years working on this, but I am not a reliable source. I will try to make sure of everything.

[gibbz00]

LLVM will in often try its best to do as GCC documents it. So too is the case here, even if different wording is used:

https://sourceware.org/binutils/docs/as/RISC_002dV_002dDirectives.html#index-align-directive-1

.align n # n = log_2(x) <=> x = 2^n

Regarding instruction alignment, is this alignment mandatory even in targets with the C ISA extension?

Yes, but down to 16 bits. From the spec:

IALIGN is 32 bits in the base ISA, but some ISA extensions, including the
compressed ISA extension, relax IALIGN to 16 bits. IALIGN may not take on any
value other than 16 or 32.
...
The alignment constraint for base ISA instructions is relaxed to a two-byte boundary
when instruction extensions with 16-bit lengths or other odd multiples of 16-bit
lengths are added (i.e., IALIGN=16).

Also, as this alignment is strongly required, I think LLVM already applies it.

Possibly. May also be the linker script doing the alignment?

Found this thread which suggests that even Rust functions can be misplaced unless special consideration is taken: https://users.rust-lang.org/t/make-sure-function-address-is-4byte-aligned/49549

[romancardenas]

Maybe we should use .balign to avoid misinterpretations.

[rmsyn]

Maybe we should use .balign to avoid misinterpretations.

Either way is fine to me. Maybe just a comment with a short explanation + link to docs if we keep it the same?

[romancardenas]

I'm now using .balign to avoid misinterpretations. Also, I added a few comments regarding when I think alignment directives are necessary or not. In any case, I'd rather remove them in a subsequent PR.

[gibbz00]

LGTM!

I agree with the move to .balign.

[romancardenas]

Soooo... now Rust 1.75 complains because "error[E0658]: lint reasons are experimental". I will force-push a new commit with reasons commented

[romancardenas]

That should do the trick