886128 - Fix several clang compilation warnings and some errors in nonstandandard configurations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Fix various clang warnings and some potential bugs

The attached patch fixes some warnings to make compiling with clang more bearable, fixes some "used but never defined" warnings in nonstandard configurations (e.g. --enable-root-analysis), and fixes a generational GC error where debug only constants were used outside debug mode.

Most of these should be very safe, but there's one warning originating from mozilla/CheckedInt.h (using class instead of struct) that I'm not sure of. It's possible that fixing this warning in the shell might spawn the opposite warning elsewhere (still safe, but annoying). It's the only instance of that particular warning in a shell build, so leaving it unfixed would be no great loss (unlike some of the others, which are repeated dozens of times).

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 2 - Fix a linker error caused by referincing IonCode with --disable-ion

As the description says. I'm not sure this is the right fix, but there's definitely something broken here (assuming we want to support the configuration).

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 766448 [details] [diff] [review]
Fix various clang warnings and some potential bugs

Asking njn for review since I know you've worked on warning-related bugs before, and terrence for feedback on the GGC part.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 766448 [details] [diff] [review]
Fix various clang warnings and some potential bugs

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

I'm surprised by all the class vs. struct mismatches, because my understanding is that MSVC warns about those.  What version of clang are you using?

::: js/src/builtin/Profilers.cpp
@@ +20,5 @@
>  #endif
>  
>  #include "jscntxtinlines.h"
>  
> +#include "vm/Shape-inl.h"

What's this needed for?  Last time I ran IWYU (which was a couple of weeks ago) it didn't come up.  If it's only for a particular configuration, you could wrap the #include inside a #ifdef.

::: js/src/frontend/FullParseHandler.h
@@ +15,5 @@
>  namespace js {
>  namespace frontend {
>  
>  template <typename ParseHandler>
> +struct Parser;

The Parser changes are fine but I'd be inclined to change them all to |class| instead of |struct|.  You'll need to add a |public:| at the top of the class if you do, of course.

::: js/src/gc/Nursery.cpp
@@ +62,4 @@
>      JS_POISON(heap, FreshNursery, NurserySize);
>      for (int i = 0; i < NumNurseryChunks; ++i)
>          chunk(i).runtime = rt;
> +#endif

I don't know if omitting this loop is safe in non-debug builds.  Terrence, can you check this?

@@ +114,2 @@
>      JS_POISON(thing, AllocatedThing, size);
> +#endif

This isn't necessary because JS_POISON turns into nothing in non-debug builds.

::: js/src/ion/AsmJS.h
@@ +18,3 @@
>  class SPSProfiler;
>  class AsmJSModule;
> +namespace frontend { class TokenStream; struct ParseNode; }

Again, bonus points if you convert these all to classes instead of structs.

::: js/src/ion/VMFunctions.h
@@ +11,5 @@
>  
>  namespace js {
>  
>  class DeclEnvObject;
> +struct ForkJoinSlice;

Ditto.

::: js/src/jsapi-tests/testVersion.cpp
@@ +7,5 @@
>  #include "jscntxt.h"
>  
>  #include "jscntxtinlines.h"
>  
> +#include "vm/Shape-inl.h"

Same question as before...

::: js/src/jsatom.cpp
@@ +6,5 @@
>  
>  /*
>   * JS atom table.
>   */
> +#include "jsatominlines.h"

Cool.

::: js/src/vm/String.cpp
@@ +12,5 @@
>  #include "gc/Marking.h"
>  
>  #include "jscompartmentinlines.h"
>  
> +#include "vm/Shape-inl.h"

Ditto.

::: mfbt/CheckedInt.h
@@ +603,5 @@
>                          detail::IsSupported<U>::value,
>                          "This type is not supported by CheckedInt");
>      }
>  
> +    friend struct detail::NegateImpl<T>;

As above, I prefer |class| to |struct|.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 766448 [details] [diff] [review]
Fix various clang warnings and some potential bugs

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

::: mfbt/CheckedInt.h
@@ +603,5 @@
>                          detail::IsSupported<U>::value,
>                          "This type is not supported by CheckedInt");
>      }
>  
> +    friend struct detail::NegateImpl<T>;

Given NegateImpl exists only because you can't partially specialize functions, and it's a struct only because class would require extra public:, this change is reasonable.  mfbt uses primarily classes, but structs see their time when it's convenient -- and this seems like such a place.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> I'm surprised by all the class vs. struct mismatches, because my
> understanding is that MSVC warns about those.  What version of clang are you
> using?

I'm using a recent svn version (because I had trouble getting IWYU to work on what the linux distro included). The mismatches are definitely real though :)

> > +#include "vm/Shape-inl.h"
> 
> What's this needed for?  Last time I ran IWYU (which was a couple of weeks
> ago) it didn't come up.  If it's only for a particular configuration, you
> could wrap the #include inside a #ifdef.

It's needed for writeBarrierPre and writeBarrierPost, so probably exact rooting. I'll check which, and see if there's a common define for it.

> The Parser changes are fine but I'd be inclined to change them all to
> |class| instead of |struct|.  You'll need to add a |public:| at the top of
> the class if you do, of course.

