[PATCH 0/3] MR8507: widl: Drop an incorrect check on runtimeclass definitions.
From: Hans Leidekker <hans(a)codeweavers.com> midlrt does require a default interface when the runtimeclass is used as a parameter. --- tools/widl/typetree.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tools/widl/typetree.c b/tools/widl/typetree.c index 2f67545393e..af62c2a03ba 100644 --- a/tools/widl/typetree.c +++ b/tools/widl/typetree.c @@ -854,9 +854,6 @@ type_t *type_runtimeclass_define(type_t *runtimeclass, attr_list_t *attrs, runtimeclass->attrs = check_runtimeclass_attrs(runtimeclass->name, attrs); runtimeclass->details.runtimeclass.ifaces = ifaces; define_type(runtimeclass, where); - if (!type_runtimeclass_get_default_iface(runtimeclass, FALSE) && - !get_attrp(runtimeclass->attrs, ATTR_STATIC)) - error_loc("runtimeclass %s must have a default interface or static factory\n", runtimeclass->name); if (ifaces) LIST_FOR_EACH_ENTRY(ref, ifaces, typeref_t, entry) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8507
From: Hans Leidekker <hans(a)codeweavers.com> This makes the runtimeclass available when metadata for the constructor interface is generated. --- tools/widl/typetree.c | 17 +++++++++++++++++ tools/widl/widltypes.h | 1 + 2 files changed, 18 insertions(+) diff --git a/tools/widl/typetree.c b/tools/widl/typetree.c index af62c2a03ba..ab671e021f0 100644 --- a/tools/widl/typetree.c +++ b/tools/widl/typetree.c @@ -748,6 +748,7 @@ type_t *type_interface_define(type_t *iface, attr_list_t *attrs, type_t *inherit iface->details.iface->inherit = inherit; iface->details.iface->disp_inherit = NULL; iface->details.iface->async_iface = NULL; + iface->details.iface->runtime_class = NULL; iface->details.iface->requires = requires; define_type(iface, where); compute_method_indexes(iface); @@ -845,6 +846,21 @@ type_t *type_runtimeclass_declare(char *name, struct namespace *namespace) return type; } +static void set_constructor_runtimeclass(type_t *runtimeclass, attr_list_t *attrs) +{ + const attr_t *attr; + if (!attrs) return; + LIST_FOR_EACH_ENTRY(attr, attrs, const attr_t, entry) + { + if (attr->type == ATTR_ACTIVATABLE || attr->type == ATTR_COMPOSABLE) + { + const expr_t *value = attr->u.pval; + if (value->u.tref.type->details.iface) + value->u.tref.type->details.iface->runtime_class = runtimeclass; + } + } +} + type_t *type_runtimeclass_define(type_t *runtimeclass, attr_list_t *attrs, typeref_list_t *ifaces, const struct location *where) { @@ -853,6 +869,7 @@ type_t *type_runtimeclass_define(type_t *runtimeclass, attr_list_t *attrs, runtimeclass->attrs = check_runtimeclass_attrs(runtimeclass->name, attrs); runtimeclass->details.runtimeclass.ifaces = ifaces; + set_constructor_runtimeclass(runtimeclass, attrs); define_type(runtimeclass, where); if (ifaces) LIST_FOR_EACH_ENTRY(ref, ifaces, typeref_t, entry) diff --git a/tools/widl/widltypes.h b/tools/widl/widltypes.h index 620a3857bc9..1b032a17c4a 100644 --- a/tools/widl/widltypes.h +++ b/tools/widl/widltypes.h @@ -403,6 +403,7 @@ struct iface_details struct _type_t *inherit; struct _type_t *disp_inherit; struct _type_t *async_iface; + struct _type_t *runtime_class; typeref_list_t *requires; }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8507
From: Hans Leidekker <hans(a)codeweavers.com> Currently we parse composable access as an attribute of the composable attribute which doesn't seem right conceptually. More importantly make_exprt() ignores any attributes on the variable declaration which means the value isn't available for metadata generation. --- tools/widl/parser.y | 18 ++++++++++-------- tools/widl/widltypes.h | 6 ++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/tools/widl/parser.y b/tools/widl/parser.y index 54b143637cf..e252385bf53 100644 --- a/tools/widl/parser.y +++ b/tools/widl/parser.y @@ -289,7 +289,6 @@ PARSER_LTYPE pop_import(void); %token tWCHAR tWIREMARSHAL %token tAPARTMENT tNEUTRAL tSINGLE tFREE tBOTH -%type <attr> access_attr %type <attr> attribute acf_attribute %type <attr_list> m_attributes attributes attrib_list %type <attr_list> acf_attributes acf_attribute_list @@ -336,7 +335,7 @@ PARSER_LTYPE pop_import(void); %type <type> coclass coclassdef %type <type> runtimeclass runtimeclass_def %type <type> apicontract apicontract_def -%type <num> contract_ver +%type <num> contract_ver composable_access %type <num> pointer_type threading_type marshaling_behavior version %type <str> libraryhdr callconv cppquote importlib import %type <str> typename m_typename @@ -623,16 +622,19 @@ activatable_attr: | contract_req { $$ = $1; } /* activatable on the default activation factory */ ; -access_attr - : tPUBLIC { $$ = attr_int( @$, ATTR_PUBLIC, 0 ); } - | tPROTECTED { $$ = attr_int( @$, ATTR_PROTECTED, 0 ); } +composable_access + : tPUBLIC { $$ = COMPOSABLE_ACCESS_PUBLIC; } + | tPROTECTED { $$ = COMPOSABLE_ACCESS_PROTECTED; } ; composable_attr - : decl_spec ',' access_attr ',' contract_req - { if ($1->type->type_type != TYPE_INTERFACE) + : decl_spec ',' composable_access ',' contract_req + { struct integer integer = { .value = $3 }; + if ($1->type->type_type != TYPE_INTERFACE) error_loc( "type %s is not an interface\n", $1->type->name ); - $$ = make_exprt( EXPR_MEMBER, declare_var( append_attr( NULL, $3 ), $1, make_declarator( NULL ), 0 ), $5 ); + $$ = make_exprl( EXPR_NUM, &integer ); + $$ = make_expr2( EXPR_MEMBER, $$, $5 ); + $$ = make_exprt( EXPR_MEMBER, declare_var( NULL, $1, make_declarator( NULL ), 0 ), $$ ); } ; diff --git a/tools/widl/widltypes.h b/tools/widl/widltypes.h index 1b032a17c4a..5b3967bc942 100644 --- a/tools/widl/widltypes.h +++ b/tools/widl/widltypes.h @@ -314,6 +314,12 @@ enum type_basic_type #define TYPE_BASIC_INT_MIN TYPE_BASIC_INT8 #define TYPE_BASIC_INT_MAX TYPE_BASIC_HYPER +enum composable_access +{ + COMPOSABLE_ACCESS_PROTECTED = 1, + COMPOSABLE_ACCESS_PUBLIC, +}; + struct location { const char *input_name; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8507
Rémi Bernon (@rbernon) commented about tools/widl/parser.y:
| contract_req { $$ = $1; } /* activatable on the default activation factory */ ;
-access_attr - : tPUBLIC { $$ = attr_int( @$, ATTR_PUBLIC, 0 ); } - | tPROTECTED { $$ = attr_int( @$, ATTR_PROTECTED, 0 ); } +composable_access + : tPUBLIC { $$ = COMPOSABLE_ACCESS_PUBLIC; } + | tPROTECTED { $$ = COMPOSABLE_ACCESS_PROTECTED; } The public/protected attributes already exist, I don't think it is right to duplicate it just for the composable attribute. If `make_exprt` discards attributes, it's probably better to just fix things there instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108821
Rémi Bernon (@rbernon) commented about tools/widl/typetree.c:
return type; }
+static void set_constructor_runtimeclass(type_t *runtimeclass, attr_list_t *attrs) +{ + const attr_t *attr; + if (!attrs) return; + LIST_FOR_EACH_ENTRY(attr, attrs, const attr_t, entry) + { + if (attr->type == ATTR_ACTIVATABLE || attr->type == ATTR_COMPOSABLE) + { + const expr_t *value = attr->u.pval; + if (value->u.tref.type->details.iface) + value->u.tref.type->details.iface->runtime_class = runtimeclass;
It doesn't seem safe to access expression like that. The `activatable` attribute may have an interface or not. When it has, the expression is an `EXPR_MEMBER (decl_spec, contract_req)` but when not, the expression is a `contract_req` expression only `EXPR_GTREQL (decl_spec, version)` and its decl_spec type is an apicontract, not an interface. This was enough until now, but maybe we should start using dedicated attribute values instead of abusing expressions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108823
Rémi Bernon (@rbernon) commented about tools/widl/typetree.c:
runtimeclass->attrs = check_runtimeclass_attrs(runtimeclass->name, attrs); runtimeclass->details.runtimeclass.ifaces = ifaces; define_type(runtimeclass, where); - if (!type_runtimeclass_get_default_iface(runtimeclass, FALSE) && - !get_attrp(runtimeclass->attrs, ATTR_STATIC)) - error_loc("runtimeclass %s must have a default interface or static factory\n", runtimeclass->name);
This might not be the right place to check for it but midl definitely emits an error for such cases, and it should be moved where appropriate, not removed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108822
On Fri Jul 4 13:00:38 2025 +0000, Rémi Bernon wrote:
It doesn't seem safe to access expression like that. The `activatable` attribute may have an interface or not. When it has, the expression is an `EXPR_MEMBER (decl_spec, contract_req)` but when not, the expression is a `contract_req` expression only `EXPR_GTREQL (decl_spec, version)` and its decl_spec type is an apicontract, not an interface. This was enough until now, but maybe we should start using dedicated attribute values instead of abusing expressions. Or, alternatively, we could change `activatable_attr` to always generate the same kind of expressions, by creating an implicit `IActivationFactory` interface.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108827
On Fri Jul 4 12:51:43 2025 +0000, Rémi Bernon wrote:
The public/protected attributes already exist, I don't think it is right to duplicate it just for the composable attribute. If `make_exprt` discards attributes, it's probably better to just fix things there instead. The attributes exist but they are only used with types in IDL, not attributes. It seems cleaner to me to follow that principle in our code. I'm also not sure how it would be better to fix things in make_exprt().
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108828
On Fri Jul 4 12:59:56 2025 +0000, Rémi Bernon wrote:
This might not be the right place to check for it but midl definitely emits an error for such cases, and it should be moved where appropriate, not removed. midl? Isn't runtimeclass a winrt-only type? midlrt definitely accepts this:
namespace Winetest {
[
contract(Windows.Foundation.UniversalApiContract, 1.0),
uuid(11111111-2222-3333-4444-555555555555),
]
interface IWineInterface : IInspectable
{
HRESULT WineMethod( int param );
};
[
contract(Windows.Foundation.UniversalApiContract, 1.0),
]
runtimeclass WineRuntimeClass
{
interface IWineInterface;
};
}
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108829
On Fri Jul 4 13:10:51 2025 +0000, Rémi Bernon wrote:
Or, alternatively, we could change `activatable_attr` to always generate the same kind of expressions, by creating an implicit `IActivationFactory` interface. Edit: Hmm, actually this might require some specific order with other attributes if that interface is supposed to live on the static interface, so possibly not a good idea, idk. This works for our IDL files which have activatable attributes with and without an interface.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108830
On Fri Jul 4 13:29:55 2025 +0000, Hans Leidekker wrote:
midl? Isn't runtimeclass a winrt-only type? midlrt definitely accepts this: ``` namespace Winetest { [ contract(Windows.Foundation.UniversalApiContract, 1.0), uuid(11111111-2222-3333-4444-555555555555), ] interface IWineInterface : IInspectable { HRESULT WineMethod( int param ); }; [ contract(Windows.Foundation.UniversalApiContract, 1.0), ] runtimeclass WineRuntimeClass { interface IWineInterface; }; } ``` The error might be relaxed to having at least one interface rather than a default one, but if you remove the interface you get an error.
midl? Isn't runtimeclass a winrt-only type?
Sure, midl, midlrt, it's more or less the same thing as one might call the other or the other way around anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108832
On Fri Jul 4 13:29:55 2025 +0000, Hans Leidekker wrote:
This works for our IDL files which have activatable attributes with and without an interface. It only works because apicontract types don't use their details union. It would be incorrect and would possibly crash otherwise.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108833
On Fri Jul 4 13:29:55 2025 +0000, Hans Leidekker wrote:
The attributes exist but they are only used with types in IDL, not attributes. It seems cleaner to me to follow that principle in our code. I'm also not sure how it would be better to fix things in make_exprt(). I think it is better to use attributes which have the proper semantics already and keep them in the attributes of a declspec/var attributes where all attributes are (even though these attributes are normally restricted to types when used in the IDL), rather than integral values arbitrarily inserted in the expression tree which is probably already abused a bit too much.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108834
On Fri Jul 4 13:52:00 2025 +0000, Rémi Bernon wrote:
I think it is better to use attributes which have the proper semantics already and keep them in the attributes of a declspec/var attributes where all attributes are (even though these attributes are normally restricted to types when used in the IDL), rather than integral values arbitrarily inserted in the expression tree which is probably already abused a bit too much. Where do you propose to store the attributes in make_exprt()?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108835
On Fri Jul 4 13:38:20 2025 +0000, Rémi Bernon wrote:
The error might be relaxed to having at least one interface rather than a default one, but if you remove the interface you get an error.
midl? Isn't runtimeclass a winrt-only type? Sure, midl, midlrt, it's more or less the same thing as one might call the other or the other way around anyway. I'll change it to a relaxed check.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108836
On Fri Jul 4 13:40:17 2025 +0000, Rémi Bernon wrote:
It only works because apicontract types don't use their details union. It would be incorrect and would possibly crash otherwise. I'll add a check on the expression type.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108837
On Fri Jul 4 14:20:44 2025 +0000, Hans Leidekker wrote:
Where do you propose to store the attributes in make_exprt()? Well we could keep a pointer to the var instead of a pointer to the type in these expressions, keeping access to the variable attributes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8507#note_108838
participants (3)
-
Hans Leidekker -
Hans Leidekker (＠hans)
-
Rémi Bernon