1325694 - Need to special case std::atomic base types on MSVC

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

This is breaking static analysis builds on Windows:

<https://treeherder.mozilla.org/logviewer.html#?job_id=41012499&repo=mozilla-inbound#L6012>

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/layout/base/gtest/Unified_cpp_layout_base_gtest0.cpp:2:

 20:50:44     INFO -  In file included from z:/build/build/src/layout/base/gtest/TestAccessibleCaretEventHub.cpp:13:

 20:50:44     INFO -  In file included from z:/build/build/src/layout/base\AccessibleCaretEventHub.h:14:

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include\nsIFrame.h:30:

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/mozilla/WritingModes.h:10:

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nsStyleContext.h:13:

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/mozilla/StyleContextSource.h:10:

 20:50:44     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nsRuleNode.h:21:

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/nsStyleStruct.h(3848,30):  error: class 'nsStyleVariables' cannot have non-memmovable member 'mVariables' of type 'mozilla::CSSVariableValues'

 20:50:44     INFO -    mozilla::CSSVariableValues mVariables;

 20:50:44     INFO -                               ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/mozilla/CSSVariableValues.h(137,44):  note: 'mozilla::CSSVariableValues' is a non-memmove()able type because member 'mVariableIDs' is a non-memmove()able type 'nsDataHashtable<nsStringHashKey, size_t>' (aka 'nsDataHashtable<nsStringHashKey, unsigned int>')

 20:50:44     INFO -    nsDataHashtable<nsStringHashKey, size_t> mVariableIDs;

 20:50:44     INFO -                                             ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/nsDataHashtable.h(23,7):  note: 'nsDataHashtable<nsStringHashKey, size_t>' (aka 'nsDataHashtable<nsStringHashKey, unsigned int>') is a non-memmove()able type because it inherits from a non-memmove()able type 'nsBaseHashtable<nsStringHashKey, unsigned int, unsigned int>'

 20:50:44     INFO -  class nsDataHashtable

 20:50:44     INFO -        ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/nsBaseHashtable.h(52,7):  note: 'nsBaseHashtable<nsStringHashKey, unsigned int, unsigned int>' is a non-memmove()able type because it inherits from a non-memmove()able type 'nsTHashtable<nsBaseHashtableET<nsStringHashKey, unsigned int> >'

 20:50:44     INFO -  class nsBaseHashtable

 20:50:44     INFO -        ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/nsTHashtable.h(306,16):  note: 'nsTHashtable<nsBaseHashtableET<nsStringHashKey, unsigned int> >' is a non-memmove()able type because member 'mTable' is a non-memmove()able type 'PLDHashTable'

 20:50:44     INFO -    PLDHashTable mTable;

 20:50:44     INFO -                 ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/PLDHashTable.h(248,19):  note: 'PLDHashTable' is a non-memmove()able type because member 'mChecker' is a non-memmove()able type 'Checker'

 20:50:44     INFO -    mutable Checker mChecker;

 20:50:44     INFO -                    ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include/PLDHashTable.h(189,37):  note: 'Checker' is a non-memmove()able type because member 'mState' is a non-memmove()able type 'mozilla::Atomic<uint32_t>' (aka 'Atomic<unsigned int>')

 20:50:44     INFO -    mutable mozilla::Atomic<uint32_t> mState;

 20:50:44     INFO -                                      ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include\mozilla/Atomics.h(650,7):  note: 'mozilla::Atomic<uint32_t>' (aka 'Atomic<unsigned int>') is a non-memmove()able type because it inherits from a non-memmove()able type 'detail::AtomicBaseIncDec<unsigned int, (MemoryOrdering)2>'

 20:50:44     INFO -  class Atomic<T, Order, typename EnableIf<IsIntegral<T>::value &&

 20:50:44     INFO -        ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include\mozilla/Atomics.h(596,7):  note: 'detail::AtomicBaseIncDec<unsigned int, (MemoryOrdering)2>' is a non-memmove()able type because it inherits from a non-memmove()able type 'AtomicBase<unsigned int, (mozilla::MemoryOrdering)2>'

 20:50:44     INFO -  class AtomicBaseIncDec : public AtomicBase<T, Order>

 20:50:44     INFO -        ^

 20:50:44     INFO -  z:/build/build/src/obj-firefox/dist/include\mozilla/Atomics.h(546,13):  note: 'AtomicBase<unsigned int, (mozilla::MemoryOrdering)2>' is a non-memmove()able type because member 'mValue' is a non-memmove()able type 'ValueType' (aka 'atomic<unsigned int>')

 20:50:44     INFO -    ValueType mValue;

 20:50:44     INFO -              ^

 20:50:44     INFO -  z:\build\build\src\vs2015u3\VC\include\xxatomic(179,9):  note: 'ValueType' (aka 'atomic<unsigned int>') is a non-memmove()able type because it inherits from a non-memmove()able type '_Atomic_uint' (aka 'std::_Atomic_uint')

 20:50:44     INFO -          struct atomic<_ITYPE>

 20:50:44     INFO -                 ^

 20:50:44     INFO -  3 warnings and 1 error generated.

