Revive the hlc.agc (accurate GC) grade. - #144
Draft
sebgod wants to merge 21 commits into
Draft
Conversation
sebgod
force-pushed
the
agc-revival
branch
2 times, most recently
from
April 30, 2026 10:07
1e2d8b2 to
71e15b4
Compare
When generating MLDS code for a procedure under --gc accurate,
ml_accurate_gc.ml_gen_make_type_info_var calls polymorphism's MI
wrapper to construct fresh HLDS goals that build the type_info needed
to trace each variable. If the type_info for the variable's type can be
built entirely from constant arguments, polymorphism takes the
const_struct path: it inserts a new entry into the module's
const_struct_db and emits a goal whose cons_id is type_info_const(N).
Those new entries land in module_info correctly, but the MLDS code
generator's ConstStructMap and GlobalData are snapshotted by
ml_top_gen.ml_code_gen *before* ml_gen_preds runs, so they never see
the entries the AGC trace generator added. The downstream map.lookup
in ml_unify_gen_construct.ml_generate_construction_unification then
fails with "key not found" for the newly-allocated ConstNum, aborting
compilation of nearly every standard library module in hlc.agc.
Fix this by extending the local ConstStructMap (and emitting any
needed MLDS data definitions into GlobalData) right after the
polymorphism call adds new entries.
compiler/ml_unify_gen_construct.m:
Add ml_extend_const_struct_map/6, which walks the module's
const_struct_db and calls ml_gen_const_struct/4 for each entry
that is not already in the supplied map. Add the private helper
ml_gen_const_struct_if_new/6 that performs the per-entry check.
compiler/ml_gen_info.m:
Add ml_gen_info_set_const_struct_map/3 to mirror the existing
getter, so callers can update the map after MLDS-time
insertions.
compiler/ml_accurate_gc.m:
Import ml_unify_gen_construct.
At the end of ml_gen_make_type_info_var, after the call to
polymorphism_make_type_info_var_mi, fetch the current map and
global data from the ml_gen_info, call ml_extend_const_struct_map
to cover any newly-inserted entries, and write the updated map
and global data back.
The accurate-GC C target previously emitted code that gcc rejected
as soon as a procedure had any pointer-typed locals or arguments.
Two distinct bugs in the MLDS->C path were combining to break it.
Reproducer (tiny.m):
:- module tiny.
:- interface.
:- import_module list.
:- pred go(list(int)::in, list(int)::out) is det.
:- implementation.
go([], []).
go([X|Xs], [X|Ys]) :- go(Xs, Ys).
Before this change, mmc --grade hlc.agc --compile-to-c tiny produced
a tiny__go_2_p_0 body that read and wrote (frame_ptr)->...frame_0__X,
without ever declaring frame_ptr, defining the per-procedure
_frame_0_s struct, or pushing a frame onto MR_StackChain. The same
pattern blew up library/array.c across roughly 60 procedures
(sort_1_f_0, samsort_subarray_*, array_equal_*, the auto-generated
__Compare____array_1_0 / __Unify____array_1_0, and others). After
that first issue was fixed, gcc reported 'stack_chain' undeclared
for every procedure that took an AGC frame, because the MLDS
backend was emitting the unqualified token where the runtime
declares mercury__private_builtin__stack_chain.
With both fixes, tiny.m generates the expected pseudocode from
ml_elim_nested.m's design discussion (per-procedure _frame_0_s,
frame_ptr = &frame, stack_chain = frame_ptr, plus the matching
trace function), and library/array.c compiles under gcc with no
errors. Around 150 standard library modules that previously could
not get past gcc in hlc.agc now build cleanly. The hlc.gc grade is
unaffected: the ml_elim_nested.m change only fires under
chain_gc_stack_frames, and the mlds.m change only renames the C
identifier emitted for the lvnc_stack_chain MLDS variable.
compiler/ml_elim_nested.m:
In ml_elim_nested_defns_in_func, the gate that decided whether
to skip ml_create_env tested only NestedFuncs0 = []. The
in-source comment explicitly noted that under accurate GC the
skip is also valid only when there are no pointer-typed locals
to chain, but the code did not check that. Under
chain_gc_stack_frames, flatten_statement still promotes such
locals into per-frame env-field accesses, so taking the skip
branch left the rewritten body referencing a frame_ptr that
was never declared.
Convert the disjunction into an if-then-else whose then-branch
(do nothing) requires both NestedFuncs0 = [] and either
Action = hoist_nested_funcs or Locals = []. Otherwise fall
through to ml_create_env so the frame struct, frame_ptr
declaration, and MR_StackChain linkage are emitted alongside
the rewritten body.
compiler/mlds.m:
In ml_local_var_name_to_string, change the C name string for
lvnc_stack_chain from "stack_chain" to
"mercury__private_builtin__stack_chain", matching the runtime
extern declared in runtime/mercury.h. The MLDS-level identifier
is left as lvnc_stack_chain so the design discussion in
ml_elim_nested.m and the related lvnc_saved_stack_chain still
read naturally. Adding a "#define stack_chain ..." in
runtime/mercury.h was tried first, but it shadows the
stack_chain parameter name used in runtime/mercury_accurate_gc.c
and is rejected by -Werror=shadow.
Two further issues block accurate-GC C compilation once the per-frame
struct/frame_ptr/stack_chain wiring (commit acc427ab2) is in place.
The first surfaces in library/builtin.c, where the auto-generated
trace function tuple_arg_3_p_0_10000 calls
mercury__private_builtin__gc_trace_1_p_0 with a TypeInfo argument
that has C type MR_Word *, not MR_Word. This happens whenever the
typeinfo source is itself a pointer-typed local — that is, an
existentially typed output parameter such as TypeInfo_for_ArgT in
tuple_arg/3, which the MLDS argument-handling code wraps in
mlds_ptr_type because the value is returned by reference. gcc rejects
this with `-Werror=int-conversion'.
The second surfaces in library/getopt.c, where the per-procedure
frame struct emitted for record_unnegated_long_option_8_p_0 has the
same set of MaybeOptArg/Error/Var/MaybeSepArg/etc. fields declared
twice in a row, because the procedure has multiple disjoint MLDS
scopes — switch arms — that each declare locals with the same name.
ml_elim_nested.m's flatten pass snocs each occurrence onto
ei_local_vars without deduplicating, so when the env struct is
materialised the C compiler reports duplicate members.
With both of these fixes (and the previous two), `mmake install'
in --grade hlc.agc compiles roughly 200 standard library modules,
including builtin, getopt and getopt_io, and stops at a different
problem in io.error_util — a missing scalar_common reference, which
is a distinct issue that will need its own change.
compiler/ml_accurate_gc.m:
In ml_gen_trace_var, wrap TypeInfoRval in an ml_cast to
CPointerType before passing it to private_builtin.gc_trace. The
cast prints as `(MR_Word)' under the C backend, which is what
gc_trace_1_p_0's first parameter is declared as in
private_builtin.mih. For the common case where TypeInfoRval
already has MR_Word-equivalent type, the cast is a no-op; for
the existential-output case described above, it bridges the
pointer-vs-integer mismatch that gcc was rejecting.
The semantics of tracing existentially typed outputs at GC time
is still imprecise (the slot the typeinfo points to may not be
initialised yet); that is the documented liveness gap from
Phase 1 of the AGC roadmap. This commit only addresses the
compile-time blocker.
compiler/ml_elim_nested.m:
Change elim_info_finish so that, after converting the
ei_local_vars cord to a list, it keeps only the first occurrence
of each mlds_local_var_name. The new keep_first_local_var_per_name
helper does the dedupe in O(n log n) using a set of names already
seen.
The duplicate entries always have the same name and type — they
come from the same logical Mercury variable that was promoted
into env-field form once per scope it appears in — so dropping
the later occurrences cannot lose information; the env struct is
just a per-procedure home for the variable, and the original
scopes are disjoint at run time.
ml_gen_proc in compiler/ml_proc_gen.m calls ml_gen_info_proc_params to
build the func_params for the procedure. Under --grade hlc.agc, this
also generates a gc_statement for every argument: each gc_statement
contains MLDS code that calls polymorphism's MI wrapper to construct
a type_info, may insert new const_struct entries into the module's
const_struct_db, and may add new scalar_common cells to the
ml_gen_info's GlobalData (the latter via ml_gen_goal_as_block as it
converts the typeinfo-construction HLDS goal to MLDS).
The call was made with the syntax `!.Info, _Info', discarding the
updated ml_gen_info. The procedure body then ran with stale state:
typeinfos referenced by the trace function (e.g. for an output
argument of type maybe(io.error)) pointed at scalar_common indices
that had never been recorded in GlobalData. The C compiler later
rejected the resulting source with errors like
io.error_util.c: In function
`mercury__io__error_util__is_error_maybe_win32_6_p_0_10000':
error: `mercury__io__error_util_scalar_common_4' undeclared
because `scalar_common_4' was referenced by the emitted trace
function but never defined.
Threading !Info through the ml_gen_info_proc_params call in the
non-external branch keeps the generated trace function and the
file-level data definitions in sync, and the io.error_util
gc-trace functions now match scalar_commons that actually exist.
After this change, `mmake install' completes the entire hlc.agc
library install, and a tiny test program
:- pred main(io::di, io::uo) is det.
main(!IO) :-
go([1,2,3,4,5], Ys),
io.write_string("Result: ", !IO),
io.write(Ys, !IO), io.nl(!IO).
go([], []).
go([X|Xs], [X|Ys]) :- go(Xs, Ys).
builds and runs correctly under --grade hlc.agc, printing
`Result: [1, 2, 3, 4, 5]'.
compiler/ml_proc_gen.m:
In ml_gen_proc, change the ml_gen_info_proc_params call in the
normal-procedure branch from `!.Info, _Info' to `!Info', so that
arg gc_statement updates to ConstStructMap and GlobalData
propagate into the rest of procedure code generation.
Leave the call in the external-procedure branch as
`!.Info, _Info'; that branch produces no trace function, so
there is nothing to keep in sync, and threading !Info there
only fires a singleton-state-variable warning. A short
comment explains the asymmetry.
MR_schedule_agc is invoked from the SIGSEGV redzone signal handler
that sets up an accurate-GC trigger on the next procedure return.
Per POSIX async-signal-safety rules, fprintf and the rest of stdio
are not safe to call in a signal handler -- they take locks, allocate,
and may interact with locale state. Calling them here was a known
TODO listed in the file header and at the top of the function body.
This change introduces small static helpers that use write(2) on
STDERR_FILENO together with stack-only buffers, and converts all
the diagnostic fprintf calls inside MR_schedule_agc -- both the
always-on error paths (already-running collector, missing stack
layout info) and the MR_DEBUG_AGC_SCHEDULING traces -- to use them.
The helpers are intentionally minimal: they format pointers, hex,
and signed/unsigned decimal without touching stdio, malloc, or the
locale, all of which are unsafe in a signal handler.
The remaining fprintf calls in this file all live in MR_garbage_collect,
MR_LLDS_garbage_collect, and the various notify_gc_* helpers, which
run after the signal handler returns and the procedure reaches the
GC entry label, so they are not in signal context and do not need
the same treatment.
runtime/mercury_accurate_gc.c:
Add agc_safe_write_str / _uhex / _ptr / _udec / _sdec helpers
just before MR_schedule_agc, gated on MR_HAVE_UNISTD_H for the
write(2) include and falling back to STDERR_FILENO == 2 if the
platform header does not define it.
Convert every fprintf inside MR_schedule_agc to a sequence of
agc_safe_write_* calls. Drop the now-unnecessary fflush(NULL)
calls in the debug paths, since write(2) is unbuffered.
Drop the file-header TODO bullet about using write() in the
signal handler, and replace the in-function XXX comment with a
note explaining the async-signal-safety constraint.
The deep-copy walk in mercury_deep_copy_body.h is structurally
right-recursive for cons-cell-shaped data (lists and similar two-arg DU
constructors): every cell does
MR_field(0, new_data, 1) = copy(data_value[1], list_type, lo, hi);
The result of copy() must be stored in the parent's tail slot, so the
parent's stack frame stays alive across the call. For a list of length
N this uses N C stack frames -- about 96 bytes per frame on aarch64 --
which overflows the default 8 MB pthread stack at roughly 80k cons
cells. Under accurate GC the walk runs from inside the collector
itself, so the stack overflow becomes an unrecoverable SIGSEGV with no
runtime handler available; under Boehm the same overflow can occur
inside MR_make_long_lived and the heap-reclamation paths.
This change adds a tail-call elimination fast path at the top of the
MR_TYPECTOR_REP_DU case. For values whose layout has MR_SECTAG_NONE,
arity 2, no packed argument locations, no existential info, and whose
second argument's resolved type ctor matches the parent's, we iterate
the spine: allocate one cell, copy the head argument recursively
(bounded by element-type complexity, not list length), wire the new
cell into the chain via *spine_dest, leave a forwarding pointer for
the old cell, and continue with the next data value via while (1)
instead of recursing through copy(). Off-spine arguments still recurse
normally; only the dominant right-leaning recursion is flattened.
Spine depth becomes O(1). The recursion bound for the head argument
is type-bounded, so even a list of arrays-of-records walks each cell
in constant stack and only recurses for the head's type complexity.
The fast path applies in both deep-copy variants because the body file
is shared, so MR_deep_copy (used by MR_make_long_lived and the
solutions/global heap copies) benefits as well.
Important caveat: the AGC code path is currently not exercised at
runtime in this workspace. The installed libmer_rt.so and libmer_std.so
under lib/mercury/lib/hlc.agc/ both carry MR_grade_v19_hlc_gc grade
markers, meaning they were built without -DMR_NATIVE_GC, so the AGC
variant of MR_deep_copy compiled to nothing. tests/hard_coded
"under hlc.agc" is in fact running under Boehm GC. This change
therefore exercises only via the Boehm path right now; once the
library is genuinely rebuilt with --grade hlc.agc and -DMR_NATIVE_GC
defined, the same fast path covers MR_agc_deep_copy. See handoff.md.
runtime/mercury_deep_copy_body.h:
Insert the spine-iteration fast path in the MR_TYPECTOR_REP_DU
case, between the ptag_layout lookup and the existing switch on
MR_sectag_locn. The path opens a do/while loop that allocates one
new cell per iteration, copies the head argument synchronously
via copy() or copy_arg(), threads new_data into the chain through
a spine_dest pointer, leaves a forwarding pointer for the old
cell, advances to the tail, and either iterates (when the next
cell has the same functor descriptor) or breaks out and recurses
once on the terminator. Falls through to the existing handling
when the pattern does not match.
Use MR_make_type_info_maybe_existq once at the top of the path to
materialise the resolved tail type, and free its allocator chain
at the single exit points. The materialised type info is reused
for every iteration, since the loop only iterates while the cell
shape and tail type ctor stay the same.
The MR_has_forwarding_pointer bitmap macros in mercury_deep_copy.c
computed `1 << fwdptr_bit` where fwdptr_bit can range over
0..MR_WORDBITS-1. On 64-bit platforms MR_WORDBITS is 64 and the literal
`1` is `int` (32 bits), so any shift by >= 32 is undefined behaviour
in C. aarch64 gcc lowers the shift to AND the count with 31, which
silently aliases bit 32 onto bit 0, bit 33 onto bit 1, and so on.
Cells whose offset within the from-space landed in the upper half of
a 64-cell stripe therefore set, and tested, the same bit as a cell
elsewhere in the stripe. When the second cell was visited by deep_copy
its first word was misinterpreted as a forwarding pointer; for cons
cells the first word is the head value (often a small int or the
empty list), so deep_copy returned that as the "copied" cell pointer.
The result was the X-side-of-list corruption pattern observed in
tests/hard_coded/array_sort under genuine accurate GC: the head field
read back as the integer 3 while the tail field survived intact.
Switching the shift to `((MR_Word) 1) << fwdptr_bit` gives the operand
the correct width on every platform we care about.
runtime/mercury_deep_copy.c:
Cast the constant 1 to MR_Word in both mark_as_forwarding_pointer
and if_forwarding_pointer before shifting by fwdptr_bit. Add a
comment above the macros explaining the failure mode in case the
pattern is repeated elsewhere in the codebase.
Programs whose live data set grows past the initial --heap-size
(hash_table_test, version_hash_table_test_2, and similar working
examples in tests/hard_coded) hit the to-space's MR_zone_hardmax
mid-copy and abort with "memory zone heap2#... overflowed", even
though MR_extend_zone exists and would have grown the zone if asked.
The post-GC resize path runs only after the copy completes; if the
copy itself overflows there is no recovery.
This change inserts a pre-GC capacity check at the top of
MR_garbage_collect: if the to-space's current capacity is smaller
than 2 * old_used (the from-space's used range, doubled to match the
existing post-GC MR_heap_expansion_factor=2 policy), call
MR_extend_zone to grow the to-space first. The Cheney copy is
bounded above by from-space usage modulo per-element type-info
materialisation overhead, so 2 * old_used is a safe ceiling. Sizing
to-space to match the post-GC steady-state target also avoids
oscillating between extending and shrinking on every cycle.
runtime/mercury_accurate_gc.c:
Add a capacity-check block immediately after the new_heap /
old_hp setup at the top of MR_garbage_collect. Compute old_used
from the from-space's MR_zone_min and the current MR_virtual_hp,
target 2 * old_used rounded up to MR_unit, and call
MR_extend_zone on the to-space when the existing capacity is
smaller. Guard against multiplication overflow by capping the
target at old_used when the doubling wraps.
The previous default of 8192 words (64 KB on 64-bit) was small enough
that any non-toy program triggered constant garbage collections and
hit the zone-extension paths immediately on startup. The setting
predates current memory norms and current Mercury workloads.
The redzone+mprotect machinery and the new pre-GC MR_extend_zone path
in MR_garbage_collect together let zones grow from any starting size,
so the default is now just a starting point that controls when GC
overhead first kicks in. 1024 * 1024 words (8 MB on 64-bit) keeps the
hello-world cost negligible while letting tests/hard_coded programs
run without requiring --heap-size to be set on the command line.
The MR_DEBUG_AGC_SMALL_HEAP path that hard-codes 13 words for stress
testing the collector is left untouched.
runtime/mercury_wrapper.c:
Change the default of MR_heap_size from 8192 * sizeof(MR_Word)
to 1024 * 1024 * sizeof(MR_Word) when MR_DEBUG_AGC_SMALL_HEAP is
not defined. Add a comment explaining the rationale and pointing
at the zone-extension path that handles the growth from here.
The last-call-modulo-cons (LCMC) optimisation in lco.m was previously
disabled in every accurate-GC grade. The transformation captured
&cell.field as a Mercury value of type store_at_ref_type(T) and then
relied on `*AddrC = V` stores in the variant. Under accurate GC that
interior pointer is invisible to the collector: when the parent cell
is evacuated during a Cheney copy, AddrC is left pointing into
from-space, and the next iteration's store either silently drops or
corrupts the chain. The handle_options.m gate dating from when the
collector was first added simply turned LCMC off rather than fix the
captured-pointer kind. As a result the lco_reorder hard_coded test
overflowed the C stack at 10M cells, since `dup_literal/1` could no
longer be flattened into an iteration.
This change introduces a cell-and-offset capture path that runs
alongside the existing interior-pointer path, gated on
HighLevelCode = yes && GC_Method = gc_accurate && HighLevelData = no
(the LLD MLDS+AGC regime). In that regime AddrVar holds the parent
cell pointer, the variant takes it under in_mode, and the in-variant
store goes through a new private_builtin.store_at_field_offset_impure
builtin whose lowering walks the cell pointer to a static word
offset baked in at lco transformation time. The collector traces
the cell normally as a tagged pointer; the spine fast path in
mercury_deep_copy_body.h already interprets a NULL hole as `[]`,
so a partially-built chain copies cleanly and a post-GC store from
the resumed iteration overwrites the tentative `[]` with the real
tail. The HLD path is unchanged. The LLDS back-end still goes
through the gate because we have not yet wired field_assign through
LLDS deep-copy stack tracing.
library/private_builtin.m:
Declare store_at_field_offset_impure/3, mirroring
store_at_ref_impure/2. The cell, offset, and value arguments
take the place of (interior pointer, value).
compiler/introduced_call_table.m:
Register the new pred so the table-based no-type-info-builtin
machinery recognises arity-3 calls from the LCMC pass.
compiler/builtin_ops.m:
Add a field_assign(Cell, Offset, Value) simple_code constructor
and the corresponding builtin_translation_private_builtin clause
that maps store_at_field_offset_impure onto it.
compiler/call_gen.m:
Lower field_assign in LLDS to assign(field(no, Cell, Offset),
Value). LLDS does not currently generate this builtin (LLDS+AGC
LCMC stays off via handle_options.m), but the path is wired in
case future work routes LLDS through the new builtin too. Also
add the field_assign disjunct to the model_semi unexpected
catchall.
compiler/ml_call_gen.m:
Lower field_assign in MLDS to ml_field(no, Cell, _, ml_field_offset(
Offset), generic) := Value. The runtime ptag-stripping form
ml_field(no, ...) lets us avoid threading the static ptag down
from lco.m. Also add the field_assign disjunct to the model_semi
unexpected catchall.
compiler/term_constr_initial.m:
Add store_at_field_offset_impure to the arity-3 list of
no_type_info_builtins that contribute no termination
constraints, alongside the analogous store_at_ref_impure entry.
compiler/hlds_pred.m:
Recognise store_at_field_offset_impure as an inline_builtin
where pred_info_builtin_special_pragma already does so for
store_at_ref_impure: builtin calls must be inlined to avoid
self-recursion through the auto-generated body.
compiler/add_pred.m:
Stub out store_at_field_offset_impure on Java/C# targets the
same way store_at_ref_impure is stubbed: those backends do not
support raw-memory stores.
compiler/lco.m:
Add lci_use_field_path to lco_const_info, set when the grade is
LLD MLDS + accurate GC. The flag drives a new third path that
parallels HLD's pass-the-cell shape:
- make_address_var leaves AddrVar's type as void_type so that
update_construct_args can later replace it with the parent
cell type (matching HLD's deferred typing).
- update_construct_args, when PassCell = yes, updates AddrVar's
type entry to the parent cell type and records a field_id; the
LLD branch leaves the inst as ground because LLD does not
track per-field insts.
- make_addr_vars in the variant signature uses in_mode rather
than HLD's from_to_mode for the LLD+AGC case, since the cell
appears ground throughout.
- make_variant_args turns on yes(field_id) for the new path so
the variant carries the cons_id+arg_num metadata needed for
offset lookup at the store site.
- make_store_goal splits into make_store_goal_hld_unify (the
existing HLD deconstruction path) and make_store_goal_lld_field,
which materialises the offset as an int constant via
make_int_const_construction_alloc_in_proc, then emits a
private_builtin.store_at_field_offset_impure call. The offset
is read out of car_pos_width on the constructor's repn, so
it includes any sectag-word adjustment.
compiler/ml_unify_gen_construct.m:
Generalise ml_gen_field_take_address_assigns: dispatch on a new
lco_assign_kind that picks the cell-passing path for
HighLevelData = yes OR GC = gc_accurate. The lco_const_info
flag and this dispatch agree about what AddrVar looks like, so
the construction's take-address arg captures the cell pointer
instead of an interior pointer in the new regime.
compiler/handle_options.m:
Relax the LCMC gate: allow LCMC under gc_accurate when
highlevel_code = yes. LLDS+AGC still disables LCMC because the
LLDS field_assign lowering has not been integrated with the
LLDS scheduler's stack-frame trace generation, and the LLDS
deep-copy path still relies on the now-stale interior-pointer
capture in lco.m's old branch.
The lco_assign_kind dispatch added in the previous commit checks the
GC method via gc_accurate, but the module was not yet importing
libs.globals. mmc rejected the reference at compile time. Add the
import.
compiler/ml_unify_gen_construct.m:
Add an explicit `:- import_module libs.globals' line alongside
the existing libs.optimization_options import.
The bootstrap compiler used to build a fresh stage-1 compiler does
not yet recognise store_at_field_offset_impure as an inline builtin,
so when it tried to compile private_builtin.m it rejected the
declaration with "predicate has no clauses". Once the stage-1
compiler is built the recognition flips on and the new compiler
lowers calls inline at the simple_code stage, bypassing whatever
body is in private_builtin.m. We just need a body that is good
enough for the bootstrap to accept.
The chosen stub is a det clause that calls imp (to satisfy the
impure declaration) and then sorry/1, so any actual runtime call
through the bootstrap-compiled libmer_std would loudly abort
rather than silently doing nothing. No call ever reaches this
body in practice because every emitter of the builtin goes through
the new compiler's inline lowering.
library/private_builtin.m:
Add a clause for store_at_field_offset_impure/3 in the
implementation section, alongside the existing sorry/no_clauses
helpers, with a comment pointing at why the stub exists.
The first stub for store_at_field_offset_impure used `impure imp,
sorry/1`, which inferred determinism `erroneous'. With
--halt-at-warn that became a fatal mismatch against the declared
`is det'. It also lived next to sorry/1 in the implementation
section, which violates the declaration-order check (the
declaration sits between typed_compare/3 and unused/0).
Replace the body with a det stub that does nothing but call
private_builtin.imp/0 to satisfy the impure marker. Move the clause
to immediately after typed_compare/3 to match declaration order.
The body remains dead code in any binary produced by the new
compiler, since the new compiler lowers store_at_field_offset_impure
inline at the simple_code stage.
library/private_builtin.m:
Remove the sorry-based stub from the implementation tail.
Add the new det stub `impure imp' immediately after
typed_compare/3, with a comment explaining why the body exists.
The new compiler recognises store_at_field_offset_impure/3 as an
inline builtin, so when it sees the bootstrap stub clause for the
predicate inside private_builtin.m it raises "Error: clause for a
builtin predicate". The stub clause must remain in private_builtin.m
until the older bootstrap compilers stop being asked to compile it,
because they reject the declaration without it. The compiler's
own --allow-defn-of-builtins option is the documented escape hatch
for exactly this lifecycle (see options.m:6139-6149).
Pin the flag to private_builtin only, so other modules still get
the strict check.
library/Mercury.options:
Add MCFLAGS-private_builtin += --allow-defn-of-builtins, with
a comment that ties the flag's lifetime to the lifetime of the
bootstrap stub.
The earlier rounds tried two staged stub bodies for the new builtin
in private_builtin.m: first a sorry-based clause that conflicted
with the declared det determinism, then a det `impure imp' clause
combined with --allow-defn-of-builtins on the new compiler. The
second arrangement got past type checking but the compiler still
crashed with `map.lookup: key not found' inside the optimization
interface pass for that module, presumably because something in
the polymorphism / pre-typecheck flow disagrees about which
typeinfo vars belong to the stub clause.
pragma external_pred is the documented Mercury idiom for "this
predicate has no Mercury body, the implementation lives in foreign
code". The bootstrap compiler accepts it without expecting a body,
and the new compiler still recognises the builtin via
builtin_ops.m and lowers calls inline, so no foreign body is ever
linked. We were already using pragma external_pred for similar
predicates in builtin.m, exception.m, par_builtin.m, etc., so this
follows the established pattern.
library/private_builtin.m:
Replace the stub body for store_at_field_offset_impure/3 with
a pragma external_pred declaration. Update the doc comment to
point at the bootstrap rationale.
library/Mercury.options:
Drop the --allow-defn-of-builtins entry for private_builtin
that the previous attempt added; the pragma external_pred
declaration removes the need for it.
pragma external_pred is not permitted in module interfaces, so the
declaration in the previous attempt failed to compile. Move the
pragma into the matching :- implementation. block immediately
after the declaration's interface section ends, alongside the
existing unused/nyi_foreign_type_*/etc. bodies.
library/private_builtin.m:
Remove the pragma external_pred line that sat next to the
interface declaration of store_at_field_offset_impure/3, and
add it to the implementation section starting at line 1569,
with a comment explaining the placement.
The currently-installed compiler (built from this branch) already
recognises store_at_field_offset_impure/3 as an inline builtin via
builtin_ops.builtin_translation_private_builtin and registers it
through introduced_call_table.m. When add_pred.add_builtin runs,
the C-target arm therefore takes the existing pseudo-recursive
forwarding path — exactly the same path that store_at_ref_impure/2
follows — and emits a self-call clause whose body the
inline-builtin lowering replaces with the field-write at the
simple_code stage.
That makes both the ad-hoc Mercury stub body and the pragma
external_pred declaration redundant, and the latter conflicts with
the auto-generated forwarding clause ("predicate has clauses, so
it cannot be marked as external"). Drop them both, leaving only
the bare declaration and its doc comment.
library/private_builtin.m:
Remove the implementation-side pragma external_pred and the
paragraph in the interface comment that explained it.
Under hlc.agc, heap allocations encode the primary tag via
MR_mkword(t, addr) = addr + t, which only produces a recoverable
ptag when addr is MR_Word-aligned. The heap zone's MR_zone_min
is therefore expected to be word-aligned. Two paths inside
mercury_memory_zones.c could violate that invariant.
MR_init_offsets seeds the per-engine cache-colouring offset from
MR_fake_reg's link-time address modulo MR_pcache_size. On targets
where MR_fake_reg is not word-aligned (which can happen on
win-arm64 and on the linux-aarch64 WSL host), the colouring
offset itself ends up byte-aligned, so MR_zone_min = base + offset
inherits the misalignment.
MR_extend_zone computes copy_size and offset by subtracting
MR_Word * pointers. C pointer arithmetic returns the difference
in element units, not bytes, so each round-trip through
MR_extend_zone scales those byte counts by 1/sizeof(MR_Word).
The first realloc shifts MR_zone_min toward the start of the
new zone instead of preserving its byte offset, eventually
producing a misaligned min pointer.
Both errors were latent under hlc.gc because Boehm allocates
outside the Mercury zone scheme. Under hlc.agc they manifest
as garbled ptags inside MR_deep_copy and intermittent
segfaults during the second collection.
runtime/mercury_memory_zones.c:
In MR_init_offsets, round each cache-colour offset up to the
next MR_Word boundary before storing it. The rounding shifts
the offset by at most sizeof(MR_Word) - 1 bytes per slice,
which is below the cache-colouring resolution we care about.
In MR_extend_zone, cast MR_zone_end / MR_zone_bottom /
MR_zone_min to (char *) before subtracting so that copy_size
and offset are byte counts. Without the casts, extending a
zone N times misaligns MR_zone_min by sizeof(MR_Word) ** N.
When LCMC inserts calls to store_at_field_offset_impure/3, the
addr-of-field var, offset, and ground-result share an arity-3 call
with no extra typeinfo arguments at the HLDS level. The polymorphism
pass walks the predicate's argument list and, by default, prepends
typeinfo args for any var whose type is polymorphic, growing the
call site to five arguments. recompute_instmap_deltas later reads
the proc's headvar list (3 entries) against that five-arg call and
aborts with "compute_inst_var_sub: length mismatch" inside the
tree234 module on any program built with --optimize-constructor-
last-call under hlc.agc.
no_type_info_builtin/3 is the canonical hook for "this private
builtin is lowered inline by the back-end and must not be wrapped
in typeinfo args by polymorphism". The companion arm in
term_constr_initial.process_no_type_info_builtin already lists the
new builtin; this patch closes the gap on the
mdbcomp/program_representation side, which the polymorphism pass
consults.
The comment block above no_type_info_builtin/3 in this file warns
that adding an entry only takes effect for code built by a
compiler linked with the rebuilt mdbcomp library, which matches
the bootstrap flow used for the AGC revival.
mdbcomp/program_representation.m:
Add store_at_field_offset_impure/3 to the no_type_info_builtin
private_builtin block, alongside store_at_ref_impure/2 and the
other LCMC store primitives.
The MLDS lowering of the field_assign(Cell, Offset, Value)
primitive in ml_gen_builtin assigned ml_lval(ValueLval) directly
into a slot whose declared type is mlds_generic_type (= MR_Box,
= void *). The C back-end therefore emitted
MR_hl_mask_field((MR_Word) Cell, Offset) = ValueRval;
with no explicit conversion of ValueRval. For ValueLval of type
MR_Word the implicit conversion to MR_Box is at most an
int-to-pointer warning; under -Werror=int-conversion (which
bootcheck imposes on hlc.agc) the warning becomes a fatal error.
For ValueLval of type MR_Float — produced by LCMC patterns that
pack a float into a generic-typed field, see
tests/hard_coded/lco_pack_args_3 — gcc rejects the conversion
outright with "cannot convert to a pointer type", because there
is no implicit float-to-pointer coercion in ISO C.
The MLDS already provides ml_box(Type, SubRval) for exactly this
case: the C back-end's mlds_output_boxed_rval_float emits
MR_box_float(...) for floats, mlds_output_boxed_rval_int64 emits
MR_box_int64 for 64-bit ints, and the default arm emits a plain
"((MR_Box) (...))" cast for word-sized values. Switching from a
bare assign (or, equivalently, from ml_cast(mlds_generic_type,
...)) to ml_box(ValueType, ...) lets the back-end pick the right
boxing routine per value type.
Extracting ValueType requires that ValueLval be ml_local_var; the
existing ref_assign branch already relies on the same shape.
Strengthen the if-then-else guard to demand both Cell and Value
are local-var lvals so the type is always available.
With this change tests/hard_coded passes cleanly under hlc.agc on
linux-aarch64 (WSL Ubuntu 24.04) with --optimize-constructor-
last-call enabled — including lco_pack_args_3 and lco_reorder.
compiler/ml_call_gen.m:
In the model_det field_assign arm of ml_gen_builtin, lift
ValueLval's type out of its ml_local_var wrapper and wrap the
rval in ml_box(ValueType, ...) before passing it to
ml_gen_assign. Update the surrounding comment to describe the
box choice and why a bare cast would mis-handle floats.
The MR_extend_zone helper, used by MR_garbage_collect's pre-extend to grow the AGC to-space, was passing new_size to MR_realloc_zone_memory while updating the zone's MR_zone_top to new_base + new_total_size, where new_total_size = new_size + 2 * MR_unit (one page for the hardzone, one extra in the MR_PROTECTPAGE case). The trailing one or two pages were claimed by the zone struct but never actually allocated. MR_setup_redzones then ran mprotect(zone->MR_zone_hardmax, MR_page_size, REDZONE_PROT) on a page that lives outside our allocation. By sheer virtual-memory layout, that page can sit inside a neighbouring live allocation -- typically the partner heap zone allocated by the same engine -- and the mprotect call silently turns a chunk of someone else's data read-only. Once that happens, the next write the collector performs into the neighbour's data area (e.g. setting a forwarding pointer in a from-space cell) faults inside the bogus protected page. The SIGSEGV handler reports `memory zone heap#1 overflowed` even though MR_virtual_hp has only advanced a few kilobytes into the to-space, because the fault is recorded against whichever zone nominally owns the protected page, not against the zone whose allocation logic actually triggered the fault. The symptom was lco_reorder under hlc.agc with --heap-size 1024 through 131072 KB: GC #2 always crashed inside the Cheney copy. After the fix the test passes cleanly across that whole range. hard_coded under hlc.agc continues to pass with no regressions. runtime/mercury_memory_zones.c: Pass new_total_size, not new_size, to MR_realloc_zone_memory in MR_extend_zone, so the realloc covers every byte of [MR_zone_bottom, MR_zone_top). Add a comment explaining why, so future readers do not collapse the size back to new_size.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
As a proof of concept of using Claude Opus 4.7 1m context model,
this PR revive the hlc.agc grade so that tests/hard_coded passes end-to-end.
The grade had bit-rotted: the compiler emitted code that did not
compile in the hlc.agc grade, the LCMC transformation was disabled
under accurate GC, and the Cheney collector had several latent bugs
that only manifested once enough live data was being copied. The
last commit on this branch passes tests/hard_coded in grade hlc.agc
on aarch64 Linux.
(Also: use async-signal-safe stderr writes in the LLDS GC scheduler
in runtime/mercury_accurate_gc.c, so panics inside the collector
no longer deadlock on stdio buffers.)