Closed Bug 1290337 Opened 3 years ago Closed 3 years ago

Replace JSVAL_ALIGNMENT with alignas(8)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(28 files, 8 obsolete files)

2.39 KB, patch
nbp
: review+
Waldo
: review+
Details | Diff | Splinter Review
18.08 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
41.18 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.21 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.38 KB, patch
nbp
: review+
Details | Diff | Splinter Review
7.73 KB, patch
terrence
: review+
Details | Diff | Splinter Review
12.55 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.33 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.18 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
1014 bytes, patch
terrence
: review+
Details | Diff | Splinter Review
52.36 KB, patch
nbp
: review+
Details | Diff | Splinter Review
798 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
5.13 KB, patch
sfink
: review+
Details | Diff | Splinter Review
25.49 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.00 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.97 KB, patch
mak
: review+
Details | Diff | Splinter Review
1.77 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.32 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.22 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
4.96 KB, patch
shu
: review+
Details | Diff | Splinter Review
956 bytes, patch
shu
: review+
Details | Diff | Splinter Review
2.22 KB, patch
terrence
: review+
Details | Diff | Splinter Review
23.80 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.01 KB, patch
sfink
: review+
Details | Diff | Splinter Review
956 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
Derived from bug 1288541.

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/js/public/Value.h#37
> /*
>  * Try to get jsvals 64-bit aligned. We could almost assert that all values are
>  * aligned, but MSVC and GCC occasionally break alignment.
>  */
> #if defined(__GNUC__) || defined(__xlc__) || defined(__xlC__)
> # define JSVAL_ALIGNMENT        __attribute__((aligned (8)))
> #elif defined(_MSC_VER)
>   /*
>    * Structs can be aligned with MSVC, but not if they are used as parameters,
>    * so we just don't try to align.
>    */
> # define JSVAL_ALIGNMENT
> #elif defined(__SUNPRO_C) || defined(__SUNPRO_CC)
> # define JSVAL_ALIGNMENT
> #elif defined(__HP_cc) || defined(__HP_aCC)
> # define JSVAL_ALIGNMENT
> #endif
> ...
> typedef union jsval_layout
> {
> ...
> } JSVAL_ALIGNMENT jsval_layout;

JSVAL_ALIGNMENT has an effect only on some compiler, so it's not guaranteed that JS::Value is always aligned to 64-bit boundary.
We could replace it with c++11 `alignas` now, so that we don't have to worry about the not-aligned case.
Try is running.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63eb88b400ad
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
so sad...

> c:\builds\moz2_slave\try_w32_sm-compacting-00000000\src\obj-spider\dist\include\js/Value.h(379): error C2719: 'l': formal parameter with requested alignment of 8 won't be aligned
> c:\builds\moz2_slave\try_w32_sm-compacting-00000000\src\obj-spider\dist\include\js/Value.h(394): error C2719: 'l': formal parameter with requested alignment of 8 won't be aligned
> c:\builds\moz2_slave\try_w32_sm-compacting-00000000\src\obj-spider\dist\include\js/Value.h(400): error C2719: 'l': formal parameter with requested alignment of 8 won't be aligned
So, apparently this restriction applies also to alignas

https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/js/public/Value.h#40
>  /*
>   * Structs can be aligned with MSVC, but not if they are used as parameters,
>   * so we just don't try to align.
>   */
only windows 32bit builds failed with parameter issue.
We will need some workaround for it...
Do you have any idea how we can avoid MSVC's restriction?
maybe we need to apply it only for non-MSCV compilers?
Flags: needinfo?(jwalden+bmo)
Here's try run with applying alignas(8) for non-MSVC compilers.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ecf78c4e20
One possibility is converting every Value parameter to a const Value& or Handle<Value> parameter.  Frankly, that'd be a bit happier-making than having raw, unrooted types as arguments.  Short of that, I'm not sure there's anything else to do.
Flags: needinfo?(jwalden+bmo)
Is there any specifier or something that explicitly prohibits using the raw type as parameter?
it would be so inconvenient that newly added function with raw Value parameter will fail only on win32.
the failure would be often missed on try run, and will fail after the patch gets landed.
and actually I'm now experiencing difficultly on finding the raw Value parameters... :/
apparently the failure is bug 1287047, so I think the patch is working.
will post them, maybe slowly (I have another tree-wide patch series in bug 1289050)
Before doing all other things, I'd first ask review about the change to ArgList and ArgSeq.
I need to make them not to drop reference qualifier, but store them as raw type, not reference.

so that we can pass JS::Value or any other related struct with reference, and we don't lose it after leaving the scope.
Attachment #8791365 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8791365 [details] [diff] [review]
01-Bug_1290337___Part_1__Make_ArgSeq_and_Ar.patch

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

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +656,5 @@
>  
>    public:
>      explicit ArgSeq(HeadType&& head, TailTypes&&... tail)
> +      : ArgSeq<TailTypes...>(mozilla::Forward<TailTypes>(tail)...),
> +        head_(head)

If you're not declaring/defining a template function, mozilla::Forward and T&& acting on arguments won't act the way you think they're supposed to act.  This should be

  template<typename ProvidedHead, typename... ProvidedTail>
  explicit ArgSeq(ProvidedHead&& head, ProvidedTail&&... tail)
    : ArgSeq<TailTypes...>(mozilla::Forward<ProvidedTail>(tail)...),
      head_(mozilla::Forward<ProvidedHead>(head))
  {}
Updated the template parameter
Attachment #8791365 - Attachment is obsolete: true
Attachment #8791365 - Flags: review?(nicolas.b.pierron)
Attachment #8791410 - Flags: review?(jwalden+bmo)
Comment on attachment 8791410 [details] [diff] [review]
Part 1: Make ArgSeq and ArgList not to drop qualifier from argument types.

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

Looks good to me, punting back for proper CodeGenerator-centric review.
Attachment #8791410 - Flags: review?(nicolas.b.pierron)
Attachment #8791410 - Flags: review?(jwalden+bmo)
Attachment #8791410 - Flags: review+
Comment on attachment 8791410 [details] [diff] [review]
Part 1: Make ArgSeq and ArgList not to drop qualifier from argument types.

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

