Restore Crossgen2's Optimizations in CoreCLR Builds

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ivdiazsa]

Addresses Issue #90265. Previously on PR #89223, we removed the -O flag from calls to Crossgen2 because some tests were failing due to their reliance on the debug bit, which Crossgen2's -O flag removes. However, removing the flag from the CoreCLR builds led to an unexpected side effect of NarrowUtf16ToAscii of targeting V256 instead of V128. A full explanation can be found here in this comment: #89986 (comment)

As a consequence of the issues mentioned above, a safety assertion had to be disabled in Ascii.Utility.cs to mitigate this problem, which was having a big impact. We should not rely on skipping safety checks to get around bugs. This PR brings back what allowed things to work properly without circumventing security.

[ivdiazsa]

@jakobbotsch I also reenabled the JIT poison tests that were disabled because of issue #56148. Restoring only the CoreCLR -O fixes the assertion problem while still allowing the poison tests to function properly. Let me know if there are specific pipelines I should run to ensure those tests really work well now.

[trylek]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

[jakobbotsch]

You should run runtime-coreclr crossgen2 to verify that the poison test works correctly during crossgen.

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

[ivdiazsa]

In general I was under the impression that the underlying problem you described is mostly a framework issue but I'm fine with this as a short-term workaround, Aaron has already created the issue to address this more cleanly - I assume that will include reverting your current hotfix; it might be worth mentioning your current PR in the issue #90265 for context.

I'm going to explain it in that issue accordingly when this PR is merged. I just think removing asserts is not the ideal way to mitigate problems when removing the cause is straightforward, and it doesn't break anything else. Otherwise, we're just adding to the snowball of patches that can potentially blow up at any time.

I agree, we should not hit asserts just because we crossgen SPC without optimizations, so there's definitely something to address at some point here.

Agreed with this as well. I will be keeping the issue open. Taking from Tanner's comment here (#89986 (comment)), it might end up being some sort of feature work so it will help tracking it.

[ivdiazsa]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).