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

[Pedro Alves <palves@redhat.com>]

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