1394696 - stylo: uninitialised value use in next<style::gecko::selector_parser::SelectorImpl>

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Julian Seward [:jseward]]

I'm seeing this in m-c 377137:d10c97627b51 (Sun Aug 27 17:31:55 2017 -0700),
optimised build.  It may or may not be a false positive; on some cursory
reading of the associated machine code, it might be legit.

STR:

DISPLAY=:2.0 STYLO_THREADS=1 FORCE_STYLO_THREAD_POOL=1 \
  DUMP_STYLE_STATISTICS=2 valgrind --px-default=allregs-at-mem-access \
  --px-file-backed=unwindregs-at-mem-access --fair-sched=yes \
  --fullpath-after=/MOZ/ --trace-children=yes --error-limit=no \
  --show-mismatched-frees=no --expensive-definedness-checks=yes \
  --track-origins=yes \
  ./gcc-O2-nondebug-stylo/dist/bin/firefox-bin -P dev -no-remote \
  /home/sewardj/MOZ/STYLO/Barack_Obama_Wikipedia.html \
  2>&1 | tee spew-02-07-mc

Comment 1

[Julian Seward [:jseward]]

Attachment: memcheck errors

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I don't think there's anything fishy there, so if something it should be where we build the selector... I'll take a quick look, but sounds like a false positive to me.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Julian Seward [:jseward] from comment #0)
> I'm seeing this in m-c 377137:d10c97627b51 (Sun Aug 27 17:31:55 2017 -0700),
> optimised build.  It may or may not be a false positive; on some cursory
> reading of the associated machine code, it might be legit.

Can you elaborate on this?

From the log, it sounds like the uninitialized memory is from a stack-allocation in [1]. That function is very small, so it should be very easy to figure out what memory it's referring to and whether this is a false-positive. NI Julian to investigate this when he's finished with the fallible allocation stuff.

http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/servo/ports/geckolib/glue.rs#992

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

The stack looks weird.

On the top of the stack, it states that the conditional jump happens inside
> next<style::gecko::selector_parser::SelectorImpl> (/checkout/src/libcore/slice/mod.rs:0)
however, SelectorIter<SelectorImpl>::next() should be iterating a slice of Component<SelectorImpl>, rather than SelectorImpl.

If we are iterating a slice of SelectorImpl, things can be more interesting, since it is a zero-length type. But Component<SelectorImpl> isn't a zero-length type anyway.

Comment 6

[Julian Seward [:jseward]]

I don't believe this is real, after some digging around.  I think it is
another manifestation of an existing problem, which is: Memcheck assumes
that every conditional branch is important, so a conditional branch on
undefined data is definitely a bug.  That was true up until a couple of
years ago.  But since then both LLVM and GCC have started doing the
following transformation:

   if A && B  -->   if B && A

provided that A is always false whenever B is undefined.  This is a bit
hard to get one's head around, but it is a correct transformation.

I think it hits us in optimized Rust in cases like

   if x == Some(123) ...

where the obvious translation is

   if x.words[0] == tag-for-Some && x.words[1] == 123 ...

but it works equally well with

   if x.words[1] == 123 && x.words[0] == tag-for-Some

My guess is that the compiler 'knows' that if x.words[1] is undefined
then x.words[0] cannot == tag-for-Some, because x.words[1] will only
be undefined in the None case.  So the transformation is valid.

I currently have no fix for this.

---------------

A second reason why I think these complaints are false errors is because
I've been running Fx/Stylo on Memcheck with the Stylo part unoptimised,
and I don't see them.

Comment 7

[Ralph Giles (:rillian)]

Thanks for investigating. We still need to suppress the warning in the tests though, so we can update rust in automation. I'll write a patch.

Comment 8

[Ralph Giles (:rillian)]

Attachment: Suppress valgrind warning

Pushed to try on top of the rust 1.20.0 bump in https://treeherder.mozilla.org/#/jobs?repo=try&revision=7366f2a1f8ce30e2a5a5cd722d49168207935cce