Thanks, I had to go read the big comment on top of Move.h [1] to understand that the ArgList function type does not enforce any move-semantic to any of its arguments if these are variables.

[1] http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/mfbt/Move.h#121-164

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +10,5 @@
>  #include "mozilla/Alignment.h"
>  #include "mozilla/Move.h"
>  #include "mozilla/TypeTraits.h"
>  
> +#include <type_traits>

http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/mfbt/TypeTraits.h#14-18

nit: s/std::remove_reference<(.*)>::type/mozilla::RemoveReference<\1>::Type/g
Attachment #8791410 - Flags: review?(nicolas.b.pierron) → review+
jsval_layout will get alignas(8) in later patch, and it requires all jsval_layout parameter to be const reference, to avoid VisualStudio's issue.
Attachment #8791688 - Flags: review?(evilpies)
JS::Value contains jsval_layout, and it also needs to be passed by const reference.
this patch changes simple cases that doesn't need to be changed to HandleValue or something.
Attachment #8791689 - Flags: review?(shu)
ValueToScript and PrintHelpString should receive HandleValue instead of raw Value.
Attachment #8791691 - Flags: review?(sphink)
also changed simple raw Value cases to const reference, in JIT.
Attachment #8791692 - Flags: review?(nicolas.b.pierron)
Attachment #8791692 - Flags: review?(nicolas.b.pierron) → review+
forgot to describe about the plan etc.

I need to make jsval_layout and JS::Value explicitly aligned, to avoid having non-aligned case in JIT code (see bug 1288541).

Currently we use JSVAL_ALIGNMENT macro to make them aligned, but it has no effect on VisualStudio, because of the compiler restriction, that is, it doesn't support having alignment on parameter type.
Now we can use C++11 alignas, but it also has the same restriction.

So, I'm going to change jsval_layout, JS::Value, and all other struct that contains JS::Value to be passed by reference,
so that we can avoid the VisualStudio's restiction.

and then add alignas to jsval_layout.

Also, because of the fact that the alignment issue with parameter happens only on VisualStudio,
it would be inconvenient that the issue (having raw Value related parameter) cannot be caught on other builds, especially when testing on try server.
I'll also add a custom annotation for static analysis build, that detects the almost same case.
in WIP patch the annotation can catch function/method declaration, and still cannot catch template and function typedef.
Comment on attachment 8791688 [details] [diff] [review]
Part 2: Replace jsval_layout parameter to const jsval_layout&.

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

::: js/public/Value.h
@@ +1391,5 @@
>      jsval_layout data;
>  
>    private:
>  #if defined(JS_VALUE_IS_CONSTEXPR)
> +    MOZ_IMPLICIT JS_VALUE_CONSTEXPR Value(const jsval_layout& layout) : data(layout) {}

Doos constexpr actually work with references? https://stackoverflow.com/questions/28614591/how-to-initialize-a-constexpr-reference#28614655 seems to suggest it depends on where layout is stored.
Attachment #8791688 - Flags: review?(evilpies) → review+
Comment on attachment 8791691 [details] [diff] [review]
Part 4: Replace Value parameter to HandleValue in js shell.

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

