fix uninitialized reg arr for VlWhole staticInst type#697
Conversation
📝 WalkthroughWalkthroughA register index array initialization call has been added to the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/arch/riscv/isa/vector/base/vector_mem.temp.isa:
- Around line 426-427: VsWholeConstructor is missing the same
register-index-array initialization added to VlWholeConstructor, leading to
potential heap-buffer-overflow when generateDisassembly reads srcRegIdxArr[8];
add the same initialization macro/token `%(set_reg_idx_arr)s;` (or equivalent
code that sets/zeros srcRegIdxArr and dstRegIdxArr) at the start of the
VsWholeConstructor constructor body so its srcRegIdxArr[8] members are
initialized before any micro-op creation or generateDisassembly usage.
| %(set_reg_idx_arr)s; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and check the structure around lines 426-427
echo "=== Checking file existence and basic structure ==="
if [ -f "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" ]; then
echo "File found."
echo ""
echo "=== Content around lines 426-427 ==="
sed -n '410,445p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
else
echo "File not found at specified path"
find . -name "vector_mem.temp.isa" 2>/dev/null
fiRepository: OpenXiangShan/GEM5
Length of output: 1205
🏁 Script executed:
#!/bin/bash
# Check usage of %(set_reg_idx_arr)s; throughout the file
echo "=== Checking %(set_reg_idx_arr)s; usage ==="
rg -n "set_reg_idx_arr" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"
echo ""
echo "=== Checking for constructor definitions ==="
rg -n "def template.*Constructor" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | head -30Repository: OpenXiangShan/GEM5
Length of output: 965
🏁 Script executed:
#!/bin/bash
# Check register array declarations
echo "=== Checking register array declarations ==="
rg -n "reg_idx_arr_decl\|reg_idx_arr" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | head -20
echo ""
echo "=== Check for generateDisassembly in macro instructions ==="
rg -n "generateDisassembly" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"Repository: OpenXiangShan/GEM5
Length of output: 829
🏁 Script executed:
#!/bin/bash
# Check VsWholeConstructor (similar structure to VlWholeConstructor)
echo "=== VsWholeConstructor (line 1186) context ==="
sed -n '1186,1235p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
echo ""
echo "=== Check if VsWholeConstructor has set_reg_idx_arr ==="
sed -n '1186,1235p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | grep -n "set_reg_idx_arr"Repository: OpenXiangShan/GEM5
Length of output: 2137
🏁 Script executed:
#!/bin/bash
# Check around line 117 and 351 to confirm they're micro templates
echo "=== Around line 117 (after VleConstructor at 45) ==="
sed -n '100,130p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
echo ""
echo "=== Around line 351 (after VleffConstructor at 310) ==="
sed -n '330,365p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -nRepository: OpenXiangShan/GEM5
Length of output: 2615
🏁 Script executed:
#!/bin/bash
# Check VleConstructor and its structure
echo "=== VleConstructor (line 45) context ==="
sed -n '45,100p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
echo ""
echo "=== VleffConstructor (line 310) context ==="
sed -n '310,360p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -nRepository: OpenXiangShan/GEM5
Length of output: 4567
🏁 Script executed:
#!/bin/bash
# Check the relationship between VsWholeConstructor and VsWholeMicroDeclare
echo "=== Checking VsWholeConstructor full structure ==="
sed -n '1186,1210p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
echo ""
echo "=== Checking VsWholeMicroDeclare definition starts ==="
grep -n "def template VsWholeMicroDeclare" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"
echo ""
echo "=== Comparing VlWholeConstructor and VlWholeMicroDeclare structure ==="
echo "=== VlWholeConstructor at line 421 ==="
sed -n '421,450p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -nRepository: OpenXiangShan/GEM5
Length of output: 2135
🏁 Script executed:
#!/bin/bash
# Check if there are any other "Whole" constructors that might have the same issue
echo "=== All 'Whole' related templates ==="
rg -n "def template.*Whole" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"
echo ""
echo "=== Check which ones have set_reg_idx_arr in constructors (not micro) ==="
echo "Looking at each Whole constructor:"
echo ""
echo "VlWholeConstructor (421):"
sed -n '421,455p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | grep "set_reg_idx_arr"
echo ""
echo "VsWholeConstructor (1186):"
sed -n '1186,1215p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | grep "set_reg_idx_arr"Repository: OpenXiangShan/GEM5
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Check VsWholeMicroDeclare for register arrays
echo "=== VsWholeMicroDeclare (line 1210) ==="
sed -n '1210,1240p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -n
echo ""
echo "=== Compare: VlWholeMicroDeclare (line 448) ==="
sed -n '448,475p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | cat -nRepository: OpenXiangShan/GEM5
Length of output: 2566
🏁 Script executed:
#!/bin/bash
# Verify if both Whole micro classes have register arrays and generateDisassembly
echo "=== Check register arrays in both Whole micro classes ==="
rg -A 10 "def template VlWholeMicroDeclare" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | head -20
echo ""
echo "=== VsWhole micro class ==="
rg -A 10 "def template VsWholeMicroDeclare" "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | head -20Repository: OpenXiangShan/GEM5
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Final verification: confirm both Whole constructors have identical structures
echo "=== VlWholeConstructor structure (has fix) ==="
sed -n '421,442p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | grep -E "(def template|%(set_reg_idx_arr|size_t NFIELDS|for|IsLoad)"
echo ""
echo "=== VsWholeConstructor structure (missing fix) ==="
sed -n '1186,1207p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa" | grep -E "(def template|%(set_reg_idx_arr|size_t NFIELDS|for|IsStore)"Repository: OpenXiangShan/GEM5
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Final verification: confirm both Whole constructors have identical structures
echo "=== VlWholeConstructor structure (has fix) ==="
sed -n '421,442p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"
echo ""
echo "=== VsWholeConstructor structure (missing fix) ==="
sed -n '1186,1207p' "src/arch/riscv/isa/vector/base/vector_mem.temp.isa"Repository: OpenXiangShan/GEM5
Length of output: 1415
Good fix, but VsWholeConstructor appears to have the same vulnerability.
The addition of %(set_reg_idx_arr)s; at line 426 correctly initializes register index arrays in VlWholeConstructor before they're accessed by generateDisassembly, preventing the heap-buffer-overflow. However, VsWholeConstructor (line 1186) has an identical structure with the same register array members (srcRegIdxArr[8]) and identical micro operation creation pattern, yet it lacks this initialization. Both macro constructors should have this fix for symmetry and correctness:
- VlWholeConstructor (line 421): ✓ Has
%(set_reg_idx_arr)s; - VsWholeConstructor (line 1186): ✗ Missing
%(set_reg_idx_arr)s;
Verify and apply the same initialization to VsWholeConstructor.
🤖 Prompt for AI Agents
In @src/arch/riscv/isa/vector/base/vector_mem.temp.isa around lines 426 - 427,
VsWholeConstructor is missing the same register-index-array initialization added
to VlWholeConstructor, leading to potential heap-buffer-overflow when
generateDisassembly reads srcRegIdxArr[8]; add the same initialization
macro/token `%(set_reg_idx_arr)s;` (or equivalent code that sets/zeros
srcRegIdxArr and dstRegIdxArr) at the start of the VsWholeConstructor
constructor body so its srcRegIdxArr[8] members are initialized before any
micro-op creation or generateDisassembly usage.
|
Logically, printing the disassembly of macrop should be meaningless, because the macrop do not real execute and it just a empty shell, you should fix the Exec debug flags printf, just print the microp not macrop |
|
Furthermore, repairing the disassembly of each macrop would be a very large undertaking. |
tastynoob
added
do not merge
wontfix
|
LGTM! I have checked the upsteam gem5 at the same template. I notice that every template XXXConstructor has initialized the srcRegIdx/dstRegIdx array to make sure that there will be no null pointers when gem5 make disassembly. Could you please fix other similar bugs that does not initialize the srcRegIdx/dstRegIdx array? |
|
Better way to repair it Completely：
|
|
Need review by @seanwang2046 |
|
Hi! The CI failure is not caused by your code - it's a permission issue with fork PRs trying to post comments. I've just merged a fix to xs-dev. Please rebase your branch to pick up the updated workflow: git fetch origin Then re-run the CI and it should pass. Thanks! |
|
I'll force you to integrate it into the main storyline. Thank you so much for your contribution. |
4 participants
Problem
When running gcpt with
--debug-flags=Exec, a segmentation fault is encountered when dumping disassembly ofvl1re64.vinstruction.When compiling gem5. with
--with-asan, the following error is shown.Root cause
The error happens in the following snippet, where accessing
destRegIdxandsrcRegIdxthrows illegal memory access errors.The illegal memory access happens because the
srcReganddstRegfields are uninitialized for this StaticInst type.From the generated StaticInst constructor for VlWhole instruction type in
build/RISCV/arch/riscv/generated/decoder-ns.cc.inc, there is no reg arr initialization.After Fix
After the fix, the register fields are correctly initialized and no illegal memory access fault is observed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.