From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A6EFE38618AA for ; Wed, 16 Sep 2020 20:21:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A6EFE38618AA Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 2D8B11E96F; Wed, 16 Sep 2020 16:21:44 -0400 (EDT) Subject: Re: [PATCH] arc: Add support for Linux coredump files To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Shahab Vahedi , Anton Kolesov , Francois Bedard References: <20200827112728.4275-1-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: <18b98a56-e3cf-e05b-49a7-bd6e1f61aefb@simark.ca> Date: Wed, 16 Sep 2020 16:21:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200827112728.4275-1-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Sep 2020 20:21:46 -0000 On 2020-08-27 7:27 a.m., Shahab Vahedi via Gdb-patches wrote: > From: Anton Kolesov > > With the implemenations in this patch, ARC gdb can handle > coredump related matters. The binutils counter part of > this patch has already been pushed [1]. > > [1] arc: Add support for ARC HS extra registers in core files > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=2745674244d6aecddcf636475034bdb9c0a6b4a0 Hi Shahab, I noted a few comments, the patch is OK to push with these fixed. I think this would be relatively safe to push to the gdb-10-branch branch as well if you want, it's all new features, it doesn't touch existing features. > @@ -227,6 +281,142 @@ arc_linux_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc) > } > } > > +void > +arc_linux_supply_gregset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *gregs, size_t size) > +{ > + gdb_static_assert (ARC_LAST_REGNUM > + <= ARRAY_SIZE (arc_linux_core_reg_offsets)); Can you explain why this is <= and not < ? I'm not saying it's wrong, but it looks unusual. If ARC_LAST_REGNUM was 2 (meaning there are 3 registers: 0, 1 and 2), then having an offsets array with size 2 would be bad, would it? Or maybe I'm just confused. > + > + const bfd_byte *buf = (const bfd_byte *) gregs; > + > + for (int reg = 0; reg <= ARC_LAST_REGNUM; reg++) > + { > + if (arc_linux_core_reg_offsets[reg] != ARC_OFFSET_NO_REGISTER) > + regcache->raw_supply (reg, buf + arc_linux_core_reg_offsets[reg]); > + } > +} > + > +void > +arc_linux_supply_v2_regset (const struct regset *regset, > + struct regcache *regcache, int regnum, > + const void *v2_regs, size_t size) > +{ > + const bfd_byte *buf = (const bfd_byte *) v2_regs; > + > + /* user_regs_arcv2 is defined in linux arch/arc/include/uapi/asm/ptrace.h. */ > + regcache->raw_supply (ARC_R30_REGNUM, buf); > + regcache->raw_supply (ARC_R58_REGNUM, buf + REGOFF (1)); > + regcache->raw_supply (ARC_R59_REGNUM, buf + REGOFF (2)); > +} > + > +/* Populate BUF with register REGNUM from the REGCACHE. */ > + > +static void > +collect_register(const struct regcache *regcache, struct gdbarch *gdbarch, Space before parenthesis. > + int regnum, gdb_byte *buf) > +{ > + /* Skip non-existing registers. */ > + if ((arc_linux_core_reg_offsets[regnum] == ARC_OFFSET_NO_REGISTER)) > + return; > + > + /* The Address where the execution has stopped is in pseudo-register Address -> address > + STOP_PC. However, when kernel code is returning from the exception, > + it uses the value from ERET register. Since, TRAP_S (the breakpoint > + instruction) commits, the ERET points to the next instruction. In > + other words: ERET != STOP_PC. To jump back from the kernel code to > + the correct address, ERET must be overwritten by GDB's STOP_PC. Else, > + the program will continue at the address after the current instruction. > + */ > + if (regnum == gdbarch_pc_regnum (gdbarch)) > + { > + int eret_offset = REGOFF (6); That REGOFF (6) looks a bit magical. Defining a constant would be clearer. > +/* Implement the `core_read_description` gdbarch method. */ > + > +static const struct target_desc * > +arc_linux_core_read_description (struct gdbarch *gdbarch, > + struct target_ops *target, > + bfd *abfd) > +{ > + arc_gdbarch_features features > + = arc_gdbarch_features_create (abfd, > + gdbarch_bfd_arch_info (gdbarch)->mach); 8 spaces -> tab Simon