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

rimage: manifest: Improve handling of empty segments #8531

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

softwarecki
Copy link
Collaborator

Fixed output segment size calculation. If a given output segment type did not contain any input section (was empty), its size was incorrectly calculated as 1 and then rounded to the page size (4096). This resulted in an unnecessary segment in a output file.

Improved handling of empty RODATA segment. If the RODATA segment does not contain any data, its type is now marked as empty. Cleared the readonly flag of RODATA segment because this segment also contains .data sections.

If a given output segment type did not contain any input section
(was empty), its size was incorrectly calculated as 1 and then rounded to
the page size (4096). This resulted in an unnecessary segment in a output
file.

Signed-off-by: Adrian Warecki <[email protected]>
@lyakh
Copy link
Collaborator

lyakh commented Nov 28, 2023

Cleared the readonly flag of RODATA segment because this segment also contains .data sections.

We only have one section in the manifest, that contains both .data and .rodata, right? This is in fact not good, can this be fixed?

lyakh
lyakh previously requested changes Nov 28, 2023
If the RODATA segment does not contain any data, its type is marked as
empty. Cleared the readonly flag because this segment also contains .data
sections.

Signed-off-by: Adrian Warecki <[email protected]>
@softwarecki
Copy link
Collaborator Author

@lyakh

We only have one section in the manifest, that contains both .data and .rodata, right? This is in fact not good, can this be fixed?

I'm afraid there is no good solution for this. The manifest only supports max three segments. Currently we use them like this:

  1. .text
  2. .rodata .data
  3. .bss
    I think this is due to the assumption that loadable modules don't have a .data section, only .text, .rodata and .bss.

@softwarecki softwarecki requested a review from lyakh November 28, 2023 15:35
@lyakh
Copy link
Collaborator

lyakh commented Nov 29, 2023

@lyakh

We only have one section in the manifest, that contains both .data and .rodata, right? This is in fact not good, can this be fixed?

I'm afraid there is no good solution for this. The manifest only supports max three segments. Currently we use them like this:

1. `.text`

2. `.rodata` `.data`

3. .`bss`
   I think this is due to the assumption that loadable modules don't have a `.data` section, only `.text`, `.rodata` and `.bss`.

I don't really understand this assumption - modules are allowed to have .rodata, i.e. const objects, e.g. operations, and they're allowed to have .bss, i.e. writable initialised to 0 data, why aren't they then supposed to have writable but initialised to non-0 values data?

@lgirdwood
Copy link
Member

@lyakh

We only have one section in the manifest, that contains both .data and .rodata, right? This is in fact not good, can this be fixed?

I'm afraid there is no good solution for this. The manifest only supports max three segments. Currently we use them like this:

1. `.text`

2. `.rodata` `.data`

3. .`bss`
   I think this is due to the assumption that loadable modules don't have a `.data` section, only `.text`, `.rodata` and `.bss`.

I don't really understand this assumption - modules are allowed to have .rodata, i.e. const objects, e.g. operations, and they're allowed to have .bss, i.e. writable initialised to 0 data, why aren't they then supposed to have writable but initialised to non-0 values data?

I think we have been merging .rodata and .data in the same manifest section to date using the linker and rimage.

@keqiaozhang
Copy link
Collaborator

SOFCI TEST

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok. @lyakh please take a look at the clarifications.

@lyakh lyakh dismissed their stale review December 1, 2023 08:57

questions remain, but none of them are critical

@kv2019i kv2019i merged commit e36b77c into thesofproject:main Dec 4, 2023
@softwarecki softwarecki deleted the rimage-fix-rodata branch March 4, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rimage Issues related to rimage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants