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

CI: Integrate macOS/arm64 #569

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ChinYikMing
Copy link
Collaborator

@ChinYikMing ChinYikMing commented Feb 13, 2025

Summary by Bito

This PR enhances the CI pipeline support for macOS/arm64 builds by updating build configurations and managing macOS-specific dependencies. Key improvements include restructuring prebuilt artifact handling, implementing conditional compilation guards for RISC-V instructions, and overall optimizations for cross-platform compatibility.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

Copy link

bito-code-review bot commented Feb 13, 2025

Code Review Agent Run #288b18

Actionable Suggestions - 0
Review Details
  • Files reviewed - 4 · Commit Range: a4f8041..7af92d2
    • mk/artifact.mk
    • mk/softfloat.mk
    • mk/tools.mk
    • src/riscv.c
  • Files skipped - 1
    • .github/workflows/main.yml - Reason: Filter setting
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

@ChinYikMing
Copy link
Collaborator Author

ChinYikMing commented Feb 13, 2025

macOS runners still suffering occasionally from fetching the latest release tag of prebuilt binaries.

This is the code snippet where the macOS runners fail:
https://github.com/sysprog21/rv32emu/blob/b8d6fa757fb65992af1e123835c0797dc9a446c8/mk/artifact.mk#L57C1-L63C10

I planned to create three tag name which are always point to the latest prebuilt release. So that, no need to fetch the latest release tag name.
For ELF binaries, call ELF-latest.
For Linux images, call Linux-image-latest.
For Sail model, call Sail-latest.
If has any other prefer name, please let me know.

Before uploading built artifact, delete the previous tag name from one of three of them by git push -d origin xxx (where xxx is one of three of the tag name). Then create the tag name and link to the release prebuilt binaries.

What do you guys think?

Quote from the issue. The idea is similar to 7af92d2.

@ChinYikMing
Copy link
Collaborator Author

ChinYikMing commented Feb 13, 2025

The JIT test fails as mention in the issue. So that this changes can be postponed to be merged until the JIT warning error is handled. See CI.

@ChinYikMing ChinYikMing marked this pull request as draft February 13, 2025 19:41
Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - CI/Build System Enhancements

artifact.mk - Updated release tag handling and prebuilt blob URL configuration

softfloat.mk - Added compiler warning suppression for uninitialized variables

tools.mk - Added macOS-specific build dependencies and clarified object file requirements

riscv.c - Added conditional compilation guards for system instructions

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: ad16cd8 Previous: f6af403 Ratio
Dhrystone 1239 Average DMIPS over 10 runs 1331 Average DMIPS over 10 runs 1.07
Coremark 937.092 Average iterations/sec over 10 runs 946.757 Average iterations/sec over 10 runs 1.01

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

@ChinYikMing
Copy link
Collaborator Author

The macOS host CI completes in approximately 25 minutes, exceeding the Linux host by 7 minutes but still noticeably faster than Linux/arm64. This seems acceptable for adopting the macOS host CI.

strategy:
fail-fast: false
matrix:
compiler: [gcc-14, clang]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if it makes sense to validate gcc-14 builds on macOS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if it makes sense to validate gcc-14 builds on macOS.

gcc is still available via brew install gcc (specifically as gcc-14, the latest version). Embracing gcc builds in CI on macOS could benefit gcc enthusiasts? Additionally, building with gcc on macOS uncovered some potential build errors or warnings, which have been addressed in these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the use of gcc. Let's skip the tests on different optimization flags since we already check the general optimizations on Ubuntu Linux prior to macOS builds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with the use of gcc. Let's skip the tests on different optimization flags since we already check the general optimizations on Ubuntu Linux prior to macOS builds.

But, the softfloat warning is revealed with gcc-14 + O1 flag on macOS, see the commit message of 1d497b1. Shall we keep them?

Copy link
Contributor

Choose a reason for hiding this comment

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

But, the softfloat warning is revealed with gcc-14 + O1 flag on macOS, see the commit message of 1d497b1. Shall we keep them?

The default CFLAGS includes -O2, and it should be sufficient to make the compilation optimization issues verbose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But, the softfloat warning is revealed with gcc-14 + O1 flag on macOS, see the commit message of 1d497b1. Shall we keep them?

The default CFLAGS includes -O2, and it should be sufficient to make the compilation optimization issues verbose.

Got it, but I will remain the '-Wno-uninitialized' CFLAGS for softfloat.

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@ChinYikMing ChinYikMing force-pushed the ci-macOS branch 2 times, most recently from 8172b84 to cd7c0e1 Compare February 18, 2025 13:57
Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@ChinYikMing
Copy link
Collaborator Author

ChinYikMing commented Feb 24, 2025

TODO: add emcc build in CI. #576

Edit: done.

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@ChinYikMing
Copy link
Collaborator Author

ChinYikMing commented Feb 27, 2025

The JIT test fails as mention in the issue. So that this changes can be postponed to be merged until the JIT warning error is handled. See CI.