Comment 9

[Ralph Giles (:rillian)]

Attachment: Suppress valgrind warning

Needed a few more instances. Does it make sense to consolidate this into just two with only a pair of stack entries?

Pushed to try as https://treeherder.mozilla.org/#/jobs?repo=try&revision=16dd1d33171b20a34a5cee1a2212d48471c721f1

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8907855 [details] [diff] [review]
Suppress valgrind warning

Review of attachment 8907855 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds a reasonable change, but I'm not familiar with either valgrind as well as this suppression, or the compiler optimization, so I don't think I can review this.

I think Julian's review would be enough.

Comment 11

[Ralph Giles (:rillian)]

Ok, thanks, Xidorn. I just wanted to make sure one of the stylo devs saw it.

Comment 12

[Julian Seward [:jseward]]

Comment on attachment 8907855 [details] [diff] [review]
Suppress valgrind warning

Review of attachment 8907855 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/valgrind/x86_64-redhat-linux-gnu.sup
@@ +231,5 @@
>  }
>  
> +# False positives triggered by rust 1.20.0 (at least) builds of stylo.
> +# See bug 1394696. The diagnosis is an llvm optimization transforming
> +# `if A && B` to `if B && A` when B is unitialized but A is alway false.

nit: typo: uninitialized & always

@@ +240,5 @@
> +#    by 0x113EBAC0: <style::selector_map::SelectorMap<style::stylist::Rule>>::get_matching_rules (matching.rs:397)
> +{
> +  Bug 1394696 Stylo selector, Sept 2017, part 1
> +  Memcheck:Cond
> +  fun:_ZN9selectors8matching33matches_complex_selector_internal17h8c454578a1becc81E

These need to be less specific; in particular we need to not have the 
hash in the symbol name, since its presence will cause the suppression not to match as soon as the hash changes for whatever reason.

I suggest removing the hash and instead just putting '*', thusly:

fun:_ZN9selectors8matching33matches_complex_selector_internal*

here and elsewhere.

@@ +257,5 @@
> +#    by 0x113EE36E: selectors::matching::matches_simple_selector (wrapper.rs:1940)
> +{
> +  Bug 1394696 Stylo selector, Sept 2017, part 2
> +  Memcheck:Cond
> +  fun:_ZN9selectors8matching33matches_complex_selector_internal17h8c454578a1becc81E

I suggest also to limit the number of frames in each supp to 4 or 5,
so as to increase the likelyhood that they keep matching even if the
calling sequence changes somewhat.  You can use "..." as a frame-level
wildcard (meaning "zero or more frames") so as to add some flexibility
without losing important specificness.  For example

fun:_ZN9selectors8matching24matches_complex_selector*
fun:_ZN9selectors8matching23matches_simple_selector*
...
fun:_ZN9selectors8matching24matches_complex_selector*
fun:_ZN69_$LT$style..selector_map..SelectorMap$LT$style..stylist..Rule$GT$$GT$18get_matching_rules*

This ensures you still match "matches_complex_selector" as the
innermost frame, and has "get_matching_rules" further out, with
the other two calls in between as shown, but elides any number of
intermediaries.  If you do this, you may find that the number of
supps required falls, in the ideal case to 1.

Comment 13

[Ralph Giles (:rillian)]

Thanks Julian, that's great guidance. I'll clean up the patch on Monday.

Comment 14

[Ralph Giles (:rillian)]

Attachment: Suppress valgrind warning

How does this look? I was able to reduce the number of entries to two.

Comment 15

[Julian Seward [:jseward]]

Comment on attachment 8909994 [details] [diff] [review]
Suppress valgrind warning

Review of attachment 8909994 [details] [diff] [review]:
-----------------------------------------------------------------

That's fine as-is.

If you want to slightly clarify the comment, so much the better.  Maybe

[..]
`if A && B` to `if B && A` if it can be proven that A is false whenever B is uninitialized.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f7779bc325
stylo: Suppress valgrind warning. r=jseward

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16f7779bc325