Add MOZ_NON_AUTOABLE to restrict using auto in place of specific types

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: Ehsan, Assigned: Nika)

Tracking

unspecified
mozilla43

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
The following pattern is a memory leak since already_AddRef'ed destructor doesn't drop the reference:

already_AddRefed<T> Foo();

auto obj = Foo();

We should add a MOZ_NON_AUTOABLE attribute which would apply to already_AddRefed and similar, so that we can detect these cases and convert them into compile time errors.
(Assignee)

Comment 1

3 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #0)
> The following pattern is a memory leak since already_AddRef'ed destructor
> doesn't drop the reference:
> 
> already_AddRefed<T> Foo();
> 
> auto obj = Foo();
> 
> We should add a MOZ_NON_AUTOABLE attribute which would apply to
> already_AddRefed and similar, so that we can detect these cases and convert
> them into compile time errors.

This should be pretty trivial to implement, I'll get something working tomorrow hopefully.
Assignee: nobody → michael
(Assignee)

Comment 2

3 years ago
Created attachment 8646950 [details] [diff] [review]
Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types
Attachment #8646950 - Flags: review?(ehsan)
(Assignee)

Comment 3

3 years ago
Created attachment 8646951 [details] [diff] [review]
Part 2: Use MOZ_NON_AUTOABLE to validate the usage of already_AddRefed
Attachment #8646951 - Flags: review?(ehsan)
(Assignee)

Comment 4

3 years ago
Created attachment 8646960 [details] [diff] [review]
Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

Oops - I didn't include code for `auto &` and `const auto &` among others :S.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3a3a8fe7eb
Attachment #8646950 - Attachment is obsolete: true
Attachment #8646950 - Flags: review?(ehsan)
Attachment #8646960 - Flags: review?(ehsan)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8646960 [details] [diff] [review]
Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

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

r=me with the below fixed.

::: build/clang-plugin/clang-plugin.cpp
@@ +1367,5 @@
> +  const VarDecl *D = Result.Nodes.getNodeAs<VarDecl>("node");
> +
> +  if (const AutoType *T = D->getType()->getContainedAutoType()) {
> +    if (const CXXRecordDecl *Rec = T->getAsCXXRecordDecl()) {
> +      if (MozChecker::hasCustomAnnotation(Rec, "moz_non_autoable")) {

Couldn't you use autoType() and hasDeducedType() AST matchers instead of doing the checks programatically?  If not, this is fine too.

::: build/clang-plugin/tests/TestNoAutoType.cpp
@@ +1,3 @@
> +#define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable")))
> +
> +struct MOZ_NON_AUTOABLE ExplicitType {};

Please also add a test where this annotation is applied to a template class.

@@ +6,5 @@
> +void f() {
> +  ExplicitType a;
> +  auto b = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}
> +  auto &br = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}
> +  const auto &brc = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}

What about auto* and const auto*?
Attachment #8646960 - Flags: review?(ehsan) → review+
(Reporter)

Updated

3 years ago
Attachment #8646951 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #5)
> Couldn't you use autoType() and hasDeducedType() AST matchers instead of
> doing the checks programatically?  If not, this is fine too.

I could do the match
autoType(hasDeducedType(nonAutoableType())), but that would unfortunately not give me enough information to be able to give good line numbers (as I won't have the VarDecl).

If I decided to include the vardecl, unfortunately varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it doesn't strip away the & and const qualifiers, so I would have to separately match against each of those :-/. 

I think it makes the most sense to just do these checks programmatically. I think that this problem is a bit awkward to shoehorn into the matcher system, even though it is possible. If you think it's worth it, I can move it into the matcher system though.
(Assignee)

Comment 7

3 years ago
(In reply to Michael Layzell [:mystor] from comment #6)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #5)
> > Couldn't you use autoType() and hasDeducedType() AST matchers instead of
> > doing the checks programatically?  If not, this is fine too.
> 
> I could do the match
> autoType(hasDeducedType(nonAutoableType())), but that would unfortunately
> not give me enough information to be able to give good line numbers (as I
> won't have the VarDecl).
> 
> If I decided to include the vardecl, unfortunately
> varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it
> doesn't strip away the & and const qualifiers, so I would have to separately
> match against each of those :-/. 
> 
> I think it makes the most sense to just do these checks programmatically. I
> think that this problem is a bit awkward to shoehorn into the matcher
> system, even though it is possible. If you think it's worth it, I can move
> it into the matcher system though.

Actually, I think I'll just add an `autoNonAutoableType()` matcher which uses getContainedAutoType internally, and performs the check there, that way I can move the logic into the matchers. Is that OK with you Ehsan?
(Assignee)

Comment 8

3 years ago
Created attachment 8648105 [details] [diff] [review]
Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

Updated patch
Attachment #8646960 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4a4eba03ae3e
https://hg.mozilla.org/mozilla-central/rev/0f595614207f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

7 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.