Skip to content

[release/10.0] Fix alignment of 8-byte thread statics on direct TLS path - #129785

Merged
JulieLeeMSFT merged 1 commit into
release/10.0from
backport/pr-129749-to-release/10.0
Sep 11, 2026
Merged

JulieLeeMSFT merged 1 commit into
release/10.0from
backport/pr-129749-to-release/10.0

Conversation

@github-actions

@github-actions github-actions Bot commented Jun 24, 2026 •

Copy link
Copy Markdown
Contributor

Backport of #129749 to release/10.0

/cc @EgorBo

Customer Impact

  • Customer reported
  • Found internally

Some TLS (ThreadStatic) fields could be allocated with 4b alignment while demanding 8b e.g. long. This may lead to DataMisalignedException in certain cases (e.g. using Interlocked on arm64, or double fields on arm32) or/and reiceve a perf penalty from misaligned access.

Regression

  • Yes
  • No

It was introduced in .NET 9.0

Testing

Validated locally that 64-bit TLS variables never get any alignment less than 64 bytes.

Risk

Low to none.

GetTLSIndexForThreadStatic computes the alignment for a thread static
placed on the direct-on-thread-local-data bump allocator. The chain of
size checks was missing an `else` before the `>= 4` case, so an 8-byte
field (long/double) first set alignment = 8 and then immediately
overwrote it with alignment = 4. The resulting offset could land on a
4-mod-8 boundary, producing a misaligned long/double.

On arm64 this causes Interlocked.Increment(ref threadStaticLong) to
throw DataMisalignedException, since the atomic lowers to ldaxr/stlxr
which fault on a misaligned address.

Fixes #129733

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@github-actions

github-actions Bot commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "bdcff0aa290faad6adb3f5be1c6a4552e3b3b74b",
  "last_dispatched_base_ref": "release/10.0",
  "last_dispatched_base_sha": "210a47bce9679dacd43a61e3dbaee6e7872293cf",
  "last_reviewed_commit": "bdcff0aa290faad6adb3f5be1c6a4552e3b3b74b",
  "last_reviewed_base_ref": "release/10.0",
  "last_reviewed_base_sha": "210a47bce9679dacd43a61e3dbaee6e7872293cf",
  "last_recorded_worker_run_id": "29677694033",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "bdcff0aa290faad6adb3f5be1c6a4552e3b3b74b",
      "review_id": 4730521503
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Holistic Review

Motivation: This is a backport of #129749 to release/10.0, fixing a .NET 9 regression in GetTLSIndexForThreadStatic where 8-byte thread statics (long/double) placed on the direct-on-thread-local-data bump allocator could be under-aligned. The chain of size-based alignment checks was missing an else before the >= 4 case, so an 8-byte field first set alignment = 8 and then unconditionally overwrote it with alignment = 4. The resulting offset could land on a 4-mod-8 boundary, producing a misaligned long/double. On arm64 this surfaces as DataMisalignedException for Interlocked.Increment(ref threadStaticLong) (the atomic lowers to ldaxr/stlxr, which fault on misaligned addresses), and on arm32 for double fields, plus a general misaligned-access perf penalty. Fixes #129733.

Approach: A single-line change converts the second if (bytesNeeded >= 4) into else if, restoring the intended mutually-exclusive if/else-if alignment ladder (8 → 4 → 2 → 1). The remaining else if (bytesNeeded >= 2) / else branches were already correct, so the 8-vs-4 case was the only broken transition. The patch is byte-for-byte identical to the merged source PR #129749, which is exactly what a backport should be.

Summary: LGTM. The fix is minimal, correct, and low-risk. Making the alignment ladder mutually exclusive is the right fix: for bytesNeeded >= 8 alignment now correctly stays at 8; for 4 <= bytesNeeded < 8 it is 4; and smaller sizes are unaffected. The subsequent AlignDown(indexOffsetWithoutAlignment, alignment) plus the alignmentAdjust <= newBytesAvailable guard then place the slot on a properly aligned offset. No behavioral change for non-8-byte statics. The change is a clean cherry-pick of an already-reviewed fix with an appropriate regression rationale for a servicing branch; I have no actionable findings.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 41 AIC · ⌖ 10.4 AIC · ⊞ 10K

@svick

svick commented Aug 6, 2026

Copy link
Copy Markdown
Member

Hi,

the code complete date for 10.0.12 (the September 2026 release) is Monday 10 August. Make sure to merge this PR on that date at the latest, or it won't make it into that release.

As a reminder, if this is a product change, you also need Tactics approval before merging this PR (test-only or infra-only changes don't require Tactics approval).

@jkotas

jkotas commented Aug 6, 2026

Copy link
Copy Markdown
Member

@EgorBo Are you going to take care of getting this approved?

@ViveliDuCh

Copy link
Copy Markdown
Member

Hi,

the code complete date for 10.0.13 (the October 2026 release) is Monday 14 September. Make sure to merge this PR on that date at the latest, or it won't make it into that release.

As a reminder, if this is a product change, you also need Tactics approval before merging this PR (test-only or infra-only changes don't require Tactics approval).

Happy to help with the merge if needed.

@EgorBo

EgorBo commented Sep 10, 2026

Copy link
Copy Markdown
Member

Ouch, didn't see any notifications for this one somehow, I'll send it to tactics.

@JulieLeeMSFT JulieLeeMSFT added the Servicing-consider Issue for next servicing release review label Sep 11, 2026
@JulieLeeMSFT JulieLeeMSFT added this to the 10.0.x milestone Sep 11, 2026
@JulieLeeMSFT JulieLeeMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 11, 2026
@JulieLeeMSFT
JulieLeeMSFT merged commit 6a66dae into release/10.0 Sep 11, 2026
117 of 120 checks passed
@JulieLeeMSFT
JulieLeeMSFT deleted the backport/pr-129749-to-release/10.0 branch September 11, 2026 20:51
@rbhanda rbhanda modified the milestones: 10.0.x, 10.0.13 Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants