Thanks Andrew. Please find the updated patch. I shall be committing this. 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. Regards, Alok -----Original Message----- From: Andrew Burgess Sent: Wednesday, August 19, 2020 6:40 PM 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 [CAUTION: External Email] * 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