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

fix(mem): Fix elf loading byte counting #505

Merged

Conversation

zarubaf
Copy link
Contributor

@zarubaf zarubaf commented Nov 13, 2024

I've added a small bug when counting the total bytes written during elf loading. The problem is that we are unpacking the elf and we need to calculate the total size of the resulting memory image, not the "compressed" bytes written.

Copy link
Member

@poemonsense poemonsense left a comment

Choose a reason for hiding this comment

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

Look good to me except for the unused len_written variable.

After reviewing the code, I noticed there do exist ambiguous meanings for the return value. For the usage in clone (ram.h), the return value (as img_size) should be the size of max non-zero bytes in the linear address space. I assume the version before this PR is incorrect because the REF will be synced with some data lost.

However, for the usage in display_stats, it seems this value is used as the size of meaningful (non-zero) inputs. However, since the stats does not really bring functional issues, I'll fix it in the future and not in this PR.

@@ -120,5 +120,8 @@ long readFromElf(void *ptr, const char *file_name, long buf_size) {
std::memcpy((uint8_t *)ptr + offset, section.data_src, section.data_len);
len_written += len;
}
return len_written;
Copy link
Member

Choose a reason for hiding this comment

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

Is this len_written still used?

@poemonsense
Copy link
Member

Do you have any idea on adding some ELF loading tests?

Much thanks again for your contribution.

Copy link
Member

@poemonsense poemonsense left a comment

Choose a reason for hiding this comment

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

For quick bug fixes, I'm merging this now. We will see how to add elf tests in the next steps.

@poemonsense poemonsense merged commit df4f7df into OpenXiangShan:master Nov 15, 2024
5 checks passed
@poemonsense
Copy link
Member

Much thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants