Conversation
main.rs declared its own `mod abi; mod arch; mod ir; ...` for 19 of the 21 modules lib.rs already exports, so rustc compiled the whole ~160k-line compiler twice per build, once for the lib and once for the c17 binary. The other three binaries (cflow, ctags, cxref) already went through the library. Import the modules from posixutils_cc instead. Nothing in main.rs reached past the library's public surface, and seven of the nineteen modules were unused there, so they are dropped rather than imported. Cold `cargo build -p posixutils-cc`, dev profile, CARGO_INCREMENTAL=0: 42.0s -> 22.1s. The two ~22s units were serialized, because cargo makes a bin depend on its package's lib, and each held ~1GB RSS. `cargo test` also stops building and running the in-module test files twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
It is a mechanical, behavior-preserving refactor where every imported module is used, no dropped module is referenced, and the module/crate names align with lib.rs and Cargo.toml.
Review effort: Balanced
Findings: None
What changed in this PR
This PR fixes a build-time inefficiency in the cc crate. The c17 binary (cc/main.rs) previously declared its own mod abi; mod arch; ... for 19 of the 21 modules that lib.rs already exports, causing rustc to compile the entire ~160k-line compiler twice per build (once for posixutils_cc lib, once for the c17 binary). This change replaces those module declarations with use posixutils_cc::... imports so the binary links against the already-compiled library, matching how the cflow, ctags, and cxref binaries already work.
Changes:
- Replace 19
mod X;declarations inmain.rswith 12use posixutils_cc::X;imports. - Drop the 7 modules that
main.rsnever used (abi, builtin_headers, constexpr, float, kw, os, rtlib) rather than importing them. - Eliminates duplicate compilation (~42s → ~22s cold dev build) and stops running in-module tests twice.
| File | Description |
|---|---|
| cc/main.rs | Swaps per-module mod declarations for use posixutils_cc::* imports so the binary reuses the library build instead of recompiling all modules. |
Verification performed:
- All 12 imported modules (
arch,builtins,diag,ir,linkargs,opt,parse,strings,symbol,target,token,types) are still referenced inmain.rs, so no unused-import warnings are introduced. - None of the 7 dropped modules are referenced anywhere in
main.rs(thefloat/os/rtlibword matches are field accesses, flag strings, and locals, not module paths). lib.rsexports all 12 imported modules aspub mod, andCargo.tomlconfirms the lib name isposixutils_cc.- No
crate::references remain inmain.rsthat would break, and the#[cfg(test)] mod testsblock only exercises local driver functions viasuper::*.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`linearize_for` emitted `br cond_bb` into the current block and then linked the edge from `post_bb`. When the post-expression contains `&&`, `||` or `?:`, lowering it splits the block and leaves the current block on the merge, so the terminator and the recorded edge ended up in different blocks: `post_bb` claimed a successor it no longer branched to, and the merge block had one nothing recorded. The compiled loop then never terminated -- `for (int i = 0; i < n; (void)(n && 1), i++)` hangs. Link from the block the branch was emitted into, which is what every other loop lowering in the file already does, and emit nothing when there is no current block (the documented post-`goto` state). The switch-body walker carries its own copy of the `for` lowering and had the same defect. Tested on the CFG rather than on the program's answer, because with the defect present the program does not return a wrong value, it fails to return: a runtime test would hang the suite instead of failing it. The new `cfg_inconsistency` helper states the invariant -- a block's `children` are exactly its terminator's targets, and `parents` is their inverse -- over every position a lowering evaluates an expression before ending a block, and a meta-test asserts the helper rejects a misrecorded edge. The end-to-end test runs only now that the shape is known to be acyclic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng them
`group_array_init_elements` tracked an element cursor but never bounded it
by the array, and did not even receive the array's type -- only its element
type. An initializer past the last element therefore got a group, an offset
beyond the object, and a store: `int x[2] = {7, 8}; int a[2] = {1, 2, 3};`
read back `x = {3, 8}`, the excess 3 having landed on the neighbouring
local, and the file-scope form emitted a third `.long` under a two-element
symbol. C17 6.7.9p2 makes the excess element a constraint violation, which
c17 already diagnoses; this is where the diagnosis stops being advisory.
Take the array type, derive the element type from it, and drop any group
whose index is out of range. Both lowering paths and every caller -- static
globals, static locals, automatic locals, compound literals, nested levels
-- share this function, so one bound covers them all.
An absent or zero size means an array sized *by* its initializer (an
incomplete type, a flexible array member, a GNU zero-length array), so
nothing in the list can be excess and the bound does not apply. A range
designator is clamped rather than dropped, so `[0 ... 4] = 1` on an
`int[3]` still fills the three elements that exist.
The bound is on the index, not on the count: `int d[3] = {[2] = 3, 1};` has
three elements and two initializers, but the 1 resumes after `[2]` at index
3 (C17 6.7.9p17) and is excess. Counting would have kept it and written
past the array -- the very defect -- so the tests state that case
explicitly, with guard objects either side. Struct and union excess members
were already dropped correctly and needed no change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Opcode::has_side_effects` lists `Store` but not `Load`, and `dce::is_root`
was that predicate alone, so no load was ever a root and DCE deleted every
`volatile` read whose value goes unused. `volatile int g; void f(void) { g; }`
emitted the load at -O0 and nothing at all from -O1 up. Reading a volatile
object is observable behaviour (C17 5.1.2.3p6).
The qualifier could not be answered from the objects the IR already tracks.
`LocalVar::is_volatile` and `memloc::GlobalFacts::is_volatile` describe a
named object, and for `volatile int *p` there is no such object to ask: `p`
is an ordinary pointer and `*p` is the volatile one. So the marker goes on
the access -- `Instruction::is_volatile`, asked through `is_volatile_access`,
printed as a trailing `volatile` in a dump, and cleared by `kill` so a `Nop`
makes no stale claim. `ir/README.md` and a comment in `validate.rs` both
documented the old design as deliberate and are corrected.
`Linearizer::emit` sets it for every access the linearizer emits, from
`types.contains_volatile` of the type that access reaches. Being the one
chokepoint all of them pass through, that covers every `Instruction::load`
site without touching any of them, and a site that knows more than the
access type -- a bit-field's carrier, a composite copy's chunks -- can still
set the marker itself and have it preserved. `ir::build::Builder` does the
same for accesses a pass synthesizes, so no pass can introduce an unmarked
one. `Store` remains a root by its opcode alone: its correctness must not
come to depend on the marker.
Four other passes needed the same question asked, each having declined a
through-pointer volatile only incidentally, by resolving the base to
`Unknown` rather than by rule: `loadfwd` will not forward one, `dse` will
not delete one, `constglobal` will not answer one from an initializer, and
`ssa` will not promote the object it reaches -- `int a; *(volatile int *)&a;`
qualifies the access alone and was being rewritten into a register copy.
`instcombine`, `sccp`, `mem2reg`, `ifconv`, `inline`, `memexpand`, `lower`,
`propagate`, `constfold`, `vrp`, `escape` and `effects` were audited and
need nothing.
Covered on both targets at every -O level, with a plain read that must
still be removed as the negative control, and a volatile store pair that
passed before this change and must keep passing.
Still wrong, and not fixed here: a member read of a volatile aggregate is
dropped, because a member-access expression's type does not inherit the
parent object's qualifiers (C17 6.5.2.3p3/p4). That belongs in the parser's
type evaluation, not here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C17 6.5.2.3p3/p4 gives `s.m` and `p->m` the *so-qualified* version of the member's type: it inherits `const` and `volatile` from the object. c17 used the member's declared type unchanged, so a member of a qualified object was an ordinary one. Two consequences, both silent: volatile struct S vs; vs.a; /* load deleted from -O1 up */ const struct S cs; cs.a = 2; /* accepted; gcc and clang error */ The read is observable behaviour (5.1.2.3) and the write is a constraint violation (6.5.16p2). Neither `LocalVar::is_volatile` nor `GlobalFacts::is_volatile` could answer for it, because both describe a whole named object while the access is of one member. `TypeTable` gains the operation and its inverse: `qualifiers`, `qualified_with` (the so-qualified version) and `unqualified` (6.3.2.1p2, which `lvalue_converted_type` becomes a wrapper over). `qualified_with` implements 6.7.3p10 -- qualifying an array qualifies its *element* type -- without which `cs.arr[0] = 2` stays accepted, since the subscript reads the element type. Four open-coded copies of the qualifier list collapse into `Type::QUALIFIERS`, and `Type::MEMBER_QUALIFIERS` records why `_Atomic` does not travel to a member (gcc agrees, and `warn_atomic_member_access` already reports reaching into one) and why `restrict` cannot. The rule lives in `member_access_type`, beside `find_member` rather than in it: qualifying interns, which needs `&mut TypeTable`, and the linearizer holds `&TypeTable` -- so a `&mut` `find_member` would thread mutability through the whole IR. Both member arms go through the new function, and `find_member` now documents that its type is the *declared* one. Its other twelve callers want an offset, a size or a bit-field width, for which that is correct. Two sites the qualifier cannot reach on its own: - `emit_member_access` built the load from `find_member`'s type, so it kept the unqualified one however good the expression's type was. It now performs the access at the expression's type; width, sign and kind still come from the member. - Bit-fields access their *carrier*, which can never hold the qualifier. `mark_volatile_access` already anticipated this and preserves a marker a site sets itself; neither emitter set one, so a `volatile` bit-field read was deleted at -O1 in both spellings. `emit_bitfield_store` now takes the field type so the read-modify-write marks both halves. `is_pure_expr` asked only whether a member's *base* was pure, so `c ? s.status : s.other` loaded both members unconditionally into a branchless select, at -O0 too -- 6.5.15p4 evaluates one arm. Its `Ident` arm had the same blind spot one level up, asking the top-level modifier where a struct with a volatile member needs `contains_volatile`; `constglobal`'s gate is respelled the same way, where it was unreachable but inconsistent. Making member types qualified would otherwise have leaked the qualifier into rvalue types, because `common_type` and `integer_promote` answer with an operand's own id and `volatile int + int` came out `volatile int`. The parser's three arithmetic chokepoints now strip it (6.3.2.1p2), which keeps the qualifier on exactly the lvalues that should carry it. Nothing in the test corpus asserted a write through a const-qualified aggregate, so no existing expectation changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Linearizer::current_bb` is `None` wherever control cannot arrive -- after a
`goto`, and in a `switch` body before the first `case`. Both are valid C that
has to be translated, and 21 sites across 8 functions took
`.unwrap()`/`.expect()` on it instead of `current_or_unreachable_bb()`, so
each turned a legal program into an internal compiler error:
switch (x) { g() ? g() : g(); case 1: return 1; }
int y = x ? ({ goto L; g(); }) : g(); L: return y;
There were 21 of them because the block plumbing was copied: four lowerings
-- both ternaries and both spellings of the GNU elvis operator -- each had
their own `cbr`, two arm blocks, two `br merge`s, four `link_bb` calls and a
phi of two `emit_phi_source`s, and complex integer division had a fifth copy
without the phi. `emit_two_way` was already a generic builder using the safe
accessor, with one caller.
So the fix is the helper, not the unwraps. `emit_fork` is now the one place
those blocks and edges are built, with `emit_diamond` (phi) and
`emit_diamond_void` (arms that write where the caller can find them) over
it. The four lowerings become one call each and `linearize.rs` loses ~150
lines of plumbing. Both the block the branch leaves *and* the block each arm
ends in are read back through the accessor, because an arm is arbitrary code
that may itself `goto` away.
`emit_diamond` takes the phi width rather than deriving it from the type: a
complex conditional merges addresses, so its phi is pointer-wide over a
pointer type, and a function designator's `size_bits` is 0 where the merge
wants 64.
The short-circuit operators keep their own lowering. They are triangles, not
diamonds: only one arm block exists, the other phi predecessor is the left
operand's own block and its phi value is emitted before the branch, and they
go through `branch_on`, whose constant case emits a plain `Br` and elides
the merge edge -- which is how `1 && g()` compiles to no branch at all.
Routing them through a builder that takes a condition pseudo would
manufacture an empty second arm and a dead `cbr`. They take the accessor
directly, and a test pins the elision so the tempting consolidation fails
loudly. The atomic CAS retry loop is not a diamond either, for the same kind
of reason.
Site accounting: 12 retired by `emit_diamond`, 3 by `emit_diamond_void`, 6
by the accessor. `current_bb` with `.unwrap()` or `.expect()` now appears
only in doc comments.
The new CFG audit runs `cfg_inconsistency` over 13 conditional shapes in
each of three placements -- reachable, before the first `case`, and after a
`goto` -- against post-`remove_unreachable_blocks` CFGs, where a mislinked
edge into a discarded block would outlive the block.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`memexpand::block_chunks` descends 8/4/2/1 and lands the tail exactly, and
`BlockOp::limit` bounds a copy or a fill at `INLINE_LIMIT_BYTES`. Three
places in the IR wrote that descent out again instead, and each lost
something in the copy: two rounded the size *up*, one had no bound and no
capacity clamp. `memexpand`'s own header records the incident this rule came
from, and `codegen_struct_copy_across_the_inline_threshold` records the two
bugs that came out of fixing it -- one of which was a second copy of the
loop, written as `while offset < size` stepping 8. These are the third,
fourth and fifth copies.
`emit_aggregate_zero` had no cap, so `char buf[N] = {0}` emitted one store
per chunk for every N: 8 KB cost 2081 instructions and 1 MB did not finish
compiling in twenty-five minutes, while the sibling two functions away had
capped all along. It now asks for a `Memset` and lets `memexpand` choose
stores or a call, which deletes the ladder rather than bounding it -- the
bound, the expansion and the fill-byte handling are all already written
there, and `memexpand::run` executes at -O0 too, so nothing is conditional
on optimizing. 1 MB now compiles in under a second. The `rvalue_addr`
handling that a `Sym` needs before it can be passed to a libc call came out
of `emit_block_copy_call` into `block_dest_addr` rather than being written a
second time.
The inliner's implicit-parameter copy stepped 8 regardless of width, so a
12-byte struct moved 16 bytes -- over-reading the argument and over-writing
the callee's local. It takes `block_chunks` now. No bound is needed and a
comment says why: the copy is at most 32 bytes, being a `long double
_Complex` at worst. The two-register return stored its high half at a
hardcoded 64 bits where the half is 1..8, and now agrees with the load in
`emit_two_reg_return`, which was already narrowing correctly.
`store_string_units` handles every literal kind, clamps to the destination
and strides by the element width. The nested-array element path did none of
those: it accepted all four kinds and then handled only the narrow one,
dropping a wide element silently; it stepped the destination by raw bytes
where a wide element is two or four; and with no clamp at all
`char s[1][3] = {"hello"}` stored five bytes into three, two of them past
the local. Routing it through `store_string_units` retires all three
together with the copy.
That left the tail. C17 6.7.9p21 zeroes what an initializer does not reach,
which the `InitList` arm did by calling `emit_aggregate_zero` first and the
bare-string arm did not -- so `char buf[8] = "hi"` wrote three bytes and
left five, visible only on re-execution because the prologue zeroes the
frame once. `store_string_units` now fills from the terminator to the end,
under a `StringTail` the caller passes: the three sites reached from an
initializer list have already been zeroed and say so, so no stores are
doubled at -O0. It fills through the same bounded `Memset`, which is why
this had to follow the cap and not precede it.
`test_compound_literal_zero_init_lvalue` counted the zero-init's stores in
raw linearizer output, which is the shape that moved. It now asserts the
linearizer asks for the `memset`, runs `memexpand`, and holds the stores to
the same account, so it still proves 6.7.8p21.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The back ends carried five more copies of the chunk loop the IR fixed one
commit ago. They cannot call `memcpy` -- they are past the point a call can
be synthesized -- but the rule is the same, and each copy lost the same
things: three rounded a width up, one had no bound.
A struct classified into two eightbytes has a high half narrower than eight
whenever its size is not a multiple of eight. The SSE parameter prologue
stored both halves at `FpSize::Double`, so `struct P { float x, y, z; }`
wrote four bytes past itself; the general register-pair prologue did the
same for `struct { int a, b, c; }`; and `copy_incoming_arg_to_local` stepped
eight regardless of width. All three now ask `eightbyte_bytes` what is
actually left and store that -- `movsd` then `movss`, and a packed
`struct { float a, b, c; _Float16 d; }` comes out `movsd`/`movl`/`movw`,
fourteen bytes exactly.
`va_arg` of a large aggregate had no limit at all, so fetching four kilobytes
cost 1081 instructions on x86-64 and 1107 on aarch64, linear in the object.
Past `INLINE_LIMIT_BYTES` -- the same constant the IR bounds a copy with --
x86-64 moves the whole eightbytes with `rep movsq` and unrolls the ragged
tail, and aarch64 runs a counted loop sixteen bytes at a time through `q16`.
Neither primitive is new: `call.rs` already had the `rep movsq` for stacked
arguments, generalized here rather than written twice, and the aarch64 loop
sits beside `emit_zero_loop` and is shaped like it. The `q` register is what
makes the loop fit in the three general registers the `va_arg` sequence has
free. 33 and 36 instructions now.
The stacked-argument copy rounded its *source* read up. The destination
round-up is correct -- the outgoing area is allocated in whole eightbytes --
but the source is the object, and a 12-byte struct read bytes 8..15 where
four of them belong to whatever follows. It faults if the object ends a
page, which is the hazard `load_object_bytes` in the same file was already
fixed for. The `rep movsq` path had it too, reading `ceil(bytes/8)` qwords:
seven bytes past a 4095-byte object.
Four literal `[8, 4, 2, 1]` ladders -- transliterations of `block_chunks`'s
body -- are gone with them, and `grep` for that literal across `cc/` now
finds nothing.
The aarch64 stacked-composite and spilled-composite paths were suspected of
being unbounded and are not: AAPCS64 B.4 replaces a composite above sixteen
bytes with a pointer to the caller's copy, so both are bounded by the ABI at
two eightbytes and a 4 KB argument is two `memcpy` calls and a pointer. They
are deduped here, not fixed.
These overruns land in slot padding rather than in a neighbouring object,
because a slot is `slot_bytes(size).max(8)` at eight-byte alignment. So the
behavioural test passes either way and the assembly assertions are what pin
the widths -- which is why both are here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A VLA's storage is released by restoring the stack pointer its scope saved. The declaration scope and the VLA scope were two stacks maintained by hand, and the second was opened at three of the five places that open the first -- so a VLA declared in a `for` init clause or a statement expression was allocated and never released. The switch body had the mirror defect: a VLA scope with no declaration scope, so the declarations in it leaked out. `push_scope` now returns a `Scope` that `pop_scope` consumes, carrying the VLA depth to release. `open_vla_scope` is gone and `close_vla_scope` is private to `pop_scope`, so there is no way to enter one scope without the other and nothing left to keep in step. The `for` lowering existed twice -- the ordinary walk and the switch-body walk, differing only in how the body is lowered -- and is now `open_for`/`close_for`, which retires one of the five sites outright rather than pairing it. That duplication has now caused three separate defects in this file. The unclosed scope also poisoned forward `goto`: `place_label` records a label's depth as `vla_marks.len()`, so a scope that never closed left every later label's depth too high and `resolve_forward_goto_vla_restores` skipped the restore for the rest of the function. A computed `goto` released nothing at all, where the plain `goto` beside it had both backward and forward handling; both now go through one `release_vla_scopes_for_goto`, and an indirect jump takes the deepest depth over the address-taken labels, which is the only one that cannot free storage still live at another candidate. An `asm goto` has two exits and released on one: the label-edge block now releases before its branch, and is allocated when a VLA is open and not only when there are outputs to write back. `push_vla_mark` reading `break_depth` before the target is pushed is not a defect and is now documented as load-bearing. C17 6.8.5.3 makes the scope of a `for` init declaration the entire loop, so a VLA declared there is allocated once outside it and its scope encloses the exit: `break` lands inside that scope and must not release it, and `continue` must not either, the storage being live next iteration. Recording the depth first is what makes the unwind skip it, and two tests pin that. No program leaks stack today: c17 always keeps a frame pointer and the backend resets `%rsp` from it, which masks the imbalance -- 400,000 iterations of every shape here, with sizes varying per iteration, exhaust nothing at -O0 or -O2. The tests therefore assert the released stack, by counting saves against restores in the IR and by observing that a repeated declaration on a repeated path keeps its address. None asserts a crash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…laps
C17 6.7.9p19 replaces a previously listed initializer for the *same*
subobject and leaves the others alone. The static path merged its field
initializers by byte span, so any intersection dropped the earlier entry
whole: `{ .t = {1,2}, .t.y = 9 }` lost the 1 with the 2. It also sorted by
address before resolving, which read "later" as later in the object rather
than later in the list, so in `{ .z = 7, .t.y = 9, .t = {1,2} }` the
whole-field initializer that is written last was the one discarded. The sort
is still needed downstream by the bit-field emitter and now runs after the
merge.
The automatic path had neither problem -- it stores in list order and lets a
later store land on an earlier one -- but that is also why it was wrong for
unions, where a union holds one member at a time: `{ .u.i = 0x01020304,
.u.s.b = 9 }` must leave the union holding `.u.s` with the rest zero, and
storing one byte over the int kept three of its bytes. So the two paths
disagreed in *both* directions and neither could simply adopt the other.
Byte spans cannot tell the two cases apart -- `.t.y` inside `.t` and
`.u.s.b` inside `.u.i` are both one span inside another -- so
`classify_subobject` walks the type instead and answers `Member`,
`ThroughUnion` or `NotASubobject`. The static merge folds a contained
initializer into the earlier one through the `Initializer` tree for a
member, replaces the union's subtree for a union, and keeps the old drop for
anything it cannot express. The automatic path keeps its stores and gains
the same question: before each store it clears the bytes a reset requires,
which is the entry's own span plus, per earlier overlapping entry, the whole
of one it contains and only the union's bytes for one it reaches through a
union.
A structured merge rather than a byte map, because the `Initializer` tree
carries `SymAddr` relocations that cannot be split into bytes.
Two more automatic-path defects fall out, both now matching gcc in each
storage duration: a whole-field initializer repeated (`{ .t = {1,2},
.t = {5} }`) left the old field's tail, and a union initialized twice by
member kept the first member's bytes.
Every case is asserted in both storage durations, since a disagreement
between them is how all of this survived.
Two shapes stay at the old behaviour and are not fixed here. An overlap
where either side is a bit-field keeps the drop, because carrier bytes are
merged downstream of the `Initializer` tree and cannot be folded into it
without the bit-field emitter taking part. And an override naming a
subobject of the member a union already holds resets the union, because the
tree records no discriminant and the fold cannot tell "same member, deeper"
from "different member".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C17 6.5.16.2p3 makes `E1 op= E2` mean `E1 = E1 op E2` bar evaluating `E1` once, so the arithmetic happens at the type the usual arithmetic conversions give the operands and only the result converts back. `emit_assign` already did that, with a comment recording the bug it came from; the atomic path had its own copy of the logic, narrowed the right operand to the target first and computed there. Its comment -- "the ordinary path does this after the point we branched from, so it has to be repeated" -- is the duplication saying so. `_Atomic unsigned char c = 50; c /= -5` stored 0 where the same ordinary object stored 246, and `m %= -3` stored 50 against 2. The expression's value had the second half of it: recomputed as a raw binop from the old value with no conversion back, so `_Atomic _Bool b = 0; r = (b -= 1)` stored 1 and handed back 255. An assignment expression has the value of the left operand after the assignment (6.5.16p3), and the ordinary path gives 1. Both are one extraction. `CompoundAssign` describes the assignment and `compound_assign_value` performs it -- choose the arithmetic type, pick the opcode, convert the operands, compute, convert back -- and `emit_assign`, the CAS loop, `try_emit_atomic_assign`, the increment forms and the GNU `__atomic_*_fetch` builtins all go through it. The type and opcode decisions are pure functions over the type table, which is what lets them be tested without a linearizer, and the atomic path's separate opcode table is gone. A descriptor rather than loose arguments because NAND needs one more bit: complementing has to happen before the conversion back, or `__atomic_fetch_nand` on an `_Atomic _Bool` stores 255. With that, `emit_atomic_nand` is no longer a second entry point -- NAND is `&=` with `invert` -- and the `_Bool` fixups the CAS loop and the increment path each carried are subsumed by the shared conversion. `emit_atomic_rmw`'s `needs_conversion` gate becomes `native_rmw_opcode`, which states the condition once: a native fetch-and-op equals the standard only when the operator is congruent modulo 2^n and the conversion back is the truncation congruence permits. `_Bool`, NAND, divide, remainder, the shifts and the floating forms all fail that and take the CAS loop, which they already did. Add, subtract and the bitwise operators keep their native lowering. The shifts are the trap in sharing this: 6.5.7p3 promotes each operand and the result has the promoted *left* operand's type, so the arithmetic type is `integer_promote(target)` and not the common type, and the right operand is promoted on its own. `_Atomic signed char s = -8; s >>= 1` is -4. Both rules came over from the ordinary path unchanged, and a test pins them. Two more defects fall out. `__atomic_add_fetch` on an `_Atomic _Bool` returned the raw arithmetic where it should return what it stored, and `__atomic_fetch_add` on a floating object selected the integer atomic add -- an integer add of float bits -- where it now selects the floating opcode and takes the CAS loop. Neither was covered by a test. clang answers 255 for the `_Bool` expression and 124 for the shift. c17 follows the standard and its own non-atomic path, which is the same answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C17 6.8.4.2p5 converts each case constant to the promoted type of the
controlling expression, and p3 forbids two of them having the same value
after that conversion. c17 evaluated labels at full width and never
converted them -- a comment in the collector said so deliberately -- so only
their *ordering* knew the switch's type, through the `unsigned` flag.
That left the two lowerings disagreeing about the same switch. With a
runtime selector the label went into the `switch` instruction as written;
with a constant selector the fast path compared at 128 bits with a signed
range test that ignored signedness. `switch (x) { case 4294967296LL: }`
matched `x == 0`, and the same switch on a constant 0 did not. `case -1:`
never matched a `switch` on `unsigned`.
`CaseConv` is the promoted type -- a width and a signedness -- and owns the
conversion, the ordering and the range test, so the fast path, the wide
comparison chain and the recorded ranges all ask one thing. `CaseSet` holds
it and converts on insert and on overlap, so only converted values are ever
in the set.
The label is looked up twice, and that is the hazard: the collector records
it, and the body walk finds its block by the same `(lo, hi)` key. Convert
one and not the other and the lookup misses -- no block for the case, its
body emitted into another one, no diagnostic. So `CaseIndex` is built from
the set and carries the set's own conv, and `lookup` is the only way into
the map. The walk hands over the label's raw constants and cannot apply a
different rule, there being no other rule reachable from it.
Converting also makes the existing duplicate-case error see collisions it
could not before: `case 0:` beside `case 4294967296LL:` in an `int` switch
is one value twice.
A lossy conversion is now diagnosed, as gcc and clang do, by converting to
the controlling type and back through the label's own type and warning when
the constant does not return. A value that is merely reinterpreted round-
trips, so `case -1:` in an `unsigned` switch stays silent, and so does a
label already of the promoted type in a `short` switch.
Nothing in the suite was relying on a label outside its controlling type, so
no existing expectation changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`range.rs` states the rule: c17 does not assume undefined behaviour away, because that makes -O0 and -O2 disagree about a program which really does divide by zero. `eval_divmod` honoured it for a zero divisor and then folded the other trap the same instruction raises. On x86-64 `idiv` raises #DE for a zero divisor and for `INT_MIN / -1`, whose quotient is not representable, and the remainder form traps identically because it is the same instruction. So `int a = INT_MIN, b = -1; a / b` faulted at -O0 and printed -2147483648 at -O2, `a % b` printed 0, and `0 / x` with `x` zero at run time printed 0 where -O0 faulted. The rule was stated three different ways, which is how it drifted: `eval_divmod` knew both values and asked about one of them, `range` knows sets and asks whether one contains zero, and `instcombine`'s algebraic arms knew one operand and did not reason about the other at all. It is now one predicate over optional operands -- unknown meaning "may trap" -- in `constfold.rs`, which is where the README already says these rules live. `eval_divmod` asks it and folds nothing when the answer is yes, which covers `instcombine`'s constant folding, `sccp` and `vrp` together, all three reaching it through `eval_binop`. `simplify_div` and `simplify_mod` ask it before their algebraic arms; that makes `0 / x` and `0 % x` unreachable, so those arms are deleted rather than left behind a new guard -- `0 / c` for a known `c` is ordinary folding, and an unknown divisor is what the predicate refuses. `x / 1` and `x % 1` cannot trap and still fold. `range`'s `contains(0)` refusal is unchanged and now says it is the same question lifted from a value to a set. gcc and clang fold all of these. This is a deliberate divergence, and the predicate says so where someone would otherwise assume a bug-for-bug match was intended. `Lsr` at 128 bits folded as an *arithmetic* shift, being correct at narrower widths only because the width-narrowing helper had already cleared the high half, and at 128 it returns the value unchanged. It shifts the unsigned view now, so it is the logical shift at every width. This is hardening rather than a repair: `arch::mapping` runs before the optimizer and either expands a 128-bit shift with a constant count into 64-bit halves or rewrites the count into a `pair64` the constant map cannot answer, so no foldable `lsr.128` reaches the optimizer by either route. A test pins the arithmetic anyway, and the old code fails it. The tests assert that the division is still emitted rather than that the program faults: the property is that it survives to run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… address
Two places decide whether a register-returned aggregate's `Ret` carries the
value or the address of the callee's copy, and they disagreed.
`emit_two_reg_return` emits the address form for three classes -- x87, an
HFA, and one SSE register at sixteen bytes -- while `returns_addr_aggregate`,
which sets the flag telling the inliner not to splice across that boundary,
named only the first two. So `struct { __float128 a; }` emitted an address
and was reported as carrying a value, and the inliner fed the `symaddr`
straight into the phi:
%16 = symaddr.64 %11(@mk_inline0_r.2)
%17 = phisrc.128 %16
The caller then stored those eight bytes into a sixteen-byte slot and read
the other eight from wherever the frame happened to leave them, so inlining
changed the answer: `leaq -96(%rbp), %rax; movq %r10, -64(%rbp);
movq -56(%rbp), %rax` where the un-inlined call had `movups %xmm0,
-64(%rbp)`. It is the two-register path's neighbour, and both are the same
idea -- the callee returns an aggregate in registers, so the caller's result
local receives its bytes -- so the inliner gains that third branch and
copies through `block_chunks` rather than phi-ing anything.
`aggregate_ret_is_address` is now the one statement of which classes those
are, including the size bound that keeps an eight-byte `struct { float a,
float b; }` on the ordinary value path, and both sites ask it.
Nothing else at sixteen bytes in one register was affected: a union of
`__float128`, a nested struct holding one, a sixteen-byte float vector,
`__int128`, `struct { double, double }`, `struct { float[4] }`,
`struct { double[3] }`, `struct { long double }` and
`struct { float, float }` all splice their value. `_Decimal128` is not
implemented.
The shape cannot be built for Darwin, where `__float128` is rejected, which
is why nothing covered it. The test compiles for an explicit target, reads
the optimized IR, and asserts no phi source is a pseudo that a `symaddr`
defined -- a phi source is a value and an address is not. Its companion
keeps the callee inlined, so the fix cannot be to stop inlining the shape.
Inlining is still refused for x87 and HFA returns. Lifting it was measured:
sixteen-byte HFAs splice correctly, but a twenty-four or thirty-two byte one
still phis its address, because `returns_reg_aggregate` is capped at 128
bits at the definition site and those returns carry no classification for
the new branch to read. Attaching it to every register-returned aggregate is
a separate change, and the comment at the refusal records that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two shapes 33ff4e0 left at the old behaviour, both of which disagreed with gcc and one of which still had the static and automatic paths disagreeing with each other -- the defect class that commit set out to end. A bit-field's bytes are merged after the `Initializer` tree is built, so the static merge had nothing to descend into and dropped the earlier entry whole: `{ .t = {1,2}, .t.a = 3 }` lost `b`, while the automatic path overlaid the carrier and kept it. Nothing needs un-lowering, as it turns out -- by the time an entry reaches the merge a nested struct's bit-fields are already one byte apiece in its initializer, so the carrier *is* those bytes. `classify_subobject`'s refusal for a carrier window becomes an answer, `BitfieldCarrier`, naming the struct that declares the field, and the fold masks the field's bits out of the byte and the new ones in. The classifier and its bit-field form now share one descent, differing only in whether the range is an object or a span of bits: for bits an exact byte match is no reason to stop, since two fields can share one byte. `bitfield_carrier_bytes` is the one definition of that arithmetic, and the emitter calls it rather than repeating it. A union holds one member, so an override naming a member *of that member* keeps the rest of it: `{ .u = {1,2}, .u.p.y = 9 }` leaves `p.x` at 1. c17 reset the union, because the tree records no discriminant and the merge could not tell "the same member, deeper" from "a different member". It is threaded now rather than guessed: the designator records the member it crosses, an initializer-list walk records the member an earlier entry gave a value to, and a union is descended only where the two agree. Both paths take the discriminant from the same walk, and the reset case -- a *different* member named -- is unchanged, including for a union whose members have identical layouts, where no shape guess could have told them apart. Two more, found while probing and of the same kind: a whole-struct initializer after a bit-field one did not supersede it, and a bit-field initializer displacing a union did not reset it. So the paths agree by construction and not by coincidence: they share the classifier, the discriminant walk and the field walk, and differ only in whether they fold a tree or clear bytes before storing. Not fixed, and not this: an eight-byte local's 32-bit store at offset 0 is widened to 64 in the x86-64 store lowering, because the guard that spares a struct asks for more than 64 bits, so `struct S { struct P t; } l = { .t = {1,2}, .t.x = 3 }` reads back y as 0 at -O0. The tests here use objects wider than eight bytes deliberately, so that they test 6.7.9p19 and not that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 32-bit store at offset 0 of a local is widened to 64 bits so a narrow
value going into a wider slot leaves no stale upper bits behind it. The
comment on it already recorded the exception that needs -- "struct/union
fields at offset 0 must use exact size to avoid clobbering the adjacent
field at offset 4" -- and the exception asked whether the object was larger
than 64 bits. An eight-byte aggregate is not, so exactly the case the
comment describes fell through it: `struct S { struct P t; }` initialized
`{ .t = {1,2}, .t.x = 3 }` read `y` back as 0 at -O0.
No size test can separate them, which is why this one only spared the
aggregates too big to be confused with a scalar in the first place: a `long`
and a `struct { int x, y; }` are both sixty-four bits. So the map the store
lowering consults records what it actually needs -- the width, and whether
the slot holds a single scalar -- taken from the local's type where the map
is built. A complex is excluded with the aggregates: `_Complex float` is
sixty-four bits with its imaginary half at offset 4, the same shape, and
declining to widen can never leave the stale bits the widening exists to
clear. A slot with no record keeps the widening, as before.
Only -O0 reached it. With the optimizer on, the field is promoted out of
memory before the store lowering sees it, so the tests name -O0 as well as
running the default matrix.
The other shapes were already right and are pinned here too: a plain field
assignment, a store through a pointer, a union member, an `int[2]`, and a
four-`short` struct. So is the reason the widening is there, each case
writing a wide pattern into the slot before overwriting it narrowly, since a
fix that merely stopped widening would pass everything else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught this on aarch64: 87489eb fixed the widening test in the x86-64 store lowering and left the identical one in aarch64, where an eight-byte aggregate is still not larger than sixty-four bits and its second field is still written over. The behavioural test failed there and passes here only because this host has no aarch64 runner, so every aarch64 behavioural test skips locally. Both back ends had grown the same rule and both had got it wrong the same way, so it is no longer written twice. `SymSlot` and the walk that builds it move to the shared code: the width, and whether the slot holds a single scalar, which is the question a widening actually turns on. `widenable()` answers it, and both lowerings ask. What each does with an *unrecorded* symbol is left as it was -- x86-64 widens, aarch64 keeps the exact width -- because that is a separate judgment about globals and stores through pointers, not the one being corrected here, and changing it quietly while fixing something else is how the two drifted apart in the first place. Verified on aarch64 by the emitted code: a field store is `str w` where it was `str x`, and a narrow value into a `long` slot is still `str x`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
main.rs declared its own
mod abi; mod arch; mod ir; ...for 19 of the 21 modules lib.rs already exports, so rustc compiled the whole ~160k-line compiler twice per build, once for the lib and once for the c17 binary. The other three binaries (cflow, ctags, cxref) already went through the library.Import the modules from posixutils_cc instead. Nothing in main.rs reached past the library's public surface, and seven of the nineteen modules were unused there, so they are dropped rather than imported.
Cold
cargo build -p posixutils-cc, dev profile, CARGO_INCREMENTAL=0: 42.0s -> 22.1s. The two ~22s units were serialized, because cargo makes a bin depend on its package's lib, and each held ~1GB RSS.cargo testalso stops building and running the in-module test files twice.