[2/2] gdb: Allow prologue detection via symbols for Intel compilers.

Message ID 20210407151209.2261068-3-felix.willgerodt@intel.com
State New
Headers show
Series
  • Allow dwarf based prologue detection for Intel compilers.
Related show

Commit Message

Kumar N, Bhuvanendra via Gdb-patches April 7, 2021, 3:12 p.m.
The next-gen Intel Fortran compiler isn't flang-based, but emits
prologue_end in the same manner.  As do the newer Intel C/C++ compilers.
This allows prologue detection based on dwarf for all newer Intel compilers.
The cut-off version was not chosen for any specific reason other than the
effort to test this.

gdb/Changelog:
2021-03-15  Felix Willgerodt  <felix.willgerodt@intel.com>

    	* i386-tdep.c (i386_skip_prologue): Use symbol table to find the
    	prologue end for Intel compilers.
    	* amd64-tdep.c (amd64_skip_prologue): Likewise.
    	* producer.c (producer_is_icc_ge_19): New function.
    	* producer.h (producer_is_icc_ge_19): New declaration.
---
 gdb/amd64-tdep.c |  9 +++++----
 gdb/i386-tdep.c  |  9 +++++----
 gdb/producer.c   | 12 ++++++++++++
 gdb/producer.h   |  3 +++
 4 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.25.4

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Comments

Andrew Burgess April 7, 2021, 8:25 p.m. | #1
* Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> [2021-04-07 17:12:09 +0200]:

> The next-gen Intel Fortran compiler isn't flang-based, but emits

> prologue_end in the same manner.  As do the newer Intel C/C++ compilers.

> This allows prologue detection based on dwarf for all newer Intel compilers.

> The cut-off version was not chosen for any specific reason other than the

> effort to test this.

> 

> gdb/Changelog:

> 2021-03-15  Felix Willgerodt  <felix.willgerodt@intel.com>

> 

>     	* i386-tdep.c (i386_skip_prologue): Use symbol table to find the

>     	prologue end for Intel compilers.

>     	* amd64-tdep.c (amd64_skip_prologue): Likewise.

>     	* producer.c (producer_is_icc_ge_19): New function.

>     	* producer.h (producer_is_icc_ge_19): New declaration.


LGTM.

Thanks,
Andrew


> ---

>  gdb/amd64-tdep.c |  9 +++++----

>  gdb/i386-tdep.c  |  9 +++++----

>  gdb/producer.c   | 12 ++++++++++++

>  gdb/producer.h   |  3 +++

>  4 files changed, 25 insertions(+), 8 deletions(-)

> 

> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c

> index 47d00634ed8..66a7c02f534 100644

> --- a/gdb/amd64-tdep.c

> +++ b/gdb/amd64-tdep.c

> @@ -2527,13 +2527,14 @@ amd64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)

>        struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);

>  

>        /* LLVM backend (Clang/Flang) always emits a line note before the

> -	 prologue and another one after.  We trust clang to emit usable

> -	 line notes.  */

> +	 prologue and another one after.  We trust clang and newer Intel

> +	 compilers to emit usable line notes.  */

>        if (post_prologue_pc

>  	  && (cust != NULL

>  	      && COMPUNIT_PRODUCER (cust) != NULL

> -	      && producer_is_llvm (COMPUNIT_PRODUCER (cust))))

> -	return std::max (start_pc, post_prologue_pc);

> +	      && (producer_is_llvm (COMPUNIT_PRODUCER (cust))

> +	      || producer_is_icc_ge_19 (COMPUNIT_PRODUCER (cust)))))

> +        return std::max (start_pc, post_prologue_pc);

>      }

>  

>    amd64_init_frame_cache (&cache);

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c

> index 2649fad08f2..50fd2767a18 100644

> --- a/gdb/i386-tdep.c

> +++ b/gdb/i386-tdep.c

> @@ -1847,13 +1847,14 @@ i386_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)

>        struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);

>  

>        /* LLVM backend (Clang/Flang) always emits a line note before the

> -	 prologue and another one after.  We trust clang to emit usable

> -	 line notes.  */

> +	 prologue and another one after.  We trust clang and newer Intel

> +	 compilers to emit usable line notes.  */

>        if (post_prologue_pc

>  	  && (cust != NULL

>  	      && COMPUNIT_PRODUCER (cust) != NULL

> -	      && producer_is_llvm (COMPUNIT_PRODUCER (cust))))

