Skip to content

Conversation

@kulst
Copy link

@kulst kulst commented Jan 1, 2026

Some targets (for now only NVPTX) do not support cycles in static initializers (see #146787). LLVM produces an error when attempting to codegen such constructs (like self referential structs).

This PR attempts to instead error on Rust side before reaching codegen to not produce LLVM UB.

This is achieved by computing a new query in rustc_const_eval. It is executed as a required analysis depending on a new flag in TargetOptions. The check

  1. organizes all local static items in a DirectedGraph where pointers / references to other local static items represent the edges
  2. calculates the strongly connected components (SCCs) of the graph
  3. checks for cycles (more than one node in a SCC)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 1, 2026
@kulst
Copy link
Author

kulst commented Jan 1, 2026

@rustbot label: +O-NVPTX

@rustbot rustbot added the O-NVPTX Target: the NVPTX LLVM backend for running rust on GPUs, https://llvm.org/docs/NVPTXUsage.html label Jan 1, 2026
@rust-log-analyzer

This comment has been minimized.

…this

Some targets (for now only NVPTX) do not support cycles in static initializers (see rust-lang#146787). LLVM produces an error when attempting to codegen such constructs (like self referential structs).

This PR attempts to instead error on Rust side before reaching codegen to not produce LLVM UB.

This is achieved by computing a new query in rustc_const_eval. It is executed as a required analysis depending on a new flag in TargetOptions. The check

1. organizes all local static items in a DirectedGraph where pointers / references to other local static items represent the edges
2. calculates the strongly connected components (SCCs) of the graph
3. checks for cycles (more than one node in a SCC)
@kulst kulst force-pushed the check_static_initializer_acyclic branch from 10f31cc to 18143a8 Compare January 1, 2026 20:53
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Haven't checked the logic closely yet.

View changes since this review

link_self_contained: LinkSelfContainedDefault::True,

// Static initializers must not have cycles on this target
requires_static_initializer_acyclic: true,
Copy link
Member

Choose a reason for hiding this comment

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

the word ordering is funny here. how about instead

Suggested change
requires_static_initializer_acyclic: true,
static_initializer_must_be_acyclic: true,

});
});

// NEW: target-gated pre-codegen error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// NEW: target-gated pre-codegen error

Comment on lines +44 to +50
let mut statics: Vec<LocalDefId> = Vec::new();
for item_id in tcx.hir_free_items() {
let item = tcx.hir_item(item_id);
if matches!(item.kind, hir::ItemKind::Static(..)) {
statics.push(item.owner_id.def_id);
}
}
Copy link
Member

@workingjubilee workingjubilee Jan 1, 2026

Choose a reason for hiding this comment

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

We don't mutate this after this part, right?

This can be something like

Suggested change
let mut statics: Vec<LocalDefId> = Vec::new();
for item_id in tcx.hir_free_items() {
let item = tcx.hir_item(item_id);
if matches!(item.kind, hir::ItemKind::Static(..)) {
statics.push(item.owner_id.def_id);
}
}
let statics: Vec<LocalDefId> = tcx.hir_free_items().filter_map(|item_id| {
let item = tcx.hir_item(item_id);
match item.kind {
hir::ItemKind::Static(..) => Some(item.owner_id.def_id),
_ => None,
}
}).collect();

Code that ends mut being available early is easier to read as you can stop scanning for mutation points in the function, even if it's a little more annoying to edit.

This general note applies to the rest of the file also.

@kulst
Copy link
Author

kulst commented Jan 1, 2026

Thank you for the reviews so far! I will address them over the next few days.

I am happy to receive comments and suggestions, especially since this is my first larger contribution to the compiler.

This is still an early version, but I wanted to ensure that my implementation does not negatively affect other components of the compiler. I also wanted to gather feedback on the general approach and the optimal location for this analysis within the code. I plan to add more tests and documentation as well.

On that note, would documenting this restriction inside the target description be sufficient? Is there a place to document nonconformity in general?

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Yes, I think that putting it in the platform support documentation would be fine. Or at least, I'm not aware of a better place at the moment.

View changes since this review

Comment on lines +99 to +105
if nodes.is_empty() {
continue;
}

let is_cycle = nodes.len() > 1
|| (nodes.len() == 1 && graph.successors(nodes[0]).any(|x| x == nodes[0]));
if !is_cycle {
Copy link
Member

Choose a reason for hiding this comment

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

Here we have three different cases: 0, 1, 2..

When we have exhausted all possibilities in a range like this, it's nice to make that clearly visible instead of requiring close attention to the particulars of each sub-expression.

That could be achieved by something like

    let acyclic = match nodes.len() {
        0 => true,
        1 ==> !graph.successors.nodes([0]).any(x| x == nodes[0])),
        2.. => false,
    }
    if acyclic {

Comment on lines +159 to +165
GlobalAlloc::Static(def_id) => {
if let Some(local_def) = def_id.as_local()
&& let Some(&node) = node_of.get(&local_def)
{
out.insert(node);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

So we are going to insert into the StaticNodeIdx set, so that can't happen twice...

Comment on lines +154 to +169
if !seen.insert(alloc_id) {
continue;
}

match tcx.global_alloc(alloc_id) {
GlobalAlloc::Static(def_id) => {
if let Some(local_def) = def_id.as_local()
&& let Some(&node) = node_of.get(&local_def)
{
out.insert(node);
}
}

GlobalAlloc::Memory(const_alloc) => {
push_ptr_alloc_ids(const_alloc.inner(), &mut stack);
}
Copy link
Member

Choose a reason for hiding this comment

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

But here we have three sources of truth for what we have interacted with so far... and a fourth if we consider the hashmap that backs alloc IDs?

Do you think it would simplify things to use FxIndexSet and a counter, instead of a HashSet and Vec? At the very least, we're not saving ourselves much memory by using a transient Vec, because the HashSet for tracking what we've examined must continually grow anyways.

continue;
}

match tcx.global_alloc(alloc_id) {
Copy link
Member

Choose a reason for hiding this comment

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

This has some interesting documentation...

    /// Panics in case the `AllocId` is dangling. Since that is impossible for `AllocId`s in
    /// constants (as all constants must pass interning and validation that check for dangling
    /// ids), this function is frequently used throughout rustc, but should not be used within
    /// the interpreter.
    pub fn global_alloc(self, id: AllocId) -> GlobalAlloc<'tcx> {

I see why you raised your concern about whether this is the best place for this code... I usually see target-based checks show up later, in rustc_monomorphize, but this is kinda special since it's about statics mostly, I guess, which are guaranteed monomorphization if they're included at all?

@workingjubilee workingjubilee changed the title Ensure that static initializers are acyclic for targets that require this Ensure that static initializers are acyclic for NVPTX Jan 1, 2026
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this was a "minicore" test so we can run it on other hosts yet cross-target it for nvptx64, otherwise this test never runs.

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

Labels

O-NVPTX Target: the NVPTX LLVM backend for running rust on GPUs, https://llvm.org/docs/NVPTXUsage.html S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants