PEXTR{B,W} and PINSR{B,W}, just like for AVX512BW, are WIG, no matter
that the SDM uses a nonstandard description of that fact.
PEXTRD, even with EVEX.W set, ignores that bit outside of 64-bit mode,
just like its AVX counterpart.
Many VEX-/EVEX-encoded instructions accessing GPRs become WIG outside of
64-bit mode. The respective templates should specify neither VexWIG nor
VexW0, but instead the setting of the bit should be determined from
- REX.W in 64-bit mode,
- the setting established through -mvexwig= / -mevexwig= otherwise.
This implies that the evex-wig2 testcase needs to go away, as being
wrong altogether.
A few test additions desirable here will only happen in later patches,
as the disassembler needs adjustments first.
Once again SSE2AVX templates are left alone, for it being unclear what
the behavior there should be.
Quite a few templates were marked LIG while really the insns aren't.
Introduce descriptive shorthands once again, instead of continuing to
use the less legible original forms.
The 0F C5 encoding is indeed a load type one (just that memory operands
are not permitted), while the 0F 3A 15 encoding is obviously a store.
Allow the pseudo prefixes to be used to select between them.
Also move (without any change) the secondary AVX512BW templates next to
the primary one.
Commits 6865c0435a ("x86: Support VEX/EVEX WIG encoding") and 6fa52824c3
("x86: Replace VexW=3 with VexWIG") omitted quite a few templates, oddly
enough in some cases despite testcases getting added (which then were
recorded with wrong expected output).
Also adjust VPMAXUB's attributes in the AVX512BW case to match ordering
of that of neighboring templates.
For the moment SSE2AVX templates are left alone, as it isn't clear
whether they were intentionally left untouched by the original commits
(the descriptions don't say either way).
In this context I question the decision in commit 0375113302 ("x86: Add
-mvexwig=[0|1] option to assembler") to move the logic to determine the
value of the W bit ahead of the decision whether to use 2-byte VEX:
While I can see this as one possible interpretation of -mvexwig=, the
other alternative (setting the value of the bit only if it actually
exists in the encoding) looks as reasonable to me, and perhaps even more
in line with us generally trying to pick the shortest encoding.
When current function changes after a next/step, GDB shows a message such as:
(gdb) s
info_fun1 ()
at /bd/home/philippe/gdb/git/build_smallthing/gdb/testsuite/../../../smallthing/gdb/testsuite/gdb.base/info_qt.c:41
41 info_qt_inc++;
(gdb)
Valgrind reports a 4K definite leak for each such message (full stacktrace of
the leak below).
This patch fixes this leak, by transferring the current s->fullname to the
unique_xmalloc_ptr fullname given to find_and_open_source.
Note that I do not understand why find_and_open_source always tries to
re-execute the substitution rules on the provided fullname, as source.c
symtab_to_fullname just blindly returns a non NULL s->fullname, counting on
forget_cached_source_info to be called if search dir or substitution rules are
changed. Similarly, psymtab_to_fullname also just returns a non NULL
ps->fullname.
==15309== VALGRIND_GDB_ERROR_BEGIN
==15309== 69,632 bytes in 17 blocks are definitely lost in loss record 3,158 of 3,186
==15309== at 0x4C2BE2D: malloc (vg_replace_malloc.c:299)
==15309== by 0x5BF0987: realpath@@GLIBC_2.3 (canonicalize.c:78)
==15309== by 0x41F713: gdb_realpath(char const*) (pathstuff.c:72)
==15309== by 0x608833: openp(char const*, enum_flags<openp_flag>, char const*, int, std::unique_ptr<char, gdb::xfree_deleter<char> >*) (source.c:861)
==15309== by 0x608B89: find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) (source.c:1049)
==15309== by 0x608D0B: open_source_file(symtab*) (source.c:1074)
==15309== by 0x609101: print_source_lines_base(symtab*, int, int, enum_flags<print_source_lines_flag>) (source.c:1291)
==15309== by 0x614ADF: print_frame_info(frame_info*, int, print_what, int, int) (stack.c:911)
==15309== by 0x614C45: print_stack_frame(frame_info*, int, print_what, int) (stack.c:181)
==15309== by 0x511D5E: print_stop_location (infrun.c:8044)
==15309== by 0x511D5E: print_stop_event(ui_out*) (infrun.c:8061)
==15309== by 0x40DD6D: cli_on_normal_stop(bpstats*, int) (cli-interp.c:145)
==15309== by 0x512409: operator() (functional:2127)
==15309== by 0x512409: notify (observable.h:106)
==15309== by 0x512409: normal_stop() (infrun.c:8334)
==15309== by 0x5156D8: fetch_inferior_event(void*) (infrun.c:3955)
==15309== by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859)
==15309== by 0x4B3FF6: gdb_do_one_event() [clone .part.4] (event-loop.c:322)
==15309== by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219)
==15309== by 0x4B41B4: start_event_loop() (event-loop.c:371)
==15309== by 0x551217: captured_command_loop() (main.c:330)
==15309== by 0x55220C: captured_main (main.c:1177)
==15309== by 0x55220C: gdb_main(captured_main_args*) (main.c:1193)
==15309== by 0x29B4F7: main (gdb.c:32)
==15309==
==15309== VALGRIND_GDB_ERROR_END
gdb/ChangeLog
2018-11-04 Philippe Waroquiers <philippe.waroquiers@skynet.be>
* source.c (open_source_file): Fix leak by transferring the
current s->fullname to the unique_xmalloc_ptr fullname given
to find_and_open_source.
commit e60f4d3bda
Author: H.J. Lu <hjl.tools@gmail.com>
Date: Mon Nov 5 09:01:26 2018 -0800
x86: Disable GOT relaxation with data prefix
Since linker GOT relaxation isn't valid for 16-bit GOT access, we should
disable GOT relaxation with data prefix.
Valgrind detected a leak for the line:
type = xstrdup ("auto");
as the compile probably dropped the type variable completely, as its
only usage was this initialization.
So, remove the useless variable.
Add AC_FUNC_MMAP to configure.ac so that HAVE_MMAP will be checked in
objdump.c and mmap is used if available.
* configure.ac (AC_FUNC_MMAP): New.
* config.in: Regenerated.
* configure: Likewise.
This patch adds support for debugging Ravenscar tasks, similar to what
is done for ppc and sparc.
gdb/ChangeLog:
* aarch64-ravenscar-thread.h, aarch64-ravenscar-thread.c:
New files.
* aarch64-tdep.c: #include "aarch64-ravenscar-thread.h".
(aarch64_gdbarch_init): Add call to register_aarch64_ravenscar_ops.
* Makefile.in (ALL_64_TARGET_OBS): Add aarch64-ravenscar-thread.o.
(HFILES_NO_SRCDIR): Add aarch64-ravenscar-thread.h.
(ALLDEPFILES): Add aarch64-ravenscar-thread.c.
* configure.tgt (cpu_obs) [aarch64*-*-*]: Add ravenscar-thread.o
and aarch64-ravenscar-thread.o.
* NEWS: Add entry documenting Ravenscar tasking support
on AArch64 ELF.
Cleans a few more test files.
* Makefile.am (MOSTLYCLEANFILES): Define.
* Makefile.in: Regnerate.
* testsuite/Makefile.am (MOSTLYCLEANFILES): Add ver_test_14 and
gnu_property_test.
* testsuite/Makefile.in: Regnerate.
On readelf examining a static executable built with current glibc,
we get a silly warning.
Section Headers:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al
[ 0] NULL 0000000000000000 000000 000000 00 0 0 0
[ 1] .note.ABI-tag NOTE 0000000000400190 000190 000020 00 A 0 0 4
[ 2] .note.gnu.build-id NOTE 00000000004001b0 0001b0 000024 00 A 0 0 4
readelf: Warning: [ 3]: Link field (0) should index a symtab section.
[ 3] .rela.plt RELA 00000000004001d8 0001d8 000228 18 AI 0 20 8
This .rela.plt section contains only IRELATIVE relocations (which have
symbol index zero), so it isn't appropriate to warn. A zero sh_link
section is deliberately chosen for such a section (see PR10337 and
PR23850).
So this patch disables the SHT_REL* sh_link warning. I've also
removed the .rel.dyn/.rela.dyn section name test to disable the
sh_info warning for SHT_REL* sections. While relocation sections in
an executable need not specify the section they relocate (the
relocation sh_offset field is an address, not a section offset), that
isn't true in a relocatable file where sh_offset is relative to a
section. If .rela.dyn happens to exist in an ET_REL object it must
specify a valid section.
* readelf.c (process_section_headers): Don't warn on a zero
sh_info or sh_link for any reloc section in an executable or
shared library. Do warn for .rel.dyn/.rela.dyn in ET_REL.
strip/objcopy can't deal with alloc reloc sections, not .rela.dyn or
.rela.plt in a dynamic executable, or .rela.plt/.rela.iplt in a static
executable. So, don't have BFD treat them as side-channel data
associated with the section they are relocating.
PR 23850
* elf.c (bfd_section_from_shdr): Treat SHF_ALLOC SHT_REL* sections
in an executable or shared library as normal sections.
Some hardware doesn't support unaligned accesses, and a bare metal target
may not have an unaligned access trap handler. So if the PC is 2-byte
aligned, then use a 2-byte breakpoint to avoid unaligned accesses.
Tested on native RV64GC Linux with gdb testsuite and cross on spike
simulator and openocd with riscv-tests/debug.
gdb/
* riscv-tdep.c (riscv_breakpoint_kind_from_pc): New local unaligned_p.
Set if pcptr if unaligned. Return 2 if unaligned_p true. Update
debugging messages.
The "watch -l EXPR" command with the language set to Ada currently
fails with the following error:
(gdb) watch -l global_var
Error in expression, near ` 0x000000000062d2d8'.
The error occurs because GDB internally translate the request into
a watchpoint on a different expression: "* (TYPE *) ADDR" where
TYPE and ADDR are the type and the address of the object returned
by the expression's evaluation (resp.). So, in the example above,
global_var being an integer stored at 0x000000000062d2d8, GDB tries
to set a watchpoint on "* (integer *) 0x000000000062d2d8", which
fails, because we try to parse this expression with Ada, when
in fact it is not valid.
This patch fixes the issue by implementing the la_watch_location_expression
language method, using a syntax that the Ada parser recognizes
("{TYPE} ADDR").
gdb/ChangeLog:
* ada-lang.c (ada_watch_location_expression): New function.
(ada_language_defn): Set la_watch_location_expression to
ada_watch_location_expression.
gdb/testsuite/ChangeLog:
* gdb.ada/watch_minus_l: New testcase.
the rs6000-tdep.c::skip_prologue function has the following code:
unsigned int all_mask = ~((1U << fdata->saved_gpr) - 1);
/* Not a recognized prologue instruction.
Handle optimizer code motions into the prologue by continuing
the search if we have no valid frame yet or if the return
address is not yet saved in the frame. Also skip instructions
if some of the GPRs expected to be saved are not yet saved. */
if (fdata->frameless == 0 && fdata->nosavedpc == 0
&& (fdata->gpr_mask & all_mask) == all_mask)
break;
The problem is that fdata->saved_gpr is initialized to -1, and so,
if no instruction is found in the function's prologue that causes us
to set that field to a non-negative value, the sanitizer crashes
with the following message:
rs6000-tdep.c:1965:34: runtime error: shift exponent -1 is negative
This patch fixes the issue the by only doing the shift if saved_gpr
is not negative. When saved_gpr is negative, we actually don't need
the shift.
gdb/ChangeLog:
* rs6000-tdep.c (skip_prologue): Fix potential negative left
shifting.
Tested on ppc-linux native.
Also tested on ppc-elf (baremetal) using AdaCore's testsuite.
On ARM, PikeOS does not support hardware single step, causing various
semi-random errors when trying to next/step over some user code. So
this patch changes this target to use software-single-step instead.
The challenge is that, up to now, the PikeOS target was in all respects
identical to a baremetal target as far as GDB was concerned, meaning
we were using the baremetal osabi for this target too. This is no longer
possible, and we need to introduce a new OSABI variant. Unfortunately,
there isn't anything in the object file that would allow us to
differentiate between the two platforms. So we have to rely on a
heuristic instead, where we look for some known symbols that are
required in a PikeOS application (these symbols are expected to be
defined by the default linker script, and correspond to routines used
to allocate the application stack).
For the long run, the hope is that the stub implementation provided
by PikeOS is enhanced so that it includes vContSupported+ to the
$qSupported query, and then that the reply to the "vCont?" query
only return support for "continue" operations (thus exclusing "step"
operations). We could then use that information to reliably determine
at connection time that the target does not support single-stepping
and therefore automatically turn software single-stepping automatically
based on it.
gdb/ChangeLog:
* defs.h (enum gdb_osabi): Add GDB_OSABI_PIKEOS.
* osabi.c (gdb_osabi_names): Add name for GDB_OSABI_PIKEOS.
* arm-pikeos-tdep.c: New file.
* configure.tgt: Add arm-pikeos-tdep.o to the case of ARM
embedded system.
* Makefile.in (ALL_TARGET_OBS): Add arm-pikeos-tdep.o.
Tested on arm-pikeos and arm-elf using AdaCore's testsuite.
We also evaluated it on armhf-linux as a cross platform.
Building with mingw currently fails:
CXX unittests/mkdir-recursive-selftests.o
/home/emaisin/src/binutils-gdb/gdb/unittests/mkdir-recursive-selftests.c: In function ‘void selftests::mkdir_recursive::test()’:
/home/emaisin/src/binutils-gdb/gdb/unittests/mkdir-recursive-selftests.c:49:20: error: ‘mkdtemp’ was not declared in this scope
if (mkdtemp (base) == NULL)
^
Commit
e418a61a67 ("Move mkdir_recursive to common/filestuff.c")
moved this code, but also removed the HAVE_MKDTEMP guard which prevented
the mkdtemp call to be compiled on mingw.
We can either put back the HAVE_MKDTEMP ifdef, or import the gnulib
mkdtemp module, which provides the function for mingw. Since the
mkdir_recursive is susceptible to be used on mingw at some point, I
think it would be nice to have it tested on mingw, so I did the latter.
Once built, I tested it on Windows (copied the resulting gdb.exe on a
Windows machine, ran it, and ran "maint selftest mkdir_recursive"). It
failed, because the temporary directory is hardcoded to "/tmp/...". I
therefore added and used a new get_standard_temp_dir function, which
returns an appropriate temporary directory for the host platform.
gdb/ChangeLog:
* common/pathstuff.c (get_standard_temp_dir): New.
* common/pathstuff.h (get_standard_temp_dir): New.
* config.in: Re-generate.
* configure: Re-generate.
* configure.ac: Don't check for mkdtemp.
* gnulib/aclocal-m4-deps.mk: Re-generate.
* gnulib/aclocal.m4: Re-generate.
* gnulib/config.in: Re-generate.
* gnulib/configure: Re-generate.
* gnulib/import/Makefile.am: Re-generate.
* gnulib/import/Makefile.in: Re-generate.
* gnulib/import/m4/gnulib-cache.m4: Re-generate.
* gnulib/import/m4/gnulib-comp.m4: Re-generate.
* gnulib/import/m4/mkdtemp.m4: New file.
* gnulib/import/mkdtemp.c: New file.
* gnulib/update-gnulib.sh (IMPORTED_GNULIB_MODULES):
Add mkdtemp module.
* unittests/mkdir-recursive-selftests.c (test): Use
get_standard_temp_dir.
(_initialize_mkdir_recursive_selftests): Remove HAVE_MKDTEMP
ifdef.
* compile/compile.c (get_compile_file_tempdir): Likewise.
run_dump_test proposes an ld action but when trying to make use of it in
a gas test it gave me some Tcl error. It turns out that it references
the check_shared_lib_support procedure and ld_elf_shared_opt variable
both only available in ld-lib.exp. I've thus moved the procedure in
binutils-common.exp and defined the variable needed in the various
default.exp of testsuite that seem to be using run_dump_test.
Since check_shared_lib_support itself references the ld variable not
defined in binutils-common I've defined it from LD in run_dump_test and
fixed LD and LDFLAGS to be defined as expected by run_dump_test in the
various default.exp of testsuite using run_dump_test.
2018-11-01 Thomas Preud'homme <thomas.preudhomme@linaro.org>
binutils/
* testsuite/config/default.exp: Define LD, LDFLAGS and
ld_elf_shared_opt.
* testsuite/lib/binutils-common.exp (check_shared_lib_support): Moved
from ld-lib.exp.
(run_dump_test): Set ld to $LD.
gas/
* testsuite/config/default.exp: Define LD, LDFLAGS and
ld_elf_shared_opt.
ld/
* testsuite/lib/ld-lib.exp (check_shared_lib_support): Moved to
binutils-common.exp.
In AIX if gdb is debugging an application which has a signal handler
and reaches the signal handler frame, then we need to read the back
chain address from sigcontext saved on the stack, similarly the LR.
As backchain at an offset 0 will be 0, because we will have a
sigconext saved after the minimum stack size. So the correct
backchain will be at an offset after minimum stack and the LR at
an offset 8 will be of the signal millicode address.
If the back chain pointer is NULL and the LR field is in the kernel
segment(ex. 0x00004a14) then we can probably assume we are in a
signal handler.
sample output
(gdb) bt
0 sig_handle_aix (signo=11) at aix-sighandle.c:7
1 0x0000000000004a94 in ?? ()
(gdb)
expected output
(gdb) bt
0 sig_handle_aix (signo=11) at aix-sighandle.c:7
1 <signal handler called>
2 0x0000000100000748 in foo () at aix-sighandle.c:14
3 0x000000010000079c in main () at aix-sighandle.c:19
gdb/ChangeLog:
2018-11-01 Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
* rs6000-aix-tdep.c: Include "trad-frame.h" and "frame-unwind.h".
(SIG_FRAME_LR_OFFSET64): New define.
(SIG_FRAME_FP_OFFSET64): New define.
(aix_sighandle_frame_cache): New Function.
(aix_sighandle_frame_this_id): New Function.
(aix_sighandle_frame_prev_register): New Function.
(aix_sighandle_frame_sniffer): New Function.
(aix_sighandle_frame_unwind): New global variable.
(rs6000_aix_init_osabi): Install new frame unwinder.
gdb/testsuite/ChangeLog:
2018-11-01 Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
* gdb.arch/aix-sighandle.c: New file.
* gdb.arch/aix-sighandle.exp: New file.
Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
This causes a build problem when building GDB there, because
"common/common-defs.h" also defines _FORTIFY_SOURCE=2:
CXX gdb.o
In file included from ../../gdb/defs.h:28:0,
from ../../gdb/gdb.c:19:
../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
#define _FORTIFY_SOURCE 2
<built-in>: note: this is the location of the previous definition
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:1619: gdb.o] Error 1
Even though it is questionable whether Gentoo's approach is the
correct one:
https://jira.mongodb.org/browse/SERVER-29982https://bugs.gentoo.org/621036
it is still possible for GDB to be a bit more robust here and make
sure it just defines _FORTIFY_SOURCE if it hasn't been defined
already. This patch does that.
Tested by rebuilding and making sure the macro was defined.
gdb/ChangeLog:
2018-10-31 Sergio Durigan Junior <sergiodj@redhat.com>
PR gdb/23835
* common/common-defs.h: Don't redefine _FORTIFY_SOURCE if it's
already defined.
The gdb.arch/riscv-reg-aliases.exp test didn't take into account that
on RV64 (and RV128) the floating point registers are represented as a
union. This patch updates the test to handle this.
Tested against RV32 and RV64.
gdb/testsuite/ChangeLog:
* gdb.arch/riscv-reg-aliases.exp: Rewrite to take account of float
registers being unions.
Factor out common code related to vgdb setup and cleanup in valgrind-bt.exp,
valgrind-disp-step.exp and gdb.base/valgrind-infcall.exp.
Tested on x86_64-linux with and without --target_board=native-gdbserver.
2018-10-31 Tom de Vries <tdevries@suse.de>
* lib/valgrind.exp: New file.
(vgdb_start, vgdb_stop): New procs, factored out of ...
* gdb.base/valgrind-bt.exp: ... here, ...
* gdb.base/valgrind-disp-step.exp: ... here and ...
* gdb.base/valgrind-infcall.exp: ... here.
Top level:
Merge from GCC:
PR bootstrap/82856
* multilib.am: New file. From automake.
config:
Merge from GCC:
PR bootstrap/82856
* math.m4, tls.m4: Use AC_LANG_SOURCE.
zlib:
Merge from GCC.
PR bootstrap/82856
* Makefile.am: Include multilib.am.
* Makefile.in: Regenerate.
The get_valueof outputs the value it has read as part of the test name. This
causes test names to vary from run to run, and adds some noise when diffing
test results. e.g.:
-PASS: gdb.guile/scm-ports.exp: buffered: get valueof "$sp" (140737488343920)
+PASS: gdb.guile/scm-ports.exp: buffered: get valueof "$sp" (140737488343968)
-PASS: gdb.guile/scm-ports.exp: unbuffered: get valueof "$sp" (140737488343920)
+PASS: gdb.guile/scm-ports.exp: unbuffered: get valueof "$sp" (140737488343968)
This patch removes that, since it's probably not very useful.
Tested on x86_64-linux.
2018-10-31 Tom de Vries <tdevries@suse.de>
* lib/gdb.exp (get_valueof): Don't output read value in test name.
For Local Exec TLS model, the offset of the variable from the thread pointer
can be computed at static link time. This doesn't require GOT indirection.
The initial change is a bad fix for a problem during TLS GD -> LE relaxation.
The proper fix is to check whether _GLOBAL_OFFSET_TABLE_ is referenced,
create got section if yes. And the fix is already in the repository.
bfd/
2018-10-31 Renlin Li <renlin.li@arm.com>
* elfnn-aarch64.c (elfNN_aarch64_check_relocs): Don't create got
section for Local Exec TLS model.
Patch "[PowerPC] Add support for PPR and DSCR" used
PTRACE_GETREGSET/SETREGSET without including the fallback definitions
from "nat/linux-ptrace.h". Include this header to avoid breaking
builds in systems that don't define them.
gdb/ChangeLog:
2018-10-31 Pedro Franco de Carvalho <pedromfc@linux.ibm.com>
* ppc-linux-nat.c: Include nat/linux-ptrace.h.
gdb/gdbserver/ChangeLog:
2018-10-31 Pedro Franco de Carvalho <pedromfc@linux.ibm.com>
* linux-ppc-low.c: Include nat/linux-ptrace.h.
I encountered a binary compiled with Intel's C Compiler (ICC) version
14.0.5.212, which seemed to contain some non-standard DWARF.
The DWARF spec (V5 3.3.2) says:
Debugging information entries for C void functions should not have
an attribute for the return type.
However, what I observed in the DWARF from this ICC compiled binary
was this:
...
<0><857>: Abbrev Number: 1 (DW_TAG_compile_unit)
<858> DW_AT_comp_dir : (indirect string, offset: 0x48d): /tmp/
<85c> DW_AT_language : 1 (ANSI C)
<85d> DW_AT_name : (indirect string, offset: 0x77c): filename.c
<861> DW_AT_producer : (indirect string, offset: 0x520): Intel(R) C Intel(R) 64 Compiler ...
<865> DW_AT_low_pc : 0x4378d0
<86d> DW_AT_high_pc : 0x4378f0
<875> DW_AT_stmt_list : 0xa37
...
<1><7ea>: Abbrev Number: 2 (DW_TAG_base_type)
<7eb> DW_AT_byte_size : 0
<7ec> DW_AT_encoding : 5 (signed)
<7ed> DW_AT_name : (indirect string, offset: 0x58f): void
...
<1><7f1>: Abbrev Number: 3 (DW_TAG_subprogram)
<7f2> DW_AT_decl_line : 268
<7f4> DW_AT_decl_column : 30
<7f5> DW_AT_decl_file : 1
<7f6> DW_AT_type : <0x7ea>
<7fa> DW_AT_prototyped : 1
<7fb> DW_AT_name : (indirect string, offset: 0x761): function_foo
<7ff> DW_AT_MIPS_linkage_name: (indirect string, offset: 0x761): function_foo
<803> DW_AT_low_pc : 0x4378a0
<80b> DW_AT_high_pc : 0x4378d0
<813> DW_AT_external : 1
...
So function 'function_foo' has void return type, but still has a
DW_AT_type attribute for a 0 sized type called void.
What was found was that when the 'finish' command was used to leave
'function_foo', GDB would crash.
The problem is that in infcmd.c:print_return_value GDB tries to filter
out void return types, by looking for the TYPE_CODE_VOID, this fails
for the 'void' type as it has code TYPE_CODE_INT and GDB then tries to
print the 'void' type.
This eventually ends in a call to valprint.c:maybe_negate_by_bytes,
however, the len (length) of the value being negated is 0, which is
not detected or expected by this code, and invalid memory accesses
occur, some of which might cause GDB to crash.
The above DWARF was seen on version 14.0.5.212 of ICC.
I have also tested ICC versions 18.0.2.199 and 17.0.7.259, on both of
these versions, the DW_AT_type on the DW_TAG_subprogram has been
removed, bringing ICC inline with the DWARF standard, and with the
DWARF produced by GCC.
I only have limited access to these specific versions of ICC so I am
unable to get more specific details for when the generated DWARF
became non-standard or when it was changed to be more inline with the
DWARF standard.
Further testing revealed additional places where ICC produced 'void'
related DWARF that GDB struggles with. When I compiled code that
contained a function with this signature:
void funcx (void *arg);
on ICC 17/18, I got the following DWARF (notice the void return type
is now gone):
...
<1><32>: Abbrev Number: 2 (DW_TAG_subprogram)
<33> DW_AT_decl_line : 2
<34> DW_AT_decl_file : 1
<35> DW_AT_prototyped : 1
<36> DW_AT_name : (indirect string, offset: 0xc5): funcx
<3a> DW_AT_MIPS_linkage_name: (indirect string, offset: 0xc5): funcx
<3e> DW_AT_low_pc : 0x6dc
<46> DW_AT_high_pc : 0x703
<4e> DW_AT_external : 1
<2><4f>: Abbrev Number: 3 (DW_TAG_formal_parameter)
<50> DW_AT_decl_line : 2
<51> DW_AT_decl_file : 1
<52> DW_AT_type : <0x6a>
<56> DW_AT_name : arg
<5a> DW_AT_location : 2 byte block: 76 70 (DW_OP_breg6 (rbp): -16)
...
<1><6a>: Abbrev Number: 5 (DW_TAG_pointer_type)
<6b> DW_AT_type : <0x6f>
<1><6f>: Abbrev Number: 6 (DW_TAG_base_type)
<70> DW_AT_byte_size : 0
<71> DW_AT_encoding : 5 (signed)
<72> DW_AT_name : (indirect string, offset: 0xcb): void
...
However, the function argument 'arg' does still reference a 'void'
type. This case doesn't seem as obviously non-standard as the
previous one, but I think that the DWARF standard (V5 5.2) does
suggest that the above is not the recommended approach. If we compare
to the DWARF generated by GCC 7.3.1:
...
<1><68>: Abbrev Number: 5 (DW_TAG_subprogram)
<69> DW_AT_external : 1
<69> DW_AT_name : (indirect string, offset: 0x221): funcx
<6d> DW_AT_decl_file : 1
<6e> DW_AT_decl_line : 2
<6f> DW_AT_prototyped : 1
<6f> DW_AT_low_pc : 0x400487
<77> DW_AT_high_pc : 0x22
<7f> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
<81> DW_AT_GNU_all_call_sites: 1
<81> DW_AT_sibling : <0xa0>
<2><85>: Abbrev Number: 6 (DW_TAG_formal_parameter)
<86> DW_AT_name : arg
<8a> DW_AT_decl_file : 1
<8b> DW_AT_decl_line : 2
<8c> DW_AT_type : <0xa0>
<90> DW_AT_location : 2 byte block: 91 58 (DW_OP_fbreg: -40)
...
<1><a0>: Abbrev Number: 7 (DW_TAG_pointer_type)
<a1> DW_AT_byte_size : 8
...
Here we see that the DW_TAG_pointer_type doesn't reference any further
type. This also seems out of line with the DWARF standard (which I
think recommends using a DW_TAG_unspecified_type entry), however GDB
does handle the GCC generated DWARF better.
If we look at how GDB handles the DWARF from GCC, then we see this:
(gdb) print *arg
Attempt to dereference a generic pointer.
While on the current HEAD of master dereferencing arg causes undefined
behaviour which will likely crash GDB (for the same reason as was
described above for the 'finish' case). On earlier versions of GDB
the ICC DWARF would cause this:
(gdb) print *arg
$1 = 0
In this patch both the return type, and general variable/parameter
type handling is fixed by transforming the synthetic void entries in
the DWARF, the ones that look like this:
<1><6f>: Abbrev Number: 6 (DW_TAG_base_type)
<70> DW_AT_byte_size : 0
<71> DW_AT_encoding : 5 (signed)
<72> DW_AT_name : (indirect string, offset: 0xcb): void
into GDB's builtin void type. My criteria for performing the fix are:
1. Binary produced by any version of ICC,
2. We're producing an integer type,
3. The size is 0, and
4. The name is "void".
I ignore the signed / unsigned nature of the integer.
Potentially we could drop the ICC detection too, this should be a
reasonably safe transformation to perform, however, I'm generally
pretty nervous when it comes to modifying how the DWARF is parsed so,
for now, I have restricted this to ICC only.
I also added an assertion to maybe_negate_by_bytes. This is nothing
to do with the actual fix, but should detect incorrect use of this
function in the future, without relying on undefined behaviour to
crash GDB.
I added a new test that makes use the of the testsuite's DWARF
generator. As it is tricky to create target independent tests that
pass function parameters using the DWARF generator (as specifying the
argument location is target specific) I have instead made use of a
global variable void*. This still shows the issue.
We already have a predicate in the DWARF parser to detect versions of
ICC prior to 14, however, this issue was spotted on a later version.
As a result I've added a new predicate that is true for any version of
ICC.
gdb/ChangeLog:
* dwarf2read.c (struct dwarf2_cu): Add producer_is_icc field.
(producer_is_icc): New function.
(check_producer): Set producer_is_icc field on dwarf2_cu.
(dwarf2_init_integer_type): New function.
(read_base_type): Call dwarf2_init_integer_type instead of
init_integer_type in all cases.
(dwarf2_cu::dwarf2_cu): Initialise producer_is_icc field.
* valprint.c (maybe_negate_by_bytes): Add an assertion that the
LEN is greater than 0.
gdb/testsuite/ChangeLog:
* gdb.dwarf2/void-type.c: New file.
* gdb.dwarf2/void-type.exp: New file.
This test never used to test the output of objdump as the old 'error-output'
check would exit after verifying the output in stdout and stderr from the
assembler. Given the use of warning_output now, the objdump runs and expects
its output to be verified. Assuming the correct disassembly of these
instructions is tested elsewhere given we never tested them here, this patch
removes the objdump run.
gas/ChangeLog
2018-10-31 Andre Vieira <andre.simoesdiasvieira@arm.com>
* testsuite/gas/arm/armv8-a+rdma-warning.d: Remove objdump execution.
The old test never checked the objdump output since the 'error-output' directive
would exit and thus never run objdump. When this test was changed to adhere to
use the new warning_output we started to run objdump. The expected objdump
output was old and had bitrotten, this fixes the layout, as the "disassembly"
itself did not change.
gas/ChangeLog
2018-10-31 Andre Vieira <andre.simoesdiasvieira@arm.com>
* testsuite/gas/arm/udf.d: Update expected output.
This test has been failing for a while and it could be argued that since we
started testing 'arm7t' here (and not Armv1) the test itself was wrong. So I
changed the assembly to Armv1. Given the changes to objdump when
"disassembling all" it seemed like a good idea to force the disassembly to
'armv2' instead and actually accept the disassembly of the 26-bit Architecture
variants of tst, teq, cmn and cmp.
gas/ChangeLog
2018-10-31 Andre Vieira <andre.simoesdiasvieira@arm.com>
* testsuite/gas/arm/armv1.d: Assemble for Armv1 and disassemble for
Armv2.
