Skip to content

ssa: fix zero-value map indexing#1835

Merged
xushiwei merged 5 commits intoxgo-dev:mainfrom
zhouguangyuan0718:main-fix-zerovalue
Apr 28, 2026
Merged

ssa: fix zero-value map indexing#1835
xushiwei merged 5 commits intoxgo-dev:mainfrom
zhouguangyuan0718:main-fix-zerovalue

Conversation

@zhouguangyuan0718
Copy link
Copy Markdown
Contributor

@zhouguangyuan0718 zhouguangyuan0718 commented Apr 27, 2026

Summary

  • represent zero-value maps as null *runtime.Map pointers in SSA zero-value lowering
  • avoid constant-extracting array elements from nil-backed values so zero-value map indexing stays on the normal load path
  • add cl/_testgo/mapzero to cover the runtime output and the FileCheck expectation

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a test case for zero-length array indexing and adjusts the handling of map zero values in the SSA builder. Specifically, it ensures that map zero values are represented as pointers and removes a constant extraction optimization in ssa/datastruct.go to ensure consistent behavior. A review comment suggests improving the robustness of the LLVM IR check by using wildcards instead of hardcoded register names.

Comment thread cl/_testlibgo/mapzero/in.go Outdated
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

Review Summary

The fix is correct: zero-value maps should be represented as null *runtime.Map pointers (matching how ssa/type.go types maps as llvm.PointerType(p.rtMap(), 0)), not as zero-initialized Map structs. The ConstExtractElement removal is also warranted since it bypassed checkIndex, allowing UB on zero-length arrays. The performance impact is negligible — LLVM's mem2reg/SROA passes will optimize the alloc+store fallback, and constfold handles the constant case automatically.

Two minor notes on the test below.

Comment thread cl/_testlibgo/mapzero/in.go Outdated
err := recover()
fmt.Println(err)
}()
// CHECK: call ptr @"github.com/goplus/llgo/runtime/internal/runtime.MapAccess1"(ptr @"map[_llgo_int]_llgo_int", ptr null, ptr %21)
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.

Nit (fragile CHECK pattern): The hardcoded %21 register will break if any unrelated compiler change shifts the IR instruction count. The tpinst test uses %{{[0-9]+}} for exactly this reason. Consider:

// CHECK: call ptr @"github.com/goplus/llgo/runtime/internal/runtime.MapAccess1"(ptr @"map[_llgo_int]_llgo_int", ptr null, ptr %{{[0-9]+}})

The key property being verified is ptr null as the map argument — the register number of the key pointer is not relevant to the fix.

fmt.Println(err)
}()
// CHECK: call ptr @"github.com/goplus/llgo/runtime/internal/runtime.MapAccess1"(ptr @"map[_llgo_int]_llgo_int", ptr null, ptr %21)
m := [0]map[int]int{}[a][0]
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.

Nit (readability): This expression is dense and tests two things at once (array bounds check + nil map representation). A short comment would help future readers:

// Index a zero-length array literal to get a nil map, then index that nil map.
// The array index panics with "index out of range" before the map access.

Represent zero-value maps as null *runtime.Map pointers and avoid constant-extracting array elements from nil-backed values. This keeps zero-value map indexing on the normal load path and emits MapAccess1 with a nil map pointer instead of a zero-initialized hmap value.

Add a focused cl/_testlibgo/mapzero case covering the runtime output and the FileCheck expectation.

Signed-off-by: ZhouGuangyuan <zhouguangyuan.xian@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.50%. Comparing base (9b0889e) to head (313211f).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   88.48%   88.50%   +0.02%     
==========================================
  Files          50       50              
  Lines       14428    14426       -2     
==========================================
+ Hits        12766    12768       +2     
+ Misses       1441     1438       -3     
+ Partials      221      220       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xushiwei xushiwei merged commit 1196550 into xgo-dev:main Apr 28, 2026
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants