1754235 - (msan) reading padding bytes from RegEx JIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:100.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

Reading of regex flags from jitted code is currently done via a 4-byte read, see https://searchfox.org/mozilla-central/rev/2a0b0ababd4541ecffb74cbe0820a9d0a25da636/js/src/jit/CodeGenerator.cpp#1982
The actual flags are just 1 byte, followed by 3 (uninit) padding bytes. While msan generally allows reading of uninit values, this is quite complex to support from JIT code. Much more easily, the read can be narrowed to from load32 to load8ZeroExtend. Also, the subsequent store should be 1 byte instead of 4.

Comment 1

[Nicolas B. Pierron [:nbp]]

Julian, any idea why Valgrind would not report these uninitialized bits being read?
Iain, can you look this issue?

Comment 2

[Julian Seward [:jseward]]

Valgrind/Memcheck doesn't complain when uninitialised memory is read.
Instead the undefinedness is "followed" through the registers and is
reported only when either, the operands to a conditional branch are
undefined, or an undefined value is used to compute a memory address
(plus a couple more obscure cases).

See https://valgrind.org/docs/manual/mc-manual.html#mc-manual.together
in particular

• When values in CPU registers are used to generate a memory address,
  or to determine the outcome of a conditional branch, the V bits for those
  values are checked, and an error emitted if any of them are undefined.

• When values in CPU registers are used for any other purpose, Memcheck
  computes the V bits for the result, but does not check them.

Comment 3

[lukas.bernhard]

Attachment: Bug 1754235 - Narrow RegEx flag reads to corresponding size - r=iain

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a43ff41952b5
Narrow RegEx flag reads to corresponding size - r=iain

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a43ff41952b5

Comment 6

[Julian Seward [:jseward]]

Random drive-by and probably unhelpful comment: are we sure the
patch doesn't create a potential store-forwarding stall (small write
followed by larger read, in some circumstances), or the equivalent at
a register level?

Comment 7

[Iain Ireland [:iain]]

We read the flags out of the RegExpShared and store them into RegExpStatics. They won't be read until much later, without any possibility of store forwarding. (The only access is in the slow path where somebody accesses one of the deprecated static properties of the RegExp object (eg $1-$99), in which case we re-execute the most recent regular expression to recover the value.) Agner Fog's microarchitecture document says that store forwarding stalls are on the order of 11 cycles for Skylake, which wouldn't matter in the slow path anyway.

Also, I don't think that there should ever be a larger read, since this is an 8-bit field and any C++ code reading it should already do the right thing.

In short: no, store forwarding stalls are not a concern here :)