Bug 1732343 - Part 1: Better support for moveonly types in IPDL, r=handyman

In part 2 of this patch, a large number of messages are being converted to
contain move-only types, both as direct arguments and within compound data
structures. This revealed some limitations in IPDL's handling of moveonly
types, which this patch hopes to rectify. This also required changes to allow
distinguishing between types which require move to send vs. them not having a
move constructor.

This does not fully fix the underlying issues, but attempts to preserve
existing behaviour while improving support for the new types being added. There
should be further cleanup in the future.

Differential Revision: https://phabricator.services.mozilla.com/D126563
This commit is contained in:
Nika Layzell 2021-11-04 19:20:17 +00:00
Родитель 8e2ddef4de
Коммит 9f23266238
6 изменённых файлов: 159 добавлений и 105 удалений

Просмотреть файл

@ -21,7 +21,7 @@ include ProtocolTypes;
using struct mozilla::void_t from "mozilla/ipc/IPCCore.h";
[MoveOnly] using struct mozilla::SerializedStructuredCloneBuffer
[MoveOnly=data] using struct mozilla::SerializedStructuredCloneBuffer
from "mozilla/ipc/SerializedStructuredCloneBuffer.h";
using class mozilla::dom::LoadingSessionHistoryInfo

Просмотреть файл

@ -236,8 +236,13 @@ class UsingStmt(Node):
def isRefcounted(self):
return "RefCounted" in self.attributes
def isMoveonly(self):
return "MoveOnly" in self.attributes
def isSendMoveOnly(self):
moveonly = self.attributes.get("MoveOnly")
return moveonly and moveonly.value in (None, "send")
def isDataMoveOnly(self):
moveonly = self.attributes.get("MoveOnly")
return moveonly and moveonly.value in (None, "data")
# "singletons"

Просмотреть файл

