Skip to content

Commit b66d2a4

Browse files
committed
Fix array self-assignment bug that was clearing arrays
When performing array self-assignment like '@arr = @arr', the array was being cleared before the values could be copied, resulting in an empty array. This particularly affected operations like 'chop(@stuff = @stuff)'. The fix detects when the source list contains references to the target array and creates a defensive copy before clearing. This ensures proper behavior for patterns like: - @arr = @arr (simple self-assignment) - @x = (@x, 3, 4) (array at beginning) - @x = (1, 2, @x) (array at end) - @x = (1, @x, 4, 5) (array in middle) - @x = (1, @x, @x, 4) (array appears multiple times) Impact: Reduces op/chop.t failures from 16 to 4 (75% reduction) Also includes: - Documentation for dynamic eval warning handler issue (op/infnan.t) - Documentation for empty file slurp mode issue (op/closure.t) - Enhanced debugging strategy documentation
1 parent c3fc71c commit b66d2a4

File tree

4 files changed

+293
-2
lines changed

4 files changed

+293
-2
lines changed
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
# Fix Dynamic Eval String Warning Handler Inheritance
2+
3+
## Objective
4+
Fix 30 test failures in op/infnan.t where dynamically constructed eval strings don't inherit the `$SIG{__WARN__}` handler, preventing numeric warnings from being captured.
5+
6+
## Current Status
7+
- **Test file:** t/op/infnan.t
8+
- **Failures:** 30 tests (all "numify warn" tests)
9+
- **Pass rate:** Currently 90% (would be ~93% after fix)
10+
- **Pattern:** All failures are for malformed Inf/NaN strings like "infxy", "nanxy", "nan34"
11+
12+
## Root Cause Analysis
13+
14+
### The Problem
15+
Warnings are not captured when eval strings are **dynamically constructed** (using string concatenation), even though they work correctly for:
16+
1. Literal eval strings: `eval 'my $x = "infxy" + 1'`
17+
2. Eval blocks: `eval { my $x = "infxy" + 1 }`
18+
3. Dynamically constructed strings: `eval '$a = "' . $value . '" + 1'`
19+
20+
### Evidence
21+
Test case demonstrates the issue clearly:
22+
```perl
23+
$SIG{__WARN__} = sub { $w = shift };
24+
25+
# Works - captures warning
26+
eval 'my $x = "infxy" + 1';
27+
28+
# Fails - doesn't capture warning
29+
my $str = '$a = "infxy" + 1';
30+
eval $str;
31+
```
32+
33+
### Technical Investigation
34+
1. Warnings ARE being generated correctly (seen with --disassemble)
35+
2. The WarnDie.warn() method IS being called
36+
3. The issue is that dynamically compiled eval code doesn't inherit the current `$SIG{__WARN__}` handler
37+
38+
## Why Simple Fixes Don't Work
39+
40+
### Attempt 1: Check evalStringHelper context
41+
- The evalStringHelper method creates a new EmitterContext from a snapshot
42+
- The symbolTable snapshot might not include the current signal handlers
43+
- Signal handlers are stored in GlobalVariable, not in the symbol table
44+
45+
### Attempt 2: Copy signal handlers
46+
- Would require passing current %SIG state to evalStringHelper
47+
- This breaks the caching mechanism (cacheKey would need to include %SIG state)
48+
- Performance impact would be significant
49+
50+
## Implementation Strategy
51+
52+
### Phase 1: Understand Current Architecture
53+
1. How are signal handlers stored? (GlobalVariable.getGlobalHash("main::SIG"))
54+
2. How does eval string compilation access globals?
55+
3. Why do literal eval strings work but dynamic ones don't?
56+
57+
### Phase 2: Implement Fix
58+
**Option A: Runtime Context Inheritance**
59+
- Pass current runtime context to dynamically compiled code
60+
- Ensure %SIG is accessible from compiled eval code
61+
- Maintain compatibility with caching
62+
63+
**Option B: Compile-time Binding**
64+
- Bind signal handlers at eval compilation time
65+
- Store handler references in compiled code
66+
- May require changes to EmitterContext
67+
68+
### Phase 3: Testing
69+
1. Verify all 30 infnan.t tests pass
70+
2. Check for regressions in other eval-related tests
71+
3. Test performance impact on eval string compilation
72+
73+
## Testing Strategy
74+
75+
### Minimal Test Case
76+
```perl
77+
#!/usr/bin/perl
78+
use strict;
79+
use warnings;
80+
81+
my $captured = '';
82+
$SIG{__WARN__} = sub { $captured = $_[0] };
83+
84+
# Test dynamic eval
85+
my $code = '$x = "infxy" + 1';
86+
eval $code;
87+
88+
if ($captured =~ /isn't numeric/) {
89+
print "PASS\n";
90+
} else {
91+
print "FAIL: Warning not captured\n";
92+
}
93+
```
94+
95+
## Expected Impact
96+
- **Tests fixed:** 30 tests in op/infnan.t
97+
- **Pass rate improvement:** 90% → 93% (+3%)
98+
- **Side benefits:** All dynamic eval string warning capture would be fixed
99+
- **Potential regressions:** None expected if implemented correctly
100+
101+
## Complexity Assessment
102+
- **Estimated effort:** 2-4 hours
103+
- **Risk level:** Medium (affects core eval mechanism)
104+
- **Files to modify:**
105+
- RuntimeCode.java (evalStringHelper)
106+
- EmitterContext.java (context passing)
107+
- Possibly WarnDie.java (signal handler lookup)
108+
109+
## Recommendation
110+
**Priority:** HIGH (30 tests, clear pattern)
111+
**Approach:** Defer for now and move to simpler targets
112+
**Reason:** Complex architectural issue requiring deep understanding of eval compilation
113+
114+
This is a valuable fix but requires careful implementation to avoid breaking the eval caching mechanism or causing performance regressions. The issue is well-documented here for future implementation.
115+
116+
## Alternative Quick Fix
117+
If we just want the tests to pass without fixing the underlying issue:
118+
- Modify the test to use literal eval strings instead of dynamic construction
119+
- This would be a test-only change, not fixing the actual bug
120+
- Not recommended as it masks a real compatibility issue
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
# Fix Empty File Read in Slurp Mode
2+
3+
## Objective
4+
Fix 18 test failures in op/closure.t where reading empty files with `local $/` (slurp mode) returns `undef` instead of an empty string.
5+
6+
## Current Status
7+
- **Test file:** t/op/closure.t
8+
- **Failures:** 18 tests (all "STDERR is silent" tests)
9+
- **Pass rate:** Currently 92% (would be ~95% after fix)
10+
11+
## Root Cause Analysis
12+
13+
### The Problem
14+
When reading an empty file in scalar context with `local $/` (slurp mode), PerlOnJava returns `undef` instead of an empty string.
15+
16+
### Test Pattern
17+
```perl
18+
# In closure.t around line 459
19+
{ local $/; open IN, $errfile; $errors = <IN>; close IN }
20+
# Test expects: $errors eq '' (empty string)
21+
# PerlOnJava returns: $errors is undef
22+
```
23+
24+
### Technical Investigation
25+
1. **Slurp mode should be triggered by `local $/`** which makes `$/` undefined
26+
2. **The slurp mode code path exists** at lines 60-79 in Readline.java
27+
3. **The fix was attempted** at line 79 to return empty string instead of undef
28+
4. **BUT the fix didn't work**, suggesting `local $/` might not trigger the slurp mode path
29+
30+
### Key Discovery
31+
The issue might be in how `$/` is handled:
32+
- `local $/` might not create an `InputRecordSeparator` with `isSlurpMode()` returning true
33+
- Or the `rsScalar instanceof InputRecordSeparator` check might be failing
34+
- The code might be taking the normal separator path instead
35+
36+
## Why Simple Fixes Don't Work
37+
38+
### Attempt 1: Change all EOF returns to empty string
39+
- **Problem:** Breaks list context reading which depends on `undef` to terminate loops
40+
- **Result:** Tests hang in infinite loops
41+
42+
### Attempt 2: Fix only slurp mode
43+
- **Problem:** The slurp mode code path might not be triggered
44+
- **Result:** No effect on the test failures
45+
46+
## Implementation Strategy
47+
48+
### Phase 1: Understand `$/` Handling
49+
1. How does `local $/` affect the global variable?
50+
2. Does it create an `InputRecordSeparator` object?
51+
3. What value does `getGlobalVariable("main::/")` return?
52+
53+
### Phase 2: Fix Detection Logic
54+
**Option A: Fix slurp mode detection**
55+
- Ensure `local $/` properly triggers slurp mode
56+
- May need to check for undefined `$/` value
57+
58+
**Option B: Handle undefined `$/` specially**
59+
- Check if `rsScalar` is undefined or empty
60+
- Treat as slurp mode
61+
62+
### Phase 3: Implement Targeted Fix
63+
- Only change behavior for first read of empty file in slurp mode
64+
- Maintain `undef` for subsequent reads to preserve list context
65+
66+
## Testing Strategy
67+
68+
### Minimal Test Case
69+
```perl
70+
#!/usr/bin/perl
71+
use strict;
72+
use warnings;
73+
74+
# Create empty file
75+
open(my $fh, '>', 'empty.tmp');
76+
close $fh;
77+
78+
# Test slurp mode
79+
{
80+
local $/;
81+
open(IN, 'empty.tmp');
82+
my $content = <IN>;
83+
close IN;
84+
85+
if (defined $content) {
86+
print "PASS: content is '", $content, "'\n";
87+
} else {
88+
print "FAIL: content is undef\n";
89+
}
90+
}
91+
92+
unlink 'empty.tmp';
93+
```
94+
95+
## Expected Impact
96+
- **Tests fixed:** 18 tests in op/closure.t
97+
- **Pass rate improvement:** 92% → 95% (+3%)
98+
- **Side benefits:** All slurp mode empty file reads would be fixed
99+
100+
## Complexity Assessment
101+
- **Estimated effort:** 1-2 hours
102+
- **Risk level:** Medium (affects core IO operations)
103+
- **Files to modify:**
104+
- Readline.java (detection logic)
105+
- Possibly InputRecordSeparator.java
106+
107+
## Recommendation
108+
**Priority:** HIGH (18 tests, clear pattern)
109+
**Approach:** Investigate `$/` handling first, then implement targeted fix
110+
**Reason:** Well-defined issue with clear expected behavior
111+
112+
## Alternative Quick Fix
113+
If investigation is too complex:
114+
- Special case empty file reads when separator is empty/undefined
115+
- Check at the beginning of readline() method
116+
- Return empty string for first read, track state for subsequent reads