Will do. I used struct because it minimized the amount of changes, but I doubt there's enough forward declarations to be cumbersome.

> >      JS_POISON(thing, AllocatedThing, size);
> > +#endif
> 
> This isn't necessary because JS_POISON turns into nothing in non-debug
> builds.

I'm not sure what the preprocessor turns it into, but apparently it still sees AllocatedThing, and that's a debug only constant. Of course, I could also remove the #ifdef DEBUG from these constants (there's three of them) instead.

> > +    friend struct detail::NegateImpl<T>;
> 
> As above, I prefer |class| to |struct|.

Do you agree with Waldo? I checked, and the shell warning before this change was the only instance. After changing it to struct, no extra warnings are introduced (in a browser build). I'm not sure how/where NegateImpl is defined, but I guess I could check.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 766451 [details] [diff] [review]
Part 2 - Fix a linker error caused by referincing IonCode with --disable-ion

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

Good find, thanks for the patch!

::: js/src/gc/StoreBuffer.cpp
@@ +68,3 @@
>      JS_ASSERT(kind == JSTRACE_IONCODE);
>      static_cast<ion::IonCode *>(tenured)->trace(trc);
> +#endif

Lets also add an #else so that the branch is treated as an assertion in non-ion builds.

#else
    MOZ_NOT_REACHED("Only objects can be in the wholeCellBuffer if IonMonkey is disabled.");
#endif

You should also add |#include "mozilla/Assertions.h"| in the include block. See https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#.23include_ordering for the proper ordering.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Emanuel Hoogeveen from comment #5)
> > > +    friend struct detail::NegateImpl<T>;
> > 
> > As above, I prefer |class| to |struct|.
> 
> Do you agree with Waldo? I checked, and the shell warning before this change
> was the only instance. After changing it to struct, no extra warnings are
> introduced (in a browser build). I'm not sure how/where NegateImpl is
> defined, but I guess I could check.

It's in the same file.  And where JS customs apply in js/src files, mfbt customs apply in mfbt/ files.  So just keep the class->struct change and move on.  :-)

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 766448 [details] [diff] [review]
Fix various clang warnings and some potential bugs

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

::: js/src/gc/Nursery.cpp
@@ +62,4 @@
>      JS_POISON(heap, FreshNursery, NurserySize);
>      for (int i = 0; i < NumNurseryChunks; ++i)
>          chunk(i).runtime = rt;
> +#endif

We very much need that loop in non-debug builds. Thanks for the heads up!

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Fix various clang warnings and some potential bugs in nonstandard configurations

I addressed the review comments, converted another struct to a class for consistency, and folded in part 2 because it was tiny. I hid the vm/Shape-inl.h includes behind #ifdef JSGC_GENERATIONAL, but I think there's a good chance they can come out after bug 886205. Asking for a quick re-review to see if you agree.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 766983 [details] [diff] [review]
Fix various clang warnings and some potential bugs in nonstandard configurations

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

Thanks!  Looks good.  I'll try-server it and land it.

BTW, you should apply for level 1 repository access, which lets you push patches to the try server.  See  http://www.mozilla.org/hacking/committer/ for the procedure.  I'm happy to vouch for you;  just CC me on the bug if you file one.

::: js/src/builtin/Profilers.cpp
@@ +22,5 @@
>  #include "jscntxtinlines.h"
>  
> +#ifdef JSGC_GENERATIONAL
> +#include "vm/Shape-inl.h"
> +#endif

This should come before jscntxtinlines.h, and likewise for some of the other conditional Shape-inl.h includes.  I'll fix this before I land the patch.

::: js/src/gc/Nursery.cpp
@@ +47,5 @@
>      // This leaks all of these poisoned pointers. It would be better if they
>      // were marked as uncommitted, but it's a little complicated to avoid
>      // clobbering pre-existing unrelated mappings.
>      while (IsPoisonedPtr(heap) || IsPoisonedPtr((void*)(uintptr_t(heap) + NurserySize)))
> +        heap = MapAlignedPages(runtime(), NurserySize, Alignment);

Are the JSGC_ROOT_ANALYSIS builds broken without this?

Comment 11

[Nicholas Nethercote [inactive]]

> ::: js/src/builtin/Profilers.cpp
> @@ +22,5 @@
> >  #include "jscntxtinlines.h"
> >  
> > +#ifdef JSGC_GENERATIONAL
> > +#include "vm/Shape-inl.h"
> > +#endif
> 
> This should come before jscntxtinlines.h, and likewise for some of the other
> conditional Shape-inl.h includes.  I'll fix this before I land the patch.

My mistake;  you put it in the right place :)

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> BTW, you should apply for level 1 repository access, which lets you push
> patches to the try server.  See  http://www.mozilla.org/hacking/committer/
> for the procedure.  I'm happy to vouch for you;  just CC me on the bug if
> you file one.

Thanks! I'll do that soon :)

> > +        heap = MapAlignedPages(runtime(), NurserySize, Alignment);
> 
> Are the JSGC_ROOT_ANALYSIS builds broken without this?

Yes - there's no two-parameter version of MapAlignedPages. I think this probably changed at some point and the call in this block was overlooked (there's another one right above it).

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f8eee3c389

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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