January 2022 - Wine-Devel - list.winehq.org

Re: [PATCH vkd3d 4/6] vkd3d-shader/hlsl: Parse bitwise AND.

by Zebediah Figura (she/her)

On 1/25/22 05:07, Giovanni Mascellani wrote: > Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> > --- > libs/vkd3d-shader/hlsl.y | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y > index cff56a74..873ba29d 100644 > --- a/libs/vkd3d-shader/hlsl.y > +++ b/libs/vkd3d-shader/hlsl.y > @@ -1057,6 +1057,30 @@ static struct list *add_binary_arithmetic_expr_merge(struct hlsl_ctx *ctx, struc > return list1; > } > > +static struct hlsl_ir_expr *add_binary_bitwise_expr(struct hlsl_ctx *ctx, struct list *instrs, > + enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2, > + const struct vkd3d_shader_location *loc) > +{ > + if (arg1->data_type->base_type == HLSL_TYPE_HALF || arg1->data_type->base_type == HLSL_TYPE_FLOAT) > + return NULL; > + if (arg2->data_type->base_type == HLSL_TYPE_HALF || arg2->data_type->base_type == HLSL_TYPE_FLOAT) > + return NULL; Shouldn't this emit some sort of parse error? > + > + return add_binary_arithmetic_expr(ctx, instrs, op, arg1, arg2, loc); > +}

Jan. 25, 2022

Re: [PATCH vkd3d 3/6] vkd3d-shader/hlsl: Write SM4 unsigned multiplication instructions.

by Zebediah Figura (she/her)

On 1/25/22 05:07, Giovanni Mascellani wrote: > +static void write_sm4_binary_op_with_null(struct vkd3d_bytecode_buffer *buffer, enum vkd3d_sm4_opcode opcode, > + const struct hlsl_ir_node *dst, unsigned dst_idx, const struct hlsl_ir_node *src1, > + const struct hlsl_ir_node *src2) > +{ > + struct sm4_instruction instr; > + unsigned int writemask; > + > + memset(&instr, 0, sizeof(instr)); > + instr.opcode = opcode; > + > + instr.dsts[1 - dst_idx].reg.type = VKD3D_SM4_RT_NULL; > + instr.dsts[1 - dst_idx].reg.dim = VKD3D_SM4_DIMENSION_NONE; > + instr.dsts[1 - dst_idx].reg.idx_count = 0; > + sm4_register_from_node(&instr.dsts[dst_idx].reg, &instr.dsts[dst_idx].writemask, NULL, dst); > + instr.dst_count = 2; > + > + sm4_register_from_node(&instr.srcs[0].reg, &writemask, &instr.srcs[0].swizzle_type, src1); > + instr.srcs[0].swizzle = hlsl_map_swizzle(hlsl_swizzle_from_writemask(writemask), instr.dsts[1].writemask); > + sm4_register_from_node(&instr.srcs[1].reg, &writemask, &instr.srcs[1].swizzle_type, src2); > + instr.srcs[1].swizzle = hlsl_map_swizzle(hlsl_swizzle_from_writemask(writemask), instr.dsts[1].writemask); > + instr.src_count = 2; > + > + write_sm4_instruction(buffer, &instr); > +} I want to find some way to put "2 dsts" in the function name, because as it is I can't tell just from reading. I'd also suggest specifying both dsts as optional hlsl_ir_node pointers, where NULL translates to SM4_RT_NULL. That would allow you to use this helper even if both dsts are used.

Jan. 25, 2022

Re: [PATCH vkd3d 2/6] tests: Test a number of simple HLSL operations.

by Zebediah Figura (she/her)

On 1/25/22 05:07, Giovanni Mascellani wrote: > diff --git a/tests/hlsl-operations.shader_test b/tests/hlsl-operations.shader_test > new file mode 100644 > index 00000000..3c8036b5 > --- /dev/null > +++ b/tests/hlsl-operations.shader_test > @@ -0,0 +1,86 @@ > +[pixel shader] > +float4 main(uniform float zero, uniform float one, uniform float x, uniform float y, > + float4 pos : SV_POSITION) : SV_TARGET > +{ > + int izero = zero; > + int ione = one; > + int ix = x; > + int iy = y; > + uint uzero = zero; > + uint uone = one; > + uint ux = x; > + uint uy = y; > + > + if (pos.x == 0.5) > + return float4(x + y, x - y, x * y, x / y); > + if (pos.x == 1.5) > + return float4(ix + iy, ix - iy, ix * iy, ix / iy); > + if (pos.x == 2.5) > + return float4(ux + uy, ux - uy, ux * uy, ux / uy); > + if (pos.x == 3.5) > + return float4(x % y, +x, -x, y / x); > + if (pos.x == 4.5) > + return float4(ix % iy, +ix, -ix, iy / ix); > + if (pos.x == 5.5) > + return float4(ux % uy, +ux, -ux, uy / ux); > + if (pos.x == 6.5) > + return float4(x == y, x != y, x < y, x <= y); > + if (pos.x == 7.5) > + return float4(x > y, x >= y, !x, !zero); > + if (pos.x == 8.5) > + return float4(ix == iy, ix != iy, ix < iy, ix <= iy); > + if (pos.x == 9.5) > + return float4(ix > iy, ix >= iy, !ix, !izero); > + if (pos.x == 10.5) > + return float4(ix & iy, ix | iy, ix ^ iy, ~ix); > + if (pos.x == 11.5) > + return float4(ix >> 2, ix << 2, iy >> 2, iy << 2); > + if (pos.x == 12.5) > + return float4(ux == uy, ux != uy, ux < uy, ux <= uy); > + if (pos.x == 13.5) > + return float4(ux > uy, ux >= uy, !ux, !uzero); > + if (pos.x == 14.5) > + return float4(ux & uy, ux | uy, ux ^ uy, ~ux); > + if (pos.x == 15.5) > + return float4(ux >> 2, ux << 2, uy >> 2, uy << 2); > + if (pos.x == 16.5) > + return float4(zero && zero, zero && one, one && zero, one && one); > + if (pos.x == 17.5) > + return float4(zero || zero, zero || one, one || zero, one || one); > + if (pos.x == 18.5) > + return float4(izero && izero, izero && ione, ione && izero, ione && ione); > + if (pos.x == 19.5) > + return float4(izero || izero, izero || ione, ione || izero, ione || ione); > + if (pos.x == 20.5) > + return float4(uzero && uzero, uzero && uone, uone && uzero, uone && uone); > + if (pos.x == 21.5) > + return float4(uzero || uzero, uzero || uone, uone || uzero, uone || uone); > + > + return float4(0.0, 0.0, 0.0, 0.0); > +} I don't really like this approach. For one thing, it makes it harder to validate e.g. patches 4-6 from this series (I essentially have to write my own shaders). The complexity of the generated shader also makes it harder to read the IR to validate it's doing the right thing. Perhaps more importantly, some of these operations are supported by all HLSL versions, but some are only supported by SM4 (notably integer operations). Anything that can be tested in sm2 or sm3 should be, and as such I'd appreciate splitting those out where possible.

Jan. 25, 2022

Re: [PATCH vkd3d 1/6] tests: Test matrix multiplication.

by Zebediah Figura (she/her)

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- This will need to be changed to be compatible with SM1, probably by using texcoord instead of sv_position, but that can be deferred for later.

Jan. 25, 2022

Re: [PATCH] ieframe: Clear a being invalidated history entry.

by Dmitry Timoshkov

Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> wrote: > On 1/24/22 15:04, Dmitry Timoshkov wrote: > > update_travellog() in order to clear forward history calls free_travellog_entry() to > > invalidate forward history entries, and when later an entry gets reused entry->stream > > contains a no longer valid pointer. > > > How does it "get reused"? Note that log buffer is also initially not > zero-initialized and generally depends on proper bounds checks. > update_travellog() decrements length when it clears forward history, > which should prevent us from treating those entries as valid. Probably "gets reused" is a wrong term. What I observe here is that once update_travellog() truncates the log, and position in the history is equal to the length, next call to go_forward() will crash because bounds check 'if (position >= length) return E_FAIL;' doesn't prevent referencing a no longer valid history entry. Does that explain what is going on? -- Dmitry.

Jan. 25, 2022

Re: [RFC PATCH v2 05/11] server: Add helpers for synchronous APC delivery via ordinary server call reply.

by Zebediah Figura (she/her)

On 1/22/22 08:36, Jinoh Kang wrote: > diff --git a/server/protocol.def b/server/protocol.def > index db73f0418a9..f21f7187c4d 100644 > --- a/server/protocol.def > +++ b/server/protocol.def > @@ -695,6 +695,13 @@ typedef union > } break_process; > } apc_result_t; > > +typedef struct > +{ > + apc_call_t call; /* APC call arguments */ > + obj_handle_t apc_handle; /* handle to next APC */ > + int __pad; > +} inline_apc_t; > + Why introduce this separate type?