dev/prompts/high-yield-test-analysis-strategy.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,34 @@ grep -r "methodName\(" src/main/java/
370370
grep -B 5 -A 10 "pattern" file.java
371371
```
372372

373+
### Technique 7: Killing Hanging Test Processes
374+
**Pattern:** When a test hangs (infinite loop or deadlock), find and kill the process
375+
376+
```bash
377+
# Find hanging jperl processes
378+
ps aux | grep "jperl test_name"
379+
380+
# Find Java processes running tests (more reliable)
381+
ps aux | grep -E "java.*test_name|PerlOnJava.*test"
382+
383+
# Kill the process (use PID from ps output)
384+
kill -9 <PID>
385+
386+
# Alternative: Kill by pattern (be careful!)
387+
pkill -f "jperl test_name"
388+
```
389+
390+
**Red Flags of a hanging test:**
391+
- Test doesn't complete after several seconds
392+
- High CPU usage (>100% in ps output)
393+
- List context operations that depend on `undef` to terminate
394+
- Infinite loops in regex or string operations
395+
396+
**Common causes:**
397+
- Changing `undef` returns to empty strings breaks list context loops
398+
- Incorrect EOF handling in IO operations
399+
- Regex engine infinite loops with certain patterns
400+
373401
## Common PerlOnJava Patterns
374402

375403
### Pattern 1: Context Parameters

src/main/java/org/perlonjava/runtime/RuntimeArray.java

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,8 +451,35 @@ public RuntimeArray set(RuntimeScalar value) {
451451
public RuntimeArray setFromList(RuntimeList list) {
452452
return switch (type) {
453453
case PLAIN_ARRAY -> {
454-
this.elements.clear();
455-
list.addToArray(this);
454+
// Check if the list contains references to this array's elements
455+
// If so, we need to save the values before clearing
456+
boolean needsCopy = false;
457+
for (RuntimeBase elem : list.elements) {
458+
if (elem instanceof RuntimeArray && elem == this) {
459+
needsCopy = true;
460+
break;
461+
}
462+
}
463+
464+
if (needsCopy) {
465+
// Make a defensive copy of the list before clearing
466+
RuntimeList listCopy = new RuntimeList();
467+
for (RuntimeBase elem : list.elements) {
468+
if (elem instanceof RuntimeArray && elem == this) {
469+
// Copy this array's current contents
470+
for (RuntimeScalar s : this.elements) {
471+
listCopy.elements.add(s);
472+
}
473+
} else {
474+
listCopy.elements.add(elem);
475+
}
476+
}
477+
this.elements.clear();
478+
listCopy.addToArray(this);
479+
} else {
480+
this.elements.clear();
481+
list.addToArray(this);
482+
}
456483
yield this;
457484
}
458485
case AUTOVIVIFY_ARRAY -> {

0 commit comments

Comments
 (0)