Closed
Bug 1394696
Opened 7 years ago
Closed 7 years ago
stylo: uninitialised value use in next<style::gecko::selector_parser::SelectorImpl>
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: jseward, Assigned: rillian)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
(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
Flags: needinfo?(jseward)
Priority: -- → P3
Comment 4•7 years ago
|
||
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.
Reporter | ||
Comment 6•7 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jseward)
Resolution: --- → INVALID
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
Pushed to try on top of the rust 1.20.0 bump in https://treeherder.mozilla.org/#/jobs?repo=try&revision=7366f2a1f8ce30e2a5a5cd722d49168207935cce
Attachment #8907812 -
Flags: review?(xidorn+moz)
Attachment #8907812 -
Flags: review?(jseward)
Assignee | ||
Comment 9•7 years ago
|
||
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
Attachment #8907812 -
Attachment is obsolete: true
Attachment #8907812 -
Flags: review?(xidorn+moz)
Attachment #8907812 -
Flags: review?(jseward)
Attachment #8907855 -
Flags: review?(xidorn+moz)
Attachment #8907855 -
Flags: review?(jseward)
Comment 10•7 years ago
|
||
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.
Attachment #8907855 -
Flags: review?(xidorn+moz)
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox-esr52:
--- → wontfix
Assignee | ||
Comment 11•7 years ago
|
||
Ok, thanks, Xidorn. I just wanted to make sure one of the stylo devs saw it.
Reporter | ||
Comment 12•7 years ago
|
||
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.
Attachment #8907855 -
Flags: review?(jseward)
Assignee | ||
Comment 13•7 years ago
|
||
Thanks Julian, that's great guidance. I'll clean up the patch on Monday.
Assignee | ||
Comment 14•7 years ago
|
||
How does this look? I was able to reduce the number of entries to two.
Attachment #8907855 -
Attachment is obsolete: true
Attachment #8909994 -
Flags: review?(jseward)
Reporter | ||
Comment 15•7 years ago
|
||
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.
Attachment #8909994 -
Flags: review?(jseward) → review+
Comment 16•7 years ago
|
||
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f7779bc325
stylo: Suppress valgrind warning. r=jseward
Comment 17•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•