[NativeAOT] add support for CriticalFinalizerObject

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis

[AustinWise]

Tested using mono/mono/tests/critical-finalizers.cs

fixes#75106

[AustinWise]

With @VSadov adding support for eager finalization in #75436, it looks pretty straightforward to add critical finalizer support.

I searched the codebase for CriticalFinalizerObject and only found a mono test for critical finalization. Should this test be moved into a common location so it can run on all the runtimes?

[jkotas]

Critical finalizers are not a core type system concept. We do not need them to introduce in managed type system. We can just check for the critical finalizer base type in ComputeFlagsEx when building EEType.

[VSadov]

Right. This should be very similar to HasEagerFinalizerFlag, just a different trait. HasCriticalFinalizer should be computed at the same time when HasEagerFinalizerFlag is computed, no need to compute it earlier.

A few things to note:

• CriticalFinalizerObject does not need to be immediate base, so you will need to get all the way up the inheritance chain until you see CriticalFinalizerObject or nothing.
• you do not need to do that to every type, it would make sense to first check if it is a finalizable type (typedesc would know that).

[AustinWise]

I reworked this so the logic is only in EETypeBuilderHelpers. Is this what you had in mind?

[jkotas]

You should be able to delete src\coreclr\nativeaot\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.NativeAot.cs

[jkotas]

And delete the nativeaot ifdef in src\libraries\System.Private.CoreLib\src\System\Runtime\InteropServices\SafeHandle.cs

[VSadov]

Should this test be moved into a common location so it can run on all the runtimes?

Yes, please. If it is the only test, it would be nice to have it shared.

I think this would be a logical location for the test:
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System/Runtime/ConstrainedExecution

the style of the mono test will need to be adjusted to fit the libraries style/xunit, but the test itself looks good.

[VSadov]

it would be nice to avoid recursion here. in abnormal cases there could be a lot of bases.

[jkotas]

private static bool HasCriticalFinalizer(TypeDesc type)
{
do
{
if (!type.HasFinalizer)
return false;
if (type is MetadataType mdType ....
return true;
type = type.BaseType;
}
while (type != null);
return false;
}

[jkotas]

LGTM. Thank you!

[jkotas]

Known failures happening that are also hit in main according to the build analysis