* [PATCH] symlookup: improves symbol lookup when a file is specified.
2017-09-06 8:47 [PATCH] icc: allow code path for newer versions of icc Walfred Tedeschi
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
@ 2017-09-06 8:47 ` Walfred Tedeschi
2017-09-20 15:22 ` [ping][PATCH] " Tedeschi, Walfred
2017-10-09 12:42 ` [PATCH] " Pedro Alves
2017-09-16 13:49 ` [PATCH] icc: allow code path for newer versions of icc Simon Marchi
2017-09-18 15:34 ` Pedro Alves
3 siblings, 2 replies; 21+ messages in thread
From: Walfred Tedeschi @ 2017-09-06 8:47 UTC (permalink / raw)
To: palves, qiyaoltc; +Cc: gdb-patches, Walfred Tedeschi
The provided patch adds a global lookup with higher priority for the
provided file.
Usual symbol lookup from GDB follows linker logic. This is what is
desired most of the time. In the usage case a file is provided as
scope, so the lookup has to follow this priority. Failing in finding
the symbol at this scope usual path is followed.
As test case it is presented two shared objects having a global variable
with the same name but comming from different source files. Without the
patch accessing them via the file scope returns the value seen in the
first loaded shared object. Using the patch the value defined in the file
scope is the one returned.
One of the already existing test cases in print-file-var.exp starts to
fail after using this patch. In fact the value that the linker sees is
different then the one debugger can read from the shared object. In
this sense it looks like to me that the test has to be changed.
The fail is in the call:
print 'print-file-var-lib2.c'::this_version_id == v2
In this case there also in both libraries the symbol this_version_id.
During load of the libraries linker resolves the symbol as the first one
loaded and independent of the scope symbol will have the same value, as
defined in the first library loaded.
However, when defining the scope the value should represent the value
in that context. Diferent evaluations of the same symbols might also better
spot the issue in users code.
- I haven't changed the test because I wanted to hear the community
thought on the subject.
2017-07-13 Walfred Tedeschi <walfred.tedeschi@intel.com>
gdb/ChangeLog:
* symtab.c (lookup_global_symbol): Add new lookup to ensure
priority on given block.
gdb/testsuite/ChangeLog:
* gdb.base/print-file-var-dlopen-main.c: New file.
* gdb.base/print-file-var-dlopen.exp: New test based on
print-file-var.exp.
---
gdb/symtab.c | 4 +
.../gdb.base/print-file-var-dlopen-main.c | 62 +++++++++++++
gdb/testsuite/gdb.base/print-file-var-dlopen.exp | 101 +++++++++++++++++++++
3 files changed, 167 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen.exp
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8492315..920461f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -2590,6 +2590,10 @@ lookup_global_symbol (const char *name,
if (objfile != NULL)
result = solib_global_lookup (objfile, name, domain);
+ /* We still need to look on the global scope of current object file. */
+ if (result.symbol == NULL && objfile != NULL)
+ result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, name, domain);
+
/* If that didn't work go a global search (of global blocks, heh). */
if (result.symbol == NULL)
{
diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
new file mode 100644
index 0000000..98cfd97
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
@@ -0,0 +1,62 @@
+/* This testcase is part of GDB, the GNU debugger.
+ Copyright 2017 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+dummy (void)
+{
+ return 1;
+}
+
+int
+main (void)
+{
+ int (*get_version1) (void);
+ int (*get_version2) (void);
+ int v1, v2;
+
+ dummy ();
+ void *lib1 = dlopen ("print-file-var-dlopen-lib1.so", RTLD_LAZY);
+ void *lib2 = dlopen ("print-file-var-dlopen-lib2.so", RTLD_LAZY);
+
+ if (lib1 == NULL || lib2 == NULL)
+ return 1;
+
+ *(int **) (&get_version1) = dlsym (lib1, "get_version");
+ *(int **) (&get_version2) = dlsym (lib2, "get_version");
+
+ if (get_version1 != NULL
+ && get_version2 != NULL)
+ {
+ v1 = get_version1();
+ v2 = get_version2();
+ }
+
+ dummy (); /* STOP */
+ dlclose (lib1);
+ dlclose (lib2);
+ if (v1 != 104)
+ return 1;
+ /* The value returned by get_version_2 depends on the target system. */
+ if (v2 != 104 || v2 != 203)
+ return 2;
+
+ return 0;
+}
+
diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen.exp b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
new file mode 100644
index 0000000..87f89f2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
@@ -0,0 +1,101 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+if {[skip_shlib_tests]} {
+ return -1
+}
+
+set executable print-file-var-dlopen-main
+
+set lib1 "print-file-var-lib1"
+set lib2 "print-file-var-lib2"
+
+set libobj1 [standard_output_file ${lib1}.so]
+set libobj2 [standard_output_file ${lib2}.so]
+
+set lib_opts { debug additional_flags=-fPIC additional_flags=-shared}
+
+if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
+ ${libobj1} \
+ ${lib_opts} ] != "" } {
+ return -1
+}
+if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib2}.c \
+ ${libobj2} \
+ ${lib_opts} ] != "" } {
+ return -1
+}
+if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
+ [standard_output_file ${executable}] \
+ executable \
+ [list debug shlib=-ldl]]
+ != ""} {
+ return -1
+}
+
+clean_restart $executable
+
+set outdir [file dirname $libobj1]
+
+gdb_test_no_output "set env LD_LIBRARY_PATH=${outdir}/:"
+
+if ![runto_main] {
+ untested "could not run to main"
+ return -1
+}
+
+# Try printing "this_version_num" qualified with the name of the file
+# where the variables are defined. There are two global variables
+# with that name, and some systems such as GNU/Linux merge them
+# into one single entity, while some other systems such as Windows
+# keep them separate. In the first situation, we have to verify
+# that GDB does not randomly select the wrong instance, even when
+# a specific filename is used to qualified the lookup. And in the
+# second case, we have to verify that GDB does select the instance
+# defined in the given filename.
+#
+# To avoid adding target-specific code in this testcase, the program
+# sets two local variable named 'v1' and 'v2' with the value of
+# our global variables. This allows us to compare the value that
+# GDB returns for each query against the actual value seen by
+# the program itself.
+
+# Get past the initialization of variables 'v1' and 'v2'.
+
+set bp_location \
+ [gdb_get_line_number "STOP" "${executable}.c"]
+gdb_test "break $executable.c:$bp_location" \
+ "Breakpoint \[0-9\]+ at 0x\[0-9a-fA-F\]+: .*" \
+ "breapoint past v1 & v2 initialization"
+
+gdb_test "continue" \
+ "Breakpoint \[0-9\]+, main \\(\\) at.*" \
+ "continue to STOP marker"
+
+# Now check the value of this_version_id in both print-file-var-lib1.c
+# and print-file-var-lib2.c.
+
+gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
+ " = 1"
+
+gdb_test "print 'print-file-var-dlopen-lib2.c'::this_version_id == v2" \
+ " = 1"
+
+gdb_test "print 'print-file-var-dlopen-lib2.c'::get_version::test == v2" \
+ " = 1"
+
+gdb_test "print 'print-file-var-dlopen-lib1.c'::get_version::test" \
+ " = 100"
+
--
2.9.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] gdb - avx512: tests were failing due to missing memory aligment.
2017-09-06 8:47 [PATCH] icc: allow code path for newer versions of icc Walfred Tedeschi
@ 2017-09-06 8:47 ` Walfred Tedeschi
2017-09-16 13:57 ` Simon Marchi
2017-09-17 20:22 ` Yao Qi
2017-09-06 8:47 ` [PATCH] symlookup: improves symbol lookup when a file is specified Walfred Tedeschi
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Walfred Tedeschi @ 2017-09-06 8:47 UTC (permalink / raw)
To: palves, qiyaoltc; +Cc: gdb-patches, Walfred Tedeschi
Test was running on a fault during code execution. Analysis have shown
that the wrong instruction had been used. An instruction that takes
not alligned memory is more appropriated for the task.
ChangeLog:
2017-06-26 Walfred Tedeschi <walfred.tedeschi@intel.com>
gdb/ChangeLog:
* testsuite/gdb.arch/i386-avx512.c (move_zmm_data_to_reg): Use
vmovups instead vmovaps.
(move_zmm_data_to_memory): Use vmovups instead vmovaps.
---
gdb/testsuite/gdb.arch/i386-avx512.c | 128 +++++++++++++++++------------------
1 file changed, 64 insertions(+), 64 deletions(-)
diff --git a/gdb/testsuite/gdb.arch/i386-avx512.c b/gdb/testsuite/gdb.arch/i386-avx512.c
index 52c0ead..3e955a4 100644
--- a/gdb/testsuite/gdb.arch/i386-avx512.c
+++ b/gdb/testsuite/gdb.arch/i386-avx512.c
@@ -127,44 +127,44 @@ move_k_data_to_memory (void)
void
move_zmm_data_to_reg (void)
{
- asm ("vmovaps 0(%0), %%zmm0\n\t"
- "vmovaps 64(%0), %%zmm1\n\t"
- "vmovaps 128(%0), %%zmm2\n\t"
- "vmovaps 192(%0), %%zmm3\n\t"
- "vmovaps 256(%0), %%zmm4\n\t"
- "vmovaps 320(%0), %%zmm5\n\t"
- "vmovaps 384(%0), %%zmm6\n\t"
- "vmovaps 448(%0), %%zmm7\n\t"
+ asm ("vmovups 0(%0), %%zmm0 \n\t"
+ "vmovups 64(%0), %%zmm1 \n\t"
+ "vmovups 128(%0), %%zmm2 \n\t"
+ "vmovups 192(%0), %%zmm3 \n\t"
+ "vmovups 256(%0), %%zmm4 \n\t"
+ "vmovups 320(%0), %%zmm5 \n\t"
+ "vmovups 384(%0), %%zmm6 \n\t"
+ "vmovups 448(%0), %%zmm7 \n\t"
: /* no output operands */
: "r" (zmm_data));
#ifdef __x86_64__
- asm ("vmovaps 512(%0), %%zmm8\n\t"
- "vmovaps 576(%0), %%zmm9\n\t"
- "vmovaps 640(%0), %%zmm10\n\t"
- "vmovaps 704(%0), %%zmm11\n\t"
- "vmovaps 768(%0), %%zmm12\n\t"
- "vmovaps 832(%0), %%zmm13\n\t"
- "vmovaps 896(%0), %%zmm14\n\t"
- "vmovaps 960(%0), %%zmm15\n\t"
+ asm ("vmovups 512(%0), %%zmm8 \n\t"
+ "vmovups 576(%0), %%zmm9 \n\t"
+ "vmovups 640(%0), %%zmm10 \n\t"
+ "vmovups 704(%0), %%zmm11 \n\t"
+ "vmovups 768(%0), %%zmm12 \n\t"
+ "vmovups 832(%0), %%zmm13 \n\t"
+ "vmovups 896(%0), %%zmm14 \n\t"
+ "vmovups 960(%0), %%zmm15 \n\t"
: /* no output operands */
: "r" (zmm_data));
- asm ("vmovaps 1024(%0), %%zmm16\n\t"
- "vmovaps 1088(%0), %%zmm17\n\t"
- "vmovaps 1152(%0), %%zmm18\n\t"
- "vmovaps 1216(%0), %%zmm19\n\t"
- "vmovaps 1280(%0), %%zmm20\n\t"
- "vmovaps 1344(%0), %%zmm21\n\t"
- "vmovaps 1408(%0), %%zmm22\n\t"
- "vmovaps 1472(%0), %%zmm23\n\t"
- "vmovaps 1536(%0), %%zmm24\n\t"
- "vmovaps 1600(%0), %%zmm25\n\t"
- "vmovaps 1664(%0), %%zmm26\n\t"
- "vmovaps 1728(%0), %%zmm27\n\t"
- "vmovaps 1792(%0), %%zmm28\n\t"
- "vmovaps 1856(%0), %%zmm29\n\t"
- "vmovaps 1920(%0), %%zmm30\n\t"
- "vmovaps 1984(%0), %%zmm31\n\t"
+ asm ("vmovups 1024(%0), %%zmm16 \n\t"
+ "vmovups 1088(%0), %%zmm17 \n\t"
+ "vmovups 1152(%0), %%zmm18 \n\t"
+ "vmovups 1216(%0), %%zmm19 \n\t"
+ "vmovups 1280(%0), %%zmm20 \n\t"
+ "vmovups 1344(%0), %%zmm21 \n\t"
+ "vmovups 1408(%0), %%zmm22 \n\t"
+ "vmovups 1472(%0), %%zmm23 \n\t"
+ "vmovups 1536(%0), %%zmm24 \n\t"
+ "vmovups 1600(%0), %%zmm25 \n\t"
+ "vmovups 1664(%0), %%zmm26 \n\t"
+ "vmovups 1728(%0), %%zmm27 \n\t"
+ "vmovups 1792(%0), %%zmm28 \n\t"
+ "vmovups 1856(%0), %%zmm29 \n\t"
+ "vmovups 1920(%0), %%zmm30 \n\t"
+ "vmovups 1984(%0), %%zmm31 \n\t"
: /* no output operands */
: "r" (zmm_data));
#endif
@@ -173,44 +173,44 @@ move_zmm_data_to_reg (void)
void
move_zmm_data_to_memory (void)
{
- asm ("vmovaps %%zmm0, 0(%0)\n\t"
- "vmovaps %%zmm1, 64(%0)\n\t"
- "vmovaps %%zmm2, 128(%0)\n\t"
- "vmovaps %%zmm3, 192(%0)\n\t"
- "vmovaps %%zmm4, 256(%0)\n\t"
- "vmovaps %%zmm5, 320(%0)\n\t"
- "vmovaps %%zmm6, 384(%0)\n\t"
- "vmovaps %%zmm7, 448(%0)\n\t"
+ asm ("vmovups %%zmm0, 0(%0)\n\t"
+ "vmovups %%zmm1, 64(%0)\n\t"
+ "vmovups %%zmm2, 128(%0)\n\t"
+ "vmovups %%zmm3, 192(%0)\n\t"
+ "vmovups %%zmm4, 256(%0)\n\t"
+ "vmovups %%zmm5, 320(%0)\n\t"
+ "vmovups %%zmm6, 384(%0)\n\t"
+ "vmovups %%zmm7, 448(%0)\n\t"
: /* no output operands */
: "r" (zmm_data));
#ifdef __x86_64__
- asm ("vmovaps %%zmm8, 512(%0)\n\t"
- "vmovaps %%zmm9, 576(%0)\n\t"
- "vmovaps %%zmm10, 640(%0)\n\t"
- "vmovaps %%zmm11, 704(%0)\n\t"
- "vmovaps %%zmm12, 768(%0)\n\t"
- "vmovaps %%zmm13, 832(%0)\n\t"
- "vmovaps %%zmm14, 896(%0)\n\t"
- "vmovaps %%zmm15, 960(%0)\n\t"
+ asm ("vmovups %%zmm8, 512(%0)\n\t"
+ "vmovups %%zmm9, 576(%0)\n\t"
+ "vmovups %%zmm10, 640(%0)\n\t"
+ "vmovups %%zmm11, 704(%0)\n\t"
+ "vmovups %%zmm12, 768(%0)\n\t"
+ "vmovups %%zmm13, 832(%0)\n\t"
+ "vmovups %%zmm14, 896(%0)\n\t"
+ "vmovups %%zmm15, 960(%0)\n\t"
: /* no output operands */
: "r" (zmm_data));
- asm ("vmovaps %%zmm16, 1024(%0)\n\t"
- "vmovaps %%zmm17, 1088(%0)\n\t"
- "vmovaps %%zmm18, 1152(%0)\n\t"
- "vmovaps %%zmm19, 1216(%0)\n\t"
- "vmovaps %%zmm20, 1280(%0)\n\t"
- "vmovaps %%zmm21, 1344(%0)\n\t"
- "vmovaps %%zmm22, 1408(%0)\n\t"
- "vmovaps %%zmm23, 1472(%0)\n\t"
- "vmovaps %%zmm24, 1536(%0)\n\t"
- "vmovaps %%zmm25, 1600(%0)\n\t"
- "vmovaps %%zmm26, 1664(%0)\n\t"
- "vmovaps %%zmm27, 1728(%0)\n\t"
- "vmovaps %%zmm28, 1792(%0)\n\t"
- "vmovaps %%zmm29, 1856(%0)\n\t"
- "vmovaps %%zmm30, 1920(%0)\n\t"
- "vmovaps %%zmm31, 1984(%0)\n\t"
+ asm ("vmovups %%zmm16, 1024(%0)\n\t"
+ "vmovups %%zmm17, 1088(%0)\n\t"
+ "vmovups %%zmm18, 1152(%0)\n\t"
+ "vmovups %%zmm19, 1216(%0)\n\t"
+ "vmovups %%zmm20, 1280(%0)\n\t"
+ "vmovups %%zmm21, 1344(%0)\n\t"
+ "vmovups %%zmm22, 1408(%0)\n\t"
+ "vmovups %%zmm23, 1472(%0)\n\t"
+ "vmovups %%zmm24, 1536(%0)\n\t"
+ "vmovups %%zmm25, 1600(%0)\n\t"
+ "vmovups %%zmm26, 1664(%0)\n\t"
+ "vmovups %%zmm27, 1728(%0)\n\t"
+ "vmovups %%zmm28, 1792(%0)\n\t"
+ "vmovups %%zmm29, 1856(%0)\n\t"
+ "vmovups %%zmm30, 1920(%0)\n\t"
+ "vmovups %%zmm31, 1984(%0)\n\t"
: /* no output operands */
: "r" (zmm_data));
#endif
--
2.9.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] icc: allow code path for newer versions of icc.
@ 2017-09-06 8:47 Walfred Tedeschi
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Walfred Tedeschi @ 2017-09-06 8:47 UTC (permalink / raw)
To: palves, qiyaoltc; +Cc: gdb-patches, Walfred Tedeschi
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.
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.
(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.
* utils.c (producer_is_intel): New function added using same
signature as producer_is_gcc.
* 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"
+
+namespace selftests {
+namespace gdbserver {
+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";
+ 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 ;
+ }
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
}
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.
+
+ 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)
+{
+ 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");
+
+ while (*cs && !isspace (*cs))
+ cs++;
+ if (*cs && isspace (*cs))
+ cs++;
+
+ int intermediate = 0;
+ int nof = sscanf (cs, "%d.%d.%d.%*d", major, &intermediate, minor);
+
+ /* Internal versions are represented only as MAJOR.MINOR, 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);
+
extern int myread (int, char *, int);
/* Ensure that V is aligned to an N byte boundary (B's assumed to be a
--
2.9.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-06 8:47 [PATCH] icc: allow code path for newer versions of icc Walfred Tedeschi
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
2017-09-06 8:47 ` [PATCH] symlookup: improves symbol lookup when a file is specified Walfred Tedeschi
@ 2017-09-16 13:49 ` Simon Marchi
2017-09-18 14:17 ` Tedeschi, Walfred
2017-09-18 15:34 ` Pedro Alves
3 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2017-09-16 13:49 UTC (permalink / raw)
To: Walfred Tedeschi; +Cc: palves, qiyaoltc, gdb-patches
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).
> (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.
> * 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.
> +
> +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";
> + 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.
> +
> + 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.
> +{
> + 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.
> +
> + while (*cs && !isspace (*cs))
> + cs++;
I suggest using skip_to_space (from common-utils.c).
> + 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.
> +
> + 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.
> +
> extern int myread (int, char *, int);
>
> /* Ensure that V is aligned to an N byte boundary (B's assumed to be a
Thanks,
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] gdb - avx512: tests were failing due to missing memory aligment.
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
@ 2017-09-16 13:57 ` Simon Marchi
2017-09-17 20:22 ` Yao Qi
1 sibling, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2017-09-16 13:57 UTC (permalink / raw)
To: Walfred Tedeschi; +Cc: palves, qiyaoltc, gdb-patches
On 2017-09-06 10:46, Walfred Tedeschi wrote:
> Test was running on a fault during code execution. Analysis have shown
> that the wrong instruction had been used. An instruction that takes
> not alligned memory is more appropriated for the task.
LGTM, you guys are the experts about that :)
> ChangeLog:
>
> 2017-06-26 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
> * testsuite/gdb.arch/i386-avx512.c (move_zmm_data_to_reg): Use
> vmovups instead vmovaps.
> (move_zmm_data_to_memory): Use vmovups instead vmovaps.
>
> ---
> gdb/testsuite/gdb.arch/i386-avx512.c | 128
> +++++++++++++++++------------------
> 1 file changed, 64 insertions(+), 64 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.arch/i386-avx512.c
> b/gdb/testsuite/gdb.arch/i386-avx512.c
> index 52c0ead..3e955a4 100644
> --- a/gdb/testsuite/gdb.arch/i386-avx512.c
> +++ b/gdb/testsuite/gdb.arch/i386-avx512.c
> @@ -127,44 +127,44 @@ move_k_data_to_memory (void)
> void
> move_zmm_data_to_reg (void)
> {
> - asm ("vmovaps 0(%0), %%zmm0\n\t"
> - "vmovaps 64(%0), %%zmm1\n\t"
> - "vmovaps 128(%0), %%zmm2\n\t"
> - "vmovaps 192(%0), %%zmm3\n\t"
> - "vmovaps 256(%0), %%zmm4\n\t"
> - "vmovaps 320(%0), %%zmm5\n\t"
> - "vmovaps 384(%0), %%zmm6\n\t"
> - "vmovaps 448(%0), %%zmm7\n\t"
> + asm ("vmovups 0(%0), %%zmm0 \n\t"
> + "vmovups 64(%0), %%zmm1 \n\t"
> + "vmovups 128(%0), %%zmm2 \n\t"
> + "vmovups 192(%0), %%zmm3 \n\t"
> + "vmovups 256(%0), %%zmm4 \n\t"
> + "vmovups 320(%0), %%zmm5 \n\t"
> + "vmovups 384(%0), %%zmm6 \n\t"
> + "vmovups 448(%0), %%zmm7 \n\t"
Any reason for the added trailing space?
Thanks,
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] gdb - avx512: tests were failing due to missing memory aligment.
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
2017-09-16 13:57 ` Simon Marchi
@ 2017-09-17 20:22 ` Yao Qi
2017-09-18 14:18 ` Tedeschi, Walfred
2017-09-20 13:39 ` [pushed] " Tedeschi, Walfred
1 sibling, 2 replies; 21+ messages in thread
From: Yao Qi @ 2017-09-17 20:22 UTC (permalink / raw)
To: Walfred Tedeschi; +Cc: Pedro Alves, gdb-patches
On Wed, Sep 6, 2017 at 9:46 AM, Walfred Tedeschi
<walfred.tedeschi@intel.com> wrote:
>
> 2017-06-26 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
> * testsuite/gdb.arch/i386-avx512.c (move_zmm_data_to_reg): Use
Nit:
gdb/testsuite/ChangeLog:
* gdb.arch/i386-avx512.c (move_zmm_data_to_reg): ....
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-16 13:49 ` [PATCH] icc: allow code path for newer versions of icc Simon Marchi
@ 2017-09-18 14:17 ` Tedeschi, Walfred
0 siblings, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-18 14:17 UTC (permalink / raw)
To: Simon Marchi; +Cc: palves, qiyaoltc, gdb-patches
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] gdb - avx512: tests were failing due to missing memory aligment.
2017-09-17 20:22 ` Yao Qi
@ 2017-09-18 14:18 ` Tedeschi, Walfred
2017-09-20 13:39 ` [pushed] " Tedeschi, Walfred
1 sibling, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-18 14:18 UTC (permalink / raw)
To: Yao Qi; +Cc: Pedro Alves, gdb-patches
Yao,
Thanks for the review!
On 09/17/2017 10:22 PM, Yao Qi wrote:
> On Wed, Sep 6, 2017 at 9:46 AM, Walfred Tedeschi
> <walfred.tedeschi@intel.com> wrote:
>> 2017-06-26 Walfred Tedeschi <walfred.tedeschi@intel.com>
>>
>> gdb/ChangeLog:
>> * testsuite/gdb.arch/i386-avx512.c (move_zmm_data_to_reg): Use
> Nit:
>
> gdb/testsuite/ChangeLog:
>
> * gdb.arch/i386-avx512.c (move_zmm_data_to_reg): ....
>
With this change can i check in?
Thanks and regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-06 8:47 [PATCH] icc: allow code path for newer versions of icc Walfred Tedeschi
` (2 preceding siblings ...)
2017-09-16 13:49 ` [PATCH] icc: allow code path for newer versions of icc Simon Marchi
@ 2017-09-18 15:34 ` Pedro Alves
2017-09-18 16:24 ` Simon Marchi
3 siblings, 1 reply; 21+ messages in thread
From: Pedro Alves @ 2017-09-18 15:34 UTC (permalink / raw)
To: Walfred Tedeschi, qiyaoltc; +Cc: gdb-patches
Hi,
I read the patch and I found a few nits to pick.
See below.
On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
> 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;
...
>
> +#if defined GDB_SELF_TEST
> +#include "selftest.h"
> +
> +namespace selftests {
> +namespace gdbserver {
> +static void
> +dwarf_producer_test ()
Simon already pointed at the gdbserver namespace. I'd like
to add that it looks odd to me to define the actual
functionality in utils.c and define the corresponding self
tests in dwarf2read.c. Why not put the tests in utils.c as well?
BTW, I'd support moving all these producer checks to
a separate file (maybe producer.c), instead of putting
it all in the utils.c kitchen sync, though that's a
preexisting issue.
> + if (producer == NULL || !startswith (producer, "Intel(R)"))
> + return 0;
> +
> +/* Preparing the used fields. */
> + int maj, min;
Missing leading double-space in comment line.
> + if (major == NULL)
> + major = &maj;
> + if (minor == NULL)
> + minor = &min;
> + /* Not recognized as Intel, let user know. */
> + warning ("Intel producer: version not recognized.");
> + return 0;
> +}
If this ever triggers, won't the user get an annoying
flood of such warnings? If you hack your local copy to reach
the warning, what do you see?
I also wonder whether we should print a more-friend clearer
message, "intel producer" may mean nothing to a user, and I'm
not sure what they could do with that. Maybe at least print
the actual version string?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-18 15:34 ` Pedro Alves
@ 2017-09-18 16:24 ` Simon Marchi
2017-09-18 16:34 ` Tedeschi, Walfred
0 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2017-09-18 16:24 UTC (permalink / raw)
To: Pedro Alves; +Cc: Walfred Tedeschi, qiyaoltc, gdb-patches
On 2017-09-18 17:34, Pedro Alves wrote:
> Hi,
>
> I read the patch and I found a few nits to pick.
> See below.
>
> On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
>> 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;
>
> ...
>
>>
>> +#if defined GDB_SELF_TEST
>> +#include "selftest.h"
>> +
>> +namespace selftests {
>> +namespace gdbserver {
>> +static void
>> +dwarf_producer_test ()
>
> Simon already pointed at the gdbserver namespace. I'd like
> to add that it looks odd to me to define the actual
> functionality in utils.c and define the corresponding self
> tests in dwarf2read.c. Why not put the tests in utils.c as well?
>
> BTW, I'd support moving all these producer checks to
> a separate file (maybe producer.c), instead of putting
> it all in the utils.c kitchen sync, though that's a
> preexisting issue.
I agree about that. I almost wrote that this function didn't really
belong in utils.c, until I saw that it's where other similar functions
were already.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-18 16:24 ` Simon Marchi
@ 2017-09-18 16:34 ` Tedeschi, Walfred
2017-09-18 19:13 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-18 16:34 UTC (permalink / raw)
To: Simon Marchi, Pedro Alves; +Cc: qiyaoltc, gdb-patches
Hi Pedro and Simon,
Thanks again for the review!
On 09/18/2017 06:23 PM, Simon Marchi wrote:
> On 2017-09-18 17:34, Pedro Alves wrote:
>> Hi,
>>
>> I read the patch and I found a few nits to pick.
>> See below.
>>
>> On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
>>> 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;
>>
>> ...
>>
>>>
>>> +#if defined GDB_SELF_TEST
>>> +#include "selftest.h"
>>> +
>>> +namespace selftests {
>>> +namespace gdbserver {
>>> +static void
>>> +dwarf_producer_test ()
>>
>> Simon already pointed at the gdbserver namespace. I'd like
>> to add that it looks odd to me to define the actual
>> functionality in utils.c and define the corresponding self
>> tests in dwarf2read.c. Why not put the tests in utils.c as well?
>>
>> BTW, I'd support moving all these producer checks to
>> a separate file (maybe producer.c), instead of putting
>> it all in the utils.c kitchen sync, though that's a
>> preexisting issue.
>
> I agree about that. I almost wrote that this function didn't really
> belong in utils.c, until I saw that it's where other similar functions
> were already.
>
> Simon
I am considering sending then two patches:
1. Add things in utils.c
2. move all producers to a new producers.h/c file.
Would this be ok?
Thanks again,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-18 16:34 ` Tedeschi, Walfred
@ 2017-09-18 19:13 ` Simon Marchi
2017-09-20 15:19 ` Tedeschi, Walfred
0 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2017-09-18 19:13 UTC (permalink / raw)
To: Tedeschi, Walfred; +Cc: Pedro Alves, qiyaoltc, gdb-patches
On 2017-09-18 17:31, Tedeschi, Walfred wrote:
> I am considering sending then two patches:
> 1. Add things in utils.c
> 2. move all producers to a new producers.h/c file.
>
> Would this be ok?
(I am just speaking for myself)
I usually prefer the opposite, cleanup first then add new features
(because sometimes the cleanup is promised and never comes :)), but
really I'm fine with both. The fact that you are ready to do some
cleanup is appreciated.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [pushed] [PATCH] gdb - avx512: tests were failing due to missing memory aligment.
2017-09-17 20:22 ` Yao Qi
2017-09-18 14:18 ` Tedeschi, Walfred
@ 2017-09-20 13:39 ` Tedeschi, Walfred
1 sibling, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-20 13:39 UTC (permalink / raw)
To: Yao Qi; +Cc: Pedro Alves, gdb-patches
On 09/17/2017 10:22 PM, Yao Qi wrote:
> On Wed, Sep 6, 2017 at 9:46 AM, Walfred Tedeschi
> <walfred.tedeschi@intel.com> wrote:
>> 2017-06-26 Walfred Tedeschi <walfred.tedeschi@intel.com>
>>
>> gdb/ChangeLog:
>> * testsuite/gdb.arch/i386-avx512.c (move_zmm_data_to_reg): Use
> Nit:
>
> gdb/testsuite/ChangeLog:
>
> * gdb.arch/i386-avx512.c (move_zmm_data_to_reg): ....
>
Pushed with recommended changes.
Regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-18 19:13 ` Simon Marchi
@ 2017-09-20 15:19 ` Tedeschi, Walfred
2017-09-20 15:45 ` Pedro Alves
0 siblings, 1 reply; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-20 15:19 UTC (permalink / raw)
To: Simon Marchi; +Cc: Pedro Alves, qiyaoltc, gdb-patches
On 09/18/2017 09:13 PM, Simon Marchi wrote:
> On 2017-09-18 17:31, Tedeschi, Walfred wrote:
>> I am considering sending then two patches:
>> 1. Add things in utils.c
>> 2. move all producers to a new producers.h/c file.
>>
>> Would this be ok?
>
> (I am just speaking for myself)
>
> I usually prefer the opposite, cleanup first then add new features
> (because sometimes the cleanup is promised and never comes :)), but
> really I'm fine with both. The fact that you are ready to do some
> cleanup is appreciated.
>
> Simon
Hello Simon and Pedro,
I have prepared a cleaning patch first and another one with the icc
version check.
The changes are in:
users/wtedesch/icc_version
Comments are welcome!
Thanks and regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [ping][PATCH] symlookup: improves symbol lookup when a file is specified.
2017-09-06 8:47 ` [PATCH] symlookup: improves symbol lookup when a file is specified Walfred Tedeschi
@ 2017-09-20 15:22 ` Tedeschi, Walfred
2017-10-09 7:35 ` Tedeschi, Walfred
2017-10-09 12:42 ` [PATCH] " Pedro Alves
1 sibling, 1 reply; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-20 15:22 UTC (permalink / raw)
To: palves, qiyaoltc; +Cc: gdb-patches
Any feedback?
On 09/06/2017 10:46 AM, Walfred Tedeschi wrote:
> The provided patch adds a global lookup with higher priority for the
> provided file.
>
> Usual symbol lookup from GDB follows linker logic. This is what is
> desired most of the time. In the usage case a file is provided as
> scope, so the lookup has to follow this priority. Failing in finding
> the symbol at this scope usual path is followed.
>
> As test case it is presented two shared objects having a global variable
> with the same name but comming from different source files. Without the
> patch accessing them via the file scope returns the value seen in the
> first loaded shared object. Using the patch the value defined in the file
> scope is the one returned.
>
> One of the already existing test cases in print-file-var.exp starts to
> fail after using this patch. In fact the value that the linker sees is
> different then the one debugger can read from the shared object. In
> this sense it looks like to me that the test has to be changed.
> The fail is in the call:
> print 'print-file-var-lib2.c'::this_version_id == v2
>
> In this case there also in both libraries the symbol this_version_id.
> During load of the libraries linker resolves the symbol as the first one
> loaded and independent of the scope symbol will have the same value, as
> defined in the first library loaded.
> However, when defining the scope the value should represent the value
> in that context. Diferent evaluations of the same symbols might also better
> spot the issue in users code.
>
> - I haven't changed the test because I wanted to hear the community
> thought on the subject.
>
>
>
> 2017-07-13 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
>
> * symtab.c (lookup_global_symbol): Add new lookup to ensure
> priority on given block.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/print-file-var-dlopen-main.c: New file.
> * gdb.base/print-file-var-dlopen.exp: New test based on
> print-file-var.exp.
>
> ---
> gdb/symtab.c | 4 +
> .../gdb.base/print-file-var-dlopen-main.c | 62 +++++++++++++
> gdb/testsuite/gdb.base/print-file-var-dlopen.exp | 101 +++++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen.exp
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 8492315..920461f 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2590,6 +2590,10 @@ lookup_global_symbol (const char *name,
> if (objfile != NULL)
> result = solib_global_lookup (objfile, name, domain);
>
> + /* We still need to look on the global scope of current object file. */
> + if (result.symbol == NULL && objfile != NULL)
> + result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, name, domain);
> +
> /* If that didn't work go a global search (of global blocks, heh). */
> if (result.symbol == NULL)
> {
> diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> new file mode 100644
> index 0000000..98cfd97
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> @@ -0,0 +1,62 @@
> +/* This testcase is part of GDB, the GNU debugger.
> + Copyright 2017 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +int
> +dummy (void)
> +{
> + return 1;
> +}
> +
> +int
> +main (void)
> +{
> + int (*get_version1) (void);
> + int (*get_version2) (void);
> + int v1, v2;
> +
> + dummy ();
> + void *lib1 = dlopen ("print-file-var-dlopen-lib1.so", RTLD_LAZY);
> + void *lib2 = dlopen ("print-file-var-dlopen-lib2.so", RTLD_LAZY);
> +
> + if (lib1 == NULL || lib2 == NULL)
> + return 1;
> +
> + *(int **) (&get_version1) = dlsym (lib1, "get_version");
> + *(int **) (&get_version2) = dlsym (lib2, "get_version");
> +
> + if (get_version1 != NULL
> + && get_version2 != NULL)
> + {
> + v1 = get_version1();
> + v2 = get_version2();
> + }
> +
> + dummy (); /* STOP */
> + dlclose (lib1);
> + dlclose (lib2);
> + if (v1 != 104)
> + return 1;
> + /* The value returned by get_version_2 depends on the target system. */
> + if (v2 != 104 || v2 != 203)
> + return 2;
> +
> + return 0;
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen.exp b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> new file mode 100644
> index 0000000..87f89f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> @@ -0,0 +1,101 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +if {[skip_shlib_tests]} {
> + return -1
> +}
> +
> +set executable print-file-var-dlopen-main
> +
> +set lib1 "print-file-var-lib1"
> +set lib2 "print-file-var-lib2"
> +
> +set libobj1 [standard_output_file ${lib1}.so]
> +set libobj2 [standard_output_file ${lib2}.so]
> +
> +set lib_opts { debug additional_flags=-fPIC additional_flags=-shared}
> +
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
> + ${libobj1} \
> + ${lib_opts} ] != "" } {
> + return -1
> +}
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib2}.c \
> + ${libobj2} \
> + ${lib_opts} ] != "" } {
> + return -1
> +}
> +if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
> + [standard_output_file ${executable}] \
> + executable \
> + [list debug shlib=-ldl]]
> + != ""} {
> + return -1
> +}
> +
> +clean_restart $executable
> +
> +set outdir [file dirname $libobj1]
> +
> +gdb_test_no_output "set env LD_LIBRARY_PATH=${outdir}/:"
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}
> +
> +# Try printing "this_version_num" qualified with the name of the file
> +# where the variables are defined. There are two global variables
> +# with that name, and some systems such as GNU/Linux merge them
> +# into one single entity, while some other systems such as Windows
> +# keep them separate. In the first situation, we have to verify
> +# that GDB does not randomly select the wrong instance, even when
> +# a specific filename is used to qualified the lookup. And in the
> +# second case, we have to verify that GDB does select the instance
> +# defined in the given filename.
> +#
> +# To avoid adding target-specific code in this testcase, the program
> +# sets two local variable named 'v1' and 'v2' with the value of
> +# our global variables. This allows us to compare the value that
> +# GDB returns for each query against the actual value seen by
> +# the program itself.
> +
> +# Get past the initialization of variables 'v1' and 'v2'.
> +
> +set bp_location \
> + [gdb_get_line_number "STOP" "${executable}.c"]
> +gdb_test "break $executable.c:$bp_location" \
> + "Breakpoint \[0-9\]+ at 0x\[0-9a-fA-F\]+: .*" \
> + "breapoint past v1 & v2 initialization"
> +
> +gdb_test "continue" \
> + "Breakpoint \[0-9\]+, main \\(\\) at.*" \
> + "continue to STOP marker"
> +
> +# Now check the value of this_version_id in both print-file-var-lib1.c
> +# and print-file-var-lib2.c.
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib2.c'::this_version_id == v2" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib2.c'::get_version::test == v2" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::get_version::test" \
> + " = 100"
> +
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] icc: allow code path for newer versions of icc.
2017-09-20 15:19 ` Tedeschi, Walfred
@ 2017-09-20 15:45 ` Pedro Alves
2017-09-20 15:55 ` Tedeschi, Walfred
2017-09-21 11:27 ` Tedeschi, Walfred
0 siblings, 2 replies; 21+ messages in thread
From: Pedro Alves @ 2017-09-20 15:45 UTC (permalink / raw)
To: Tedeschi, Walfred, Simon Marchi; +Cc: qiyaoltc, gdb-patches
On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:
> Hello Simon and Pedro,
>
> I have prepared a cleaning patch first and another one with the icc
> version check.
>
> The changes are in:
> users/wtedesch/icc_version
>
> Comments are welcome!
Thanks!
Speaking for myself, while a git branch is handy for trying
locally (and I tend to push changes to branches too to help
others for larger series, in addition to posting on the list),
I still find it easier to reply/review to patches sent via email,
because that allows conveniently quoting patches.
But I went ahead and took a quick look. I have to say that
don't care for the "dwarf2utils" name much BTW. My issue with
"utils.h|c" is that "utils" is a kitchen sync name; almost anything
can be called an "utility". And "dwarf2utils" has almost the same
issue, in my view. Note that I had suggested producers.h/c without
a "dwarf" qualification because the producers string is exposed
in generic code, through symtab.h:compunit_symtab etc.
(even though the producers info is currently only sourced
from DWARF info).
As for the the rest, it seems like several previous comments
haven't been addressed yet, so I'll wait for a repost.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] icc: allow code path for newer versions of icc.
2017-09-20 15:45 ` Pedro Alves
@ 2017-09-20 15:55 ` Tedeschi, Walfred
2017-09-21 11:27 ` Tedeschi, Walfred
1 sibling, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-20 15:55 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi; +Cc: qiyaoltc, gdb-patches
-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Wednesday, September 20, 2017 5:46 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>
Cc: qiyaoltc@gmail.com; gdb-patches@sourceware.org
Subject: Re: [PATCH] icc: allow code path for newer versions of icc.
On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:
> Hello Simon and Pedro,
>
> I have prepared a cleaning patch first and another one with the icc
> version check.
>
> The changes are in:
> users/wtedesch/icc_version
>
> Comments are welcome!
Thanks!
Speaking for myself, while a git branch is handy for trying locally (and I tend to push changes to branches too to help others for larger series, in addition to posting on the list), I still find it easier to reply/review to patches sent via email, because that allows conveniently quoting patches.
But I went ahead and took a quick look. I have to say that don't care for the "dwarf2utils" name much BTW. My issue with "utils.h|c" is that "utils" is a kitchen sync name; almost anything can be called an "utility". And "dwarf2utils" has almost the same issue, in my view. Note that I had suggested producers.h/c without a "dwarf" qualification because the producers string is exposed in generic code, through symtab.h:compunit_symtab etc.
(even though the producers info is currently only sourced from DWARF info).
As for the the rest, it seems like several previous comments haven't been addressed yet, so I'll wait for a repost.
Thanks,
Pedro Alves
Pedro,
Thanks for your quick answer!
I will prepare new patches.
Thanks and regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] icc: allow code path for newer versions of icc.
2017-09-20 15:45 ` Pedro Alves
2017-09-20 15:55 ` Tedeschi, Walfred
@ 2017-09-21 11:27 ` Tedeschi, Walfred
1 sibling, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-09-21 11:27 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi; +Cc: qiyaoltc, gdb-patches
-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Wednesday, September 20, 2017 5:46 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>
Cc: qiyaoltc@gmail.com; gdb-patches@sourceware.org
Subject: Re: [PATCH] icc: allow code path for newer versions of icc.
On 09/20/2017 03:17 PM, Tedeschi, Walfred wrote:
>> Hello Simon and Pedro,
>>
>> I have prepared a cleaning patch first and another one with the icc
>> version check.
>>
>> The changes are in:
>> users/wtedesch/icc_version
>>
>> Comments are welcome!
>
>Thanks!
>
>Speaking for myself, while a git branch is handy for trying locally (and I tend to push changes to branches too to help others for larger series, in >addition to posting on the list), I still find it easier to reply/review to patches sent via email, because that allows conveniently quoting patches. >
>
>But I went ahead and took a quick look. I have to say that don't care for the "dwarf2utils" name much BTW. My issue with "utils.h|c" is that >"utils" is a kitchen sync name; almost anything can be called an "utility". And "dwarf2utils" has almost the same issue, in my view. Note that I >had suggested producers.h/c without a "dwarf" qualification because the producers string is exposed in generic code, through >symtab.h:compunit_symtab etc.
> (even though the producers info is currently only sourced from DWARF info).
>
>As for the the rest, it seems like several previous comments haven't been addressed yet, so I'll wait for a repost.
>
>Thanks,
>Pedro Alves
Hello Pedro,
Thanks for the quick reply!
My concern with the name was to create a file that could be used for more things, but not much more.
Do you consider a better approach to have a specialized file, having only the producers on it?
Best regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [ping][PATCH] symlookup: improves symbol lookup when a file is specified.
2017-09-20 15:22 ` [ping][PATCH] " Tedeschi, Walfred
@ 2017-10-09 7:35 ` Tedeschi, Walfred
0 siblings, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-10-09 7:35 UTC (permalink / raw)
To: Tedeschi, Walfred, palves, qiyaoltc, Keith Seitz (keiths@redhat.com)
Cc: gdb-patches
Any feedback?
-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Tedeschi, Walfred
Sent: Wednesday, September 20, 2017 4:20 PM
To: palves@redhat.com; qiyaoltc@gmail.com
Cc: gdb-patches@sourceware.org
Subject: [ping][PATCH] symlookup: improves symbol lookup when a file is specified.
Any feedback?
On 09/06/2017 10:46 AM, Walfred Tedeschi wrote:
> The provided patch adds a global lookup with higher priority for the
> provided file.
>
> Usual symbol lookup from GDB follows linker logic. This is what is
> desired most of the time. In the usage case a file is provided as
> scope, so the lookup has to follow this priority. Failing in finding
> the symbol at this scope usual path is followed.
>
> As test case it is presented two shared objects having a global
> variable with the same name but comming from different source files.
> Without the patch accessing them via the file scope returns the value
> seen in the first loaded shared object. Using the patch the value
> defined in the file scope is the one returned.
>
> One of the already existing test cases in print-file-var.exp starts to
> fail after using this patch. In fact the value that the linker sees is
> different then the one debugger can read from the shared object. In
> this sense it looks like to me that the test has to be changed.
> The fail is in the call:
> print 'print-file-var-lib2.c'::this_version_id == v2
>
> In this case there also in both libraries the symbol this_version_id.
> During load of the libraries linker resolves the symbol as the first
> one loaded and independent of the scope symbol will have the same
> value, as defined in the first library loaded.
> However, when defining the scope the value should represent the value
> in that context. Diferent evaluations of the same symbols might also
> better spot the issue in users code.
>
> - I haven't changed the test because I wanted to hear the community
> thought on the subject.
>
>
>
> 2017-07-13 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> gdb/ChangeLog:
>
> * symtab.c (lookup_global_symbol): Add new lookup to ensure
> priority on given block.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/print-file-var-dlopen-main.c: New file.
> * gdb.base/print-file-var-dlopen.exp: New test based on
> print-file-var.exp.
>
> ---
> gdb/symtab.c | 4 +
> .../gdb.base/print-file-var-dlopen-main.c | 62 +++++++++++++
> gdb/testsuite/gdb.base/print-file-var-dlopen.exp | 101 +++++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> create mode 100644 gdb/testsuite/gdb.base/print-file-var-dlopen.exp
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c index 8492315..920461f 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2590,6 +2590,10 @@ lookup_global_symbol (const char *name,
> if (objfile != NULL)
> result = solib_global_lookup (objfile, name, domain);
>
> + /* We still need to look on the global scope of current object
> + file. */ if (result.symbol == NULL && objfile != NULL)
> + result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, name,
> + domain);
> +
> /* If that didn't work go a global search (of global blocks, heh). */
> if (result.symbol == NULL)
> {
> diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> new file mode 100644
> index 0000000..98cfd97
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c
> @@ -0,0 +1,62 @@
> +/* This testcase is part of GDB, the GNU debugger.
> + Copyright 2017 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +int
> +dummy (void)
> +{
> + return 1;
> +}
> +
> +int
> +main (void)
> +{
> + int (*get_version1) (void);
> + int (*get_version2) (void);
> + int v1, v2;
> +
> + dummy ();
> + void *lib1 = dlopen ("print-file-var-dlopen-lib1.so", RTLD_LAZY);
> + void *lib2 = dlopen ("print-file-var-dlopen-lib2.so", RTLD_LAZY);
> +
> + if (lib1 == NULL || lib2 == NULL)
> + return 1;
> +
> + *(int **) (&get_version1) = dlsym (lib1, "get_version"); *(int **)
> + (&get_version2) = dlsym (lib2, "get_version");
> +
> + if (get_version1 != NULL
> + && get_version2 != NULL)
> + {
> + v1 = get_version1();
> + v2 = get_version2();
> + }
> +
> + dummy (); /* STOP */
> + dlclose (lib1);
> + dlclose (lib2);
> + if (v1 != 104)
> + return 1;
> + /* The value returned by get_version_2 depends on the target
> + system. */ if (v2 != 104 || v2 != 203)
> + return 2;
> +
> + return 0;
> +}
> +
> diff --git a/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> new file mode 100644
> index 0000000..87f89f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/print-file-var-dlopen.exp
> @@ -0,0 +1,101 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or
> +modify # it under the terms of the GNU General Public License as
> +published by # the Free Software Foundation; either version 3 of the
> +License, or # (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful, #
> +but WITHOUT ANY WARRANTY; without even the implied warranty of #
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU
> +General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License #
> +along with this program. If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +if {[skip_shlib_tests]} {
> + return -1
> +}
> +
> +set executable print-file-var-dlopen-main
> +
> +set lib1 "print-file-var-lib1"
> +set lib2 "print-file-var-lib2"
> +
> +set libobj1 [standard_output_file ${lib1}.so] set libobj2
> +[standard_output_file ${lib2}.so]
> +
> +set lib_opts { debug additional_flags=-fPIC additional_flags=-shared}
> +
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
> + ${libobj1} \
> + ${lib_opts} ] != "" } {
> + return -1
> +}
> +if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib2}.c \
> + ${libobj2} \
> + ${lib_opts} ] != "" } {
> + return -1
> +}
> +if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
> + [standard_output_file ${executable}] \
> + executable \
> + [list debug shlib=-ldl]]
> + != ""} {
> + return -1
> +}
> +
> +clean_restart $executable
> +
> +set outdir [file dirname $libobj1]
> +
> +gdb_test_no_output "set env LD_LIBRARY_PATH=${outdir}/:"
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}
> +
> +# Try printing "this_version_num" qualified with the name of the file
> +# where the variables are defined. There are two global variables #
> +with that name, and some systems such as GNU/Linux merge them # into
> +one single entity, while some other systems such as Windows # keep
> +them separate. In the first situation, we have to verify # that GDB
> +does not randomly select the wrong instance, even when # a specific
> +filename is used to qualified the lookup. And in the # second case,
> +we have to verify that GDB does select the instance # defined in the
> +given filename.
> +#
> +# To avoid adding target-specific code in this testcase, the program
> +# sets two local variable named 'v1' and 'v2' with the value of # our
> +global variables. This allows us to compare the value that # GDB
> +returns for each query against the actual value seen by # the program
> +itself.
> +
> +# Get past the initialization of variables 'v1' and 'v2'.
> +
> +set bp_location \
> + [gdb_get_line_number "STOP" "${executable}.c"] gdb_test "break
> +$executable.c:$bp_location" \
> + "Breakpoint \[0-9\]+ at 0x\[0-9a-fA-F\]+: .*" \
> + "breapoint past v1 & v2 initialization"
> +
> +gdb_test "continue" \
> + "Breakpoint \[0-9\]+, main \\(\\) at.*" \
> + "continue to STOP marker"
> +
> +# Now check the value of this_version_id in both
> +print-file-var-lib1.c # and print-file-var-lib2.c.
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib2.c'::this_version_id == v2" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib2.c'::get_version::test == v2" \
> + " = 1"
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::get_version::test" \
> + " = 100"
> +
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
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] symlookup: improves symbol lookup when a file is specified.
2017-09-06 8:47 ` [PATCH] symlookup: improves symbol lookup when a file is specified Walfred Tedeschi
2017-09-20 15:22 ` [ping][PATCH] " Tedeschi, Walfred
@ 2017-10-09 12:42 ` Pedro Alves
2017-10-09 14:06 ` Tedeschi, Walfred
1 sibling, 1 reply; 21+ messages in thread
From: Pedro Alves @ 2017-10-09 12:42 UTC (permalink / raw)
To: Walfred Tedeschi, qiyaoltc; +Cc: gdb-patches
On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
> The provided patch adds a global lookup with higher priority for the
> provided file.
>
> Usual symbol lookup from GDB follows linker logic. This is what is
> desired most of the time. In the usage case a file is provided as
> scope, so the lookup has to follow this priority. Failing in finding
> the symbol at this scope usual path is followed.
>
> As test case it is presented two shared objects having a global variable
> with the same name but comming from different source files. Without the
> patch accessing them via the file scope returns the value seen in the
> first loaded shared object. Using the patch the value defined in the file
> scope is the one returned.
>
> One of the already existing test cases in print-file-var.exp starts to
> fail after using this patch. In fact the value that the linker sees is
> different then the one debugger can read from the shared object. In
> this sense it looks like to me that the test has to be changed.
> The fail is in the call:
> print 'print-file-var-lib2.c'::this_version_id == v2
> In this case there also in both libraries the symbol this_version_id.
> During load of the libraries linker resolves the symbol as the first one
> loaded and independent of the scope symbol will have the same value, as
> defined in the first library loaded.
> However, when defining the scope the value should represent the value
> in that context. Diferent evaluations of the same symbols might also better
> spot the issue in users code.
>
> - I haven't changed the test because I wanted to hear the community
> thought on the subject.
I'm not sure I understand the description above, so I'd like to try this
locally to try to understand it better, but the test fails for me:
(gdb) break print-file-var-dlopen-main.c:51
Breakpoint 2 at 0x400751: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c, line 51.
(gdb) PASS: gdb.base/print-file-var-dlopen.exp: breapoint past v1 & v2 initialization
continue
Continuing.
[Inferior 1 (process 11534) exited with code 01]
(gdb) FAIL: gdb.base/print-file-var-dlopen.exp: continue to STOP marker (the program exited)
The program exits because dlopen failed. Is there a reason the test
is trying to use LD_LIBRARY_PATH instead of compiling the program
with the shared library file name defined as an absolute path via
additional_flags=-DXXXX like seemingly every other dlopen test does?
> +
> + dummy (); /* STOP */
> + dlclose (lib1);
> + dlclose (lib2);
> + if (v1 != 104)
> + return 1;
> + /* The value returned by get_version_2 depends on the target system. */
> + if (v2 != 104 || v2 != 203)
> + return 2;
> +
> + return 0;
> +}
> +
I tried to fix the above to get it running for me, but then noticed other
things clearly bad with the test. This certainly can't have passed as
is for you.
For example, the test reuses these source files from the
print-file-var.exp test:
+set lib1 "print-file-var-lib1"
+set lib2 "print-file-var-lib2"
But then looks for "get_version" symbols in them, but there's
no such symbols...:
+ *(int **) (&get_version1) = dlsym (lib1, "get_version");
+ *(int **) (&get_version2) = dlsym (lib2, "get_version");
While here:
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> + " = 1"
it looks like the test actually wanted to use some different
source files, but there's no print-file-var-dlopen-lib1.c included
in the patch.
Walfred, please double check what you intend to submit.
Pedro Alves
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] symlookup: improves symbol lookup when a file is specified.
2017-10-09 12:42 ` [PATCH] " Pedro Alves
@ 2017-10-09 14:06 ` Tedeschi, Walfred
0 siblings, 0 replies; 21+ messages in thread
From: Tedeschi, Walfred @ 2017-10-09 14:06 UTC (permalink / raw)
To: Pedro Alves, qiyaoltc, Keith Seitz (keiths@redhat.com); +Cc: gdb-patches
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com]
Sent: Monday, October 9, 2017 2:42 PM
To: Tedeschi, Walfred <walfred.tedeschi@intel.com>; qiyaoltc@gmail.com
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] symlookup: improves symbol lookup when a file is specified.
On 09/06/2017 09:46 AM, Walfred Tedeschi wrote:
> The provided patch adds a global lookup with higher priority for the
> provided file.
>
> Usual symbol lookup from GDB follows linker logic. This is what is
> desired most of the time. In the usage case a file is provided as
> scope, so the lookup has to follow this priority. Failing in finding
> the symbol at this scope usual path is followed.
>
> As test case it is presented two shared objects having a global
> variable with the same name but comming from different source files.
> Without the patch accessing them via the file scope returns the value
> seen in the first loaded shared object. Using the patch the value
> defined in the file scope is the one returned.
>
> One of the already existing test cases in print-file-var.exp starts to
> fail after using this patch. In fact the value that the linker sees is
> different then the one debugger can read from the shared object. In
> this sense it looks like to me that the test has to be changed.
> The fail is in the call:
> print 'print-file-var-lib2.c'::this_version_id == v2 In this case
> there also in both libraries the symbol this_version_id.
> During load of the libraries linker resolves the symbol as the first
> one loaded and independent of the scope symbol will have the same
> value, as defined in the first library loaded.
> However, when defining the scope the value should represent the value
> in that context. Diferent evaluations of the same symbols might also
> better spot the issue in users code.
>
> - I haven't changed the test because I wanted to hear the community
> thought on the subject.
I'm not sure I understand the description above, so I'd like to try this locally to try to understand it better, but the test fails for me:
(gdb) break print-file-var-dlopen-main.c:51 Breakpoint 2 at 0x400751: file /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/print-file-var-dlopen-main.c, line 51.
(gdb) PASS: gdb.base/print-file-var-dlopen.exp: breapoint past v1 & v2 initialization continue Continuing.
[Inferior 1 (process 11534) exited with code 01]
(gdb) FAIL: gdb.base/print-file-var-dlopen.exp: continue to STOP marker (the program exited)
The program exits because dlopen failed. Is there a reason the test is trying to use LD_LIBRARY_PATH instead of compiling the program with the shared library file name defined as an absolute path via additional_flags=-DXXXX like seemingly every other dlopen test does?
> +
> + dummy (); /* STOP */
> + dlclose (lib1);
> + dlclose (lib2);
> + if (v1 != 104)
> + return 1;
> + /* The value returned by get_version_2 depends on the target
> + system. */ if (v2 != 104 || v2 != 203)
> + return 2;
> +
> + return 0;
> +}
> +
I tried to fix the above to get it running for me, but then noticed other things clearly bad with the test. This certainly can't have passed as is for you.
For example, the test reuses these source files from the print-file-var.exp test:
+set lib1 "print-file-var-lib1"
+set lib2 "print-file-var-lib2"
But then looks for "get_version" symbols in them, but there's no such symbols...:
+ *(int **) (&get_version1) = dlsym (lib1, "get_version"); + *(int **) (&get_version2) = dlsym (lib2, "get_version");
While here:
> +
> +gdb_test "print 'print-file-var-dlopen-lib1.c'::this_version_id == v1" \
> + " = 1"
it looks like the test actually wanted to use some different source files, but there's no print-file-var-dlopen-lib1.c included in the patch.
Walfred, please double check what you intend to submit.
Pedro Alves
Pedro,
Thanks a lot for your review and sorry.
I should have started patching the test that was already existing in GDB and then moved to a new one.
I believe that a fixup was missed in the process. I will prepare the patch again!
Regards,
/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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-10-09 14:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 8:47 [PATCH] icc: allow code path for newer versions of icc Walfred Tedeschi
2017-09-06 8:47 ` [PATCH] gdb - avx512: tests were failing due to missing memory aligment Walfred Tedeschi
2017-09-16 13:57 ` Simon Marchi
2017-09-17 20:22 ` Yao Qi
2017-09-18 14:18 ` Tedeschi, Walfred
2017-09-20 13:39 ` [pushed] " Tedeschi, Walfred
2017-09-06 8:47 ` [PATCH] symlookup: improves symbol lookup when a file is specified Walfred Tedeschi
2017-09-20 15:22 ` [ping][PATCH] " Tedeschi, Walfred
2017-10-09 7:35 ` Tedeschi, Walfred
2017-10-09 12:42 ` [PATCH] " Pedro Alves
2017-10-09 14:06 ` Tedeschi, Walfred
2017-09-16 13:49 ` [PATCH] icc: allow code path for newer versions of icc Simon Marchi
2017-09-18 14:17 ` Tedeschi, Walfred
2017-09-18 15:34 ` Pedro Alves
2017-09-18 16:24 ` Simon Marchi
2017-09-18 16:34 ` Tedeschi, Walfred
2017-09-18 19:13 ` Simon Marchi
2017-09-20 15:19 ` Tedeschi, Walfred
2017-09-20 15:45 ` Pedro Alves
2017-09-20 15:55 ` Tedeschi, Walfred
2017-09-21 11:27 ` Tedeschi, Walfred
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox