This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
ClosedPublic

Authored by nitesh.jain on Nov 24 2016, 1:04 AM.

Diff Detail

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, labath, zturner.
labath requested changes to this revision.Nov 24 2016, 2:48 AM
labath edited edge metadata.

Looks fine in principle, but please encapsulate this better.

packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
589

Let's encapsulate the if(mips) dance in a separate function (getExpectedBitsize(reg_info, reg_infos), for example) and not bloat the general function with it.

This revision now requires changes to proceed.Nov 24 2016, 2:48 AM
nitesh.jain updated this revision to Diff 79881.Dec 1 2016, 2:27 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion.

nitesh.jain marked an inline comment as done.Dec 1 2016, 2:28 AM
labath accepted this revision.Dec 1 2016, 2:48 AM
labath edited edge metadata.

lgtm, after you consider my comment below.

packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
30

The reason I suggested a different signature for the function (getExpectedBitsize(reg_info, reg_infos)) was that then you would not need to maintain any global state. The function could compute the presence of the flag as necessary (maybe even via some helper function). Maybe we'd need to do some of the work multiple times, but it's not like this is performance critical code anyway.

If you don't want to do that, then at least indicate that this flag is mips-specific (_mips_fr_flag, leading underscore is python convention of "private").

This revision is now accepted and ready to land.Dec 1 2016, 2:48 AM
nitesh.jain updated this revision to Diff 79890.Dec 1 2016, 3:42 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion.

Thanks

nitesh.jain marked an inline comment as done.Dec 1 2016, 3:42 AM
clayborg edited edge metadata.Dec 1 2016, 10:41 AM

I am not sure I like the implications of all this MIPS specific knowledge in this test. I would like it if we can abstract this into the GDB remote protocol. Since we get the register descriptions from the lldb-server, it would be nice if the register description for the float registers that change size get an extra key/value pair in them. Maybe something like: "variable-size-key:mips-fpr". Then each time we get a stop reply packet from the remote server, any GDB server binaries that specify the "variable-size-key" in their description would update their sizes when we stop. So the stop reply packet would contain an extra key value pair: "mips-fpr:4;" or "mips-fpr:8;". The ProcessGDBRemote would need to save any variable-size-key values and know how to update the register sizes for any such registers.

So my general take is there are way too many places that are getting special knowledge of the MIPS SR register, so lets try to abstract this out in an architecture agnostic way.

Hi Greg,

I am not sure whether this approach will work in Multithreaded application. Since if any thread modify the SR.FR bit then the change will get reflect in all copy of SR.FR of all other threads. So if we stop at thread A and thread B (running) try to modify SR.FR bit then the above approach will give incorrect register size.

Let me check this case and get back to you

Thanks

We should be able to handle this being thread specific. Each stop reply packet and qThreadStopInfo (asks for a complete stop reply packet for each thread), or the the "jThreadsInfo" packet (see "$trunk/docs/lldb-gdb-remote.txt" for detail) has the ability to return key/value pairs that are specific to each thread. So this is where this key would be specified, once for each thread. Let me know if anything isn't clear.

Overall we really shouldn't ever see anything MIPS specific in generic code. It is ok to have MIPS specific code in say "RegisterContextMIPSXXX" classes, but not in general classes. We really want to abstract out anything arch specific into arch agnostic code.

The other way to do this that might be easier is to require this register is always read via a "p" packet and watch for the size that comes back and adjust the register size for that register accordingly.

Hi Greg,

The patch https://reviews.llvm.org/D20357 evaluated the DwarfExpression and update the floating point register size. So should we implement SBRegisterContext and SBArchSpec to evaluate dwarf expression and update the floating point register size accordingly for python test cases ?

OR

Should we implement above approach ?

Thanks

If we can parse the register info that was retrieved via the GDB remote packets and emulate the DWARF expression that would allow us to do things correctly in the test case. Anything that is agnostic will work and we probably have all the info we need. We will need to write a quick DWARF opcode parser, but we only need to handle the opcodes in the MIPS for now so it should be easy to make work in python.