> -	return std::max (start_pc, post_prologue_pc);

> +	      && (producer_is_llvm (COMPUNIT_PRODUCER (cust))

> +	      || producer_is_icc_ge_19 (COMPUNIT_PRODUCER (cust)))))

> +        return std::max (start_pc, post_prologue_pc);

>      }

>   

>    cache.locals = -1;

> diff --git a/gdb/producer.c b/gdb/producer.c

> index 1cda48c204a..591509fa85c 100644

> --- a/gdb/producer.c

> +++ b/gdb/producer.c

> @@ -73,6 +73,18 @@ producer_is_gcc (const char *producer, int *major, int *minor)

>    return 0;

>  }

>  

> +/* See producer.h.  */

> +

> +bool

> +producer_is_icc_ge_19 (const char *producer)

> +{

> +  int major, minor;

> +

> +  if (! producer_is_icc (producer, &major, &minor))

> +    return false;

> +

> +  return major >= 19;

> +}

>  

>  /* See producer.h.  */

>  

> diff --git a/gdb/producer.h b/gdb/producer.h

> index 9cfccd63371..d08062e3e6d 100644

> --- a/gdb/producer.h

> +++ b/gdb/producer.h

> @@ -30,6 +30,9 @@ extern int producer_is_gcc_ge_4 (const char *producer);

>     is NULL or it isn't GCC.  */

>  extern int producer_is_gcc (const char *producer, int *major, int *minor);

>  

> +/* Check for Intel compilers >= 19.0.  */

> +extern bool producer_is_icc_ge_19 (const char *producer);

> +

>  /* Returns true if the given PRODUCER string is Intel or false

>     otherwise.  Sets the MAJOR and MINOR versions when not NULL.  */

>  extern bool producer_is_icc (const char *producer, int *major, int *minor);

> -- 

> 2.25.4

> 

> Intel Deutschland GmbH

> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany

> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>

> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  

> Chairperson of the Supervisory Board: Nicole Lau

> Registered Office: Munich

> Commercial Register: Amtsgericht Muenchen HRB 186928

>
Kumar N, Bhuvanendra via Gdb-patches April 8, 2021, 7:25 a.m. | #2
>-----Original Message-----

>From: Andrew Burgess <andrew.burgess@embecosm.com> 

>

>

>LGTM.

>

>Thanks,

>Andrew


Thanks, I pushed both patches.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Tom Tromey April 15, 2021, 4:44 p.m. | #3
Felix> The next-gen Intel Fortran compiler isn't flang-based, but emits
Felix> prologue_end in the same manner.  As do the newer Intel C/C++ compilers.
Felix> This allows prologue detection based on dwarf for all newer Intel compilers.
Felix> The cut-off version was not chosen for any specific reason other than the
Felix> effort to test this.

It would be nice someday if we could switch this code to assume a good
compiler and only sniff for ones that don't emit prologues properly.

Tom
Kumar N, Bhuvanendra via Gdb-patches April 16, 2021, 11:07 a.m. | #4
>Felix> The next-gen Intel Fortran compiler isn't flang-based, but emits 

>Felix> prologue_end in the same manner.  As do the newer Intel C/C++ compilers.

>Felix> This allows prologue detection based on dwarf for all newer Intel compilers.

>Felix> The cut-off version was not chosen for any specific reason other 

>Felix> than the effort to test this.

>

>It would be nice someday if we could switch this code to assume a good compiler and only sniff for ones that don't emit prologues >properly.

>

>Tom


I totally agree! The current state has been there far too long: https://sourceware.org/legacy-ml/gdb-patches/2012-11/msg00718.html
If we would use a list of bad compilers instead - creating compiler tickets for those that we add - we would probably increase the motivation to fix things on the compiler side. It would also improve debugging of optimized code in some cases. That is just my two cents, I didn't want to challenge the status quo with this patch or promise to work on that.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Pedro Alves April 17, 2021, 5:10 p.m. | #5
Hi!

On 07/04/21 16:12, Felix Willgerodt via Gdb-patches wrote:
> +	 prologue and another one after.  We trust clang and newer Intel

> +	 compilers to emit usable line notes.  */


Please don't use "newer" in comments, since time isn't frozen.  A few years from now
these "newer" will be meaningless and potentially confusing.

Pedro Alves
Kumar N, Bhuvanendra via Gdb-patches April 19, 2021, 7:39 a.m. | #6
>On 07/04/21 16:12, Felix Willgerodt via Gdb-patches wrote:

