Skip to content

Conversation

@ThadHouse
Copy link
Contributor

85507b9 introduced a new call to Marshal.Sizeof(Type). This call is not AOT compatible, which is a regression as prior versions were AOT compatible, even if not directly supported.

This conditionally uses the generic version of Marshal.SizeOf, which is AOT compatible to fix the issue.

Closes #21824

@ThadHouse ThadHouse requested a review from a team as a code owner May 25, 2025 19:13
@ThadHouse ThadHouse requested review from jskeet and removed request for a team May 25, 2025 19:13
@google-cla
Copy link

google-cla bot commented May 25, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jskeet jskeet removed their request for review May 25, 2025 19:14
@honglooker honglooker added the c# label Jun 5, 2025
@esrauchg esrauchg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 25, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 25, 2025
@eerhardt
Copy link

@JasonLunn @YarinOmesi - it looks like #19667 caused this regression with native AOT. Is there any way this PR could get merged to unblock the AOT issue?

@snailcatcher
Copy link

I think this is a pretty good solution.
I understand that you not officially support AoT.
But, if someone wants to use AoT on his own risk, why not let them do that?

@eerhardt
Copy link

@JasonLunn - What is the process for getting this change merged? Are there C# owners/contributors in this project that could review and get the change merged?

@jskeet
Copy link
Contributor

jskeet commented Aug 28, 2025

I can't speak to whether this will fix any AOT issues, but it at worst looks harmless.

@jskeet jskeet assigned googleberg and unassigned jskeet Aug 28, 2025
@jskeet
Copy link
Contributor

jskeet commented Aug 28, 2025

(Assigned back to @googleberg for the next step.)

@googleberg googleberg added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Aug 28, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Aug 28, 2025
@googleberg
Copy link
Member

@ThadHouse Tests seem to be failing. You may need to rebase.

@martincostello
Copy link

This pull request is from an unsafe fork and hasn't been approved to run tests.
A protobuf team member will need to review the PR and add the 'safe for tests' tag.

@ThadHouse
Copy link
Contributor Author

Rebased anyway, but as said it seems like tests were just not running due to being external.

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 6, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 6, 2025
@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2025
@zhangskz
Copy link
Member

Looks like this got stuck at some point after it was approved and likely got garbage collected. Giving this a kick now to see if we can get this submitted. Feel free to follow up if this seems to get stuck again.

copybara-service bot pushed a commit that referenced this pull request Oct 13, 2025
85507b9 introduced a new call to Marshal.Sizeof(Type). This call is not AOT compatible, which is a regression as prior versions were AOT compatible, even if not directly supported.

This conditionally uses the generic version of Marshal.SizeOf, which is AOT compatible to fix the issue.

Closes #21824

Closes #21964

COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
PiperOrigin-RevId: 818676280
copybara-service bot pushed a commit that referenced this pull request Oct 13, 2025
85507b9 introduced a new call to Marshal.Sizeof(Type). This call is not AOT compatible, which is a regression as prior versions were AOT compatible, even if not directly supported.

This conditionally uses the generic version of Marshal.SizeOf, which is AOT compatible to fix the issue.

Closes #21824

Closes #21964

COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
PiperOrigin-RevId: 818676280
@martincostello
Copy link

Was the fix for this expected to be in the 3.33.0 release?

After getting a renovate PR for the version bump, I'm still getting this error in CI:

/_/csharp/src/Google.Protobuf/Collections/RepeatedField.cs(714): AOT analysis error IL3050: Google.Protobuf.Collections.RepeatedField`1.TryGetArrayAsSpanPinnedUnsafe(FieldCodec`1<T>,Span`1<Byte>&,GCHandle&): Using member 'System.Runtime.InteropServices.Marshal.SizeOf(Type)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. Marshalling code for the object might not be available. Use the SizeOf<T> overload instead. [/home/runner/work/api/api/src/API/API.csproj]

@martincostello
Copy link

Looks like it didn't. The NuGet package was built from a79f2d2, and that line of code is still:

if (BitConverter.IsLittleEndian && (codec.FixedSize > 0 && Marshal.SizeOf(typeof(T)) == codec.FixedSize))

@zhangskz
Copy link
Member

Looks like this just barely missed the branch cut for v33.0 unforutnately, but I'll flag this for the v33.2 patch.

zhangskz pushed a commit that referenced this pull request Dec 2, 2025
85507b9 introduced a new call to Marshal.Sizeof(Type). This call is not AOT compatible, which is a regression as prior versions were AOT compatible, even if not directly supported.

This conditionally uses the generic version of Marshal.SizeOf, which is AOT compatible to fix the issue.

Closes #21824

Closes #21964

COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
PiperOrigin-RevId: 818694739
@zhangskz
Copy link
Member

zhangskz commented Dec 2, 2025

We should be patching this in v33.2 ~this week.

zhangskz added a commit that referenced this pull request Dec 2, 2025
85507b9 introduced a new call to Marshal.Sizeof(Type). This call is not AOT compatible, which is a regression as prior versions were AOT compatible, even if not directly supported.

This conditionally uses the generic version of Marshal.SizeOf, which is AOT compatible to fix the issue.

Closes #21824

Closes #21964

COPYBARA_INTEGRATE_REVIEW=#21964 from ThadHouse:genericsizeof a12294e
PiperOrigin-RevId: 818694739

Co-authored-by: Thad House <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update to 3.31.0 breaks AOT build

10 participants