Skip to content

Add support for system BDWGC#29

Merged
kou merged 3 commits intouim:mainfrom
ivmai:add-bundled-bdwgc-option
Mar 28, 2026
Merged

Add support for system BDWGC#29
kou merged 3 commits intouim:mainfrom
ivmai:add-bundled-bdwgc-option

Conversation

@ivmai
Copy link
Copy Markdown
Contributor

@ivmai ivmai commented Mar 13, 2026

BDWGC 8.3.0 will have the feature that is provided by libgcroots: bdwgc/bdwgc#378

If we find system BDWGC 8.3.0 or later, we use it instead of libgcroots. We'll deprecate libgcroots and drop support for libgcroots eventually.

ivmai added 2 commits March 13, 2026 16:02
To use bdwgc, pass `--with_libgcroots=bundled-bdwgc` to configure.
@kou
Copy link
Copy Markdown
Member

kou commented Mar 16, 2026

Thanks!

But we don't want to bundle bdwgc. We want to use system bdwgc instead. If system bdwgc is enough recent, we'll use bdwgc. Otherwise, we'll use bundled libgcroots. We'll remove bundled libgcroots eventually.

Can I push to this branch something? I'll add a CI job that install unreleased bdwgc separately and use it as a system bdwgc.

@ivmai
Copy link
Copy Markdown
Contributor Author

ivmai commented Mar 17, 2026

We want to use system bdwgc instead.

Agree.

Can I push to this branch something? I'll add a CI job that install unreleased bdwgc separately and use it as a system bdwgc.

Sure.

@kou kou force-pushed the add-bundled-bdwgc-option branch 9 times, most recently from b315a2c to a7ad69e Compare March 26, 2026 08:33
@kou kou changed the title Add bundled bdwgc option Add support for system BDWGC Mar 26, 2026
@kou kou requested a review from Copilot March 26, 2026 08:52
@kou
Copy link
Copy Markdown
Member

kou commented Mar 26, 2026

  • Add CI jobs with system BDWGC master
  • Don't use src/gcroots/ with system BDWGC
  • src/storage-gc.c uses BDWGC directly

I'm not sure whether this works well but this is buildable now.

I'll merge this and test this in uim CI.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a build-time option to prefer a system-installed BDWGC (when available) over libgcroots, with corresponding build-system and CI updates to exercise the different GC backend configurations.

Changes:

  • Add SCM_WITH_BDWGC code path in src/storage-gc.c to use BDWGC stack-base handling and stack/register scanning callbacks.
  • Update Autotools + pkg-config metadata to detect and link BDWGC (falling back to libgcroots) and to make GC deps private for pkg-config consumers.
  • Adjust GitHub Actions CI to build with different --with-libgcroots settings and build/install BDWGC in the “installed” matrix case.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/storage-gc.c Adds BDWGC-based GC ready-stack handling and stack/register scanning integration.
src/sigscheme.h Removes unconditional gcroots.h inclusion from the public header.
src/Makefile.am Adds @BDWGC_*@ substitution vars to compilation/link lines.
sigscheme.pc.in Moves GC dependencies to Requires.private.
configure.ac Adds BDWGC pkg-config detection (fallback to gcroots) and substitutes BDWGC vars.
.github/workflows/linux.yml Adds a matrix over --with-libgcroots and builds BDWGC for the “installed” case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread configure.ac Outdated
Comment on lines 293 to 298
AC_DEFINE(SCM_WITH_BDWGC, 1, [Define to 1 to use BDWGC.])],
[PKG_CHECK_MODULES(
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots],
[AC_MSG_ERROR([installed libgcroots is not found.])])])
use_libgcroots="installed one"
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

When BDWGC is found, use_libgcroots is still set to "installed one", so the final configure summary prints libgcroots: installed one even though the build is actually using BDWGC instead of libgcroots. Consider updating the summary string (or adding a separate use_bdwgc/gc_backend summary) so ./configure output accurately reflects the selected GC backend.

