Re: [PATCH mig] Introduce max_alignof to replace word_size

[Samuel Thibault]

Renamed to complex_alignof and commited, thanks!

Flavio Cruz, le dim. 12 févr. 2023 17:14:23 -0500, a ecrit:
> Remove the concept of word_size since it is meaningless in some
> architectures. This is also done in preparation to possibly introduce
> 8-byte aligned messages.
> ---
>  cpu.sym   |  3 +-
>  global.c  |  5 +--
>  global.h  |  5 +--
>  parser.y  |  8 ++---
>  routine.c |  5 ++-
>  server.c  | 20 ++++++------
>  type.c    | 95 ++++++++++++++++++++++++++++---------------------------
>  user.c    | 22 ++++++-------
>  utils.c   |  6 +++-
>  utils.h   |  2 ++
>  10 files changed, 91 insertions(+), 80 deletions(-)
> 
> diff --git a/cpu.sym b/cpu.sym
> index 6bcb878..57084d0 100644
> --- a/cpu.sym
> +++ b/cpu.sym
> @@ -95,8 +95,7 @@ expr MACH_MSG_TYPE_POLYMORPHIC
>  
>  
>  /* Types used in interfaces */
> -expr sizeof(integer_t)                       word_size
> -expr (sizeof(integer_t)*8)           word_size_in_bits
> +expr sizeof(natural_t)                       desired_max_alignof
>  expr sizeof(void*)                   sizeof_pointer
>  expr sizeof(char)                    sizeof_char
>  expr sizeof(short)                   sizeof_short
> diff --git a/global.c b/global.c
> index bf3d10d..ad477c2 100644
> --- a/global.c
> +++ b/global.c
> @@ -66,8 +66,9 @@ string_t InternalHeaderFileName = strNULL;
>  string_t UserFileName = strNULL;
>  string_t ServerFileName = strNULL;
>  
> -int port_size = port_name_size;
> -int port_size_in_bits = port_name_size_in_bits;
> +size_t port_size = port_name_size;
> +size_t port_size_in_bits = port_name_size_in_bits;
> +size_t max_alignof = desired_max_alignof;
>  
>  void
>  more_global(void)
> diff --git a/global.h b/global.h
> index 8e57df2..bf8eb38 100644
> --- a/global.h
> +++ b/global.h
> @@ -67,8 +67,9 @@ extern string_t InternalHeaderFileName;
>  extern string_t UserFileName;
>  extern string_t ServerFileName;
>  
> -extern int port_size;
> -extern int port_size_in_bits;
> +extern size_t port_size;
> +extern size_t port_size_in_bits;
> +extern size_t max_alignof;
>  
>  extern void more_global(void);
>  
> diff --git a/parser.y b/parser.y
> index 03c5ec8..ccf4726 100644
> --- a/parser.y
> +++ b/parser.y
> @@ -212,6 +212,10 @@ Subsystem                :       SubsystemStart 
> SubsystemMods
>              IsKernelUser ? ", KernelUser" : "",
>              IsKernelServer ? ", KernelServer" : "");
>      }
> +    if (IsKernelUser || IsKernelServer) {
> +        port_size = vm_offset_size;
> +        port_size_in_bits = vm_offset_size_in_bits;
> +    }
>      init_type();
>  }
>                       ;
> @@ -237,16 +241,12 @@ SubsystemMod            :       syKernelUser
>      if (IsKernelUser)
>       warn("duplicate KernelUser keyword");
>      IsKernelUser = true;
> -    port_size = vm_offset_size;
> -    port_size_in_bits = vm_offset_size_in_bits;
>  }
>                       |       syKernelServer
>  {
>      if (IsKernelServer)
>       warn("duplicate KernelServer keyword");
>      IsKernelServer = true;
> -    port_size = vm_offset_size;
> -    port_size_in_bits = vm_offset_size_in_bits;
>  }
>                       ;
>  
> diff --git a/routine.c b/routine.c
> index c0a016c..5ed0064 100644
> --- a/routine.c
> +++ b/routine.c
> @@ -47,6 +47,7 @@
>  #include "routine.h"
>  #include "message.h"
>  #include "cpu.h"
> +#include "utils.h"
>  
>  u_int rtNumber = 0;
>  
> @@ -316,19 +317,21 @@ rtFindSize(const argument_t *args, u_int mask)
>      const argument_t *arg;
>      u_int size = sizeof_mach_msg_header_t;
>  
> +    size = ALIGN(size, max_alignof);
>      for (arg = args; arg != argNULL; arg = arg->argNext)
>       if (akCheck(arg->argKind, mask))
>       {
>           ipc_type_t *it = arg->argType;
>  
>           /* might need proper alignment on demanding 64bit archies */
> -         size = (size + word_size-1) & ~(word_size-1);
>           if (arg->argLongForm) {
>               size += sizeof_mach_msg_type_long_t;
>           } else {
>               size += sizeof_mach_msg_type_t;
>           }
> +         size = ALIGN(size, max_alignof);
>  
> +         /* Note itMinTypeSize is already aligned to max_alignof. */
>           size += it->itMinTypeSize;
>       }
>  
> diff --git a/server.c b/server.c
> index 95b0056..14b129b 100644
> --- a/server.c
> +++ b/server.c
> @@ -483,7 +483,7 @@ WriteCheckArgSize(FILE *file, const argument_t *arg)
>               arg->argLongForm ? ".msgtl_header" : "");
>      }
>  
> -    if (btype->itTypeSize % word_size != 0)
> +    if (btype->itTypeSize % max_alignof != 0)
>       fprintf(file, "(");
>  
>      if (multiplier > 1)
> @@ -491,10 +491,10 @@ WriteCheckArgSize(FILE *file, const argument_t *arg)
>  
>      fprintf(file, "In%dP->%s", arg->argRequestPos, count->argMsgField);
>  
> -    /* If the base type size of the data field isn`t a multiple of word_size,
> +    /* If the base type size of the data field isn`t a multiple of 
> max_alignof,
>         we have to round up. */
> -    if (btype->itTypeSize % word_size != 0)
> -     fprintf(file, " + %d) & ~%d", word_size - 1, word_size - 1);
> +    if (btype->itTypeSize % max_alignof != 0)
> +     fprintf(file, " + %d) & ~%d", max_alignof - 1, max_alignof - 1);
>  
>      if (ptype->itIndefinite) {
>       fprintf(file, " : sizeof(%s *)", FetchServerType(btype));
> @@ -530,8 +530,8 @@ WriteCheckMsgSize(FILE *file, const argument_t *arg)
>       bool LastVarArg = arg->argRequestPos+1 == rt->rtNumRequestVar;
>  
>       /* calculate the actual size in bytes of the data field.  note
> -        that this quantity must be a multiple of word_size.  hence, if
> -        the base type size isn't a multiple of word_size, we have to
> +        that this quantity must be a multiple of max_alignof.  hence, if
> +        the base type size isn't a multiple of max_alignof, we have to
>          round up.  note also that btype->itNumber must
>          divide btype->itTypeSize (see itCalculateSizeInfo). */
>  
> @@ -1090,7 +1090,7 @@ WriteArgSize(FILE *file, const argument_t *arg)
>               arg->argLongForm ? ".msgtl_header" : "");
>      }
>  
> -    if (bsize % word_size != 0)
> +    if (bsize % max_alignof != 0)
>       fprintf(file, "(");
>  
>      if (bsize > 1)
> @@ -1103,11 +1103,11 @@ WriteArgSize(FILE *file, const argument_t *arg)
>       fprintf(file, "%s", count->argVarName);
>  
>      /*
> -     * If the base type size is not a multiple of word_size,
> +     * If the base type size is not a multiple of max_alignof,
>       * we have to round up.
>       */
> -    if (bsize % word_size != 0)
> -     fprintf(file, " + %d) & ~%d", word_size - 1, word_size - 1);
> +    if (bsize % max_alignof != 0)
> +     fprintf(file, " + %d) & ~%d", max_alignof - 1, max_alignof - 1);
>  
>      if (ptype->itIndefinite) {
>       fprintf(file, " : sizeof(%s *)",
> diff --git a/type.c b/type.c
> index b104c66..f44be57 100644
> --- a/type.c
> +++ b/type.c
> @@ -35,17 +35,8 @@
>  #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
> +#define int_name MACH_MSG_TYPE_INTEGER_32
> +#define int_name_string "MACH_MSG_TYPE_INTEGER_32"
>  
>  ipc_type_t *itByteType;         /* used for defining struct types */
>  ipc_type_t *itRetCodeType;   /* used for return codes */
> @@ -169,10 +160,12 @@ itNameToString(u_int name)
>  static void
>  itCalculateSizeInfo(ipc_type_t *it)
>  {
> +    assert(it->itAlignment > 0);
> +
>      if (it->itInLine)
>      {
>       u_int bytes = (it->itNumber * it->itSize + 7) / 8;
> -     u_int padding = (word_size - bytes % word_size) % word_size;
> +     u_int padding = (max_alignof - bytes % max_alignof) % max_alignof;
>  
>       it->itTypeSize = bytes;
>       it->itPadSize = padding;
> @@ -189,7 +182,7 @@ itCalculateSizeInfo(ipc_type_t *it)
>       it->itTypeSize = bytes;
>       it->itPadSize = 0;
>       it->itMinTypeSize = bytes;
> -     it->itAlignment = bytes;
> +     assert(it->itAlignment == bytes);
>      }
>  
>      /* Unfortunately, these warning messages can't give a type name;
> @@ -542,7 +535,7 @@ itLongDecl(u_int inname, const_string_t instr, u_int 
> outname,
>      it->itOutName = outname;
>      it->itOutNameStr = outstr;
>      it->itSize = size;
> -    it->itAlignment = MIN(word_size, size / 8);
> +    it->itAlignment = MIN(max_alignof, size / 8);
>      if (inname == MACH_MSG_TYPE_STRING_C)
>      {
>       it->itStruct = false;
> @@ -682,6 +675,7 @@ itPtrDecl(ipc_type_t *it)
>      it->itInLine = false;
>      it->itStruct = true;
>      it->itString = false;
> +    it->itAlignment = sizeof_pointer;
>  
>      itCalculateSizeInfo(it);
>      return it;
> @@ -806,7 +800,6 @@ itCIntTypeDecl(const_string_t ctype, const size_t size)
>            exit(EXIT_FAILURE);
>      }
>      it->itName = ctype;
> -    it->itAlignment = size;
>      itCalculateNameInfo(it);
>      return it;
>  }
> @@ -817,11 +810,12 @@ itMakeCountType(void)
>      ipc_type_t *it = itAlloc();
>  
>      it->itName = "mach_msg_type_number_t";
> -    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 = int_name;
> +    it->itInNameStr = int_name_string;
> +    it->itOutName = int_name;
> +    it->itOutNameStr = int_name_string;
> +    it->itSize = sizeof_int * 8;
> +    it->itAlignment = sizeof_int;
>  
>      itCalculateSizeInfo(it);
>      itCalculateNameInfo(it);
> @@ -834,11 +828,12 @@ itMakeNaturalType(const char *name)
>      ipc_type_t *it = itAlloc();
>  
>      it->itName = name;
> -    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 = int_name;
> +    it->itInNameStr = int_name_string;
> +    it->itOutName = int_name;
> +    it->itOutNameStr = int_name_string;
> +    it->itSize = sizeof_int * 8;
> +    it->itAlignment = sizeof_int;
>  
>      itCalculateSizeInfo(it);
>      itCalculateNameInfo(it);
> @@ -851,11 +846,12 @@ itMakePolyType(void)
>      ipc_type_t *it = itAlloc();
>  
>      it->itName = "mach_msg_type_name_t";
> -    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 = int_name;
> +    it->itInNameStr = int_name_string;
> +    it->itOutName = int_name;
> +    it->itOutNameStr = int_name_string;
> +    it->itSize = sizeof_int * 8;
> +    it->itAlignment = sizeof_int;
>  
>      itCalculateSizeInfo(it);
>      itCalculateNameInfo(it);
> @@ -872,7 +868,8 @@ itMakeDeallocType(void)
>      it->itInNameStr = "MACH_MSG_TYPE_BOOLEAN";
>      it->itOutName = MACH_MSG_TYPE_BOOLEAN;
>      it->itOutNameStr = "MACH_MSG_TYPE_BOOLEAN";
> -    it->itSize = 32;
> +    it->itSize = sizeof_int * 8;
> +    it->itAlignment = sizeof_int;
>  
>      itCalculateSizeInfo(it);
>      itCalculateNameInfo(it);
> @@ -899,17 +896,19 @@ init_type(void)
>      itByteType->itInNameStr = "MACH_MSG_TYPE_BYTE";
>      itByteType->itOutName = MACH_MSG_TYPE_BYTE;
>      itByteType->itOutNameStr = "MACH_MSG_TYPE_BYTE";
> -    itByteType->itSize = 8;
> +    itByteType->itSize = sizeof_char * 8;
> +    itByteType->itAlignment = sizeof_char;
>      itCalculateSizeInfo(itByteType);
>      itCalculateNameInfo(itByteType);
>  
>      itRetCodeType = itAlloc();
>      itRetCodeType->itName = "kern_return_t";
> -    itRetCodeType->itInName = MACH_MSG_TYPE_INTEGER_32;
> -    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->itInName = int_name;
> +    itRetCodeType->itInNameStr = int_name_string;
> +    itRetCodeType->itOutName = int_name;
> +    itRetCodeType->itOutNameStr = int_name_string;
> +    itRetCodeType->itSize = sizeof_int * 8;
> +    itRetCodeType->itAlignment = sizeof_int;
>      itCalculateSizeInfo(itRetCodeType);
>      itCalculateNameInfo(itRetCodeType);
>  
> @@ -919,7 +918,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 = max_alignof * 8;
> +    itDummyType->itAlignment = max_alignof;
>      itCalculateSizeInfo(itDummyType);
>      itCalculateNameInfo(itDummyType);
>  
> @@ -930,6 +930,7 @@ init_type(void)
>      itRequestPortType->itOutName = MACH_MSG_TYPE_PORT_SEND;
>      itRequestPortType->itOutNameStr = "MACH_MSG_TYPE_PORT_SEND";
>      itRequestPortType->itSize = port_size_in_bits;
> +    itRequestPortType->itAlignment = port_size;
>      itCalculateSizeInfo(itRequestPortType);
>      itCalculateNameInfo(itRequestPortType);
>  
> @@ -940,6 +941,7 @@ init_type(void)
>      itZeroReplyPortType->itOutName = 0;
>      itZeroReplyPortType->itOutNameStr = "0";
>      itZeroReplyPortType->itSize = port_size_in_bits;
> +    itZeroReplyPortType->itAlignment = port_size;
>      itCalculateSizeInfo(itZeroReplyPortType);
>      itCalculateNameInfo(itZeroReplyPortType);
>  
> @@ -950,6 +952,7 @@ init_type(void)
>      itRealReplyPortType->itOutName = MACH_MSG_TYPE_PORT_SEND_ONCE;
>      itRealReplyPortType->itOutNameStr = "MACH_MSG_TYPE_PORT_SEND_ONCE";
>      itRealReplyPortType->itSize = port_size_in_bits;
> +    itRealReplyPortType->itAlignment = port_size;
>      itCalculateSizeInfo(itRealReplyPortType);
>      itCalculateNameInfo(itRealReplyPortType);
>  
> @@ -1030,8 +1033,8 @@ itCheckReplyPortType(identifier_t name, const 
> ipc_type_t *it)
>  void
>  itCheckIntType(identifier_t name, const ipc_type_t *it)
>  {
> -    if ((it->itInName != MACH_MSG_TYPE_INTEGER_32) ||
> -     (it->itOutName != MACH_MSG_TYPE_INTEGER_32) ||
> +    if ((it->itInName != int_name) ||
> +     (it->itOutName != int_name) ||
>       (it->itNumber != 1) ||
>       (it->itSize != 32) ||
>       !it->itInLine ||
> @@ -1041,17 +1044,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 != int_name) ||
> +     (it->itOutName != int_name) ||
>       (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 %s", name, int_name_string);
>  }
> diff --git a/user.c b/user.c
> index e951ee2..dd43335 100644
> --- a/user.c
> +++ b/user.c
> @@ -507,7 +507,7 @@ WriteArgSize(FILE *file, const argument_t *arg)
>       fprintf(file, "(InP->%s%s.msgt_inline) ? ",
>               arg->argTTName, arg->argLongForm ? ".msgtl_header" : "");
>      }
> -    if (bsize % word_size != 0)
> +    if (bsize % max_alignof != 0)
>       fprintf(file, "(");
>  
>      if (bsize > 1)
> @@ -523,11 +523,11 @@ WriteArgSize(FILE *file, const argument_t *arg)
>               count->argVarName);
>  
>      /*
> -     * If the base type size is not a multiple of word_size,
> +     * If the base type size is not a multiple of max_alignof,
>       * we have to round up.
>       */
> -    if (bsize % word_size != 0)
> -     fprintf(file, " + %d) & ~%d", word_size - 1, word_size - 1);
> +    if (bsize % max_alignof != 0)
> +     fprintf(file, " + %d) & ~%d", max_alignof - 1, max_alignof - 1);
>  
>      if (ptype->itIndefinite) {
>       fprintf(file, " : sizeof(%s *)",
> @@ -831,7 +831,7 @@ WriteCheckArgSize(FILE *file, const argument_t *arg)
>               arg->argTTName, arg->argLongForm ? ".msgtl_header" : "");
>      }
>  
> -    if (btype->itTypeSize % word_size != 0)
> +    if (btype->itTypeSize % max_alignof != 0)
>       fprintf(file, "(");
>  
>      if (multiplier > 1)
> @@ -839,10 +839,10 @@ WriteCheckArgSize(FILE *file, const argument_t *arg)
>  
>      fprintf(file, "OutP->%s", count->argMsgField);
>  
> -    /* If the base type size of the data field isn`t a multiple of word_size,
> +    /* If the base type size of the data field isn`t a multiple of 
> max_alignof,
>         we have to round up. */
> -    if (btype->itTypeSize % word_size != 0)
> -     fprintf(file, " + %d) & ~%d", word_size - 1, word_size - 1);
> +    if (btype->itTypeSize % max_alignof != 0)
> +     fprintf(file, " + %d) & ~%d", max_alignof - 1, max_alignof - 1);
>  
>      if (ptype->itIndefinite)
>       fprintf(file, " : sizeof(%s *)", FetchUserType(btype));
> @@ -877,8 +877,8 @@ WriteCheckMsgSize(FILE *file, const argument_t *arg)
>       bool LastVarArg = arg->argReplyPos+1 == rt->rtNumReplyVar;
>  
>       /* calculate the actual size in bytes of the data field.  note
> -        that this quantity must be a multiple of word_size.  hence, if
> -        the base type size isn't a multiple of word_size, we have to
> +        that this quantity must be a multiple of max_alignof.  hence, if
> +        the base type size isn't a multiple of max_alignof, we have to
>          round up.  note also that btype->itNumber must
>          divide btype->itTypeSize (see itCalculateSizeInfo). */
>  
> @@ -1127,7 +1127,7 @@ WriteReturnValue(FILE *file, const routine_t *rt)
>  /*************************************************************
>   *  Writes the elements of the message type declaration: the
>   *  msg_type structure, the argument itself and any padding
> - *  that is required to make the argument a multiple of word_size bytes.
> + *  that is required to make the argument a multiple of max_alignof bytes.
>   *  Called by WriteRoutine for all the arguments in the request
>   *  message first and then the reply message.
>   *************************************************************/
> diff --git a/utils.c b/utils.c
> index 2fb8c2e..b65e304 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 && max_alignof > 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, max_alignof - 
> sizeof_mach_msg_type_t);
> +    }
>  
>      if (it->itInLine && it->itVarArray)
>      {
> @@ -338,7 +342,7 @@ 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);
> +    fprintf(file, "#pragma pack(push,%d)\n", max_alignof);
>      fprintf(file, "\ttypedef struct {\n");
>      fprintf(file, "\t\tmach_msg_header_t Head;\n");
>      WriteList(file, args, func, mask, "\n", "\n");
> diff --git a/utils.h b/utils.h
> index 4be4f4c..477ea74 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -39,6 +39,8 @@
>    ({ __typeof__ (a) _a = (a); \
>        __typeof__ (b) _b = (b); \
>      _a > _b ? _b : _a; })
> +#define ALIGN(x, alignment) \
> +    ((x) + (alignment)-1) & ~((alignment)-1)
>  
>  typedef void write_list_fn_t(FILE *file, const argument_t *arg);
>  
> -- 
> 2.39.1
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.