Skip to content

Ptr#1469

Open
sbillig wants to merge 3 commits into
argotorg:masterfrom
sbillig:ptr
Open

Ptr#1469
sbillig wants to merge 3 commits into
argotorg:masterfrom
sbillig:ptr

Conversation

@sbillig

@sbillig sbillig commented May 23, 2026

Copy link
Copy Markdown
Collaborator

Diff line count is mostly due to the treesitter grammar update.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42ce9fb159

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/mir/src/runtime/lower/body.rs Outdated
@sbillig sbillig marked this pull request as draft May 23, 2026 23:15
@sbillig sbillig marked this pull request as ready for review May 26, 2026 14:49

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce37ed5675

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +654 to +660
if root_local.is_some_and(|local| {
self.body
.locals
.get(local.index())
.is_some_and(|local| local.ty.as_borrow(self.db).is_some())
}) {
current

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve non-root deref projections in place classification

This Projection::Deref branch skips dereferencing whenever the root local is a borrow, but it does so for every deref in the projection path instead of only the root-level borrow deref. For places like Deref -> Field -> Deref -> ... (e.g. accessing through a pointer field of a borrowed root), the second deref is incorrectly ignored, so later projection is computed against the wrong runtime class and can mis-lower or panic on invalid class projections. The skip needs to be limited to the initial borrow-root deref, not all derefs.

Useful? React with 👍 / 👎.

Comment thread crates/hir/src/analysis/semantic/borrowck/check.rs Outdated
@sbillig

sbillig commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

On hold till after 26.2 release.

@micahscopes micahscopes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agent:

One borrowck question without a single line to hang it on: callee analysis
seeds pointer params as pairwise-disjoint pointees, but I couldn't find
anywhere that rejects f(p, p) — no check at the call site, no diagnostic in
the callee. Is the no-alias contract meant to be checked, modeled as may-alias,
or documented as a soundness precondition?

Comment on lines +462 to +464
if dst_ty.as_borrow(self.db).is_some() || !is_pointer_bearing_type(self.db, dst_ty) {
return Ok(());
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agent:

If I'm reading this right, a call whose result isn't pointer-bearing exits here,
and since summaries only describe the return value, callee writes through mut
args never invalidate caller facts — fn set(h: mut Holder, p: *u256) { h.ptr = p }
leaves the caller holding a stale strong fact, and the later mut/mut conflict on
*h.ptr goes undetected (strong_lookup's exact hit suppresses the Unknown
fallback). Is havoc on arg-reachable slots planned? Similar staleness shows up
with >32-elem arrays: a whole-array overwrite at [Any] doesn't clear an earlier
arr[5] fact.

Comment thread ingots/core/src/ptr.fe
impl<T> MemArray<T> {
#[arithmetic(unchecked)]
pub fn new_uninit(_ len: u256) -> Self {
Self::from_raw_parts(ptr: cast<u8, T>(alloc_raw(len * size_of<T>())), len: len)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agent:

len * size_of<T>() wraps here (fn is #[arithmetic(unchecked)]), so len ≥ 2^251
gives a tiny allocation while the stored len keeps the Index bounds check happy —
OOB writes through the safe API. byte_len() is checked; probably wants a checked
multiply here too.

@micahscopes

Copy link
Copy Markdown
Collaborator

agent:

One more from the same family as the call-havoc question: the Store handler
early-returns when the source type isn't pointer-bearing (canon.rs:847-850),
deciding from the value type alone that pointer slots can't be affected. With
ptr-to-ptr casts (core::ptr::cast does this generically) the written region
can overlap a pointer slot of a different type:
let raw = core::ptr::cast<Holder, u256>(h); *raw = 123 physically overwrites
h.ptr while its strong fact stays live — and slot keys are typed-path-based, so
the punned view never collides with the original's key either. Punning is
misuse territory, but if the hardening goal is that laundering degrades to
Unknown, a store through a cast-derived view should probably havoc the pointer
slots under the written roots rather than keep stale precise facts.

@micahscopes

Copy link
Copy Markdown
Collaborator

agent:

Possible ICE on the lowering side: when a Deref projection hits a class with no
deref_target() — i.e. RawAddr { target: None }, every pointer-to-scalar —
both body.rs:4428-4431 and classify.rs:653-668 fall back to the root local's
pointee type. For struct S { p: *u256 }, the place s + [Field(p), Deref]
has a non-pointer root, so the fallback returns None and classify's
expect("invalid deref projection class") panics; for pp: **u256, **pp
reuses the first-level pointee for the second deref (right mload chain by
accident, wrong class label). Might be unreachable depending on how HIR
normalizes these shapes — no fixture covers either.

@micahscopes

Copy link
Copy Markdown
Collaborator

agent:

Two policy-level things. ty_is_noesc returns false for *T (and for
aggregates containing only pointers), so noesc's check_store permits storing
raw pointers / MemArray into storage and transient — memory addresses that
persist past the transaction and dangle on the next one. If pointer-in-storage
is intended (transient scratch?), worth stating; otherwise pointers should
probably be noesc. Separately, the parser change making infix * terminate at
a line break (parser/expr.rs:378-385) silently reparses previously-valid
multi-line multiplications — a newline * b was a * b, now a followed by
a deref statement. Was a corpus check done for that pattern?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants