From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) by sourceware.org (Postfix) with ESMTPS id 46EA13844010 for ; Wed, 19 Aug 2020 13:09:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 46EA13844010 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x341.google.com with SMTP id 9so2030441wmj.5 for ; Wed, 19 Aug 2020 06:09:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=qToZipCHRcROVzZzAlYD/rXkq2FNCILYmAIQyibsuCo=; b=C9l+3+rx1zwdZQoGkYvPKDkouMsFZaBPZAP7ajtbJBaunYAsItZ4Algul4RFBBSAn/ r6qx9Kgj9mVehGUCQXnVlzRdtRwf4Hd4qBEhkdwdQTIC0caAso7EKgVlXVHbtEARKh2X QEsolcC7sEumAkLXUZ9HaniW/0RXGzdIo/mKTSDC1xW8SuavKzZ12dOyiNFHRsGkP3zO EILvl7JCrLNIS6xpJDEzYRFYTV8JWpyFPxOnBwfIsOE4z/6F4dgCGCV/vaaU+nnQh55v FojhUhz8eBvGeYLbxXcyPD5Fi2fczwNbu5KfwX8X8Opd71BkJj2bDmOb5U/lxjzVZGbk bzMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=qToZipCHRcROVzZzAlYD/rXkq2FNCILYmAIQyibsuCo=; b=I4fVoC9WsMU+srOyAqpfX5ikx7gbjOzZpPVVzrtqiY7NTfY5TKsCvQHhepWFRKmSet lYCVwcfr8IyppVBfnIZndNwbTvSnc81mfkX8cChtHmNGv/sfJO73G49Fsmt59mFcZD6p hFMXy4ojeZXbWU7Ea1N4wix4h0/Ihisnd33P6tUqdNxDO0u/NWEu5y2J9tamsAyUFKsb JiK0XMMqotVbCbyeOwfeE6Fds3aaQBFIy7XZKKHMGfhPU/EkYf8cfz1VooIrfStZ4blD YgfXIjvr/534sOkEjJ2Ref/E0BFzwbe4OO3LBEoNP6zW/stNDMDyNBI+Lm8owY+58e9u H9EA== X-Gm-Message-State: AOAM533ibWbCw7Ogp5FLJY6jxFBleL3HmV4lGSo2kUkDhGO30OSZO5Go 3WEHsACPV0HE5fvAZcQlmsKVvJNpc+TvhA== X-Google-Smtp-Source: ABdhPJyzpH/bjzut1wbVRFZ+hbHQhpZcaZMEE5ll/AnKsSJZClnJhPbFzV3nwRzjnUWfvpze2Gq6ow== X-Received: by 2002:a1c:18b:: with SMTP id 133mr4911314wmb.178.1597842597143; Wed, 19 Aug 2020 06:09:57 -0700 (PDT) Received: from localhost (host86-130-161-98.range86-130.btcentralplus.com. [86.130.161.98]) by smtp.gmail.com with ESMTPSA id a10sm39877525wro.35.2020.08.19.06.09.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Aug 2020 06:09:56 -0700 (PDT) Date: Wed, 19 Aug 2020 14:09:55 +0100 From: Andrew Burgess To: "Sharma, Alok Kumar" Cc: Tom Tromey , "gdb-patches@sourceware.org" , "George, Jini Susan" , "Achra, Nitika" Subject: Re: [PATCH] Fix for incorrect breakpoint set in case of flang compiled binary Message-ID: <20200819130955.GN853475@embecosm.com> References: <20200818102903.GL853475@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 14:05:44 up 31 days, 22:20, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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, 19 Aug 2020 13:10:00 -0000 * Sharma, Alok Kumar [2020-08-18 12:53:10 +0000]: > Thanks Tom and Andrew for your comments. Hopefully addressed in the attached updated patch. > > Please find my below answers for your queries. > > > We have the files producer.{c,h} that wrap up tests just like this one. If feels like we should add a new test to those files that matches all clang backend based compilers, then make use of this new test throughout. > > I'd keep the name of the test fairly generic, the important thing here is the backend technology right, not the frontend language? So if I added a new language with a clang backend I'd likely run into the same issues. > Thanks for your suggestion. I have updated the patch for same. > > > Space before "(". > Though the code is moved, I have taken care of the comment in general. > > > Does it really start with a space? That seems weird. > Agreed but it has space at the start. > > >> +# break main for Flang compiler already breaks here > > Do you know why? > Line number 46 is the place where breakpoint to main is hit for Flang compiled binary and that looks Okey. > Below is the snippet from gdb.fortran/ > ------------------------------------------ > 16 program vla_struct > 17 type :: one > 18 integer, allocatable :: ivla (:, :, :) > 19 end type one > 20 type :: two > 21 integer, allocatable :: ivla1 (:, :, :) > 22 integer, allocatable :: ivla2 (:, :) > 23 end type two > 24 type :: three > 25 integer :: ivar > 26 integer, allocatable :: ivla (:) > 27 end type three > 28 type :: four > 29 integer, allocatable :: ivla (:) > 30 integer :: ivar > 31 end type four > 32 type :: five > 33 type(one) :: tone > 34 end type five > 35 > 36 type(one), target :: onev > 37 type(two) :: twov > 38 type(three) :: threev > 39 type(four) :: fourv > 40 type(five) :: fivev > 41 type(five) :: fivearr (2) > 42 type(five), allocatable :: fivedynarr (:) > 43 logical :: l > 44 integer :: i, j > 45 > 46 allocate (onev%ivla (11,22,33)) ! before-allocated > 47 l = allocated(onev%ivla) > 48 > ------------------------------------------ > > gdb/ChangeLog > > * amd64-tdep.c (amd64_skip_prologue): Using symbol table > to find the end of prologue for flang compiled binaries. > * arm-tdep.c (arm_skip_prologue): Likewise. > * i386-tdep.c (i386_skip_prologue): Likewise. > * producer.c (producer_is_llvm): New function. > (producer_parsing_tests): Added new tests for clang/flang. > * producer.h (producer_is_llvm): New declaration. > > gdb/testsuite/ChangeLog > > * gdb.fortran/vla-type.exp: Skip commands not required for > the Flang compiled binaries after prologue fix. This looks good to me with two minor nits fixed. > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index 768fe63bdd..59f7c9f885 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -2547,12 +2547,13 @@ amd64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc) > = skip_prologue_using_sal (gdbarch, func_addr); > struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr); > > - /* Clang always emits a line note before the prologue and another > - one after. We trust clang to emit usable line notes. */ > + /* LLVM backend (Clang/Flang) always emits a line note before the > + prologue and another one after. We trust clang to emit usable > + line notes. */ > if (post_prologue_pc > && (cust != NULL > && COMPUNIT_PRODUCER (cust) != NULL > - && startswith (COMPUNIT_PRODUCER (cust), "clang "))) > + && producer_is_llvm (COMPUNIT_PRODUCER (cust)))) > return std::max (start_pc, post_prologue_pc); > } > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 9cedcc8575..074eedb480 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -60,6 +60,8 @@ > #include "record-full.h" > #include > > +#include "producer.h" > + > #if GDB_SELF_TEST > #include "gdbsupport/selftest.h" > #endif > @@ -1351,7 +1353,7 @@ arm_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) > && (cust == NULL > || COMPUNIT_PRODUCER (cust) == NULL > || startswith (COMPUNIT_PRODUCER (cust), "GNU ") > - || startswith (COMPUNIT_PRODUCER (cust), "clang "))) > + || producer_is_llvm (COMPUNIT_PRODUCER (cust)))) > return post_prologue_pc; > > if (post_prologue_pc != 0) > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index 9b905c1996..d9fa2b9264 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c > @@ -65,6 +65,7 @@ > #include > #include > #include > +#include "producer.h" > > /* Register names. */ > > @@ -1847,12 +1848,13 @@ i386_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc) > = skip_prologue_using_sal (gdbarch, func_addr); > struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr); > > - /* Clang always emits a line note before the prologue and another > - one after. We trust clang to emit usable line notes. */ > + /* LLVM backend (Clang/Flang) always emits a line note before the > + prologue and another one after. We trust clang to emit usable > + line notes. */ > if (post_prologue_pc > && (cust != NULL > && COMPUNIT_PRODUCER (cust) != NULL > - && startswith (COMPUNIT_PRODUCER (cust), "clang "))) > + && producer_is_llvm (COMPUNIT_PRODUCER (cust)))) > return std::max (start_pc, post_prologue_pc); > } > > diff --git a/gdb/producer.c b/gdb/producer.c > index 735a928f33..fe89e7d536 100644 > --- a/gdb/producer.c > +++ b/gdb/producer.c > @@ -125,6 +125,18 @@ producer_is_icc (const char *producer, int *major, int *minor) > return false; > } > > +/* See producer.h. */ > + > +bool > +producer_is_llvm (const char *producer) > +{ > + if (producer == NULL || !(startswith (producer, "clang ") > + || startswith (producer, " F90 Flang "))) > + return false; > + > + return true; I'm really not a fan of 'if (condition) return condition' type code, I'd rather see just: return !(producer == NULL || !(startswith (producer, "clang ") || startswith (producer, " F90 Flang "))); > +} > + > #if defined GDB_SELF_TEST > namespace selftests { > namespace producer { > @@ -203,6 +215,22 @@ Version 18.0 Beta"; > SELF_CHECK (producer_is_gcc (gnu_exp, &major, &minor) > && major == 5 && minor == 0); > } > + > + { > + static const char clang_llvm_exp[] = "clang version 12.0.0 (CLANG: bld#8)"; > + int major = 0, minor = 0; > + SELF_CHECK (!producer_is_icc (clang_llvm_exp, NULL, NULL)); > + SELF_CHECK (!producer_is_gcc (clang_llvm_exp, &major, &minor)); > + SELF_CHECK (producer_is_llvm (clang_llvm_exp)); > + } > + > + { > + static const char flang_llvm_exp[] = " F90 Flang - 1.5 2017-05-01"; > + int major = 0, minor = 0; > + SELF_CHECK (!producer_is_icc (flang_llvm_exp, NULL, NULL)); > + SELF_CHECK (!producer_is_gcc (flang_llvm_exp, &major, &minor)); > + SELF_CHECK (producer_is_llvm (flang_llvm_exp)); > + } > } > } > } > diff --git a/gdb/producer.h b/gdb/producer.h > index d8974d3814..e9bc309b0c 100644 > --- a/gdb/producer.h > +++ b/gdb/producer.h > @@ -52,4 +52,8 @@ extern int producer_is_gcc (const char *producer, int *major, int *minor); > running on Intel(R) 64, Version 18.0 Beta ....". */ > extern bool producer_is_icc (const char *producer, int *major, int *minor); > > +/* Returns true if the given PRODUCER string is LLVM (clang/flang) or > + false otherwise.*/ Two whitespace after the final '.' here. Thanks, Andrew > +extern bool producer_is_llvm (const char *producer); > + > #endif > diff --git a/gdb/testsuite/gdb.fortran/vla-type.exp b/gdb/testsuite/gdb.fortran/vla-type.exp > index 925c583edc..e2b8d71b4c 100755 > --- a/gdb/testsuite/gdb.fortran/vla-type.exp > +++ b/gdb/testsuite/gdb.fortran/vla-type.exp > @@ -33,8 +33,12 @@ set int [fortran_int4] > > # Check if not allocated VLA in type does not break > # the debugger when accessing it. > -gdb_breakpoint [gdb_get_line_number "before-allocated"] > -gdb_continue_to_breakpoint "before-allocated" > +# break main for Flang compiler already breaks here > +if ![test_compiler_info "clang-*"] { > + gdb_breakpoint [gdb_get_line_number "before-allocated"] > + gdb_continue_to_breakpoint "before-allocated" > +} > + > gdb_test "print twov" " = \\\( ivla1 = , ivla2 = \\\)" \ > "print twov before allocated" > gdb_test "print twov%ivla1" " = " \ > -- > 2.17.1