Closed
Bug 1290337
Opened 8 years ago
Closed 8 years ago
Replace JSVAL_ALIGNMENT with alignas(8)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(28 files, 8 obsolete files)
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.
Assignee | ||
Comment 1•8 years ago
|
||
Try is running. https://treeherder.mozilla.org/#/jobs?repo=try&revision=63eb88b400ad
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 3•8 years ago
|
||
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. > */
Assignee | ||
Comment 4•8 years ago
|
||
only windows 32bit builds failed with parameter issue. We will need some workaround for it...
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Here's try run with applying alignas(8) for non-MSVC compilers. https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ecf78c4e20
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
and actually I'm now experiencing difficultly on finding the raw Value parameters... :/
Assignee | ||
Comment 10•8 years ago
|
||
almost working https://treeherder.mozilla.org/#/jobs?repo=try&revision=834fec2618a1b8494f7e174e334b8831bd1a3687 https://treeherder.mozilla.org/#/jobs?repo=try&revision=d103db5e76209b69781c72213c98cc272cb62158 there's some failure maybe related to GC, will investigate.
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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)) {}
Assignee | ||
Comment 14•8 years ago
|
||
Updated the template parameter
Attachment #8791365 -
Attachment is obsolete: true
Attachment #8791365 -
Flags: review?(nicolas.b.pierron)
Attachment #8791410 -
Flags: review?(jwalden+bmo)
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Assignee | ||
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
ValueToScript and PrintHelpString should receive HandleValue instead of raw Value.
Attachment #8791691 -
Flags: review?(sphink)
Assignee | ||
Comment 20•8 years ago
|
||
also changed simple raw Value cases to const reference, in JIT.
Attachment #8791692 -
Flags: review?(nicolas.b.pierron)
Updated•8 years ago
|
Attachment #8791692 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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 23•8 years ago
|
||
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+
Assignee | ||
Comment 24•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8791689 -
Flags: review?(shu) → review+
Assignee | ||
Comment 25•8 years ago
|
||
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)
Assignee | ||
Comment 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
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)
Assignee | ||
Comment 28•8 years ago
|
||
changed NumLit to receive const Value&, and changed NumLit parameter to const NumLit&, as it has Value.
Attachment #8791879 -
Flags: review?(bbouvier)
Assignee | ||
Comment 29•8 years ago
|
||
changed WrapperValue parameter to const WrapperValue&, as it has Value.
Attachment #8791880 -
Flags: review?(terrence)
Assignee | ||
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
PodSet can be called with JS::Value, so changed the aSrc parameter to const reference.
Attachment #8791882 -
Flags: review?(sphink)
Updated•8 years ago
|
Attachment #8791879 -
Flags: review?(bbouvier) → review+
Updated•8 years ago
|
Attachment #8791881 -
Flags: review?(nicolas.b.pierron) → review+
Updated•8 years ago
|
Attachment #8791882 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 32•8 years ago
|
||
simply changed Value to const reference in finalizeInBackground method, including binding codegen.
Attachment #8792265 -
Flags: review?(sphink)
Assignee | ||
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
simply changed Value to const reference, in js/xpconnect
Attachment #8792267 -
Flags: review?(jdemooij)
Assignee | ||
Comment 35•8 years ago
|
||
simply changed JS::Value parameter to const reference.
Attachment #8792268 -
Flags: review?(bugs)
Assignee | ||
Comment 36•8 years ago
|
||
also changed JS::Value parameter to const reference in dom/bindings
Attachment #8792269 -
Flags: review?(bugs)
Assignee | ||
Comment 37•8 years ago
|
||
Attachment #8792270 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8791876 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8791877 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8791878 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8791880 -
Flags: review?(terrence) → review+
Updated•8 years ago
|
Attachment #8792265 -
Flags: review?(sphink) → review+
Updated•8 years ago
|
Attachment #8792267 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8792268 -
Flags: review?(bugs) → review+
Comment 38•8 years ago
|
||
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 39•8 years ago
|
||
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+
Comment 40•8 years ago
|
||
So, arai, could you explain the requirement why JS::Value -> const JS::Value& is needed and when.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 41•8 years ago
|
||
(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)
Comment 42•8 years ago
|
||
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?
Assignee | ||
Comment 43•8 years ago
|
||
(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.
Comment 44•8 years ago
|
||
(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)
Assignee | ||
Comment 45•8 years ago
|
||
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
Assignee | ||
Comment 46•8 years ago
|
||
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)
Assignee | ||
Comment 47•8 years ago
|
||
simply replaced JS::Value parameter to const reference.
Attachment #8793249 -
Flags: review?(mak77)
Comment 48•8 years ago
|
||
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 49•8 years ago
|
||
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+
Assignee | ||
Comment 50•8 years ago
|
||
thanks :) here's updated patch that uses UniquePtr.
Attachment #8793248 -
Attachment is obsolete: true
Attachment #8793387 -
Flags: review?(bugs)
Assignee | ||
Comment 51•8 years ago
|
||
updated default ctor to initialize mRv
Attachment #8793387 -
Attachment is obsolete: true
Attachment #8793387 -
Flags: review?(bugs)
Attachment #8793389 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8793389 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 52•8 years ago
|
||
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 53•8 years ago
|
||
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+
Assignee | ||
Comment 54•8 years ago
|
||
Now it can catch more cases (almost all cases, I think). https://treeherder.mozilla.org/#/jobs?repo=try&revision=1abe270cfc30 will post patches later.
Updated•8 years ago
|
Attachment #8793249 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 55•8 years ago
|
||
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
Assignee | ||
Comment 56•8 years ago
|
||
Assignee | ||
Comment 57•8 years ago
|
||
replaced JSVAL_ALIGNMENT with |MOZ_NON_PARAM alignas(8)|
Assignee | ||
Comment 58•8 years ago
|
||
now we can also use alignof. so replaced JS_ALIGNMENT_OF with alignof.
Assignee | ||
Comment 59•8 years ago
|
||
Changed some const Value& parameters for functions that receives JSContext to HandleValue.
Attachment #8795147 -
Flags: review?(shu)
Assignee | ||
Comment 60•8 years ago
|
||
Attachment #8795148 -
Flags: review?(shu)
Assignee | ||
Comment 61•8 years ago
|
||
followup for Part 4. Changed |unsigned argc, Value* argv| to |CallArgs&|.
Attachment #8795151 -
Flags: review?(terrence.d.cole)
Updated•8 years ago
|
Attachment #8795151 -
Flags: review?(terrence.d.cole) → review+
Assignee | ||
Comment 62•8 years ago
|
||
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 63•8 years ago
|
||
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+
Assignee | ||
Comment 64•8 years ago
|
||
Attachment #8796409 -
Flags: review?(ehsan)
Assignee | ||
Comment 65•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Updated•8 years ago
|
Attachment #8795144 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 66•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8795146 -
Flags: review?(jwalden+bmo)
Comment 67•8 years ago
|
||
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-
Assignee | ||
Comment 68•8 years ago
|
||
* 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 69•8 years ago
|
||
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 70•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8796409 -
Attachment is obsolete: true
Comment 71•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/95c5ce6ed75d https://hg.mozilla.org/mozilla-central/rev/a3a7f0fa1186 https://hg.mozilla.org/mozilla-central/rev/9796ed81f17a https://hg.mozilla.org/mozilla-central/rev/f81c58393110 https://hg.mozilla.org/mozilla-central/rev/9f0b9ebc2f1f https://hg.mozilla.org/mozilla-central/rev/6dbf0a82ba22 https://hg.mozilla.org/mozilla-central/rev/76234719298a https://hg.mozilla.org/mozilla-central/rev/e5865e178cc4 https://hg.mozilla.org/mozilla-central/rev/7e2d5ee669e9 https://hg.mozilla.org/mozilla-central/rev/c05dd2167ba5 https://hg.mozilla.org/mozilla-central/rev/9550e82a1bc0 https://hg.mozilla.org/mozilla-central/rev/b8795d891dba https://hg.mozilla.org/mozilla-central/rev/3fb25baa6f83 https://hg.mozilla.org/mozilla-central/rev/677eeee9a9a9 https://hg.mozilla.org/mozilla-central/rev/3ea9e52ee4ce https://hg.mozilla.org/mozilla-central/rev/0646390427e4 https://hg.mozilla.org/mozilla-central/rev/06a188296ad2 https://hg.mozilla.org/mozilla-central/rev/5df8e6617ec6 https://hg.mozilla.org/mozilla-central/rev/a22273977e5a https://hg.mozilla.org/mozilla-central/rev/a2c62b2557b4
Comment 72•8 years ago
|
||
(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 73•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8795147 -
Flags: review?(shu) → review+
Updated•8 years ago
|
Attachment #8795148 -
Flags: review?(shu) → review+
Assignee | ||
Comment 74•8 years ago
|
||
(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)
Comment 75•8 years ago
|
||
(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)
Assignee | ||
Comment 76•8 years ago
|
||
(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.
Assignee | ||
Comment 77•8 years ago
|
||
Fixed newly added code that uses raw Value.
Attachment #8801454 -
Flags: review?(sphink)
Assignee | ||
Comment 78•8 years ago
|
||
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)
Assignee | ||
Comment 79•8 years ago
|
||
I'll renumber previous Part 21- patches of course.
Assignee | ||
Comment 80•8 years ago
|
||
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)
Assignee | ||
Comment 81•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8796649 -
Attachment description: Part 21: Add moz_non_param annotation. r=jwalden → Part 22: Add moz_non_param annotation. r=jwalden
Assignee | ||
Comment 82•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8801454 -
Flags: review?(sphink) → review+
Comment 83•8 years ago
|
||
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+
Assignee | ||
Comment 84•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c4496adb2bfd8620e74d9394f5a13228d0bfbc Bug 1290337 - Part 21: Use const reference in BarrierMethods::exposeToJS. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/0f461766e3e8ca3753f7e818ac8e999dd9dbe158 Bug 1290337 - Part 22: Add moz_non_param annotation. r=ehsan,jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/845980c4ce45f77358c92414f8911c70c7040852 Bug 1290337 - Part 22.1: Do not check deleted function parameter. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/e519304465c1172c3ac4dcc2b5510d0f5c985b1e Bug 1290337 - Part 23: Use alignas and MOZ_NON_PARAM attribute for jsval_layout. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/e7194a291ff3e054538a7c6c3a4e0ea0b22d3112 Bug 1290337 - Part 24: Replace JS_ALIGNMENT_OF with alignof. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/1190abb16e18347ca1640a6ae90c3ab4de38e73b Bug 1290337 - Part 25: Use HandleValue in ToNumber and ToNumberSlow. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6c42c1429b3fde4ab7df4c4dba00662118ebf2 Bug 1290337 - Part 26: Use HandleValue in ToPropertyKey. r=shu https://hg.mozilla.org/integration/mozilla-inbound/rev/e81fec3ed9903da002e0ea3241c7a50dd524d64d Bug 1290337 - Part 27: Pass CallArgs& to GetScriptAndPCArgs. r=terrence
Comment 85•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1c4496adb2b https://hg.mozilla.org/mozilla-central/rev/0f461766e3e8 https://hg.mozilla.org/mozilla-central/rev/845980c4ce45 https://hg.mozilla.org/mozilla-central/rev/e519304465c1 https://hg.mozilla.org/mozilla-central/rev/e7194a291ff3 https://hg.mozilla.org/mozilla-central/rev/1190abb16e18 https://hg.mozilla.org/mozilla-central/rev/0e6c42c1429b https://hg.mozilla.org/mozilla-central/rev/e81fec3ed990
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
affected → ---
status-firefox52:
--- → fixed
Keywords: leave-open
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•