::: js/src/shell/js.cpp
@@ +2224,5 @@
>  {
>      RootedScript script(cx, GetTopScript(cx));
>      *ip = 0;
>      if (argc != 0) {
> +        RootedValue v(cx, argv[0]);

Why doesn't GetScriptAndPCArgs take a const CallArgs& instead of argc/argv? Then this would be 

  HandleValue v = args[0];

But for the purposes of this bug, either works fine.

(The if (argc != 0) really ought to be !args.get(0).isUndefined(), but that's a bigger change.)
Attachment #8791691 - Flags: review?(sphink) → review+
Thank you for reviewing :D

(In reply to Tom Schuster [:evilpie] from comment #22)
> Comment on attachment 8791688 [details] [diff] [review]
> Part 2: Replace jsval_layout parameter to const jsval_layout&.
> 
> Review of attachment 8791688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/Value.h
> @@ +1391,5 @@
> >      jsval_layout data;
> >  
> >    private:
> >  #if defined(JS_VALUE_IS_CONSTEXPR)
> > +    MOZ_IMPLICIT JS_VALUE_CONSTEXPR Value(const jsval_layout& layout) : data(layout) {}
> 
> Doos constexpr actually work with references?
> https://stackoverflow.com/questions/28614591/how-to-initialize-a-constexpr-
> reference#28614655 seems to suggest it depends on where layout is stored.

it's not storing the reference.  just receives as a reference and stores the actual data,
so the address is not stored to anywhere.  I think the linked case is different than this.

as far as I can see, it's working at least on clang.

Waldo, can I have your opinion?
Flags: needinfo?(jwalden+bmo)
Attachment #8791689 - Flags: review?(shu) → review+
Changed parameter from template type from T to const T&, where T can be JS::Value.

following function are also changed to receive const reference of non-JS::Value type, because it uses MaybeRooted.
  * js::NativeLookupOwnProperty
  * GetNonexistentProperty
  * js::ConcatStrings
Attachment #8791876 - Flags: review?(terrence)
other GC related things.

here too, changed parameter with template type from T to const T& where T can be JS::Value.
and also changed some Value parameter to const Value&.

DispatchTyped is changed to receive const reference even for non-JS::Value, to make it consistent with Value's case
Attachment #8791877 - Flags: review?(terrence)
and remaining maybe GC related things.

changes parameter of template type from S to const S& where S can be JS::Value.
Attachment #8791878 - Flags: review?(terrence)
changed NumLit to receive const Value&,
and changed NumLit parameter to const NumLit&, as it has Value.
Attachment #8791879 - Flags: review?(bbouvier)
changed WrapperValue parameter to const WrapperValue&, as it has Value.
Attachment #8791880 - Flags: review?(terrence)
Changed ConstantOrRegister parameter to const ConstantOrRegister&, as it has JS::Value.

ConstantOrRegister parameter will be passed to ArgList. it will now receive const reference too.
Attachment #8791881 - Flags: review?(nicolas.b.pierron)
PodSet can be called with JS::Value, so changed the aSrc parameter to const reference.
Attachment #8791882 - Flags: review?(sphink)
Attachment #8791879 - Flags: review?(bbouvier) → review+
Attachment #8791881 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8791882 - Flags: review?(sphink) → review+
simply changed Value to const reference in finalizeInBackground method, including binding codegen.
Attachment #8792265 - Flags: review?(sphink)
js-ctypes code doesn't use HandleValue in so much case, where it should use.
changed them to use HandleValue.
Attachment #8792266 - Flags: review?(evilpies)
simply changed Value to const reference, in js/xpconnect
Attachment #8792267 - Flags: review?(jdemooij)
simply changed JS::Value parameter to const reference.
Attachment #8792268 - Flags: review?(bugs)
also changed JS::Value parameter to const reference in dom/bindings
Attachment #8792269 - Flags: review?(bugs)
Attachment #8791876 - Flags: review?(terrence) → review+
Attachment #8791877 - Flags: review?(terrence) → review+
Attachment #8791878 - Flags: review?(terrence) → review+
Attachment #8791880 - Flags: review?(terrence) → review+
Attachment #8792265 - Flags: review?(sphink) → review+
Attachment #8792267 - Flags: review?(jdemooij) → review+
Attachment #8792268 - Flags: review?(bugs) → review+
Comment on attachment 8792269 [details] [diff] [review]
Part 17: Replace Value parameter to const Value& in dom/bindings.


> // A specialization of Optional for JS::Value to make sure no one ever uses it.
> template<>
> class Optional<JS::Value>
> {
> private:
>   Optional() = delete;
> 
>-  explicit Optional(JS::Value aValue) = delete;
>+  explicit Optional(const JS::Value& aValue) = delete;
Hmm, should we have both ctors marked as = delete;



So in which case passing JS::Value is ok and in which case const JS::Value& is needed?
That needs to be documented somewhere, or does the former fail to compile or something?
Attachment #8792269 - Flags: review?(bugs) → review+
Comment on attachment 8792270 [details] [diff] [review]
Part 18: Replace Value parameter to const Value& in ipc.

Still don't know when JS::Value -> const JS::Value& is needed, but at least it shouldn't be harmful :)
Attachment #8792270 - Flags: review?(bugs) → review+
So, arai, could you explain the requirement why JS::Value -> const JS::Value&  is needed and when.
Flags: needinfo?(arai.unmht)
(In reply to Olli Pettay [:smaug] (TPAC) from comment #40)
> So, arai, could you explain the requirement why JS::Value -> const
> JS::Value&  is needed and when.

it's always needed because we want to apply alignment to jsval_layout union, and MSVC doesn't support
having alignas on parameter type.

after this bug gets fixed, having raw JS::Value parameter will fail to compile on MSVC and static analysis build.
Flags: needinfo?(arai.unmht)
So if one doesn't compile on Windows and does a normal build, does something fail if raw JS::Value is passed as a param?
(In reply to Olli Pettay [:smaug] (TPAC) from comment #42)
> So if one doesn't compile on Windows and does a normal build, does something
> fail if raw JS::Value is passed as a param?

No.
The issue is only on MSVC, and I'm trying to emulate the same thing on static analysis build.
(In reply to Tooru Fujisawa [:arai] from comment #24)
> > Doos constexpr actually work with references?
> > https://stackoverflow.com/questions/28614591/how-to-initialize-a-constexpr-
> > reference#28614655 seems to suggest it depends on where layout is stored.
> 
> it's not storing the reference.  just receives as a reference and stores the
> actual data,
> so the address is not stored to anywhere.  I think the linked case is
> different than this.

It's possible to call a constexpr function, in a way that won't perform the entirety of the call at compile time.  That is, something like this is perfectly fine:

  constexpr int Min(int x, int y) { return x < y ? x : y; }
  int main(int argc) {
    return Min(argc, 7);
  }

Likewise it's perfectly fine to have a constexpr Value constructor that takes a const&.  *If* you pass it a static constexpr, the entire thing (including dereference of the reference) happens at compile time.  If you don't, it won't (necessarily, at least).  And that's not an error, so long as the context that uses this non-constexpr Value construction isn't itself a constant context.  Untested examples:

  template<typename T, T t> struct Saver {};
  constexpr jsval_layout foo = some expression that's a constant jsval_layout;
  Saver<jsval_layout, foo> ok; // OK, foo is constexpr
  int f() {
    jsval_layout bar;
    Saver<jsval_layout, bar> bad; // BAD, bar not constexpr but a constexpr context
  }

Basically, this means that switching these to const& *potentially* deoptimizes things, somewhat invisibly.  If we want to guarantee that these won't deoptimize, we would have to make these take the relevant subcomponents of the jsval_layout union, I think.

Frankly, at this point jsval_layout doesn't have much reason to exist, and there are fair reasons for it to be folded into JS::Value.  I'm not horribly concerned about potential non-constexpring here.  So if it seems important to eliminate this risk, I'd say we get rid of jsval_layout in a followup bug/patch to do it.
Flags: needinfo?(jwalden+bmo)
Thanks :)
Will try removing jsval_layout and moving alignas to JS::Value.

so far, it's used also in JIT, but it won't matter much (I hope):
https://dxr.mozilla.org/mozilla-central/search?q=jsval_layout&case=true&=mozilla-central
See Also: → 1304191
This is tricky one.

WrappableJSErrorResult has JS::Value (with JSErrorResult mRv), and it's passed to other thread with WrapRunnable and RUN_ON_THREAD,
and I cannot find a simple way to make them receive const reference but hold actual value.
So I've changed WrappableJSErrorResult to allocate mRv separately.
Do you have better solution?
Attachment #8793248 - Flags: review?(bugs)
simply replaced JS::Value parameter to const reference.
Attachment #8793249 - Flags: review?(mak77)
Comment on attachment 8793248 [details] [diff] [review]
Part 19: Allocate JSErrorResult separately in WrappableJSErrorResult.


> class WrappableJSErrorResult {
> public:
>   WrappableJSErrorResult() : isCopy(false) {}
>-  WrappableJSErrorResult(const WrappableJSErrorResult &other) : mRv(), isCopy(true) {}
>+  WrappableJSErrorResult(const WrappableJSErrorResult &other) : mRv(nullptr), isCopy(true) {}
>   ~WrappableJSErrorResult() {
>     if (isCopy) {
>       MOZ_ASSERT(NS_IsMainThread());
>     }
>+    if (mRv) {
>+      delete mRv;
>+    }
>   }
>-  operator JSErrorResult &() { return mRv; }
>-  operator ErrorResult &() { return mRv; }
>+  bool init(void) {
>+    mRv = new JSErrorResult();
>+    return !!mRv;
>+  }
'new' is infallible, so there isn't need for this init(). Just create JSErrorResult in ctor.

>+  operator JSErrorResult &() { return *mRv; }
>+  operator ErrorResult &() { return *mRv; }
> private:
>-  JSErrorResult mRv;
>+  JSErrorResult* mRv;
This really should be UniquePtr or nsAutoPtr, and then you don't need delete mRv in dtor.
I hope we can use one of those in this code.

With those changes the patch should become even simpler.
Attachment #8793248 - Flags: review?(bugs) → review-
Comment on attachment 8792266 [details] [diff] [review]
Part 14: Replace Value parameter to HandleValue in js/ctypes.

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

::: js/src/ctypes/CTypes.cpp
@@ +1630,5 @@
>    return false;
>  }
>  
>  static bool
> +FunctionReturnTypeError(JSContext* cx, HandleValue typeVal, const char* reason)

Super nit: I would have just kept 'type'.

@@ +5193,5 @@
>  
>    // The third argument is an optional error sentinel that js-ctypes will return
>    // if an exception is raised while executing the closure. The type must match
>    // the return type of the callback.
> +  RootedValue errVal(cx, JS::UndefinedValue());

JS::UndefinedValue is unnecessary.
Attachment #8792266 - Flags: review?(evilpies) → review+
thanks :)
here's updated patch that uses UniquePtr.
Attachment #8793248 - Attachment is obsolete: true
Attachment #8793387 - Flags: review?(bugs)
updated default ctor to initialize mRv
Attachment #8793387 - Attachment is obsolete: true
Attachment #8793387 - Flags: review?(bugs)
Attachment #8793389 - Flags: review?(bugs)
Attachment #8793389 - Flags: review?(bugs) → review+
here's current implementation of static analysis annotation.

it can catch function declaration, but cannot catch following:
  * typedef
  * using
  * lambda
  * template

the issue is that template will be common use case for JS::Value.
do you have any idea how we can support it?
Attachment #8793455 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8793455 [details] [diff] [review]
Part 21: Add moz_non_param annotation.

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

::: build/clang-plugin/clang-plugin.cpp
@@ +597,5 @@
> +
> +      if (NonParam.hasEffectiveAnnotation(T)) {
> +        unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +            DiagnosticIDs::Error,
> +            "Type %0 must not be uses as parameter.  Consider using const reference instead");

used

::: build/clang-plugin/tests/TestNonParameterChecker.cpp
@@ +1,4 @@
> +#define MOZ_NON_PARAM __attribute__((annotate("moz_non_param")))
> +
> +struct Param {};
> +struct MOZ_NON_PARAM NonParam {};

union MOZ_NON_PARAM NonParamUnion {};

probably also worth testing, I think.  And maybe various enum types as well, too?

@@ +69,5 @@
> +
> +void test()
> +{
> +    auto paramLambda = [](Param x) -> void {};
> +    auto nonParamLambda = [](NonParam x) -> void {}; // FIXME: NonParam x should show an error

Should also add the HasNonParam{Struct,Union,StructUnion} versions as well.

@@ +71,5 @@
> +{
> +    auto paramLambda = [](Param x) -> void {};
> +    auto nonParamLambda = [](NonParam x) -> void {}; // FIXME: NonParam x should show an error
> +
> +    tmplClass<NonParam> x; // FIXME: raw(T x) should show an error

This actually shouldn't be the case.  You've instantiated the *template*, but you haven't instantiated the particular *method* that's problematic.  You can have bad/non-compiling code in a member function in a template class, and as long as you never call the function, it's not actually a compile error in C++.  For example, this compiles without error, unless you pass in -DINSTANTIATE:

  template<typename T>
  struct S
  {
    void foo(T t) {
      static_cast<void>(*t);
    }
  };

  int main()
  {
    S<int> s1;
  #ifdef INSTANTIATE
    s1.foo(1);
  #endif

    S<void*> s2;
  }

In your code here, I would expect to see an error if you tried

  x.raw();

assuming you had

  void raw(T x = T()) {}

instead of what you have.  You should test that.  But your current test should actually, somewhat surprisingly, work.
Attachment #8793455 - Flags: feedback?(jwalden+bmo) → feedback+
Now it can catch more cases (almost all cases, I think).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1abe270cfc30

will post patches later.
Attachment #8793249 - Flags: review?(mak77) → review+
Changed to use AstMatcher, to match function/constructor/method/lambda definitions.

it will catch:
  * function definition
  * class constructor/method definition
  * lambda

and ignores:
  * function declaration
  * function type definition with typedef/using
Attachment #8793455 - Attachment is obsolete: true
replaced JSVAL_ALIGNMENT with |MOZ_NON_PARAM alignas(8)|
now we can also use alignof.
so replaced JS_ALIGNMENT_OF with alignof.
Changed some const Value& parameters for functions that receives JSContext to HandleValue.
Attachment #8795147 - Flags: review?(shu)
followup for Part 4.
Changed |unsigned argc, Value* argv| to |CallArgs&|.
Attachment #8795151 - Flags: review?(terrence.d.cole)
Attachment #8795151 - Flags: review?(terrence.d.cole) → review+
Comment on attachment 8795143 [details] [diff] [review]
Part 21: Add moz_non_param annotation.

(posting comment again, to make it clearer)

Changed to use AstMatcher, to match function/constructor/method/lambda definitions.

it will catch:
  * function definition
  * class constructor/method definition
  * lambda

and ignores:
  * function declaration
  * function type definition with typedef/using
Attachment #8795143 - Flags: review?(jwalden+bmo)
Comment on attachment 8795143 [details] [diff] [review]
Part 21: Add moz_non_param annotation.

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

Ugh, this plugin code is impossible to read with its capitalization anti-style.

Appears fine to me, but surely there's someone actually qualified to review clang-plugin code who should look at this.  Find 'em and ask 'em too.  :-)

::: build/clang-plugin/clang-plugin.cpp
@@ +1876,5 @@
> +      DiagnosticIDs::Error, "Type %0 must not be used as parameter");
> +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "Please consider using const reference instead");
> +  unsigned SpecNoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "bad instantiation of %0 requested here");

Seems like all these decls should move below the |CheckedFunctionDecls.insert(func);| down at the bottom.

::: build/clang-plugin/tests/TestNonParameterChecker.cpp
@@ +91,5 @@
> +    void raw(Param x) {}
> +    void raw(NonParam x) {} //expected-error {{Type 'NonParam' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    void raw(HasNonParamStruct x) {} //expected-error {{Type 'HasNonParamStruct' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    void raw(HasNonParamUnion x) {} //expected-error {{Type 'HasNonParamUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    void raw(HasNonParamStructUnion x) {} //expected-error {{Type 'HasNonParamStructUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}

Probably test static versions of these, too.

@@ +208,5 @@
> +    tmplFuncForParam<Param>(param);
> +
> +    NonParam nonParam;
> +    tmplFuncForNonParam<NonParam>(nonParam); // FIXME: note for bad instantiationshould be shown here
> +    tmplFuncForNonParamImplicit(nonParam); // FIXME: note for bad instantiationshould be shown here

s/tionshould/tion should/g

And also below, too.
Attachment #8795143 - Flags: review?(jwalden+bmo) → review+
Attachment #8796409 - Flags: review?(ehsan)
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c5ce6ed75dd9427d4eb9eac65704260074d577
Bug 1290337 - Part 1: Make ArgSeq and ArgList not to drop qualifier from argument types. r=nbp,jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a7f0fa1186ada56b6a66f1181069790c61a3bd
Bug 1290337 - Part 2: Replace jsval_layout parameter to const jsval_layout&. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/9796ed81f17a936804ec792c843dadf2c00023df
Bug 1290337 - Part 3: Replace Value parameter to const Value& in simple case. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/f81c58393110bf0a3cf9f6c39262dca1af57cdd0
Bug 1290337 - Part 4: Replace Value parameter to HandleValue in js shell. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0b9ebc2f1fa06820db74823ebe9179f8e627cf
Bug 1290337 - Part 5: Replace Value parameter to const Value& in JIT. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbf0a82ba22448445d4e6706513c537f1bd0c6c
Bug 1290337 - Part 6: Replace Value parameter to const Value& in RootingAPI. r=terrence

https://hg.mozilla.org/integration/mozilla-inbound/rev/76234719298ad869a44d6d2272db71b1fd8d7278
Bug 1290337 - Part 7: Replace Value parameter to const Value& in GC. r=terrence

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5865e178cc442982a37088902945487488377ee
Bug 1290337 - Part 8: Use const reference in VoidDefaultAdaptor and BoolDefaultAdaptor. r=terrence

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2d5ee669e9c0fa4e14d74e052d526b286c0395
Bug 1290337 - Part 9: Replace NumLit parameter to const NumLit&. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/c05dd2167ba573dee54a0626d9539c973103b7ef
Bug 1290337 - Part 10: Replace WrapperValue parameter tor const WrapperValue&. r=terrence

https://hg.mozilla.org/integration/mozilla-inbound/rev/9550e82a1bc0fdc7274998a7a97a591268ad32bd
Bug 1290337 - Part 11: Replace ConstantOrRegister parameter to const ConstantOrRegister&. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8795d891dba1086abc141de2a248d386f009085
Bug 1290337 - Part 12: Use const reference in PodSet. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb25baa6f83cc05b2e44c896d4bb922904ac0c3
Bug 1290337 - Part 13: Replace Value parameter to const Value& in finalizeInBackground. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/677eeee9a9a932fb010ba103db85dd54da664807
Bug 1290337 - Part 14: Replace Value parameter to HandleValue in js/ctypes. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea9e52ee4cee7ee9c2e815610d44eb6bfc61b70
Bug 1290337 - Part 15: Replace Value parameter to const Value& in js/xpconnect. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/0646390427e486957caef6f81360f92873b8b1f9
Bug 1290337 - Part 16: Replace Value parameter to const Value& in dom. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/06a188296ad2f189a77910156acf780f9837f119
Bug 1290337 - Part 17: Replace Value parameter to const Value& in dom/bindings. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/5df8e6617ec65c2c14183c814cac3597e5dd4db9
Bug 1290337 - Part 18: Replace Value parameter to const Value& in ipc. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a22273977e5ac0c81663ede57b6e280732e14f3c
Bug 1290337 - Part 19: Allocate JSErrorResult separately in WrappableJSErrorResult. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c62b2557b44af96ccf8913cacc9a71cd292c51
Bug 1290337 - Part 20: Replace Value parameter to const Value& in storage. r=mak
Keywords: leave-open
Attachment #8795144 - Flags: review?(jwalden+bmo)
Comment on attachment 8795145 [details] [diff] [review]
Part 23: Use alignas and MOZ_NON_PARAM attribute for jsval_layout.

Added alignas and MOZ_NON_PARAM to jsval_layout.
both will be moved to JS::Value in bug 1304191.
Attachment #8795145 - Flags: review?(jwalden+bmo)
Attachment #8795146 - Flags: review?(jwalden+bmo)
Comment on attachment 8796409 [details] [diff] [review]
Part 21: Add moz_non_param annotation. r=jwalden

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

If I read the bug correctly, this analysis is supposed to catch code that would be miscompiled on Windows, right?  Did you note that for now we don't have support for running the clang based static analyser on Windows?  froydnj is working on enabling that, but if this is really needed to detect miscompiles on Windows, you may not want to land your patches before Nathan has enabled the Windows static analysis builds...

::: build/clang-plugin/clang-plugin.cpp
@@ +1216,5 @@
> +                                hasAncestor(classTemplateSpecializationDecl()
> +                                                .bind("spec"))),
> +                          isDefinition()))
> +          .bind("func"),
> +      &NonParamInsideFunctionDecl);

