Conversation
Greptile SummaryThis PR moves the vendored Capstone build into Autotools, retains support for an external Capstone installation, pins C compilation to GNU17, and adds the libc shims needed by the newly integrated static library.
Confidence Score: 4/5The external-Capstone build path should be fixed before merging because an offline Git checkout can fail during autogen even though vendoring was explicitly bypassed. The new fatal submodule update runs before the configuration layer honors CAPSTONE_DIR, making the supported prebuilt-library override unusable when the checkout lacks initialized submodules and cannot access the network. Files Needing Attention: autogen.sh and m4/codspeed_capstone.m4
|
| Filename | Overview |
|---|---|
| autogen.sh | Adds fatal, unconditional submodule initialization for Git worktrees, preventing offline external-Capstone builds when the submodule is absent. |
| m4/codspeed_capstone.m4 | Defines vendored-versus-external Capstone configuration and pins GNU17, but its override is evaluated too late to prevent autogen.sh from cloning. |
| third_party/Makefile.am | Builds a primary-platform x86/AArch64 Capstone convenience archive and declares the vendored distribution inputs. |
| callgrind/cycledecode_capstone.c | Adds fprintf and strcat shims required by Capstone when compiled under Valgrind’s tool flags. |
| configure.ac | Integrates the new configuration macros and generates the third-party Makefile. |
| .github/workflows/release.yml | Removes the redundant standalone CMake Capstone build and relies on the integrated Autotools path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Git checkout] --> B[autogen.sh]
B --> C[Configure]
C --> D{External Capstone supplied?}
D -- No --> E[Build vendored libcapstone.a]
D -- Yes --> F[Use prebuilt libcapstone.a]
E --> G[Link primary Callgrind]
F --> G
Prompt To Fix All With AI
### Issue 1
autogen.sh:25
**Override still initializes submodule**
When a Git checkout has no initialized Capstone submodule and runs offline with `CAPSTONE_DIR` set, this fatal update attempts to clone the vendored source before configure can honor the external-library override, causing `autogen.sh` to exit and preventing the supported prebuilt-Capstone build.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "build: build the vendored Capstone from ..." | Re-trigger Greptile
| git config blame.ignoreRevsFile .git-blame-ignore-revs | ||
| # CodSpeed: the vendored Capstone decoder that Callgrind's cycle estimation | ||
| # links against. A clone without --recurse-submodules leaves it empty. | ||
| run git submodule update --init third_party/capstone |
There was a problem hiding this comment.
Override still initializes submodule
When a Git checkout has no initialized Capstone submodule and runs offline with CAPSTONE_DIR set, this fatal update attempts to clone the vendored source before configure can honor the external-library override, causing autogen.sh to exit and preventing the supported prebuilt-Capstone build.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: autogen.sh
Line: 25
Comment:
**Override still initializes submodule**
When a Git checkout has no initialized Capstone submodule and runs offline with `CAPSTONE_DIR` set, this fatal update attempts to clone the vendored source before configure can honor the external-library override, causing `autogen.sh` to exit and preventing the supported prebuilt-Capstone build.
**Knowledge Base Used:**
- [Build, platform, and testing infrastructure](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/valgrind-codspeed/-/docs/build-platform-and-testing.md)
- [Build and packaging workflows](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/valgrind-codspeed/-/docs/build-and-packaging.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
We should not run this unconditionnally IMO, especially if people provide a CAPSTONE_DIR argument
There was a problem hiding this comment.
Agreed. Making the update non-fatal still causes an unnecessary network attempt, and it can fail before configure gets a chance to honor CAPSTONE_DIR. The submodule initialization should be conditional on CAPSTONE_DIR being unset. --with-capstone=PATH is only parsed by configure, so autogen.sh cannot detect that form unless configure arguments are explicitly passed through to it.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
219de63 to
1f523ac
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
Callgrind's cycle estimation links a static Capstone, which until now had to
be built by hand before Valgrind: configure hard-failed unless it was given
--with-capstone=PATH or CAPSTONE_DIR. That put the same cmake invocation in a
wrapper script and in a CI action, where the two copies of the required
compiler flags could drift apart, and every new consumer had to repeat it.
Compile the submodule as an automake convenience library instead, so a plain
`./autogen.sh && ./configure && make && make install` is the whole build.
--with-capstone (or CAPSTONE_DIR) stays as an override for a prebuilt decoder,
which the Debian packaging still wires through, but nothing needs it any more:
the Nix dev shell no longer sets it either.
The submodule sources are compiled through one-line stub units in
third_party/capstone-stubs/ rather than where they live. Automake is configured
with subdir-objects, so it writes an object next to its source, and Valgrind
only builds in-tree: compiling them in place would drop objects, .deps
directories and .dirstamp files into the capstone checkout and leave the
submodule permanently reported as dirty. The stubs keep every build artefact in
this repository, where .gitignore covers it, instead of asking git to look away
from a submodule that is genuinely being written to.
The CodSpeed logic lives in files of its own (m4/codspeed_capstone.m4 and
third_party/Makefile.am), leaving three one-line touch points in upstream
files -- the macro call, an AC_CONFIG_FILES entry and a SUBDIRS entry -- in
place of the 28-line block this removes from configure.ac, so rebasing onto
upstream conflicts on single lines.
Two things the wrapper script was hiding, now handled by the build system:
- The tool CFLAGS pass -fno-builtin, so GCC no longer folds Capstone's
fprintf(stderr, "...") guard into fwrite. The tool shimmed only fwrite and
thus only linked by accident of that optimisation; shim fprintf and strcat
explicitly, and compile Capstone -DNDEBUG as its own Release build does so
assert() does not pull in __assert_fail.
- CFLAGS=-std=gnu17 was passed by the script alone, so builds through
debian/rules or nix never got it. AC_PROG_CC picks gnu23 on GCC 15+, under
which glibc 2.42+ defines strchr/strrchr/strstr as _Generic macros that
clash with Callgrind's own definitions. Pin it in configure instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CAPSTONE_LIBS is named in callgrind's _LDADD but not in its _DEPENDENCIES, and automake cannot tell that a configure substitution expands to a file, so it derived no dependency of its own either. Rebuilding the decoder therefore left callgrind-<platform> linked against the previous archive, silently, until something else forced a relink. Add it to the _DEPENDENCIES line the fork already sets for the primary platform. The secondary platform has no Capstone and needs nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1f523ac to
9cea895
Compare
Review feedback on #40. The submodule sources are compiled by explicit rules that write objects and dependency files under third_party/capstone-obj/ in the build directory, so the committed stub translation units are gone and the submodule checkout is left untouched by a build. - autogen.sh initialises the submodule only when CAPSTONE_DIR is unset and the checkout is empty, so a build against a prebuilt Capstone makes no network access and prints no warning. - flake.nix no longer ships Capstone; the vendored build covers the dev shell. - New CI job covering --with-capstone, and a check that the vendored build leaves the submodule clean. The job builds Capstone with cmake rather than installing libcapstone-dev: a distribution build is compiled with stack-protector (unresolved __stack_chk_guard) and with every architecture enabled (unresolved strtol from the other instruction printers), so it cannot be linked into a tool. - configure accepts a multiarch libdir for --with-capstone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
-std=gnu17 was appended to CFLAGS, so it reached the test programs too and changed the debug info gcc 11 emits for them, which made the inline-crossfile callgrind test fail on ubuntu-22.04. The pin exists for the strchr/strrchr/strstr definitions cycledecode_capstone.c makes under C23, so configure exports it as CODSPEED_C_STD and only the primary Callgrind tool is built with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cf751be to
b5f6cba
Compare
Callgrind's cycle estimation links a static Capstone, which until now had to be built by hand before Valgrind:
configurehard-failed unless it was given--with-capstone=PATHorCAPSTONE_DIR. That put the same cmake invocation in a wrapper script and in a CI action, where the two copies of the required compiler flags could drift apart, and every new consumer had to repeat it.This compiles the submodule as an automake convenience library instead, so a plain
./autogen.sh && ./configure && make && make installis the whole build.--with-capstonestays as an override for the Debian packaging, but nothing needs it any more.The CodSpeed logic lives in files of its own (
m4/codspeed_capstone.m4andthird_party/Makefile.am), leaving three one-line touch points in upstream files -- the macro call, anAC_CONFIG_FILESentry and aSUBDIRSentry -- in place of the 28-line block this removes fromconfigure.ac, so rebasing onto upstream conflicts on single lines.The submodule checkout is left untouched by a build: the sources are compiled by explicit rules that write every object and dependency file under
third_party/capstone-obj/in the build directory, rather than through_SOURCES, which would have automake drop objects,.depsand.dirstampnext to the sources.Two things the wrapper script was hiding, now handled by the build system
-fno-builtin, so GCC no longer folds Capstone'sfprintf(stderr, "...")guard intofwrite, nor expandsstrcatintostrlen+strcpy. The tool shimmed onlyfwriteand thus only linked by accident of those optimisations; this shimsfprintfandstrcatexplicitly, and compiles Capstone-DNDEBUGas its own Release build does soassert()does not pull in__assert_fail.CFLAGS=-std=gnu17was passed by the script alone, so builds throughdebian/rulesnever got it.AC_PROG_CCpicks gnu23 on GCC 15+, under which glibc 2.42+ definesstrchr/strrchr/strstras_Genericmacros that clash with Callgrind's own definitions.configurenow exports it asCODSPEED_C_STDand applies it to the primary Callgrind tool only, so the rest of the tree keeps the compiler's own default.CI: the
inline-crossfilefailure on ubuntu-22.04 is not caused by this PRThe same commit is green on
workflow_dispatchand red onpull_request, reproducibly on both sides, on the same runner image. Comparing a red run with a green one:third_party/libcapstone.aand theinline-crossfiletest binary are identical bit for bit,configure's output and the tool's link command line are identical line for line, and onlycallgrind-amd64-linuxdiffers -- built fromcoregrind/VEX/callgrindobjects this PR does not touch.That is enough to flip the expectation, because
cfni=???is not a marker of inlined code:callgrind/dump.cemits it when leaving an inlined range, guarded by a file-staticlast_inline_fnthat is never reset between dumps and is compared by pointer rather than by string. Its presence therefore depends on the order in which functions are dumped, i.e. on the tool binary's layout. Tracked separately.