[RFA,4/7] Use ui_out_emit_tuple in disasm.c
Commit Message
This changes one spot in disasm.c to use ui_out_emit_tuple. This
patch required a large reindentation, so I've separated it out.
ChangeLog
2017-09-09 Tom Tromey <tom@tromey.com>
* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn): Use
ui_out_emit_tuple.
---
gdb/ChangeLog | 5 ++
gdb/disasm.c | 164 +++++++++++++++++++++++++++++-----------------------------
2 files changed, 87 insertions(+), 82 deletions(-)
Comments
On 2017-09-09 17:35, Tom Tromey wrote:
> This changes one spot in disasm.c to use ui_out_emit_tuple. This
> patch required a large reindentation, so I've separated it out.
"git show -w" does wonders! The hard part is figuring out if there's
something else in this big function using cleanups (and therefore if the
cleanup variable should be kept).
Anyway, this LGTM.
Simon
On 2017-09-09 02:35 PM, Simon Marchi wrote:
> On 2017-09-09 17:35, Tom Tromey wrote:
>> This changes one spot in disasm.c to use ui_out_emit_tuple. This
>> patch required a large reindentation, so I've separated it out.
>
> "git show -w" does wonders! The hard part is figuring out if there's something else in this big function using cleanups (and therefore if the cleanup variable should be kept).
>
> Anyway, this LGTM.
>
> Simon
>
I found a regression, bisect pointed to this patch (which makes sense).
1. Run "disassemble /s" on a small enough terminal so that pagination kicks in.
2. Type q <return> to quit pagination.
I get:
---Type <return> to continue, or q <return> to quit---q
/home/emaisin/src/binutils-gdb/gdb/ui-out.c:344: internal-error: void ui_out::pop_level(ui_out_type): Assertion `current_level ()->type () == type' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
Simon
On 2017-10-12 11:36 AM, Simon Marchi wrote:
> On 2017-09-09 02:35 PM, Simon Marchi wrote:
>> On 2017-09-09 17:35, Tom Tromey wrote:
>>> This changes one spot in disasm.c to use ui_out_emit_tuple. This
>>> patch required a large reindentation, so I've separated it out.
>>
>> "git show -w" does wonders! The hard part is figuring out if there's something else in this big function using cleanups (and therefore if the cleanup variable should be kept).
>>
>> Anyway, this LGTM.
>>
>> Simon
>>
>
> I found a regression, bisect pointed to this patch (which makes sense).
>
> 1. Run "disassemble /s" on a small enough terminal so that pagination kicks in.
> 2. Type q <return> to quit pagination.
>
> I get:
>
> ---Type <return> to continue, or q <return> to quit---q
> /home/emaisin/src/binutils-gdb/gdb/ui-out.c:344: internal-error: void ui_out::pop_level(ui_out_type): Assertion `current_level ()->type () == type' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
>
> Simon
>
Mixing RAII and cleanups probably changed the order in which the uiout elements
are closed.
In an outer scope (likely do_mixed_source_and_assembly), some uiout elements are
opened and cleanups are installed. In pretty_print_insn, some other uiout elements
are opened and RAII closers are used. When typing 'q', the cleanups are ran, just
before the quit exception would be thrown. The RAII closers have not ran yet, because
they would be when the exception is actually thrown. The solution would be to finish
converting the uiout cleanups to RAII (I won't have time to do that in the near future,
feel free to pick it up).
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
Simon> The solution would be to finish
Simon> converting the uiout cleanups to RAII (I won't have time to do that in the near future,
Simon> feel free to pick it up).
I think I'll just back out this particular change.
The way disasm code uses cleanups is very hard to untangle.
Tom
Tom> I think I'll just back out this particular change.
Tom> The way disasm code uses cleanups is very hard to untangle.
It turns out that with gdb::optional, you don't have to untangle it.
I'm testing a patch to convert the rest of disasm.c away from cleanups.
Tom
@@ -1,5 +1,10 @@
2017-09-09 Tom Tromey <tom@tromey.com>
+ * disasm.c (gdb_pretty_print_disassembler::pretty_print_insn): Use
+ ui_out_emit_tuple.
+
+2017-09-09 Tom Tromey <tom@tromey.com>
+
* target.c (flash_erase_command): Use ui_out_emit_tuple.
* stack.c (print_frame): Use ui_out_emit_tuple.
* spu-tdep.c (info_spu_event_command): Use ui_out_emit_tuple.
@@ -198,95 +198,95 @@ gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
int offset;
int line;
int size;
- struct cleanup *ui_out_chain;
char *filename = NULL;
char *name = NULL;
CORE_ADDR pc;
struct gdbarch *gdbarch = arch ();
- ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
- pc = insn->addr;
-
- if (insn->number != 0)
- {
- uiout->field_fmt ("insn-number", "%u", insn->number);
- uiout->text ("\t");
- }
-
- if ((flags & DISASSEMBLY_SPECULATIVE) != 0)
- {
- if (insn->is_speculative)
- {
- uiout->field_string ("is-speculative", "?");
-
- /* The speculative execution indication overwrites the first
- character of the PC prefix.
- We assume a PC prefix length of 3 characters. */
- if ((flags & DISASSEMBLY_OMIT_PC) == 0)
- uiout->text (pc_prefix (pc) + 1);
- else
- uiout->text (" ");
- }
- else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
- uiout->text (pc_prefix (pc));
- else
- uiout->text (" ");
- }
- else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
- uiout->text (pc_prefix (pc));
- uiout->field_core_addr ("address", gdbarch, pc);
-
- if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
- &line, &unmapped))
- {
- /* We don't care now about line, filename and unmapped. But we might in
- the future. */
- uiout->text (" <");
- if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
- uiout->field_string ("func-name", name);
- uiout->text ("+");
- uiout->field_int ("offset", offset);
- uiout->text (">:\t");
- }
- else
- uiout->text (":\t");
-
- if (filename != NULL)
- xfree (filename);
- if (name != NULL)
- xfree (name);
-
- m_insn_stb.clear ();
-
- if (flags & DISASSEMBLY_RAW_INSN)
- {
- CORE_ADDR end_pc;
- bfd_byte data;
- int err;
- const char *spacer = "";
-
- /* Build the opcodes using a temporary stream so we can
- write them out in a single go for the MI. */
- m_opcode_stb.clear ();
-
+ {
+ ui_out_emit_tuple tuple_emitter (uiout, NULL);
+ pc = insn->addr;
+
+ if (insn->number != 0)
+ {
+ uiout->field_fmt ("insn-number", "%u", insn->number);
+ uiout->text ("\t");
+ }
+
+ if ((flags & DISASSEMBLY_SPECULATIVE) != 0)
+ {
+ if (insn->is_speculative)
+ {
+ uiout->field_string ("is-speculative", "?");
+
+ /* The speculative execution indication overwrites the first
+ character of the PC prefix.
+ We assume a PC prefix length of 3 characters. */
+ if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+ uiout->text (pc_prefix (pc) + 1);
+ else
+ uiout->text (" ");
+ }
+ else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+ uiout->text (pc_prefix (pc));
+ else
+ uiout->text (" ");
+ }
+ else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+ uiout->text (pc_prefix (pc));
+ uiout->field_core_addr ("address", gdbarch, pc);
+
+ if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
+ &line, &unmapped))
+ {
+ /* We don't care now about line, filename and unmapped. But we might in
+ the future. */
+ uiout->text (" <");
+ if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
+ uiout->field_string ("func-name", name);
+ uiout->text ("+");
+ uiout->field_int ("offset", offset);
+ uiout->text (">:\t");
+ }
+ else
+ uiout->text (":\t");
+
+ if (filename != NULL)
+ xfree (filename);
+ if (name != NULL)
+ xfree (name);
+
+ m_insn_stb.clear ();
+
+ if (flags & DISASSEMBLY_RAW_INSN)
+ {
+ CORE_ADDR end_pc;
+ bfd_byte data;
+ int err;
+ const char *spacer = "";
+
+ /* Build the opcodes using a temporary stream so we can
+ write them out in a single go for the MI. */
+ m_opcode_stb.clear ();
+
+ size = m_di.print_insn (pc);
+ end_pc = pc + size;
+
+ for (;pc < end_pc; ++pc)
+ {
+ read_code (pc, &data, 1);
+ m_opcode_stb.printf ("%s%02x", spacer, (unsigned) data);
+ spacer = " ";
+ }
+
+ uiout->field_stream ("opcodes", m_opcode_stb);
+ uiout->text ("\t");
+ }
+ else
size = m_di.print_insn (pc);
- end_pc = pc + size;
- for (;pc < end_pc; ++pc)
- {
- read_code (pc, &data, 1);
- m_opcode_stb.printf ("%s%02x", spacer, (unsigned) data);
- spacer = " ";
- }
-
- uiout->field_stream ("opcodes", m_opcode_stb);
- uiout->text ("\t");
- }
- else
- size = m_di.print_insn (pc);
-
- uiout->field_stream ("inst", m_insn_stb);
- do_cleanups (ui_out_chain);
+ uiout->field_stream ("inst", m_insn_stb);
+ }
uiout->text ("\n");
return size;