Re: [PATCH] Fix for incorrect breakpoint set in case of flang compiled binary

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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