Skip to content

Commit 1bcd398

Browse files
aardvark179aardvark179
committed
[GR-38888] Refactor rb_sprintf to avoid intermittent failure.
PullRequest: truffleruby/3410
2 parents 7314b67 + 87cced5 commit 1bcd398

File tree

9 files changed

+103
-48
lines changed

9 files changed

+103
-48
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ Changes:
2121
* No more conversion between Java Strings and Ruby Strings at the interop boundary.
2222
* Removed `Truffle::Interop.{import_without_conversion,export_without_conversion}` (use `Polyglot.{import,export}` instead).
2323
* Removed `Truffle::Interop.members_without_conversion` (use `Truffle::Interop.members` instead).
24+
* Refactored internals of `rb_sprintf` to simplify handling of `VALUE`s in common cases (@aardvark179).
2425

2526
# 22.2.0
2627

‎lib/cext/ABI_check.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
6
1+
7

‎spec/nativeconversion.mspec

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,11 @@ class NativeHandleChecker
2121
"C-API Kernel function rb_rescue raises an exception if any exception is raised inside the 'rescue function'" => 1,
2222
"C-API Kernel function rb_rescue makes $! available only during the 'rescue function' execution" => 1,
2323
"CApiObject rb_obj_call_init sends #initialize" => 2,
24-
"C-API String function rb_sprintf formats a string VALUE using to_s if sign not specified in format" => 1,
25-
"C-API String function rb_sprintf formats a string VALUE using inspect if sign specified in format" => 1,
26-
"C-API String function rb_sprintf formats a TrueClass VALUE as `TrueClass` if sign not specified in format" => 1,
27-
"C-API String function rb_sprintf truncates a VALUE string to a supplied precision if that is shorter than the VALUE string" => 1,
28-
"C-API String function rb_sprintf does not truncates a VALUE string to a supplied precision if that is longer than the VALUE string" => 1,
29-
"C-API String function rb_sprintf pads a VALUE string to a supplied width if that is longer than the VALUE string" => 1,
3024
"C-API String function rb_sprintf can format a string VALUE as a pointer and gives the same output as sprintf in C" => 1,
3125
"C-API String function rb_string_value_cstr returns a non-null pointer for a simple string" => 1,
3226
"C-API String function rb_string_value_cstr returns a non-null pointer for a UTF-16 string" => 1,
3327
"C-API String function rb_string_value_cstr raises an error if a string contains a null" => 1,
3428
"C-API String function rb_string_value_cstr raises an error if a UTF-16 string contains a null" => 1,
35-
"CApiWrappedTypedStruct throws an exception for a wrong type" => 1,
3629
"C-API Util function rb_scan_args assigns Hash arguments" => 1,
3730
"C-API Util function rb_scan_args assigns required and Hash arguments" => 1,
3831
"C-API Util function rb_scan_args assigns required, optional, splat, post-splat, Hash and block arguments" => 1,