@ -269,10 +269,6 @@ def _refptr(T):
return Type("RefPtr", T=T)
def _uniqueptr(T):
return Type("UniquePtr", T=T)
def _alreadyaddrefed(T):
return Type("already_AddRefed", T=T)
@ -300,8 +296,8 @@ def _makePromise(returns, side, resolver=False):
def _resolveType(returns, side):
if len(returns) > 1:
return _tuple([d.moveType(side) for d in returns])
return returns[0].moveType(side)
return _tuple([d.inType(side) for d in returns])
return returns[0].inType(side)
def _makeResolver(returns, side):
@ -563,16 +559,13 @@ def _cxxConstRefType(ipdltype, side):
if ipdltype.isIPDL() and ipdltype.isShmem():
t.ref = True
return t
if ipdltype.isIPDL() and ipdltype.isByteBuf():
t.ref = True
return t
if ipdltype.isIPDL() and ipdltype.hasBaseType():
# Keep same constness as inner type.
inner = _cxxConstRefType(ipdltype.basetype, side)
t.const = inner.const or not inner.ref
t.ref = True
return t
if ipdltype.isCxx() and ipdltype.isMoveonly():
if ipdltype.isCxx() and (ipdltype.isSendMoveOnly() or ipdltype.isDataMoveOnly()):
t.const = True
t.ref = True
return t
@ -581,41 +574,46 @@ def _cxxConstRefType(ipdltype, side):
t = t.T
t.ptr = True
return t
if ipdltype.isUniquePtr():
t.ref = True
return t
t.const = True
t.ref = True
return t
def _cxxTypeCanMoveSend(ipdltype):
return ipdltype.isUniquePtr()
def _cxxTypeNeedsMoveForSend(ipdltype, context="root", visited=None):
"""Returns `True` if serializing ipdltype requires a mutable reference, e.g.
because the underlying resource represented by the value is being
transferred to another process. This is occasionally distinct from whether
the C++ type exposes a copy constructor, such as for types which are not
cheaply copiable, but are not mutated when serialized."""
if visited is None:
visited = set()
def _cxxTypeNeedsMove(ipdltype):
if _cxxTypeNeedsMoveForSend(ipdltype):
return True
if ipdltype.isIPDL():
return ipdltype.isArray()
return False
def _cxxTypeNeedsMoveForSend(ipdltype):
if ipdltype.isUniquePtr():
return True
visited.add(ipdltype)
if ipdltype.isCxx():
return ipdltype.isMoveonly()
return ipdltype.isSendMoveOnly()
if ipdltype.isIPDL():
if ipdltype.hasBaseType():
return _cxxTypeNeedsMove(ipdltype.basetype)
return _cxxTypeNeedsMoveForSend(ipdltype.basetype, "wrapper", visited)
if ipdltype.isStruct() or ipdltype.isUnion():
return any(
_cxxTypeNeedsMoveForSend(t, "compound", visited)
for t in ipdltype.itercomponents()
if t not in visited
)
# For historical reasons, shmem is `const_cast` to a mutable reference
# when being stored in a struct or union (see
# `_StructField.constRefExpr` and `_UnionMember.getConstValue`), meaning
# that they do not cause the containing struct to require move for
# sending.
if ipdltype.isShmem():
return context != "compound"
return (
ipdltype.isShmem()
or ipdltype.isByteBuf()
ipdltype.isByteBuf()
or ipdltype.isEndpoint()
or ipdltype.isManagedEndpoint()
)
@ -623,28 +621,40 @@ def _cxxTypeNeedsMoveForSend(ipdltype):
return False
# FIXME Bug 1547019 This should be the same as _cxxTypeNeedsMoveForSend, but
# a lot of existing code needs to be updated and fixed before
# we can do that.
def _cxxTypeCanOnlyMove(ipdltype, visited=None):
def _cxxTypeNeedsMoveForData(ipdltype, context="root", visited=None):
"""Returns `True` if the bare C++ type corresponding to ipdltype does not
satisfy std::is_copy_constructible_v<T>. All C++ types supported by IPDL
must support std::is_move_constructible_v<T>, so non-movable types must be
passed behind a `UniquePtr`."""
if visited is None:
visited = set()
visited.add(ipdltype)
if ipdltype.isUniquePtr():
return True
if ipdltype.isCxx():
return ipdltype.isMoveonly()
return ipdltype.isDataMoveOnly()
if ipdltype.isIPDL():
if ipdltype.isMaybe() or ipdltype.isArray():
return _cxxTypeCanOnlyMove(ipdltype.basetype, visited)
# When nested within a maybe or array, arrays are no longer copyable.
if context == "wrapper" and ipdltype.isArray():
return True
if ipdltype.hasBaseType():
return _cxxTypeNeedsMoveForData(ipdltype.basetype, "wrapper", visited)
if ipdltype.isStruct() or ipdltype.isUnion():
return any(
_cxxTypeCanOnlyMove(t, visited)
_cxxTypeNeedsMoveForData(t, "compound", visited)
for t in ipdltype.itercomponents()
if t not in visited
)
return ipdltype.isManagedEndpoint()
return (
ipdltype.isByteBuf()
or ipdltype.isEndpoint()
or ipdltype.isManagedEndpoint()
)
return False
@ -653,14 +663,6 @@ def _cxxTypeCanMove(ipdltype):
return not (ipdltype.isIPDL() and ipdltype.isActor())
def _cxxMoveRefType(ipdltype, side):
t = _cxxBareType(ipdltype, side)
if _cxxTypeNeedsMove(ipdltype):
t.rvalref = True
return t
return _cxxConstRefType(ipdltype, side)
def _cxxForceMoveRefType(ipdltype, side):
assert _cxxTypeCanMove(ipdltype)
t = _cxxBareType(ipdltype, side)
@ -689,6 +691,23 @@ def _cxxConstPtrToType(ipdltype, side):
return t
def _cxxInType(ipdltype, side):
t = _cxxBareType(ipdltype, side)
if ipdltype.isIPDL() and ipdltype.isActor():
return t
if _cxxTypeNeedsMoveForSend(ipdltype):
t.rvalref = True
return t
if ipdltype.isCxx() and ipdltype.isRefcounted():
# Use T* instead of const RefPtr<T>&
t = t.T
t.ptr = True
return t
t.const = True
t.ref = True
return t
def _allocMethod(ptype, side):
return "Alloc" + ptype.name() + side.title()
@ -729,10 +748,6 @@ class _HybridDecl:
"""Return this decl's C++ type as a const, 'reference' type."""
return _cxxConstRefType(self.ipdltype, side)
def rvalueRefType(self, side):
"""Return this decl's C++ type as an r-value 'reference' type."""
return _cxxMoveRefType(self.ipdltype, side)
def ptrToType(self, side):
return _cxxPtrToType(self.ipdltype, side)
@ -740,18 +755,8 @@ class _HybridDecl:
return _cxxConstPtrToType(self.ipdltype, side)
def inType(self, side):
"""Return this decl's C++ Type with inparam semantics."""
if self.ipdltype.isIPDL() and self.ipdltype.isActor():
return self.bareType(side)
elif _cxxTypeNeedsMoveForSend(self.ipdltype):
return self.rvalueRefType(side)
return self.constRefType(side)
def moveType(self, side):
"""Return this decl's C++ Type with move semantics."""
if self.ipdltype.isIPDL() and self.ipdltype.isActor():
return self.bareType(side)
return self.rvalueRefType(side)
"""Return this decl's C++ Type with sending inparam semantics."""
return _cxxInType(self.ipdltype, side)
def outType(self, side):
"""Return this decl's C++ Type with outparam semantics."""
@ -852,10 +857,6 @@ class _StructField(_CompoundTypeComponent):
refexpr = self.refExpr(thisexpr)
if "Shmem" == self.ipdltype.name():
refexpr = ExprCast(refexpr, Type("Shmem", ref=True), const=True)
if "ByteBuf" == self.ipdltype.name():
refexpr = ExprCast(refexpr, Type("ByteBuf", ref=True), const=True)
if "FileDescriptor" == self.ipdltype.name():
refexpr = ExprCast(refexpr, Type("FileDescriptor", ref=True), const=True)
return refexpr
def argVar(self):
@ -1029,12 +1030,8 @@ class _UnionMember(_CompoundTypeComponent):
def getConstValue(self):
v = ExprDeref(self.callGetConstPtr())
# sigh
if "ByteBuf" == self.ipdltype.name():
v = ExprCast(v, Type("ByteBuf", ref=True), const=True)
if "Shmem" == self.ipdltype.name():
v = ExprCast(v, Type("Shmem", ref=True), const=True)
if "FileDescriptor" == self.ipdltype.name():
v = ExprCast(v, Type("FileDescriptor", ref=True), const=True)
return v
@ -1127,9 +1124,23 @@ class MessageDecl(ipdl.ast.MessageDecl):
return Decl(Type("Tainted", T=d.bareType(side)), d.name)
if sems == "in":
return Decl(d.inType(side), d.name)
t = d.inType(side)
# If this is the `recv` side, and we're not using "move"
# semantics, that means we're an alloc method, and cannot accept
# values by rvalue reference. Downgrade to an lvalue reference.
if direction == "recv" and t.rvalref:
t.rvalref = False
t.ref = True
return Decl(t, d.name)
elif sems == "move":
return Decl(d.moveType(side), d.name)
assert direction == "recv"
# For legacy reasons, use an rvalue reference when generating
# parameters for recv methods which accept arrays.
if d.ipdltype.isIPDL() and d.ipdltype.isArray():
t = d.bareType(side)
t.rvalref = True
return Decl(t, d.name)
return Decl(d.inType(side), d.name)
elif sems == "out":
return Decl(d.outType(side), d.name)
else:
@ -2010,8 +2021,6 @@ class _ParamTraits:
@classmethod
def write(cls, var, msgvar, actor, ipdltype=None):
# WARNING: This doesn't set AutoForActor for you, make sure this is
# only called when the actor is already correctly set.
if ipdltype and _cxxTypeNeedsMoveForSend(ipdltype):
var = ExprMove(var)
return ExprCall(ExprVar("WriteIPDLParam"), args=[msgvar, actor, var])
@ -2158,7 +2167,7 @@ class _ParamTraits:
)
@classmethod
def generateDecl(cls, fortype, write, read, constin=True):
def generateDecl(cls, fortype, write, read, needsmove=False):
# IPDLParamTraits impls are selected ignoring constness, and references.
pt = Class(
"IPDLParamTraits",
@ -2174,7 +2183,10 @@ class _ParamTraits:
iprotocoltype = Type("mozilla::ipc::IProtocol", ptr=True)
# static void Write(Message*, const T&);
intype = Type("paramType", ref=True, const=constin)
if needsmove:
intype = Type("paramType", rvalref=True)
else:
intype = Type("paramType", ref=True, const=True)
writemthd = MethodDefn(
MethodDecl(
"Write",
@ -2331,7 +2343,9 @@ class _ParamTraits:
read.append(StmtReturn.TRUE)
return cls.generateDecl(cxxtype, write, read)
return cls.generateDecl(
cxxtype, write, read, needsmove=_cxxTypeNeedsMoveForSend(structtype)
)
@classmethod
def unionPickling(cls, uniontype):
@ -2431,7 +2445,9 @@ class _ParamTraits:
StmtBlock([cls.fatalError("unknown union type"), StmtReturn.FALSE]),
)
return cls.generateDecl(cxxtype, write, read)
return cls.generateDecl(
cxxtype, write, read, needsmove=_cxxTypeNeedsMoveForSend(uniontype)
)
# --------------------------------------------------
@ -2605,12 +2621,11 @@ def _generateCxxStruct(sd):
constreftype = Type(sd.name, const=True, ref=True)
def fieldsAsParamList():
# FIXME Bug 1547019 inType() should do the right thing once
# _cxxTypeCanOnlyMove is replaced with
# _cxxTypeNeedsMoveForSend
return [
Decl(
f.forceMoveType() if _cxxTypeCanOnlyMove(f.ipdltype) else f.inType(),
f.forceMoveType()
if _cxxTypeNeedsMoveForData(f.ipdltype)
else f.constRefType(),
f.argVar().name,
)
for f in sd.fields_ipdl_order()
@ -2642,7 +2657,7 @@ def _generateCxxStruct(sd):
valctor.memberinits = []
for f in sd.fields_member_order():
arg = f.argVar()
if _cxxTypeCanOnlyMove(f.ipdltype):
if _cxxTypeNeedsMoveForData(f.ipdltype):
arg = ExprMove(arg)
valctor.memberinits.append(ExprMemberInit(f.memberVar(), args=[arg]))
@ -2953,9 +2968,9 @@ def _generateCxxUnion(ud):
# Union(const T&) copy & Union(T&&) move ctors
othervar = ExprVar("aOther")
for c in ud.components:
if not _cxxTypeCanOnlyMove(c.ipdltype):
if not _cxxTypeNeedsMoveForData(c.ipdltype):
copyctor = ConstructorDefn(
ConstructorDecl(ud.name, params=[Decl(c.inType(), othervar.name)])
ConstructorDecl(ud.name, params=[Decl(c.constRefType(), othervar.name)])
)
copyctor.addstmts(
[
@ -2965,7 +2980,7 @@ def _generateCxxUnion(ud):
)
cls.addstmts([copyctor, Whitespace.NL])
if not _cxxTypeCanMove(c.ipdltype) or _cxxTypeNeedsMoveForSend(c.ipdltype):
if not _cxxTypeCanMove(c.ipdltype):
continue
movector = ConstructorDefn(
ConstructorDecl(ud.name, params=[Decl(c.forceMoveType(), othervar.name)])
@ -2978,7 +2993,7 @@ def _generateCxxUnion(ud):
)
cls.addstmts([movector, Whitespace.NL])
unionNeedsMove = any(_cxxTypeCanOnlyMove(c.ipdltype) for c in ud.components)
unionNeedsMove = any(_cxxTypeNeedsMoveForData(c.ipdltype) for c in ud.components)
# Union(const Union&) copy ctor
if not unionNeedsMove:
@ -3095,11 +3110,13 @@ def _generateCxxUnion(ud):
# Union& operator= methods
rhsvar = ExprVar("aRhs")
for c in ud.components:
if not _cxxTypeCanOnlyMove(c.ipdltype):
if not _cxxTypeNeedsMoveForData(c.ipdltype):
# Union& operator=(const T&)
opeq = MethodDefn(
MethodDecl(
"operator=", params=[Decl(c.inType(), rhsvar.name)], ret=refClsType
"operator=",
params=[Decl(c.constRefType(), rhsvar.name)],
ret=refClsType,
)
)
opeq.addstmts(
@ -3114,7 +3131,7 @@ def _generateCxxUnion(ud):
cls.addstmts([opeq, Whitespace.NL])
# Union& operator=(T&&)
if not _cxxTypeCanMove(c.ipdltype) or _cxxTypeNeedsMoveForSend(c.ipdltype):
if not _cxxTypeCanMove(c.ipdltype):
continue
opeq = MethodDefn(
@ -3255,7 +3272,7 @@ def _generateCxxUnion(ud):
opeqeq = MethodDefn(
MethodDecl(
"operator==",
params=[Decl(c.inType(), rhsvar.name)],
params=[Decl(c.constRefType(), rhsvar.name)],
ret=Type.BOOL,
const=True,
)
@ -4730,7 +4747,11 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
helper.addstmts(
[
self.callAllocActor(md, retsems="out", side=self.side),
StmtReturn(ExprCall(ExprVar(helperdecl.name), args=md.makeCxxArgs())),
StmtReturn(
ExprCall(
ExprVar(helperdecl.name), args=md.makeCxxArgs(paramsems="move")
)
),
]
)
return helper

Просмотреть файл

@ -163,12 +163,13 @@ VOID = VoidType()
class ImportedCxxType(Type):
def __init__(self, qname, refcounted, moveonly):
def __init__(self, qname, refcounted, sendmoveonly, datamoveonly):
assert isinstance(qname, QualifiedId)
self.loc = qname.loc
self.qname = qname
self.refcounted = refcounted
self.moveonly = moveonly
self.sendmoveonly = sendmoveonly
self.datamoveonly = datamoveonly
def isCxx(self):
return True
@ -179,8 +180,11 @@ class ImportedCxxType(Type):
def isRefcounted(self):
return self.refcounted
def isMoveonly(self):
return self.moveonly
def isSendMoveOnly(self):
return self.sendmoveonly
def isDataMoveOnly(self):
return self.datamoveonly
def name(self):
return self.qname.baseid
@ -1107,7 +1111,7 @@ class GatherDecls(TcheckVisitor):
self.checkAttributes(
using.attributes,
{
"MoveOnly": None,
"MoveOnly": (None, "data", "send"),
"RefCounted": None,
},
)
@ -1120,7 +1124,10 @@ class GatherDecls(TcheckVisitor):
ipdltype = FDType(using.type.spec)
else:
ipdltype = ImportedCxxType(
using.type.spec, using.isRefcounted(), using.isMoveonly()
using.type.spec,
using.isRefcounted(),
using.isSendMoveOnly(),
using.isDataMoveOnly(),
)
existingType = self.symtab.lookup(ipdltype.fullname())
if existingType and existingType.fullname == ipdltype.fullname():
@ -1130,7 +1137,10 @@ class GatherDecls(TcheckVisitor):
"inconsistent refcounted status of type `%s`",
str(using.type),
)
if ipdltype.isMoveonly() != existingType.type.isMoveonly():
if (
ipdltype.isSendMoveOnly() != existingType.type.isSendMoveOnly()
or ipdltype.isDataMoveOnly() != existingType.type.isDataMoveOnly()
):
self.error(
using.loc,
"inconsistent moveonly status of type `%s`",

Просмотреть файл

@ -1,8 +1,12 @@
//error: inconsistent moveonly status of type `mozilla::ipc::SomeMoveonlyType`
//error: inconsistent moveonly status of type `mozilla::ipc::SomeMoveonlySendType`
[MoveOnly] using class mozilla::ipc::SomeMoveonlyType from "SomeFile.h";
using class mozilla::ipc::SomeMoveonlyType from "SomeFile.h";
[MoveOnly=send] using class mozilla::ipc::SomeMoveonlySendType from "SomeFile.h";
[MoveOnly=data] using class mozilla::ipc::SomeMoveonlySendType from "SomeFile.h";
protocol PInconsistentMoveOnly {
child:
async SomeMessage();

Просмотреть файл

@ -14,6 +14,14 @@ using struct SomeStruct from "SomeFile.h";
[RefCounted, MoveOnly] using class SomeRefcountedMoveonlyClass from "SomeFile.h";
[RefCounted, MoveOnly] using struct SomeRefcountedMoveonlyStruct from "SomeFile.h";
[MoveOnly=data] using SomeMoveonlyDataType from "SomeFile.h";
[MoveOnly=data] using class SomeMoveonlyDataClass from "SomeFile.h";
[MoveOnly=data] using struct SomeMoveonlyDataStruct from "SomeFile.h";
[MoveOnly=send] using SomeMoveonlySendType from "SomeFile.h";
[MoveOnly=send] using class SomeMoveonlySendClass from "SomeFile.h";
[MoveOnly=send] using struct SomeMoveonlySendStruct from "SomeFile.h";
union SomeUnion
{
SomeType;
@ -28,6 +36,12 @@ union SomeUnion
SomeRefcountedMoveonlyType;
SomeRefcountedMoveonlyClass;
SomeRefcountedMoveonlyStruct;
SomeMoveonlyDataType;
SomeMoveonlyDataClass;
SomeMoveonlyDataStruct;
SomeMoveonlySendType;
SomeMoveonlySendClass;
SomeMoveonlySendStruct;
};
protocol PbasicUsing {