Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
t-moe opened this issue Nov 23, 2023 · 13 comments
Labels
bug Something isn't working

Comments

@t-moe
Copy link

t-moe commented Nov 23, 2023

See rust-lang/rust#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:

main...t-moe:espflash:main

@t-moe t-moe changed the title merge_adjacent_segements fails if there is a gap between .text and .text_init segments merge_adjacent_segements fails if there is a gap between .text and .text_init segments Nov 23, 2023
@t-moe
Copy link
Author

t-moe commented Nov 27, 2023

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.

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

@jessebraham
Copy link
Member

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
Copy link

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
Copy link
Author

t-moe commented Nov 27, 2023

@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
Copy link

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

@jessebraham
Copy link
Member

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.

@jessebraham jessebraham added the bug Something isn't working label Dec 6, 2023
@jessebraham jessebraham pinned this issue Dec 15, 2023
@bjoernQ
Copy link
Contributor

bjoernQ commented Dec 18, 2023

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
Copy link
Contributor

bjoernQ commented Dec 18, 2023

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
Copy link
Member

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
Copy link
Contributor

bjoernQ commented Dec 18, 2023

esp-rs/esp-hal#1038 should solve the problem (in a different way) - feel free to re-open if there are still issues

@bjoernQ bjoernQ closed this as completed Dec 18, 2023
@t-moe
Copy link
Author

t-moe commented Jan 4, 2024

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
Copy link
Member

MabezDev commented Jan 4, 2024

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
Copy link
Author

t-moe commented Jan 4, 2024

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 🤕 )

@jessebraham jessebraham unpinned this issue Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants