Re: [PATCH 4/6] Disassembly unit test: disassemble one instruction

Pedro Alves
2017-02-02T16:46:36Z

On 01/24/2017 03:23 PM, Yao Qi wrote:
> On 17-01-20 00:03:48, Pedro Alves wrote:
>>> + /* Test gdb_disassembler for a given gdbarch by reading data from a
>>> + pre-allocated buffer. If you want to see the disassembled
>>> + instruction printed to gdb_stdout, set DISASSEMBLER_TEST_VERBOSE
>>> + to true. */
>>> +
>>> + class gdb_disassembler_test : public gdb_disassembler
>>> + {
>>> + public:
>>> +
>>> + const bool DISASSEMBLER_TEST_VERBOSE = false;
>>
>> static. We give macros long unique names in order to
>> avoid naming conflicts, but if this is no longer a macro,
>> the name could be shortened, to e.g., just:
>>
>> static const bool verbose = false;
>>
>
> gdb_disassembler_test is a local class, so it can't have a static data
> member.

The end result is broken then, unfortunately. I didn't realize it
either until I saw a spurious fail in the gdb.base/unittest.exp test
today.

Running

gdb -ex "maintenance selftest"

manually I see:

~~~
Type "apropos word" to search for commands related to "word".
warning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default ARC600 settings.

.long 0x256f003fwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default ARC600 settings.

.long 0x256f003fwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default A6 settings.

.long 0x256f003fwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default ARC601 settings.

warning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default ARC700 settings.

brkwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default A7 settings.

brkwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default ARCv2 settings.

brkwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default EM settings.

brkwarning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default HS settings.

brkmov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0mov r0, #0breakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakbreakM3.L = 0xffff;/* ( -1) M3=0x0xffff(65535) */break 8break 8warning: A handler for the OS ABI "GNU/Linux" is not built into this configuration
of GDB. Attempting to continue with the default cris:common_v10_v32 settings.
~~~

etc. (this is what shows up in the gdb.log too).

Note the odd bits of instructions printed.

They appear because here:

class gdb_disassembler_test : public gdb_disassembler
{
public:

const bool verbose = false;

explicit gdb_disassembler_test (struct gdbarch *gdbarch,
const gdb_byte *insn,
size_t len)
: gdb_disassembler (gdbarch,
(verbose ? gdb_stdout : &null_stream),
gdb_disassembler_test::read_memory),

specifically in this line:

(verbose ? gdb_stdout : &null_stream),

"verbose" has not been initialized yet, because the order of initialization
is always base classes first. I.e. "verbose" is only initialized after
the base constructor is called. Since the gdb_disassembler_test object
is created on the stack, "verbose" has garbage at that point, and
thus we end up with the stream incorrectly pointing to gdb_stdout.

I was surprised that GCC doesn't warn about this. I filed a GCC
bug here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

I'm not sure whether this was the reason for the unittest.exp
random failure, but I suspect so. (I failed to save the log,
and also forgot to check whether all selftests had passed.)

The patchlet below fixes the spurious insn printing. OK?

diff --git a/gdb/disasm-selftests.c b/gdb/disasm-selftests.c
index 7d0b006..9eb80b4 100644
--- a/gdb/disasm-selftests.c
+++ b/gdb/disasm-selftests.c
@@ -102,13 +102,12 @@ print_one_insn_test (struct gdbarch *gdbarch)
/* Test gdb_disassembler for a given gdbarch by reading data from a
pre-allocated buffer. If you want to see the disassembled
instruction printed to gdb_stdout, set verbose to true. */
+ static const bool verbose = false;

class gdb_disassembler_test : public gdb_disassembler
{
public:

- const bool verbose = false;
-
explicit gdb_disassembler_test (struct gdbarch *gdbarch,
const gdb_byte *insn,
size_t len)