This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Fix FPU Size Based on Dynamic FR
ClosedPublic

Authored by nitesh.jain on May 18 2016, 2:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Fix FPU Size Based on Dynamic FR/FRE bit.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, jingham.
nitesh.jain set the repository for this revision to rL LLVM.

Hi Greg/jingham,

Could you please find some time to review this ?

clayborg requested changes to this revision.May 24 2016, 10:10 AM
clayborg edited edge metadata.

This patch adds architecture specific code to generic GDB remote classes which should not happen. If we are going to fix correctly in the GDB remote classes, we need to abstract this. No mention of MIPS or any mips flags should be made inside of the GDB remote classes. The qRegisterInfo or the target XML data that is received from the GDB server describes all registers, you will need to add key/value pairs to the FP registers that describe these limitations then modify the DynamicRegisterInfo do the right thing. It shouldn't be too hard to add functionality that says that the byte size of a register depends on a value in another register.

This revision now requires changes to proceed.May 24 2016, 10:10 AM

One way we could abstract the size is to add a key named "bitsize-expr" that is a DWARF expression. The expression in your case would be a DWARF expression:

DW_OP_regx(32) DW_OP_lit1 DW_OP_lit26 DW_OP_shl

"DW_OP_regx(32)" would push the value of register 32 onto the stack. Then it would push 1 onto the stack (DW_OP_lit1) followed by 26 onto the stack (DW_OP_lit26) and then shift 1 << 26 with DW_OP_shl... Etc. The DWARF expression would result in the byte size being on the stack at the end as the DWARF expressions can branch and do other things. So it would be possible to come up with a byte stream that can be used by all registers that is agnostic to the architecture.

Then if a register has a "bitsize-expr", the size would need to be determined by running a DWARF expression.

nitesh.jain edited edge metadata.

Added field "dynamic_size" in struct RegisterInfo. If the value of this field is 1 then register size should be detected at runtime. In case of MIPS, the value of this field will be always 1 for all floating point registers. Then the size of register will be evaluated based on result of Dwarf Expression.

nitesh.jain edited edge metadata.Jun 23 2016, 2:58 AM
nitesh.jain added a subscriber: lldb-commits.
clayborg requested changes to this revision.Jun 23 2016, 11:02 AM
clayborg edited edge metadata.

So this is close. My idea was that the lldb-server would not just set a key/value pair named "dynamic_size" to "1" in the "qRegisterInfo" or "$qXfer:features:read:target.xml:0,1ffff" XML data, it would add a key value pair: whose name is "dynamic_size_dwarf_expr" whose value is the HEX ASCII bytes encded. So qRegisterInfo would have:

dynamic_size_dwarf_expr:112233AABB

where "112233AABB" are the bytes for the DWARF expression:

llvm::dwarf::DW_OP_regx, sr_reg_num, llvm::dwarf::DW_OP_lit1,
llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and,
llvm::dwarf::DW_OP_regx, config5_reg_num, llvm::dwarf::DW_OP_lit1,
llvm::dwarf::DW_OP_lit8, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and,
llvm::dwarf::DW_OP_lit18, llvm::dwarf::DW_OP_shl, 
llvm::dwarf::DW_OP_xor

The result of the expression should be the byte size for the register, not a zero or 1. This way, any target can hook up to LLDB and provide the needed info and LLDB will just work. With the above solution, you must go and modify LLDB source code.

See inlined comments for more details.

I really want to get this dynamic register size stuff right so the next target that needs it just works with no modifications to existing code. Thanks for making all the changes.