If I'm reading these correctly, they're the same matcher.  Since CXXConstrucotrDecl is a subclass of CXXMethodDecl, the second matcher can match everything that the first one can, so you should be able to remove the first one.

@@ +1223,5 @@
> +                               hasAncestor(classTemplateSpecializationDecl()
> +                                               .bind("spec"))),
> +                         isDefinition()))
> +          .bind("func"),
> +      &NonParamInsideFunctionDecl);

And since CXXMethodDecl itself inherits from FunctionDecl, you can remove the second matcher too!

(Note that cxxConstructorDecl, cxxMethodDecl and functionDecl are all VariadicDynCastAllOfMatcher matchers, which basically means they will match whatever they can dyn_cast to the template argument type, in this case, FunctionDecl.  This is why the inheritance matters here.)

@@ +1895,5 @@
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Error, "Type %0 must not be used as parameter");
> +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "Please consider using const reference instead");

Nit: "Please consider passing a const reference instead"

::: build/clang-plugin/tests/TestNonParameterChecker.cpp
@@ +1,1 @@
> +#define MOZ_NON_PARAM __attribute__((annotate("moz_non_param")))

Don't you need to add this to Attributes.h too?  Since that's where we usually document these flags, you should do that in the same patch.

@@ +97,5 @@
> +    static void rawStatic(Param x) {}
> +    static void rawStatic(NonParam x) {} //expected-error {{Type 'NonParam' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    static void rawStatic(HasNonParamStruct x) {} //expected-error {{Type 'HasNonParamStruct' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    static void rawStatic(HasNonParamUnion x) {} //expected-error {{Type 'HasNonParamUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> +    static void rawStatic(HasNonParamStructUnion x) {} //expected-error {{Type 'HasNonParamStructUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}

