Skip to content

Commit

Permalink
Fix string localization bug in some IO read procedures (chapel-lang#2…
Browse files Browse the repository at this point in the history
…4444)

Fix a bug where `readTo`, `readThrough` and `readLine` were not handling
non-local string/bytes arguments properly. These methods all rely on
`readStringBytesData`, which accessed a string/bytes argument's buffer
regardless of whether it was declared locally. It now checks whether the
argument is local, and if not, creates a local temporary string/bytes to
complete the operation.

New tests are added to ensure the methods behave properly when called
with non-local arguments (note: the multilocale behavior of `readLine`
is locked in by some tests added in
chapel-lang#24323 — this PR removes the
`notest`s from those).

resolves: chapel-lang#24443

- [x] local paratest
- [x] gasnet paratest

[ reviewed by @ShreyasKhandekar ] - thanks!
  • Loading branch information
jeremiah-corrado authored Feb 26, 2024
2 parents 74482f5 + 6f42f56 commit 5704ccf
Show file tree
Hide file tree
Showing 12 changed files with 81 additions and 14 deletions.
32 changes: 18 additions & 14 deletions modules/standard/IO.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -7235,37 +7235,41 @@ proc fileReader.readline(ref arg: ?t): bool throws where t==string || t==bytes {
// Passing -1 to 'nCodepoints' tells this function to compute the number
// of codepoints itself, and store the result in 'cachedNumCodepoints'.
@chpldoc.nodoc
proc readStringBytesData(ref s /*: string or bytes*/,
proc readStringBytesData(ref s: ?t /*: string or bytes*/,
_channel_internal:qio_channel_ptr_t,
nBytes: int,
nCodepoints: int): errorCode {
import BytesStringCommon;
var sLoc: t;
ref sLocal = if s.locale == here then s else sLoc;

BytesStringCommon.resizeBuffer(s, nBytes);
BytesStringCommon.resizeBuffer(sLocal, nBytes);

// TODO: if the fileReader is working with non-UTF-8 data
// (which is a feature not yet implemented at all)
// this would need to call a read than can do character set conversion
// this would need to call a read that can do character set conversion
// in the event that s.type == string.

var len:c_ssize_t = nBytes.safeCast(c_ssize_t);
var err = qio_channel_read_amt(false, _channel_internal, s.buff, len);
var err = qio_channel_read_amt(false, _channel_internal, sLocal.buff, len);
if !err {
s.buffLen = nBytes;
if nBytes != 0 then s.buff[nBytes] = 0; // include null-byte
if s.type == string {
sLocal.buffLen = nBytes;
if nBytes != 0 then sLocal.buff[nBytes] = 0; // include null-byte
if t == string {
if nCodepoints == -1
then s.cachedNumCodepoints = BytesStringCommon.countNumCodepoints(s);
else s.cachedNumCodepoints = nCodepoints;
s.hasEscapes = false;
then sLocal.cachedNumCodepoints = BytesStringCommon.countNumCodepoints(sLocal);
else sLocal.cachedNumCodepoints = nCodepoints;
sLocal.hasEscapes = false;
}
} else {
s.buffLen = 0;
if s.type == string {
s.cachedNumCodepoints = 0;
s.hasEscapes = false;
sLocal.buffLen = 0;
if t == string {
sLocal.cachedNumCodepoints = 0;
sLocal.hasEscapes = false;
}
}

if s.locale != here then s <=> sLoc;
return err;
}

Expand Down
20 changes: 20 additions & 0 deletions test/library/standard/IO/readThrough/readThroughML.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
use IO;

var fr = openReader("listInput.txt", locking=false);

test(fr, Locales[0], Locales[1]);
test(fr, Locales[1], Locales[0]);
test(fr, Locales[1], Locales[1]);
test(fr, Locales[1], Locales[2]);

proc test(fr, L1: locale, L2: locale) {
on L1 {
var s = "", b = b"";
on L2 {
fr.readThrough(",", s);
fr.readThrough(b",", b);
}
writeln(s);
writeln(b);
}
}
8 changes: 8 additions & 0 deletions test/library/standard/IO/readThrough/readThroughML.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
1,
2,
3,
4,
5,
6,
7,
8,
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
3
1 change: 1 addition & 0 deletions test/library/standard/IO/readThrough/readThroughML.skipif
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CHPL_COMM==none
1 change: 1 addition & 0 deletions test/library/standard/IO/readTo/listInput.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1,2,3,4,5,6,7,8,
22 changes: 22 additions & 0 deletions test/library/standard/IO/readTo/readToML.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
use IO;

var fr = openReader("listInput.txt", locking=false);

test(fr, Locales[0], Locales[1]);
test(fr, Locales[1], Locales[0]);
test(fr, Locales[1], Locales[1]);
test(fr, Locales[1], Locales[2]);

proc test(fr, L1: locale, L2: locale) {
on L1 {
var s = "", b = b"";
on L2 {
fr.readTo(",", s);
fr.matchLiteral(",");
fr.readTo(b",", b);
fr.matchLiteral(",");
}
writeln(s);
writeln(b);
}
}
8 changes: 8 additions & 0 deletions test/library/standard/IO/readTo/readToML.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
1
2
3
4
5
6
7
8
1 change: 1 addition & 0 deletions test/library/standard/IO/readTo/readToML.numlocales
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
3
1 change: 1 addition & 0 deletions test/library/standard/IO/readTo/readToML.skipif
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CHPL_COMM==none
Empty file.
Empty file.

0 comments on commit 5704ccf

Please sign in to comment.