esp-rs / espflash

Serial flasher utility for Espressif SoCs and modules based on esptool.py
Apache License 2.0
478 stars 117 forks source link

`merge_adjacent_segements` fails if there is a gap between `.text` and `.text_init` segments #522

Closed t-moe closed 9 months ago

t-moe commented 10 months ago

See https://github.com/rust-lang/rust/issues/118214 .

I'm trying to figure out, if rustc is allowed to do this (insert a gap at will), and we should handle it, or if it is a rustc bug.

Workaround

Works when applying the following patch: https://github.com/esp-rs/espflash/compare/main...t-moe:espflash:main

t-moe commented 10 months ago

Upon further reflection and after the discussions in the linked rustc issue, I believe we need to address this on our end. Specifically, I propose reading the alignment for each section directly from the ELF file and respecting this alignment in merge_adjacent_segments.

I've noticed we have several other pieces of code related to IDF/ESP alignment, and I'm hesitant to modify them without a deeper understanding. Could someone more familiar with this part of the codebase review it? @jessebraham

Thank you.

(https://github.com/rust-lang/rust/issues/118214#issuecomment-1827351764) (One can easily produce a problematic binary where the text section is 16-byte aligned by invoking semihosting::process::abort.)

jessebraham commented 10 months ago

I have some other things on my plate right now, but will try to take a look at some point in the next couple days. Thanks for digging into this problem and providing this information!

Vollbrecht commented 10 months ago

i could be wrong here, but to my understanding the alignment for riscv needs to be absolutely fixed here. riscv with the I extension for compressed instructions is always 16bit aligned. Compared to xtensa where we have flexible lenght here. So if you read through some text data you would parse it accordingly to the riscv spec here. The in memory representation needs to be that way per spec

t-moe commented 10 months ago

@Vollbrecht The issue I'm having is that sometimes the .text segment is 16 byte(!!) aligned. When data is 16 byte aligned, it is also 16bit aligned :). Individual functions (like semihosting::process::abort) request a such large alignment and rustc or llvm will then align the entire text segment on a 16-byte boundary. We need to handle that case accordingly, and merge adjacent flash segments, even if they have some alignment padding between them.

Vollbrecht commented 10 months ago

reading comprehension failed :D yeah i only read byte as bit's everywhere, thanks for clearing that up.

jessebraham commented 10 months ago

Sorry, work did not go as planned at all last week and I'm still quite busy with other tasks. If somebody else is able to investigate this that'd be very much appreciated, otherwise I probably can't get to it until the new year unfortunately.

bjoernQ commented 9 months ago

when fixing it we need to keep this in mind: https://github.com/esp-rs/esp-hal/blob/260c882b088c416c457e0b3337d849338a0a56e2/esp-hal-common/ld/esp32c6/esp32c6.x#L72-L82

Not sure if newer bootloaders still need it this way

bjoernQ commented 9 months ago

This is a minimal repro of the issue:

#![no_std]
#![no_main]

use esp_backtrace as _;
use esp32c3_hal::prelude::entry;

#[entry]
fn main() -> ! {
    unsafe {
        foo();
    }
    loop {}
}

#[inline]
pub(crate) unsafe fn foo() {
    unsafe {
        core::arch::asm!(
            ".balign 16",
            ".option push",
            ".option norvc",
            "nop",
            ".option pop",
        );
    }
}

We could also solve this by just moving

    KEEP(*(.init));
    KEEP(*(.init.rust));
    KEEP(*(.text.abort));

into .text (needs some adjustments where .text_init is mentioned) - or are there any downsides doing this?

MabezDev commented 9 months ago

or are there any downsides doing this

We did do it for a reason, but I can't find the reason nor remember it :D. Let's try it and see what happens.

bjoernQ commented 9 months ago

https://github.com/esp-rs/esp-hal/pull/1038 should solve the problem (in a different way) - feel free to re-open if there are still issues

t-moe commented 9 months ago

Is it possible that we have the same problem, when using esp_idf_hal ? @bjoernQ

See the gap here, in a esp_idf_hal project that uses alignment of the rom section because of semihosting...

# riscv32-esp-elf-objdump -h  target/riscv32imac-esp-espidf/debug/esp32_devkit

target/riscv32imac-esp-espidf/debug/esp32_devkit:     file format elf32-littleriscv

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  .....
  5 .iram0.text   00009f66  40800000  40800000  00001000  2**8
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  6 .iram0.text_end 00000000  40809f66  40809f66  0007f7e0  2**0
                  CONTENTS

Not sure if this is a problem or not, but I have a binary here, that does not boot up as expected...

MabezDev commented 9 months ago

I don't think it is the same issue, because even though they have different section alignments, the two sections are contiguous where as before there was a gap which caused the error. I suggest filling a separate issue.

t-moe commented 9 months ago

Thank you @MabezDev. You're indeed right, there is no gap here. case closed.

(I've locally added some diagnostics to espflash that warn me of gaps, but it seems the diagnostic is wrong. And i posted the output without checking.... sorry :face_with_head_bandage: )