In case of MIPS, the floating point register size is based on FR bit of status register(SR) (https://reviews.llvm.org/rL277343). In this patch, we update reg_info["bitsize"] based on SR.FR bit.
Details
Details
- Reviewers
zturner clayborg labath - Commits
- rGf440244d673f: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
rGf468b5d32f98: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
rLLDB291554: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
rLLDB291549: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
rL291554: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
rL291549: [LLDB][MIPS] Fix TestLldbGdbServer failure for MIPS
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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. | |
Comment Actions
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"). | |
Comment Actions
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.
Comment Actions
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
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.
Comment Actions
Instead of creating header SBDwarf.h, is it possible that python can reuse llvm Dwarf.h ?
Thanks
Comment Actions
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.
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").