Skip to content

[LoongArch64] Fix the m_usComponentSize load in RhpNewArrayFast.#130958

Open
LuckyXu-HF wants to merge 3 commits into
dotnet:mainfrom
LuckyXu-HF:main-LA64
Open

[LoongArch64] Fix the m_usComponentSize load in RhpNewArrayFast.#130958
LuckyXu-HF wants to merge 3 commits into
dotnet:mainfrom
LuckyXu-HF:main-LA64

Conversation

@LuckyXu-HF

Copy link
Copy Markdown
Contributor
  • Fix the SIGBUS error of Loader.sh hit by PR#130019's MaxArrayElementSize.
  • Add dasm GC info print.

* Fix the SIGBUS error of Loader.sh hit by PR#130019's MaxArrayElementSize.
* Add dasm GC info print.
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 17, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@LuckyXu-HF

Copy link
Copy Markdown
Contributor Author

@shushanhf @jakobbotsch Could you please review this PR? Thanks.

@shushanhf shushanhf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! thanks
We had tested is ok for LA64.

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "02106a9638726248148983054a1b9bd2793d1419",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "630a7009ba484fe96b8f0b81e2f4cc0bb6827903",
  "last_reviewed_commit": "02106a9638726248148983054a1b9bd2793d1419",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "630a7009ba484fe96b8f0b81e2f4cc0bb6827903",
  "last_recorded_worker_run_id": "29733475276",
  "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": "ab0e0c0f347e1354a9cebdfebd1fd133b8aec442",
      "review_id": 4730731920
    },
    {
      "commit": "02106a9638726248148983054a1b9bd2793d1419",
      "review_id": 4734014621
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: PR #130019 introduced MaxArrayElementSize, and the LoongArch64 RhpNewArrayFast allocation stub was loading the 16-bit m_usComponentSize field with a sign-extending load (ld.h) and a signed multiply (mulw.d.w). Because m_usComponentSize is an unsigned 16-bit value, sign extension of a component size with the high bit set produces a wrong (negative) value, corrupting the allocation size computation and manifesting as the SIGBUS observed in Loader.sh. The PR also adds LoongArch64 to the GC PREFETCH support set and enables disasm-with-GC-info output in the LoongArch64 JIT emitter for parity with arm64/riscv64.

Approach: In src/coreclr/runtime/loongarch64/AllocFast.S, ld.h/mulw.d.w are replaced with the unsigned ld.hu/mulw.d.wu. This matches the documented contract in the surrounding comment ("component size is <= 0xffff ... an unsigned 16-bit value") and the equivalent arm64 (ldrh) and riscv64 (lhu) stubs. emitloongarch64.cpp gains the standard emitDispGCInfoDelta() call guarded by EMIT_GC_VERBOSE || m_compiler->opts.disasmWithGC inside the existing #ifdef DEBUG block, identical to the arm64/riscv64 emitters. gcinternal.h adds TARGET_LOONGARCH64 to the PREFETCH enablement #if; the Unix path already uses __builtin_prefetch, which is valid for LoongArch64.

Summary: This is a small, correct, and well-scoped bug fix. The core allocation-stub change is a genuine correctness fix (signed vs. unsigned load/multiply of the unsigned component size) and is consistent with the other 64-bit architectures. The two supporting diagnostic/parity changes are low risk and mirror established patterns. No correctness, security, or performance concerns identified; 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 · 44 AIC · ⌖ 16.1 AIC · ⊞ 10K

@LuckyXu-HF

Copy link
Copy Markdown
Contributor Author

The CI Format jit failed is unrelated to this PR, it's brought in by the main branch:

format.patch
diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h
index 4880f769..74c4df51 100644
--- a/src/coreclr/jit/rangecheck.h
+++ b/src/coreclr/jit/rangecheck.h
@@ -311,68 +311,69 @@ struct RangeOps
     template <typename Operation>
     static Range ApplyRangeOp(const Range& r1, const Range& r2, Operation op)
     {
         const Limit& r1lo = r1.LowerLimit();
         const Limit& r1hi = r1.UpperLimit();
         const Limit& r2lo = r2.LowerLimit();
         const Limit& r2hi = r2.UpperLimit();
 
         Range result = Limit(Limit::keUnknown);
         // If either limit is dependent, the result is dependent.
         // otherwise, apply the operation.
         result.lLimit = (r1lo.IsDependent() || r2lo.IsDependent()) ? Limit(Limit::keDependent) : op(r1lo, r2lo);
         result.uLimit = (r1hi.IsDependent() || r2hi.IsDependent()) ? Limit(Limit::keDependent) : op(r1hi, r2hi);
         return result;
     }
 
     static Range Add(const Range& r1, const Range& r2, bool unsignedAdd = false)
     {
         if (unsignedAdd)
         {
-            bool r1StraddlesZero = r1.IsConstantRange() && (r1.LowerLimit().GetConstant() < 0) &&
-                                   (r1.UpperLimit().GetConstant() >= 0);
-            bool r2StraddlesZero = r2.IsConstantRange() && (r2.LowerLimit().GetConstant() < 0) &&
-                                   (r2.UpperLimit().GetConstant() >= 0);
+            bool r1StraddlesZero =
+                r1.IsConstantRange() && (r1.LowerLimit().GetConstant() < 0) && (r1.UpperLimit().GetConstant() >= 0);
+            bool r2StraddlesZero =
+                r2.IsConstantRange() && (r2.LowerLimit().GetConstant() < 0) && (r2.UpperLimit().GetConstant() >= 0);
             if (r1StraddlesZero || r2StraddlesZero)
             {
                 // Signed intervals that straddle zero are not monotonic when interpreted as unsigned.
                 return Limit(Limit::keUnknown);
             }
         }
 
         return ApplyRangeOp(r1, r2, [unsignedAdd](const Limit& a, const Limit& b) {
             // For Add we support:
             //   keConstant + keConstant  => keConstant
             //   keBinOpArray + keConstant => keBinOpArray
             //   keConstant + keBinOpArray => keBinOpArray
             if (a.IsConstantOrBinOp() && b.IsConstantOrBinOp())
             {
                 if (a.IsBinOpArray() && b.IsBinOpArray())
                 {
                     // We can't represent the sum of two BinOpArrays.
                     return Limit(Limit::keUnknown);
                 }
 
                 static_assert(CheckedOps::Unsigned == true);
                 // For unsigned adds, require both unsigned and signed endpoint sums to not overflow.
                 bool requestedAddOverflows = CheckedOps::AddOverflows(a.GetConstant(), b.GetConstant(), unsignedAdd);
-                bool signedEndpointOverflows = unsignedAdd && CheckedOps::AddOverflows(a.GetConstant(), b.GetConstant(), CheckedOps::Signed);
+                bool signedEndpointOverflows =
+                    unsignedAdd && CheckedOps::AddOverflows(a.GetConstant(), b.GetConstant(), CheckedOps::Signed);
                 if (!requestedAddOverflows && !signedEndpointOverflows)
                 {
                     if (a.IsConstant() && b.IsConstant())
                     {
                         return Limit(Limit::keConstant, a.GetConstant() + b.GetConstant());
                     }
 
                     return Limit(Limit::keBinOpArray, a.IsBinOpArray() ? a.vn : b.vn,
                                  a.GetConstant() + b.GetConstant());
                 }
             }
             return Limit(Limit::keUnknown);
         });
     }
 
     static Range Subtract(const Range& r1, const Range& r2, bool unsignedSub = false)
     {
         if (unsignedSub)
         {
             return Limit(Limit::keUnknown); // Give up on unsigned subtraction for now

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: PR #130019 introduced MaxArrayElementSize, and the LoongArch64 RhpNewArrayFast allocation stub was loading the 16-bit m_usComponentSize field with a sign-extending load (ld.h) and a signed multiply (mulw.d.w). Because m_usComponentSize is an unsigned 16-bit value, sign extension of a component size with the high bit set produces a wrong (negative) value, corrupting the allocation size computation and manifesting as the SIGBUS observed in Loader.sh. The PR also adds LoongArch64 to the GC PREFETCH support set and enables disasm-with-GC-info output in the LoongArch64 JIT emitter for parity with arm64/riscv64.

Approach: In src/coreclr/runtime/loongarch64/AllocFast.S, ld.h/mulw.d.w are replaced with the unsigned ld.hu/mulw.d.wu, matching the surrounding comment and the arm64 (ldrh) and riscv64 (lhu) stubs. emitloongarch64.cpp gains the standard emitDispGCInfoDelta() call guarded by EMIT_GC_VERBOSE || m_compiler->opts.disasmWithGC inside the existing #ifdef DEBUG block, identical to arm64/riscv64. gcinternal.h adds TARGET_LOONGARCH64 to the PREFETCH enablement #if; the Unix path already uses __builtin_prefetch, valid for LoongArch64.

Summary: This remains a small, correct, and well-scoped bug fix consistent with the other 64-bit architectures. The core allocation-stub change is a genuine correctness fix, and the two supporting diagnostic/parity changes are low risk and mirror established patterns. Since the prior review, the PR was rebased onto a newer base with no change to the cumulative PR patch (identical patch IDs, same head commit range content), so there are no new findings. No correctness, security, or performance concerns identified.

Assessment History:

  • review 4730731920 reviewed commit ab0e0c0, verdict LGTM (no actionable findings). Current verdict LGTM. Assessment unchanged: the PR patch is identical after the rebase (matching patch IDs), so motivation, approach, and risk are all unchanged.

Note

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

Generated by Holistic Review · 34.2 AIC · ⌖ 20.1 AIC · ⊞ 10K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: PR #130019 introduced MaxArrayElementSize, and the LoongArch64 RhpNewArrayFast allocation stub loaded the unsigned 16-bit m_usComponentSize field with a sign-extending load (ld.h) and a signed multiply (mulw.d.w), corrupting the allocation size and causing the SIGBUS observed in Loader.sh. The PR also adds LoongArch64 to the GC PREFETCH set, enables disasm-with-GC-info output in the LoongArch64 emitter, and (in rangecheck.h) hardens the unsigned-add monotonicity/overflow logic.

Approach: The core fix replaces ld.h/mulw.d.w with unsigned ld.hu/mulw.d.wu, matching the documented contract and the arm64/riscv64 stubs. emitloongarch64.cpp adds the standard emitDispGCInfoDelta() call, and gcinternal.h enables PREFETCH for LoongArch64. The latest commit (02106a9, "apply format.patch") is a pure jit-format whitespace reflow of rangecheck.h — wrapping three multi-line boolean initializers into single-line form — with no semantic change.

Summary: The incremental change since the prior review is formatting-only and low risk; it correctly satisfies the repository's jit-format line-length conventions without altering behavior. The cumulative assessment is unchanged: a small, correct, well-scoped bug fix consistent with the other 64-bit architectures. No correctness, security, or performance concerns; no actionable findings.

Assessment History

  • review 4730731920 reviewed commit ab0e0c0 with an LGTM (no actionable findings) verdict. Current verdict is also LGTM. The assessment is unchanged: the only new patch content is the 02106a9 formatting reflow, which does not change motivation, approach, or risk.

Detailed Findings

None. The incremental commit reflows three boolean initializer expressions in src/coreclr/jit/rangecheck.h (lines ~331-334 and ~358-359) to single-line form per jit-format; the logic is byte-for-byte equivalent.

Note

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

Generated by Holistic Review · 54.6 AIC · ⌖ 20.2 AIC · ⊞ 10K

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-loongarch64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants