1185044 - The MOZ_MUST_USE annotation should propagate through subclasses and members

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

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

When a function returns a value which "must be used", it may make sense for any class which contains a value of this type (either by subclassing or member) to also be marked as "must be used".

If this is done, there should be a way to override this behavior if, for example, the wrapper class defines a destructor which "uses" the value. (perhaps MOZ_ALLOW_UNUSED?)

Comment 1

[Daniel Holbert [:dholbert]]

Some other situations beyond subclasses and members where this would make sense:
 - Maybe<T> (which doesn't exactly have a member of type T -- it has AlignedStorage2<T>)
 - Nullable<T> (which has a Maybe<T>)
 - Variant<..., T, ...> (which has AlignedStorage)

If T is declared as MOZ_MUST_USE, then each of these types should be considered MOZ_MUST_USE, too.

Comment 2

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

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Some other situations beyond subclasses and members where this would make
> sense:
>  - Maybe<T> (which doesn't exactly have a member of type T -- it has
> AlignedStorage2<T>)
>  - Nullable<T> (which has a Maybe<T>)
>  - Variant<..., T, ...> (which has AlignedStorage)
> 
> If T is declared as MOZ_MUST_USE, then each of these types should be
> considered MOZ_MUST_USE, too.

In the case of aligned_storage, the analysis wouldn't propagate. I suppose that we could also have some kind of MOZ_FAKE_TEMPLATE_MEMBER (or a better name) annotation which would connect to our static analysis and pretend as though the class has an inline member with the type of each of its template parameters for static analysis purposes.

This would already be important for analyses like MOZ_STACK_CLASS, MOZ_NON_MEMMOVABLE_CLASS etc. Ehsan, do you think that this would be worth implementing as well? If it would, I think it belongs in its own bug. It will be much easier to implement once we refactor the clang-plugin code, such that there is a common utility for propagating these annotations.

Comment 3

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #2)
> (In reply to Daniel Holbert [:dholbert] from comment #1)
> > Some other situations beyond subclasses and members where this would make
> > sense:
> >  - Maybe<T> (which doesn't exactly have a member of type T -- it has
> > AlignedStorage2<T>)
> >  - Nullable<T> (which has a Maybe<T>)
> >  - Variant<..., T, ...> (which has AlignedStorage)
> > 
> > If T is declared as MOZ_MUST_USE, then each of these types should be
> > considered MOZ_MUST_USE, too.

Hmm, are there practical stuff in the tree that would have been caught by this?  Basically, where does this bug come from? :-)

> In the case of aligned_storage, the analysis wouldn't propagate. I suppose
> that we could also have some kind of MOZ_FAKE_TEMPLATE_MEMBER (or a better
> name) annotation which would connect to our static analysis and pretend as
> though the class has an inline member with the type of each of its template
> parameters for static analysis purposes.

Yeah, this seems like a good idea.  But let's not tie this to a member.  MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG seems much better.

> This would already be important for analyses like MOZ_STACK_CLASS,
> MOZ_NON_MEMMOVABLE_CLASS etc. Ehsan, do you think that this would be worth
> implementing as well? If it would, I think it belongs in its own bug. It
> will be much easier to implement once we refactor the clang-plugin code,
> such that there is a common utility for propagating these annotations.

I think the correct way to fix this is actually to modify MozChecker::hasCustomAnnotation and for each type that has MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG, if we don't find the spelling on annotation on the declaration, we should try to find it on the template argument.

The reason is that for any annotation that applies to a CXXRecordDecl, this will make sense, since AlignedStorage is basically used when you want to represent type T, but want to create it lazily, so the semantics of that type should be the same as T.

There are probably other types that we need to mark as MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG, though for now I can only think of the consumers of AlignedStorage.  Thinking about this a bit more though, maybe it's better to put the annotations on the consumers...

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #3)
> > > If T is declared as MOZ_MUST_USE, then each of these types should be
> > > considered MOZ_MUST_USE, too.
> 
> Hmm, are there practical stuff in the tree that would have been caught by
> this?  Basically, where does this bug come from? :-)

