Repository navigation
m3front: define the static link on both arms of a closure call; cm3: pass debug/optimize to the C back end (fixes #1243) - #1244
Open
mikan-cerebras wants to merge 2 commits into
Conversation
GenClosureCall emitted Pop_static_link only inside the arm taken when
the procedure value is a closure, while the indirect call after the
join reads the static link on both arms. For a top-level procedure
the static link was therefore read having never been written.
The value is genuinely a don't-care there -- the callee ignores it --
but "don't care" and "undefined" are not the same thing to an
optimizer. With the C back end and clang -O2 the never-written local
becomes undef, which lets SimplifyCFG treat the two arms as
interchangeable and fold the closure test away: the CL_marker
comparison is demoted to llvm.assume, and every suitably aligned
non-NIL procedure value then takes the closure path. It loads
CL_proc and CL_frame out of the target function's own instruction
stream and branches to the result.
Whether a given program crashes depends on where the linker places
the target procedure, because the "not 8-aligned, so not a closure"
pre-check survives the fold. A procedure landing at 4 mod 8 escapes
through it and is called correctly; one landing at 0 mod 8 is called
through its own code. Adding an unrelated field to a record can move
a program between the two.
Pop_static_link spills the stack top to a fresh temporary each time
it is called, so calling it a second time before the branch does not
help -- it allocates a second temp and the first store is dead. This
uses one temp, written on both arms, popped once after the join.
Fixed in the front end rather than in the C back end's
loadBeforeStore analysis. That analysis is branch-insensitive by
design ("This knowingly imagines the IR runs in order, without
branches around") and so does not mark this temp, but more
importantly M3CG_LLVM and M3x86 have no equivalent mitigation at all,
and the undefined read is the front end's to begin with.
Tested on ARM64_DARWIN, C back end:
- a 27-line reproducer passing a procedure as a parameter crashes
at -O2 before, prints the right answer after, with the target
procedure forced to 8-byte alignment so the arm is the failing one
- nested procedures still receive the correct static link: a closure
reading a captured local returns identical values under patched
-O2 and stock -O1
- a 20k-line numerical model builds at -O2 and reproduces its
reference output bit-for-bit, and a checkpoint-restart that
segfaulted at -O2 before now completes
- -O1 was unaffected throughout, before and after
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Builder.m3 hardcoded TRUE, FALSE for debug and optimize at the DoRunC call site, so with the C back end m3_optimize in an m3makefile and -O on the command line were silently ignored: every build was compiled -O0 with debug forced on, whatever the m3makefile asked for. Every other back end on the same path already passes u.debug and u.optimize, as do the other two RunCC call sites in the same file. Only this one did not. The flag does reach compile_c once this is fixed, measured on ARM64_DARWIN with a config carrying the current Unix.common: before: optimize= args=-g -O0 after: optimize=TRUE args=-g -O2 and the resulting objects differ, as expected. _m3main.cpp is unaffected: it is compiled through a separate call at the bottom of this file that passes debug := TRUE, optimize := FALSE deliberately, and still reports optimize= after this change. ORDERING NOTE: this change is what exposes the undefined static link fixed in the preceding commit. Taking this one alone turns on -O2 for the C back end and will produce programs that call procedure parameters through a word read out of their own instruction stream, depending on where the linker places the target. The two belong together, or this one belongs after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mikan-cerebras
changed the title
Contributor
|
This is not |
Author
sorry I don't know how this works. Do I need to log in with my maintainer credentials and approve something? Can you approve it? |
Contributor
|
I - can't. I don't have _any_ rights on cm3 repo.
This is annoying -(
- - -
Yes: you can logon and press something.
( In fact I don't know what exactly - see Ok part 1 of msg.)
…________________________________
From: Mika Nystrom ***@***.***>
Sent: Wednesday, October 28, 2026 8:37 PM
To: modula3/cm3 ***@***.***>
Cc: VictorMiasnikov ***@***.***>; Comment ***@***.***>
Subject: Re: [modula3/cm3] m3front: define the static link on both arms of a closure call; cm3: pass debug/optimize to the C back end (fixes #1243) (PR #1244)
[https://avatars.githubusercontent.com/u/254522122?s=20&v=4]mikan-cerebras left a comment (modula3/cm3#1244)<#1244 (comment)>
This is not https://github.com/modula3/cm3/actions/runs/40608654077 runned?
sorry I don't know how this works. Do I need to log in with my maintainer credentials and approve something? Can you approve it?
—
Reply to this email directly, view it on GitHub<#1244?email_source=notifications&email_token=ALUI7HGHSGS2ECLDPE7PHML5WIVETA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMNBXGYYTIOBUGMZ2M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-6476148433>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALUI7HHX4BYFXV2E7BOCTT35WIVETAVCNFSNUABDKJSXA33TNF2G64TZHM4TKOBTHE4DMO2JONZXKZJ3GYYDOMBQGI3TEMZQUF3AE>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Contributor
I can't You can. Yes: first logon, and... I don't know details -( |
Contributor
|
( Empty msg. May will be deleted later by my) |
Contributor
|
( Thank! Have you seen this? |
Contributor
|
P.S. Can You fix this: |
Contributor
|
It is best if Rodney fixes this problem. In the llvm backend there is some tenuous code to remove |
Contributor
Ok |
This branch has not been deployed
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
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.
GenClosureCallemitsPop_static_linkonly inside the arm taken when the procedure value is a closure, but the indirect call after the join reads the static link on both arms. When the value is a top-level procedure the static link is therefore read having never been written.m3-sys/m3front/src/types/UserProc.m3, before:The value really is a don't-care for a top-level procedure, since the callee ignores it. But don't care and undefined are not the same thing to an optimizer.
What goes wrong
With the C back end and clang
-O2, the never-written local becomesundef:undefmeans "any value I like", so the two arms become interchangeable andSimplifyCFGPassfolds the branch, keeping theCL_markercomparison only as an assumption:After that, every suitably aligned non-NIL procedure value takes the closure path, loads
CL_procandCL_frameout of the target function's own instruction stream, and branches to the result. The resultingpcdecodes to a real instruction — in our casestp x29, x30, [sp, #0x16], the prologue of the very function that should have been called.Bisected with
-opt-bisect-limit: pass #132,SimplifyCFGPass.Why it is intermittent
The "not 8-aligned, so not a closure" pre-check survives the fold, so whether a program crashes depends on where the linker puts the target procedure. Four variants of one test differing only in a record's field count:
& 70x1000012440x1000012880x1000012f00x1000015bcAdding an unrelated field can move a program between the two.
Why the fix is here and not in the C back end
M3C.m3'sloadBeforeStoreanalysis would normally emit={0}for a read-before-write temp, but it is branch-insensitive by design — its own comment says "This knowingly imagines the IR runs in order, without branches around." Patching it would fix one back end.M3CG_LLVMandM3x86have no equivalent mitigation at all, and the undefined read originates in the front end regardless.Note also that
Pop_static_linkspills the stack top to a fresh temporary on each call, so simply calling it again before the branch does not work — it allocates a second temp and the first store is dead. The fix uses one temp, written on both arms, popped once after the join.Testing
ARM64_DARWIN, C back end, Apple clang.
-O2before and prints the correct answer after, with the target procedure forced to 8-byte alignment so the failing arm is the one exercised.-O2and stock-O1(1407,407), so the environment is still delivered correctly — this was the obvious regression risk.-O2and reproduces its reference output bit-for-bit (99,999 rows, matching SHA-256), and a checkpoint-restart that segfaulted at-O2before now completes.-O1behaviour is unchanged before and after.Related, but not this bug
The
CL_markertest itself dereferences a function pointer as data, which is undefined in C (a function is not an object, so there is nothing there to read). That is worth addressing separately, but it is not what is happening here: under opaque pointers LLVM cannot tell the pointer designates a function, and a hand transcription of the dispatch sequence into plain C does not reproduce at any optimization level. Theundefis the whole trigger.Second commit: pass the unit's debug/optimize flags to the C back end
Added after the above, because it is what makes
-O2reachable for the C back end in the first place — and therefore what exposes the bug fixed in the first commit.Builder.m3hardcodedTRUE, FALSEfordebug, optimizeat theDoRunCcall site:so with the C back end,
m3_optimizein an m3makefile and-Oon the command line were silently ignored — every build was-O0with debug forced on. Every other back end on the same path already passesu.debug, u.optimize, as do the other twoRunCCcall sites in the same file. This fixes #1243.Measured on ARM64_DARWIN with a config carrying the current
Unix.common:compile_cccCallee_m.ooptimize=-g -O0f4e0ea4a…optimize=TRUE-g -O2ec98460b…_m3main.cppis unaffected — it goes through a separate call at the bottom of the file that passesdebug := TRUE, optimize := FALSEdeliberately, and still reportsoptimize=after the change.Please take these together, or this one second
Landing this commit alone turns on
-O2for the C back end and will produce programs that call procedure parameters through a word read out of their own instruction stream, on whichever procedures the linker happens to place at a multiple of 8. The static-link fix is what makes-O2safe here. With both commits applied, the same test program compiles at-O2and produces correct results for both top-level and nested procedures.Happy to split this into two PRs if you'd prefer to review them separately — the ordering constraint is the only reason they are together.