From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121820 invoked by alias); 18 Sep 2017 15:34:42 -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 121806 invoked by uid 89); 18 Sep 2017 15:34:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=kitchen X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Sep 2017 15:34:40 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 60B4768A2; Mon, 18 Sep 2017 15:34:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 60B4768A2 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com 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 6E3D561F21; Mon, 18 Sep 2017 15:34:36 +0000 (UTC) Subject: Re: [PATCH] icc: allow code path for newer versions of icc. To: Walfred Tedeschi , qiyaoltc@gmail.com References: <1504687613-14649-1-git-send-email-walfred.tedeschi@intel.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <59e18301-7b12-2e92-6277-0aef4164243e@redhat.com> Date: Mon, 18 Sep 2017 15:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1504687613-14649-1-git-send-email-walfred.tedeschi@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-09/txt/msg00447.txt.bz2 Hi, I read the patch and I found a few nits to pick. See below. On 09/06/2017 09:46 AM, Walfred Tedeschi wrote: > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 6678b33..4aa3b3e 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -604,7 +604,7 @@ struct dwarf2_cu > unsigned int checked_producer : 1; ... > > +#if defined GDB_SELF_TEST > +#include "selftest.h" > + > +namespace selftests { > +namespace gdbserver { > +static void > +dwarf_producer_test () Simon already pointed at the gdbserver namespace. I'd like to add that it looks odd to me to define the actual functionality in utils.c and define the corresponding self tests in dwarf2read.c. Why not put the tests in utils.c as well? BTW, I'd support moving all these producer checks to a separate file (maybe producer.c), instead of putting it all in the utils.c kitchen sync, though that's a preexisting issue. > + if (producer == NULL || !startswith (producer, "Intel(R)")) > + return 0; > + > +/* Preparing the used fields. */ > + int maj, min; Missing leading double-space in comment line. > + if (major == NULL) > + major = &maj; > + if (minor == NULL) > + minor = &min; > + /* Not recognized as Intel, let user know. */ > + warning ("Intel producer: version not recognized."); > + return 0; > +} If this ever triggers, won't the user get an annoying flood of such warnings? If you hack your local copy to reach the warning, what do you see? I also wonder whether we should print a more-friend clearer message, "intel producer" may mean nothing to a user, and I'm not sure what they could do with that. Maybe at least print the actual version string? Thanks, Pedro Alves