Page MenuHomePhabricator

expose 64 bit addresses through MI
ClosedPublic

Authored by ki.stfu on Mar 10 2015, 4:47 PM.

Details

Summary

This changes all reporting of addresses from lldb-mi to be 64 bit capable. There could have been cases where a 64 bit address was getting truncated to 32 bit format.

Patch from chuckr@microsoft.com

Diff Detail

Event Timeline

ChuckR updated this revision to Diff 21660.Mar 10 2015, 4:47 PM
ChuckR retitled this revision from to expose 64 bit breakpoint addresses through MI.
ChuckR updated this object.
ChuckR edited the test plan for this revision. (Show Details)
ChuckR added a reviewer: abidh.
ChuckR added a subscriber: Unknown Object (MLST).

Hi Chuck, can you resubmit this patch attached to the other review? The
workflow isn't entirely obvious, but there's a couple of things you need to
do different.

  1. When you create the diff with the suggested changes, create the diff

against origin (or whatever you created the *original* patch against), not
against the first version of the patch. So if HEAD is the first version of
the patch you uploaded, create your diff against HEAD~1. You want the new
patch to contain the changes _plus_ the original patch.

  1. When you attach the patch to Phabricator, you'll get a combo box and the

default value is "Create new revision". Change this to "Attach to existing
revision" and then choose the entry for the first patch you uploaded.

Ahh, sorry. It seems I was the one that was confused then. Sorry for the
noise.

abidh accepted this revision.Mar 11 2015, 3:28 AM
abidh edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 11 2015, 3:28 AM
In D8238#138685, @abidh wrote:

Looks good.

Should we print address as "%016llx" everywhere? Previously I supposed that we should use %08llx for 32bit & %016llx for 64bit app.

As you may understood, I'm not sure about this patch.
In addition, if we should use %016 everywhere, we should fix it everywhere and it'll be nice to do in one patch.

abidh added a comment.Mar 11 2015, 4:41 AM

As you may understood, I'm not sure about this patch.
In addition, if we should use %016 everywhere, we should fix it everywhere and it'll be nice to do in one patch.

I also thought about that when testing the patch. The address in some frame was printed as 32-bit. It will be a good cleanup going forward.

These are the files that have the string '%08' in them, should I fix all of them up as part of this patch?

