RE: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

["Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>]

Pedro,

If I understood you right you are in favour of keeping the siginfo as global one?
In case this is correct, I will start working on the code you pointed out.

What I could see from the glibc code the structure is reset via memset.

Thanks a lot for your feedback! 
Muito obrigado!

Best regards,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Thursday, November 19, 2015 2:28 PM
To: Tedeschi, Walfred; Joel Brobecker; Yao Qi <yao@codesourcery.com> (yao@codesourcery.com)
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

On 11/19/2015 09:51 AM, Tedeschi, Walfred wrote:

> I understood from the kernel and glibc patches that the kernel is 
> generic (all architectures) and the glibc is added for Intel only.

IIRC, not all architectures in the kernel use /include/uapi/asm-generic/siginfo.h, and some have their own custom siginfo.h.  Those should have a custom gdbarch_get_siginfo_type hook in gdb as well, but nobody has ever bothered to fix that this far, supposedly because the siginfo tests in the testsuite don't depend in bits of the siginfo objects that might differ on such architectures.

It does look a bit odd to me to expose these new fields on all architectures, given that this is Intel/x86-only.  But it doesn't _really_ hurt, other than potentially being a little bit confusing.

>  
> Finally I think it is also better to describe a bit what happens with the new fields in the kernel side.
> 
> In case we have a bound violation, the kernel does a disassemble of 
> the current instruction, Reads the values of the operands of the compare bound instructions and set the _low and _upper bound.
> At the same time it sets the sig_code to 3. I.e. Those fields are only valid if the sigcode is 3.
> 
> 1. Make a mechanism to determine which siginfo version to be used depending from the system being debugged.
> I am not sure if there is such a mechanism in GDB already or how error 
> prune it would be to be able to detect pre presence of these fields 
> from the kernel side and glibc (in the case of the glibc also taking into account the architecture).

As alluded to above, the 'get_siginfo_type' method can be set different for each gdbarch.
So x86, arm, etc. each can tailor the siginfo type to their needs by installing a different hook.  In fact, it's what we did prior to 5cd867b414fe, except that before that commit, all archs were actually installing the same hook.

Checking the kernel version would be more complicated, considering remote debugging.  We could have gdbserver probe for support for the feature, and then report it in qSupported at connection time, for example, or even have the server fully describe the layout of the object (similar to how it can describe register types, which you used in gdb/features/i386/64bit-mpx.xml), though I don't think either would be warranted here.
The siginfo object's layout and size is ABI, the kernel can't change it.  Even though new fields have been added to the end of struct _sigfault, the _sigfault field itself is part of a union, which is padded to SI_PAD_SIZE bytes.  That is, this change did not change the siginfo object's size.  Since we're only supposed to interpret the contents of the new elements when si_code is 3 and the kernel never set si_code to 3 before, the change is also fully backwards compatible.

> 
> 2. Set the field _low and _upper to 0 in case the sigcode is not 3.

I'd just leave them be.  You always have to know how to interpret the siginfo object's elements, and know which enum field to look at between siginfo._sifields.{_kill|_timer|_sigchld|etc.}, depending on si_code and si_signo.  Doesn't the kernel of glibc already always memset the whole object before filling in the valid bits, BTW?


Lastly, I think you missed tweaking amd64_linux_siginfo_fixup & co in order to correctly convert these fields when debugging 32-bit and x32 programs on a 64-bit kernel.  Likewise the gdbserver equivalents.


Thanks,
Pedro Alves

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928