From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by sourceware.org (Postfix) with ESMTPS id B7CA73858D35 for ; Mon, 6 Jul 2020 14:50:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B7CA73858D35 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 066Efh9r015788; Mon, 6 Jul 2020 14:50:35 GMT Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 323wacapy3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 06 Jul 2020 14:50:34 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 066Ei7EU115914; Mon, 6 Jul 2020 14:50:34 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3030.oracle.com with ESMTP id 3233pvfa88-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 06 Jul 2020 14:50:34 +0000 Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 066EoW66029059; Mon, 6 Jul 2020 14:50:33 GMT Received: from termi.oracle.com (/10.175.54.43) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 06 Jul 2020 07:50:32 -0700 From: "Jose E. Marchesi" To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] gdb: support for eBPF References: <20200703103713.7676-1-jose.marchesi@oracle.com> <20200703103713.7676-2-jose.marchesi@oracle.com> <20200703113913.GU2737@embecosm.com> Date: Mon, 06 Jul 2020 16:50:28 +0200 In-Reply-To: <20200703113913.GU2737@embecosm.com> (Andrew Burgess's message of "Fri, 3 Jul 2020 12:39:13 +0100") Message-ID: <87o8osn1e3.fsf@oracle.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9673 signatures=668680 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2007060112 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9673 signatures=668680 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 priorityscore=1501 phishscore=0 spamscore=0 mlxlogscore=999 adultscore=0 cotscore=-2147483648 suspectscore=0 impostorscore=0 bulkscore=0 mlxscore=0 clxscore=1011 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2007060112 X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP, UNPARSEABLE_RELAY 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: Mon, 06 Jul 2020 14:50:38 -0000 Hi Andrew. Thanks for the review. > + > +static unsigned int ebpf_debug_flag = 0; > + > +static void ATTRIBUTE_PRINTF (1, 2) > +ebpf_debug (const char *fmt, ...) > +{ > + if (ebpf_debug_flag) > + { > + va_list args; > + > + va_start (args, fmt); > + printf_unfiltered ("eBPF: "); > + vprintf_unfiltered (fmt, args); > + va_end (args); > + } > +} Every top level item function, variable, struct, etc, should have a comment on it, you've mostly done this, but there's a few places where these are missing, I'll not point them all out. Ok, I will add the missing descriptive comments. You might want to consider changing this into a real debug flag so that there's a 'set debug ebpf' option. For RISC-V I went one further and added sub-flags like 'set debug riscv unwinders'. Not a requirement I think, but maybe worth considering. Ok. > +/* Implement the "unwind_pc" gdbarch method. */ > + > +static CORE_ADDR > +ebpf_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame) > +{ > + return frame_unwind_register_unsigned (next_frame, > + gdbarch_pc_regnum (gdbarch)); > +} Given the implementation I think you shouldn't need this function, default_unwind_pc should do just fine for you. Ok, will remove it. > +/* Return PC of first real instruction of the function starting at > + START_PC. */ > + > +static CORE_ADDR > +ebpf_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc) > +{ > + ebpf_debug ("Skipping prologue: start_pc=%s\n", > + paddress (gdbarch, start_pc)); > + /* XXX */ You've got these 'XXX' comments though out. I think they mean different things in different places. I think these should either be removed, or converted into real text. In some places you might want to consider making use of 'internal_error (....)' so GDB will given some indication that you've run into an area what needs implementing, in other cases, just expanding the comment to indicate that the current code isn't final will be the correct thing to do. Yeah, these comments are definitely too terse :) Will fix in a subsequent version of the patches. Thanks!