I saw some failures in the test gdb.mi/mi-multi-commands.exp that I
added recently. This test was added in commit:
commit d08cbc5d32
Date: Wed Dec 22 12:57:44 2021 +0000
gdb: unbuffer all input streams when not using readline
The failures I see only occurred when my machine was very heavily
loaded.
In this test I send multiple commands from dejagnu to gdb with a
single send_gdb call. In a well behaving world what I want to happen
is that the gdb console sees both commands arrive and echos the text
of those commands. Then gdb starts processing the first command,
prints the result, and then processes the second command, and prints
the result.
However, what I saw in my loaded environment was that only after
sending the two commands, only the first command was echoed to gdb's
terminal. Then gdb started processing the first command, and started
to write the output. Now, mixed in with the first command output, the
second command was echoed to gdb's terminal. Finally, gdb would
finish printing the first command output, and would read and handle
the second command.
This mixing of command echoing with the first command output was
causing the test matching patterns to fail.
In this commit I change the command I use in the test from a CLI
command to an MI command, this reduces the number of lines of output
that come from the test, CLI commands sent through the MI interpreter
are echoed back like this:
(gdb)
set $a = "FIRST COMMAND"
&"set $a = \"FIRST COMMAND\"\n"
^done
(gdb)
While this is not the case for true MI command:
(gdb)
-data-evaluate-expression $a
^done,value="\"FIRST COMMAND\""
(gdb)
Less output makes for simpler patterns to match against.
Next, when sending two command to gdb I was previously trying to spot
the output of the first command followed by the prompt with nothing
between. This is not really needed, for the first command I can look
for just the ^done,value="\"FIRST COMMAND\"" string, then I can start
looking for the output of the second command.
So long as the second pattern matches up to the gdb prompt, then I can
be sure than nothing is left over in the expect buffer to muck up
later matches.
As to see the second command output gdb must have read in the second
command, the second command output never suffers from the corruption
that the first command output does.
Since making this change, I've not seen a failure in this test.
This fixes a GDB crash reported in bug pr/28900, related to reading in
some stabs debug information.
In this commit my goal is to stop GDB crashing. I am not trying to
ensure that GDB makes the best possible use of the available stabs
debug information. At this point I consider stabs a legacy debug
format, with only limited support in GDB.
So, the problem appears to be that, when reading in the stabs data, we
need to find a N_SO entry, this is the entry that defines the start of
a compilation unit (or at least the location of a corresponding source
file).
It is while handling an N_SO that GDB creates a psymtab to hold the
incoming debug information (symbols, etc).
The problem we hit in the bug is that we encounter some symbol
information (an N_PC entry) outside of an N_SO entry - that is we find
some symbol information that is not associated with any source file.
We already have some protection for this case, look (in
read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f',
if we have no psymtab (the pst variable is nullptr) then we issue a
complaint. However, for whatever reason, in both 'f' and 'F'
handling, there is one place where we assume that the pst
variable (the psymtab) is not nullptr. This is a mistake.
In this commit, I guard these two locations (in 'f' and 'F' handling)
so we no longer assume pst is not nullptr.
While I was at it, I audited all the other uses of pst in
read_dbx_symtab, and in every potentially dangerous case I added a
nullptr check, and issue a suitable complaint if pst is found to be
nullptr.
It might well be true that we could/should do something smarter if we
see a debug symbol outside of an N_SO entry, and if anyone wanted to
do that work, they're welcome too. But this commit is just about
preventing the nullptr access, and the subsequent GDB crash.
I don't have any tests for this change, I have no idea how to generate
weird stabs data for testing. The original binary from the bug report
now loads just fine without GDB crashing.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900
While taking a look through dbxread.c I spotted a couple of places
where making use of std::string would remove the need for manual
memory allocation and memcpy.
During review Simon pointed out that the same code exists in
xcoffread.c, so I've applied the same fix there too.
There should be no user visible changes after this commit.
A have had situation where a unfiltered output (done using
fputs_unfiltered) ended up triggering pagination. The backtrace for this was:
...
#24 0x000055839377ee4e in check_async_event_handlers () at ../../gdb/async-event.c:335
#25 0x0000558394b67b57 in gdb_do_one_event () at ../../gdbsupport/event-loop.cc:216
#26 0x0000558394587454 in gdb_readline_wrapper (prompt=0x7ffd907712d0 "--Type <RET> for more, q to quit, c to continue without paging--") at ../../gdb/top.c:1148
#27 0x0000558394707270 in prompt_for_continue () at ../../gdb/utils.c:1438
#28 0x00005583947088b3 in fputs_maybe_filtered (linebuffer=0x60c0000f4000 " [...quite big message...]", stream=0x60300028e9d0, filter=0) at ../../gdb/utils.c:1752
#29 0x0000558394708e57 in fputs_unfiltered (linebuffer=0x60c0000f4000 " [...quite big message...]", stream=0x60300028e9d0) at ../../gdb/utils.c:1811
...
This comes from what appears to be a oversight in fputs_maybe_filtered. This
function has a FILTER parameter which if true makes the function pause after
every screenful (i.e. triggers pagination).
The filter parameter is correctly used to guard the first place where
prompt_for_continue. There is a second place in the function which can call
prompt_for_continue, but is currently unguarded. I believe that this is an
oversight, this patch fixes that.
Tested on Linux-x86_64, no regression observed.
Change-Id: Iad8ffd50a87cf20077500878e2564b5a7dc81ece
As seen in https://sourceware.org/bugzilla/show_bug.cgi?id=24069 this
code will typically wait4() a second time on the same process that was
already wait4()'d a few lines above. While this used to be
harmless/idempotent (when we assumed that the process already exited),
this now causes a deadlock in the WIFSTOPPED case.
The early (~2019) history of bug #24069 cautiously suggests to use
WNOHANG instead of outright deleting the call. However, tests on the
current version of Darwin (Big Sur) demonstrate that gdb runs just fine
without a redundant call to wait4(), as would be expected.
Notwithstanding the debatable value of conserving bug compatibility with
an OS release that is more than a decade old, there is scant evidence of
what that double-wait4() was supposed to achieve in the first place - A
cursory investigation with `git blame` pinpoints commits bb00b29d78
and a80b95ba67 from the 2008-2009 era, but fails to answer the
"why" question conclusively.
Co-Authored-By: Philippe Blain <levraiphilippeblain@gmail.com>
Change-Id: Id4e4415d66d6ff6b3552b60d761693f17015e4a0
This adds a constructor to bound_minimal_symbol, to avoid a build
failure with clang that Simon pointed out.
I also took the opportunity to remove some redundant initializations,
and to change one use of push_back to emplace_back, as suggested by
Simon.
Much of the gas source and older BFD source use "long" for function
parameters and variables, when other types would be more appropriate.
This patch fixes one of those cases. Dollar labels and numeric local
labels do not need large numbers. Small positive itegers are usually
all that is required. Due to allowing longs, it was possible for
fb_label_name and dollar_label_name to overflow their buffers.
* symbols.c: Delete unnecessary forward declarations.
(dollar_labels, dollar_label_instances): Use unsigned int.
(dollar_label_defined, dollar_label_instance): Likewise.
(define_dollar_label): Likewise.
(fb_low_counter, fb_labels, fb_label_instances): Likewise.
(fb_label_instance_inc, fb_label_instance): Likewise.
(fb_label_count, fb_label_max): Make them size_t.
(dollar_label_name, fb_label_name): Rewrite using sprintf.
* symbols.h (dollar_label_defined): Update prototype.
(define_dollar_label, dollar_label_name): Likewise.
(fb_label_instance_inc, fb_label_name): Likewise.
* config/bfin-lex.l (yylex): Remove unnecessary casts.
* expr.c (integer_constant): Likewise.
* read.c (read_a_source_file): Limit numeric label range to int.
There are quite a few ubsan warnings in gas. This one disappears with
a code tidy.
* read.c (s_app_line): Rename 'l' to 'linenum'. Avoid ubsan
warning.
BFD generally doesn't handle anything but a power of two section
alignment, and ELF sh_addralign is required to be an integral power of
two (or zero) by the ELF spec. Of course this is ignored by fuzzers,
and because bfd_log2 rounds up, we can end up with alignment_power
being 32 on a 32-bit object or 64 on a 64-bit object. That then
triggers ubsan warnings in places like bfd_update_compression_header
where we want to convert from alignment_power back to an alignment.
I suppose we could reject object files that have non-compliant
sh_addralign, but I think it's also reasonable to use the greatest
power of two divisor of sh_addralign, ie. the rightmost 1 bit.
* elf.c (_bfd_elf_make_section_from_shdr): Use greatest power
of two divisor of sh_addralign.
(_bfd_elf_assign_file_position_for_section): Likewise.
(assign_file_positions_for_non_load_sections): Likewise.
There was an omission on 3e6dc39ed7 "sim/testsuite: Set
global_cc_os also when no compiler is found"; global_cc_os
wasn't set for other than the primary target, which means
that the "unguarded" use of global_cc_os in
testsuite/cris/c/c.exp caused the dreaded "ERROR: can't read
"global_cc_os": no such variable" when e.g. configuring for
pru-elf and doing "make check-sim". Better initializing
both variables at the top to default values, rather than
adding another single 'set global_cc_os ""', to reduce the
risk of not setting them properly if or when that
if-statement-chain is made longer.
sim/testsuite:
* lib/sim-defs.exp (sim_init_toolchain): Default
global_cc_os and global_cc_works properly, before if-chain.
Add has_sib to struct instr_info and use SIB info only if ins->has_sib
is true.
PR binutils/28892
* i386-dis.c (instr_info): Add has_sib.
(get_sib): Set has_sib.
(OP_E_memory): Replace havesib with ins->has_sib.
(OP_VEX): Use ins->sib.index only if ins->has_sib is true.
It is possible for a compiler to optimize a function in a such ways that
the function does not follow the calling convention of the target. In
such situation, the compiler can use the DW_AT_calling_convention
attribute with the value DW_CC_nocall to tell the debugger that it is
unsafe to call the function. The DWARF5 standard states, in 3.3.1.1:
> If the value of the calling convention attribute is the constant
> DW_CC_nocall, the subroutine does not obey standard calling
> conventions, and it may not be safe for the debugger to call this
> subroutine.
Non standard calling convention can affect GDB's assumptions in multiple
ways, including how arguments are passed to the function, how values are
returned, and so on. For this reason, it is unsafe for GDB to try to do
the following operations on a function with marked with DW_CC_nocall:
- call / print an expression requiring the function to be evaluated,
- inspect the value a function returns using the 'finish' command,
- force the value returned by a function using the 'return' command.
This patch ensures that if a command which relies on GDB's knowledge of
the target's calling convention is used on a function marked nocall, GDB
prints an appropriate message to the user and does not proceed with the
operation which is unreliable.
Note that it is still possible for someone to use a vendor specific
value for the DW_AT_calling_convention attribute for example to indicate
the use of an alternative calling convention. This commit does not
prevent this, and target dependent code can be adjusted if one wanted to
support multiple calling conventions.
Tested on x86_64-Linux, with no regression observed.
Change-Id: I72970dae68234cb83edbc0cf71aa3d6002a4a540
Add an argument to the get_return_value function to indicate the symbol
of the function the debuggee is returning from. This will be used by
the following patch.
Since the function return type can be deduced from the symbol remove the
value_type argument which becomes redundant.
No user visible change after this patch.
Tested on x86_64-linux.
Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
...when we know we have a working compiler. This will reduce
the risk of faulty edits by exposing them rather than hiding
them as "unresolved". It also harmonizes behavior with that of
run_sim_test.
* c/c.exp: Mark C tests failing compilation test errors.
Calls to basename were added here as part of commit
e1e1ae6e9b "sim: testsuite: fix objdir handling", but that
commit missed adding "#include <libgen.h>" or the equivalent
GNU extension, see basename(3). Fixing that shows a logical
error in the change to openpf1.c; the non-/-prefixed
code-path was changed instead of the "/"-prefixed code-path,
which is the one executed after that commit.
For "newlib" these tests failed linking after that commit.
Recent newlib has the (asm-renamed) GNU-extension-variant of
basename, but we're better off not using it at all.
Unfortunately, compilation failures for C tests run by the
machinery in c.exp are currently just marked "unresolved",
in contrast to C and assembler tests run by calling
run_sim_test.
The interaction of calling with the full program-path vs.
use of --sysroot exposes a consistency problem: when
--sysroot is used, argv[0] isn't the path by which the
program can find itself. It's undecided whether argv[0] for
the program running in the simulator should be edited
(related to the naked argument to the simulator before
passing on to the simulated program) to remove a leading
--sysroot. Either way, such a change would be out of scope
for this commit.
* c/stat3.c (mybasename): New macro. Use it instead of basename.
* c/openpf1.c: Correct basename-related change and update related
comment.
With commit 7a259895bb "sim: testsuite: expand arch specific
toolchain settings", trying to use out-of-tree ld and as at test-time
broke for the "primary target", like when testing a release-tarball.
Subsequent to that commit, all assembler tests without in-tree-built
tools FAIL, getting errors when trying to call
$(abs_builddir)/../gas/as-new. But, that isn't the actual culprint;
it's actually it's its immediate predecessor, commit 8996c21067
"sim: testsuite: setup per-port toolchain settings for multitarget
build", which hardcodes in-tree-paths to those tools instead of
considering e.g. $(<X>_FOR_TARGET), the preferred overridable variable
for single-target builds, as set up by the toplevel Makefile.
This commit calls GCC_TARGET_TOOL (a deceptive name; gcc-specific
features aren't used) from toplev/config/acx.m4, somewhat like calls
in toplev/configure.ac but without the NCN_STRICT_CHECK_TARGET_TOOLS
step, for each X to find a value for $(<X>_FOR_TARGET). N.B.: in-tree
tools still override any ${target}-${tool} found in $PATH, i.e. only
previously broken builds are affected.
The variables $(<X>_FOR_TARGET) are usually overridden by the toplevel
Makefile to the same value or better, but has to be set here too, as
automake "wants" Makefiles to be self-contained (you get an error
pointing out that the variable may be empty). If it hadn't been for
that, SIM_AC_CHECK_TOOLCHAIN_FOR_PRIMARY_TARGET would not be needed.
This detail should only (positively) affect users invoking "make
check" in sim/ instead of "make check-sim" (or "make check") at the
toplevel. Now the output from "configure" matches the target tools
actually used by sim at test-time, for the "primary target".
Using $(CC) for "example-" targets CC_FOR_TARGET is not changed, as
that appears to be a deliberate special-case.
Note that all tools still have to be installed and present in
$PATH at configure-time to be properly used at test-time.
sim:
* m4/sim_ac_toolchain.m4 (SIM_AC_CHECK_TOOLCHAIN_FOR_PRIMARY_TARGET):
New defun.
(SIM_TOOLCHAIN_VARS): Call it using AC_REQUIRE, and use variables
AS_FOR_TARGET, LD_FOR_TARGET and CC_FOR_TARGET instead of hard-coded
values.
* Makefile.in, configure: Regenerate.
With --disable-sim-hardware (--enable-sim-hardware=no),
whose default was changed to --enable-sim-hardware(=yes) in
commit 34cf511206, building for cris-elf fails as
sim_hw_parse then doesn't exist.
A cris-elf simulator configured for --enable-sim-hardware
(or the default after to the mentioned commit) runs about
2.5x slower than one configured --disable-sim-hardware.
A further 2-5% performance regression was not investigated.
When sim_hw_parse doesn't exist, --cris-900000xx can't be
supported. The best action here is to remove it completely,
so its absence can be identified through --help, but
avoiding littering the code with "#if WITH_HW".
sim/cris:
* sim-if.c (cris_options) [WITH_HW]: Conditionalize
support of option --cris-900000xx.
(sim_open) [WITH_HW]: Conditionalize sim_hw_parse
call.
Apply the new run_sim_test option "require" as in "#require
simoption --cris-900000xx" for all tests using that option.
This allows a clean test-suite-run for a build with
--disable-sim-hardware, where that option is not supported,
by skipping those tests as "untested".
sim/testsuite/cris:
* asm/io1.ms, asm/io2.ms, asm/io3.ms, asm/io6.ms,
asm/io7.ms: Call "#require: simoption --cris-900000xx".
Simulator features can be present or not, typically
depending on different-valued configure options, like
--enable-sim-hardware[=off|=on]. To avoid failures in
test-suite-runs when testing such configurations, a new
predicate is needed, as neither "target", "progos" nor
"mach" fits cleanly.
The immediate need was to check for presence of a simulator
option, but rather than a specialized "requires-simoption:"
predicate I thought I'd handle the general (parametrized)
need, so here's a generic predicate machinery and a (first)
predicate to use together with it; checking whether a
particular option is supported, by looking at "run --help"
output. This was inspired by the check_effective_target_
machinery in the gcc test-suite.
Multiple "requires: <requirement> <parameter>" form a list of
predicates (with parameters), to be used as a conjunction.
sim/testsuite:
* lib/sim-defs.exp (sim_check_requires_simoption): New function.
(run_sim_test): Support "requires: <requirement> <parameter>".
For reasons that remain largely to be investigated (besides
the apparent lack of synchronization between two processes),
this test fails randomly, with two different sets of common
outputs. Curiously, that doesn't happen for the other
similar tests. There's a comment that mentions this, though
that doesn't make it a sustainable part of a test-suite.
(Known-blinking tests should be disabled until fixed.)
sim/testsuite/cris:
* hw/rv-n-cris/irq1.ms: Disable by use of a never-matched
"progos" value.
Commit a39487c668 "sim: cris: use -sim with C tests for cris-elf
targets" caused " -sim" to be appended to CFLAGS_FOR_TARGET for
cris*-*-elf, where testing had until then relied on
"RUNTESTFLAGS=--target_board=cris-sim" being passed when running "make
check-sim", adding the right options. While "-sim" happens to work,
the baseboard-file cris-sim.exp uses "-sim3" so for consistency use
that instead.
Then commit b42f20d2ac "sim: testsuite: drop most specific istarget
checks" caused " -sim" to be appended for *all* targets, which just
doesn't work. For example, for crisv32-linux-gnu, that's not a
recognized option and will cause a dejagnu error and further testing
in c.exp will be aborted.
While cris-sim.exp appends "-static" for *-linux-gnu, further changes
in the test-suite have caused "linux"-specific tests to break, so that
part will be tended to separately.
But, save and restore CFLAGS_FOR_TARGET around the modification and
use where needed, to not have the CRIS-specific modification affect a
continuing test-run (possibly for other targets).
sim/testsuite/cris:
* c/c.exp (CFLAGS_FOR_TARGET): Replace appended option " -sim"
with " -sim3", but do it conditionally for newlib targets. Save
and restore CFLAGS_FOR_TARGET in saved_CFLAGS_FOR_TARGET such
that it doesn't affect the value of CFLAGS_FOR_TARGET outside
c.exp.
If we don't set this variable, it doesn't exist, and using "#progos:"
in an assembler-file will cause an error rather than just skipping the
test, viz:
Running /src/sim/testsuite/cris/hw/rv-n-cris/rvc.exp ...
ERROR: tcl error sourcing /src/sim/testsuite/cris/hw/rv-n-cris/rvc.exp.
ERROR: can't read "global_cc_os": no such variable
while executing
"if { $opts(progos) != "" && $opts(progos) != $global_cc_os } {
untested $subdir/$name
return
}"
(procedure "run_sim_test" line 102)
Neither the commit introducing progos, nor the top comment
in run_sim_test, mentions progos as intended only for C
tests, or that its use must be gated on $global_cc_works !=
0, so (not) setting it in the no-working-compiler path seems
just overlooked.
Allowing it to be used for assembler tests makes it usable
for e.g. an always-false predicate and in expressions in
.exp files without gating on $global_cc_works != 0.
With this patch, global_cc_os is set to "", just as for "unknown OS".
sim/testsuite:
* lib/sim-defs.exp (sim_init_toolchain): Set global_cc_os also when
no working target C compiler is found.
In 5ee0bc23a6 "sim: clean up bfd_vma printing" there was
an additional introduction of PRIx32 and PRIu32 but just in
sim/cris/sim-if.c. One type of bug was fixed in commit
d16ce6e4d5 "sim: cris: fix memory setup typos" but one
remained; the PRIu32 usage is wrong, as hex output is
desired; note the 0x prefix.
Without this fix, you'll see output like:
memory map 0:0x4000..0x5fff (8192 bytes) overlaps 0:0x0..0x16383 (91012 bytes)
program stopped with signal 6 (Aborted).
for some C programs, like some of the ones in the sim/cris/c
testsuite from where the example is taken (freopen2.c).
The bug behavior was with memory allocation. With an
attempt to allocate memory using the brk syscall such that
the room up to the next 8192-byte "page boundary" wasn't
sufficient, the simulator memory allocation machinery horked
on a consistency error when trying to allocate a memory
block to raise the "end of the data segment": there was
already memory allocated at that address.
Unfortunately, none of the programs in sim/cris/asm exposed
this bug at the time, but an assembler test-case is
committed after this fix.
sim/cris:
* sim-if.c (sim_open): Correct PRIu32 to PRIx32.
