Re: [PATCH 2/3 v2] Define _KMEMUSER in arm-nbsd-nat.c

[Kamil Rytarowski <n54@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4498 bytes --]

On 24.01.2020 16:35, Christian Biesinger via gdb-patches wrote:
> On Fri, Jan 24, 2020 at 4:23 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>
>> On 24.01.2020 15:53, Christian Biesinger via gdb-patches wrote:
>>> Hi Kamil,
>>>
>>> I have a related question. NetBSD applied this patch:
>>> https://www.mail-archive.com/tech@openbsd.org/msg44100.html
>>>
>>
>> Is this the right link?
> 
> Yeah -- that patch changes a system header at the top and patches GDB
> at the bottom.
> 

This is not a change in NetBSD, so it is unrelated.

>>> Do you know which NetBSD version that shipped in? Can we apply that
>>> patch to GDB as-is or should we attempt to support the older struct
>>> layout as well?
>>
>> Please go for the current FPU layout on NetBSD. Massive ptrace(2) fixes
>> were introduced in NetBSD-8 and later. Soon NetBSD 7.x will go EOL
>> (after releasing 9.0, rc2 is planned soon).
> 
> OK, great. Thanks.
> 
>> In LLDB we support NetBSD 9.0 or newer. In GDB we should keep the same
>> minimal requirements and deal with older NetBSD versions (if at all)
>> with downstream patches.
>>
>> We have got a pile of local GDB patches.
> 
> OK. Maybe I should look through those at some point...  I was
> surprised that NetBSD apparently has an oldish GDB if
> http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/devel/gdb/README.html
> is correct (8.1)
> 
>> There is also a functional gdbserver implementation on NetBSD/amd64 and
>> I intend to upstream it. (Help wanted! Would you be interested in this
>> and in upstreaming?)
>>
>> The patches are located here:
>>
>> https://github.com/NetBSD/pkgsrc-wip/tree/master/gdb-netbsd/patches
>>
>> * with core/basic features... but it is difficult as there is no OS with
>> finished transition...
>> https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
> 
> I can definitely not commit to upstreaming the gdbserver. I am only
> looking at NetBSD because I wanted to remove a deprecated function in
> GDB, and one of the two callers is in NetBSD ARM code. So, I wanted to
> build ARM NetBSD first so I can test if it still works after that
> change. But I can't commit to any further NetBSD work.
> 

OK, thanks!

> BTW, is there a reason why your patches have one .patch per changed
> file? I usually find it easier to follow them if they are instead
> grouped by some kind of topic per patch.
> 

This is a convention in pkgsrc and it is practical for its use-case.

> Thanks,
> Christian
> 
>>
>>>
>>> Thanks,
>>> Christian
>>>
>>> On Fri, Jan 24, 2020 at 3:29 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>>>
>>>> On 24.01.2020 15:18, cbiesinger@chromium.org wrote:
>>>>> From: Christian Biesinger <cbiesinger@google.com>
>>>>>
>>>>> Fixes the below compile error on ARM NetBSD 9.0_RC1 (the only version I
>>>>> tested).  types.h does not define register_t by default.
>>>>>
>>>>> We already use this define elsewhere, notably in bsd-kvm.c.
>>>>>
>>>>> In file included from ../../gdb/arm-nbsd-nat.c:28:
>>>>> /usr/include/machine/frame.h:54:2: error: unknown type name 'register_t'; did you mean '__register_t'?
>>>>>         register_t tf_spsr;
>>>>>         ^
>>>>> /usr/include/machine/types.h:77:14: note: '__register_t' declared here
>>>>> typedef int             __register_t;
>>>>>                         ^
>>>>>
>>>>> There are other compile errors that this does not fix.
>>>>>
>>>>> gdb/ChangeLog:
>>>>>
>>>>> 2020-01-24  Christian Biesinger  <cbiesinger@google.com>
>>>>>
>>>>>       * arm-nbsd-nat.c: Define _KMEMUSER to get the declaration of
>>>>>       register_t.
>>>>>
>>>>> Change-Id: I82c21d38189ee59ea0af2538ba84b771d268722e
>>>>> ---
>>>>>  gdb/arm-nbsd-nat.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/gdb/arm-nbsd-nat.c b/gdb/arm-nbsd-nat.c
>>>>> index 00f919194b..4844b51a3c 100644
>>>>> --- a/gdb/arm-nbsd-nat.c
>>>>> +++ b/gdb/arm-nbsd-nat.c
>>>>> @@ -17,6 +17,8 @@
>>>>>     You should have received a copy of the GNU General Public License
>>>>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>>>>
>>>>> +/* We define this to get types like register_t.  */
>>>>> +#define _KMEMUSER
>>>>>  #include "defs.h"
>>>>>  #include "gdbcore.h"
>>>>>  #include "inferior.h"
>>>>>
>>>>
>>>> While gdb is the right user for _KMEMUSER, here we should probably go
>>>> for -D_KERNTYPES as it is the canonical symbol for register_t.
>>>>
>>
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]