Jan. 25, 2022

Re: [RFC PATCH v2 04/11] server: Attempt to complete I/O request immediately in recv_socket.

by Zebediah Figura (she/her)

On 1/22/22 08:36, Jinoh Kang wrote: > diff --git a/include/wine/afd.h b/include/wine/afd.h > index efd5787e90a..1e26739229d 100644 > --- a/include/wine/afd.h > +++ b/include/wine/afd.h > @@ -37,6 +37,23 @@ struct afd_wsabuf_32 > # define WS(x) x > #endif > > + > +/* Used in the iosb.result field to indicate that the current socket I/O > + * operation is in synchronous non-blocking mode. This value is normally > + * transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) > + * when the server gives the client a chance to complete the I/O synchronously > + * before resuming the request as fully asynchronous I/O or failing it. > + * If the I/O fails with EWOULDBLOCK and the iosb.result field is set to any > + * other value, the client shall request the server to resume the asynchronous > + * operation. > + * > + * The value (ULONG_PTR)-1 (the maximum value of ULONG_PTR) is chosen so that > + * it will be least likely to be confused with "the number of bytes transferred > + * so far." Any I/O operation that has made it to the maximum number of bytes > + * shall complete immediately anyway. > + */ > +#define AFD_WINE_IN_NONBLOCKING_MODE ((ULONG_PTR)-1) > + Why add this instead of returning a separate field from the recv_socket request?