.//MICmdCmdBreak.cpp:    // "^done,bkpt={number=\"%d\",type=\"breakpoint\",disp=\"%s\",enabled=\"%c\",addr=\"0x%08x\",func=\"%s\",file=\"%s\",fullname=\"%s/%s\",line=\"%d\",thread-groups=[\"%s\"],times=\"%d\",original-location=\"%s\"}"
.//MICmdCmdData.cpp:        // MI "{address=\"0x%08llx\",func-name=\"%s\",offset=\"%lld\",inst=\"%s %s\"}"
.//MICmdCmdData.cpp:        const CMICmnMIValueConst miValueConst(CMIUtilString::Format("0x%08llx", addr));
.//MICmdCmdData.cpp:    // MI: memory=[{begin=\"0x%08x\",offset=\"0x%08x\",end=\"0x%08x\",contents=\" \" }]"
.//MICmdCmdData.cpp:    const CMICmnMIValueConst miValueConst(CMIUtilString::Format("0x%08llx", m_nAddrStart));
.//MICmdCmdData.cpp:    const CMICmnMIValueConst miValueConst2(CMIUtilString::Format("0x%08llx", m_nAddrOffset));
.//MICmdCmdData.cpp:    const CMICmnMIValueConst miValueConst3(CMIUtilString::Format("0x%08llx", m_nAddrStart + m_nAddrNumBytesToRead));
.//MICmdCmdGdbInfo.cpp:                  rStdout.TextToStdout(CMIUtilString::Format("~\"0x%08x\t0x%08x\t%s\t\t%s\"", addrLoadS, addrLoadS + addrLoadSize,
.//MICmnLLDBDebuggerHandleEvents.cpp:    // MI print "=breakpoint-modified,bkpt={number=\"%d\",type=\"breakpoint\",disp=\"%s\",enabled=\"%c\",addr=\"0x%08x\",
.//MICmnLLDBDebuggerHandleEvents.cpp:        // "=breakpoint-modified,bkpt={number=\"%d\",type=\"breakpoint\",disp=\"%s\",enabled=\"%c\",addr=\"0x%08x\",func=\"%s\",file=\"%s\",fullname=\"%s/%s\",line=\"%d\",times=\"%d\",original-location=\"%s\"}"
.//MICmnLLDBDebuggerHandleEvents.cpp:        // "=breakpoint-created,bkpt={number=\"%d\",type=\"breakpoint\",disp=\"%s\",enabled=\"%c\",addr=\"0x%08x\",func=\"%s\",file=\"%s\",fullname=\"%s/%s\",line=\"%d\",times=\"%d\",original-location=\"%s\"}"
.//MICmnLLDBDebuggerHandleEvents.cpp:    // "*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"%d\",frame={addr=\"0x%08x\",func=\"%s\",args=[],file=\"%s\",fullname=\"%s\",line=\"%d\"},thread-id=\"%d\",stopped-threads=\"all\""
.//MICmnLLDBDebuggerHandleEvents.cpp:    // frame={addr=\"0x%08x\",func=\"%s\",args=[],file=\"%s\",fullname=\"%s\",line=\"%d\"}
.//MICmnLLDBDebuggerHandleEvents.cpp:    // "*stopped,reason=\"end-stepping-range\",frame={addr=\"0x%08x\",func=\"%s\",args=[\"%s\"],file=\"%s\",fullname=\"%s\",line=\"%d\"},thread-id=\"%d\",stopped-threads=\"all\""
.//MICmnLLDBDebugSessionInfo.cpp:    // "frame={level=\"%d\",addr=\"0x%08llx\",func=\"%s\",args=[%s],file=\"%s\",fullname=\"%s\",line=\"%d\"},frame={level=\"%d\",addr=\"0x%08llx\",func=\"%s\",args=[%s],file=\"%s\",fullname=\"%s\",line=\"%d\"},
.//MICmnLLDBDebugSessionInfo.cpp:    // "frame={level=\"%d\",addr=\"0x%08llx\",func=\"%s\",args=[%s],file=\"%s\",fullname=\"%s\",line=\"%d\"},frame={level=\"%d\",addr=\"0x%08llx\",func=\"%s\",args=[%s],file=\"%s\",fullname=\"%s\",line=\"%d\"},
.//MICmnLLDBDebugSessionInfo.cpp:    // MI print "{level=\"0\",addr=\"0x%08llx\",func=\"%s\",file=\"%s\",fullname=\"%s\",line=\"%d\"}"
.//MICmnLLDBDebugSessionInfo.cpp:    const CMIUtilString strAddr(CMIUtilString::Format("0x%08llx", vPc));
.//MICmnLLDBDebugSessionInfo.cpp:    const CMIUtilString strAddr(CMIUtilString::Format("0x%08llx", vPc));
.//MICmnLLDBDebugSessionInfo.cpp:    // MI print "=breakpoint-modified,bkpt={number=\"%d\",type=\"breakpoint\",disp=\"%s\",enabled=\"%c\",addr=\"0x%08x\",
.//MICmnResources.cpp:    {IDS_CMD_ERR_LLDB_ERR_NOT_READ_WHOLE_BLK, "Command '%s'. LLDB unable to read entire memory block of %u bytes at address 0x%08x"},
.//MICmnResources.cpp:    {IDS_CMD_ERR_LLDB_ERR_READ_MEM_BYTES, "Command '%s'. Unable to read memory block of %u bytes at address 0x%08x: %s "},
.//MICmnResources.cpp:    {IDS_CMD_ERR_LLDB_ERR_READ_MEM_BYTES, "Command '%s'. Unable to write memory block of %u bytes at address 0x%08x: %s "},
.//MICmnResources.cpp:    {IDS_CMD_ERR_LLDB_ERR_NOT_WRITE_WHOLEBLK, "Command '%s'. LLDB unable to write entire memory block of %u bytes at address 0x%08x"},
ChuckR updated this revision to Diff 21886.Mar 12 2015, 4:25 PM
ChuckR retitled this revision from expose 64 bit breakpoint addresses through MI to expose 64 bit addresses through MI.
ChuckR updated this object.
ChuckR edited edge metadata.