include/lldb/lldb-private-types.h
57–58 ↗(On Diff #61654)

We shouldn't have to add this as any register size adjustments can be taken care of in:

const RegisterInfo * RegisterContext::GetRegisterInfoAtIndex (uint32_t reg_index) const;

Another thing we could do is store the DWARF expression bytes right in the RegisterInfo:

const uint8_t *dynamic_size_dwarf_expr_bytes; // A DWARF expression that when evaluated gives the byte size of this register
size_t dynamic_size_dwarf_len; // The length of the DWARF expression in bytes in the dynamic_size_dwarf_expr_bytes member
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
495 ↗(On Diff #61654)

Just check if reg_index is in the FPUs. We shouldn't need to add the dynamic_size to RegisterInfo, or we just check the new "dynamic_size_dwarf_expr_bytes" field in RegisterInfo.

497–505 ↗(On Diff #61654)

If we don't add anything to RegisterInfo, then this code is fine. Else we will need to check "reg_info->dynamic_size_dwarf_expr_bytes" and evaluate the DWARF expression to get the size.

930–963 ↗(On Diff #61654)

This whole function goes away if we add "dynamic_size_dwarf_expr_bytes" to RegisterInfo. Else this stays.

source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
295–297 ↗(On Diff #61654)

I would change this to get a "dynamic_size_dwarf_expr" key whose value is a string that has HEXASCII encoded DWARF DW_OP bytes. We wouldn't store this in the "reg_info", but somewhere else locally in this class. Maybe a map of register number to DWARFExpression:

typedef std::map<uint32_t, DWARFExpression> DynamicRegisterSizeMap;

If we modify RegisterInfo to add the "dynamic_size_dwarf_expr_bytes", then we would store the bytes in there, but we will need to make a copy of the bytes.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1637–1640 ↗(On Diff #61654)

Remove? Or we can change RegisterInfo to contain a "const uint8_t *dynamic_size_dwarf_expr_bytes; size_t dynamic_size_dwarf_len;".

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
956–1000 ↗(On Diff #61654)

This should be removed in favor or having the DWARF expression supplied to us via the qRegisterInfo or $qXfer:features:read:target.xml:0,1ffff packets. The XML would have a key/value pair for "dynamic_size_dwarf_expr" that would be the DWARF expression.

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
49–55 ↗(On Diff #61654)

Remove this and just evaluate the DWARF expression that was given to us via the qRegisterInfo or $qXfer:features:read:target.xml:0,1ffff

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
557 ↗(On Diff #61654)

remove

642–645 ↗(On Diff #61654)

change to:

else if (name.compare("dynamic_size_dwarf_expr") == 0)
{
    // TODO: store DWARF expression somewhere
}
This revision now requires changes to proceed.Jun 23 2016, 11:02 AM
nitesh.jain added inline comments.Jul 1 2016, 5:24 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
497–505 ↗(On Diff #61654)

If Dwarf expression need to be evaluate at server side, then we need to add one more function in DWARFExpression::Evaluate to take NativeRegisterContext has a parameter. So should I add one more functionality in it ?

Thanks

labath added a subscriber: labath.Jul 1 2016, 6:05 AM
labath added inline comments.
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
497–505 ↗(On Diff #61654)

Please be careful about pulling more code to lldb-server, as it may cause the size to blow up due to transitive dependencies. If you're going to do that, please check the impact on the binary size, but I think it would be best to keep dwarf expression evaluation out of the server altogether.

There should be no need to add code on the lldb-server side. It would just send the DWARF expression bytes over when qRegisterInfo or the target xml is sent to the host. The host would store this expression and run it each time the register info is asked for for a register so that the host can set the register byte size correctly.

nitesh.jain updated this revision to Diff 63201.Jul 8 2016, 5:09 AM
nitesh.jain retitled this revision from [LLDB][MIPS] Fix FPU Size Based on Dynamic FR/FRE bit to [LLDB][MIPS] Fix FPU Size Based on Dynamic FR.
nitesh.jain updated this object.
nitesh.jain edited edge metadata.

Update diff as per suggestion. In case of FRE mode, FPU registers are all 64 bit.

nitesh.jain marked 6 inline comments as done.Jul 8 2016, 5:11 AM
nitesh.jain marked an inline comment as done.Jul 8 2016, 5:14 AM
labath added inline comments.Jul 8 2016, 5:37 AM
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
98 ↗(On Diff #63201)

Can we get rid of these const_casts? It looks like UpdateDynamicRegisterSize does not actually need this to be non-const.

nitesh.jain added inline comments.Jul 8 2016, 6:44 AM
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
98 ↗(On Diff #63201)

No, it require so that we can modify the reg_info->byte_size member.

labath added inline comments.Jul 8 2016, 6:51 AM
source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
98 ↗(On Diff #63201)

In that case, there is something else wrong with the design here. Well behaved code should never need to call const_cast.

m_reg_info is non-const, so maybe you just need a non-const version of DynamicRegisterInfo::GetRegisterInfoAtIndex or something like that...

nitesh.jain updated this revision to Diff 63224.Jul 8 2016, 7:50 AM
nitesh.jain edited edge metadata.
nitesh.jain removed rL LLVM as the repository for this revision.

Added lldb_private::RegisterInfo * DynamicRegisterInfo::GetRegisterInfoAtIndex (uint32_t i)

nitesh.jain marked an inline comment as done.Jul 8 2016, 7:53 AM

Hi Greg,

Please could you find some time to review this ?

Thanks

clayborg requested changes to this revision.Jul 12 2016, 10:27 AM
clayborg edited edge metadata.

Many issues. See inlined comments.

include/lldb/lldb-private-types.h
57–58 ↗(On Diff #63224)

If you choose to add these fields to RegisterInfo.h, then you will need to update all macros for that create arrays of RegisterInfo structs to fill in NULL into dynamic_size_dwarf_expr_bytes, and 0 into dynamic_size_dwarf_len.

source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
298–299 ↗(On Diff #63224)

We don't need a key named "dynamic_size_dwarf_len", we just need "dynamic_size_dwarf_expr_bytes". We can fill in "reg_info.dynamic_size_dwarf_len" after decoding the bytes in "dynamic_size_dwarf_expr_bytes".

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1639 ↗(On Diff #63224)

We really don't need a "dynamic_size_dwarf_len" key. Just "dynamic_size_dwarf_expr_bytes" and we can determine the byte size from how many bytes are encoded as hex ASCII chars.

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
96–97 ↗(On Diff #63224)

Put these two statements inside the "if (reg_info->dynamic_size_dwarf_len)" statement

99 ↗(On Diff #63224)

add space and make sure "reg_info" isn't NULL.

if (reg_info && reg_info->dynamic_size_dwarf_len)
954–994 ↗(On Diff #63224)

This should be a function in RegisterContext.h/RegisterContext.cpp and then the first two arguments are not needed. See previous comment for reasons why.

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
50–55 ↗(On Diff #63224)

This should be moved to RegisterContext.h as any register from any context can have DWARF expressions that describe the byte size. If this is a function on the RegisterContext class, you won't need to first two parameters "exe_ctx" and "reg_ctx" since the RegisterContext contains a member variable named "m_thread" which is a "lldb_private::Thread &".

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
542 ↗(On Diff #63224)

We are going to assume that 8 bytes is enough for any size expression?

658 ↗(On Diff #63224)

We can't use "dwarf_opcode" here. It is a local variable above:

uint8_t dwarf_opcode[8];

Then the register info will point to random data on the stack after this function. This need to be dynamically stored on the dynamic register info class so that it maintains a reference to the data so that it lives as long as the RegisterInfo structs do.

4395 ↗(On Diff #63224)

We can't use a local variable for the dwarf_opcode data.

4505–4508 ↗(On Diff #63224)

Don't need this key, remove this code.

4518 ↗(On Diff #63224)

You must dynamically store the dwarf opcode bytes in the dynamic register info class so it keeps the bytes alive as long as the register info structs.

This revision now requires changes to proceed.Jul 12 2016, 10:27 AM
nitesh.jain edited edge metadata.

Removed dynamic_size_dwarf_len field from RegisterInfo struct.

nitesh.jain marked 8 inline comments as done.Jul 20 2016, 8:18 AM
clayborg requested changes to this revision.Jul 25 2016, 11:11 AM
clayborg edited edge metadata.

A few comments:

  • we need the DWARF opcode length in RegisterInfo, but not in the structured data (XML or JSON)
  • all macros that initialize RegisterInfo structs must be expended to null and zero out the opcode pointer and size (in all register contexts for all archs)
  • remove all virtual and overrides for GetDwarfOpcodeLength()
include/lldb/Host/common/NativeRegisterContext.h
90–92 ↗(On Diff #64693)

Remove this, the length needs to stay in the RegisterInfo.

include/lldb/Host/common/NativeRegisterContextRegisterInfo.h
40–42 ↗(On Diff #64693)

Remove this, the length needs to stay in the RegisterInfo.

include/lldb/Target/RegisterContext.h
53 ↗(On Diff #64693)

Remove opcode_len, the length needs to stay in the RegisterInfo.

include/lldb/lldb-private-types.h
57–58 ↗(On Diff #64693)

So we either remove both fields from RegisterInfo, or we leave both in. If we remove them both, then getting the register info from the RegisterContext will update the register info automatically if needed as determined by the RegisterContext subclass. I don't mind them being in RegisterInfo and I think both should stay. In this case, we do need the length for the expression in the RegisterInfo struct, we just don't need to *send* the length in the structured data (XML or JSON).

source/Host/common/NativeRegisterContext.cpp
273–278 ↗(On Diff #64693)

Remove this, the length needs to stay in the RegisterInfo.

source/Host/common/NativeRegisterContextRegisterInfo.cpp
52–58 ↗(On Diff #64693)

Remove this, the length needs to stay in the RegisterInfo.

source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
317 ↗(On Diff #64693)

set the length in RegisterInfo from the size of the bytes extracted.

320 ↗(On Diff #64693)

set the opcode length to zero here.

424–428 ↗(On Diff #64693)

remove.

435 ↗(On Diff #64693)

remove dwarf_expr_bytes_len.

source/Plugins/Process/Utility/DynamicRegisterInfo.h
43 ↗(On Diff #64693)

Remove the dwarf_expr_bytes_len, the length needs to stay in the RegisterInfo. If the opcode and length are non-zero, then a copy of the data will need to be made so it can persist. We already do this with the value_regs and invalidate_regs fields in RegisterInfo.

57–59 ↗(On Diff #64693)

remove

source/Plugins/Process/Utility/RegisterContextLinux_mips.cpp
72–84 ↗(On Diff #64693)

remove

source/Plugins/Process/Utility/RegisterContextLinux_mips.h
34–35 ↗(On Diff #64693)

remove

source/Plugins/Process/Utility/RegisterContextLinux_mips64.cpp
124–136 ↗(On Diff #64693)

remove

source/Plugins/Process/Utility/RegisterContextLinux_mips64.h
36–38 ↗(On Diff #64693)

remove

source/Plugins/Process/Utility/RegisterInfoInterface.h
51–56 ↗(On Diff #64693)

remove

This revision now requires changes to proceed.Jul 25 2016, 11:11 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion.

nitesh.jain marked 9 inline comments as done.Jul 28 2016, 6:44 AM
clayborg accepted this revision.Jul 28 2016, 10:26 AM
clayborg edited edge metadata.

Looks good, thanks for all of the updates.

This revision is now accepted and ready to land.Jul 28 2016, 10:26 AM
This revision was automatically updated to reflect the committed changes.

Thanks Greg for all your help.