From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93957 invoked by alias); 18 Sep 2017 14:17:20 -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 93236 invoked by uid 89); 18 Sep 2017 14:17:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*M:8176 X-HELO: mga06.intel.com Received: from mga06.intel.com (HELO mga06.intel.com) (134.134.136.31) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Sep 2017 14:17:16 +0000 Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 18 Sep 2017 07:17:13 -0700 X-ExtLoop1: 1 Received: from labpc1530.iul.intel.com ([172.28.48.195]) by fmsmga004.fm.intel.com with ESMTP; 18 Sep 2017 07:17:12 -0700 Subject: Re: [PATCH] icc: allow code path for newer versions of icc. To: Simon Marchi References: <1504687613-14649-1-git-send-email-walfred.tedeschi@intel.com> Cc: palves@redhat.com, qiyaoltc@gmail.com, gdb-patches@sourceware.org From: "Tedeschi, Walfred" Message-ID: <2a90f3d1-8176-7c98-a5d8-525a11c0c269@intel.com> Date: Mon, 18 Sep 2017 14:17: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: Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00444.txt.bz2 Hello Simon, Thanks for your review! On 09/16/2017 03:49 PM, Simon Marchi wrote: > On 2017-09-06 10:46, Walfred Tedeschi wrote: >> Patch adds a version checkin for workaround an icc problem. >> Icc problem was fixed in version 14, and gdb code has to >> reflect the fix. >> This patch contains a parser for the icc string version and conditional >> workaround execution. Adds also gdb self tests for the dwarf producers. > > Hi Walfred, > > Thanks for the patch, a few comments below. > >> >> 2017-06-28 Walfred Tedeschi >> >> gdb/ChangeLog: >> * dwarf2read.c (dwarf2_cu): Remove field producer_is_icc and added >> producer_is_icc_lt_14. >> (producer_is_icc_lt_14): New function. >> (check_producer): Added code for checking version of icc. >> (producer_is_icc): Removed. > > Use the present tense (Added -> Add, Removed -> Remove). I will change, thanks! > >> (read_structure_type): Add a check for the later version of icc >> where the issue was still not fixed. >> (dwarf_producer_test): Add new unit test. >> (_initialize_dwarf2_read): Register the unit test. > > The spaces should be replaced with a tab in that last line. I will double check, sometimes there is also problems with the e-mail. > >> * utils.c (producer_is_intel): New function added using same >> signature as producer_is_gcc. > > You can just say "New function.". > >> * utils.h (producer_is_intel): Declaration of a new function. >> >> --- >> gdb/dwarf2read.c | 103=20 >> +++++++++++++++++++++++++++++++++++++++++++++++-------- >> gdb/utils.c | 66 +++++++++++++++++++++++++++++++++++ >> gdb/utils.h | 3 ++ >> 3 files changed, 157 insertions(+), 15 deletions(-) >> >> 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; >> unsigned int producer_is_gxx_lt_4_6 : 1; >> unsigned int producer_is_gcc_lt_4_3 : 1; >> - unsigned int producer_is_icc : 1; >> + unsigned int producer_is_icc_lt_14 : 1; >> >> /* When set, the file that we're processing is known to have >> debugging info for C++ namespaces. GCC 3.3.x did not produce >> @@ -9331,6 +9331,19 @@ read_import_statement (struct die_info *die, >> struct dwarf2_cu *cu) >> do_cleanups (cleanups); >> } >> >> +/* For versions older than 14 ICC did not output the required=20 >> DW_AT_declaration >> + on incomplete types, but gives them a size of zero. Starting on=20 >> version 14 >> + ICC is compatible with GCC. */ >> + >> +static int >> +producer_is_icc_lt_14 (struct dwarf2_cu *cu) >> +{ >> + if (!cu->checked_producer) >> + check_producer (cu); >> + >> + return cu->producer_is_icc_lt_14; >> +} >> + >> /* Check for possibly missing DW_AT_comp_dir with relative .debug_line >> directory paths. GCC SVN r127613 (new option -fdebug-prefix-map)=20 >> fixed >> this, it was first present in GCC release 4.3.0. */ >> @@ -12818,6 +12831,71 @@ dwarf2_record_block_ranges (struct die_info >> *die, struct block *block, >> } >> } >> >> +#if defined GDB_SELF_TEST >> +#include "selftest.h" > > You can put this at the top of the file with the other includes. > Ok! I will also change it! >> + >> +namespace selftests { >> +namespace gdbserver { > > Change that namespace name. namespace dwarf2read would make sense I=20 > think. > > Thanks for the test btw! > >> +static void >> +dwarf_producer_test () >> +{ >> + int major =3D 0, minor =3D 0; >> + >> + const char *extern_f_14_1 =3D "Intel(R) Fortran Intel(R) 64 Compiler \ >> + XE for applications running on Intel(R) 64, Version \ >> + 14.0.1.074 Build 20130716"; > > Watch out, the spaces you use to indent the last two lines will be=20 > included in the string, which is probably not what you want. Try this=20 > instead (hopefully this doesn't get wrapped by my email client): > > const char *extern_f_14_1 =3D "Intel(R) Fortran Intel(R) 64 Compiler=20 > XE for \ > applications running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; > Thank you! I will also do it! >> + int retval =3D producer_is_intel (extern_f_14_1, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 14 && minor =3D=3D 1); >> + retval =3D producer_is_gcc (extern_f_14_1, &major, &minor); >> + SELF_CHECK (retval =3D=3D 0); >> + >> + const char *intern_f_14 =3D "Intel(R) Fortran Intel(R) 64 Compiler \ >> + XE for applications running on Intel(R) 64, Version \ >> + 14.0"; >> + major =3D 0; >> + minor =3D 0; >> + retval =3D producer_is_intel (intern_f_14, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 14 && minor =3D=3D 0); >> + retval =3D producer_is_gcc (intern_f_14, &major, &minor); >> + SELF_CHECK (retval =3D=3D 0); >> + >> + const char *intern_c_14 =3D "Intel(R) C++ Intel(R) 64 Compiler \ >> + XE for applications running on Intel(R) 64, Version \ >> + 14.0"; >> + major =3D 0; >> + minor =3D 0; >> + retval =3D producer_is_intel (intern_c_14, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 14 && minor =3D=3D 0); >> + retval =3D producer_is_gcc (intern_c_14, &major, &minor); >> + SELF_CHECK (retval =3D=3D 0); >> + >> + const char *intern_c_18 =3D "Intel(R) C++ Intel(R) 64 Compiler \ >> + for applications running on Intel(R) 64, Version 18.0 Beta"; >> + major =3D 0; >> + minor =3D 0; >> + retval =3D producer_is_intel (intern_c_18, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 18 && minor =3D=3D 0); >> + >> + const char *gnu =3D "GNU C 4.7.2"; >> + major =3D 0; >> + minor =3D 0; >> + retval =3D producer_is_intel (gnu, &major, &minor); >> + SELF_CHECK (retval =3D=3D 0); >> + retval =3D producer_is_gcc (gnu, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 4 && minor =3D=3D 7); >> + >> + const char *gnu_exp =3D"GNU C++14 5.0.0 20150123 (experimental)"; >> + major =3D 0; >> + minor =3D 0; >> + retval =3D producer_is_intel (gnu_exp, &major, &minor); >> + SELF_CHECK (retval =3D=3D 0); >> + retval =3D producer_is_gcc (gnu_exp, &major, &minor); >> + SELF_CHECK (retval =3D=3D 1 && major =3D=3D 5 && minor =3D=3D 0); >> +} >> +} >> +} >> +#endif >> + >> /* Check whether the producer field indicates either of GCC < 4.6,=20 >> or the >> Intel C/C++ compiler, and cache the result in CU. */ >> >> @@ -12842,8 +12920,10 @@ check_producer (struct dwarf2_cu *cu) >> cu->producer_is_gxx_lt_4_6 =3D major < 4 || (major =3D=3D 4 && mi= nor=20 >> < 6); >> cu->producer_is_gcc_lt_4_3 =3D major < 4 || (major =3D=3D 4 && mi= nor=20 >> < 3); >> } >> - else if (startswith (cu->producer, "Intel(R) C")) >> - cu->producer_is_icc =3D 1; >> + else if (producer_is_intel (cu->producer, &major, &minor)) >> + { >> + cu->producer_is_icc_lt_14 =3D major < 14 ; >> + } > > Remove the curly braces and the space before the semi-colon. > >> else >> { >> /* For other non-GCC compilers, expect their behavior is DWARF=20 >> version >> @@ -13584,17 +13664,6 @@ quirk_gcc_member_function_pointer (struct >> type *type, struct objfile *objfile) >> smash_to_methodptr_type (type, new_type); >> } >> >> -/* Return non-zero if the CU's PRODUCER string matches the Intel=20 >> C/C++ compiler >> - (icc). */ >> - >> -static int >> -producer_is_icc (struct dwarf2_cu *cu) >> -{ >> - if (!cu->checked_producer) >> - check_producer (cu); >> - >> - return cu->producer_is_icc; >> -} >> >> /* Called when we find the DIE that starts a structure or union scope >> (definition) to create a type for the structure or union. Fill in >> @@ -13700,7 +13769,7 @@ read_structure_type (struct die_info *die, >> struct dwarf2_cu *cu) >> TYPE_LENGTH (type) =3D 0; >> } >> >> - if (producer_is_icc (cu) && (TYPE_LENGTH (type) =3D=3D 0)) >> + if (producer_is_icc_lt_14 (cu) && (TYPE_LENGTH (type) =3D=3D 0)) >> { >> /* ICC does not output the required DW_AT_declaration >> on incomplete types, but gives them a size of zero. */ >> @@ -24207,4 +24276,8 @@ Usage: save gdb-index DIRECTORY"), >> &dwarf2_block_frame_base_locexpr_funcs); >> dwarf2_loclist_block_index =3D register_symbol_block_impl (LOC_BLOCK, >> &dwarf2_block_frame_base_loclist_funcs); >> + >> +#if defined GDB_SELF_TEST >> + register_self_test (selftests::gdbserver::dwarf_producer_test); >> +#endif > > Just a heads up, you'll have to modify this to give the test a name.=20=20 > I suggest "dwarf-producer". > >> } >> diff --git a/gdb/utils.c b/gdb/utils.c >> index af50cf0..4432318 100644 >> --- a/gdb/utils.c >> +++ b/gdb/utils.c >> @@ -3036,6 +3036,72 @@ producer_is_gcc (const char *producer, int >> *major, int *minor) >> return 0; >> } >> >> +/* Returns nonzero if the given PRODUCER string is Intel or zero >> + otherwise. Sets the MAJOR and MINOR versions when not NULL. >> + >> + Internal and external versions have to be taken into account. >> + Before a public release string for the PRODUCER is slightly >> + different than the public one. Internal releases have mainly >> + a major release number and 0 as minor release. External >> + releases have 4 fields, 3 of them are not 0 and only two >> + are of interest, major and update. > > The syntax is a bit strange. Suggested wording: > > Internal and external versions have to be taken into account. > PRODUCER strings for internal releases are slightly different > than for public ones. Internal releases have a major release > number and 0 as minor release. External releases have 4 > fields, 3 of them are not 0 and only two are of interest, > major and update. > Accepted, Thanks! >> + >> + Examples are: >> + >> + Public release: >> + "Intel(R) Fortran Intel(R) 64 Compiler XE for applications >> + running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; >> + "Intel(R) C++ Intel(R) 64 Compiler XE for applications >> + running on Intel(R) 64, Version 14.0.1.074 Build 20130716"; >> + >> + Internal releases: >> + "Intel(R) C++ Intel(R) 64 Compiler for applications >> + running on Intel(R) 64, Version 18.0 Beta ....". */ >> + >> +int >> +producer_is_intel (const char *producer, int *major, int *minor) > > The return value should be bool, and the code should use true/false=20 > instead of 1/0. The comment above should also be updated. > Ok! I think i have copied from the GCC code. Nevertheless, your=20 reasoning makes sense. >> +{ >> + if (producer =3D=3D NULL || !startswith (producer, "Intel(R)")) >> + return 0; >> + >> +/* Preparing the used fields. */ >> + int maj, min; >> + if (major =3D=3D NULL) >> + major =3D &maj; >> + if (minor =3D=3D NULL) >> + minor =3D &min; >> + >> + *minor =3D 0; >> + *major =3D 0; >> + >> + /* Consumes the string till a "Version" is found. */ >> + const char *cs =3D strstr(producer, "Version"); > > Missing space after strstr. > Sorry! >> + >> + while (*cs && !isspace (*cs)) >> + cs++; > > I suggest using skip_to_space (from common-utils.c). > I will change it! >> + if (*cs && isspace (*cs)) >> + cs++; > > That could be skip_spaces (from common-utils.c as well). > > Actually, instead of doing this, you could also include "Version " in=20 > the sscanf format string below. You wouldn't need to skip anything. > Yes, I will try that! Thank you! >> + >> + int intermediate =3D 0; >> + int nof =3D sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor); >> + >> + /* Internal versions are represented only as MAJOR.MINOR, whereas > > Do you mean "where" instead of "whereas"? > >> + minor is usually 0. >> + Public versions have 4 fields as described with the command=20 >> above. */ >> + if (nof =3D=3D 3) >> + return 1; >> + >> + if (nof =3D=3D 2) >> + { >> + *minor =3D intermediate; >> + return 1; >> + } >> + >> + /* Not recognized as Intel, let user know. */ >> + warning ("Intel producer: version not recognized."); >> + return 0; >> +} >> + >> /* Helper for make_cleanup_free_char_ptr_vec. */ >> >> static void >> diff --git a/gdb/utils.h b/gdb/utils.h >> index 3ceefc1..1d8caae 100644 >> --- a/gdb/utils.h >> +++ b/gdb/utils.h >> @@ -453,6 +453,9 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, >> int *status, int timeout); >> extern int producer_is_gcc_ge_4 (const char *producer); >> extern int producer_is_gcc (const char *producer, int *major, int=20 >> *minor); >> >> +/* See documentation in the utils.c file. */ >> +extern int producer_is_intel (const char *producer, int *major, int=20 >> *minor); > > The documentation of the function should be done the other way, the=20 > actual doc in the header file, and > > /* See utils.h. */ > > in utils.c. > > I think it would make more sense to call it "producer_is_icc", since=20 > we talk about the compiler and not the company. > Indeed! > >> + >> extern int myread (int, char *, int); >> >> /* Ensure that V is aligned to an N byte boundary (B's assumed to be a > > Thanks, > > Simon Thank you! Fred Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928