Re: [PATCH] Add support to define structures in mig.

[Samuel Thibault]

Applied, thanks!

Flavio Cruz, le ven. 04 nov. 2022 01:29:37 -0400, a ecrit:
> Basic syntax is presented below and allows users to define
> nested structured types by using simpler or structure types as
> members. Mig will use the C padding and alignment rules to produce
> the same size as the corresponding C structures.
> 
> type timespec_t = struct {
>   uint32_t tv_sec;
>   uint32_t tv_nsec;
> };
> 
> This allows us to build stubs that are more easily adaptable to other
> architectures.
> ---
> Hi
> 
> On Thu, Nov 03, 2022 at 09:02:03AM +0100, Luca wrote:
> > Due to 64-bit gnumach compatibility, my last patches on mig forced a 4-byte
> > alignment for all data fields. IIRC it's also forced on 32 bit, so I think
> > the alignment of struct here might not be working as expected.
> 
> If the struct has alignment that is less than 4 bytes, mig will pad the
> parameter with enough chars so that it is 4 bytes aligned, see here:
> https://git.savannah.gnu.org/cgit/hurd/mig.git/tree/type.c#n162
> I also made sure the alignment is never greater than the word size so it
> will be compatible with aligning the mig request to 4 bytes.
> 
> I've updated the test case with a smaller structure to ensure it works
> with our existing size assertions in the generated code.
> 
> diff --git a/lexxer.l b/lexxer.l
> index 71f43b2..508603a 100644
> --- a/lexxer.l
> +++ b/lexxer.l
> @@ -204,6 +204,8 @@ static void doSharp(const char *body); /* process body of 
> # directives */
>  <Normal>">"          RETURN(syRAngle);
>  <Normal>"["          RETURN(syLBrack);
>  <Normal>"]"          RETURN(syRBrack);
> +<Normal>"{"          RETURN(syLCBrack);
> +<Normal>"}"          RETURN(syRCBrack);
>  <Normal>"|"          RETURN(syBar);
>  
>  <Normal>{Ident}              { yylval.identifier = strmake(yytext);
> diff --git a/parser.y b/parser.y
> index 8521a84..104f604 100644
> --- a/parser.y
> +++ b/parser.y
> @@ -84,6 +84,8 @@
>  %token       syRAngle
>  %token       syLBrack
>  %token       syRBrack
> +%token       syLCBrack
> +%token       syRCBrack
>  %token       syBar
>  
>  %token       syError                 /* lex error */
> @@ -103,6 +105,7 @@
>  
>  %type        <statement_kind> ImportIndicant
>  %type        <number> VarArrayHead ArrayHead StructHead IntExp
> +%type   <structured_type> StructList
>  %type        <type> NamedTypeSpec TransTypeSpec TypeSpec
>  %type        <type> CStringSpec
>  %type        <type> BasicTypeSpec PrevTypeSpec ArgumentType
> @@ -124,6 +127,7 @@
>  #include "type.h"
>  #include "routine.h"
>  #include "statement.h"
> +#include "utils.h"
>  
>  static const char *import_name(statement_kind_t sk);
>  
> @@ -149,6 +153,14 @@ yyerror(const char *s)
>       const_string_t outstr;
>       u_int size;             /* 0 means there is no default size */
>      } symtype;
> +    /* Holds information about a structure while parsing. */
> +    struct
> +    {
> +        /* The required alignment (in bytes) so far. */
> +        u_int type_alignment_in_bytes;
> +        /* The size of the struct in bytes so far. */
> +        u_int size_in_bytes;
> +    } structured_type;
>      routine_t *routine;
>      arg_kind_t direction;
>      argument_t *argument;
> @@ -466,11 +478,43 @@ TypeSpec                :       BasicTypeSpec
>                       |       syCaret TypeSpec
>                               { $$ = itPtrDecl($2); }
>                       |       StructHead TypeSpec
> -                             { $$ = itStructDecl($1, $2); }
> +                             { $$ = itStructArrayDecl($1, $2); }
> +                        |       syStruct syLCBrack StructList syRCBrack
> +                             { $$ = itStructDecl($3.size_in_bytes, 
> $3.type_alignment_in_bytes); }
>                       |       CStringSpec
>                               { $$ = $1; }
>                       ;
>  
> +StructList                   :       syIdentifier syIdentifier sySemi
> +{
> +    ipc_type_t *t = itPrevDecl($1);
> +    if (!t) {
> +        error("Type %s not found\n", $1);
> +    }
> +    if (!t->itInLine) {
> +        error("Type %s must be inline\n", $2);
> +    }
> +
> +    $$.type_alignment_in_bytes = t->itAlignment;
> +    $$.size_in_bytes = t->itTypeSize;
> +}
> +                     |       StructList syIdentifier syIdentifier sySemi
> +{
> +    ipc_type_t *t = itPrevDecl($2);
> +    if (!t) {
> +        error("Type %s not found\n", $2);
> +    }
> +    if (!t->itInLine) {
> +        error("Type %s must be inline\n", $2);
> +    }
> +    $$.type_alignment_in_bytes = MAX(t->itAlignment, 
> $1.type_alignment_in_bytes);
> +    int padding_bytes = 0;
> +    if ($1.size_in_bytes % t->itAlignment)
> +        padding_bytes = t->itAlignment - ($1.size_in_bytes % t->itAlignment);
> +    $$.size_in_bytes = $1.size_in_bytes + padding_bytes + t->itTypeSize;
> +}
> +                        ;
> +
>  BasicTypeSpec                :       IPCType
>  {
>      $$ = itShortDecl($1.innumber, $1.instr,
> diff --git a/tests/good/complex-types.defs b/tests/good/complex-types.defs
> index 0a5c952..58d417e 100644
> --- a/tests/good/complex-types.defs
> +++ b/tests/good/complex-types.defs
> @@ -32,9 +32,21 @@ type mach_port_array_t = array[] of mach_port_t;
>  type char_struct_t = struct[4] of char;
>  type string_t = array[256] of char;
>  
> +type simple_struct_t = struct { byte a; };
> +
> +type complex_struct_x_t = struct { simple_struct_t a; simple_struct_t b; int 
> c; };
> +
> +type complex_struct_y_t = struct { complex_struct_x_t a; byte b; };
> +
> +type complex_struct_z_t = struct { complex_struct_y_t a; int d; };
> +
>  routine callcomplex(port : mach_port_t;
>                    p : pointer_t;
>                    q : intptr_t;
>                    str : char_struct_t;
>                    strt : string_t;
> +                  simple : simple_struct_t;
> +                  x : complex_struct_x_t;
> +                  y : complex_struct_y_t;
> +                  z : complex_struct_z_t;
>                    out vec : mach_port_array_t);
> diff --git a/tests/includes/types.h b/tests/includes/types.h
> index fe70e69..2a70443 100644
> --- a/tests/includes/types.h
> +++ b/tests/includes/types.h
> @@ -31,6 +31,26 @@ typedef struct char_struct {
>  typedef char string_t[256];
>  typedef const char* const_string_t;
>  
> +typedef struct simple_struct {
> +  char a;
> +} simple_struct_t;
> +
> +typedef struct complex_struct_x {
> +  simple_struct_t a;
> +  simple_struct_t b;
> +  int c;
> +} complex_struct_x_t;
> +
> +typedef struct complex_struct_y {
> +  complex_struct_x_t a;
> +  char b;
> +} complex_struct_y_t;
> +
> +typedef struct complex_struct_z {
> +  complex_struct_y_t a;
> +  int d;
> +} complex_struct_z_t;
> +
>  static inline int8_t int_to_int8(int n) {
>    return (int8_t) n;
>  }
> diff --git a/type.c b/type.c
> index 86137ae..66944d9 100644
> --- a/type.c
> +++ b/type.c
> @@ -24,6 +24,7 @@
>   * the rights to redistribute these changes.
>   */
>  
> +#include <assert.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  
> @@ -32,6 +33,7 @@
>  #include "type.h"
>  #include "message.h"
>  #include "cpu.h"
> +#include "utils.h"
>  
>  #if word_size_in_bits == 32
>  #define      word_size_name MACH_MSG_TYPE_INTEGER_32
> @@ -45,6 +47,7 @@
>  #endif
>  #endif
>  
> +ipc_type_t *itByteType;         /* used for defining struct types */
>  ipc_type_t *itRetCodeType;   /* used for return codes */
>  ipc_type_t *itDummyType;     /* used for camelot dummy args */
>  ipc_type_t *itRequestPortType;       /* used for default Request port arg */
> @@ -52,6 +55,8 @@ ipc_type_t *itZeroReplyPortType;/* used for dummy Reply 
> port arg */
>  ipc_type_t *itRealReplyPortType;/* used for default Reply port arg */
>  ipc_type_t *itWaitTimeType;  /* used for dummy WaitTime args */
>  ipc_type_t *itMsgOptionType; /* used for dummy MsgOption args */
> +ipc_type_t *itShortType;        /* used for the short type */
> +ipc_type_t *itIntType;          /* used for the int type */
>  
>  static ipc_type_t *list = itNULL;
>  
> @@ -100,6 +105,7 @@ itAlloc(void)
>       0,                      /* u_int itTypeSize */
>       0,                      /* u_int itPadSize */
>       0,                      /* u_int itMinTypeSize */
> +     0,                      /* u_int itAlignment */
>       0,                      /* u_int itInName */
>       0,                      /* u_int itOutName */
>       0,                      /* u_int itSize */
> @@ -173,6 +179,7 @@ itCalculateSizeInfo(ipc_type_t *it)
>       it->itTypeSize = bytes;
>       it->itPadSize = 0;
>       it->itMinTypeSize = bytes;
> +     it->itAlignment = bytes;
>      }
>  
>      /* Unfortunately, these warning messages can't give a type name;
> @@ -417,10 +424,10 @@ itCheckDecl(identifier_t name, ipc_type_t *it)
>  static void
>  itPrintTrans(const ipc_type_t *it)
>  {
> -    if (!streql(it->itName, it->itUserType))
> +    if (it->itName != strNULL && it->itUserType != strNULL && 
> !streql(it->itName, it->itUserType))
>       printf("\tCUserType:\t%s\n", it->itUserType);
>  
> -    if (!streql(it->itName, it->itServerType))
> +    if (it->itName != strNULL && !streql(it->itName, it->itServerType))
>       printf("\tCServerType:\t%s\n", it->itServerType);
>  
>      if (it->itInTrans != strNULL)
> @@ -525,6 +532,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);
>      if (inname == MACH_MSG_TYPE_STRING_C)
>      {
>       it->itStruct = FALSE;
> @@ -643,6 +651,7 @@ itArrayDecl(u_int number, const ipc_type_t *old)
>      it->itNumber *= number;
>      it->itStruct = FALSE;
>      it->itString = FALSE;
> +    it->itAlignment = old->itAlignment;
>  
>      itCalculateSizeInfo(it);
>      return it;
> @@ -673,7 +682,7 @@ itPtrDecl(ipc_type_t *it)
>   *   type new = struct[number] of old;
>   */
>  ipc_type_t *
> -itStructDecl(u_int number, const ipc_type_t *old)
> +itStructArrayDecl(u_int number, const ipc_type_t *old)
>  {
>      ipc_type_t *it = itResetType(itCopyType(old));
>  
> @@ -682,6 +691,48 @@ itStructDecl(u_int number, const ipc_type_t *old)
>      it->itNumber *= number;
>      it->itStruct = TRUE;
>      it->itString = FALSE;
> +    it->itAlignment = old->itAlignment;
> +
> +    itCalculateSizeInfo(it);
> +    return it;
> +}
> +
> +/*
> + *  Handles the declaration
> + *   type new = struct { type1 a1; type2 a2; ... };
> + */
> +ipc_type_t *
> +itStructDecl(u_int min_type_size_in_bytes, u_int required_alignment_in_bytes)
> +{
> +    int final_struct_bytes = min_type_size_in_bytes;
> +    if (final_struct_bytes % required_alignment_in_bytes) {
> +         final_struct_bytes += required_alignment_in_bytes - 
> (final_struct_bytes % required_alignment_in_bytes);
> +    }
> +    ipc_type_t *element_type = NULL;
> +    int number_elements = 0;
> +    // If the struct is short or int aligned, use that as the base type.
> +    switch (required_alignment_in_bytes) {
> +     case 2:
> +         element_type = itShortType;
> +         assert(final_struct_bytes % 2 == 0);
> +         number_elements = final_struct_bytes / 2;
> +         break;
> +     case 4:
> +         element_type = itIntType;
> +         assert(final_struct_bytes % 4 == 0);
> +         number_elements = final_struct_bytes / 4;
> +         break;
> +        case 1:
> +     default:
> +         element_type = itByteType;
> +         number_elements = final_struct_bytes;
> +         break;
> +    }
> +    ipc_type_t *it = itResetType(itCopyType(element_type));
> +    it->itNumber = number_elements;
> +    it->itStruct = TRUE;
> +    it->itString = FALSE;
> +    it->itAlignment = required_alignment_in_bytes;
>  
>      itCalculateSizeInfo(it);
>      return it;
> @@ -709,6 +760,7 @@ itCStringDecl(u_int count, boolean_t varying)
>      it->itVarArray = varying;
>      it->itStruct = FALSE;
>      it->itString = TRUE;
> +    it->itAlignment = itElement->itAlignment;
>  
>      itCalculateSizeInfo(it);
>      return it;
> @@ -744,6 +796,7 @@ itCIntTypeDecl(const_string_t ctype, const size_t size)
>            exit(EXIT_FAILURE);
>      }
>      it->itName = ctype;
> +    it->itAlignment = size;
>      itCalculateNameInfo(it);
>      return it;
>  }
> @@ -822,6 +875,16 @@ itMakeDeallocType(void)
>  void
>  init_type(void)
>  {
> +    itByteType = itAlloc();
> +    itByteType->itName = "unsigned char";
> +    itByteType->itInName = MACH_MSG_TYPE_BYTE;
> +    itByteType->itInNameStr = "MACH_MSG_TYPE_BYTE";
> +    itByteType->itOutName = MACH_MSG_TYPE_BYTE;
> +    itByteType->itOutNameStr = "MACH_MSG_TYPE_BYTE";
> +    itByteType->itSize = 8;
> +    itCalculateSizeInfo(itByteType);
> +    itCalculateNameInfo(itByteType);
> +
>      itRetCodeType = itAlloc();
>      itRetCodeType->itName = "kern_return_t";
>      itRetCodeType->itInName = MACH_MSG_TYPE_INTEGER_32;
> @@ -877,8 +940,10 @@ init_type(void)
>  
>      /* Define basic C integral types. */
>      itInsert("char", itCIntTypeDecl("char", sizeof_char));
> -    itInsert("short", itCIntTypeDecl("short", sizeof_short));
> -    itInsert("int", itCIntTypeDecl("int", sizeof_int));
> +    itShortType = itCIntTypeDecl("short", sizeof_short);
> +    itInsert("short", itShortType);
> +    itIntType = itCIntTypeDecl("int", sizeof_int);
> +    itInsert("int", itIntType);
>  }
>  
>  /******************************************************
> diff --git a/type.h b/type.h
> index 6cf5d63..6031ee1 100644
> --- a/type.h
> +++ b/type.h
> @@ -143,6 +143,7 @@ typedef struct ipc_type
>      u_int itTypeSize;                /* size of the C type */
>      u_int itPadSize;         /* amount of padding after data */
>      u_int itMinTypeSize;     /* minimal amount of space occupied by data */
> +    u_int itAlignment;         /* alignment required for this type */
>  
>      u_int itInName;          /* name supplied to kernel in sent msg */
>      u_int itOutName;         /* name in received msg */
> @@ -193,7 +194,8 @@ extern ipc_type_t *itResetType(ipc_type_t *it);
>  extern ipc_type_t *itVarArrayDecl(u_int number, const ipc_type_t *it);
>  extern ipc_type_t *itArrayDecl(u_int number, const ipc_type_t *it);
>  extern ipc_type_t *itPtrDecl(ipc_type_t *it);
> -extern ipc_type_t *itStructDecl(u_int number, const ipc_type_t *it);
> +extern ipc_type_t *itStructArrayDecl(u_int number, const ipc_type_t *it);
> +extern ipc_type_t *itStructDecl(u_int min_type_size_in_bytes, u_int 
> required_alignment_in_bytes);
>  extern ipc_type_t *itCStringDecl(u_int number, boolean_t varying);
>  
>  extern ipc_type_t *itRetCodeType;
> diff --git a/utils.h b/utils.h
> index a5673b0..64e2ebf 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -27,8 +27,19 @@
>  #ifndef      _UTILS_H
>  #define      _UTILS_H
>  
> +#include "routine.h"
> +
>  /* stuff used by more than one of header.c, user.c, server.c */
>  
> +#define MAX(a,b) \
> +  ({ __typeof__ (a) _a = (a); \
> +      __typeof__ (b) _b = (b); \
> +    _a > _b ? _a : _b; })
> +#define MIN(a,b) \
> +  ({ __typeof__ (a) _a = (a); \
> +      __typeof__ (b) _b = (b); \
> +    _a > _b ? _b : _a; })
> +
>  typedef void write_list_fn_t(FILE *file, const argument_t *arg);
>  
>  extern void WriteImport(FILE *file, const_string_t filename);
> -- 
> 2.37.2
> 
> 

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