‎src/main/c/cext/printf.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ enum printf_arg_types {
118118
TYPE_PTRDIFF_T,
119119
TYPE_STRING,
120120
TYPE_POINTER,
121+
TYPE_VALUE,
121122
TYPE_SCHAR = 0x11,
122123
TYPE_SSHORT,
123124
TYPE_SINT,
@@ -185,6 +186,9 @@ VALUE rb_tr_get_sprintf_args(va_list args, VALUE types) {
185186
case TYPE_SLONGLONG:
186187
val = LL2NUM(va_arg(args, long long));
187188
break;
189+
case TYPE_VALUE:
190+
val = va_arg(args, VALUE);
191+
break;
188192
default:
189193
{
190194
char *err_str;

‎src/main/java/org/truffleruby/cext/CExtNodes.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1733,19 +1733,19 @@ public abstract static class RBSprintfFormatNode extends CoreMethodArrayArgument
17331733
protected Object typesCached(VirtualFrame frame, Object format,
17341734
@CachedLibrary(limit = "LIBSTRING_CACHE") RubyStringLibrary libFormat,
17351735
@Cached("libFormat.getRope(format)") Rope cachedFormatRope,
1736-
@Cached("compileArgTypes(format, libFormat)") Object cachedTypes,
1736+
@Cached("compileArgTypes(format, libFormat)") RubyArray cachedTypes,
17371737
@Cached RopeNodes.EqualNode equalNode) {
17381738
return cachedTypes;
17391739
}
17401740

17411741
@Specialization(guards = "libFormat.isRubyString(format)")
1742-
protected Object typesUncachd(VirtualFrame frame, Object format,
1742+
protected RubyArray typesUncachd(VirtualFrame frame, Object format,
17431743
@CachedLibrary(limit = "LIBSTRING_CACHE") RubyStringLibrary libFormat) {
17441744
return compileArgTypes(format, libFormat);
17451745
}
17461746

17471747
@TruffleBoundary
1748-
protected Object compileArgTypes(Object format, RubyStringLibrary libFormat) {
1748+
protected RubyArray compileArgTypes(Object format, RubyStringLibrary libFormat) {
17491749
try {
17501750
return new RBSprintfCompiler(getLanguage(), this)
17511751
.typeList(libFormat.getRope(format), getContext(), getLanguage());

‎src/main/java/org/truffleruby/core/format/rbsprintf/RBSprintfCompiler.java

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public RootCallTarget compile(Rope format, Object stringReader) {
4747

4848
private static int SIGN = 0x10;
4949

50-
public Object typeList(Rope format, RubyContext context, RubyLanguage language) {
50+
public RubyArray typeList(Rope format, RubyContext context, RubyLanguage language) {
5151
final RBSprintfSimpleParser parser = new RBSprintfSimpleParser(bytesToChars(format.getBytes()), false);
5252
final List<RBSprintfConfig> configs = parser.parse();
5353
final int[] types = new int[3 * configs.size()]; // Ensure there is enough space for the argument types that might be in the format string.
@@ -76,13 +76,17 @@ public Object typeList(Rope format, RubyContext context, RubyLanguage language)
7676
types[config.getWidth()] = FormatArgumentType.INT.ordinal();
7777
highWaterMark = Math.max(highWaterMark, config.getWidth());
7878
}
79-
switch (config.getFormat()) {
80-
case 'd':
81-
case 'i':
82-
typeInt = config.getFormatArgumentType().ordinal() | SIGN;
83-
break;
84-
default:
85-
typeInt = config.getFormatArgumentType().ordinal();
79+
if (config.getFormatType() == RBSprintfConfig.FormatType.RUBY_VALUE) {
80+
typeInt = RBSprintfConfig.FormatArgumentType.VALUE.ordinal();
81+
} else {
82+
switch (config.getFormat()) {
83+
case 'd':
84+
case 'i':
85+
typeInt = config.getFormatArgumentType().ordinal() | SIGN;
86+
break;
87+
default:
88+
typeInt = config.getFormatArgumentType().ordinal();
89+
}
8690
}
8791
if (config.getAbsoluteArgumentIndex() != null) {
8892
typePos = config.getAbsoluteArgumentIndex() - 1; //Parameters are 1 indexed, but our array is 0 indexed.

‎src/main/java/org/truffleruby/core/format/rbsprintf/RBSprintfConfig.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ public enum FormatArgumentType {
3737
PTRDIFF_T,
3838
STRING,
3939
POINTER,
40+
VALUE,
4041
}
4142

4243
private boolean literal = false;

‎src/main/java/org/truffleruby/core/format/rbsprintf/RBSprintfSimpleParser.java

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@
1010
package org.truffleruby.core.format.rbsprintf;
1111

1212
import java.util.ArrayList;
13+
import java.util.HashSet;
1314
import java.util.List;
1415

1516
import org.truffleruby.core.format.exceptions.InvalidFormatException;
1617
import org.truffleruby.core.format.rbsprintf.RBSprintfConfig.FormatArgumentType;
17-
import org.truffleruby.language.RubyGuards;
1818

1919
public class RBSprintfSimpleParser {
2020

@@ -267,6 +267,7 @@ public List<RBSprintfConfig> parse() {
267267
if (i + 1 < this.source.length && this.source[i + 1] == '\u000B' &&
268268
config.getFormatArgumentType() == FormatArgumentType.LONG) {
269269
config.setFormatType(RBSprintfConfig.FormatType.RUBY_VALUE);
270+
config.setFormatArgumentType(RBSprintfConfig.FormatArgumentType.VALUE);
270271
i += 2;
271272
} else {
272273
config.setFormatType(RBSprintfConfig.FormatType.INTEGER);
@@ -328,14 +329,7 @@ public List<RBSprintfConfig> parse() {
328329
}
329330
}
330331

331-
return configs;
332-
}
333-
334-
private static void checkHash(Object[] arguments) {
335-
if (arguments.length != 1 ||
336-
!RubyGuards.isRubyHash(arguments[0])) {
337-
throw new InvalidFormatException("one hash required");
338-
}
332+
return normalizeArgumentTypes(configs);
339333
}
340334

341335
private static void checkNextArg(ArgType argType, int nextArgumentIndex) {
@@ -359,15 +353,6 @@ private static void checkPosArg(ArgType posarg, int nextArgumentIndex) {
359353
}
360354
}
361355

362-
private static void checkNameArg(ArgType argType, char[] name) {
363-
if (argType == ArgType.UNNUMBERED) {
364-
throw new InvalidFormatException("named" + new String(name) + " after unnumbered(%d)");
365-
}
366-
if (argType == ArgType.NUMBERED) {
367-
throw new InvalidFormatException("named" + new String(name) + " after numbered");
368-
}
369-
}
370-
371356
private enum ArgType {
372357
NONE,
373358
NUMBERED,
@@ -467,4 +452,52 @@ private static byte[] charsToBytes(char[] chars) {
467452
return bytes;
468453
}
469454

455+
private List<RBSprintfConfig> normalizeArgumentTypes(List<RBSprintfConfig> configs) {
456+
// We want to check for any uses of RUBY_VALUEs conflicting with uses of the raw numerical representation.
457+
RBSprintfConfig[] inPosition = new RBSprintfConfig[configs.size()];
458+
HashSet<RBSprintfConfig> conflicts = new HashSet<>();
459+
int pos = 0;
460+
for (var config : configs) {
461+
int typePos;
462+
if (config.getAbsoluteArgumentIndex() != null) {
463+
typePos = config.getAbsoluteArgumentIndex() - 1; // Parameters are 1 indexed, but our array is 0 indexed.
464+
} else {
465+
typePos = pos++;
466+
}
467+
if (inPosition[typePos] == null) {
468+
inPosition[typePos] = config;
469+
} else {
470+
if (config.getFormatArgumentType() == FormatArgumentType.VALUE) {
471+
inPosition[typePos] = config;
472+
}
473+
conflicts.add(config);
474+
conflicts.add(inPosition[typePos]);
475+
}
476+
}
477+
// If we found any conflicts then change them
478+
// This can only happen if all the configs have absolute argument positions.
479+
if (conflicts.size() > 0) {
480+
for (var config : inPosition) {
481+
if (config.getFormatArgumentType() == FormatArgumentType.VALUE &&
482+
conflicts.contains(config)) {
483+
boolean typeConflict = false;
484+
ArrayList<RBSprintfConfig> toFix = new ArrayList<>();
485+
for (var conflict : conflicts) {
486+
if (conflict.getAbsoluteArgumentIndex() == config.getAbsoluteArgumentIndex()) {
487+
toFix.add(conflict);
488+
typeConflict |= conflict.getFormatArgumentType() != config.getFormatArgumentType();
489+
}
490+
}
491+
if (typeConflict) {
492+
for (var fixConfig : toFix) {
493+
fixConfig.setFormatArgumentType(FormatArgumentType.LONG);
494+
}
495+
}
496+
}
497+
}
498+
return configs;
499+
} else {
500+
return configs;
501+
}
502+
}
470503
}

‎src/main/java/org/truffleruby/core/format/rbsprintf/RBSprintfSimpleTreeBuilder.java

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,11 @@
2727
import org.truffleruby.core.format.format.FormatIntegerBinaryNodeGen;
2828
import org.truffleruby.core.format.format.FormatIntegerNodeGen;
2929
import org.truffleruby.core.format.printf.PrintfSimpleTreeBuilder;
30+
import org.truffleruby.core.format.rbsprintf.RBSprintfConfig.FormatArgumentType;
3031
import org.truffleruby.core.format.read.SourceNode;
3132
import org.truffleruby.core.format.read.array.ReadArgumentIndexValueNodeGen;
3233
import org.truffleruby.core.format.read.array.ReadIntegerNodeGen;
34+
import org.truffleruby.core.format.read.array.ReadStringNodeGen;
3335
import org.truffleruby.core.format.read.array.ReadCStringNodeGen;
3436
import org.truffleruby.core.format.read.array.ReadCValueNodeGen;
3537
import org.truffleruby.core.format.read.array.ReadValueNodeGen;
@@ -266,17 +268,34 @@ private void buildTree() {
266268
case RUBY_VALUE: {
267269
final String conversionMethodName = config.isPlus() ? "inspect" : "to_s";
268270
final FormatNode conversionNode;
269-
conversionNode = ToStringNodeGen
270-
.create(
271-
true,
272-
conversionMethodName,
273-
false,
274-
EMPTY_ROPE,
275-
config.isPlus(),
276-
(config.getAbsoluteArgumentIndex() == null)
277-
? (ReadCValueNodeGen
278-
.create(ReadIntegerNodeGen.create(new SourceNode())))
279-
: ReadCValueNodeGen.create(valueNode));
271+
if (config.getFormatArgumentType() == FormatArgumentType.VALUE) {
272+
if (config.getAbsoluteArgumentIndex() == null) {
273+
conversionNode = ReadStringNodeGen
274+
.create(
275+
true,
276+
conversionMethodName,
277+
false,
278+
EMPTY_ROPE,
279+
config.isPlus(),
280+
new SourceNode());
281+
} else {
282+
conversionNode = ToStringNodeGen
283+
.create(true, conversionMethodName, false, EMPTY_ROPE, config.isPlus(),
284+
valueNode);
285+
}
286+
} else {
287+
conversionNode = ToStringNodeGen
288+
.create(
289+
true,
290+
conversionMethodName,
291+
false,
292+
EMPTY_ROPE,
293+
config.isPlus(),
294+
(config.getAbsoluteArgumentIndex() == null)
295+
? (ReadCValueNodeGen
296+
.create(ReadIntegerNodeGen.create(new SourceNode())))
297+
: ReadCValueNodeGen.create(valueNode));
298+
}
280299
if (config.getWidth() != null || config.isWidthStar() || config.getPrecision() != null ||
281300
config.isPrecisionStar()) {
282301
node = WritePaddedBytesNodeGen

0 commit comments

Comments
 (0)