string.h: give the flags word its final field order - #7173

The flags word of struct RString carries four fields, and two of them went where the word had room at the time rather than where the layout wants them. The coderange took bits 4-5 in e6eaf48, the lowest pair free once the three bits it replaced were gone. The encoding index took bits 13-14 in 839b164, the lowest pair free while the coderange was still those three separate bits.

What that leaves is free bits in two pieces and the field most likely to widen sitting in the middle of the word:

This PR gives the four fields the order they keep. All three commits are include/mruby/string.h alone.

What goes into include/mruby/string.h

Three shift constants, and nothing else:

#define MRB_STR_EMBED_LEN_SHIFT 4      /* was 6 */
#define MRB_STR_CODERANGE_SHIFT 9      /* was 4 */
#define MRB_STR_ENCODING_SHIFT 11      /* was 13 */

The widths stay as they are and every mask is derived from a width and a shift, so a field moves by its _SHIFT and nothing else has to be told.

Why this order

A field at the top of what is used grows into the free bits above it, which is a change to its own MRB_STR_*_BITS and nothing besides. A field anywhere else pushes every field above it along. So the order is the one that puts what is likeliest to widen at the top and leaves what is free in one run.

Widening the encoding index is therefore MRB_STR_ENCODING_BITS 2 to 3, with bit 13 coming out of the free run and no other field moving.

Nothing outside the header names a bit