>> +	 prologue and another one after.  We trust clang and newer Intel

>> +	 compilers to emit usable line notes.  */

>

>Please don't use "newer" in comments, since time isn't frozen.  A few years from now these "newer" will be meaningless and >potentially confusing.

>

>Pedro Alves


I get your point, I thought it would be enough here as the code below explains exactly what "newer" means:
+             || producer_is_icc_ge_19 (COMPUNIT_PRODUCER (cust)))))

I have only limited it to 19.0 and newer as a precaution and as testing all older (mostly no longer supported) compilers didn't seem very beneficial.

I can change it to:

We trust clang and Intel compilers newer than 19.0 to emit usable line notes.  

Or

We trust clang and Intel compilers to emit usable line notes.

What do you prefer? Or do you have any other suggestion?

Felix


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Tom Tromey April 19, 2021, 1:25 p.m. | #7
>> If we would use a list of bad compilers instead - creating compiler

>> tickets for those that we add - we would probably increase the

>> motivation to fix things on the compiler side.


Yeah.

>> It would also improve

>> debugging of optimized code in some cases. That is just my two cents,

>> I didn't want to challenge the status quo with this patch or promise

>> to work on that.


Understood, I am not committing to it either :)

Tom

Patch

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 47d00634ed8..66a7c02f534 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2527,13 +2527,14 @@  amd64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
       struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);
 
       /* LLVM backend (Clang/Flang) always emits a line note before the
-	 prologue and another one after.  We trust clang to emit usable
-	 line notes.  */
+	 prologue and another one after.  We trust clang and newer Intel
+	 compilers to emit usable line notes.  */
       if (post_prologue_pc
 	  && (cust != NULL
 	      && COMPUNIT_PRODUCER (cust) != NULL
-	      && producer_is_llvm (COMPUNIT_PRODUCER (cust))))
-	return std::max (start_pc, post_prologue_pc);
+	      && (producer_is_llvm (COMPUNIT_PRODUCER (cust))
+	      || producer_is_icc_ge_19 (COMPUNIT_PRODUCER (cust)))))
+        return std::max (start_pc, post_prologue_pc);
     }
 
   amd64_init_frame_cache (&cache);
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 2649fad08f2..50fd2767a18 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1847,13 +1847,14 @@  i386_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
       struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);
 
       /* LLVM backend (Clang/Flang) always emits a line note before the
-	 prologue and another one after.  We trust clang to emit usable
-	 line notes.  */
+	 prologue and another one after.  We trust clang and newer Intel
+	 compilers to emit usable line notes.  */
       if (post_prologue_pc
 	  && (cust != NULL
 	      && COMPUNIT_PRODUCER (cust) != NULL
-	      && producer_is_llvm (COMPUNIT_PRODUCER (cust))))
-	return std::max (start_pc, post_prologue_pc);
+	      && (producer_is_llvm (COMPUNIT_PRODUCER (cust))
+	      || producer_is_icc_ge_19 (COMPUNIT_PRODUCER (cust)))))
+        return std::max (start_pc, post_prologue_pc);
     }
  
   cache.locals = -1;
diff --git a/gdb/producer.c b/gdb/producer.c
index 1cda48c204a..591509fa85c 100644
--- a/gdb/producer.c
+++ b/gdb/producer.c
@@ -73,6 +73,18 @@  producer_is_gcc (const char *producer, int *major, int *minor)
   return 0;
 }
 
+/* See producer.h.  */
+
+bool
+producer_is_icc_ge_19 (const char *producer)
+{
+  int major, minor;
+
+  if (! producer_is_icc (producer, &major, &minor))
+    return false;
+
+  return major >= 19;
+}
 
 /* See producer.h.  */
 
diff --git a/gdb/producer.h b/gdb/producer.h
index 9cfccd63371..d08062e3e6d 100644
--- a/gdb/producer.h
+++ b/gdb/producer.h
@@ -30,6 +30,9 @@  extern int producer_is_gcc_ge_4 (const char *producer);
    is NULL or it isn't GCC.  */
 extern int producer_is_gcc (const char *producer, int *major, int *minor);
 
+/* Check for Intel compilers >= 19.0.  */
+extern bool producer_is_icc_ge_19 (const char *producer);
+
 /* Returns true if the given PRODUCER string is Intel or false
    otherwise.  Sets the MAJOR and MINOR versions when not NULL.  */
 extern bool producer_is_icc (const char *producer, int *major, int *minor);