diff mbox

[2/3,v2] Define _KMEMUSER in arm-nbsd-nat.c

Message ID 20200124141818.172490-1-cbiesinger@chromium.org
State New, archived
Headers

Commit Message

Christian Biesinger Jan. 24, 2020, 2:18 p.m. UTC 
  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(+)

Comments

Kamil Rytarowski Jan. 24, 2020, 2:28 p.m. UTC | #1 
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.
Terekhov, Mikhail via Gdb-patches Jan. 24, 2020, 2:31 p.m. UTC | #2 
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.

Ah thank you! I was hoping someone who knows this stuff would respond.
I will send a new patch shortly.

Christian
Terekhov, Mikhail via Gdb-patches Jan. 24, 2020, 2:53 p.m. UTC | #3 
Hi Kamil,

I have a related question. NetBSD applied this patch:
https://www.mail-archive.com/tech@openbsd.org/msg44100.html

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?

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.
>
Kamil Rytarowski Jan. 24, 2020, 3:22 p.m. UTC | #4 
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?

> 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).

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.

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

> 
> 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.
>>
Terekhov, Mikhail via Gdb-patches Jan. 24, 2020, 3:35 p.m. UTC | #5 
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.

> > 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.

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.

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.
> >>
>
>
Kamil Rytarowski Jan. 24, 2020, 3:48 p.m. UTC | #6 
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.
>>>>
>>
>>
Terekhov, Mikhail via Gdb-patches Jan. 24, 2020, 4:01 p.m. UTC | #7 
On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
>
> 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.

