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

chore: compile in release mode with debug info by default #250

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

edubart
Copy link
Contributor

@edubart edubart commented Jul 1, 2024

This PR switch the default compilation from release with asserts enabled for everything, to release with debug information with asserts disabled only for interpret.cpp. This makes sure people will compile an efficient emulator with just make when packaging by default, not requiring the use of make relwithdebinfo=yes or make release=yes anymore when packaging.

The default make is chosen to contain debug information because some distros may want to strip and move the debug information into a second debug package (for example ArchLinux does this).

Change were mades in CI to always test with debug information, except for tagged releases.

IMPORTANT: After this PR, if you want to compile with asserts for interpret.cpp, please compile with make debug=yes, this is something only emulator developers care.

PR main changes

  • Compile interpret.cpp with -DNDEBUG for release/relwithdebinfo modes
  • Compile with relwithdebinfo=yes by default, but use debug=yes for CI tests
  • Always compile sha3.c (third party source) with -O3 to speed up ci
  • Use -O1 optimizations on sanitize action to make CI faster
  • Add libcartesi.so symlinks in src, to make linkage on external programs dependent on in-development emulator straightforward

@edubart edubart added the enhancement New feature or request label Jul 1, 2024
@edubart edubart added this to the v0.18.0 milestone Jul 1, 2024
@edubart edubart requested review from diegonehab and a team July 1, 2024 19:31
@edubart edubart self-assigned this Jul 1, 2024
@edubart edubart force-pushed the feature/default-relwithdebinfo branch 4 times, most recently from 5740018 to 729c8b6 Compare July 2, 2024 13:51
@edubart edubart linked an issue Jul 2, 2024 that may be closed by this pull request
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from 729c8b6 to 369e244 Compare July 2, 2024 15:07
@edubart edubart changed the base branch from main to fix/tlb-uarch-root-hash July 2, 2024 15:07
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from 369e244 to 06fb18a Compare July 5, 2024 21:12
@edubart edubart modified the milestones: v0.18.0, v0.19.0 Jul 23, 2024
Base automatically changed from fix/tlb-uarch-root-hash to main July 28, 2024 17:15
@edubart edubart force-pushed the feature/default-relwithdebinfo branch 2 times, most recently from f578360 to 6a175f3 Compare July 30, 2024 11:28
@edubart edubart changed the base branch from main to increase-word-log2-size July 30, 2024 11:32
@edubart edubart requested review from vfusco and removed request for diegonehab July 30, 2024 11:32
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from 6a175f3 to 8315bce Compare July 30, 2024 11:36
Makefile Show resolved Hide resolved
@edubart edubart requested a review from mpolitzer July 30, 2024 14:01
@vfusco vfusco modified the milestones: v0.19.0, v0.18.0 Jul 30, 2024
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from 8315bce to d67773d Compare July 30, 2024 14:39
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from d67773d to 988d4fd Compare July 30, 2024 15:19
@mpolitzer
Copy link
Contributor

Tested the different builds and the flags look good.
A suggesting is to add those options to the make help target. Something like:

help:
	@echo 'Main targets:'
	@echo '* all - Build the src/ and uarch/ code. To build from a clean clone, run: make submodules all'
	@echo '        Additionally, to specify the build type, pass yes to one of:'
	@echo '        - sanitize, debug, relwithdebinfo, *release*, coverage. e.g: make sanitize=yes

Another point, a bit unrelated to this PR is removing the Emscripten documentation from the README.md. A brief comment pointing to the wiki should suffice. Should we do the same for the mingw toolchain?

mpolitzer
mpolitzer previously approved these changes Jul 31, 2024
@edubart
Copy link
Contributor Author

edubart commented Aug 1, 2024

Another point, a bit unrelated to this PR is removing the Emscripten documentation from the README.md. A brief comment pointing to the wiki should suffice. Should we do the same for the mingw toolchain?

I created issue #261 for me to improve README/help later (to not delay release).

@mpernambuco mpernambuco force-pushed the increase-word-log2-size branch 5 times, most recently from 70d0530 to e6bf24e Compare August 6, 2024 15:08
@edubart edubart force-pushed the feature/default-relwithdebinfo branch from 988d4fd to 0dcb572 Compare August 6, 2024 15:50
Base automatically changed from increase-word-log2-size to main August 6, 2024 17:29
@mpernambuco mpernambuco dismissed mpolitzer’s stale review August 6, 2024 17:29

The base branch was changed.

@edubart edubart requested review from mpolitzer and a team August 6, 2024 20:18
release?=no
sanitize?=no
coverage?=no

# If not build type is chosen, set the default to release with debug information,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# If not build type is chosen, set the default to release with debug information,
# If no build type is chosen, set the default to release with debug information,

@vfusco vfusco merged commit 0dcb572 into main Aug 8, 2024
8 checks passed
@vfusco vfusco deleted the feature/default-relwithdebinfo branch August 8, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Sanitize is taking too long in CI
4 participants