This is an archive of the discontinued LLVM Phabricator instance.

Refactor string conversion for InstructionControlFlowKind enum
ClosedPublic

Authored by jj10306 on Jul 26 2022, 8:36 AM.

Details

Summary

Refactor the string conversion of the lldb::InstructionControlFlowKind enum out
of Instruction::Dump to enable reuse of this logic by the
JSON TraceDumper (to be implemented in separate diff).

Will coordinate the landing of this change with D130320 since there will be a minor merge conflict between
these changes.

Test Plan:
Run unittests

> ninja check-lldb-unittests
[4/5] Running lldb unit test suite

Testing Time: 10.13s
  Passed: 1084

Verify '-k' flag's output

(lldb) thread trace dump instructions -k
thread #1: tid = 1375377
  libstdc++.so.6`std::ostream::flush() + 43
    7048: 0x00007ffff7b54dab    return      retq
    7047: 0x00007ffff7b54daa    other       popq   %rbx
    7046: 0x00007ffff7b54da7    other       movq   %rbx, %rax
    7045: 0x00007ffff7b54da5    cond jump   je     0x11adb0                  ; <+48>
    7044: 0x00007ffff7b54da2    other       cmpl   $-0x1, %eax
  libc.so.6`_IO_fflush + 249
    7043: 0x00007ffff7161729    return      retq
    7042: 0x00007ffff7161728    other       popq   %rbp
    7041: 0x00007ffff7161727    other       popq   %rbx
    7040: 0x00007ffff7161725    other       movl   %edx, %eax
    7039: 0x00007ffff7161721    other       addq   $0x8, %rsp
    7038: 0x00007ffff7161709    cond jump   je     0x87721                   ; <+241>
    7037: 0x00007ffff7161707    other       decl   (%rsi)
    7036: 0x00007ffff71616fe    cond jump   je     0x87707                   ; <+215>
    7035: 0x00007ffff71616f7    other       cmpl   $0x0, 0x33de92(%rip)      ; __libc_multiple_threads
    7034: 0x00007ffff71616ef    other       movq   $0x0, 0x8(%rsi)
    7033: 0x00007ffff71616ed    cond jump   jne    0x87721                   ; <+241>
    7032: 0x00007ffff71616e9    other       subl   $0x1, 0x4(%rsi)
    7031: 0x00007ffff71616e2    other       movq   0x88(%rbx), %rsi
    7030: 0x00007ffff71616e0    cond jump   jne    0x87721                   ; <+241>
    7029: 0x00007ffff71616da    other       testl  $0x8000, (%rbx)           ; imm = 0x8000

Diff Detail

Event Timeline

jj10306 created this revision.Jul 26 2022, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 8:36 AM
jj10306 requested review of this revision.Jul 26 2022, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 8:36 AM
jj10306 updated this revision to Diff 447727.Jul 26 2022, 8:50 AM

remove accidentily included change to trace dumper

lldb/source/Target/TraceDumper.cpp
221

ignore this accidentally included change, will delete this and update the diff

jj10306 retitled this revision from Add string conversion for InstructionControlFlowKind enum to Refactor string conversion for InstructionControlFlowKind enum.Jul 26 2022, 8:53 AM
jj10306 added inline comments.Jul 26 2022, 8:57 AM
lldb/source/Core/Disassembler.cpp
930

Thoughts on adding a default case with a string that indicates that no string conversion was implemented for the provided variant?
This would potentially make it easier for someone to realize they didn't add a case here for a newly added variant of InstructionControlFlowKind. Otherwise undefined data would be returned if a new case isn't added for a new variant.
wdyt?

persona0220 accepted this revision.Jul 26 2022, 11:48 AM
persona0220 added inline comments.
lldb/source/Core/Disassembler.cpp
930

I agree to add a default here!

This revision is now accepted and ready to land.Jul 26 2022, 11:48 AM
wallace accepted this revision.Jul 26 2022, 11:51 AM

just remove that comment. I've just pushed Sujin's diff as well, so you can rebase

lldb/include/lldb/lldb-enumerations.h
975–976

you can safely remove this comment because the compiler will complain if a new enum value is added and the switch case in GetNameForInstructionControlFlowKind is not updated

lldb/source/Core/Disassembler.cpp
930

no defaults for enums, otherwise the compiler won't complain if a new enum value is added and a case is not added here

jj10306 updated this revision to Diff 447802.Jul 26 2022, 12:40 PM

Rebase, fix merge conflicts with D130320

jj10306 updated this revision to Diff 447804.Jul 26 2022, 12:44 PM
jj10306 marked 3 inline comments as done.

Remove unnecessary comment

This revision was automatically updated to reflect the committed changes.