This allows to specify an error message (though, given that the code is supposed to be unreachable, I didn't bother with a full printf-style facility) and, given that it is marked as "noreturn", it should silence warnings about some branches not returning without requiring some additional "return 0" statement or similar.
With this patch, no warnings are generated in my environment ("gcc (Debian 11.2.0-19) 11.2.0").
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com --- include/private/vkd3d_common.h | 8 ++++++ libs/vkd3d-shader/hlsl.c | 2 +- libs/vkd3d-shader/hlsl.h | 3 +-- libs/vkd3d-shader/hlsl.y | 9 +++---- libs/vkd3d-shader/hlsl_codegen.c | 3 +-- libs/vkd3d-shader/hlsl_constant_ops.c | 13 +++------ libs/vkd3d-shader/hlsl_sm1.c | 9 +++---- libs/vkd3d-shader/hlsl_sm4.c | 38 ++++++++++----------------- libs/vkd3d-shader/vkd3d_shader_main.c | 2 +- 9 files changed, 38 insertions(+), 49 deletions(-)
diff --git a/include/private/vkd3d_common.h b/include/private/vkd3d_common.h index 11acc643..56382a8b 100644 --- a/include/private/vkd3d_common.h +++ b/include/private/vkd3d_common.h @@ -27,6 +27,7 @@ #include <limits.h> #include <stdbool.h> #include <stdint.h> +#include <stdio.h>
#ifdef _MSC_VER #include <intrin.h> @@ -61,6 +62,13 @@ static inline size_t align(size_t addr, size_t alignment) # define VKD3D_UNUSED #endif /* __GNUC__ */
+#define vkd3d_unreachable(msg) vkd3d_unreachable_(__FILE__, __LINE__, msg) +VKD3D_NORETURN static inline void vkd3d_unreachable_(const char *filename, unsigned int line, const char *msg) +{ + fprintf(stderr, "%s:%u: Aborting, unreachable code reached%s%s.\n", filename, line, msg ? ": " : "", msg ? msg : ""); + abort(); +} + static inline unsigned int vkd3d_popcount(unsigned int v) { #ifdef _MSC_VER diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index eabe189f..1ebd97c1 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1188,7 +1188,7 @@ static void dump_ir_constant(struct vkd3d_string_buffer *buffer, const struct hl break;
default: - assert(0); + vkd3d_unreachable("Unknown type"); } } if (type->dimx != 1) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 802adf87..2ccc3640 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -692,8 +692,7 @@ static inline unsigned int hlsl_sampler_dim_count(enum hlsl_sampler_dim dim) case HLSL_SAMPLER_DIM_CUBEARRAY: return 4; default: - assert(0); - return 0; + vkd3d_unreachable("Unknown sampler type"); } }
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 1eb7d2ee..437f26c0 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -869,8 +869,7 @@ static unsigned int evaluate_array_dimension(struct hlsl_ir_node *node) case HLSL_TYPE_BOOL: return !!value->u; default: - assert(0); - return 0; + vkd3d_unreachable("Unknown type"); } }
@@ -887,10 +886,10 @@ static unsigned int evaluate_array_dimension(struct hlsl_ir_node *node) case HLSL_IR_STORE: WARN("Invalid node type %s.\n", hlsl_node_type_to_string(node->type)); return 0; - }
- assert(0); - return 0; + default: + vkd3d_unreachable("Unknown node type"); + } }
static bool expr_compatible_data_types(struct hlsl_type *t1, struct hlsl_type *t2) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 72c00430..1a2193b9 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1257,8 +1257,7 @@ static void allocate_const_registers_recurse(struct hlsl_ctx *ctx, struct hlsl_b return;
default: - assert(0); - return; + vkd3d_unreachable("Unknown type"); } defs->values[constant->reg.id + y].f[x] = f; } diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 5cac4bde..84599563 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -77,8 +77,7 @@ static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_ir_constant *dst, struct break;
default: - assert(0); - return false; + vkd3d_unreachable("Unknown type"); }
switch (dst->node.data_type->base_type) @@ -101,13 +100,10 @@ static bool fold_cast(struct hlsl_ctx *ctx, struct hlsl_ir_constant *dst, struct break;
case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
default: - assert(0); - return false; + vkd3d_unreachable("Unknown type"); } } return true; @@ -246,8 +242,7 @@ static bool fold_nequal(struct hlsl_ctx *ctx, struct hlsl_ir_constant *dst, break;
default: - assert(0); - return false; + vkd3d_unreachable("Unknown type"); }
dst->value[k].u *= ~0u; diff --git a/libs/vkd3d-shader/hlsl_sm1.c b/libs/vkd3d-shader/hlsl_sm1.c index 0cdd3917..67a4345f 100644 --- a/libs/vkd3d-shader/hlsl_sm1.c +++ b/libs/vkd3d-shader/hlsl_sm1.c @@ -159,8 +159,7 @@ static D3DXPARAMETER_CLASS sm1_class(const struct hlsl_type *type) return D3DXPC_VECTOR; default: ERR("Invalid class %#x.\n", type->type); - assert(0); - return 0; + vkd3d_unreachable("Unknown class"); } }
@@ -193,6 +192,7 @@ static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) return D3DXPT_SAMPLER; default: ERR("Invalid dimension %#x.\n", type->sampler_dim); + vkd3d_unreachable("Unknown dimension"); } break; case HLSL_TYPE_STRING: @@ -212,6 +212,7 @@ static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) return D3DXPT_TEXTURE; default: ERR("Invalid dimension %#x.\n", type->sampler_dim); + vkd3d_unreachable("Unknown dimension"); } break; case HLSL_TYPE_VERTEXSHADER: @@ -219,10 +220,8 @@ static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) case HLSL_TYPE_VOID: return D3DXPT_VOID; default: - assert(0); + vkd3d_unreachable("Unknown type"); } - assert(0); - return 0; }
static const struct hlsl_type *get_array_type(const struct hlsl_type *type) diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index 76f08339..41110a99 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -279,8 +279,7 @@ static D3D_SHADER_VARIABLE_CLASS sm4_class(const struct hlsl_type *type) return D3D_SVC_VECTOR; default: ERR("Invalid class %#x.\n", type->type); - assert(0); - return 0; + vkd3d_unreachable("Unknown class"); } }
@@ -313,7 +312,7 @@ static D3D_SHADER_VARIABLE_TYPE sm4_base_type(const struct hlsl_type *type) case HLSL_SAMPLER_DIM_GENERIC: return D3D_SVT_SAMPLER; default: - assert(0); + vkd3d_unreachable("Unknown dimension"); } break; case HLSL_TYPE_STRING: @@ -332,7 +331,7 @@ static D3D_SHADER_VARIABLE_TYPE sm4_base_type(const struct hlsl_type *type) case HLSL_SAMPLER_DIM_GENERIC: return D3D_SVT_TEXTURE; default: - assert(0); + vkd3d_unreachable("Unknown dimension"); } break; case HLSL_TYPE_UINT: @@ -342,10 +341,8 @@ static D3D_SHADER_VARIABLE_TYPE sm4_base_type(const struct hlsl_type *type) case HLSL_TYPE_VOID: return D3D_SVT_VOID; default: - assert(0); + vkd3d_unreachable("Unknown type"); } - assert(0); - return 0; }
static void write_sm4_type(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *buffer, struct hlsl_type *type) @@ -409,8 +406,7 @@ static D3D_SHADER_INPUT_TYPE sm4_resource_type(const struct hlsl_type *type) case HLSL_TYPE_TEXTURE: return D3D_SIT_TEXTURE; default: - assert(0); - return 0; + vkd3d_unreachable("Unknown type"); } }
@@ -434,8 +430,7 @@ static D3D_RESOURCE_RETURN_TYPE sm4_resource_format(const struct hlsl_type *type return D3D_RETURN_TYPE_UINT;
default: - assert(0); - return 0; + vkd3d_unreachable("Unknown type"); } }
@@ -462,8 +457,7 @@ static D3D_SRV_DIMENSION sm4_rdef_resource_dimension(const struct hlsl_type *typ case HLSL_SAMPLER_DIM_CUBEARRAY: return D3D_SRV_DIMENSION_TEXTURECUBEARRAY; default: - assert(0); - return D3D_SRV_DIMENSION_UNKNOWN; + vkd3d_unreachable("Unknown dimension"); } }
@@ -759,8 +753,7 @@ static enum vkd3d_sm4_resource_type sm4_resource_dimension(const struct hlsl_typ case HLSL_SAMPLER_DIM_CUBEARRAY: return VKD3D_SM4_RESOURCE_TEXTURE_CUBEARRAY; default: - assert(0); - return 0; + vkd3d_unreachable("Unknown dimension"); } }
@@ -795,8 +788,7 @@ static uint32_t sm4_encode_instruction_modifier(const struct sm4_instruction_mod break;
default: - assert(0); - break; + vkd3d_unreachable("Unknown modifier"); }
return word; @@ -1477,7 +1469,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break;
default: - assert(0); + vkd3d_unreachable("Unknown type"); } break;
@@ -1503,7 +1495,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break;
default: - assert(0); + vkd3d_unreachable("Unknown type"); } break;
@@ -1529,7 +1521,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break;
default: - assert(0); + vkd3d_unreachable("Unknown type"); } break;
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break;
case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
default: - assert(0); + vkd3d_unreachable("Unknown type"); } }
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 62baf17f..17e9ea98 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -1282,7 +1282,7 @@ int vkd3d_shader_compile(const struct vkd3d_shader_compile_info *compile_info, break;
default: - assert(0); + vkd3d_unreachable("Unknown source type"); }
vkd3d_shader_message_context_trace_messages(&message_context);
On 4/8/22 08:04, Giovanni Mascellani wrote:
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index eabe189f..1ebd97c1 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1188,7 +1188,7 @@ static void dump_ir_constant(struct vkd3d_string_buffer *buffer, const struct hl break;
default:
assert(0);
vkd3d_unreachable("Unknown type"); } } if (type->dimx != 1)
I don't think there's value in specifying a message for cases like this, and I'd rather not have to do so in new code. This applies to pretty much anywhere we have an assert() in a switch default case.
(Ideally we should avoid the 'default' keyword where possible, although in some cases this is difficult.)
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break;
case HLSL_TYPE_BOOL:
/* Casts to bool should have already been lowered. */assert(0);break;
vkd3d_unreachable("Casts to bool should have already been lowered");
This one I think should remain an assert. I can more easily see this getting triggered due to some forgotten implicit agreement. Hence we probably shouldn't tell the compiler this code path is unreachable.
Using an assert(!"string") or assert(0 && "string") construction (or defining our own macro) seems reasonable to me, though.
Hi,
Il 08/04/22 19:00, Zebediah Figura ha scritto:
I don't think there's value in specifying a message for cases like this, and I'd rather not have to do so in new code. This applies to pretty much anywhere we have an assert() in a switch default case.
(Ideally we should avoid the 'default' keyword where possible, although in some cases this is difficult.)
vkd3d_unreachable() is written in a way such that you can pass it NULL to it for precisely this reason. I went adding messages for all present uses, but it is not mandatory to have one. Does this address your objection?
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break; case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
This one I think should remain an assert. I can more easily see this getting triggered due to some forgotten implicit agreement. Hence we probably shouldn't tell the compiler this code path is unreachable.
Using an assert(!"string") or assert(0 && "string") construction (or defining our own macro) seems reasonable to me, though.
From the point of view of the compiler, assert(0) is not really different from vkd3d_unreachable(NULL): nothing tells the compiler that control will never ever reach it; if it happens, the compiler will print a message and abort(). The advantage of vkd3d_unreachable() is that the compiler won't try to tell you "Hey, you didn't write your return statement!" in places where it doesn't make any sense (because it's marked "noreturn").
Thanks, Giovanni.
On Thu, 14 Apr 2022 at 13:00, Giovanni Mascellani gmascellani@codeweavers.com wrote:
From the point of view of the compiler, assert(0) is not really different from vkd3d_unreachable(NULL): nothing tells the compiler that control will never ever reach it; if it happens, the compiler will print a message and abort(). The advantage of vkd3d_unreachable() is that the compiler won't try to tell you "Hey, you didn't write your return statement!" in places where it doesn't make any sense (because it's marked "noreturn").
For what it's worth, note that assert() is effectively compiled out when NDEBUG is defined.
On 4/14/22 06:00, Giovanni Mascellani wrote:
Hi,
Il 08/04/22 19:00, Zebediah Figura ha scritto:
I don't think there's value in specifying a message for cases like this, and I'd rather not have to do so in new code. This applies to pretty much anywhere we have an assert() in a switch default case.
(Ideally we should avoid the 'default' keyword where possible, although in some cases this is difficult.)
vkd3d_unreachable() is written in a way such that you can pass it NULL to it for precisely this reason. I went adding messages for all present uses, but it is not mandatory to have one. Does this address your objection?
I noticed that, but even so, I don't think there's value in printing a message in any of these cases. A message only makes sense when we expect it to be printed, and we don't expect any of these messages to ever be printed. The only reason I added assert(0) was for code clarity, and I think it's quite clear why the default code paths are unreachable.
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break; case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
This one I think should remain an assert. I can more easily see this getting triggered due to some forgotten implicit agreement. Hence we probably shouldn't tell the compiler this code path is unreachable.
Using an assert(!"string") or assert(0 && "string") construction (or defining our own macro) seems reasonable to me, though.
From the point of view of the compiler, assert(0) is not really different from vkd3d_unreachable(NULL): nothing tells the compiler that control will never ever reach it; if it happens, the compiler will print a message and abort(). The advantage of vkd3d_unreachable() is that the compiler won't try to tell you "Hey, you didn't write your return statement!" in places where it doesn't make any sense (because it's marked "noreturn").
As it's currently written, yes. However, I think we want to hook this up to __builtin_unreachable() if the latter is available.
On 14/04/2022 19:14, Zebediah Figura wrote:
On 4/14/22 06:00, Giovanni Mascellani wrote:
Hi,
Il 08/04/22 19:00, Zebediah Figura ha scritto:
I don't think there's value in specifying a message for cases like this, and I'd rather not have to do so in new code. This applies to pretty much anywhere we have an assert() in a switch default case.
(Ideally we should avoid the 'default' keyword where possible, although in some cases this is difficult.)
vkd3d_unreachable() is written in a way such that you can pass it NULL to it for precisely this reason. I went adding messages for all present uses, but it is not mandatory to have one. Does this address your objection?
I noticed that, but even so, I don't think there's value in printing a message in any of these cases. A message only makes sense when we expect it to be printed, and we don't expect any of these messages to ever be printed. The only reason I added assert(0) was for code clarity, and I think it's quite clear why the default code paths are unreachable.
@@ -1542,12 +1534,10 @@ static void write_sm4_cast(struct hlsl_ctx *ctx, break; case HLSL_TYPE_BOOL: - /* Casts to bool should have already been lowered. */ - assert(0); - break; + vkd3d_unreachable("Casts to bool should have already been lowered");
This one I think should remain an assert. I can more easily see this getting triggered due to some forgotten implicit agreement. Hence we probably shouldn't tell the compiler this code path is unreachable.
Using an assert(!"string") or assert(0 && "string") construction (or defining our own macro) seems reasonable to me, though.
From the point of view of the compiler, assert(0) is not really different from vkd3d_unreachable(NULL): nothing tells the compiler that control will never ever reach it; if it happens, the compiler will print a message and abort(). The advantage of vkd3d_unreachable() is that the compiler won't try to tell you "Hey, you didn't write your return statement!" in places where it doesn't make any sense (because it's marked "noreturn").
As it's currently written, yes. However, I think we want to hook this up to __builtin_unreachable() if the latter is available.
What about something like DEFAULT_UNREACHABLE from winnt.h?
On 4/14/22 11:49, Gabriel Ivăncescu wrote:
What about something like DEFAULT_UNREACHABLE from winnt.h?
Possibly, although I think we may want to use unreachable() in other cases, and personally I'd rather let C look like C.
-
Gabriel Ivăncescu -
Giovanni Mascellani -
Henri Verbeet -
Zebediah Figura