From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99984 invoked by alias); 7 Feb 2018 12:11:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 99941 invoked by uid 89); 7 Feb 2018 12:11:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_FAIL,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Feb 2018 12:11:30 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 89C664075171; Wed, 7 Feb 2018 12:11:28 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id F39ABAE7A4; Wed, 7 Feb 2018 12:11:27 +0000 (UTC) Subject: Re: [PATCH v2 0/7] improve btrace enable error reporting To: "Metzger, Markus T" , "gdb-patches@sourceware.org" References: <1516976072-19282-1-git-send-email-markus.t.metzger@intel.com> <2c57146a-7111-4381-3851-bd1ff4d610b9@redhat.com> From: Pedro Alves Message-ID: Date: Wed, 07 Feb 2018 12:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-02/txt/msg00117.txt.bz2 On 02/07/2018 10:41 AM, Metzger, Markus T wrote: > Hello Pedro, > >> This LGTM, though I have a couple questions, and a nit. >> >> #1 - Where does this leave up wrt to: >> >> 'old gdb' x 'new gdbserver' >> and >> 'new gdb' x 'old gdbserver' >> >> ? > > An old gdbserver would still produce the old error responses. They would be turned > into errors on the GDB side in remote.c both for old and new GDB. No change. > > Removing remote_supports_btrace in a new GDB will defer the packet availability check > to the individual functions. The "Target does not support branch tracing" error will now > come from record_enable_btrace() instead of btrace_enble(). The old gdbserver still does > the initial availability check and will not announce the respective enabling packets. No > user-visible change. > > A new gdbserver will produce the new error messages and will always announce btrace > packets in qSupported. An old GDB will hence always ask to enable branch tracing and > the new gdbserver will always try and report the reason using the new error messages. > This will look like a new GDB. Great, thanks. I'd think it a good idea to copy/paste that to the relevant patch's git log. > > >> #2 - Where we now say >> >> + error (_("GDB does not support Intel PT.")); >> >> (and similarly for BTS) >> >> shouldn't that say something like "_This_ GDB does not", so that the user can tell >> that it's a matter of that particular build of gdb, not that GDB-the-project is lacking >> support for PT? > > I thought we meant GDB in errors to always refer to this instance of GDB. There would > hardly be an error about missing PT support if the GDB project didn't know about it. GDB can know about PT but not have support implemented, because no one wrote the support. The error message "GDB does not support Intel PT" to me sounds like what we'd say if we had written some initial support in form of commands that do nothing. How can the user tell the difference to "gdb supports PT, but you need to build it against a newer version of some library, or something like that." > > I don't mind changing the error string. Would "This GDB" be the correct wording? Or > should we refer to the configuration and say something like "GDB has not been configured > to support ..." or "GDB has been built without support for ..."? > > Both are substantially longer and not more helpful IMHO, even though they describe > more accurately what's wrong. The term "This GDB" would refer to this particular GDB > executable more clearly but I'm not sure whether this would be more helpful, either. Yeah "This GDB" was a straw man proposal, something to get the discussion started to maybe find some better warning. Off hand, I recall that in the no-expat cases, we say things like this: warning (_("Can not parse XML trace frame info; XML support " "was disabled at compile time")); warning (_("Can not parse XML memory map; XML support was disabled " "at compile time")); warning (_("Can not parse XML OS data; XML support was disabled " "at compile time")); So maybe something around "Cannot do X; Intel PT support disabled at compile time." > > >> #3 - in patch 7: >> >> Instead of: >> >> const char *filename = "/proc/sys/kernel/perf_event_paranoid"; >> >> write: >> >> static const char filename[] = ...; > > Regarding that last patch, it is checking a Debian-specific kernel feature. The upstream > kernel only knows levels -1, 0, 1, and 2. Even setting the perf_event_paranoid level to 3 > wouldn't cause any issues. > > What is our policy regarding this? Do we accept such distribution-specific checks into > upstream GDB or do we expect the Debian maintainers to maintain this patch on top > of upstream GDB? I don't think we have a written down policy. I don't mind having it in master. It helps users/developers running Debian and derivatives, and is not invasive. Thanks, Pedro Alves