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

Introduce Valgrind to the CI pipeline #369

Closed
wants to merge 1 commit into from

Conversation

henrybear327
Copy link
Collaborator

@henrybear327 henrybear327 commented Mar 1, 2024

The static analyzer alone can't catch all the runtime issues. In this commit, the dynamic analysis tool Valgrind is added to the CI pipeline.

ENABLE_SDL is set to 0, in order to reduce noise caused by the external
libraries.

Reference:

@henrybear327
Copy link
Collaborator Author

henrybear327 commented Mar 1, 2024

To merge this pull request, all the memory-related leaks need to be addressed first.

Currently, based on my initial testing, when disabling SDL and JIT, no memory leaks are found. When enabling JIT, there will be plenty of leaks.

@jserv jserv requested a review from visitorckw March 1, 2024 20:51
@henrybear327 henrybear327 force-pushed the ci/valgrind branch 2 times, most recently from 7069c80 to 485d987 Compare March 1, 2024 20:54
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@jserv
Copy link
Contributor

jserv commented Jun 3, 2024

Memory leaks of JIT compiler reported by Valgrind:

  • definitely lost: 144 bytes in 4 blocks
  • indirectly lost: 132 bytes in 9 blocks
  • still reachable: 64 bytes in 3 blocks

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: 53dabb2 Previous: b80dcb4 Ratio
Dhrystone 3.44 Average DMIPS over 10 runs 12.37 Average DMIPS over 10 runs 3.60
Coremark 0.003 Average iterations/sec over 10 runs 0.005 Average iterations/sec over 10 runs 1.67

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

@henrybear327 henrybear327 force-pushed the ci/valgrind branch 8 times, most recently from b80dcb4 to 53dabb2 Compare June 17, 2024 05:22
@henrybear327
Copy link
Collaborator Author

Memory leaks of JIT compiler reported by Valgrind:

  • definitely lost: 144 bytes in 4 blocks
  • indirectly lost: 132 bytes in 9 blocks
  • still reachable: 64 bytes in 3 blocks

The initial investigation is that the memory leak is detected only when JIT is enabled.

Currently, there are 2 identified leaks so far

  • ir->fuse: it allocates memory outside the memory pool. When JIT is enabled, the clean-up logic is missing.
  • releasing allocated memory after the memory arena expansion

@jserv
Copy link
Contributor

jserv commented Jun 17, 2024

The initial investigation is that the memory leak is detected only when JIT is enabled.
Currently, there are 2 identified leaks so far

  • ir->fuse: it allocates memory outside the memory pool. When JIT is enabled, the clean-up logic is missing.
  • releasing allocated memory after the memory arena expansion

@qwe661234, can you comment this?

@henrybear327 henrybear327 marked this pull request as draft June 17, 2024 07:46
@henrybear327
Copy link
Collaborator Author

The initial investigation is that the memory leak is detected only when JIT is enabled.
Currently, there are 2 identified leaks so far

  • ir->fuse: it allocates memory outside the memory pool. When JIT is enabled, the clean-up logic is missing.
  • releasing allocated memory after the memory arena expansion

@qwe661234, can you comment this?

Reference:

free(ir->fuse);

@qwe661234
Copy link
Collaborator

The initial investigation is that the memory leak is detected only when JIT is enabled.
Currently, there are 2 identified leaks so far

  • ir->fuse: it allocates memory outside the memory pool. When JIT is enabled, the clean-up logic is missing.
  • releasing allocated memory after the memory arena expansion

@qwe661234, can you comment this?

Reference:

free(ir->fuse);

This function is only using in non-JIT mode, the basic block information is stored in block cache in JIT mode. Therefore, I need to add a free for ir->fuse when clear block cache.

The static analyzer alone can't catch all the runtime issues. In this
commit, the dynamic analysis tool Valgrind is added to the CI pipeline.

ENABLE_SDL is set to 0, in order to reduce noise caused by the external
libraries.

Reference:
- https://valgrind.org/docs/manual/quick-start.html
- https://linux.die.net/man/1/gcc
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.

Rebase the latest master branch.

@jserv
Copy link
Contributor

jserv commented Oct 31, 2024

The build system is being reworked (#473), and I am about to close this pull request. Let's introduce Valgrind to CI pipeline later.

@jserv jserv closed this Oct 31, 2024
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