Fixed all the places I could find.

ki.stfu requested changes to this revision.Mar 13 2015, 8:21 AM
ki.stfu added a reviewer: ki.stfu.

In some places address is printed as "int" instead of "long long". I suppose that we should use "%llx" instead of "%x".

But perhaps would be better to use PRIx64 (or PRIxPTR) macro.

tools/lldb-mi/MICmdCmdBreak.cpp
337

address should be printed as a long long value:

addr=\"0x%016llx\"
tools/lldb-mi/MICmdCmdData.cpp
642

address should be printed as a long long value:

begin=\"0x%016llx\",offset=...,end=...
tools/lldb-mi/MICmdCmdGdbInfo.cpp
212

I am worried about how it will work in case of 32 bit arch.

232

address should be printed as a long long value:

~\"0x%016llx\t0x%016llx\t..."
tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
1091

address should be printed as a long long value:

addr=\"0x%016llx\"
tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
348

address should be printed as a long long value:

addr=\"0x%016llx\"
444

address should be printed as a long long value:

addr=\"0x%016llx\"
466

address should be printed as a long long value:

addr=\"0x%016llx\"
1278

address should be printed as a long long value:

addr=\"0x%016llx\"
1290

address should be printed as a long long value:

addr=\"0x%016llx\"
1355

address should be printed as a long long value:

addr=\"0x%016llx\"
tools/lldb-mi/MICmnResources.cpp
255–256

address should be printed as a long long value

261–262

address should be printed as a long long value

This revision now requires changes to proceed.Mar 13 2015, 8:21 AM

You must use PRIx64 for 64 bit types. %ll isn't right on windows

Or PRIxPTR as you said. Definitely not %ll though

Or PRIxPTR as you said. Definitely not %ll though

It depends on how an address type was specified. If it's uint64_t then use PRIx64, but if it was specified like "void*" then use PRIxPTR.

ChuckR updated this revision to Diff 22230.Mar 18 2015, 5:02 PM
ChuckR edited edge metadata.

I am now using the PRIx64 macro in all locations. I checked and made sure that every type being used is either an MIUint64 or and lldb::addr_t, both of with are uint64s. I also noticed that IDS_CMD_ERR_LLDB_ERR_READ_MEM_BYTES was being initialized twice, and switched the second initialization to the correct IDS_CMD_ERR_LLDB_ERR_NOT_WRITE_WHOLEBLK

ki.stfu accepted this revision.Mar 19 2015, 3:28 AM
ki.stfu edited edge metadata.

lgtm apart few comments

tools/lldb-mi/MICmdCmdData.cpp
21

#include <inttypes.h> // For PRIx64

tools/lldb-mi/MICmdCmdGdbInfo.cpp
13

#include <inttypes.h> // For PRIx64

tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
11

#include <inttypes.h> // For PRIx64

tools/lldb-mi/MICmnResources.cpp
11

#include <inttypes.h> // For PRIx64

This revision is now accepted and ready to land.Mar 19 2015, 3:28 AM
ChuckR updated this revision to Diff 22271.Mar 19 2015, 10:15 AM
ChuckR edited edge metadata.

I fixed the comments based on feedback from Ilia K. Can somebody please check this in on my behalf?

I fixed the comments based on feedback from Ilia K. Can somebody please check this in on my behalf?

Yes, I can.

ki.stfu commandeered this revision.Mar 19 2015, 10:27 AM
ki.stfu edited reviewers, added: ChuckR; removed: ki.stfu.
ki.stfu updated this object.
ki.stfu updated this revision to Diff 22275.Mar 19 2015, 10:28 AM

Fix MiDataTestCase.test_lldbmi_data_disassemble

ki.stfu closed this revision.Mar 19 2015, 10:29 AM