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

Avoid to wrongly allocate memory in map_file() #531

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

RinHizakura
Copy link
Collaborator

@RinHizakura RinHizakura commented Dec 28, 2024

In the case that HAVE_MMAP is 0, the emulator crashes.

$ make ENABLE_SYSTEM=1 system
rv32emu: src/emulate.c:626: block_translate: Assertion `insn' failed.
Aborted

The map_file() logic when HAVE_MMAP == 0 is weird as it allocates extra resources on the emulated memory. After this change, the aborting issue is gone, so I believe the original is not the correct implementation.

@RinHizakura
Copy link
Collaborator Author

RinHizakura commented Dec 28, 2024

BTW, I guess that we don't have coverage for HAVE_MMAP = 0. Do we need to have it?

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Benchmarks

Benchmark suite Current: f554a00 Previous: cb5eb10 Ratio
Dhrystone 1287 Average DMIPS over 10 runs 1328 Average DMIPS over 10 runs 1.03
Coremark 962.761 Average iterations/sec over 10 runs 964.938 Average iterations/sec over 10 runs 1.00

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Collaborator

@vacantron vacantron left a comment

Choose a reason for hiding this comment

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

Please refine the commit message that might contain "avoid reallocating memory" (or something like that) to indicate your change.

@jserv
Copy link
Contributor

jserv commented Dec 28, 2024

BTW, I guess that we don't have coverage for HAVE_MMAP = 0. Do we need to have it?

The condition HAVE_MMAP=0 is useful when profiling, as it is not straightforward to measure the system memory consumption when mmap is heavily used during emulation.

@ChinYikMing
Copy link
Collaborator

@RinHizakura Thanks for the fix. It indeed an implementation bug.

@RinHizakura
Copy link
Collaborator Author

The condition HAVE_MMAP=0 is useful when profiling, as it is not straightforward to measure the system memory consumption when mmap is heavily used during emulation.

Sorry for the unclear question, I was asking about the CI/CD coverage in fact.

@RinHizakura RinHizakura changed the title Fix crash on system without mmap() Avoid to wrongly allocate memory in map_file() Dec 29, 2024
@jserv
Copy link
Contributor

jserv commented Dec 30, 2024

I was asking about the CI/CD coverage in fact.

You can simply skip the condition HAVE_MMAP=0 in CI/CD pipeline.

@jserv
Copy link
Contributor

jserv commented Dec 30, 2024

Breakage reported by CI:

src/jit.c:1760:17: warning: ‘do_sret’ defined but not used [-Wunused-function]
 1760 |     static void do_##inst(struct jit_state *state UNUSED, riscv_t *rv UNUSED, \
      |                 ^~~
src/rv32_jit.c:355:1: note: in expansion of macro ‘GEN’
  355 | GEN(sret, {assert(NULL);
      | ^~~

In the case that HAVE_MMAP=0, we allocates extra resources on the
emulated memory. This is not correct and will abort the system
emulation. Fix the issue to make system emulation works correctly.
@RinHizakura
Copy link
Collaborator Author

RinHizakura commented Dec 30, 2024

Breakage reported by CI:

This is not related to this change. The same warning can be found in other CI/CD jobs https://github.com/sysprog21/rv32emu/actions/runs/12540405349/job/34967656388 even if it passes. Not sure if any issues on the CI/CD justify this as a fail.

@jserv jserv merged commit e89e613 into sysprog21:master Dec 30, 2024
8 checks passed
@jserv
Copy link
Contributor

jserv commented Dec 30, 2024

Thank @RinHizakura for contributing!

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.

4 participants