Re: [PATCH] gdb/MicroBlaze: Add support for native linux gdb

[Gopi Kumar Bulusu <gopi@sankhya.com>]

On Fri, May 29, 2026 at 8:26 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> Gopi Kumar Bulusu <gopi@sankhya.com> writes:
>
> > Attached is a patch to add native linux support for gdb including
> > cache target support.
>
> Thanks for the patch.
>
> I read through it and have some nits.  There's nothing very serious
> except this:
>
> > The original sources for the patch come from Xilinx/AMD Yocto git
> > repository (2025.2)
>
> Who wrote them and are they covered by the copyright assignment?
>
> This is the most important consideration, the patch can't land without
> this being clear.

This work is being performed under a contract with Xilinx/AMD as
confirmed in a different response to
Simon and therefore covered by the copyright assignment.

I will make the changes as described below and retest.

Approved for commit ?

dhanyavaadaaha
gopi

>
> > From a3328b4cdfc58783b5f290f22255b36e26a6fb14 Mon Sep 17 00:00:00 2001
> > From: Gopi Kumar Bulusu <gopi@sankhya.com>
> > Date: Thu, 7 May 2026 17:47:40 +0530
> > Subject: [PATCH] gdb/MicroBlaze: Add support for native linux gdb
>
> It's best to just git send-email rather than using an attachment.
>
> >     bfd/ChangeLog:
>
> >     * elf32-microblaze.c (microblaze_elf_grok_prstatus): New function.
> >       (microblaze_elf_grok_psinfo): Likewise.
>
> >     gdb/ChangeLog:
>
> >     * Makefile.in (HFILES_NO_SRCDIR): Add microblaze-linux-tdep.h
> >       ALLDEPFILES: Add microblaze-linux-nat.c
>
> gdb doesn't use ChangeLogs any more and this text should all be removed.
>
> > +#define MICROBLAZE_TARGET_HAS_GETREGS 0
> > +#define MICROBLAZE_TARGET_HAS_SETREGS 0
>
> This seems weird:
>
> > +/* Wrapper function around ptrace.  */
> > +
> > +static void
> > +fetch_target_gp_regs (int tid, elf_gregset_t *gregs)
> > +{
> > +  elf_greg_t *gregp = *gregs;
> > +
> > +#if MICROBLAZE_TARGET_HAS_GETREGS
> > +  if (ptrace (PTRACE_GETREGS, tid, 0, (long) gregp) < 0)
> > +    {
> > +      perror_with_name (_("Couldn't get registers"));
> > +      return;
> > +    }
> > +#error "this configuration did not work during testing"
>
> Normally we don't put new dead code into gdb.
>
> I think this and the defines could just be replaced with a comment
> explaining that PTRACE_GETREGS doesn't work on this platform.
>
> > +++ b/gdb/microblaze-linux-tdep.h
> > @@ -0,0 +1,24 @@
> > +/* Target-dependent code for GNU/Linux on MicroBlaze.
> > +
> > +   Copyright (C) 2021-2026 Free Software Foundation, Inc.
> > +
> > +   This file is part of GDB.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > +#ifndef MICROBLAZE_LINUX_TDEP_H
> > +#define MICROBLAZE_LINUX_TDEP_H
> > + /* Target descriptions.  */
> > + extern struct target_desc *tdesc_microblaze_linux;
>
> Blank line between the #define and the comment.
>
> Also this define should have a slightly different name, see
> check-include-guards.py.
>
> >         non_stack_instruction_found = 0;
> > +          cache->register_offsets[rd] = -imm;
> >         continue;
>
> Indentation looks wrong, probably not using tabs.
>
> > @@ -399,8 +400,7 @@ microblaze_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
> >      {
> >        sal = find_sal_for_pc (func_start, 0);
>
> > -      if (sal.end < func_end
> > -       && start_pc <= sal.end)
> > +      if (sal.line != 0 && sal.end <= func_end  && start_pc <= sal.end)
> >       start_pc = sal.end;
>
> No objection from me but this seems somewhat unrelated.
>
>
> >    /* Call for side effects.  */
> > -  get_frame_func (next_frame);
> > +  cache->pc = get_frame_func (next_frame);
>
> Please remove that comment as it is now wrong.
>
> > +  if (regnum == MICROBLAZE_SP_REGNUM)
> > +        regnum = 1;
>
> Wrong indentation.
>
> > +  if (regnum == -1) {
> > +    int i;
> > +
> > +    for (i = 0; i < MICROBLAZE_REDR_REGNUM; i++) {
> > +      regcache->raw_supply (i, regs + i * MICROBLAZE_REGISTER_SIZE);
> > +    }
>
> Wrong brace placement in this hunk.
>
> > +  cb (".reg", tdep->sizeof_gregset, tdep->sizeof_gregset, tdep->gregset, NULL,
> > +      cb_data);
>
> Normally in new code we're using 'nullptr' now.  Best would be to go
> through the whole patch and check for this.  You don't have to fix
> pre-existing code that is using NULL though.
>
> >    gdbarch *gdbarch
> >      = gdbarch_alloc (&info, gdbarch_tdep_up (new microblaze_gdbarch_tdep));
>
> > +  microblaze_gdbarch_tdep *tdep
> > +    = gdbarch_tdep<microblaze_gdbarch_tdep> (gdbarch);
> > +
> > +  tdep->gregset = NULL;
> > +  tdep->sizeof_gregset = 0;
> > +  tdep->fpregset = NULL;
> > +  tdep->sizeof_fpregset = 0;
>
> Here it would be better to just add inline initializers to the new
> fields in microblaze_gdbarch_tdep.  Then they'll automatically be
> initialized by the 'new'.
>
> Tom