Closed Bug 1132953 Opened 7 years ago Closed 7 years ago

Zero AsmJSModule::CodeRange and AsmJSModule::ExportedFunction::pod on construction, to avoid Valgrind warnings

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(2 files, 1 obsolete file)

dom/asmjscache/test/test_cachingBasic.html causes Valgrind to
complain, because structs with uninitialised alignment holes and
uninitialised fields are written in to the cache file (at msync).
This isn't a functional problem, but it does cause noise in Valgrind
runs that obscures real errors.

One crude approach to fixing this is to paint the entire area as
initialised just before doing msync().  But that has the disadvantage
of hiding real bugs in which we genuinely mistakenly copy
uninitialised values into the cache.

A better approach is to selectively initialise just the fields and
padding holes needed, for Valgrind-enabled builds.
Attached file Valgrind complaint
Attached patch bug1132953-1.diff (obsolete) — Splinter Review
Initialise just the structure fields and holes needed to make V happy.
At least on x86_64-linux.
Attachment #8564200 - Flags: feedback?(luke)
Comment on attachment 8564200 [details] [diff] [review]
bug1132953-1.diff

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

Thanks!

::: js/src/asmjs/AsmJSModule.cpp
@@ +1339,5 @@
> +    // Zero fields and padding as best we can, so that Valgrind
> +    // doesn't complain when we write this object into a file.
> +    nameIndex_ = 0;
> +    lineNumber_ = 0;
> +    profilingReturn_ = 0;

With the #ifdef removed (next comment), you could put these in the initializer list (here and below).

::: js/src/asmjs/AsmJSModule.h
@@ +458,5 @@
>              argCoercions_ = mozilla::Move(argCoercions);
> +#ifdef MOZ_VALGRIND
> +            // Zero the entire union so that Valgrind doesn't complain
> +            // when we write it into a file.
> +            mozilla::PodZero(&pod);

I appreciate the concern for not penalizing perf, but I don't think this path is hot enough to matter, so I'd take out the #ifdefs and multiline comment and just have:
  mozilla::PodZero(&pod);  // zero padding for valgrind
here and everywhere else.

@@ +499,5 @@
>              maybeFieldName_ = rhs.maybeFieldName_;
>              argCoercions_ = mozilla::Move(rhs.argCoercions_);
> +#ifdef MOZ_VALGRIND
> +            // Zero the entire union so that Valgrind doesn't complain
> +            // when we write it into a file.            

Trailing whitespace (shows up as red in diff view and hg diff, if you have "diff.trailingwhitespace = bold red_background" in your .hgrc, btw)
Attachment #8564200 - Flags: feedback?(luke) → feedback+
Respin patch incorporating changes in comment #3.
Attachment #8564200 - Attachment is obsolete: true
Attachment #8569763 - Flags: review?(luke)
Comment on attachment 8569763 [details] [diff] [review]
bug1132953-2.diff

Great, thanks!
Attachment #8569763 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/07bf3ce1c889
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.