On Sun, Feb 12, 2023 at 9:44 AM Samuel Thibault <samuel.thiba...@gnu.org>
wrote:
> 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
>
