rustc_monomorphize: Skip vtable alloc collection in the pre-optimisation MentionedItems mode

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.

[jakubadamw]

When collecting mentioned items during monomorphization, the collect_alloc() routine was recursively walking vtable allocations, which could lead to infinite recursion if a vtable method's body contained a const embedding another vtable. We can skip the recursive walk when we're only gathering mentioned items. Vtable methods are only relevant during actual codegen, so this should be safe.

Closes#141911.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 66 candidates
• Random selection from 13 candidates

[Mark-Simulacrum]

Vtable methods are only relevant during actual codegen, so this should be safe.

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

[tmiasko]

The problem resurfaces when compiling without optimizations -Zmir-opt-level=0. The motivation behind "mentioned" collection mode is to ensure consistency across optimization levels. In that sense the existing implementation is working as intended (putting aside the glaring issue of ICE itself).

[tmiasko]

ICE will be turned into an error in #152120. I don't think collection process needs any changes (except perhaps to improve the diagnostic to indicate a source location that gives rise to the error).

[jakubadamw]

Can you elaborate on "safe"? Are we reaching the mentioned items only brought in via vtables in some other way already? Can this change behavior of existing programs (e.g., make more things compile in check mode)?

@Mark-Simulacrum, these are very helpful questions! You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

@tmiasko, thank you for your feedback as well!

[tmiasko]

You're right that this attempt may be preventing some invalid programs from being rejected, and I'll try to come up with a test case that demonstrates it.

To be clear, the test case you have already included should fail to compile. This is because evaluating const { std::ptr::null::<VirtualWrapper<T>>() as *const dyn MyTrait }; for T = u8 requires constructing vtable for VirtualWrapper<u8> as MyTrait, which in turn requires constructing vtable for VirtualWrapper<VirtualWrapper<u8>> as MyTrait and so on.

The only issue is that this results in an ICE instead of an error.

[jakubadamw]

#141911 has been fixed, closing this.