This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] icc: allow code path for newer versions of icc.
Hello Simon,
Thanks for your review!
On 09/16/2017 03:49 PM, Simon Marchi wrote:
On 2017-09-06 10:46, Walfred Tedeschi wrote:
Patch adds a version checkin for workaround an icc problem.
Icc problem was fixed in version 14, and gdb code has to
reflect the fix.
This patch contains a parser for the icc string version and conditional
workaround execution. Adds also gdb self tests for the dwarf producers.
Hi Walfred,
Thanks for the patch, a few comments below.
2017-06-28 Walfred Tedeschi <walfred.tedeschi@intel.com>
gdb/ChangeLog:
* dwarf2read.c (dwarf2_cu): Remove field producer_is_icc and added
producer_is_icc_lt_14.
(producer_is_icc_lt_14): New function.
(check_producer): Added code for checking version of icc.
(producer_is_icc): Removed.
Use the present tense (Added -> Add, Removed -> Remove).
I will change, thanks!
(read_structure_type): Add a check for the later version of icc
where the issue was still not fixed.
(dwarf_producer_test): Add new unit test.
(_initialize_dwarf2_read): Register the unit test.
The spaces should be replaced with a tab in that last line.
I will double check, sometimes there is also problems with the e-mail.
* utils.c (producer_is_intel): New function added using same
signature as producer_is_gcc.
You can just say "New function.".
* utils.h (producer_is_intel): Declaration of a new function.
---
gdb/dwarf2read.c | 103
+++++++++++++++++++++++++++++++++++++++++++++++--------
gdb/utils.c | 66 +++++++++++++++++++++++++++++++++++
gdb/utils.h | 3 ++
3 files changed, 157 insertions(+), 15 deletions(-)
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 6678b33..4aa3b3e 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -604,7 +604,7 @@ struct dwarf2_cu
unsigned int checked_producer : 1;
unsigned int producer_is_gxx_lt_4_6 : 1;
unsigned int producer_is_gcc_lt_4_3 : 1;
- unsigned int producer_is_icc : 1;
+ unsigned int producer_is_icc_lt_14 : 1;
/* When set, the file that we're processing is known to have
debugging info for C++ namespaces. GCC 3.3.x did not produce
@@ -9331,6 +9331,19 @@ read_import_statement (struct die_info *die,
struct dwarf2_cu *cu)
do_cleanups (cleanups);
}
+/* For versions older than 14 ICC did not output the required
DW_AT_declaration
+ on incomplete types, but gives them a size of zero. Starting on
version 14
+ ICC is compatible with GCC. */
+
+static int
+producer_is_icc_lt_14 (struct dwarf2_cu *cu)
+{
+ if (!cu->checked_producer)
+ check_producer (cu);
+
+ return cu->producer_is_icc_lt_14;
+}
+
/* Check for possibly missing DW_AT_comp_dir with relative .debug_line
directory paths. GCC SVN r127613 (new option -fdebug-prefix-map)
fixed
this, it was first present in GCC release 4.3.0. */
@@ -12818,6 +12831,71 @@ dwarf2_record_block_ranges (struct die_info
*die, struct block *block,
}
}
+#if defined GDB_SELF_TEST
+#include "selftest.h"
You can put this at the top of the file with the other includes.
Ok! I will also change it!
+
+namespace selftests {
+namespace gdbserver {
Change that namespace name. namespace dwarf2read would make sense I
think.
Thanks for the test btw!
+static void
+dwarf_producer_test ()
+{
+ int major = 0, minor = 0;
+
+ const char *extern_f_14_1 = "Intel(R) Fortran Intel(R) 64 Compiler \
+ XE for applications running on Intel(R) 64, Version \
+ 14.0.1.074 Build 20130716";
Watch out, the spaces you use to indent the last two lines will be
included in the string, which is probably not what you want. Try this
instead (hopefully this doesn't get wrapped by my email client):
const char *extern_f_14_1 = "Intel(R) Fortran Intel(R) 64 Compiler
XE for \
applications running on Intel(R) 64, Version 14.0.1.074 Build 20130716";
Thank you! I will also do it!
+ int retval = producer_is_intel (extern_f_14_1, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 14 && minor == 1);
+ retval = producer_is_gcc (extern_f_14_1, &major, &minor);
+ SELF_CHECK (retval == 0);
+
+ const char *intern_f_14 = "Intel(R) Fortran Intel(R) 64 Compiler \
+ XE for applications running on Intel(R) 64, Version \
+ 14.0";
+ major = 0;
+ minor = 0;
+ retval = producer_is_intel (intern_f_14, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 14 && minor == 0);
+ retval = producer_is_gcc (intern_f_14, &major, &minor);
+ SELF_CHECK (retval == 0);
+
+ const char *intern_c_14 = "Intel(R) C++ Intel(R) 64 Compiler \
+ XE for applications running on Intel(R) 64, Version \
+ 14.0";
+ major = 0;
+ minor = 0;
+ retval = producer_is_intel (intern_c_14, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 14 && minor == 0);
+ retval = producer_is_gcc (intern_c_14, &major, &minor);
+ SELF_CHECK (retval == 0);
+
+ const char *intern_c_18 = "Intel(R) C++ Intel(R) 64 Compiler \
+ for applications running on Intel(R) 64, Version 18.0 Beta";
+ major = 0;
+ minor = 0;
+ retval = producer_is_intel (intern_c_18, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 18 && minor == 0);
+
+ const char *gnu = "GNU C 4.7.2";
+ major = 0;
+ minor = 0;
+ retval = producer_is_intel (gnu, &major, &minor);
+ SELF_CHECK (retval == 0);
+ retval = producer_is_gcc (gnu, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 4 && minor == 7);
+
+ const char *gnu_exp ="GNU C++14 5.0.0 20150123 (experimental)";
+ major = 0;
+ minor = 0;
+ retval = producer_is_intel (gnu_exp, &major, &minor);
+ SELF_CHECK (retval == 0);
+ retval = producer_is_gcc (gnu_exp, &major, &minor);
+ SELF_CHECK (retval == 1 && major == 5 && minor == 0);
+}
+}
+}
+#endif
+
/* Check whether the producer field indicates either of GCC < 4.6,
or the
Intel C/C++ compiler, and cache the result in CU. */
@@ -12842,8 +12920,10 @@ check_producer (struct dwarf2_cu *cu)
cu->producer_is_gxx_lt_4_6 = major < 4 || (major == 4 && minor
< 6);
cu->producer_is_gcc_lt_4_3 = major < 4 || (major == 4 && minor
< 3);
}
- else if (startswith (cu->producer, "Intel(R) C"))
- cu->producer_is_icc = 1;
+ else if (producer_is_intel (cu->producer, &major, &minor))
+ {
+ cu->producer_is_icc_lt_14 = major < 14 ;
+ }
Remove the curly braces and the space before the semi-colon.
else
{
/* For other non-GCC compilers, expect their behavior is DWARF
version
@@ -13584,17 +13664,6 @@ quirk_gcc_member_function_pointer (struct
type *type, struct objfile *objfile)
smash_to_methodptr_type (type, new_type);
}
-/* Return non-zero if the CU's PRODUCER string matches the Intel
C/C++ compiler
- (icc). */
-
-static int
-producer_is_icc (struct dwarf2_cu *cu)
-{
- if (!cu->checked_producer)
- check_producer (cu);
-
- return cu->producer_is_icc;
-}
/* Called when we find the DIE that starts a structure or union scope
(definition) to create a type for the structure or union. Fill in
@@ -13700,7 +13769,7 @@ read_structure_type (struct die_info *die,
struct dwarf2_cu *cu)
TYPE_LENGTH (type) = 0;
}
- if (producer_is_icc (cu) && (TYPE_LENGTH (type) == 0))
+ if (producer_is_icc_lt_14 (cu) && (TYPE_LENGTH (type) == 0))
{
/* ICC does not output the required DW_AT_declaration
on incomplete types, but gives them a size of zero. */
@@ -24207,4 +24276,8 @@ Usage: save gdb-index DIRECTORY"),
&dwarf2_block_frame_base_locexpr_funcs);
dwarf2_loclist_block_index = register_symbol_block_impl (LOC_BLOCK,
&dwarf2_block_frame_base_loclist_funcs);
+
+#if defined GDB_SELF_TEST
+ register_self_test (selftests::gdbserver::dwarf_producer_test);
+#endif
Just a heads up, you'll have to modify this to give the test a name.
I suggest "dwarf-producer".
}
diff --git a/gdb/utils.c b/gdb/utils.c
index af50cf0..4432318 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3036,6 +3036,72 @@ producer_is_gcc (const char *producer, int
*major, int *minor)
return 0;
}
+/* Returns nonzero if the given PRODUCER string is Intel or zero
+ otherwise. Sets the MAJOR and MINOR versions when not NULL.
+
+ Internal and external versions have to be taken into account.
+ Before a public release string for the PRODUCER is slightly
+ different than the public one. Internal releases have mainly
+ a major release number and 0 as minor release. External
+ releases have 4 fields, 3 of them are not 0 and only two
+ are of interest, major and update.
The syntax is a bit strange. Suggested wording:
Internal and external versions have to be taken into account.
PRODUCER strings for internal releases are slightly different
than for public ones. Internal releases have a major release
number and 0 as minor release. External releases have 4
fields, 3 of them are not 0 and only two are of interest,
major and update.
Accepted, Thanks!
+
+ Examples are:
+
+ Public release:
+ "Intel(R) Fortran Intel(R) 64 Compiler XE for applications
+ running on Intel(R) 64, Version 14.0.1.074 Build 20130716";
+ "Intel(R) C++ Intel(R) 64 Compiler XE for applications
+ running on Intel(R) 64, Version 14.0.1.074 Build 20130716";
+
+ Internal releases:
+ "Intel(R) C++ Intel(R) 64 Compiler for applications
+ running on Intel(R) 64, Version 18.0 Beta ....". */
+
+int
+producer_is_intel (const char *producer, int *major, int *minor)
The return value should be bool, and the code should use true/false
instead of 1/0. The comment above should also be updated.
Ok! I think i have copied from the GCC code. Nevertheless, your
reasoning makes sense.
+{
+ if (producer == NULL || !startswith (producer, "Intel(R)"))
+ return 0;
+
+/* Preparing the used fields. */
+ int maj, min;
+ if (major == NULL)
+ major = &maj;
+ if (minor == NULL)
+ minor = &min;
+
+ *minor = 0;
+ *major = 0;
+
+ /* Consumes the string till a "Version" is found. */
+ const char *cs = strstr(producer, "Version");
Missing space after strstr.
Sorry!
+
+ while (*cs && !isspace (*cs))
+ cs++;
I suggest using skip_to_space (from common-utils.c).
I will change it!
+ if (*cs && isspace (*cs))
+ cs++;
That could be skip_spaces (from common-utils.c as well).
Actually, instead of doing this, you could also include "Version " in
the sscanf format string below. You wouldn't need to skip anything.
Yes, I will try that! Thank you!
+
+ int intermediate = 0;
+ int nof = sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor);
+
+ /* Internal versions are represented only as MAJOR.MINOR, whereas
Do you mean "where" instead of "whereas"?
+ minor is usually 0.
+ Public versions have 4 fields as described with the command
above. */
+ if (nof == 3)
+ return 1;
+
+ if (nof == 2)
+ {
+ *minor = intermediate;
+ return 1;
+ }
+
+ /* Not recognized as Intel, let user know. */
+ warning ("Intel producer: version not recognized.");
+ return 0;
+}
+
/* Helper for make_cleanup_free_char_ptr_vec. */
static void
diff --git a/gdb/utils.h b/gdb/utils.h
index 3ceefc1..1d8caae 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -453,6 +453,9 @@ extern pid_t wait_to_die_with_timeout (pid_t pid,
int *status, int timeout);
extern int producer_is_gcc_ge_4 (const char *producer);
extern int producer_is_gcc (const char *producer, int *major, int
*minor);
+/* See documentation in the utils.c file. */
+extern int producer_is_intel (const char *producer, int *major, int
*minor);
The documentation of the function should be done the other way, the
actual doc in the header file, and
/* See utils.h. */
in utils.c.
I think it would make more sense to call it "producer_is_icc", since
we talk about the compiler and not the company.
Indeed!
+
extern int myread (int, char *, int);
/* Ensure that V is aligned to an N byte boundary (B's assumed to be a
Thanks,
Simon
Thank you!
Fred
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928