1339537 - change MOZ_NON_PARAM check to consider alignas members instead

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nathan Froyd [:froydnj]]

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).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 2

[Nathan Froyd [:froydnj]]

(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. =/

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types

MozReview-Commit-ID: 2VDJRxxkVjV

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Pass IntIntervals by const reference in TestIntervalSet

MozReview-Commit-ID: BGg1yS2Ovhz

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Pass FilterPrimitiveDescription by const reference in AddLightingAttributes

MozReview-Commit-ID: J3m2uzv7jW0

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(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 9

[(no longer active)]

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 10

[Jean-Yves Avenard [:jya]]

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 11

[Markus Stange [:mstange]]

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.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Produce better annotation reason diagnostics for implicit annotations

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 13

[Nika Layzell [:nika] (ni? for response)]

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 14

[(no longer active)]

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 15

[(no longer active)]

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.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 19

[Jeff Walden [:Waldo]]

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.)

Comment 20

[Nathan Froyd [:froydnj]]

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. :)

Comment 21

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Whitelist std::function to be passed as an argument, even if it has an alignas(_) member

MozReview-Commit-ID: 9ljJA8k9edG

Comment 23

[(no longer active)]

(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 24

[(no longer active)]

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!)

Comment 25

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 26

[Nathan Froyd [:froydnj]]

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.

Comment 27

[Nika Layzell [:nika] (ni? for response)]

(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).

Comment 28

[Matthew Gregan [:kinetik]]

(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.

Comment 29

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 30

[Matthew Gregan [:kinetik]]

(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.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

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

Comment 32

[Jeff Walden [:Waldo]]

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.

Comment 33

[(no longer active)]

(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?

Comment 34

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Teach the clang plugin about ThirdPartyPaths.txt

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

Comment 35

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update the MOZ_NON_PARAM analysis to implicitly apply to alignas(_) types

MozReview-Commit-ID: 2VDJRxxkVjV

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Pass IntIntervals by const reference in TestIntervalSet

MozReview-Commit-ID: BGg1yS2Ovhz

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Pass FilterPrimitiveDescription by const reference in AddLightingAttributes

MozReview-Commit-ID: J3m2uzv7jW0

Comment 38

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Produce better annotation reason diagnostics for implicit annotations

This allows for the alignas(_) case to be distinguished from the
MOZ_NON_PARAM case through notes.

MozReview-Commit-ID: 4KIbzEKnmNU

Comment 39

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Pass std::function values tree by const reference instead of by value

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
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

Comment 41

[Carsten Book [:Tomcat]]

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

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 43

[(no longer active)]

I wonder if inThirdPartyPath() is insanely expensive?

Comment 44

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 45

[(no longer active)]

(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.