1744604 - Relax the non-param and non-memmovable analysis to reduce false positives

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

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

Currently these two analyses are occasionally over-zealous, and will complain about passing certain types by-value or including certain types within nsTArray which should not be linted against. This bug tracks relaxes some of these requirements to align better with the actual problems which they're intended to fix.

Specifically, this change aims to make the exceptions for non-memmovable types easier to implement, without requiring knowledge of stl internals, as well as supporting types like std::function which contain alignas on some platforms but not on the impacted win32 platform, to not be impacted by the non-param analysis.

Comment 1

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

Attachment: Bug 1744604 - Part 1: Make CustomTypeAnnotation more flexible, r=andi

This allows CustomTypeAnnotation to customize the type traversal behaviour to
allow things like opting library types out of static analysis easier. It will
be used in parts 2 and 3 to improve the non-memmoveable and non-param trait
analysis.

Comment 2

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

Attachment: Bug 1744604 - Part 2: Be less restrictive with the non-memmovable static analysis, r=andi

This change adds an exception for stl types which implement
std::is_trivially_move_constructible and std::is_trivially_destructible, as
these are generally stable parts of the standard library type definition which
may be relied on by downstream code.

Depends on D132995

Comment 3

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

Attachment: Bug 1744604 - Part 3: Relax the alignas lint to be more accurate to the potential alignment issue, r=andi

The platform in question which triggered this lint being added is the windows
x86 ABI, which pre-msvc version 19.14 emitted an error when passing complex
aggregate types as parameters due to not being able to align them.
Unfortunately modern versions of Clang still have a bug in the implementation
of the post-19.14 ABI and will pass types with incorrect alignments.

This change makes the intended behaviour more clear and avoids linting against
alignments <= the pointer alignment on all platforms, as well as alignment for
types which will be re-aligned in the callee by clang.

Finally, it also adds an exception for the std::function stdlib type which is
16-byte aligned on macOS, but has a legal alignment on windows x86, and
therefore should not be linted against as it will behave correctly on all
platforms.

In the future, if we update to a version of clang without this ABI bug, we may
want to remove this lint entirely.

Depends on D132996

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/051767551298
Part 1: Make CustomTypeAnnotation more flexible, r=andi
https://hg.mozilla.org/integration/autoland/rev/3fc47ff6a295
Part 2: Be less restrictive with the non-memmovable static analysis, r=andi
https://hg.mozilla.org/integration/autoland/rev/31d7633b2826
Part 3: Relax the alignas lint to be more accurate to the potential alignment issue, r=andi

Comment 5

[Sandor Molnar[:smolnar]]

Backed out for causing win build bustages in TestNonParameterChecker

Backout link: https://hg.mozilla.org/integration/autoland/rev/69c10b57c1223dacb93ed5d4b75d29314fdc180e

Push with failures

Failure log

INFO -  error: 'error' diagnostics expected but not seen:
[task 2021-12-08T01:47:45.768Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 182: Type 'AlignasStruct' must not be used as parameter
[task 2021-12-08T01:47:45.769Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 186: Type 'AlignasMember' must not be used as parameter
[task 2021-12-08T01:47:45.769Z] 01:47:45     INFO -  error: 'note' diagnostics expected but not seen:
[task 2021-12-08T01:47:45.769Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 181: 'AlignasStruct' is a non-param type because it has an explicit alignment of '16'
[task 2021-12-08T01:47:45.770Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 182: Please consider passing a const reference instead
[task 2021-12-08T01:47:45.770Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 185: 'AlignasMember' is a non-param type because member 'a' has an explicit alignment of '16'
[task 2021-12-08T01:47:45.771Z] 01:47:45     INFO -    File /builds/worker/checkouts/gecko/build/clang-plugin/tests/TestNonParameterChecker.cpp Line 186: Please consider passing a const reference instead
[task 2021-12-08T01:47:45.771Z] 01:47:45     INFO -  6 errors generated.
[task 2021-12-08T01:47:45.771Z] 01:47:45    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:660: TestNonParameterChecker.obj] Error 1

Comment 6

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

Attachment: Bug 1744604 - Part 1: Make CustomTypeAnnotation more flexible, r=andi

This allows CustomTypeAnnotation to customize the type traversal behaviour to
allow things like opting library types out of static analysis easier. It will
be used in parts 2 and 3 to improve the non-memmoveable and non-param trait
analysis.

Comment 7

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

Attachment: Bug 1744604 - Part 2: Be less restrictive with the non-memmovable static analysis, r=andi

This change adds an exception for stl types which implement
std::is_trivially_move_constructible and std::is_trivially_destructible, as
these are generally stable parts of the standard library type definition which
may be relied on by downstream code.

Depends on D133680

Comment 8

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

Attachment: Bug 1744604 - Part 3: Relax the alignas lint to be more accurate to the potential alignment issue, r=andi

The platform in question which triggered this lint being added is the windows
x86 ABI, which pre-msvc version 19.14 emitted an error when passing complex
aggregate types as parameters due to not being able to align them.
Unfortunately modern versions of Clang still have a bug in the implementation
of the post-19.14 ABI and will pass types with incorrect alignments.

This change makes the intended behaviour more clear and avoids linting against
alignments <= the pointer alignment on all platforms, as well as alignment for
types which will be re-aligned in the callee by clang.

Finally, it also adds an exception for the std::function stdlib type which is
16-byte aligned on macOS, but has a legal alignment on windows x86, and
therefore should not be linted against as it will behave correctly on all
platforms.

In the future, if we update to a version of clang without this ABI bug, we may
want to remove this lint entirely.

Depends on D133681

Comment 9

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

Attachment: Bug 1744604 - Part 4: Make the alignas lint behave more consistently across platforms, r=andi

On aarch64 and x86-64 windows, the behaviour of canPassInRegisters is slightly
different than other platforms, accepting types to be passed in registers which
wouldn't be passed in registers on other platforms. This causes the lint to
behave slightly differently on that platform. This patch changes the lint to
always follow the non-win64 behaviour required by the C++ standard.

Depends on D133682

Comment 10

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

Attachment: Bug 1744604 - Part 4: Make the alignas lint behave more consistently across platforms, r=andi

On aarch64 and x86-64 windows, the behaviour of canPassInRegisters is slightly
different than other platforms, accepting types to be passed in registers which
wouldn't be passed in registers on other platforms. This causes the lint to
behave slightly differently on that platform. This patch changes the lint to
always follow the non-win64 behaviour required by the C++ standard.

Depends on D132997

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3c7378172a4
Part 1: Make CustomTypeAnnotation more flexible, r=andi
https://hg.mozilla.org/integration/autoland/rev/1d7f0bd57bca
Part 2: Be less restrictive with the non-memmovable static analysis, r=andi
https://hg.mozilla.org/integration/autoland/rev/78b99726e2b7
Part 3: Relax the alignas lint to be more accurate to the potential alignment issue, r=andi
https://hg.mozilla.org/integration/autoland/rev/767b45bf681a
Part 4: Make the alignas lint behave more consistently across platforms, r=andi

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c3c7378172a4
https://hg.mozilla.org/mozilla-central/rev/1d7f0bd57bca
https://hg.mozilla.org/mozilla-central/rev/78b99726e2b7
https://hg.mozilla.org/mozilla-central/rev/767b45bf681a