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

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

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