Thanks for your work on the 64bit RPC part, that's tricky :)
Flavio Cruz, le dim. 12 févr. 2023 01:15:05 -0500, a ecrit:
> diff --git a/cpu.sym b/cpu.sym
> index 6bcb878..7858b47 100644
> --- a/cpu.sym
> +++ b/cpu.sym
> @@ -95,8 +95,8 @@ expr MACH_MSG_TYPE_POLYMORPHIC
>
>
> /* Types used in interfaces */
> -expr sizeof(integer_t) word_size
> -expr (sizeof(integer_t)*8) word_size_in_bits
> +/* The minimum alignment required for this architecture */
> +expr sizeof(uintptr_t) desired_machine_alignment
I'm really not at ease with such general "architecture-required
alignment". Alignment always depends on the kind of data you are
manipulating. If you throw a __float128 field in your structure, it'll
have to be 16-byte-aligned.
I'd tend to think that we probably want to follow C on that: each
type has its own alignment constraint that we can encode in mig,
and alignment constraints of structures is the max of the alignment
requirements of the elements of the structure.
Fair enough. I changed it to max_alignof. I don't think we allow scalars larger than 8 bytes so I don't think this can happen today.
> diff --git a/type.c b/type.c
> index b104c66..05ee201 100644
> --- a/type.c
> +++ b/type.c
> @@ -35,18 +35,6 @@
> #include "cpu.h"
> #include "utils.h"
>
> -#if word_size_in_bits == 32
> -#define word_size_name MACH_MSG_TYPE_INTEGER_32
> -#define word_size_name_string "MACH_MSG_TYPE_INTEGER_32"
> -#else
> -#if word_size_in_bits == 64
> -#define word_size_name MACH_MSG_TYPE_INTEGER_64
> -#define word_size_name_string "MACH_MSG_TYPE_INTEGER_64"
> -#else
> -#error Unsupported word size!
> -#endif
> -#endif
I agree on dropping the "word size" term that is ambiguous on a 64bit
architecture, where you can still want to consider 32bit as a word.
I'd however say to introduce int_name and int_name_string, so that:
Done
> - it->itInName = word_size_name;
> - it->itInNameStr = word_size_name_string;
> - it->itOutName = word_size_name;
> - it->itOutNameStr = word_size_name_string;
> - it->itSize = word_size_in_bits;
> + it->itInName = MACH_MSG_TYPE_INTEGER_32;
> + it->itInNameStr = "MACH_MSG_TYPE_INTEGER_32";
> + it->itOutName = MACH_MSG_TYPE_INTEGER_32;
> + it->itOutNameStr = "MACH_MSG_TYPE_INTEGER_32";
> + it->itSize = sizeof_int * 8;
> + it->itAlignment = sizeof_int;
These become coherent, and less surprising to change if we ever need to.
> @@ -873,6 +865,7 @@ itMakeDeallocType(void)
> it->itOutName = MACH_MSG_TYPE_BOOLEAN;
> it->itOutNameStr = "MACH_MSG_TYPE_BOOLEAN";
> it->itSize = 32;
> + it->itAlignment = sizeof_int;
I'd say also use sizeof_int * 8 for itSize.
> @@ -909,7 +903,8 @@ init_type(void)
> itRetCodeType->itInNameStr = "MACH_MSG_TYPE_INTEGER_32";
> itRetCodeType->itOutName = MACH_MSG_TYPE_INTEGER_32;
> itRetCodeType->itOutNameStr = "MACH_MSG_TYPE_INTEGER_32";
> - itRetCodeType->itSize = 32;
> + itRetCodeType->itSize = sizeof_int * 8;
> + itRetCodeType->itAlignment = sizeof_int;
Also here, use int_name and int_name_string.
> @@ -919,7 +914,8 @@ init_type(void)
> itDummyType->itInNameStr = "MACH_MSG_TYPE_UNSTRUCTURED";
> itDummyType->itOutName = MACH_MSG_TYPE_UNSTRUCTURED;
> itDummyType->itOutNameStr = "MACH_MSG_TYPE_UNSTRUCTURED";
> - itDummyType->itSize = word_size_in_bits;
> + itDummyType->itSize = word_size * 8;
> + itDummyType->itAlignment = word_size;
So, now the question raises: what do we want to permit in an
unstructured? If we're fine with not including __float128, we can indeed
go with alignof(intptr_t). But I'd really say to call this so (e.g.
max_alignof), rather than word_size which does not convey much meaning.
> @@ -1041,17 +1040,15 @@ itCheckIntType(identifier_t name, const ipc_type_t *it)
> error("argument %s isn't a proper integer", name);
> }
> void
> -itCheckNaturalType(name, it)
> - identifier_t name;
> - ipc_type_t *it;
> +itCheckNaturalType(identifier_t name, ipc_type_t *it)
> {
> - if ((it->itInName != word_size_name) ||
> - (it->itOutName != word_size_name) ||
> + if ((it->itInName != MACH_MSG_TYPE_INTEGER_32) ||
> + (it->itOutName != MACH_MSG_TYPE_INTEGER_32) ||
Also, here, int_name for coherency with sizeof_int.:
> (it->itNumber != 1) ||
> - (it->itSize != word_size_in_bits) ||
> + (it->itSize != sizeof_int * 8) ||
> !it->itInLine ||
> it->itDeallocate != d_NO ||
> !it->itStruct ||
> it->itVarArray)
> - error("argument %s should have been a %s", name, word_size_name_string);
> + error("argument %s should have been a MACH_MSG_TYPE_INTEGER_32", name);
> }
> diff --git a/utils.c b/utils.c
> index 2fb8c2e..e8aec9a 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -304,6 +304,10 @@ WriteFieldDeclPrim(FILE *file, const argument_t *arg,
>
> fprintf(file, "\t\tmach_msg_type_%st %s;\n",
> arg->argLongForm ? "long_" : "", arg->argTTName);
> + if (!arg->argLongForm && word_size > sizeof_mach_msg_type_t) {
> + /* Pad mach_msg_type_t in case we need alignment by more than its size. */
> + fprintf(file, "\t\tchar %s_pad[%d];\n", arg->argTTName, word_size - sizeof_mach_msg_type_t);
> + }
Here as well we'd want to call it max_alignof.
> @@ -338,12 +342,16 @@ void
> WriteStructDecl(FILE *file, const argument_t *args, write_list_fn_t *func,
> u_int mask, const char *name)
> {
> - fprintf(file, "#pragma pack(push,%d)\n", word_size);
> + if (word_size == 4) {
> + fprintf(file, "#pragma pack(push,%d)\n", word_size);
> + }
How is it needed for word size 4 and not for word size 8 ?
Reverted.
Samuel