Re: [committed mig] Do not generate code dereferencing type-punned pointers

[David Michael]

On Mon, Mar 2, 2015 at 11:03 AM, Justus Winter
<4winter@informatik.uni-hamburg.de> wrote:
> Hi David :)
>
> thanks for cleaning up after me ;)  (again and again...)
>
> Quoting David Michael (2015-02-18 05:39:46)
>> On Sun, Feb 15, 2015 at 11:09 AM, Justus Winter
>> <4winter@informatik.uni-hamburg.de> wrote:
>> > * utils.c (WriteFieldDeclPrim): Generate a union with an additional
>> > pointer field for variable-length arrays.
>>
>> This makes GDB's awk script go haywire because it doesn't know how to
>> deal with unions.  The following is a workaround to get it building
>> again, but I'm not sure of its correctness.  Can someone more
>> knowledgeable than me check on this?
>
> Not that I'm claiming to know why gdb does what it does, but Samuel
> asked me to look into this.

Thanks for looking into it.

> For some reason (proxying?), gdb uses mig to create server-side stubs
> for the *reply*-half of some protocols.  It then uses an awk-script to
> insert the following code into the server-stubs:
>
> if (In0P->Head.msgh_size == sizeof (Reply)
>          && ! (In0P->Head.msgh_bits & MACH_MSGH_BITS_COMPLEX)
>          && ! BAD_TYPECHECK(&In0P->return_codeType, &return_codeCheck)
>          && In0P->return_code != 0)
> /* Error return, only the error code argument is passed.  */
> {
>   kern_return_t (*sfun)(mach_port_t, kern_return_t, int, data_t, 
> mach_msg_type_number_t, data_t, mach_msg_type_number_t) = 
> S_proc_getprocinfo_reply;
>   OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t))sfun) 
> (In0P->Head.msgh_request_port, In0P->return_code);
>   return;
> }
>
> This is inserted *before* any type checks.  The awk script has this to
> say:
>
> # The first args type checking statement; we need to insert our chunk of
> # code that bypasses all the type checks if this is an error return, after
> # which we're done until we get to the next function.  Handily, the size
> # of mig's Reply structure is also the size of the alternate Request
> # structure that we want to check for.
> [...]
> # Force the function into a type that only takes the first two args, via
> # the temp variable SFUN (is there another way to correctly do this cast?).
> # This is possibly bogus, but easier than supplying bogus values for all
> # the other args (we can't just pass 0 for them, as they might not be scalar).
>
> I don't see why it bothers with SFUN in the first place.  It could just do
>
>   OutP->RetCode = (*(kern_return_t (*)(mach_port_t, kern_return_t)) 
> S_proc_getprocinfo_reply) (In0P->Head.msgh_request_port, In0P->return_code);

That was my first thought as well, but the comment "is there another
way to correctly do this cast?" made me think I was missing some
subtlety.  Thomas took care of updating this last time mig was
modified; maybe he is familiar with its design choices?

> right?  If so, the main reason for parsing the struct is gone, and we
> could simplify the script a lot and thus making it less likely to
> break in the future.  Getting rid of it entirely would be nice though.
>
> A word about the patch.  It seems to cheat by assuming `data_t' as the
> type as soon at it sees a union.  That shouldn't matter since the type
> is only used for the stupid cast.  But the better way would be to just
> get rid of the parsing in the first place, or the whole script when
> we're at it.

Yes, I'd be in favor of anything that makes the build process less
dependent on the exact mig output.  I don't know enough about what gdb
is doing to remove this script entirely, but I'll see if it still
works when using a direct cast instead of all the argument parsing.

Thanks.

David