Fixed in commit 297db9a, check CI.

@ChinYikMing
Copy link
Collaborator Author

ChinYikMing commented Feb 28, 2025

One remaining issue to be fixed is that fetching failure of the latest release tag of rv32emu-prebuilt. on macOS/arm64, the failure can be happened very often. The commit da51ade eliminates calling the Github API then avoid the issue. Trying to investigate the root cause.

ChinYikMing and others added 6 commits March 1, 2025 13:51
Error:
Undefined symbols for architecture arm64:
  "_ebreak_handler", referenced from:
      lC0 in riscv.o
  "_ecall_handler", referenced from:
      lC0 in riscv.o
  "_log_impl", referenced from:
      _rv_create in riscv.o
      _rv_create in riscv.o
      _rv_create in riscv.o
      _rv_profile in riscv.o
      _rv_profile in riscv.o
      _rv_run in riscv.o
  "_log_level_string", referenced from:
      _rv_create in riscv.o
  "_log_set_level", referenced from:
      _rv_create in riscv.o
  "_log_set_stdout_stream", referenced from:
      _rv_remap_stdstream in riscv.o
  "_memcpy_handler", referenced from:
      lC0 in riscv.o
  "_memory_delete", referenced from:
      _rv_delete in riscv.o
      _rv_create in riscv.o
  "_memory_ifetch", referenced from:
      _on_mem_ifetch in riscv.o
  "_memory_new", referenced from:
      _rv_create in riscv.o
  "_memory_read_b", referenced from:
      _on_mem_read_b in riscv.o
  "_memory_read_s", referenced from:
      _on_mem_read_s in riscv.o
  "_memory_read_w", referenced from:
      _on_mem_read_w in riscv.o
  "_memory_write_b", referenced from:
      _on_mem_write_b in riscv.o
  "_memory_write_s", referenced from:
      _on_mem_write_s in riscv.o
  "_memory_write_w", referenced from:
      _on_mem_write_w in riscv.o
  "_memset_handler", referenced from:
      lC0 in riscv.o
  "_rv", referenced from:
      _rv_async_block_clear in riscv.o
  "_rv_step", referenced from:
      _rv_run in riscv.o
      _rv_run in riscv.o
  "_trap_handler", referenced from:
      lC0 in riscv.o
ld: symbol(s) not found for architecture arm64
collect2: error: ld returned 1 exit status
make: *** [build/rv_histogram] Error 1

Add emulate.o, syscall.o, syscall_sdl.o, io.o, and log.o objects and
conditional build rv_async_block_clear() fixes the error and passes the
build.
Specify using gcc-14 instead of gcc (which typically points to the
latest version of gcc) due to a macOS limitation in the
rlalik/setup-cpp-compiler@master action. According to [1], each gcc must
have specified version.

Softfloat build warning error using gcc-14 with optimization flag O1:
In file included from src/softfloat/source/include/internals.h:42,
                 from src/softfloat/source/s_mulAddF64.c:40:
In function 'softfloat_sub128',
    inlined from 'softfloat_mulAddF64' at
src/softfloat/source/s_mulAddF64.c:185:17:
src/softfloat/source/include/primitives.h:526:17: error: 'sig128C.v64'
may be used uninitialized [-Werror=maybe-uninitialized]
  526 |     z.v64 = a64 - b64;
      |             ~~~~^~~~~
src/softfloat/source/s_mulAddF64.c: In function 'softfloat_mulAddF64':
src/softfloat/source/s_mulAddF64.c:66:20: note: 'sig128C.v64' was
declared here
   66 |     struct uint128 sig128C;
      |                    ^~~~~~~
In function 'softfloat_sub128',
    inlined from 'softfloat_mulAddF64' at
src/softfloat/source/s_mulAddF64.c:185:17:
src/softfloat/source/include/primitives.h:527:18: error: 'sig128C.v0'
may be used uninitialized [-Werror=maybe-uninitialized]
  527 |     z.v64 -= (a0 < b0);
      |              ~~~~^~~~~
src/softfloat/source/s_mulAddF64.c: In function 'softfloat_mulAddF64':
src/softfloat/source/s_mulAddF64.c:66:20: note: 'sig128C.v0' was
declared here
   66 |     struct uint128 sig128C;
Add -Wno-initialized to surpress them.

Drop the undefined behavior test on macOS/arm64 using gcc-14, as its
libsanitizer's configure.txt does not yet support it, check [2].

[1] https://github.com/rlalik/setup-cpp-compiler
[2] https://github.com/iains/gcc-darwin-arm64/blob/master-wip-apple-si/
    libsanitizer/configure.tgt

Close sysprog21#519
unused function 'emit_dataproc_3source' [-Werror,-Wunused-function]
Copy link

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

@ChinYikMing ChinYikMing marked this pull request as ready for review March 1, 2025 05:56
@ChinYikMing ChinYikMing requested review from vacantron and jserv March 1, 2025 06:28
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.

3 participants