Jan. 25, 2022

Re: [RFC PATCH v2 03/11] include: Add a helper function to test if a status indicates deferred completion.

by Zebediah Figura (she/her)

On 1/22/22 08:36, Jinoh Kang wrote: > Put it in a separate header file so that it won't pollute everyone's > pre-processor macro definition namespace with ntstatus.h. Note that > winnt.h omits STATUS_ALERTED. NTSTATUS values are already defined by all server files; there's no need for a separate header. The NTSTATUS typedef isn't, but we use "unsigned int" anyway.

Jan. 25, 2022

Re: [RFC PATCH v2 02/11] server: Add a helper function to trigger synchronous completion of I/O via APC_ASYNC_IO.

by Zebediah Figura (she/her)

On 1/22/22 08:36, Jinoh Kang wrote: > diff --git a/server/async.c b/server/async.c > index aa6d50cde75..5d0857f3eec 100644 > --- a/server/async.c > +++ b/server/async.c > @@ -383,6 +383,23 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ > return async->wait_handle; > } > > +/* Set up synchronous completion of I/O via the APC_ASYNC_IO system APC. > + * This function is intended to be called immediately before async_handoff(), > + * and expects the last error status to be STATUS_ALERTED. > + * > + * The information argument can be used to pass extra information to the client > + * (e.g. whether the socket is in non-blocking mode). > + */ > +void async_start_sync_io_request( struct async *async, data_size_t information ) > +{ > + assert( get_error() == STATUS_ALERTED ); > + assert( async->thread == current ); > + assert( !async->pending ); > + > + async->direct_result = 0; /* force APC to fire off */ > + async->iosb->result = information; > +} > + > /* complete a request-based async with a pre-allocated buffer */ > void async_request_complete( struct async *async, unsigned int status, data_size_t result, > data_size_t out_size, void *out_data ) Please add helpers only in the patches where they're used; it makes things very difficult to review otherwise.

Jan. 25, 2022

Re: Re: Arbitrary DOS device name

by Marvin

Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=105983 Your paranoid android. === debian11 (build log) === error: corrupt patch at line 77 Task: Patch failed to apply === debian11 (build log) === error: corrupt patch at line 77 Task: Patch failed to apply

Jan. 25, 2022