Comment 1

[(no longer active)]

Attachment: Mark the MSVC specific std::atomic base classes as memmovable for static analysis

Comment 2

[(no longer active)]

Attachment: Mark the MSVC specific std::atomic base classes as memmovable for static analysis

With a test this time.

Comment 3

[(no longer active)]

Attachment: Mark the MSVC specific std::atomic base classes as memmovable for static analysis

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8821629 [details] [diff] [review]
Mark the MSVC specific std::atomic base classes as memmovable for static analysis

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

OK, so looking at MSVC's <atomic>, we have two cases:

1. atomic<T> normally inherits from _Atomic_base<T>...which we don't catch here.
2. atomic<T> is specialized for mostly <cstdint> types and void* for separate _Atomic<> classes.

So...AFAICT, we need the below changes.  WDYT?

::: build/clang-plugin/clang-plugin.cpp
@@ +536,5 @@
> +          Name == "atomic" ||
> +          // libstdc++ specific names
> +          Name == "__atomic_base" ||
> +          // MSVCRT specific names
> +          Name == "_Atomic_bool" ||

...I think this misses _Atomic_base.

::: build/clang-plugin/tests/TestNonMemMovableStd.cpp
@@ +20,5 @@
>  static Mover<HasString> bad_mem; // expected-note {{instantiation of 'Mover<HasString>' requested here}}
>  static Mover<std::arbitrary_name> assumed_bad; // expected-note {{instantiation of 'Mover<std::arbitrary_name>' requested here}}
>  static Mover<std::pair<bool, int>> good;
> +static Mover<std::atomic<bool>> also_good;
> +static Mover<std::atomic<unsigned>> more_goodies;

...I would feel much better about these tests if they covered all the types that we're liable to see in the changed |if| statement, i.e. all the <cstdint> types, void*, and T* for some T.  Don't forget to cover all the |char| types!

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8821634 [details] [diff] [review]
Mark the MSVC specific std::atomic base classes as memmovable for static analysis

Patch mid-aired with an r- from comment 4. :(

Comment 6

[(no longer active)]

They both sound like good ideas.

Comment 7

[(no longer active)]

Attachment: Mark the MSVC specific std::atomic base classes as memmovable for static analysis

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8821652 [details] [diff] [review]
Mark the MSVC specific std::atomic base classes as memmovable for static analysis

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

r=me assuming this works!  Thanks for the test changes.

Comment 9

[(no longer active)]

I'll test this on try first.  I don't have a clang-cl setup handy right now...

Comment 10

[(no longer active)]

The idea of testing more cases was right on, I found a couple of missing types that we also need to handle on try!

Comment 11

[Nathan Froyd [:froydnj]]

\o/ for tests!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8375c8f7d0
Mark the MSVC specific std::atomic base classes as memmovable for static analysis; r=froydnj

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e8375c8f7d0