$ git grep -nE 'MRB_STR_(EMBED_LEN|CODERANGE|ENCODING)_(SHIFT|BITS|MASK)' -- ':!include/mruby/string.h'
mrbgems/mruby-sprintf/src/sprintf.c:624:            RSTRING(result)->flags &= ~MRB_STR_EMBED_LEN_MASK;
mrbgems/mruby-sprintf/src/sprintf.c:625:            RSTRING(result)->flags |= tmp_n << MRB_STR_EMBED_LEN_SHIFT;
src/string.c:4053:  mrb_static_assert(RSTRING_EMBED_LEN_MAX < (1 << MRB_STR_EMBED_LEN_BITS),

sprintf.c is the one place outside this header that writes a field by hand, and it goes through the mask and the shift, so the move is transparent to it. The mrb_static_assert is over the width, which does not change.

Everywhere else reaches a field through RSTR_EMBED_LEN / RSTR_SET_EMBED_LEN, RSTR_CODERANGE / RSTR_CODERANGE_SET, or RSTR_ENCODING / RSTR_ENCODING_SET. git grep -- '->flags' over src/string.c and the string gems comes back with the three sprintf.c lines above and nothing else.

The one thing the move makes worse

RSTR_SET_EMBED_LEN shifts a length in without masking it to the field's width:

(s)->flags |= (tmp_n) << MRB_STR_EMBED_LEN_SHIFT;

That is deliberate, and unlike RSTR_CODERANGE_SET and RSTR_ENCODING_SET it should stay so: a length is read at run time, so a mask there is an instruction rather than something a constant folds away.

What changes is where an over-long length would land. With the field at bits 6-10 the overflow went into bit 11 and up, free at the time; at bits 4-8 it goes into the coderange and then the encoding index. It is the one way this PR makes a mistake cost more than it did, so the third commit says out loud what the write needs, the way ARY_SET_LEN says it for the same field in include/mruby/array.h:

mrb_assert(tmp_n <= (size_t)RSTRING_EMBED_LEN_MAX);

Nothing in tree reaches it, in two layers:

• a string stops being embedded before it grows past the field, since resize_capa() moves it to the heap through RSTR_EMBEDDABLE_P and there is no path back;
• a length too big for the field has already been written into the embedded byte array by the time it reaches the flags, since str_init_embed() copies the bytes and then sets the length. The flags would be the second thing to go wrong, not the first.

The assertion is live in full-debug, the one ci/gcc-clang build that defines MRB_DEBUG, and in the 32-bit build, where RSTRING_EMBED_LEN_MAX is 11 rather than 27 and it is that much tighter. Both run the whole suite green. Where it is not live it costs nothing: bin/mruby is unchanged to the byte in the three builds without MRB_DEBUG.

The one write that does not go through the macro, mrbgems/mruby-sprintf/src/sprintf.c:622-626, cannot reach an embedded string at all. result is allocated at :416-421 with bsiz = (end - p) + 120, so its capacity is at least 120, and mrb_str_new_capa() only embeds when RSTR_EMBEDDABLE_P(capa) holds, which is 27 bytes on 64-bit and 11 on 32-bit. CHECK() only ever grows it through mrb_str_resize(), and resize_capa() has no branch back from the heap to embedded, so RSTRING(result)->flags & MRB_STR_EMBED is never true and the branch does not run.

The two fields cross in one mask

The second commit is what the fields being neighbours allows. RSTR_ENC_CR_COPY read a field out of the source and wrote it into the destination twice over:

#define RSTR_ENC_CR_COPY(dst, src) \
  (RSTR_ENC_COPY(dst, src), RSTR_CODERANGE_SET(dst, RSTR_CODERANGE(src)))

#define MRB_STR_ENC_CR_MASK (MRB_STR_ENCODING_MASK|MRB_STR_CODERANGE_MASK)
#define RSTR_ENC_CR_COPY(dst, src) \
  ((dst)->flags = ((dst)->flags & ~MRB_STR_ENC_CR_MASK) | \
                  ((src)->flags & MRB_STR_ENC_CR_MASK))

MRB_STR_ENC_CR_MASK would have been a constant wherever the two fields sat, so being neighbours is not what makes the single copy possible: it is what makes it one run of bits to read. str_replace() and mrb_str_times(), the two call sites, do not change.

Where a build indexes by character the two spell the same thing outright, since each field is exactly as wide as the mask over it and the masking in the old macros truncated nothing.

Where it indexes by byte they differ on paper: RSTR_CODERANGE_SET() is ((void)0) there, so the old macro left the destination's coderange bits alone and the new one carries the source's across. Those bits are zero on both sides in such a build, and nothing can make them otherwise: RSTR_CODERANGE_SET() is their only writer and it does nothing, MRB_STR_CODERANGE_MASK appears nowhere else, and every object is allocated zeroed (*p = RVALUE_zero, src/gc.c:743). So what crosses is zero onto zero.

Two smaller differences, neither reaching a caller in tree. The macro evaluates dst twice rather than four times and src once rather than twice, which matters only for an argument with side effects, and both call sites pass none. And in a build that indexes by byte the expression's type goes from void, which the trailing ((void)0) gave it, to the value of the assignment.

In mrb_str_times() the copy goes from two passes over the flags word to one:

   1b9f:  mov    0x8(%r12),%eax
   1ba4:  mov    0x8(%r13),%edx
   1ba8:  mov    %eax,%ecx
   1baa:  and    $0xfff,%eax
   1baf:  shr    $0xc,%ecx
   1bb2:  shr    $0xc,%edx
   1bb5:  and    $0x6000,%edx        <- the encoding index out of the source
   1bbb:  and    $0x9f,%ch           <- clear it in the destination
   1bbe:  or     %edx,%ecx
   1bc0:  mov    %ecx,%edx
   1bc2:  and    $0xffffffcf,%ecx    <- clear the coderange
   1bc5:  shl    $0xc,%edx
   1bc8:  or     %edx,%eax
   1bca:  mov    %eax,0x8(%r12)      <- the first write
   1bcf:  mov    0x8(%r13),%edx      <- read the source a second time
   1bd3:  and    $0xfff,%eax
   1bd8:  shr    $0xc,%edx
   1bdb:  and    $0x30,%edx          <- the coderange out of the source
   ...                                  and a second write

   1b1f:  mov    0x8(%r12),%eax
   1b24:  mov    0x8(%r13),%ecx
   1b28:  mov    %eax,%edx
   1b2a:  and    $0xfff,%eax
   1b2f:  shr    $0xc,%edx
   1b32:  shr    $0xc,%ecx
   1b35:  and    $0x1e00,%ecx        <- both fields out of the source
   1b3b:  and    $0xe1,%dh           <- clear both in the destination
   1b3e:  or     %ecx,%edx
   1b40:  mov    %edx,%ecx
   1b42:  shl    $0xc,%ecx
   1b45:  or     %ecx,%eax
   1b47:  mov    %eax,0x8(%r12)      <- one write

Generated code

gcc -O3, against the same objects built from master, over the four builds ci/gcc-clang makes. .text of bin/mruby:

The field order alone costs, and the coderange is what costs: it crosses bit 7 on the way up, and clearing a field up there stops fitting an x86-64 one byte immediate. From mrb_str_valid_encoding_p(), master above and this branch below:

   3870:  add    %r12,%rdi
   3873:  and    $0xffffffcf,%r9d      <- clear the coderange at bits 4-5
   3877:  mov    $0x20,%eax            <- VALID

   3880:  add    %r12,%rdi
   3883:  and    $0xfffff9ff,%r9d      <- clear it at bits 9-10
   388a:  mov    $0x400,%eax           <- VALID

The embedded length costs nothing either way, though it moves further than anything else: it is read as a shift and an and $0x1f in both places, and RSTR_SET_TYPE clears it with a four byte immediate in both (andl $0xff830fff becomes andl $0xffe00fff).

The single mask more than gives that back. It changes src/string.o and nothing else, taking 132, 304, and 368 bytes off the three builds that carry a coderange, so the two commits together come out smaller than master.

The assertion is not in those numbers because it is not in those builds. full-debug is the one that defines MRB_DEBUG, and there it adds 1248 bytes of .text; the other three are unchanged to the byte.

Objects differing, comparing objdump -d over every .o in the build:

A build without MRB_UTF8_STRING carries no coderange, but it does carry the encoding index, since String#b and Integer#chr mark a string there too. So its objects hold different instructions of the same size and its .text does not move by a byte.

Testing

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

The 32-bit build is the one that matters beyond the usual, since it is where RSTRING_EMBED_LEN_MAX is 11 rather than 27, where the embedded length has the most room above it, and where the assertion the third commit adds is tightest. build_config/host-m32.rb needs a multilib toolchain, so the build above is full-core with the compiler set to i686-linux-gnu-gcc instead.

full-debug and the 32-bit build with enable_debug are what put the assertion to work, since those are the two that define MRB_DEBUG. Neither trips it anywhere in the suite, and the 32-bit one holds it to 11 rather than 27.

Beyond the suites, embedded strings across the boundary (every length from 0 to 40, their length and bytesize, what they hold after an append, a slice, a *, a +), and the encoding index and coderange along the paths that copy them, come back the same as master on the UTF-8, byte-indexed, and 32-bit builds.

No test accompanies the change. No string answers anything different and no field changes width, so there is nothing new to pin.

Not in this PR

The widths do not change, no caller changes, and the coderange clearing spread across mrb_str_modify() and mrb_str_modify_keep_ascii() stays where it is: folding those into one masked write touches what mrb_str_modify_keep_ascii() promises, which is a separate change from where the bits sit.

Summary by CodeRabbit

• Refactor
  • Improved the reliability of string metadata handling, including embedded length, character-range status, and encoding information.
  • Updated internal flag documentation to clarify field boundaries and safer handling when string properties are copied or updated.
  • These changes help maintain consistent string behavior across encoding and character-range operations without altering the public API.