nitesh.jain edited edge metadata.

Update diff as per suggestion.

Instead of creating header SBDwarf.h, is it possible that python can reuse llvm Dwarf.h ?

Thanks

clayborg requested changes to this revision.Jan 4 2017, 9:13 AM
clayborg edited edge metadata.

I would skip the SBDwarf.h and just make a simple DWARF opcode parser all in python. We only need it to handle the opcodes that are currently used by any supported targets. Since this is for testing we shouldn't add something to the public API unless it is going to be used by clients of the LLDB library. So I would port your SBDwarf.h to python:

DW_OP_addr                          = 0x03
DW_OP_deref                         = 0x06
DW_OP_const1u                       = 0x08
DW_OP_const1s                       = 0x09
DW_OP_const2u                       = 0x0A
DW_OP_const2s                       = 0x0B
DW_OP_const4u                       = 0x0C
DW_OP_const4s                       = 0x0D
DW_OP_const8u                       = 0x0E
DW_OP_const8s                       = 0x0F
DW_OP_constu                        = 0x10
DW_OP_consts                        = 0x11
DW_OP_dup                           = 0x12
DW_OP_drop                          = 0x13
DW_OP_over                          = 0x14
DW_OP_pick                          = 0x15
DW_OP_swap                          = 0x16
DW_OP_rot                           = 0x17
DW_OP_xderef                        = 0x18
DW_OP_abs                           = 0x19
DW_OP_and                           = 0x1A
DW_OP_div                           = 0x1B
DW_OP_minus                         = 0x1C
DW_OP_mod                           = 0x1D
DW_OP_mul                           = 0x1E
DW_OP_neg                           = 0x1F
DW_OP_not                           = 0x20
DW_OP_or                            = 0x21
DW_OP_plus                          = 0x22
DW_OP_plus_uconst                   = 0x23
DW_OP_shl                           = 0x24
DW_OP_shr                           = 0x25
DW_OP_shra                          = 0x26
DW_OP_xor                           = 0x27
DW_OP_skip                          = 0x2F
DW_OP_bra                           = 0x28
DW_OP_eq                            = 0x29
DW_OP_ge                            = 0x2A
DW_OP_gt                            = 0x2B
DW_OP_le                            = 0x2C
DW_OP_lt                            = 0x2D
DW_OP_ne                            = 0x2E
DW_OP_lit0                          = 0x30
DW_OP_lit1                          = 0x31
DW_OP_lit2                          = 0x32
DW_OP_lit3                          = 0x33
DW_OP_lit4                          = 0x34
DW_OP_lit5                          = 0x35
DW_OP_lit6                          = 0x36
DW_OP_lit7                          = 0x37
DW_OP_lit8                          = 0x38
DW_OP_lit9                          = 0x39
DW_OP_lit10                         = 0x3A
DW_OP_lit11                         = 0x3B
DW_OP_lit12                         = 0x3C
DW_OP_lit13                         = 0x3D
DW_OP_lit14                         = 0x3E
DW_OP_lit15                         = 0x3F
DW_OP_lit16                         = 0x40
DW_OP_lit17                         = 0x41
DW_OP_lit18                         = 0x42
DW_OP_lit19                         = 0x43
DW_OP_lit20                         = 0x44
DW_OP_lit21                         = 0x45
DW_OP_lit22                         = 0x46
DW_OP_lit23                         = 0x47
DW_OP_lit24                         = 0x48
DW_OP_lit25                         = 0x49
DW_OP_lit26                         = 0x4A
DW_OP_lit27                         = 0x4B
DW_OP_lit28                         = 0x4C
DW_OP_lit29                         = 0x4D
DW_OP_lit30                         = 0x4E
DW_OP_lit31                         = 0x4F
DW_OP_reg0                          = 0x50
DW_OP_reg1                          = 0x51
DW_OP_reg2                          = 0x52
DW_OP_reg3                          = 0x53
DW_OP_reg4                          = 0x54
DW_OP_reg5                          = 0x55
DW_OP_reg6                          = 0x56
DW_OP_reg7                          = 0x57
DW_OP_reg8                          = 0x58
DW_OP_reg9                          = 0x59
DW_OP_reg10                         = 0x5A
DW_OP_reg11                         = 0x5B
DW_OP_reg12                         = 0x5C
DW_OP_reg13                         = 0x5D
DW_OP_reg14                         = 0x5E
DW_OP_reg15                         = 0x5F
DW_OP_reg16                         = 0x60
DW_OP_reg17                         = 0x61
DW_OP_reg18                         = 0x62
DW_OP_reg19                         = 0x63
DW_OP_reg20                         = 0x64
DW_OP_reg21                         = 0x65
DW_OP_reg22                         = 0x66
DW_OP_reg23                         = 0x67
DW_OP_reg24                         = 0x68
DW_OP_reg25                         = 0x69
DW_OP_reg26                         = 0x6A
DW_OP_reg27                         = 0x6B
DW_OP_reg28                         = 0x6C
DW_OP_reg29                         = 0x6D
DW_OP_reg30                         = 0x6E
DW_OP_reg31                         = 0x6F
DW_OP_breg0                         = 0x70
DW_OP_breg1                         = 0x71
DW_OP_breg2                         = 0x72
DW_OP_breg3                         = 0x73
DW_OP_breg4                         = 0x74
DW_OP_breg5                         = 0x75
DW_OP_breg6                         = 0x76
DW_OP_breg7                         = 0x77
DW_OP_breg8                         = 0x78
DW_OP_breg9                         = 0x79
DW_OP_breg10                        = 0x7A
DW_OP_breg11                        = 0x7B
DW_OP_breg12                        = 0x7C
DW_OP_breg13                        = 0x7D
DW_OP_breg14                        = 0x7E
DW_OP_breg15                        = 0x7F
DW_OP_breg16                        = 0x80
DW_OP_breg17                        = 0x81
DW_OP_breg18                        = 0x82
DW_OP_breg19                        = 0x83
DW_OP_breg20                        = 0x84
DW_OP_breg21                        = 0x85
DW_OP_breg22                        = 0x86
DW_OP_breg23                        = 0x87
DW_OP_breg24                        = 0x88
DW_OP_breg25                        = 0x89
DW_OP_breg26                        = 0x8A
DW_OP_breg27                        = 0x8B
DW_OP_breg28                        = 0x8C
DW_OP_breg29                        = 0x8D
DW_OP_breg30                        = 0x8E
DW_OP_breg31                        = 0x8F
DW_OP_regx                          = 0x90
DW_OP_fbreg                         = 0x91
DW_OP_bregx                         = 0x92
DW_OP_piece                         = 0x93
DW_OP_deref_size                    = 0x94
DW_OP_xderef_size                   = 0x95
DW_OP_nop                           = 0x96
DW_OP_push_object_address           = 0x97
DW_OP_call2                         = 0x98
DW_OP_call4                         = 0x99
DW_OP_call_ref                      = 0x9A
DW_OP_form_tls_address              = 0x9B
DW_OP_call_frame_cfa                = 0x9C
DW_OP_bit_piece                     = 0x9D
DW_OP_implicit_value                = 0x9E
DW_OP_stack_value                   = 0x9F
DW_OP_lo_user                       = 0xE0
DW_OP_GNU_push_tls_address          = 0xE0
DW_OP_APPLE_uninit                  = 0xF0
DW_OP_hi_user                       = 0xFF

You are already writing a manual opcode parser so adding a few defines ins't a big deal. You might want to create a new lldbdwarf.py file that contains DWARF stuff to keep things clean. You can also make the DWARF expression evaluator live in that new file.

This revision now requires changes to proceed.Jan 4 2017, 9:13 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion

clayborg accepted this revision.Jan 9 2017, 10:17 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Jan 9 2017, 10:17 AM
This revision was automatically updated to reflect the committed changes.