Closed Bug 1201309 Opened 4 years ago Closed 4 years ago

Make MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS work with MOZ_NON_MEMMOVABLE

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

The ananlysis for MOZ_NON_MEMMOVABLE uses different code from the other storage-class-ish attributes, so it doesn't currently respect MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS.  This should be fixed, so that we can detect memmove problems with an intervening use of mozilla::Maybe or other sum-type-like containers that use AlignedStorage2.
One approach is to just loosely copypaste the template argument check into the memmovable checker; this gets us the… not maximally helpful error message seen in bug 1198979 comment #5.  Instead, if I reuse the reason-tracking we already have (and remove ~100 lines of code in the process), the error looks like this:

> ../../dist/include/nsTArray.h:662:34: error: Cannot instantiate 'nsTArray_CopyChooser<mozilla::layers::Edit>' with non-memmovable template argument 'mozilla::layers::Edit'
> struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_CopyChooser
>                                  ^
> ../../dist/include/nsTArray.h:793:42: note: instantiation of 'nsTArray_CopyChooser<mozilla::layers::Edit>' requested here
>   : public nsTArray_base<Alloc, typename nsTArray_CopyChooser<E>::Type>
>                                          ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:10222:11: note: 'mozilla::layers::Edit' is a non-memmove()able type because member 'mValue' is a non-memmove()able type 'mozilla::layers::Edit::Value'
>     Value mValue;
>           ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:9506:56: note: 'mozilla::layers::Edit::Value' is a non-memmove()able type because member 'VOpSetLayerAttributes' is a non-memmove()able type 'mozilla::AlignedStorage2<OpSetLayerAttributes>'
>         mozilla::AlignedStorage2<OpSetLayerAttributes> VOpSetLayerAttributes;
>                                                        ^
> ../../dist/include/mozilla/Alignment.h:126:56: note: 'mozilla::AlignedStorage2<OpSetLayerAttributes>' is a non-memmove()able type because it has a template argument non-memmove()able type 'mozilla::layers::OpSetLayerAttributes'
> struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
>                                                        ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:5490:21: note: 'mozilla::layers::OpSetLayerAttributes' is a non-memmove()able type because member 'attrs_' is a non-memmove()able type 'LayerAttributes' (aka 'mozilla::layers::LayerAttributes')
>     LayerAttributes attrs_;
>                     ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:5276:27: note: 'LayerAttributes' (aka 'mozilla::layers::LayerAttributes') is a non-memmove()able type because member 'common_' is a non-memmove()able type 'CommonLayerAttributes' (aka 'mozilla::layers::CommonLayerAttributes')
>     CommonLayerAttributes common_;
>                           ^
> /home/jld/src/obj.gecko-dev/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:4247:12: note: 'CommonLayerAttributes' (aka 'mozilla::layers::CommonLayerAttributes') is a non-memmove()able type because member 'contentDescription_' is a non-memmove()able type 'string' (aka 'basic_string<char>')
>     string contentDescription_;
>            ^

(This is also using work-in-progress for bug 1201314 and bug 1201329.)
I agree that you should re-use the CustomTypeAnnotation logic for this. The memmovable checker was just missed in the migration to the CustomTypeAnnotation logic :).

If you have any questions about the plugin, just let me know!
Ehsan: sorry about asking for review when your bugzilla userinfo says not to ask for reviews, but I can't find this on the modules wiki page and revision history suggests you're the person to ask; feel free to adjust the attachment flags as appropriate.
Attachment #8656959 - Flags: review?(ehsan)
Attachment #8656959 - Flags: feedback?(michael)
Comment on attachment 8656959 [details] [diff] [review]
bug1201309-template-nonmove-hg0.diff

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

Very nice, thanks for the patch!
Attachment #8656959 - Flags: review?(ehsan) → review+
Comment on attachment 8656959 [details] [diff] [review]
bug1201309-template-nonmove-hg0.diff

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

LGTM too!

Thanks :)

::: build/clang-plugin/clang-plugin.cpp
@@ +747,5 @@
>        Diag.Report(Decl->getLocation(), TemplID) << Pretty << T << Reason.Type;
>        break;
>      }
>      default:
> +      // FIXME: is it possible to point out the original annotation?

Would you file a follow-up bug about this? I originally left this annotation out because I didn't want to clutter up the error output, but I've found myself wanting a reference to where the annotation is located a few times while I've been working on other plugins, so I think we should get this.
Attachment #8656959 - Flags: feedback?(michael) → feedback+
See Also: → 1203263
Trivial comment change to cite the followup bug.  Thanks for catching that; I don't like landing FIXMEs without attached bug numbers, but I forgot about that one.  And carrying over r+; thanks for the review.
Attachment #8658872 - Flags: review+
Attachment #8656959 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/27221624668e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.