Suggested change
AC_DEFINE(SCM_WITH_BDWGC, 1, [Define to 1 to use BDWGC.])],
[PKG_CHECK_MODULES(
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots],
[AC_MSG_ERROR([installed libgcroots is not found.])])])
use_libgcroots="installed one"
AC_DEFINE(SCM_WITH_BDWGC, 1, [Define to 1 to use BDWGC.])
use_libgcroots="BDWGC (installed)"],
[PKG_CHECK_MODULES(
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots
use_libgcroots="installed one"],
[AC_MSG_ERROR([installed libgcroots is not found.])])])

Copilot uses AI. Check for mistakes.
Comment thread configure.ac Outdated
Comment on lines 268 to 273
AC_ARG_WITH(libgcroots,
AS_HELP_STRING([--with-libgcroots@<:@=WHICH@:>@],
[use alternative libgcroots instead of
package-bundled one.
[use system BDWGC or alternative libgcroots
instead of package-bundled libgcroots.
WHICH=(installed|tiny-subst)
@<:@default=installed@:>@]))
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The help text says --with-libgcroots defaults to installed, but the implementation still falls back to no when the option is omitted (case "${with_libgcroots:=no}"). This makes the configure help misleading and also prevents the advertised "prefer system BDWGC when available" behavior unless the user explicitly passes --with-libgcroots=installed. Align the actual default with the help text (or update the help text to match the real default).

Copilot uses AI. Check for mistakes.
Comment thread src/storage-gc.c Outdated
static void GC_CALLBACK
bdwgc_mark(void **start, void **end, void *data, unsigned hint)
{
gc_mark_locations(*start, *end, scm_false, scm_false);
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

In the BDWGC marking callback, gc_mark_locations is being called with *start / *end (dereferencing the pointers) instead of the pointer range itself. gc_mark_locations expects a start/end address range; dereferencing will pass arbitrary stack contents as addresses and can lead to incorrect marking or crashes. Pass the range (with appropriate casts) rather than dereferencing.

Suggested change
gc_mark_locations(*start, *end, scm_false, scm_false);
gc_mark_locations(start, end, scm_false, scm_false);

Copilot uses AI. Check for mistakes.
Comment thread configure.ac Outdated
[PKG_CHECK_MODULES(
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots],
[AC_MSG_ERROR([installed libgcroots is not found.])])])
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

If bdw-gc is not found and gcroots is also missing, the configure error currently says only "installed libgcroots is not found" even though BDWGC was also attempted first. Adjust the failure message to mention that neither bdw-gc nor gcroots was found (and ideally include the required minimum versions) to make diagnosis easier.

Suggested change
[AC_MSG_ERROR([installed libgcroots is not found.])])])
[AC_MSG_ERROR([neither BDWGC (bdw-gc >= 8.3.0) nor libgcroots (gcroots >= 0.2.3) was found.])])])

Copilot uses AI. Check for mistakes.
@kou kou force-pushed the add-bundled-bdwgc-option branch from a7ad69e to 01bd5aa Compare March 26, 2026 09:07
@kou kou requested a review from Copilot March 26, 2026 09:13
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread configure.ac Outdated
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots
use_libgcroots="installed one"],
[AC_MSG_ERROR([neighter BDWGC nor libgcroots was found.])])])
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The error message has a spelling mistake: "neighter" → "neither". This string is user-visible during configure failures, so it’s worth correcting.

Suggested change
[AC_MSG_ERROR([neighter BDWGC nor libgcroots was found.])])])
[AC_MSG_ERROR([neither BDWGC nor libgcroots was found.])])])

Copilot uses AI. Check for mistakes.
@kou kou force-pushed the add-bundled-bdwgc-option branch from 01bd5aa to 8864e47 Compare March 26, 2026 09:24
@kou kou requested a review from Copilot March 26, 2026 09:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +60 to +62
--enable-conf=uim \
--enable-maintainer-mode \
--prefix=/usr \
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This job now configures with --enable-conf=uim, --prefix=/usr, and a --with-libgcroots matrix, whereas previously it only ran ./configure --enable-maintainer-mode. If the goal of this workflow is specifically to validate the GC backend selection, these extra flags change what configuration is being exercised and may reduce coverage of the default build. Consider either keeping the default configure flags and adding a separate job for the uim config, or documenting why the uim config is required here.

