[PATCH vkd3d] vkd3d-shader: Allow a NULL buffer in vkd3d_string_buffer.
Semantically, a NULL buffer is considered equivalent to the empty string. The accessor vkd3d_string_buffer_data implements this equivalence. The reason for this change is to avoid asserting the result of a memory allocation in vkd3d_string_buffer_init, while at the same time keeping it always successful. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/glsl.c | 2 +- libs/vkd3d-shader/hlsl.c | 9 ++++--- libs/vkd3d-shader/hlsl.y | 34 +++++++++++++----------- libs/vkd3d-shader/hlsl_codegen.c | 8 +++--- libs/vkd3d-shader/hlsl_sm1.c | 2 +- libs/vkd3d-shader/hlsl_sm4.c | 5 ++-- libs/vkd3d-shader/preproc.l | 7 ++--- libs/vkd3d-shader/spirv.c | 2 +- libs/vkd3d-shader/trace.c | 2 +- libs/vkd3d-shader/vkd3d_shader_main.c | 25 ++++++++++------- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 11 files changed, 57 insertions(+), 40 deletions(-) diff --git a/libs/vkd3d-shader/glsl.c b/libs/vkd3d-shader/glsl.c index 7e028d23..fec412b1 100644 --- a/libs/vkd3d-shader/glsl.c +++ b/libs/vkd3d-shader/glsl.c @@ -127,7 +127,7 @@ int vkd3d_glsl_generator_generate(struct vkd3d_glsl_generator *generator, if ((code = vkd3d_malloc(generator->buffer.buffer_size))) { - memcpy(code, generator->buffer.buffer, generator->buffer.content_size); + memcpy(code, vkd3d_string_buffer_data(&generator->buffer), generator->buffer.content_size); out->size = generator->buffer.content_size; out->code = code; } diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 66ca389a..e64d8808 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -64,7 +64,8 @@ void hlsl_fixme(struct hlsl_ctx *ctx, const struct vkd3d_shader_location loc, co string = hlsl_get_string_buffer(ctx); vkd3d_string_buffer_printf(string, "Aborting due to not yet implemented feature: "); vkd3d_string_buffer_vprintf(string, fmt, args); - vkd3d_shader_error(ctx->message_context, &loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "%s", string->buffer); + vkd3d_shader_error(ctx->message_context, &loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, + "%s", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); va_end(args); @@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); } @@ -881,7 +882,7 @@ const char *debug_hlsl_type(struct hlsl_ctx *ctx, const struct hlsl_type *type) if (!(string = hlsl_type_to_string(ctx, type))) return NULL; - ret = vkd3d_dbg_sprintf("%s", string->buffer); + ret = vkd3d_dbg_sprintf("%s", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); return ret; } @@ -975,7 +976,7 @@ static void dump_ir_var(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffer struct vkd3d_string_buffer *string; if ((string = hlsl_modifiers_to_string(ctx, var->modifiers))) - vkd3d_string_buffer_printf(buffer, "%s ", string->buffer); + vkd3d_string_buffer_printf(buffer, "%s ", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); } vkd3d_string_buffer_printf(buffer, "%s %s", debug_hlsl_type(ctx, var->data_type), var->name); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index d1ef9eb4..4f5e629d 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -274,7 +274,8 @@ static struct hlsl_ir_node *add_implicit_conversion(struct hlsl_ctx *ctx, struct dst_string = hlsl_type_to_string(ctx, dst_type); if (src_string && dst_string) hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Can't implicitly convert from %s to %s.", src_string->buffer, dst_string->buffer); + "Can't implicitly convert from %s to %s.", + vkd3d_string_buffer_data(src_string), vkd3d_string_buffer_data(dst_string)); hlsl_release_string_buffer(ctx, src_string); hlsl_release_string_buffer(ctx, dst_string); return NULL; @@ -298,7 +299,7 @@ static DWORD add_modifiers(struct hlsl_ctx *ctx, DWORD modifiers, DWORD mod, con if ((string = hlsl_modifiers_to_string(ctx, mod))) hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifier '%s' was already specified.", string->buffer); + "Modifier '%s' was already specified.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); return modifiers; } @@ -919,7 +920,8 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type if ((string = hlsl_type_to_string(ctx, t1))) hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Expression of type \"%s\" cannot be used in a numeric expression.", string->buffer); + "Expression of type \"%s\" cannot be used in a numeric expression.", + vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); return NULL; } @@ -930,7 +932,8 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type if ((string = hlsl_type_to_string(ctx, t2))) hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Expression of type \"%s\" cannot be used in a numeric expression.", string->buffer); + "Expression of type \"%s\" cannot be used in a numeric expression.", + vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); return NULL; } @@ -946,7 +949,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type if (t1_string && t2_string) hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Expression data types \"%s\" and \"%s\" are incompatible.", - t1_string->buffer, t2_string->buffer); + vkd3d_string_buffer_data(t1_string), vkd3d_string_buffer_data(t2_string)); hlsl_release_string_buffer(ctx, t1_string); hlsl_release_string_buffer(ctx, t2_string); return NULL; @@ -1410,7 +1413,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if ((string = hlsl_modifiers_to_string(ctx, modifiers & invalid))) hlsl_error(ctx, var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers '%s' are not allowed on local variables.", string->buffer); + "Modifiers '%s' are not allowed on local variables.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); } if (var->semantic.name) @@ -1673,7 +1676,7 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name, if ((string = hlsl_type_to_string(ctx, params->args[i]->data_type))) hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Wrong type for argument %u of '%s': expected a numeric type, but got '%s'.\n", - i + 1, name, string->buffer); + i + 1, name, vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); free_parse_initializer(params); return NULL; @@ -1725,7 +1728,7 @@ static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type if ((string = hlsl_type_to_string(ctx, arg->data_type))) hlsl_error(ctx, arg->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Invalid type %s for constructor argument.", string->buffer); + "Invalid type %s for constructor argument.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); continue; } @@ -2171,7 +2174,8 @@ field: if ((string = hlsl_modifiers_to_string(ctx, modifiers))) hlsl_error(ctx, @1, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifiers '%s' are not allowed on struct fields.", string->buffer); + "Modifiers '%s' are not allowed on struct fields.", + vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); } $$ = gen_struct_fields(ctx, type, $3); @@ -2355,7 +2359,7 @@ input_mods: if ((string = hlsl_modifiers_to_string(ctx, $2))) hlsl_error(ctx, @2, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, - "Modifier \"%s\" was already specified.", string->buffer); + "Modifier \"%s\" was already specified.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT; } @@ -2386,7 +2390,7 @@ type: string = hlsl_type_to_string(ctx, $3); if (string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Vector base type %s is not scalar.", string->buffer); + "Vector base type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT; } @@ -2408,7 +2412,7 @@ type: string = hlsl_type_to_string(ctx, $3); if (string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Matrix base type %s is not scalar.", string->buffer); + "Matrix base type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT; } @@ -2779,7 +2783,7 @@ selection_statement: if ((string = hlsl_type_to_string(ctx, condition->data_type))) hlsl_error(ctx, instr->node.loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "if condition type %s is not scalar.", string->buffer); + "if condition type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); } $$ = $3; @@ -3022,7 +3026,7 @@ postfix_expr: if ((string = hlsl_type_to_string(ctx, $2))) hlsl_error(ctx, @2, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Constructor data type %s is not numeric.", string->buffer); + "Constructor data type %s is not numeric.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); free_parse_initializer(&$4); YYABORT; @@ -3106,7 +3110,7 @@ unary_expr: dst_string = hlsl_type_to_string(ctx, dst_type); if (src_string && dst_string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Can't cast from %s to %s.", - src_string->buffer, dst_string->buffer); + vkd3d_string_buffer_data(src_string), vkd3d_string_buffer_data(dst_string)); hlsl_release_string_buffer(ctx, src_string); hlsl_release_string_buffer(ctx, dst_string); YYABORT; diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 9c9a3837..2240f489 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -45,7 +45,7 @@ static void prepend_uniform_copy(struct hlsl_ctx *ctx, struct list *instrs, stru if (!(name = hlsl_get_string_buffer(ctx))) return; vkd3d_string_buffer_printf(name, "<temp-%s>", temp->name); - temp->name = hlsl_strdup(ctx, name->buffer); + temp->name = hlsl_strdup(ctx, vkd3d_string_buffer_data(name)); hlsl_release_string_buffer(ctx, name); if (!(load = hlsl_new_var_load(ctx, uniform, temp->loc))) @@ -76,7 +76,8 @@ static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct return; } new_semantic.index = semantic->index; - if (!(input = hlsl_new_var(ctx, hlsl_strdup(ctx, name->buffer), type, var->loc, &new_semantic, 0, NULL))) + if (!(input = hlsl_new_var(ctx, hlsl_strdup(ctx, vkd3d_string_buffer_data(name)), + type, var->loc, &new_semantic, 0, NULL))) { hlsl_release_string_buffer(ctx, name); vkd3d_free((void *)new_semantic.name); @@ -147,7 +148,8 @@ static void append_output_copy(struct hlsl_ctx *ctx, struct list *instrs, struct return; } new_semantic.index = semantic->index; - if (!(output = hlsl_new_var(ctx, hlsl_strdup(ctx, name->buffer), type, var->loc, &new_semantic, 0, NULL))) + if (!(output = hlsl_new_var(ctx, hlsl_strdup(ctx, vkd3d_string_buffer_data(name)), + type, var->loc, &new_semantic, 0, NULL))) { vkd3d_free((void *)new_semantic.name); hlsl_release_string_buffer(ctx, name); diff --git a/libs/vkd3d-shader/hlsl_sm1.c b/libs/vkd3d-shader/hlsl_sm1.c index ee2870ec..e64ca5f2 100644 --- a/libs/vkd3d-shader/hlsl_sm1.c +++ b/libs/vkd3d-shader/hlsl_sm1.c @@ -326,7 +326,7 @@ static void write_sm1_uniforms(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffe } vkd3d_string_buffer_printf(name, "$%s", var->name); vkd3d_free((char *)var->name); - var->name = hlsl_strdup(ctx, name->buffer); + var->name = hlsl_strdup(ctx, vkd3d_string_buffer_data(name)); hlsl_release_string_buffer(ctx, name); } } diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index f7249431..05612806 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -187,7 +187,8 @@ static void write_sm4_signature(struct hlsl_ctx *ctx, struct dxbc_writer *dxbc, default: if ((string = hlsl_type_to_string(ctx, var->data_type))) hlsl_error(ctx, var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, - "Invalid data type %s for semantic variable %s.", string->buffer, var->name); + "Invalid data type %s for semantic variable %s.", + vkd3d_string_buffer_data(string), var->name); hlsl_release_string_buffer(ctx, string); put_u32(&buffer, D3D_REGISTER_COMPONENT_UNKNOWN); } @@ -1165,7 +1166,7 @@ static void write_sm4_expr(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *string; if ((string = hlsl_type_to_string(ctx, expr->node.data_type))) - hlsl_fixme(ctx, expr->node.loc, "SM4 %s expression.", string->buffer); + hlsl_fixme(ctx, expr->node.loc, "SM4 %s expression.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); break; } diff --git a/libs/vkd3d-shader/preproc.l b/libs/vkd3d-shader/preproc.l index bf4d669f..75b8e358 100644 --- a/libs/vkd3d-shader/preproc.l +++ b/libs/vkd3d-shader/preproc.l @@ -321,7 +321,8 @@ static bool preproc_push_expansion(struct preproc_ctx *ctx, return false; exp = &ctx->expansion_stack[ctx->expansion_count++]; exp->text = text; - exp->buffer.lexer_buffer = yy_scan_bytes(text->text.buffer, text->text.content_size, ctx->scanner); + exp->buffer.lexer_buffer = yy_scan_bytes(vkd3d_string_buffer_data(&text->text), + text->text.content_size, ctx->scanner); exp->buffer.location = text->location; exp->macro = macro; TRACE("Expansion stack size is now %zu.\n", ctx->expansion_count); @@ -465,7 +466,7 @@ int yylex(YYSTYPE *lval, YYLTYPE *lloc, yyscan_t scanner) { for (i = 0; i < expansion->text.content_size; ++i) { - char c = expansion->text.buffer[i]; + char c = vkd3d_string_buffer_data(&expansion->text)[i]; if (c == '\\' || c == '"') vkd3d_string_buffer_printf(&ctx->buffer, "\\"); @@ -804,7 +805,7 @@ int preproc_lexer_parse(const struct vkd3d_shader_compile_info *compile_info, vkd3d_string_buffer_cleanup(&ctx.buffer); return VKD3D_ERROR_OUT_OF_MEMORY; } - memcpy(output_code, ctx.buffer.buffer, ctx.buffer.content_size); + memcpy(output_code, vkd3d_string_buffer_data(&ctx.buffer), ctx.buffer.content_size); out->size = ctx.buffer.content_size; out->code = output_code; vkd3d_string_buffer_trace(&ctx.buffer); diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index f604b8ef..1894e140 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2632,7 +2632,7 @@ static struct vkd3d_shader_descriptor_binding vkd3d_dxbc_compiler_get_descriptor if (shader_interface->binding_count) { struct vkd3d_string_buffer *buffer = vkd3d_shader_register_range_string(compiler, range); - const char *range_str = buffer ? buffer->buffer : ""; + const char *range_str = buffer ? vkd3d_string_buffer_data(buffer) : ""; FIXME("Could not find descriptor binding for type %#x, space %u, registers %s, shader type %#x.\n", descriptor_type, range->space, range_str, compiler->shader_type); vkd3d_dxbc_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_BINDING_NOT_FOUND, diff --git a/libs/vkd3d-shader/trace.c b/libs/vkd3d-shader/trace.c index 45f38d13..340a4968 100644 --- a/libs/vkd3d-shader/trace.c +++ b/libs/vkd3d-shader/trace.c @@ -1938,7 +1938,7 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(struct vkd3d_shader_parser *parser, if ((code = vkd3d_malloc(buffer->content_size))) { - memcpy(code, buffer->buffer, buffer->content_size); + memcpy(code, vkd3d_string_buffer_data(buffer), buffer->content_size); out->size = buffer->content_size; out->code = code; } diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index ef9eaef5..8a2dd0e3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -25,11 +25,9 @@ VKD3D_DEBUG_ENV_NAME("VKD3D_SHADER_DEBUG"); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer) { - buffer->buffer_size = 16; + buffer->buffer_size = 0; buffer->content_size = 0; - buffer->buffer = vkd3d_malloc(buffer->buffer_size); - assert(buffer->buffer); - memset(buffer->buffer, 0, buffer->buffer_size); + buffer->buffer = NULL; } void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) @@ -37,20 +35,29 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) vkd3d_free(buffer->buffer); } +const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) +{ + return buffer->buffer ? buffer->buffer : ""; +} + static void vkd3d_string_buffer_clear(struct vkd3d_string_buffer *buffer) { - buffer->buffer[0] = '\0'; - buffer->content_size = 0; + if (buffer->buffer) + { + buffer->buffer[0] = '\0'; + buffer->content_size = 0; + } } static bool vkd3d_string_buffer_resize(struct vkd3d_string_buffer *buffer, int rc) { - unsigned int new_buffer_size = rc >= 0 ? buffer->content_size + rc + 1 : buffer->buffer_size * 2; + unsigned int new_buffer_size = rc >= 0 ? buffer->content_size + rc + 1 : max(32, buffer->buffer_size * 2); if (!vkd3d_array_reserve((void **)&buffer->buffer, &buffer->buffer_size, new_buffer_size, 1)) { ERR("Failed to grow buffer.\n"); - buffer->buffer[buffer->content_size] = '\0'; + if (buffer->buffer) + buffer->buffer[buffer->content_size] = '\0'; return false; } return true; @@ -194,7 +201,7 @@ bool vkd3d_shader_message_context_copy_messages(struct vkd3d_shader_message_cont if (!(messages = vkd3d_malloc(context->messages.content_size + 1))) return false; - memcpy(messages, context->messages.buffer, context->messages.content_size + 1); + memcpy(messages, vkd3d_string_buffer_data(&context->messages), context->messages.content_size + 1); *out = messages; return true; } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index fa56ba1d..0bad2aa3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -926,6 +926,7 @@ enum vkd3d_result vkd3d_dxbc_binary_to_text(struct vkd3d_shader_parser *parser, void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer); struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_cache *list); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer); +const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer); void vkd3d_string_buffer_cache_cleanup(struct vkd3d_string_buffer_cache *list); void vkd3d_string_buffer_cache_init(struct vkd3d_string_buffer_cache *list); int vkd3d_string_buffer_printf(struct vkd3d_string_buffer *buffer, const char *format, ...) VKD3D_PRINTF_FUNC(2, 3); -- 2.33.0
On 10/4/21 3:32 AM, Giovanni Mascellani wrote:
Semantically, a NULL buffer is considered equivalent to the empty string. The accessor vkd3d_string_buffer_data implements this equivalence.
The reason for this change is to avoid asserting the result of a memory allocation in vkd3d_string_buffer_init, while at the same time keeping it always successful.
Well, I guess this is one alternative to 216057. I don't dislike it.
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru
if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index ef9eaef5..8a2dd0e3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -25,11 +25,9 @@ VKD3D_DEBUG_ENV_NAME("VKD3D_SHADER_DEBUG");
void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer) { - buffer->buffer_size = 16; + buffer->buffer_size = 0; buffer->content_size = 0; - buffer->buffer = vkd3d_malloc(buffer->buffer_size); - assert(buffer->buffer); - memset(buffer->buffer, 0, buffer->buffer_size); + buffer->buffer = NULL; }
I'm assuming this was accidental.
void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) @@ -37,20 +35,29 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) vkd3d_free(buffer->buffer); }
+const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) +{ + return buffer->buffer ? buffer->buffer : ""; +} +
Should this be defined in vkd3d_shader_private.h as an inline function?
Hi, Il 04/10/21 19:03, Zebediah Figura ha scritto:
On 10/4/21 3:32 AM, Giovanni Mascellani wrote:
Semantically, a NULL buffer is considered equivalent to the empty string. The accessor vkd3d_string_buffer_data implements this equivalence.
The reason for this change is to avoid asserting the result of a memory allocation in vkd3d_string_buffer_init, while at the same time keeping it always successful.
Well, I guess this is one alternative to 216057. I don't dislike it.
Mmmh, I'm not really sure why you see it as such, but it was not meant to. It's true that it conflicts with it (sorry for that, when I wrote and submitted it I hadn't yet seen your patch set), but the intention is another, namely remove the assert(buffer->buffer) in Matteo's patch. This has nothing to do with how to store type names, which you address in your patch.
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
I'm pretty sure that in many cases vkd3d_string_buffer_data could be avoided, when it can be shown that the buffer cannot be NULL. Though ideally I'd like to encapsulate as much as possible the fact that the buffer can be NULL inside the accessor vkd3d_string_buffer_data, so I am using the accessor nearly everywhere (a couple of places were actually using knowledge of the internals of vkd3d_string_buffer, and I didn't change that, because after all we're writing C and nobody really expects full encapsulation).
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index ef9eaef5..8a2dd0e3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -25,11 +25,9 @@ VKD3D_DEBUG_ENV_NAME("VKD3D_SHADER_DEBUG"); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer) { - buffer->buffer_size = 16; + buffer->buffer_size = 0; buffer->content_size = 0; - buffer->buffer = vkd3d_malloc(buffer->buffer_size); - assert(buffer->buffer); - memset(buffer->buffer, 0, buffer->buffer_size); + buffer->buffer = NULL; }
I'm assuming this was accidental.
That was actually the point of the whole patch.
void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) @@ -37,20 +35,29 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) vkd3d_free(buffer->buffer); } +const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) +{ + return buffer->buffer ? buffer->buffer : ""; +} +
Should this be defined in vkd3d_shader_private.h as an inline function?
Good point about that. Thanks, Giovanni.
On 10/5/21 06:57, Giovanni Mascellani wrote:
Hi,
Il 04/10/21 19:03, Zebediah Figura ha scritto:
On 10/4/21 3:32 AM, Giovanni Mascellani wrote:
Semantically, a NULL buffer is considered equivalent to the empty string. The accessor vkd3d_string_buffer_data implements this equivalence.
The reason for this change is to avoid asserting the result of a memory allocation in vkd3d_string_buffer_init, while at the same time keeping it always successful.
Well, I guess this is one alternative to 216057. I don't dislike it.
Mmmh, I'm not really sure why you see it as such, but it was not meant to. It's true that it conflicts with it (sorry for that, when I wrote and submitted it I hadn't yet seen your patch set), but the intention is another, namely remove the assert(buffer->buffer) in Matteo's patch. This has nothing to do with how to store type names, which you address in your patch.
Well, in the sense that 216057 (well, 216055 really) is mainly just there to get rid of the boilerplate around calling hlsl_type_to_string(). This patch does lesson that boilerplate a bit.
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
I'm pretty sure that in many cases vkd3d_string_buffer_data could be avoided, when it can be shown that the buffer cannot be NULL. Though ideally I'd like to encapsulate as much as possible the fact that the buffer can be NULL inside the accessor vkd3d_string_buffer_data, so I am using the accessor nearly everywhere (a couple of places were actually using knowledge of the internals of vkd3d_string_buffer, and I didn't change that, because after all we're writing C and nobody really expects full encapsulation).
I guess you're looking at this with the perspective that it's just there to avoid asserting the result of an allocation. That's fine, but I think that it's useful to avoid checking the result of one as well. Sort of like put_u32() and friends.
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index ef9eaef5..8a2dd0e3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -25,11 +25,9 @@ VKD3D_DEBUG_ENV_NAME("VKD3D_SHADER_DEBUG"); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer) { - buffer->buffer_size = 16; + buffer->buffer_size = 0; buffer->content_size = 0; - buffer->buffer = vkd3d_malloc(buffer->buffer_size); - assert(buffer->buffer); - memset(buffer->buffer, 0, buffer->buffer_size); + buffer->buffer = NULL; }
I'm assuming this was accidental.
That was actually the point of the whole patch.
Oh, I see now. Right, I that the whole point of preallocating the buffer was so that it was never NULL.
void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) @@ -37,20 +35,29 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) vkd3d_free(buffer->buffer); } +const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) +{ + return buffer->buffer ? buffer->buffer : ""; +} +
Should this be defined in vkd3d_shader_private.h as an inline function?
Good point about that.
Thanks, Giovanni.
Hi, Il 05/10/21 18:21, Zebediah Figura (she/her) ha scritto:
Well, in the sense that 216057 (well, 216055 really) is mainly just there to get rid of the boilerplate around calling hlsl_type_to_string(). This patch does lesson that boilerplate a bit.
Mmh, I am not sure I understand why that would lessen the boilerplate. You still need to name and destroy the string buffer, each of which takes a line. And vkd3d_string_buffer_data(string) is definitely longer than string->buffer. I don't really think this helps here.
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
I'm pretty sure that in many cases vkd3d_string_buffer_data could be avoided, when it can be shown that the buffer cannot be NULL. Though ideally I'd like to encapsulate as much as possible the fact that the buffer can be NULL inside the accessor vkd3d_string_buffer_data, so I am using the accessor nearly everywhere (a couple of places were actually using knowledge of the internals of vkd3d_string_buffer, and I didn't change that, because after all we're writing C and nobody really expects full encapsulation).
I guess you're looking at this with the perspective that it's just there to avoid asserting the result of an allocation. That's fine, but I think that it's useful to avoid checking the result of one as well. Sort of like put_u32() and friends.
Sorry, I don't think I'm following you. Giovanni.
On 10/6/21 07:48, Giovanni Mascellani wrote:
Hi,
Il 05/10/21 18:21, Zebediah Figura (she/her) ha scritto:
Well, in the sense that 216057 (well, 216055 really) is mainly just there to get rid of the boilerplate around calling hlsl_type_to_string(). This patch does lesson that boilerplate a bit.
Mmh, I am not sure I understand why that would lessen the boilerplate. You still need to name and destroy the string buffer, each of which takes a line. And vkd3d_string_buffer_data(string) is definitely longer than string->buffer. I don't really think this helps here.
Well, it lessens the boilerplate if you no longer need to check for success from hlsl_type_to_string().
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
I'm pretty sure that in many cases vkd3d_string_buffer_data could be avoided, when it can be shown that the buffer cannot be NULL. Though ideally I'd like to encapsulate as much as possible the fact that the buffer can be NULL inside the accessor vkd3d_string_buffer_data, so I am using the accessor nearly everywhere (a couple of places were actually using knowledge of the internals of vkd3d_string_buffer, and I didn't change that, because after all we're writing C and nobody really expects full encapsulation).
I guess you're looking at this with the perspective that it's just there to avoid asserting the result of an allocation. That's fine, but I think that it's useful to avoid checking the result of one as well. Sort of like put_u32() and friends.
Sorry, I don't think I'm following you.
I mean, it's very convenient to be able to not have to check for success from an allocation, in some way or another, if you're going to be doing that frequently. vkd3d_string_buffer_data() achieves that, so I think it's useful to see that as an end goal of vkd3d_string_buffer_data() in itself, and to take advantage of it to get rid of ubiquitous error checking.
Hi, Il 06/10/21 16:54, Zebediah Figura (she/her) ha scritto:
Mmh, I am not sure I understand why that would lessen the boilerplate. You still need to name and destroy the string buffer, each of which takes a line. And vkd3d_string_buffer_data(string) is definitely longer than string->buffer. I don't really think this helps here.
Well, it lessens the boilerplate if you no longer need to check for success from hlsl_type_to_string().
In this form, you still have to check for the result of hlsl_type_to_string. My patch doesn't change the fact that hlsl_type_to_string can still return NULL if it cannot allocate the vkd3d_string_buffer as a whole. Notice that my patch deals with the buffer field of vkd3d_string_buffer being NULL, but the whole structure still needs to be correctly allocated. (en passant, I also notice that hlsl_type_to_string has insufficient error checking: it doesn't check the return value of vkd3d_string_buffer_prints, so if allocations fails there it will return an empty string; this doesn't lead to UB, but it generates a bad error message, which is still a bug, although much smaller)
I mean, it's very convenient to be able to not have to check for success from an allocation, in some way or another, if you're going to be doing that frequently. vkd3d_string_buffer_data() achieves that, so I think it's useful to see that as an end goal of vkd3d_string_buffer_data() in itself, and to take advantage of it to get rid of ubiquitous error checking.
Well, I guess this can be added as an additional goal, but we're not there yet. Taking this example of the error boilerplate:
struct vkd3d_string_buffer *string;
string = hlsl_type_to_string(ctx, $3); if (string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Matrix base type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT;
You cannot remove the "if (string)", because hlsl_type_to_string can still return NULL, and vkd3d_string_buffer_data wouldn't like it (no problems if string->buffer is NULL, but segfault if string itself is NULL). It's true that vkd3d_string_buffer_data might be replaced with something like:
static inline const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) { return buffer && buffer->buffer ? buffer->buffer : ""; }
In this case it would work as you say, though it would risk to generate a bad error message: "Matrix base type is not scalar.". As I said, less grave than UB, but still a bug. If you want to really achieve what you say, it seems to me that more hack^W engineering is required (and I am not against it). Giovanni.
On 10/7/21 02:32, Giovanni Mascellani wrote:
Hi,
Il 06/10/21 16:54, Zebediah Figura (she/her) ha scritto:
Mmh, I am not sure I understand why that would lessen the boilerplate. You still need to name and destroy the string buffer, each of which takes a line. And vkd3d_string_buffer_data(string) is definitely longer than string->buffer. I don't really think this helps here.
Well, it lessens the boilerplate if you no longer need to check for success from hlsl_type_to_string().
In this form, you still have to check for the result of hlsl_type_to_string. My patch doesn't change the fact that hlsl_type_to_string can still return NULL if it cannot allocate the vkd3d_string_buffer as a whole. Notice that my patch deals with the buffer field of vkd3d_string_buffer being NULL, but the whole structure still needs to be correctly allocated.
Indeed, I misread it. Could we check the string_buffer itself for NULL as well, then, like you describe below? Maybe in a separate patch...
(en passant, I also notice that hlsl_type_to_string has insufficient error checking: it doesn't check the return value of vkd3d_string_buffer_prints, so if allocations fails there it will return an empty string; this doesn't lead to UB, but it generates a bad error message, which is still a bug, although much smaller)
I think that's inescapable; if we run out of memory we are going to do *something* wrong. Likely we can't even print the message in the first place.
I mean, it's very convenient to be able to not have to check for success from an allocation, in some way or another, if you're going to be doing that frequently. vkd3d_string_buffer_data() achieves that, so I think it's useful to see that as an end goal of vkd3d_string_buffer_data() in itself, and to take advantage of it to get rid of ubiquitous error checking.
Well, I guess this can be added as an additional goal, but we're not there yet. Taking this example of the error boilerplate:
struct vkd3d_string_buffer *string;
string = hlsl_type_to_string(ctx, $3); if (string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Matrix base type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT;
You cannot remove the "if (string)", because hlsl_type_to_string can still return NULL, and vkd3d_string_buffer_data wouldn't like it (no problems if string->buffer is NULL, but segfault if string itself is NULL).
It's true that vkd3d_string_buffer_data might be replaced with something like:
static inline const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) { return buffer && buffer->buffer ? buffer->buffer : ""; }
In this case it would work as you say, though it would risk to generate a bad error message: "Matrix base type is not scalar.". As I said, less grave than UB, but still a bug. If you want to really achieve what you say, it seems to me that more hack^W engineering is required (and I am not against it).
Giovanni.
participants (3)
-
Giovanni Mascellani -
Zebediah Figura -
Zebediah Figura (she/her)