[PATCH] Fix for incorrect breakpoint set in case of flang compiled binary
Andrew Burgess
andrew.burgess@embecosm.com
Wed Aug 19 13:09:55 GMT 2020
* Sharma, Alok Kumar <AlokKumar.Sharma@amd.com> [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 <algorithm>
>
> +#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 <ctype.h>
> #include <algorithm>
> #include <unordered_set>
> +#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 = <not allocated>, ivla2 = <not allocated> \\\)" \
> "print twov before allocated"
> gdb_test "print twov%ivla1" " = <not allocated>" \
> --
> 2.17.1
More information about the Gdb-patches
mailing list