Suggested change
--enable-conf=uim \
--enable-maintainer-mode \
--prefix=/usr \
--enable-maintainer-mode \

Copilot uses AI. Check for mistakes.
Comment thread src/storage-gc.c

static void GC_CALLBACK
bdwgc_mark(void **start, void **end, void *data, unsigned hint)
{
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

bdwgc_mark doesn’t use the data or hint parameters. With -Wall this can produce unused-parameter warnings; consider explicitly marking them unused (e.g., via (void)data; (void)hint;) or annotating the parameters if you want warning-clean builds.

Suggested change
{
{
(void)data;
(void)hint;

Copilot uses AI. Check for mistakes.
Comment thread src/storage-gc.c
Comment on lines +1040 to +1044
static void GC_CALLBACK
bdwgc_find_object(void **start, void **end, void *data, unsigned hint)
{
BDWGCFindObjectData *find_object_data = (BDWGCFindObjectData *)data;
char *p = (char *)start;
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

bdwgc_find_object doesn’t use the hint parameter. To avoid unused-parameter warnings under -Wall, consider marking it unused explicitly (or using an attribute/macro consistent with the project).

Copilot uses AI. Check for mistakes.
Comment thread configure.ac
Comment on lines 268 to +299
@@ -287,11 +287,16 @@ case "${with_libgcroots:=no}" in
GCROOTS_REQ=gcroots
;;
yes|installed)
PKG_CHECK_MODULES(GCROOTS, [gcroots >= 0.2.3],
[],
[AC_MSG_ERROR([installed libgcroots is not found.])])
use_libgcroots="installed one"
GCROOTS_REQ=gcroots
PKG_CHECK_MODULES(
BDWGC, [bdw-gc >= 8.3.0],
[BDWGC_REQ=bdw-gc
AC_DEFINE(SCM_WITH_BDWGC, 1, [Define to 1 to use BDWGC.])
use_libgcroots="installed BDWGC"],
[PKG_CHECK_MODULES(
GCROOTS, [gcroots >= 0.2.3],
[GCROOTS_REQ=gcroots
use_libgcroots="installed one"],
[AC_MSG_ERROR([neither BDWGC nor libgcroots was found.])])])
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The PR description says the build should use system BDWGC when it is found, but configure currently only probes for BDWGC in the --with-libgcroots=yes|installed branch. With the current logic, the default (with_libgcroots=no) will always build/use the bundled libgcroots even if a suitable system BDWGC is available. If the intent is auto-preference for system BDWGC, consider probing BDWGC in the default path (and only falling back to bundled libgcroots if not found), or clarify the intended behavior in the option help/PR description.

Copilot uses AI. Check for mistakes.
uses: actions/checkout@v6
with:
path: bdwgc
repository: bdwgc/bdwgc
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The workflow checks out and builds BDWGC from the repository default branch without pinning a tag/commit (repository: bdwgc/bdwgc with no ref). This can make CI non-reproducible (upstream changes can break the build or change the reported pkg-config version relative to the bdw-gc >= 8.3.0 requirement). Consider pinning to a known-good release tag/commit (>= 8.3.0) or installing a packaged BDWGC version to keep CI deterministic.

Suggested change
repository: bdwgc/bdwgc
repository: bdwgc/bdwgc
ref: v8.3.0

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no such tag (v8.3.0), use commit sha instead, e.g. 946696b9b16580fcece3bc4d640f35afc139426d

Comment thread src/storage-gc.c
Comment thread configure.ac
If BDWGC 8.3.0 or later exists, we use it directly.
@kou kou force-pushed the add-bundled-bdwgc-option branch from 8864e47 to 1dab17a Compare March 26, 2026 21:30
@kou kou merged commit 2072b23 into uim:main Mar 28, 2026
11 of 12 checks passed
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