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

[Pedro Alves <palves@redhat.com>]

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)