You're missing tests that check that these are acceptable as a method.

I think testing this would be somewhat simpler if you refactor the above functions (raw, const_, array, ptr, ref, constRef and so on) into a header and #include it once in the global scope, and once inside class_, and similarly in the classes below.

@@ +193,5 @@
> +void testNestedTemplateClass()
> +{
> +    nestedTemplateOuter<NonParam> outer;
> +    NonParam nonParam;
> +    outer.constRef(nonParam); // FIXME: note for bad instantiation should be shown here

Why?  You aren't instantiating anything here, right?

@@ +218,5 @@
> +    tmplFuncForNonParamImplicit(nonParam); // FIXME: note for bad instantiation should be shown here
> +
> +    HasNonParamStruct hasNonParamStruct;
> +    tmplFuncForHasNonParamStruct<HasNonParamStruct>(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here
> +    tmplFuncForHasNonParamStructImplicit(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here

Why these comments?
Attachment #8796409 - Flags: review?(ehsan) → review-
* folded Part 22 into this, that had Attributes.h change.
  * removed redundant constructor and method matcher
  * fixed note text
  * moved function definitions to header (NonParameterTestCases.h)
  * template functions are still in TestNonParameterChecker.cpp
    as I want to catch those methods only if those are actually called


(In reply to :Ehsan Akhgari from comment #67)
> Comment on attachment 8796409 [details] [diff] [review]
> Part 21: Add moz_non_param annotation. r=jwalden
> 
> Review of attachment 8796409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I read the bug correctly, this analysis is supposed to catch code that
> would be miscompiled on Windows, right?  Did you note that for now we don't
> have support for running the clang based static analyser on Windows? 
> froydnj is working on enabling that, but if this is really needed to detect
> miscompiles on Windows, you may not want to land your patches before Nathan
> has enabled the Windows static analysis builds...

The purpose of this annotation is to catch the breakage of Windows VisualStudio build on non-Windows build.
If soneome adds raw JS::Value parameter, it will break Windows build only.  It's inconvenient, as the breakage might be overlooked until it's landed.
This annotation will catch the same thing on linux and osx static analysis build, to make it easier to catch it on try run, even if it doesn't contain windows build.


> @@ +97,5 @@
> > +    static void rawStatic(Param x) {}
> > +    static void rawStatic(NonParam x) {} //expected-error {{Type 'NonParam' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> > +    static void rawStatic(HasNonParamStruct x) {} //expected-error {{Type 'HasNonParamStruct' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> > +    static void rawStatic(HasNonParamUnion x) {} //expected-error {{Type 'HasNonParamUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> > +    static void rawStatic(HasNonParamStructUnion x) {} //expected-error {{Type 'HasNonParamStructUnion' must not be used as parameter}} expected-note {{Please consider using const reference instead}}
> 
> You're missing tests that check that these are acceptable as a method.

They're not acceptable as a method, as tested above.
I'm testing they also throw error for static methods.


> I think testing this would be somewhat simpler if you refactor the above
> functions (raw, const_, array, ptr, ref, constRef and so on) into a header
> and #include it once in the global scope, and once inside class_, and
> similarly in the classes below.

separated the class to `class_` and `classWithStatic`, and test same things with/without static.


> @@ +193,5 @@
> > +void testNestedTemplateClass()
> > +{
> > +    nestedTemplateOuter<NonParam> outer;
> > +    NonParam nonParam;
> > +    outer.constRef(nonParam); // FIXME: note for bad instantiation should be shown here
> 
> Why?  You aren't instantiating anything here, right?

I want to note where nestedTemplateOuter is used, but it's not possible as we can catch only one enclosing template specialization, as it helps understanding which type is actually used as template parameter (NonParam here).
maybe I'm using wrong term?


> @@ +218,5 @@
> > +    tmplFuncForNonParamImplicit(nonParam); // FIXME: note for bad instantiation should be shown here
> > +
> > +    HasNonParamStruct hasNonParamStruct;
> > +    tmplFuncForHasNonParamStruct<HasNonParamStruct>(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here
> > +    tmplFuncForHasNonParamStructImplicit(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here
> 
> Why these comments?

Here too, I want to note where tmplFuncForHasNonParamStructImplicit is used, in addition to show error on tmplFuncForHasNonParamStructImplicit definition that is template, as showing note on the actual use will help understanding which type is used as template parameter.
Attachment #8795143 - Attachment is obsolete: true
Attachment #8795144 - Attachment is obsolete: true
Attachment #8795144 - Flags: review?(jwalden+bmo)
Attachment #8796649 - Flags: review?(ehsan)
Comment on attachment 8795145 [details] [diff] [review]
Part 23: Use alignas and MOZ_NON_PARAM attribute for jsval_layout.

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

::: js/public/Value.h
@@ +220,5 @@
>  } JSWhyMagic;
>  
>  #if defined(IS_LITTLE_ENDIAN)
>  # if defined(JS_NUNBOX32)
> +typedef union MOZ_NON_PARAM alignas(8) jsval_layout

If we're touching these lines, get rid of the typedef and trailing jsval_layout, and just have the union declaration introduce the jsval_layout name.  And for all the others.  (I wonder if there's a clang-tidy action to remove redundant typedefs...)
Attachment #8795145 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8795146 [details] [diff] [review]
Part 24: Replace JS_ALIGNMENT_OF with alignof.

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

::: js/src/jsscript.cpp
@@ +2335,1 @@
>       sizeof(T) % sizeof(JS::Value) == 0)

Convert these into constexpr functions, i.e.

template<class T>
constexpr bool
KeepsValueAlignment()
{
    return alignof(JS::Value) % alignof(T) == 0 &&
           sizeof(T) % sizeof(JS::Value) == 0;
}

and then KeepsValueAlignment<ConstArray>(), say, at the use sites.  Same for the other two macros.  And get rid of the all-caps since they're not macros, of course.
Attachment #8795146 - Flags: review?(jwalden+bmo) → review+
Attachment #8796409 - Attachment is obsolete: true
(In reply to Tooru Fujisawa [:arai] from comment #68)
> (In reply to :Ehsan Akhgari from comment #67)
> > Comment on attachment 8796409 [details] [diff] [review]
> > Part 21: Add moz_non_param annotation. r=jwalden
> > 
> > Review of attachment 8796409 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > If I read the bug correctly, this analysis is supposed to catch code that
> > would be miscompiled on Windows, right?  Did you note that for now we don't
> > have support for running the clang based static analyser on Windows? 
> > froydnj is working on enabling that, but if this is really needed to detect
> > miscompiles on Windows, you may not want to land your patches before Nathan
> > has enabled the Windows static analysis builds...
> 
> The purpose of this annotation is to catch the breakage of Windows
> VisualStudio build on non-Windows build.
> If soneome adds raw JS::Value parameter, it will break Windows build only. 
> It's inconvenient, as the breakage might be overlooked until it's landed.
> This annotation will catch the same thing on linux and osx static analysis
> build, to make it easier to catch it on try run, even if it doesn't contain
> windows build.

Yes, your patch achieves the goals stated above, but it will not catch such code if it is only compiled on Windows, and we have a good amount of Windows specific code in the tree.  I just want to be explicit that this patch is insufficient for detecting all such breakages for that reason.

> > @@ +193,5 @@
> > > +void testNestedTemplateClass()
> > > +{
> > > +    nestedTemplateOuter<NonParam> outer;
> > > +    NonParam nonParam;
> > > +    outer.constRef(nonParam); // FIXME: note for bad instantiation should be shown here
> > 
> > Why?  You aren't instantiating anything here, right?
> 
> I want to note where nestedTemplateOuter is used, but it's not possible as
> we can catch only one enclosing template specialization, as it helps
> understanding which type is actually used as template parameter (NonParam
> here).
> maybe I'm using wrong term?

Yes you are.  Template instantiation is what happens when the compiler needs to generate the real code from a template by replacing all template arguments with types/values, and that happens on the |nestedTemplateOuter<NonParam> outer;| line here.  Perhaps you should reword the note to say "The bad argument was passed to constRef here"?

> > @@ +218,5 @@
> > > +    tmplFuncForNonParamImplicit(nonParam); // FIXME: note for bad instantiation should be shown here
> > > +
> > > +    HasNonParamStruct hasNonParamStruct;
> > > +    tmplFuncForHasNonParamStruct<HasNonParamStruct>(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here
> > > +    tmplFuncForHasNonParamStructImplicit(hasNonParamStruct); // FIXME: note for bad instantiation should be shown here
> > 
> > Why these comments?
> 
> Here too, I want to note where tmplFuncForHasNonParamStructImplicit is used,
> in addition to show error on tmplFuncForHasNonParamStructImplicit definition
> that is template, as showing note on the actual use will help understanding
> which type is used as template parameter.

Ditto, same as above.
Comment on attachment 8796649 [details] [diff] [review]
Part 22: Add moz_non_param annotation. r=jwalden

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

r=me with the comments below addressed.  Thanks!

::: build/clang-plugin/clang-plugin.cpp
@@ +1883,5 @@
> +      DiagnosticIDs::Error, "Type %0 must not be used as parameter");
> +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "Please consider passing a const reference instead");
> +  unsigned SpecNoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note, "bad instantiation of %0 requested here");

Please revise this as per my comment before.

::: mfbt/Attributes.h
@@ +465,5 @@
>   *   and to include the marked function in the scan mechanism that determines witch
>   *   member variables still remain uninitialized.
> + * MOZ_NON_PARAM: Applies to types. Makes it compile time error to use the type
> + *   in parameter without pointer or reference.  This is intended to catch the
> + *   MSVC's restriction that |alignas| cannot be applied to parameter type.

The static analysis is in no way specific to the MSVC restriction, so while it is the motavating factor for adding it, people may want to use it for other purposes, so I'd drop the second sentence.
Attachment #8796649 - Flags: review?(ehsan) → review+
Attachment #8795147 - Flags: review?(shu) → review+
Attachment #8795148 - Flags: review?(shu) → review+
(In reply to :Ehsan Akhgari from comment #73)
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +1883,5 @@
> > +      DiagnosticIDs::Error, "Type %0 must not be used as parameter");
> > +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> > +      DiagnosticIDs::Note, "Please consider passing a const reference instead");
> > +  unsigned SpecNoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> > +      DiagnosticIDs::Note, "bad instantiation of %0 requested here");
> 
> Please revise this as per my comment before.

the note is used only for the following lines.
Do you mean the following notes are also wrong?

>     tmplClassForNonParam<NonParam> nonParamClass; //expected-note 3 {{bad instantiation of 'tmplClassForNonParam<NonParam>' requested here}}
> ...
>     tmplClassForHasNonParamStruct<HasNonParamStruct> hasNonParamStructClass;//expected-note 3 {{bad instantiation of 'tmplClassForHasNonParamStruct<HasNonParamStruct>' requested here}}
> ...
>         NestedTemplateInner<T> inner; //expected-note {{bad instantiation of 'NestedTemplateInner<NonParam>' requested here}}
Flags: needinfo?(ehsan)
(In reply to Tooru Fujisawa [:arai] from comment #74)
> (In reply to :Ehsan Akhgari from comment #73)
> > ::: build/clang-plugin/clang-plugin.cpp
> > @@ +1883,5 @@
> > > +      DiagnosticIDs::Error, "Type %0 must not be used as parameter");
> > > +  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> > > +      DiagnosticIDs::Note, "Please consider passing a const reference instead");
> > > +  unsigned SpecNoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
> > > +      DiagnosticIDs::Note, "bad instantiation of %0 requested here");
> > 
> > Please revise this as per my comment before.
> 
> the note is used only for the following lines.
> Do you mean the following notes are also wrong?
> 
> >     tmplClassForNonParam<NonParam> nonParamClass; //expected-note 3 {{bad instantiation of 'tmplClassForNonParam<NonParam>' requested here}}
> > ...
> >     tmplClassForHasNonParamStruct<HasNonParamStruct> hasNonParamStructClass;//expected-note 3 {{bad instantiation of 'tmplClassForHasNonParamStruct<HasNonParamStruct>' requested here}}
> > ...
> >         NestedTemplateInner<T> inner; //expected-note {{bad instantiation of 'NestedTemplateInner<NonParam>' requested here}}

Yeah, both need to be fixed.  See the second paragraph of my response in comment 72 please.  Thanks!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #75)
> Yeah, both need to be fixed.  See the second paragraph of my response in
> comment 72 please.  Thanks!

thanks, fixed locally.
I'll land remaining patches at the same time as bug 1304191.
Fixed newly added code that uses raw Value.
Attachment #8801454 - Flags: review?(sphink)
Also fixed newly added code that possibly uses raw Value.

> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/js/src/Unified_cpp_js_src15.cpp:2:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/jit/EffectiveAddressAnalysis.cpp:7:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/jit/EffectiveAddressAnalysis.h:10:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/jit/MIRGenerator.h:18:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/jscompartment.h:17:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/asmjs/WasmCompartment.h:22:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/asmjs/WasmJS.h:22:
> 19:40:00     INFO -  In file included from /home/worker/workspace/build/src/js/src/asmjs/WasmTypes.h:32:
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:51:41: error: Type 'JS::Value' must not be used as parameter
> 19:40:00     INFO -      template<class U> MOZ_IMPLICIT Raw(U) = delete;
> 19:40:00     INFO -                                          ^
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:51:41: note: Please consider passing a const reference instead
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:590:44: note: The bad argument was passed to 'Raw' here
> 19:40:00     INFO -      MOZ_MUST_USE bool writeFixedF64(RawF64 d) {
> 19:40:00     INFO -                                             ^
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:51:41: error: Type 'JS::Value' must not be used as parameter
> 19:40:00     INFO -      template<class U> MOZ_IMPLICIT Raw(U) = delete;
> 19:40:00     INFO -                                          ^
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:51:41: note: Please consider passing a const reference instead
> 19:40:00     INFO -  /home/worker/workspace/build/src/js/src/asmjs/WasmBinary.h:587:44: note: The bad argument was passed to 'Raw' here
> 19:40:00     INFO -      MOZ_MUST_USE bool writeFixedF32(RawF32 f) {
> 19:40:00     INFO -                                             ^
Attachment #8801456 - Flags: review?(bbouvier)
I'll renumber previous Part 21- patches of course.
Comment on attachment 8801456 [details] [diff] [review]
Part 22: Use const reference in Raw::Raw.

Clearing r? for now.
will investigate what's actually happening there.
Attachment #8801456 - Flags: review?(bbouvier)
Comment on attachment 8801456 [details] [diff] [review]
Part 22: Use const reference in Raw::Raw.

Okay, so the issue was that overloaded function's parameter.
Here's minimal case that hits the issue:

> #include "js/Value.h"
> 
> template<class T>
> class Raw
> {
>   public:
>     explicit Raw(T value) {}
> 
>     template<class U> MOZ_IMPLICIT Raw(U) = delete;
> };
> 
> using RawF64 = Raw<double>;
> using RawF32 = Raw<float>;
> 
> class MConstant
> {
>   public:
>     static void New(const JS::Value& v);
>     static void New(RawF32 bits);
>     static void New(RawF64 bits);
> };
> 
> void foo() {
>     (void) MConstant::New(JS::Int32Value(1));
> }


MConstant::New in MIR.h has overload for wasm::RawF32/wasm::RawF64/Value, and
passing JS::Value to MConstant::New creates clang's AST for JS::Value specialization for Raw::Raw,
while it's not actually used.

I'll change moz_non_param to ignore deleted function.
Attachment #8801456 - Attachment is obsolete: true
Attachment #8796649 - Attachment description: Part 21: Add moz_non_param annotation. r=jwalden → Part 22: Add moz_non_param annotation. r=jwalden
Followup for moz_non_param annotation.
Due to comment #81 issue, changed it to ignore deleted function.

deleted function's case should be simply caught by standard compiler error.
Attachment #8801719 - Flags: review?(ehsan)
Attachment #8801454 - Flags: review?(sphink) → review+
Comment on attachment 8801719 [details] [diff] [review]
Part 22.1: Do not check deleted function parameter.

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

Nice catch!
Attachment #8801719 - Flags: review?(ehsan) → review+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.