Fix build failures and expand test coverage #535
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the -Werror flag to the CFLAGS to treat all warnings as errors during the build process. This ensures that warnings are blocked by CI, enforcing cleaner code and preventing builds from succeeding with any warnings
When using make 'ENABLE_JIT=1', the following error is observed:
src/jit.c:1760:17: error: ‘do_sret’ defined but not used [-Werror=unused-function]
1760 | static void do_##inst(struct jit_state *state UNUSED, riscv_t *rv UNUSED, \
| ^~~
Add "SYSTEM" to the EXT_LIST, which cause SERT to be add SKIP_LIST
during template generation. The prevents the generatoin of unused JIT
code for the SERT instruction.
Fixes: fc0d878 ("Preliminary support for trap handling during block emulation")
Add compiler matrix strategy to test builds with both gcc-latest and clang-latest compilers. This ensures the codebase compiles and passes tests with different compilers.
When using 'make ENABLE_Zba=0 ENABLE_JIT=1', the following errors are observed: src/jit.c:1760:17: error: ‘do_sh3add’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_sh2add’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_sh1add’ defined but not used [-Werror=unused-function] Include 'Zba' in EXT_LIST to ensure instructions like 'sh3add', 'sh2add', and 'sh1add' are properly handled, resolving unused function errors during JIT builds.
Add a new test case to the CI pipeline to verify JIT functionality when the Zba extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_Zbb=0 ENABLE_JIT=1', the following errors are observed: src/jit.c:1760:17: error: ‘do_rev8’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_orcb’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_rori’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_ror’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_rol’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_zexth’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_sexth’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_sextb’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_minu’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_maxu’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_min’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_max’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_cpop’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_ctz’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_clz’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_xnor’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_orn’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_andn’ defined but not used [-Werror=unused-function] Include 'Zbb' in EXT_LIST to ensure instructions like 'rev8', 'orcb', and 'rori' are properly handled, resolving unused function errors during JIT builds.
Add a new test case to the CI pipeline to verify JIT functionality when the Zbb extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_Zbc=0 ENABLE_JIT=1', the following errors are observed: src/jit.c:1760:17: error: ‘do_clmulr’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_clmulh’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_clmul’ defined but not used [-Werror=unused-function] Include 'Zbc' in EXT_LIST to ensure instructions like 'clmulr', 'clmulh', and 'clmul' are properly handled, resolving unused function errors during JIT builds.
Add a new test case to the CI pipeline to verify JIT functionality when the Zbc extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_Zbs=0 ENABLE_JIT=1', the following errors are observed: src/jit.c:1760:17: error: ‘do_bseti’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_bset’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_binvi’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_binv’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_bexti’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_bext’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_bclri’ defined but not used [-Werror=unused-function] src/jit.c:1760:17: error: ‘do_bclr’ defined but not used [-Werror=unused-function ] Include "Zbs" in EXT_LIST to ensure instructions like `bseti`, `bset`, and `bclr` are properly handled, resolving unused function errors during JIT builds.
Add a new test case to the CI pipeline to verify JIT functionality when the Zbs extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_Zicsr=0', the following error is observed:
src/emulate.c:561:10: error: ‘rv_insn_csrrw’ undeclared (first use in this function); did you mean ‘rv_insn_csrai’?
561 | case rv_insn_csrrw:
| ^~~~~~~~~~~~~
| rv_insn_csrai
The error occurs because CSR instruction enums are only defined when
the Zicsr extension is enabled. By adding the feature guard, we ensure
the switch cases are only compiled when the required instruction
definitions are available.
When using 'make ENABLE_Zicsr=0 ENABLE_JIT=1', the following error is
observed:
src/jit.c:1760:17: error: ‘do_csrrwi’ defined but not used [-Werror=unused-function]
1760 | static void do_##inst(struct jit_state *state UNUSED, riscv_t *rv UNUSED, \
| ^~~
src/rv32_jit.c:363:1: note: in expansion of macro ‘GEN’
363 | GEN(csrrwi, {assert(NULL);
| ^~~
This ensures csrrwi instructions are properly excluded from JIT code
generation when the Zicsr extension is disabled.
Add a new test case to the CI pipeline when the Zicsr extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_EXT_M=0 ENABLE_JIT=1', the following error is
observed:
src/jit.c:1693:13: error: ‘ra_load2_sext’ defined but not used [-Werror=unused-function]
1693 | static void ra_load2_sext(struct jit_state *state,
| ^~~~~~~~~~~~~
Fix this by guarding the ra_load2_sext function definition with
RV32_HAS(EXT_M) macro. This ensures the function is only defined when
the M extension is enabled.
Add a new test case to the CI pipeline to verify JIT functionality when the EXT_M extension is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_MOP_FUSION=0', the following error is observed:
src/emulate.c:714:13: error: ‘match_pattern’ defined but not used [-Werror=unused-function]
714 | static void match_pattern(riscv_t *rv, block_t *block)
| ^~~~~~~~~~~~~
Fix this by guarding the match_pattern function definition with
RV32_HAS(MOP_FUSION) macro. This ensures the function is only defined
when the MOP_FUSION is enabled.
Add a new test case to the CI pipeline when the MOP FUSION is disabled. This ensures better coverage and prevents regressions for this specific configuration.
Add a new test case to the CI pipeline when the BLOCK_CHAINING is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make tests CC=clang', the following error is observed: build/cache/test-cache.o: file not recognized: file format not recognized clang: error: linker command failed with exit code 1 (use -v to see invocation) Adding $(LDFLAGS) resolves the issue, ensuring successful linking.
Add a new test case to the CI pipeline when the Zba is disabled. This ensures better coverage and prevents regressions for this specific configuration.
Add a new test case to the CI pipeline when the Zbb is disabled. This ensures better coverage and prevents regressions for this specific configuration.
Add a new test case to the CI pipeline when the Zbc is disabled. This ensures better coverage and prevents regressions for this specific configuration.
Add a new test case to the CI pipeline when the Zbs is disabled. This ensures better coverage and prevents regressions for this specific configuration.
When using 'make ENABLE_Zicsr=0 CC=clang', the following error is
observed:
src/emulate.c:83:20: error: unused function 'update_time' [-Werror,-Wunused-function]
83 | static inline void update_time(riscv_t *rv)
| ^~~~~~~~~~~
1 error generated
Fix this by guarding the update_time function definition with
RV32_HAS(Zicsr) macro. This ensures the function is only defined
when the Zicsr is enabled.
When using 'make ENABLE_MOP_FUSION=0 CC=clang', the following error is observed: src/emulate.c:695:20: error: unused function 'remove_next_nth_ir' [-Werror,-Wunused-function] 695 | static inline void remove_next_nth_ir(const riscv_t *rv, | ^~~~~~~~~~~~~~~~~~ 1 error generated. Fix this by guarding the remove_next_nth_ir function definition with RV32_HAS(MOP_FUSION) macro. This ensures the function is only defined when the MOP_FUSION is enabled.
When using 'make ENABLE_MBLOCK_CHAINING=0 CC=clang', the following
errors are observed:
src/emulate.c:555:19: error: unused function 'insn_is_unconditional_branch' [-Werror,-Wunused-function]
555 | FORCE_INLINE bool insn_is_unconditional_branch(uint8_t opcode)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/emulate.c:582:19: error: unused function 'insn_is_direct_branch' [-Werror,-Wunused-function]
582 | FORCE_INLINE bool insn_is_direct_branch(uint8_t opcode)
| ^~~~~~~~~~~~~~~~~~~~~
Fix this by guarding the 'insn_is_unconditional_branch' and
'insn_is_direct_branch' function definition with
RV32_HAS(BLOCK_CHAINING) macro. This ensures the functions are only
defined when the BLOCK_CHAINING is enabled.
When using 'make make ENABLE_EXT_C=0 ENABLE_JIT=1 CC=clang', the
following errors are observed:
rc/t2c.c:77:1: error: unused function 't2c_gen_ra_addr' [-Werror,-Wunused-function]
77 | T2C_LLVM_GEN_ADDR(ra, X, rv_reg_ra);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/t2c.c:64:31: note: expanded from macro 'T2C_LLVM_GEN_ADDR'
64 | FORCE_INLINE LLVMValueRef t2c_gen_##reg##_addr( \
| ^~~~~~~~~~~~~~~~~~~~
<scratch space>:75:1: note: expanded from here
75 | t2c_gen_ra_addr
| ^~~~~~~~~~~~~~~
src/t2c.c:78:1: error: unused function 't2c_gen_sp_addr' [-Werror,-Wunused-function]
78 | T2C_LLVM_GEN_ADDR(sp, X, rv_reg_sp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/t2c.c:64:31: note: expanded from macro 'T2C_LLVM_GEN_ADDR'
64 | FORCE_INLINE LLVMValueRef t2c_gen_##reg##_addr( \
| ^~~~~~~~~~~~~~~~~~~~
Fix this by guarding the 't2c_gen_ra_addr' and 't2c_gen_sp_addr'
function definition with RV32_HAS(EXT_C) macro. This ensures the
functions are only defined when the EXT_C is enabled.
When using 'make ENABLE_EXT_M=0 ENABLE_JIT=1 CC=clang', the following
error is observed:
src/jit.c:728:20: error: unused function 'emit_alu64_imm8' [-Werror,-Wunused-function]
728 | static inline void emit_alu64_imm8(struct jit_state *state,
| ^~~~~~~~~~~~~~~
Fix this by guarding the 'emit_alu64_imm8' function definition with
RV32_HAS(EXT_M) macro. This ensures the function is only defined
when the EXT_M is enabled.
When using 'make ENABLE_UBSAN=1 check CC=clang', the following error is observed: src/emulate.c:1110:13: runtime error: call to function do_fuse1 through pointer to incorrect function type 'bool (*)(struct riscv_internal *, const struct rv_insn *, unsigned long, unsigned int)' /home/eleanor/code/rv32emu/src/emulate.c:415: note: do_fuse1 defined here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/emulate.c:1110:13 After fixing the first error with 'do_fuse1', similar errors were observed for other functions like 'do_fuse2', 'do_fuse3', 'do_fuse4', and 'do_jal'. The root cause was type mismatches in parameter declarations, where 'rv_insn_t *' was used instead of the expected 'const rv_insn_t *'. Since 'do_jal' was generated by the 'RVOP' macro in 'rv32emu_template.c', the macro was also corrected. These changes resolve the UBSAN errors and align all function pointers and implementations.
jserv
reviewed
Jan 6, 2025
There was a problem hiding this comment.
Benchmarks
| Benchmark suite | Current: 08146d9 | Previous: ba5521c | Ratio |
|---|---|---|---|
Dhrystone |
1329 Average DMIPS over 10 runs |
1346 Average DMIPS over 10 runs |
1.01 |
Coremark |
942.861 Average iterations/sec over 10 runs |
963.43 Average iterations/sec over 10 runs |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
visitorckw
reviewed
Jan 6, 2025
| make ENABLE_JIT=1 clean && make ENABLE_Zba=0 ENABLE_JIT=1 check -j$(nproc) | ||
| make ENABLE_JIT=1 clean && make ENABLE_Zbb=0 ENABLE_JIT=1 check -j$(nproc) | ||
| make ENABLE_JIT=1 clean && make ENABLE_Zbc=0 ENABLE_JIT=1 check -j$(nproc) | ||
| make ENABLE_JIT=1 clean && make ENABLE_Zbs=0 ENABLE_JIT=1 check -j$(nproc) |
There was a problem hiding this comment.
How about repeating all diverse configuration tests with JIT enabled?
| make distclean && make ENABLE_Zba=0 check -j$(nproc) | ||
| make distclean && make ENABLE_Zbb=0 check -j$(nproc) | ||
| make distclean && make ENABLE_Zbc=0 check -j$(nproc) | ||
| make distclean && make ENABLE_Zbs=0 check -j$(nproc) |
There was a problem hiding this comment.
Maybe also add:
make distclean && make ENABLE_Zifencei=0 check -j$(nproc)
|
Hmmm, it seems CI fails due to insufficient memory. Any ideas on how we can reduce the memory usage? |
eleanorLYJ
force-pushed
the
improve-build-tests
branch
2 times, most recently
from
ba5521c to
08146d9
Compare
eleanorLYJ
changed the title
henrybear327
reviewed
Jan 7, 2025
| @@ -7,7 +7,7 @@ BIN := $(OUT)/rv32emu | |||
| CONFIG_FILE := $(OUT)/.config | |||
| -include $(CONFIG_FILE) | |||
|
|
|||
| CFLAGS = -std=gnu99 -O2 -Wall -Wextra | |||
There was a problem hiding this comment.
Since we are modifying this, do we also want to add -Wshadow to catch more problems :) ?
There was a problem hiding this comment.
I think this depends on whether our coding style aims to forbid local variables from shadowing other variables?
jserv
changed the title
jserv
reviewed
Jan 8, 2025
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| compiler: [gcc-latest, clang-latest] |
There was a problem hiding this comment.
Any reason why ordinary gcc and clang fail to work?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix build issues and enhance build system tests
Improve our build system testing and fix various build issues discovered in the process
CI Improvements
Build Fixes
$(LDFLAGS)Testing Done