Repository navigation
Link newlib's memcpy, memset and strcmp instead of libnix's - #3
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b7ff4381
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -DBENCH_VERSION='"$(VERSION)"' | ||
| # __NO_INLINE__ keeps libnix's string.h from inlining its own memmove, | ||
| # strcpy and strlen, so those calls reach the newlib versions linked below. | ||
| ALL_CFLAGS = $(CFLAGS) -noixemul -D__NO_INLINE__ $(INCLUDES) \ |
There was a problem hiding this comment.
When this commit is built in an existing checkout that already has build/ objects, make considers those caller objects up to date because the object rule depends only on the .c file, not on the Makefile or its flags. It therefore compiles only the new newlib objects, while pre-commit callers retain libnix's header-inlined memmove, strcpy, and strlen; the resulting benchmark binary mixes implementations and can still measure the old library/ROM paths. Make the objects depend on the Makefile or otherwise force a clean rebuild when these global flags change.
Useful? React with 👍 / 👎.
The string functions the timed code calls out of line now come from newlib's portable C sources, compiled by the compiler under test, instead of libnix, whose memcpy reaches exec CopyMem and whose archive was built by whatever built the toolchain. What libnix's string.h inlines stays. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
c2b7ff4 to
ae2fcd4
Compare
codewiz
changed the title
Workbench is not WorkBench, so this is not BenchWork either. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
codewiz
changed the title
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is coherent and the only identified issue is a non-blocking inaccurate comment.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Links portable newlib string routines into Benchwork binaries to ensure benchmark timings measure compiler-generated code rather than libnix implementations.
Changes:
- Vendors and builds newlib’s
memcpy,memset, andstrcmp. - Updates benchmark descriptions and documentation.
- Standardizes the project name as “Benchwork.”
| File | Description |
|---|---|
Makefile |
Builds and links newlib routines. |
README.md |
Expands benchmark and licensing details. |
benches/memcpy.c |
Updates benchmark descriptions. |
src/main.c |
Corrects project capitalization. |
.github/workflows/release.yml |
Corrects release title capitalization. |
third_party/newlib/README.md |
Documents vendored sources and rationale. |
third_party/newlib/LICENSE |
Adds applicable licensing. |
third_party/newlib/_ansi.h |
Provides an internal header shim. |
third_party/newlib/local.h |
Provides the loop-transformation guard. |
third_party/newlib/memcpy.c |
Adds portable memcpy. |
third_party/newlib/memset.c |
Adds portable memset. |
third_party/newlib/strcmp.c |
Adds portable strcmp. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* The one definition the string functions take from newlib's libc/string/local.h: | ||
| stop gcc from recognizing the copy loop inside memcpy() as a memcpy() and | ||
| calling the function from itself. */ |
libnix's
memcpyends in execCopyMem, which under volamos costs no emulated cycles at all, and the rest of libnix.a was compiled by whatever built the toolchain. The string functions the timed code calls out of line,memcpy,memsetandstrcmp, are now newlib's portable C versions (third_party/newlib/, unchanged, BSD), linked as objects so they win over the archive. What libnix'sstring.hinlines (memmove,strcpy, ...) stays as it is for any program.Same compiler, volamos 68040 at 25 MHz, checksums unchanged:
Second commit: README with one benchmark per row and the details in footnotes, and the name is Benchwork, like Workbench.