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

Set CI buildtypes to highest opt level possible and set default buildtype to debugoptimized #260

Merged
merged 99 commits into from Mar 10, 2021

Conversation

kazarmy
Copy link
Member

@kazarmy kazarmy commented Dec 25, 2020

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the rizin book with the relevant information (if needed)

Detailed description

This pr:

  1. Sets the buildtype for CI builds to release (-O3) except the asan and Codecov builds. This is done because compiler optimizers increase performance but can also change code behavior (especially if the code invokes UB) so this needs to be tested.
    1.1 The asan buildtype is set to debugoptimized (-O2 -g) for hopefully sane stack traces.
    1.2 The Codecov buildtype is set to debug because debugoptimized (as seen in Set CI buildtypes to highest opt level possible and set default buildtype to debugoptimized #260 (comment)) produces wonky Codecov reports.

  2. Set the default buildtype to debugoptimized due to reasons given in Set default meson buildtype to debugoptimized? #256 i.e. users might leave performance on the table otherwise.

Test plan

All meson builds are green, including Windows and asan builds.

Closing issues

Closes #256.

@kazarmy kazarmy marked this pull request as draft December 25, 2020 13:24
@codecov-io
Copy link

codecov-io commented Dec 25, 2020

Codecov Report

Merging #260 (06fba52) into dev (b37d6fb) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #260      +/-   ##
==========================================
- Coverage   42.80%   42.79%   -0.02%     
==========================================
  Files         871      871              
  Lines      316886   316550     -336     
==========================================
- Hits       135653   135457     -196     
+ Misses     181233   181093     -140     
Impacted Files Coverage Δ
librz/asm/p/asm_dcpu16.c 0.00% <ø> (ø)
librz/asm/arch/hexagon/hexagon_disas.c 0.11% <100.00%> (ø)
librz/bin/format/wasm/wasm.c 40.28% <100.00%> (ø)
shlr/gdb/src/gdbclient/responses.c 15.03% <0.00%> (-18.43%) ⬇️
librz/asm/p/asm_pyc.c 0.00% <0.00%> (-14.29%) ⬇️
librz/core/cplugin.c 65.38% <0.00%> (-13.57%) ⬇️
librz/bin/format/pyc/pyc_magic.c 0.00% <0.00%> (-12.83%) ⬇️
librz/bin/p/bin_pyc.c 0.00% <0.00%> (-7.32%) ⬇️
librz/reg/reg.c 77.77% <0.00%> (-5.26%) ⬇️
librz/util/unum.c 71.48% <0.00%> (-5.08%) ⬇️
... and 778 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b37d6fb...2a5d47a. Read the comment docs.

@@ -8,7 +8,13 @@

extern ut32 constant_extender;

int hexagon_disasm_instruction(ut32 hi_u32, HexInsn *hi, ut32 addr) {
#if ASAN
Copy link
Member

Choose a reason for hiding this comment

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

why this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -66,7 +66,7 @@ jobs:
compiler: gcc
enabled: ${{ github.event_name == 'pull_request' }}
timeout: 45
cflags: '-Werror -Wno-cpp'
Copy link
Member

Choose a reason for hiding this comment

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

did you remove -Werror just for test?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it should be reenabled at the end

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

I'm ok overall with the idea of moving to debugoptmized.

@kazarmy
Copy link
Member Author

kazarmy commented Dec 26, 2020

Not sure whether Codecov output is reliable under optimization though.

@kazarmy
Copy link
Member Author

kazarmy commented Dec 27, 2020

Not sure whether Codecov output is reliable under optimization though.

Unfortunately, it appears that lines can disappear alarmingly from the Codecov report under -O2 optimization. Compare e.g. a report for librz/parse/p/parse_ppc_pseudo.c on -O0:

ppc_O0

and on -O2:

ppc_O2

especially the line on which the report restarts.

@ret2libc
Copy link
Member

@kazarmy we can still default to debugoptimized but use debug in the codecov CI job.

@kazarmy
Copy link
Member Author

kazarmy commented Dec 28, 2020

I did a separate build for Codecov 6a1af99 since didn't want to lose the running of tests on Linux with debugoptimized.

@kazarmy kazarmy marked this pull request as ready for review March 9, 2021 14:05
@kazarmy kazarmy merged commit 7a4f9b3 into dev Mar 10, 2021
@kazarmy kazarmy deleted the debugoptimized-asan branch March 10, 2021 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default meson buildtype to debugoptimized?
6 participants