In Bug 1176124, Seth's got a patch that modifies a function which previously returned already_AddRefed<CachedSurface> (and hence benefits from MOZ_MUST_USE) to now return Pair<already_AddRefed<CachedSurface>, MatchType>. That change makes us lose the MOZ_MUST_USE benefit.

(This Pair<already_AddRefed<>, ...> thing isn't a pattern we should encourage, but I was fine r+'ing it, given that this is a function that's only called once and not exposed outside of an imagelib-internal .cpp file.)

Link to the relevant chunk of that patch:
https://bugzilla.mozilla.org/attachment.cgi?id=8635638&action=diff#a/image/SurfaceCache.cpp_sec5

As shown there, we ended up using MOZ_WARN_UNUSED_RESULT to reduce the impact of losing MOZ_MUST_USE. (but that's kind of unfortunate)

Comment 5

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

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #3) 
> I think the correct way to fix this is actually to modify
> MozChecker::hasCustomAnnotation and for each type that has
> MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARG, if we don't find the spelling on
> annotation on the declaration, we should try to find it on the template
> argument.

If we do that, then MozChecker::hasCustomAnnotation will have to handle traversing members and base classes automatically, which I think would be a bad idea (otherwise, you could include an implicitly annotated class as a template argument).

If we want that functionality, I think we should introduce MozChecker::hasCustomAnnotationRecursive() (A better name would work too) and MozChecker::noteAnnotationReason() which would perform the reason lookup, and provide the functionality for determining why a type has an annotation and providing that information. This could just also support MOZ_INHERIT_ANNOTATIONS_FROM_TEMPLATE_ARGS (or whatever we decide to call it).

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Layzell [:mystor] from comment #2)
> In the case of aligned_storage, the analysis wouldn't propagate. I suppose
> that we could also have some kind of MOZ_FAKE_TEMPLATE_MEMBER (or a better
> name) annotation which would connect to our static analysis and pretend as
> though the class has an inline member with the type of each of its template
> parameters for static analysis purposes.

Are there any cases in the tree other than AlignedStorage/AlignedStorage2 that would need this? I don't think there's any good reason not to just make AlignedStorage*<T> have a T member in the union, and in fact I think that I have a patch laying around somewhere that made that change.

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (This Pair<already_AddRefed<>, ...> thing isn't a pattern we should
> encourage, but I was fine r+'ing it, given that this is a function that's
> only called once and not exposed outside of an imagelib-internal .cpp file.)

It's a perfectly reasonable pattern IMO. It's something our static analyses need to support.

Comment 7

[(no longer active)]

(In reply to Seth Fowler [:seth] from comment #6)
> (In reply to Michael Layzell [:mystor] from comment #2)
> > In the case of aligned_storage, the analysis wouldn't propagate. I suppose
> > that we could also have some kind of MOZ_FAKE_TEMPLATE_MEMBER (or a better
> > name) annotation which would connect to our static analysis and pretend as
> > though the class has an inline member with the type of each of its template
> > parameters for static analysis purposes.
> 
> Are there any cases in the tree other than AlignedStorage/AlignedStorage2
> that would need this? I don't think there's any good reason not to just make
> AlignedStorage*<T> have a T member in the union, and in fact I think that I
> have a patch laying around somewhere that made that change.

Would that compile?  If yes, let's do that!

> (In reply to Daniel Holbert [:dholbert] from comment #4)
> > (This Pair<already_AddRefed<>, ...> thing isn't a pattern we should
> > encourage, but I was fine r+'ing it, given that this is a function that's
> > only called once and not exposed outside of an imagelib-internal .cpp file.)
> 
> It's a perfectly reasonable pattern IMO. It's something our static analyses
> need to support.

Agreed.

Comment 8

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

Attachment: Unify type annotation logic between MOZ_*_CLASS and MOZ_MUST_USE

This patch introduces a unified mechanism for detecting type annotation logic which should be inherited from inline members and base classes. It also uses this logic for MOZ_*_CLASS and MOZ_MUST_USE.

Comment 9

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

Full code for this and other static analysis can be found on https://github.com/mystor/mozilla-central/tree/sa-bomb (first new commit: 93dcf7862ded6929f088daf8599523f1af81bf49)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e4c664cf94

Comment 10

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

(In reply to Seth Fowler [:seth] from comment #6)
> Are there any cases in the tree other than AlignedStorage/AlignedStorage2
> that would need this? I don't think there's any good reason not to just make
> AlignedStorage*<T> have a T member in the union, and in fact I think that I
> have a patch laying around somewhere that made that change.

We can't just have a T in the union, because T may not be plain old data, and C++ doesn't allow you to put objects with non-trivial destructors or constructors into C-style unions. Did you have a different mechanism for making this work that I don't know about?

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Layzell [:mystor] from comment #10)
> We can't just have a T in the union, because T may not be plain old data,
> and C++ doesn't allow you to put objects with non-trivial destructors or
> constructors into C-style unions. Did you have a different mechanism for
> making this work that I don't know about?

Hasn't that restriction been relaxed in C++-11?

It's a hassle to link to the standard directly, but see here:

http://en.cppreference.com/w/cpp/language/union

Comment 12

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

Attachment: DON'T LAND - Include a T member in AlignedStorage2's union

(In reply to Seth Fowler [:seth] from comment #11)
> Hasn't that restriction been relaxed in C++-11?
>
> It's a hassle to link to the standard directly, but see here:
>
> http://en.cppreference.com/w/cpp/language/union

According to https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code, we can't just unconditionally include one of those class-like unions in our codebase, as it isn't supported by visual C++.

Getting code to work with the approach of including non-trivial types in unions was mildly annoying, as it means that AlignedStorage2 can't have a trivial destructor, constructor, copy constructor, or assignment operator anymore (as we have to explicitly implement a destructor, etc. in the union). I did the work of making these changes (without actually #ifdef-ing them out on platforms which don't support class-like unions, so we definitely can't land this patch), and there are some messy things which we need to change because AlignedStorage2 doesn't have trivial constructors/destructors.

We also have the problem that AlignedStorage2 could previously store abstract classes (which is done in https://dxr.mozilla.org/mozilla-central/source/js/public/UbiNode.h?from=UbiNode.h#293), but that isn't possible anymore with the type participating in the union. I changed UbiNode in this patch to instead use AlignedStorage, rather than AlignedStorage2, and explicitly casted at the use site.

I'm not sure if we want these changes in our codebase, as they don't necessarially improve readability etc, and will have to be ifdefed out on windows for now anyways. Any thoughts?

Comment 13

[Seth Fowler [:seth] [:s2h]]

(In reply to Michael Layzell [:mystor] from comment #12)
> According to
> https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code, we can't
> just unconditionally include one of those class-like unions in our codebase,
> as it isn't supported by visual C++.

If I recall correctly, this is what stopped me from working on my own patch along these lines.

However, since the motivation is different here - we want to let static analysis "see through" AlignedStorage2 - it'd probably be OK to temporarily use a different AlignedStorage2 implementation on Windows, where we after all, to my knowledge, don't run static analysis right now. It seems that VS2015 implements the C++11 union behavior, so we're not that far away from being able to use them.

(In reply to Michael Layzell [:mystor] from comment #12)
> We also have the problem that AlignedStorage2 could previously store
> abstract classes (which is done in
> https://dxr.mozilla.org/mozilla-central/source/js/public/UbiNode.
> h?from=UbiNode.h#293), but that isn't possible anymore with the type
> participating in the union. I changed UbiNode in this patch to instead use
> AlignedStorage, rather than AlignedStorage2, and explicitly casted at the
> use site.

I find this to be an awful hack, and not something that we should penalize the rest of the code base to support.

> I'm not sure if we want these changes in our codebase, as they don't
> necessarially improve readability etc, and will have to be ifdefed out on
> windows for now anyways. Any thoughts?

I agree that the readability consequences here are not the greatest for the case where we have unions that *contain* AlignedStorage2 values.

One option that in theory might help would be to specialize AlignedStorage2<T> so that when T is trivially constructable/destructable, so is AlignedStorage2<T>. I took a look at a specific case here, AlignedStorage2<AnyRegister>, and that wouldn't help since AnyRegister is not trivially constructable itself. Alas. =(

I'll think more about this. One option is just to keep the existing AlignedStorage2 type around for the cases where its behavior is preferable (which is very few cases, I think), and switch as many uses as possible over to a new type. Taking that path would imply accepting that the remaining users of AlignedStorage2 would still be vulnerable to misuse of MOZ_MUST_USE type parameters, though, which isn't ideal.

Comment 14

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

(In reply to Seth Fowler [:seth] from comment #13)
> If I recall correctly, this is what stopped me from working on my own patch
> along these lines.
> 
> However, since the motivation is different here - we want to let static
> analysis "see through" AlignedStorage2 - it'd probably be OK to temporarily
> use a different AlignedStorage2 implementation on Windows, where we after
> all, to my knowledge, don't run static analysis right now. It seems that
> VS2015 implements the C++11 union behavior, so we're not that far away from
> being able to use them.

Yes, we don't run static analysis on windows, and if we did in the future it would be through clang-cl, which I believe would support these class-like unions.

> I find this to be an awful hack, and not something that we should penalize
> the rest of the code base to support.

I'm fine with that

> I agree that the readability consequences here are not the greatest for the
> case where we have unions that *contain* AlignedStorage2 values.

In these cases, the reason why AlignedStorage2 is being used is to get around the fact that But there's also the fact that the main reason why people used AlignedStorage in unions was to get around the fact that the original type wasn't trivially constructable/destructible, and AlignedStorage was a way to get around that (AFAICT).

For example, in those cases where we have unions of AlignedStorage, we could probably now use a union of the original type, as AlignedStorage was mostly used as a convenient wrapper to get around the fact that you couldn't put non-trivially destructable/constructable types into unions. For example:

    union U {
        struct {
            Value v;
        } constant;
        struct {
            mozilla::AlignedStorage2<ValueOperand> reg;
        } reg;
        struct {
            uint32_t slot;
        } local;
        struct {
            uint32_t slot;
        } arg;

        U() {}
        ~U() {}
        U(const U& u) {
            memcpy(this, &u, sizeof(U));
        }
        U& operator=(const U& u) {
            memcpy(this, &u, sizeof(U));
            return *this;
        }
    } data;

could be written just as 

    union U {
        struct {
            Value v;
        } constant;
        struct {
            ValueOperand reg;
        } reg;
        struct {
            uint32_t slot;
        } local;
        struct {
            uint32_t slot;
        } arg;

        U() {}
        ~U() {}
        U(const U& u) {
            memcpy(this, &u, sizeof(U));
        }
        U& operator=(const U& u) {
            memcpy(this, &u, sizeof(U));
            return *this;
        }
    } data;

with some small modifications to the original code. (although that wouldn't compile on MSVC pre-2015).

> One option that in theory might help would be to specialize
> AlignedStorage2<T> so that when T is trivially constructable/destructable,
> so is AlignedStorage2<T>. I took a look at a specific case here,
> AlignedStorage2<AnyRegister>, and that wouldn't help since AnyRegister is
> not trivially constructable itself. Alas. =(
> 
> I'll think more about this. One option is just to keep the existing
> AlignedStorage2 type around for the cases where its behavior is preferable
> (which is very few cases, I think), and switch as many uses as possible over
> to a new type. Taking that path would imply accepting that the remaining
> users of AlignedStorage2 would still be vulnerable to misuse of MOZ_MUST_USE
> type parameters, though, which isn't ideal.

The patch which I wrote compiles on my computer, so I think you can see all of the places where the "old behavior is preferable". I think that in those limited situations it isn't worth keeping around another type just to avoid the syntactic overhead of the explicit constructors/destructors (especially considering how few of these types there are). Also, class-like unions are probably preferable for the current uses of AlignedStorage2 where we need non-trivially constructable/destructable types in unions, but that would break MSVC even more.

Comment 15

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

Comment on attachment 8638019 [details] [diff] [review]
DON'T LAND - Include a T member in AlignedStorage2's union

Our discussion about AlignedStorage2 isn't really related to this bug. Let's move that discussion over to bug 1187073.

Comment 16

[(no longer active)]

Comment on attachment 8637454 [details] [diff] [review]
Unify type annotation logic between MOZ_*_CLASS and MOZ_MUST_USE

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

Thanks, this is really nice!

::: build/clang-plugin/clang-plugin.cpp
@@ +227,5 @@
> +    QualType Type;
> +    ReasonKind Kind;
> +    const FieldDecl *Field;
> +
> +    bool valid() { return Kind != RK_None; }

Nit: please declare all methods that can be const as const.

@@ +256,5 @@
> +static CustomTypeAnnotation MOZ_GLOBAL_CLASS =
> +  CustomTypeAnnotation("moz_global_class", "global");
> +static CustomTypeAnnotation MOZ_NONHEAP_CLASS =
> +  CustomTypeAnnotation("moz_nonheap_class", "non-heap");
> +static CustomTypeAnnotation MOZ_MUST_USE =

These names look like macro names.  Please call them StackClass, MustUse, etc.

@@ +646,5 @@
>  
>  namespace {
>  
> +void
> +CustomTypeAnnotation::dumpAnnotationReason(DiagnosticsEngine &Diag, QualType T) {

Please put the return type on the same line as the definition, as this code follows the LLVM style.

@@ +660,5 @@
> +  AnnotationReason Reason = directAnnotationReason(T);
> +  for (;;) {
> +    switch (Reason.Kind) {
> +    case RK_ArrayElement:
> +      Diag.Report(ArrayID)

Reporting notes that are not attached to anything is terrible.  In every call site of dumpAnnotationReason, you have a SourceLocation that you can pass down to dumpAnnotationReason.  Please do that and attach the notes to the right place.

@@ +669,5 @@
> +        SourceLocation SL = Decl->getLocation();
> +        Diag.Report(SL, InheritsID)
> +          << Pretty << T << Reason.Type;
> +      } else {
> +        // This should probably never be reached, but we may as well handle it

I don't think you need to handle this.  Maybe just add an assert?

Comment 17

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

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #16)
> Reporting notes that are not attached to anything is terrible.  In every
> call site of dumpAnnotationReason, you have a SourceLocation that you can
> pass down to dumpAnnotationReason.  Please do that and attach the notes to
> the right place.

I originally didn't attach a location because I knew that in the output there would be a location on the previous line due to the error, and I wanted to make the note take up less space if the location information would add no meaningful information to the diagnostic, but I can definitely add that in if you'd prefer.

> I don't think you need to handle this.  Maybe just add an assert?

We never run our tests with asserts enabled, adding an assert feels weird, as it is basically a comment given our current clang-plugin testing.

Comment 18

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #17)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #16)
> > Reporting notes that are not attached to anything is terrible.  In every
> > call site of dumpAnnotationReason, you have a SourceLocation that you can
> > pass down to dumpAnnotationReason.  Please do that and attach the notes to
> > the right place.
> 
> I originally didn't attach a location because I knew that in the output
> there would be a location on the previous line due to the error, and I
> wanted to make the note take up less space if the location information would
> add no meaningful information to the diagnostic, but I can definitely add
> that in if you'd prefer.

That's currently true, but I'm pretty sure that is not an invariant that we can rely on in the future, so I prefer to use the locations that we already have...

> > I don't think you need to handle this.  Maybe just add an assert?
> 
> We never run our tests with asserts enabled, adding an assert feels weird,
> as it is basically a comment given our current clang-plugin testing.

Fair.  How about just assuming that Decl will be non-null and don't check it?  If that proves to not be the case, we'll crash and that will fail the tests.  :-)

Comment 19

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

Attachment: Unify type annotation logic between MOZ_*_CLASS and MOZ_MUST_USE

Updated version of patch.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8991390305e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8991390305e