My apologies, I completely misread that. I'll see if I can find where
NetBSD changed their FP register data structure, or perhaps your
downstream patch will still work (although that probably has to come
from one of y'all for copyright reasons?)

> >>> 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.

OK. Some of those patches should be upstreamable very easily. For some
others it would be helpful to have a description & I guess a copyright
assignment.

I also noticed that some of those files are 0 bytes and could probably
be deleted? And this does not seem necessary:
https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-bfd_netbsd-core.c

> >>>
> >>> 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.
> >>>>
> >>
> >>
>
>
Kamil Rytarowski Jan. 24, 2020, 4:36 p.m. UTC | #8 
On 24.01.2020 17:01, Christian Biesinger wrote:
> On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>
>> 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.
> 
> My apologies, I completely misread that. I'll see if I can find where
> NetBSD changed their FP register data structure, or perhaps your
> downstream patch will still work (although that probably has to come
> from one of y'all for copyright reasons?)
> 

Please cherry-pick what you need and I will find the original author.
Many people in NetBSD have FSF papers done.

>>>>> 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.
> 
> OK. Some of those patches should be upstreamable very easily. For some
> others it would be helpful to have a description & I guess a copyright
> assignment.
> 

My intention was to revamp the general NetBSD support from native-only
to gdbserver... (same like lldb-server).. and it would result in deep
rewrite of the current code, but there is work in progress status in
Linux so it does not help.

> I also noticed that some of those files are 0 bytes and could probably
> be deleted? And this does not seem necessary:
> https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-bfd_netbsd-core.c
> 

Good catch.

>>>>>
>>>>> 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.
>>>>>>
>>>>
>>>>
>>
>>
Terekhov, Mikhail via Gdb-patches Jan. 27, 2020, 12:38 p.m. UTC | #9 
On Fri, Jan 24, 2020 at 5:37 PM Kamil Rytarowski <n54@gmx.com> wrote:
>
> On 24.01.2020 17:01, Christian Biesinger wrote:
> > On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
> >>
> >> 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.
> >
> > My apologies, I completely misread that. I'll see if I can find where
> > NetBSD changed their FP register data structure, or perhaps your
> > downstream patch will still work (although that probably has to come
> > from one of y'all for copyright reasons?)
> >
>
> Please cherry-pick what you need and I will find the original author.
> Many people in NetBSD have FSF papers done.

Thanks. I think this patch would probably be the most important since
it fixes a compile error:
https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-gdb_arm-nbsd-nat.c

However, I don't understand why arm_netbsd_core_fns /
fetch_core_registers was added. As far as I can tell, because that
struct uses bfd_target_unknown_flavour and default_core_sniffer, it
will never be used.

Christian
Terekhov, Mikhail via Gdb-patches Feb. 5, 2020, 6:02 p.m. UTC | #10 
On Mon, Jan 27, 2020 at 7:38 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> On Fri, Jan 24, 2020 at 5:37 PM Kamil Rytarowski <n54@gmx.com> wrote:
> >
> > On 24.01.2020 17:01, Christian Biesinger wrote:
> > > On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
> > >>
> > >> 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.
> > >
> > > My apologies, I completely misread that. I'll see if I can find where
> > > NetBSD changed their FP register data structure, or perhaps your
> > > downstream patch will still work (although that probably has to come
> > > from one of y'all for copyright reasons?)
> > >
> >
> > Please cherry-pick what you need and I will find the original author.
> > Many people in NetBSD have FSF papers done.
>
> Thanks. I think this patch would probably be the most important since
> it fixes a compile error:
> https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-gdb_arm-nbsd-nat.c
>
> However, I don't understand why arm_netbsd_core_fns /
> fetch_core_registers was added. As far as I can tell, because that
> struct uses bfd_target_unknown_flavour and default_core_sniffer, it
> will never be used.

Hi Kamil,

I also filed https://github.com/NetBSD/pkgsrc-wip/issues/17 on some
patches that can be dropped, and I upstreamed a couple of
trivial/obvious patches you had.

Christian
Kamil Rytarowski Feb. 6, 2020, 1:18 p.m. UTC | #11 
On 27.01.2020 13:38, Christian Biesinger via gdb-patches wrote:
> On Fri, Jan 24, 2020 at 5:37 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>
>> On 24.01.2020 17:01, Christian Biesinger wrote:
>>> On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>>>
>>>> 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.
>>>
>>> My apologies, I completely misread that. I'll see if I can find where
>>> NetBSD changed their FP register data structure, or perhaps your
>>> downstream patch will still work (although that probably has to come
>>> from one of y'all for copyright reasons?)
>>>
>>
>> Please cherry-pick what you need and I will find the original author.
>> Many people in NetBSD have FSF papers done.
> 
> Thanks. I think this patch would probably be the most important since
> it fixes a compile error:
> https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-gdb_arm-nbsd-nat.c
> 
> However, I don't understand why arm_netbsd_core_fns /
> fetch_core_registers was added. As far as I can tell, because that
> struct uses bfd_target_unknown_flavour and default_core_sniffer, it
> will never be used.
> 
> Christian
> 

This looks like a leftover after a.out. It was removed upstream and we
by a mistake keep it locally as a local modification.

commit 1736a7bd96e8927c3f889a35f9153df4fd19d833
Author: Pedro Alves <palves@redhat.com>
Date:   Fri Dec 9 16:08:49 2016 +0000

    gdb: Remove support for obsolete OSABIs and a.out

    gdb/ChangeLog:
    2016-12-09  Pedro Alves  <palves@redhat.com>

a.out should be removed.

We switched to ELF back in NetBSD 1.6/2.0 and don't have toolchain for it.

We still support running old a.out binaries and plan to do it in
foreseeable future, but attaching a debugger to these processes is not
our priority.
Kamil Rytarowski Feb. 6, 2020, 1:25 p.m. UTC | #12 
On 05.02.2020 19:02, Christian Biesinger via gdb-patches wrote:
> On Mon, Jan 27, 2020 at 7:38 AM Christian Biesinger
> <cbiesinger@google.com> wrote:
>>
>> On Fri, Jan 24, 2020 at 5:37 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>>
>>> On 24.01.2020 17:01, Christian Biesinger wrote:
>>>> On Fri, Jan 24, 2020 at 4:49 PM Kamil Rytarowski <n54@gmx.com> wrote:
>>>>>
>>>>> 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.
>>>>
>>>> My apologies, I completely misread that. I'll see if I can find where
>>>> NetBSD changed their FP register data structure, or perhaps your
>>>> downstream patch will still work (although that probably has to come
>>>> from one of y'all for copyright reasons?)
>>>>
>>>
>>> Please cherry-pick what you need and I will find the original author.
>>> Many people in NetBSD have FSF papers done.
>>
>> Thanks. I think this patch would probably be the most important since
>> it fixes a compile error:
>> https://github.com/NetBSD/pkgsrc-wip/blob/master/gdb-netbsd/patches/patch-gdb_arm-nbsd-nat.c
>>
>> However, I don't understand why arm_netbsd_core_fns /
>> fetch_core_registers was added. As far as I can tell, because that
>> struct uses bfd_target_unknown_flavour and default_core_sniffer, it
>> will never be used.
> 
> Hi Kamil,
> 
> I also filed https://github.com/NetBSD/pkgsrc-wip/issues/17 on some
> patches that can be dropped, and I upstreamed a couple of
> trivial/obvious patches you had.
> 
> Christian
> 

Thank you for your contribution!
diff mbox

Patch

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"