mruby-sprintf: set the result's length through RSTR_SET_LEN() - #7174

mrb_str_format() sets the result's length by reaching into struct RString itself, choosing between the flags word and as.heap.len. It is the last place outside include/mruby/string.h that writes a string's flags by hand, and its two arms do not write the same thing:

/* Update result string length for embedded strings */
if (RSTRING(result)->flags & MRB_STR_EMBED) {
  mrb_int tmp_n = len;
  RSTRING(result)->flags &= ~MRB_STR_EMBED_LEN_MASK;
  RSTRING(result)->flags |= tmp_n << MRB_STR_EMBED_LEN_SHIFT;
}
else {
  RSTRING(result)->as.heap.len = blen;
}

len is the length of the argument being formatted. blen is how much of the buffer has been written. They are different numbers, and only one of them is the result's length.

/* Keep the result's length at what has been written into it, which
   CHECK() leaves at the size of the whole buffer. */
RSTR_SET_LEN(mrb_str_ptr(result), blen);

Which value belongs there

blen. The line the two arms were split out of read

RSTRING(result)->as.heap.len = blen;

and the split, in 54132e4 ("make embed string when create literals", 2014), took len into the new arm from the assignment two lines above it:

         len = RSTRING_LEN(str);
-        RSTRING(result)->as.heap.len = blen;
+        if (RSTRING(result)->flags & MRB_STR_EMBED) {
+          int tmp_n = len;
+          RSTRING(result)->flags &= ~MRB_STR_EMBED_LEN_MASK;
+          RSTRING(result)->flags |= tmp_n << MRB_STR_EMBED_LEN_SHIFT;
+        } else {
+          RSTRING(result)->as.heap.len = blen;
+        }

Why it has never shown

The embedded arm cannot run. result is allocated at sprintf.c:416-421 with a capacity of at least 120 bytes:

bsiz = (end - p) + 120;
for (const char *scan = p; scan < end; scan++) {
  if (*scan == '%') bsiz += 24;
}
if (bsiz > 4096) bsiz = 4096;
result = mrb_str_new_capa(mrb, bsiz);

mrb_str_new_capa() embeds only what RSTR_EMBEDDABLE_P(capa) accepts, which is 27 bytes on 64-bit and 11 on 32-bit. From there CHECK() only ever grows the string, through mrb_str_resize(), and resize_capa() has no branch back from the heap to embedded. So RSTRING(result)->flags & MRB_STR_EMBED is false on every run.

That is why this is a cleanup rather than a fix: no output changes, on any build. What it removes is a write that would be wrong the moment the branch became reachable, and a second hand-rolled copy of where an embedded length sits.

What the header is left holding

$ git grep -nE 'MRB_STR_(EMBED_LEN|CODERANGE|ENCODING)_(SHIFT|BITS|MASK)' -- ':!include/mruby/string.h'
src/string.c:4053:  mrb_static_assert(RSTRING_EMBED_LEN_MAX < (1 << MRB_STR_EMBED_LEN_BITS),

The one line left is over a width, not a position. git grep -- '->flags' across src/string.c and the string gems comes back with nothing on a struct RString, so after this the flags word is reached through the header's accessors everywhere, and where a field sits is the header's business alone.

Generated code

gcc -O3, against the same objects built from master, over the four builds ci/gcc-clang makes. Both arms stay in the emitted code, since nothing tells the compiler the string is not embedded. What changes is the value the embedded arm would store.

master:

   19a5:	44 89 de             	mov    %r11d,%esi          <- len
   19a8:	25 3f f8 0f 00       	and    $0xff83f,%eax       <- clear bits 6-10
   19ad:	81 e2 ff 0f 00 00    	and    $0xfff,%edx
   19b3:	c1 e6 06             	shl    $0x6,%esi
   19b6:	09 f0                	or     %esi,%eax
   19b8:	25 ff ff 0f 00       	and    $0xfffff,%eax
   19bd:	89 c6                	mov    %eax,%esi
   19bf:	c1 e6 0c             	shl    $0xc,%esi
   19c2:	09 f2                	or     %esi,%edx
   19c4:	89 57 08             	mov    %edx,0x8(%rdi)

this branch:

   19a5:	8b 74 24 10          	mov    0x10(%rsp),%esi     <- blen
   19a9:	25 3f f8 0f 00       	and    $0xff83f,%eax
   19ae:	81 e2 ff 0f 00 00    	and    $0xfff,%edx
   19b4:	c1 e6 06             	shl    $0x6,%esi
   19b7:	09 f0                	or     %esi,%eax
   19b9:	25 ff ff 0f 00       	and    $0xfffff,%eax
   19be:	89 c6                	mov    %eax,%esi
   19c0:	c1 e6 0c             	shl    $0xc,%esi
   19c3:	09 f2                	or     %esi,%edx
   19c5:	89 57 08             	mov    %edx,0x8(%rdi)

One operand, and it is the whole of the change. %r11d and 0x10(%rsp) are len and blen, which the bound CHECK() computes a few blocks on names together:

   1a78:	48 8b 44 24 10       	mov    0x10(%rsp),%rax
   1a7d:	4e 8d 24 18          	lea    (%rax,%r11,1),%r12   <- blen + len
   1a81:	4d 39 ec             	cmp    %r13,%r12            <- against bsiz

The byte the wider operand costs comes back out of alignment padding further down, where nopl 0x0(%rax) becomes nopl (%rax), so nothing moves in size:

Testing

rake -m test, all green with 0 KO, 0 crash, 0 warnings:

The 32-bit build is worth having here because it is the one where RSTRING_EMBED_LEN_MAX is 11 rather than 27, so it is the closest any build comes to the arm being reachable. It is not close: the capacity is still at least 120. build_config/host-m32.rb needs a multilib toolchain, so that build is full-core with the compiler set to i686-linux-gnu-gcc instead.

Relation to #7173

This is the follow-up noted in the review there. It does not depend on it: this branch is on master, and the two touch different files.

Summary by CodeRabbit

• Bug Fixes
  • Improved string-formatting length handling for %s and %p conversions.
  • Ensured formatted output consistently reports the correct number of bytes written.