Bug 1287006 comment 42 says: > In principle we *could* just make static analysis detect any parameter that's > a class, one of whose members has an alignas() attribute. Then, no need for > the attribute at all. Given MOZ_NON_PARAM exists *only* for that purpose, it's > something we could at least consider -- although EIBTI is at least one argument > against doing so (not sure if it's persuasive or not, on first thought). Removing MOZ_NON_PARAM and adding a static analysis check for alignas() members in the current class, any superclasses, or members of member types seems like a good idea, given that folks may not even be aware that alignas might have ABI implications (bug 1287006 comment 26).

(In reply to Nathan Froyd [:froydnj] from comment #0) > Bug 1287006 comment 42 says: > > > In principle we *could* just make static analysis detect any parameter that's > > a class, one of whose members has an alignas() attribute. Then, no need for > > the attribute at all. Given MOZ_NON_PARAM exists *only* for that purpose, it's > > something we could at least consider -- although EIBTI is at least one argument > > against doing so (not sure if it's persuasive or not, on first thought). > > Removing MOZ_NON_PARAM and adding a static analysis check for alignas() > members in the current class, any superclasses, or members of member types > seems like a good idea, given that folks may not even be aware that alignas > might have ABI implications (bug 1287006 comment 26). So the idea here would effectively be to make alignas() imply MOZ_NON_PARAM?

(In reply to Michael Layzell [:mystor] from comment #1) > So the idea here would effectively be to make alignas() imply MOZ_NON_PARAM? Right. I guess js::Value is MOZ_NON_PARAM and that should probably stick around, so we couldn't completely eliminate MOZ_NON_PARAM. =/

JS::Value is alignas(8), so detecting a parameter type that's a class with alignas, or that has a base class with alignas, or a member with alignas, would sweep up that, too.

MozReview-Commit-ID: 2VDJRxxkVjV

MozReview-Commit-ID: BGg1yS2Ovhz

MozReview-Commit-ID: J3m2uzv7jW0

Comment on attachment 8837330 [details] [diff] [review] Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types Review of attachment 8837330 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/tests/TestNonParameterChecker.cpp @@ +185,5 @@ > +void takesAlignasStructByRef(const AlignasStruct& x) { } > + > +struct AlignasMember { alignas(8) char a; }; > +void takesAlignasMember(AlignasMember x) { } // expected-error {{Type 'AlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} > +void takesAlignasMemberByRef(const AlignasMember& x) { } This limitation also applies to structs that contain structs that contain alignas stuff, and to base classes: struct AlignasMemberOfMember { AlignasMember mem; }; void takesAlignasMemberOfMember(AlignasMemberOfMember x) { } // expected-error {{Type 'AlignasMemberOfMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} void takesAlignasMemberOfMember(const AlignasMemberOfMember& x) { } struct BaseAlignas : AlignasStruct { }; void takesBaseAlignas(BaseAlignas x) { } // expected-error {{Type 'BaseAlignas' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} void takesBaseAlignas(const BaseAlignas& x) { } struct BaseAlignasMember : AlignasMember { }; void takesBaseAlignasMember(BaseAlignasMember x) { } // expected-error {{Type 'BaseAlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} void takesBaseAlignasMember(const BaseAlignasMember& x) { } struct BaseAlignasMemberOfMember : AlignasMemberOfMember { }; void takesBaseAlignasMemberOfMember(BaseAlignasMemberOfMember x) { } // expected-error {{Type 'BaseAlignasMemberOfMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} void takesBaseAlignasMemberOfMember(const BaseAlignasMemberOfMember& x) { } I could easily be misreading the patch, but it appears to only go one level deep and would miss some of these subtilties. I think.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7) > Comment on attachment 8837330 [details] [diff] [review] > Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) > types > > Review of attachment 8837330 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/clang-plugin/tests/TestNonParameterChecker.cpp > @@ +185,5 @@ > > +void takesAlignasStructByRef(const AlignasStruct& x) { } > > + > > +struct AlignasMember { alignas(8) char a; }; > > +void takesAlignasMember(AlignasMember x) { } // expected-error {{Type 'AlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} > > +void takesAlignasMemberByRef(const AlignasMember& x) { } > > This limitation also applies to structs that contain structs that contain > alignas stuff, and to base classes: > > struct AlignasMemberOfMember { AlignasMember mem; }; > void takesAlignasMemberOfMember(AlignasMemberOfMember x) { } // > expected-error {{Type 'AlignasMemberOfMember' must not be used as > parameter}} expected-note {{Please consider passing a const reference > instead}} > void takesAlignasMemberOfMember(const AlignasMemberOfMember& x) { } > > struct BaseAlignas : AlignasStruct { }; > void takesBaseAlignas(BaseAlignas x) { } // expected-error {{Type > 'BaseAlignas' must not be used as parameter}} expected-note {{Please > consider passing a const reference instead}} > void takesBaseAlignas(const BaseAlignas& x) { } > > struct BaseAlignasMember : AlignasMember { }; > void takesBaseAlignasMember(BaseAlignasMember x) { } // expected-error > {{Type 'BaseAlignasMember' must not be used as parameter}} expected-note > {{Please consider passing a const reference instead}} > void takesBaseAlignasMember(const BaseAlignasMember& x) { } > > struct BaseAlignasMemberOfMember : AlignasMemberOfMember { }; > void takesBaseAlignasMemberOfMember(BaseAlignasMemberOfMember x) { } // > expected-error {{Type 'BaseAlignasMemberOfMember' must not be used as > parameter}} expected-note {{Please consider passing a const reference > instead}} > void takesBaseAlignasMemberOfMember(const BaseAlignasMemberOfMember& x) { } > > I could easily be misreading the patch, but it appears to only go one level > deep and would miss some of these subtilties. I think. Nope, this test uses the same infrastructure as MOZ_NON_PARAM and should go more than one level deep. Both of the examples which it caught in parts 2 and 3 were many levels deep before hitting the place where alignas() was used. I'll add some more tests however, just to make that clear.

Comment on attachment 8837330 [details] [diff] [review] Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types Review of attachment 8837330 [details] [diff] [review]: ----------------------------------------------------------------- I think reusing the non-param infrastructure is fine, but we should produce a better diagnostic that explains when a type is complained about because it was MOZ_NON_PARAM or because of alignas().

Comment on attachment 8837331 [details] [diff] [review] Part 2: Pass IntIntervals by const reference in TestIntervalSet Review of attachment 8837331 [details] [diff] [review]: ----------------------------------------------------------------- thank you

Comment on attachment 8837332 [details] [diff] [review] Part 3: Pass FilterPrimitiveDescription by const reference in AddLightingAttributes Review of attachment 8837332 [details] [diff] [review]: ----------------------------------------------------------------- Huh, not sure what I was thinking there. This looks much better.

This allows for the alignas(_) case to be distinguished from the MOZ_NON_PARAM case through notes. The following is what the IntInterval bug fixed in Part 2 of this bug looks like with the new diagnostics: /home/mlayzell/Code/moz/working/dom/media/gtest/TestIntervalSet.cpp:196:47: error: Type 'IntIntervals' (aka 'IntervalSet<int>') must not be used as parameter static void GeneratePermutations(IntIntervals aI1, ^ /home/mlayzell/Code/moz/working/dom/media/gtest/TestIntervalSet.cpp:196:47: note: Please consider passing a const reference instead /home/mlayzell/Code/moz/working/dom/media/Intervals.h:695:17: note: 'IntIntervals' (aka 'IntervalSet<int>') is a non-param type because member 'mIntervals' is a non-param type 'ContainerType' (aka 'AutoTArray<Interval<int>, 4>') ContainerType mIntervals; ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3: note: 'ContainerType' (aka 'AutoTArray<Interval<int>, 4>') is a non-param type because member '' is a non-param type 'union (anonymous union at /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3)' union ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2477:72: note: 'union (anonymous union at /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3)' is a non-param type because member 'mAlign' is a non-param type 'mozilla::AlignedElem<(mozilla::AlignmentFinder<Header>::alignment > mozilla::AlignmentFinder<elem_type>::alignment) ? mozilla::AlignmentFinder<Header>::alignment : mozilla::AlignmentFinder<elem_type>::alignment>' MOZ_ALIGNOF(Header) : MOZ_ALIGNOF(elem_type)> mAlign; ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:85:8: note: 'mozilla::AlignedElem<(mozilla::AlignmentFinder<Header>::alignment > mozilla::AlignmentFinder<elem_type>::alignment) ? mozilla::AlignmentFinder<Header>::alignment : mozilla::AlignmentFinder<elem_type>::alignment>' is a non-param type because member 'elem' has an alignas(_) annotation struct AlignedElem<4> ^ MozReview-Commit-ID: 4KIbzEKnmNU

Comment on attachment 8837330 [details] [diff] [review] Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types re-requesting review given part 4 improving the diagnostic messages.

Comment on attachment 8837330 [details] [diff] [review] Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types Review of attachment 8837330 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay.

Comment on attachment 8838773 [details] [diff] [review] Part 4: Produce better annotation reason diagnostics for implicit annotations Review of attachment 8838773 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/MemMoveAnnotation.h @@ +52,2 @@ > } > + return "it is an stl-provided type not guaranteed to be memmove-able"; Nit: STL.

Backed out for failing in cubeb_audiounit.cpp:165 on OSX: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f5e363685af64c5d8a5c83f64df525ec22ec818 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8a33e4051a3138db75ab544a2bfc2f94adef01 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b53302c7924134147273bcfc63c60a1491fbdec https://hg.mozilla.org/integration/mozilla-inbound/rev/609ed9f634a4270237abfa58c121d976d374a7d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffbf8ae7d06dcb05f1385784c1642b66ea54732 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6e48de2c2065d614a39e5d8c7a0b7bfa23dba830&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86303146&repo=mozilla-inbound [task 2017-03-24T20:14:14.885268Z] 20:14:14 INFO - /home/worker/workspace/build/src/sccache2/sccache /home/worker/workspace/build/src/clang/bin/clang++ -target x86_64-apple-darwin11 -B /home/worker/workspace/build/src/cctools/bin -isysroot /home/worker/workspace/build/src/MacOSX10.7.sdk -std=gnu++11 -o regextxt.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG=1 -DTRIMMED=1 -DU_I18N_IMPLEMENTATION -DUCONFIG_NO_BREAK_ITERATION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_LEGACY_CONVERSION -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_CHARSET_IS_UTF8 -I/home/worker/workspace/build/src/config/external/icu/i18n -I/home/worker/workspace/build/src/obj-firefox/config/external/icu/i18n -I/home/worker/workspace/build/src/intl/icu/source/common -I/home/worker/workspace/build/src/obj-firefox/dist/include -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /home/worker/workspace/build/src/obj-firefox/mozilla-config.h -MD -MP -MF .deps/regextxt.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wc++1z-compat -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /home/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.dylib -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -frtti /home/worker/workspace/build/src/intl/icu/source/i18n/regextxt.cpp [task 2017-03-24T20:14:15.272034Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:165:28: error: Type 'downmix_func' (aka 'function<void (float *const, long, float *, unsigned int, unsigned int, cubeb_channel_layout, cubeb_channel_layout)>') must not be used as parameter [task 2017-03-24T20:14:15.277012Z] 20:14:15 INFO - mixing_impl(downmix_func dmfunc) { [task 2017-03-24T20:14:15.285176Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.285286Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:165:28: note: Please consider passing a const reference instead [task 2017-03-24T20:14:15.285363Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:1313:27: note: The bad argument was passed to 'mixing_impl' here [task 2017-03-24T20:14:15.285412Z] 20:14:15 INFO - stm->mixing.reset(new mixing_impl<float>(&cubeb_downmix_float)); [task 2017-03-24T20:14:15.285451Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.285574Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/functional:1566:53: note: 'downmix_func' (aka 'function<void (float *const, long, float *, unsigned int, unsigned int, cubeb_channel_layout, cubeb_channel_layout)>') is a non-param type because member '__buf_' is a non-param type 'typename aligned_storage<3 * sizeof(void *)>::type' [task 2017-03-24T20:14:15.285624Z] 20:14:15 INFO - typename aligned_storage<3*sizeof(void*)>::type __buf_; [task 2017-03-24T20:14:15.285667Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.285748Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/type_traits:1663:1: note: 'typename aligned_storage<3 * sizeof(void *)>::type' is a non-param type because it has an alignas(_) annotation [task 2017-03-24T20:14:15.285791Z] 20:14:15 INFO - _CREATE_ALIGNED_STORAGE_SPECIALIZATION(0x10); [task 2017-03-24T20:14:15.285815Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.285879Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/type_traits:1653:24: note: expanded from macro '_CREATE_ALIGNED_STORAGE_SPECIALIZATION' [task 2017-03-24T20:14:15.285914Z] 20:14:15 INFO - struct _ALIGNAS(n) type\ [task 2017-03-24T20:14:15.285942Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.286041Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:165:28: error: Type 'downmix_func' (aka 'function<void (short *const, long, short *, unsigned int, unsigned int, cubeb_channel_layout, cubeb_channel_layout)>') must not be used as parameter [task 2017-03-24T20:14:15.286079Z] 20:14:15 INFO - mixing_impl(downmix_func dmfunc) { [task 2017-03-24T20:14:15.286112Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.286176Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:165:28: note: Please consider passing a const reference instead [task 2017-03-24T20:14:15.286241Z] 20:14:15 INFO - /home/worker/workspace/build/src/media/libcubeb/src/cubeb_audiounit.cpp:1315:27: note: The bad argument was passed to 'mixing_impl' here [task 2017-03-24T20:14:15.286285Z] 20:14:15 INFO - stm->mixing.reset(new mixing_impl<short>(&cubeb_downmix_short)); [task 2017-03-24T20:14:15.286316Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.286437Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/functional:1566:53: note: 'downmix_func' (aka 'function<void (short *const, long, short *, unsigned int, unsigned int, cubeb_channel_layout, cubeb_channel_layout)>') is a non-param type because member '__buf_' is a non-param type 'typename aligned_storage<3 * sizeof(void *)>::type' [task 2017-03-24T20:14:15.286480Z] 20:14:15 INFO - typename aligned_storage<3*sizeof(void*)>::type __buf_; [task 2017-03-24T20:14:15.286519Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.286599Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/type_traits:1663:1: note: 'typename aligned_storage<3 * sizeof(void *)>::type' is a non-param type because it has an alignas(_) annotation [task 2017-03-24T20:14:15.286637Z] 20:14:15 INFO - _CREATE_ALIGNED_STORAGE_SPECIALIZATION(0x10); [task 2017-03-24T20:14:15.286659Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.286725Z] 20:14:15 INFO - /home/worker/workspace/build/src/clang/bin/../include/c++/v1/type_traits:1653:24: note: expanded from macro '_CREATE_ALIGNED_STORAGE_SPECIALIZATION' [task 2017-03-24T20:14:15.286758Z] 20:14:15 INFO - struct _ALIGNAS(n) type\ [task 2017-03-24T20:14:15.286785Z] 20:14:15 INFO - ^ [task 2017-03-24T20:14:15.287097Z] 20:14:15 INFO - 2 errors generated. [task 2017-03-24T20:14:15.287445Z] 20:14:15 INFO - /home/worker/workspace/build/src/config/rules.mk:1011: recipe for target 'cubeb_audiounit.o' failed [task 2017-03-24T20:14:15.287786Z] 20:14:15 INFO - gmake[5]: *** [cubeb_audiounit.o] Error 1 [task 2017-03-24T20:14:15.288122Z] 20:14:15 INFO - gmake[5]: Leaving directory '/home/worker/workspace/build/src/obj-firefox/media/libcubeb/src' [task 2017-03-24T20:14:15.288484Z] 20:14:15 INFO - /home/worker/workspace/build/src/config/recurse.mk:71: recipe for target 'media/libcubeb/src/target' failed [task 2017-03-24T20:14:15.288821Z] 20:14:15 INFO - gmake[4]: *** [media/libcubeb/src/target] Error 2

Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/474208b76717 Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/cc384918477b Part 2: Pass IntIntervals by const reference in TestIntervalSet, r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/828eba714030 Part 3: Pass FilterPrimitiveDescription by const reference in AddLightingAttributes, r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8fc58d0cef Part 4: Produce better annotation reason diagnostics for implicit annotations, r=ehsan

This looks to me like the implementation of `std::function` on OS X uses an alignof(T) member. If we land this patch with no changes, this means that passing a std::function by value as an argument will be an error (on OS X at least). Do we want to prevent code like that? I'm not sure if it is problematic.

Perhaps you could restrict the warning to only apply to function declarations outside of system headers? If the system header does it, that's sort of a claim that it should be okay. (At least, unless the declaration is a template declaration and the std::function is the actual type of a dependent type in the signature, mumble mumble terminology mumble.)

Ugh. I dug into this a little further, and the problem that this static analysis is attempting to solve does not apply to all architectures. For the case of std::function on x86-64 Mac/Linux, everything is OK: (this implementation of) std::function has a non-trivial destructor, so values of it are passed by invisible reference. Thus, any alignment problems are taken care of by the caller. AFAICT, this would also be true on x86-64 Windows. x86 (Unix and Windows), however, would just copy the parameters onto the stack, so you have no guaranteed alignment. Surprisingly, I think ARM does this too (though they make a half-hearted attempt at aligning things). ARM64 appears to do this as well, though that particular part of the ABI is full of acronyms and I didn't want to wade through them all. This language feature seems to be a huge huge footgun, especially for what purport to be cross-platform C++ libraries. We could consider whitelisting std::function for Mac only. I think we might want to consider fixing this in libcubeb, though; I hear we have a few people who work on libcubeb. :)

(In reply to Nathan Froyd [:froydnj] from comment #20) > We could consider whitelisting std::function for Mac only. I think we might > want to consider fixing this in libcubeb, though; I hear we have a few > people who work on libcubeb. :) It seems weird to me that we would prevent std::function from being passed to a function by value. I would be inclined to whitelist that type.

MozReview-Commit-ID: 9ljJA8k9edG

(In reply to Nathan Froyd [:froydnj] from comment #20) > We could consider whitelisting std::function for Mac only. I think we might > want to consider fixing this in libcubeb, though; I hear we have a few > people who work on libcubeb. :) IIRC Matthew would be the right person.

Comment on attachment 8852496 [details] [diff] [review] Part 5: Whitelist std::function to be passed as an argument, even if it has an alignas(_) member Review of attachment 8852496 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp @@ +40,5 @@ > } > + > + bool isWhitelisted(const TagDecl *D) const override { > + // std::function may have an alignas(_) member which we don't want to fail > + // the build. Let's just do this on OSX? Also, do I understand this correctly that we can take this hack out when libcubeb is fixed? If yes, can you please file a follow-up for that? Thanks! ::: build/clang-plugin/tests/TestNonParameterChecker.cpp @@ +188,5 @@ > struct AlignasMember { alignas(8) char a; }; // expected-note {{'AlignasMember' is a non-param type because member 'a' has an alignas(_) annotation}} > void takesAlignasMember(AlignasMember x) { } // expected-error {{Type 'AlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}} > void takesAlignasMemberByRef(const AlignasMember& x) { } > + > +void takesStdFunction(std::function<void()> x) { } I hope an #ifdef XP_MACOSX works here for testing purposes, I'm honestly not sure whether the macro definitions get clobbered somewhere in the build system or not (they might...) If that doesn't work out, don't worry about the other comment, consider it as a nit, to be used at your discretion!)

(In reply to :Ehsan Akhgari (busy) from comment #24) > Comment on attachment 8852496 [details] [diff] [review] > Part 5: Whitelist std::function to be passed as an argument, even if it has > an alignas(_) member > > Review of attachment 8852496 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp > @@ +40,5 @@ > > } > > + > > + bool isWhitelisted(const TagDecl *D) const override { > > + // std::function may have an alignas(_) member which we don't want to fail > > + // the build. > > Let's just do this on OSX? Also, do I understand this correctly that we can > take this hack out when libcubeb is fixed? If yes, can you please file a > follow-up for that? Thanks! On non-OSX platforms, std::function does not have an `alignas(_)` member, so ignoring it has no effect. In addition, as Nathan said, on OS X, passing std::function by value to a function is actually safe, as the calling convention doesn't put the value on the stack. It seems like it should be safe to pass around std::function objects by value, so I am inclined to leave this type whitelisted on OS X. How do you feel about that?

Note that Android uses libc++, so std::function will use alignas there, and we don't run static analysis on Android for the moment. I don't know whether std::function values would cause problems on Android--I think they would, but I'd have to check. So restricting it would probably be better.

(In reply to :Ehsan Akhgari (busy) from comment #24) > I hope an #ifdef XP_MACOSX works here for testing purposes, I'm honestly not > sure whether the macro definitions get clobbered somewhere in the build > system or not (they might...) If that doesn't work out, don't worry about > the other comment, consider it as a nit, to be used at your discretion!) XP_MACOSX is thrown out in the parameters in the clang plugin. I could use __APPLE__ potentially to detect macOS for testing? Otherwise, I could leave it on on all platforms, and then file a bug to remove it when libcubeb is fixed (which seems like we'll want to do from Nathan's comment).

(In reply to Nathan Froyd [:froydnj] from comment #20) > We could consider whitelisting std::function for Mac only. I think we might > want to consider fixing this in libcubeb, though; I hear we have a few > people who work on libcubeb. :) We'll remove it in https://github.com/kinetiknz/cubeb/pull/271, there's no reason we need it.

(In reply to Matthew Gregan [:kinetik] from comment #28) > We'll remove it in https://github.com/kinetiknz/cubeb/pull/271, there's no > reason we need it. It looks like this has merged into the GitHub repo now. Will it take long before it is re-vendored into the tree?

(In reply to Michael Layzell [:mystor] from comment #29) > It looks like this has merged into the GitHub repo now. Will it take long > before it is re-vendored into the tree? It'll land via bug 1352929 in the next day or so, hopefully.

Bug 1352929 has landed now, but we still can't land this patch - it turns out there are _lots_ of places which pass std::functions around by value as parameters, including within libc++! Wheee! Not sure what to do about this - perhaps just keep the whitelist around? If android actually has this problem where alignment isn't guaranteed for alignas(_) values, and we use libc++ there, I'm not sure what the best approach is. This is a failure which appeared on try when I tried to run the SA there after rebasing on central today: https://treeherder.mozilla.org/logviewer.html#?job_id=88652816&repo=try&lineNumber=6001

Still seems to me that comment 19 is the right approach for fixing this, and in a way that deals with every oddball compiler/STL/architecture that might or might not guarantee requested alignment of alignas-infected parameters.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #32) > Still seems to me that comment 19 is the right approach for fixing this, and > in a way that deals with every oddball compiler/STL/architecture that might > or might not guarantee requested alignment of alignas-infected parameters. Yeah, can you please try that Michael?

In order to exclude std::function being passed by value in third party code which we can't change, I tought the clang plugin about ThirdPartyPaths.txt. I did this by adding a generated file to the clang plugin and clang-tidy module, ThirdPartyPaths.cpp, which provides this list of paths to implement a function in Utils.h. MozReview-Commit-ID: PS9x7Nh1h8

MozReview-Commit-ID: 2VDJRxxkVjV

MozReview-Commit-ID: BGg1yS2Ovhz

MozReview-Commit-ID: J3m2uzv7jW0

This allows for the alignas(_) case to be distinguished from the MOZ_NON_PARAM case through notes. MozReview-Commit-ID: 4KIbzEKnmNU

This is the set of changes required to pass std::function objects by const reference instead of by value. I'm not sure if I should split up these changes more, as they are fairly rubber-stampable. MozReview-Commit-ID: PVAqU2DPs2

Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1c55598c82 Part 1: Teach the clang plugin about ThirdPartyPaths.txt, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/36ec718cab5a Part 2: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/05353a4fa276 Part 3: Pass IntIntervals by const reference in TestIntervalSet, r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/3046b1dbf8ed Part 4: Pass FilterPrimitiveDescription by const reference in AddLightingAttributes, r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcbf84527cc Part 5: Produce better annotation reason diagnostics for implicit annotations, r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/f1caa40c8c81 Part 6: Pass std::function values tree by const reference instead of by value, r=ehsan

https://hg.mozilla.org/mozilla-central/rev/bf1c55598c82 https://hg.mozilla.org/mozilla-central/rev/36ec718cab5a https://hg.mozilla.org/mozilla-central/rev/05353a4fa276 https://hg.mozilla.org/mozilla-central/rev/3046b1dbf8ed https://hg.mozilla.org/mozilla-central/rev/0fcbf84527cc https://hg.mozilla.org/mozilla-central/rev/f1caa40c8c81

as a note, this increased the build time for linux64 static analysis: == Change summary for alert #6238 (as of April 27 2017 17:37 UTC) == Regressions: 7% build times summary linux64 opt static-analysis taskcluster-c4.4xlarge 830.71 -> 888.79 5% build times summary linux64 debug static-analysis taskcluster-c4.4xlarge 957.51 -> 1004.02 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6238

I wonder if inThirdPartyPath() is insanely expensive?

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #43) > I wonder if inThirdPartyPath() is insanely expensive? Perhaps I should look into, rather than using llvm's path iterator, normalizing the path strings and performing a substring check? Doing a series of substring checks should hopefully be quite a bit faster. If the actual work of the analysis is faster than the inThirdPartyPath check, we could move it to not check until the actual diagnostic is emitted? For example, I could make all calls to diag() check inThirdPartyPath, remove this check, and see if the performance improves?

(In reply to Michael Layzell [:mystor] from comment #44) > (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from > comment #43) > > I wonder if inThirdPartyPath() is insanely expensive? > > Perhaps I should look into, rather than using llvm's path iterator, > normalizing the path strings and performing a substring check? Doing a > series of substring checks should hopefully be quite a bit faster. Yeah that may be a good idea but I was just guessing, I haven't measured anything myself. > If the actual work of the analysis is faster than the inThirdPartyPath > check, we could move it to not check until the actual diagnostic is emitted? > For example, I could make all calls to diag() check inThirdPartyPath, remove > this check, and see if the performance improves? That seems like a good idea.