Repository navigation
Managed ilasm: honour explicit local slots, scope local names lexically, and emit LocalScope/LocalVariable rows - #135311
Open
pcshrosbree wants to merge 3 commits into
Open
pcshrosbree wants to merge 3 commits into
pcshrosbree wants to merge 3 commits into
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
This was referenced Oct 6, 2026
pcshrosbree
force-pushed
the
ilasm-goodwill/local-scopes
branch
from
October 7, 2026 16:13
4779e33 to
c68ba68
Compare
pcshrosbree
force-pushed
the
ilasm-goodwill/local-scopes
branch
from
October 8, 2026 03:18
c68ba68 to
e908e28
Compare
pcshrosbree
force-pushed
the
ilasm-goodwill/local-scopes
branch
6 times, most recently
from
October 11, 2026 00:42
ae35425 to
9a7d2bd
Compare
…ical block
The managed ilasm appended every .locals declaration as the next slot of the
local signature and dropped the [n] of `.locals ([n] type name)`. It also
added the names declared in a nested { } block to the enclosing scope's names
when that scope had a .locals of its own, so an inner declaration of an outer
name resolved to the outer slot, and inner names stayed visible after the
block. Both change the IL of the body.
Locals now go through a per-method slot table, as in native ilasm's
EmitLocals: a declaration without [n] takes the slot after the highest one;
[n] takes slot n, padding the table up to it; the local signature has one
entry per slot in slot order. Redeclaring a slot that is in use reports
"Local var slot n is in use" (warning); as in native ilasm, a slot is in use
from a declaration until any scope that declared it closes. Redeclaring a
slot with another type reports "Local var slot n: type conflict", and the
slot takes the new type; a slot that no declaration types reports "Undefined
type of local var slot n in method m" and is written as int32. [-1] means no
explicit slot, as in native; an explicit slot outside the 16-bit range is
reported as written and the local takes the next slot, and a local whose next
slot would be past 65535 is reported and not declared. The range is checked
on the literal as written, so a literal too large for 32 bits is reported
too, rather than narrowed to a slot in range or to -1. A ... sentinel
standing alone in a .locals list no longer takes a slot, as in native ilasm.
On parameters, [n] keeps its meaning as a raw attribute value.
Each { } block, including .try, catch, filter, finally and fault bodies, now
opens a name scope that closes with it. A name resolves to the first
declaration of that name in the innermost open scope that declares it; names
are case-sensitive.
Tests: unit tests for each rule, and seeded generated methods with nested
blocks, explicit, padded and reused slots and shadowed names, checked against
a model of native ilasm's rules.
The managed ilasm wrote no LocalScope or LocalVariable rows, so debuggers saw
no local names. Each lexical scope of a method body (the body itself and each
{ } block, including .try, catch, filter, finally and fault bodies) is now
recorded when it closes, with its IL offsets and the named locals it declares,
and the PDB gets the rows native ilasm writes: one LocalScope per scope that
declares a named local, spanning the offsets at its { and } (the whole body
for the method's own scope), followed by one LocalVariable per named local
with the local's slot as Index and attributes 0. Unnamed locals get no row.
The scopes are recorded whenever a PDB is requested, by any of the switches
that request one.
The rows are sorted by start offset, then longest first, as the table
requires; scopes with the same range keep source order. Two cases where
native ilasm writes rows the specification does not allow are left out: a
scope without instructions (zero length), and a second variable with the
same name or index in one scope. Within a scope, each name gets a row at the
slot of its first declaration, the declaration the name refers to, unless an
earlier row of the scope already describes that slot; later declarations of
a name, and unnamed locals, take no part. So a row never describes a local
that its name does not refer to. Import scopes and local constants are not
recorded, as in native ilasm.
Tests: LocalScopeTests and LocalTests, and seeded generated cases that compare the
local signature, named references and LocalScope/LocalVariable rows with a model
of native ilasm's rules.
pcshrosbree
force-pushed
the
ilasm-goodwill/local-scopes
branch
from
October 15, 2026 20:53
9a7d2bd to
4d34622
Compare
The [n] of a .locals declaration was read by a digit loop of its own, so that a literal past 32 bits is checked against the slot range rather than narrowed by the int32 rule. It now takes the bases of the int32 rule (0x is hexadecimal, any other leading 0 is octal) and reads a decimal or hexadecimal magnitude with ulong.TryParse, whose parse fails only for a magnitude past 64 bits. An octal literal, for which NumberStyles has no option, is read by the int32 rule's own reader, which keeps only the low 64 bits of a longer literal; since 65535 is 0177777, an octal literal with more than six significant digits is past the slot range whatever those bits are. So a literal of any base outside the range, of any length, is reported as before, and none reads as a slot in it or as -1. An invalid octal literal is still reported once, by the int32 rule, and keeps its value. Tests: rows for 0xFFFFFFFFFFFFFFFF, 18446744073709551615 and 01777777777777777777777, which a 64-bit read takes as -1, and for 02000000000000000000000 and -02000000000000000000000, which it takes as 0; rows for in-range hexadecimal and octal literals with many leading zeros; and an invalid octal slot, which is reported only as an invalid literal.
jkoritzinsky
approved these changes
Oct 15, 2026
jkoritzinsky
enabled auto-merge (squash)
October 15, 2026 22:03
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.
Part of making the managed ilasm (
src/tools/ilasm) a drop-in replacement for native ilasm, as discussed in #135212. There @jkotas pointed to the managed rewrite as the way forward, @jkoritzinsky confirmed that PDB differences from native ilasm are unintended, and @am11 suggested combining efforts.#135297 (PDB documents) and #135289 (the separate
.pdbfile) have both merged, and this branch now sits directly on main: the two commits here are the whole diff.With the document changes (#135297) applied, the managed ilasm still failed the four
TestPortablePdbLocalScopecases of the runtime's PortablePdb tests (src/tests/ilasm/PortablePdb): it passed 6 of those 10 cases, where native ilasm passes all 10. I measured this by running those cases against both ilasms on Linux x64, outside the tree, at #135297's head before this change; this PR does not changesrc/tests/ilasm. The cause is three defects in local handling:{to the offset at its}. The method body as a whole is a scope, and so are.try,catch,filter,finallyandfaultbodies. Each named local in the scope gets a LocalVariable row (Index = slot, attributes 0). Rows are sorted by start offset, then longest first. Scope information is kept whenever a PDB is requested, by any of the switches that request one..locals init ([3] int32 x)was appended as the next slot. InTestLocalScopes4.il, the nameLOCAL_12andstloc.3meant different variables, and the signature had 6 slots instead of 5. After, as in nativeEmitLocals:[n]takes slotn, padding the table;[n]takes the slot after the highest one;...sentinel in a.localslist no longer takes a slot. Before, it took one with an invalid type byte and shifted every later local; native ilasm ignores it;[n]keeps its meaning..locals, an inner.localsdeclaring the same name resolved to the outer slot, and inner names stayed visible after the block. After: each block has its own name scope, searched innermost first; the first declaration of a name in a scope wins, and names are case-sensitive.Items 2 and 3 change the emitted IL for inputs with sparse or reused slots, with shadowed or out-of-scope names, or with a sentinel in
.locals. A name declared in a block and used after it is now an error, as in native ilasm. ildasm always writes sequential slots and unique names, so a round trip through ildasm cannot show these cases.Deliberate differences from native, listed in
MANAGED-ILASM-FIXES.md:[-1], which means none) is reported as written and the local takes the next slot. The range is checked on the literal as written, so a literal too large for 32 bits is reported too; native reads[4294967295]as[-1]and[0x10000000000000000]as slot 0 without a diagnostic. A local whose next slot would be past 65535 is reported and not declared. Native accepts such slots, which no local instruction can name; it writes slot 65536's PDB index as 0 and does not finish on other negative values.Other differences from native ilasm found in the same survey, not taken by any of these PRs, are tracked in #135314.
A question: the spec says a method's first scope spans the whole body. Like native ilasm, this change writes no whole-body scope when the method-level
.localsnames nothing (TestLocalScopes3expects that). Should a root scope always be written instead?A note on output:
TestLocalScopes4.ilnow triggers the in-use warning, as it does with native ilasm. The CLI's diagnostic rendering prints the location verbosely (the whole source text); that is a separate fix.Testing:
LocalTests(33): slot rules and diagnostics, including the in-use flag's native behaviour, the type a slot keeps after a conflict, out-of-range slots reported as written (including literals too large for 32 bits, one of which would otherwise read as[-1]), slot 65535 in decimal, hexadecimal and octal, and the 65535 boundary with and without a PDB; a sentinel in.locals, and a sentinel-prefixed type, which is a local; shadowing; out-of-scope names; duplicate names; case-sensitive names.LocalScopeTests(38), read back with System.Reflection.Metadata:[n]up to 65535, attributes;.try,catch,finally,filter, a block inside.try, and a program with a slot gap, a slot declared by both a.trybody and acatchbody, an unnamed local and a name declared again in thecatchbody, checked against the rows native ilasm writes for it and against its local signature;/DEBUG,/PDB, and the debug mode alone) gives the same rows and the same IL;/OPTIMIZEoffsets;/DETbyte-identical PDBs;/FOLDwith each folded method keeping its own names.src/tests/ilasm/PortablePdb) against both ilasms on Linux x64, with the fourTestPortablePdbLocalScopecases also run against the managed ilasm, and a fifth local-scope case for a slot gap, a reused slot, an unnamed local,.tryandcatchbodies and a shadowed name. Before this change the managed ilasm failed all five; after it, both ilasms pass all 14IlasmPortablePdbTestercases, so the managed ilasm passes the original 10 where it passed 6. The fifth case is now theLocalScopeTestsfactSlotGapReusedSlotAndShadowedNameInTryAndCatchBodies_GiveTheRowsOfNativeIlasm. This PR changes nothing undersrc/tests/ilasm.Note
This change was prepared with AI assistance (Anthropic Claude and OpenAI Codex) under my direction. AI agents wrote the code and ran the build and tests on my machine. I reviewed the change before posting.