1226376 - Mark mozilla::StaticMutex as MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

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

This will prohibit it from being allocated outside of a static context, which the documentation comment seems to suggest is mandatory.

Comment 1

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

Attachment: Annotate StaticMutex with MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS

SA Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03ad2c27851

Comment 2

[Pulsebot]

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

Comment 3

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae540161167aa228eca563fa93e45aac567b2015
Backout changeset aad9086f8efd (bug 1226376) for breaking Opt static-analysis build.

Comment 4

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

So, the problem here is that Atomic<T> doesn't have a non-trivial constructor, instead, it has a constexpr constructor which dispatches to AtomicBase, which also has a constexpr constructor.

I'm not sure about what the exact semantics of constexpr are, and whether that is acceptable. For example, on MSVC right now it seems like StaticMutex has a "non-trivial" constructor, (though it will probably? be optimized away?), and on clang/gcc it's a constexpr constructor, and shouldn't therefore have any runtime cost at startup (I think?).

Comment 5

[Nathan Froyd [:froydnj]]

We should = delete some of the things on StaticMutex (and probably StaticAutoPtr, if we don't already).  I'm not sure what to do about the DEBUG-only StaticMutex constructor; I think now that we have the static analysis that ensures StaticMutex only occurs at toplevel, we could get rid of the assert.

Comment 6

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

(In reply to Nathan Froyd [:froydnj] from comment #5)
> We should = delete some of the things on StaticMutex (and probably
> StaticAutoPtr, if we don't already).  I'm not sure what to do about the
> DEBUG-only StaticMutex constructor; I think now that we have the static
> analysis that ensures StaticMutex only occurs at toplevel, we could get rid
> of the assert.

So, the analysis to ensure that things only appear at the top level is currently (MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS), which also asserts that all constructors are trivial (in release mode), which is not the case for StaticMutex, as Atomic has a constructor. Do you think that I should also expose MOZ_GLOBAL_CLASS directly for types which must be global, but which may have a non-trivial constructor?

I agree that removing the assert is probably OK, but I don't really see any reason to. The thing I'm more worried about is that StaticMutex has a non-trivial destructor in release mode, which AFAICR is a slight problem.

Comment 7

[Nathan Froyd [:froydnj]]

Where do you see the release-mode destructor for StaticMutex?  That would certainly be a bad thing.  I'm a bit surprised that MSVC would have a non-trivial constructor; constexpr isn't supported by 2013, but I think MSVC would optimize the initialization away...unless there's something else I'm missing.

I am not super-thrilled about adding another annotation, as I think the current set of annotations is complicated enough.  It seems like we ought to permit constexpr constructors in the analysis.

Comment 8

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

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Where do you see the release-mode destructor for StaticMutex?  That would
> certainly be a bad thing.  I'm a bit surprised that MSVC would have a
> non-trivial constructor; constexpr isn't supported by 2013, but I think MSVC
> would optimize the initialization away...unless there's something else I'm
> missing.
> 
> I am not super-thrilled about adding another annotation, as I think the
> current set of annotations is complicated enough.  It seems like we ought to
> permit constexpr constructors in the analysis.

That's what I was thinking too, I think that it would be optimized away, so it _probably_ isn't a problem (I don't really want to read the output from the compiler to find out ^.^).

It's only constructor, sorry, I typoed and typed destructor instead of constructor. I can probably whitelist constexpr constructors in the analysis, I'll take a look at how annoying that will be to do.

Comment 9

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

Attachment: Allow non-trivial constexpr constructors in MOZ_TRIVIAL_CTOR_DTOR classes

I believe that this should be OK for the purpose of MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS, as constexpr functions should be run at compile time, and thus there shouldn't be any of the static constructor overhead.

We may want to re-name the internal annotation name to MOZ_CONSTEXPR_CTOR_DTOR if we choose to use this.

Comment 10

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

Attachment: Allow non-trivial constexpr constructors in MOZ_TRIVIAL_CTOR_DTOR classes

Sorry for asking for review again. I was going to push, but noticed some weird edge cases. I just want to make sure that the edge cases are OK before actually making this change, as they may make this entire thing too complicated and annoying (meaning that we should scrap this entire patch).

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e911e48008
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c58ad8358d

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22e911e48008
https://hg.mozilla.org/mozilla-central/rev/09c58ad8358d