D3D shader assembler (round 2)
Henri Verbeet
hverbeet at gmail.com
Mon Aug 10 05:28:37 CDT 2009
I only looked at the first patch.
2009/8/9 Matteo Bruni <matteo.mystral at gmail.com>:
> +static void asmparser_instr(struct asm_parser *This, DWORD opcode,
...
> + if(srcs) {
> + for(i=0; i<srcs->nr; i++) {
...
> + if(srcs && (srcs->nr != expectednsrcs)) {
...
> + if(srcs) {
> + for(i=0; i<srcs->nr; i++) {
You can simplify that a bit by doing something like "unsigned int
src_count = srcs ? src->nr : 0;" at the start, and then just using
src_count.
> + rc = vsnprintf(base, MESSAGEBUFFER_SIZE - 1 - ctx->messagesize, fmt, args);
You should include wine/port.h if you use vsnprintf(). It's probably
not a bad idea to just include that in general.
> +typedef struct wine_shader_ {
That's not very nice (both the name and the typedef).
> +static inline LPVOID my_alloc(SIZE_T size) {
You'll want to give that a better name than "my_alloc", although I
wonder if you need a function for this at all.
> +struct src_regs {
> + struct shader_reg reg[MAX_SRC_REGS];
> + int nr;
> +};
I think "nr" can be unsigned. Personally I'd named the variable "count".
> +#define MESSAGEBUFFER_SIZE 4096
> +struct asm_parser {
> + /* The function table of the parser implementation */
> + struct asmparser_backend *funcs;
> +
> + /* Private data follows */
> + wine_shader *shader;
> + unsigned int m3x3pad_count;
> +
> + /* Reentrant lexer pointer */
> + void *yyscanner;
> +
> + enum parse_status{
> + PARSE_SUCCESS = 0,
> + PARSE_WARN = 1,
> + PARSE_ERR = 2
> + } status;
> + char messages[MESSAGEBUFFER_SIZE];
> + unsigned int messagesize;
> + unsigned int line_no;
> +};
A dynamically allocated message buffer wouold probably be better.
> +#define SWIZZLE_ERR -1
...
> + return SWIZZLE_ERR; /* 0 is a valid swizzle */
DWORDs are unsigned, so you should define SWIZZLE_ERR to something
like ~0U. Also, if you allow text2swizzle() to return an error, you
should check for it when calling the function.
> +void add_instruction(wine_shader *shader, struct instruction *instr) {
> + struct instruction **new_instructions;
> +
> + if(shader->instr_alloc_size == 0) {
> + shader->instr = my_alloc(sizeof(*shader->instr));
> + shader->instr_alloc_size = 1;
> + } else if(shader->instr_alloc_size == shader->num_instrs) {
> + shader->instr_alloc_size += max(shader->instr_alloc_size, 1);
> + new_instructions = my_realloc(shader->instr,
> + sizeof(*shader->instr) * (shader->instr_alloc_size));
> + if(!new_instructions) {
> + ERR("Failed to grow the shader instruction array\n");
> + return;
> + }
> + shader->instr = new_instructions;
> + } else if(shader->num_instrs > shader->instr_alloc_size) {
> + ERR("More instructions than allocated. This should not happen\n");
> + }
> +
> + shader->instr[shader->num_instrs] = instr;
> + shader->num_instrs++;
> +}
This function can fail, so the caller needs to handle the failure.
> + if(shader->num_cf) {
> + shader->constF = my_realloc(shader->constF,
> + sizeof(*shader->constF) * (shader->num_cf + 1));
> + } else {
> + shader->constF = my_alloc(sizeof(*shader->constF));
> + }
> +
> + newconst = my_alloc(sizeof(*newconst));
These can fail. Some of the other functions have similar flaws.
> + shader->samplers = my_realloc(shader->samplers,
> + sizeof(*shader->samplers) * (shader->num_samplers + 1));
> + }
> + if(!shader->samplers) {
> + ERR("Out of memory\n");
> + return FALSE;
> + }
If my_realloc() fails, you've now leaked the original shader->samplers memory.
> + /* Allocate 8 DWORDs for the start - even a minimalistic shader will need
> + * some instructions. Allocating it here makes the growing code easier since
> + * it just needs a realloc then
> + */
> + ret->alloc_size = 8;
I think you should aim for an "average" shader rather than a minimal one here.
> + buffer->data = my_realloc(buffer->data,
> + sizeof(DWORD) * buffer->alloc_size);
> + if(!buffer->data) {
> + ERR("Failed to grow the buffer data memory\n");
> + buffer->state = E_OUTOFMEMORY;
> + }
Similar to record_sampler() above, you'll leak the original
buffer->data memory if my_realloc() fails.
More information about the wine-devel
mailing list