Page MenuHomePhabricator

Add size field to library load event
ClosedPublic

Authored by paulmaybee on Jul 28 2015, 1:31 PM.

Details

Summary

(This revision supersedes the abandon: http://reviews.llvm.org/D9716)
Size field is used to let the debugger attribute an address to a specific library when symbols are not available.
For example:
OpenGLESApp4.app!Cube_draw() Line 74 C

	OpenGLESApp4.app!-[GameViewController glkView:drawInRect:](GameViewController * self, SEL _cmd, GLKView * view, CGRect rect) Line 89	C++
	GLKit!<redacted>	
	QuartzCore!<redacted>	
	QuartzCore!<redacted>	
	QuartzCore!<redacted>	
	QuartzCore!<redacted>	
	QuartzCore!<redacted>	
	UIKit!<redacted>	
	UIKit!<redacted>	
	UIKit!<redacted>	
	UIKit!<redacted>	
	FrontBoardServices!<redacted>	
	CoreFoundation!<redacted>

Diff Detail

Repository
rL LLVM

Event Timeline

paulmaybee updated this revision to Diff 30846.Jul 28 2015, 1:31 PM
paulmaybee retitled this revision from to Add size field to library load event.
paulmaybee updated this object.
paulmaybee added reviewers: abidh, ki.stfu, ChuckR.
paulmaybee added subscribers: lldb-commits, greggm.
ki.stfu requested changes to this revision.Jul 28 2015, 11:56 PM
ki.stfu edited edge metadata.

Why have you opened a new CL? Anyway, close/abandon the D9716 and see my inline comments.

test/tools/lldb-mi/TestMiLibraryLoaded.py
30 ↗(On Diff #30846)

Add the size field here please.

31 ↗(On Diff #30846)

As I said in D9716, it will cause a false negative if path contains symbols that should be escaped (" or \ etc). I faced with the same issue in MiSyntaxTestCase. You should use analogue of CMIUtilString::AddSlashes here, or pass exactly = True option.

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
731 ↗(On Diff #30846)

ignore a return code here. It should cause a compiler error since r242911. Have you tried to compile it?

735 ↗(On Diff #30846)

Does it work for you? O_O

Should be:

const CMIUtilString strSize(CMIUtilString::Format("%" PRIu64, sbSection.GetByteSize()));
738 ↗(On Diff #30846)

ignore a return code

This revision now requires changes to proceed.Jul 28 2015, 11:56 PM

BTW, update MIExtensions.txt file please.

paulmaybee edited edge metadata.
paulmaybee marked 4 inline comments as done.

Escape paths in library-loaded event and fix test case.

ki.stfu requested changes to this revision.Jul 29 2015, 10:31 PM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
test/tools/lldb-mi/TestMiLibraryLoaded.py
31 ↗(On Diff #30924)

It's still may cause a false negative, so let's do the following (more details in D9716):

def add_slashes(x): return x.replace("\\", "\\\\").replace("\"", "\\\"").replace("\'", "\\\'").replace("\0", "\\\0")
self.expect([
        "=library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",symbols-loaded=\"1\",symbols-path=\"%s\",loaded_addr=\"-\",size=\"[0-9]+\"" % (add_slashes(path), add_slashes(path), add_slashes(path), add_slashes(symbols_path)),
        "=library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",symbols-loaded=\"0\",loaded_addr=\"-\",size=\"[0-9]+\"" % (add_slashes(path), add_slashes(path), add_slashes(path))
    ])
tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
691 ↗(On Diff #30924)

Just use the AddSlashes here.

695 ↗(On Diff #30924)

Just use the AddSlashes here.

701 ↗(On Diff #30924)

Just use the AddSlashes here.

718 ↗(On Diff #30924)

Just use the AddSlashes here.

This revision now requires changes to proceed.Jul 29 2015, 10:31 PM
paulmaybee updated this revision to Diff 31038.Jul 30 2015, 9:29 AM
paulmaybee edited edge metadata.
paulmaybee marked 4 inline comments as done.

Add add_slashes to test case. Replace Escape with AddSlashes. Update extension doc.

ping

test/tools/lldb-mi/TestMiLibraryLoaded.py
31 ↗(On Diff #30846)

exactly = True is there?

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
731 ↗(On Diff #30846)

Oops. Sorry, wrong diff. Fixed now.

abidh edited edge metadata.Aug 7 2015, 7:35 AM

I asked this question in the other revision too. Can you give a reason why this field is needed. The following description you gave in this revision does not make much sense to me. I am not opposed to adding this field but would like to know what problem it solves before it is committed.

"Size field is used to let the debugger attribute an address to a specific library when symbols are not available.
For example:
OpenGLESApp4.app!Cube_draw() Line 74 C"

I'm not sure what the misunderstanding is, so I'll rephrase our desire. The VS debugger decorates code addresses with the name of the shared library from which the code originated (example above). We wish to use the load address and size of the shared library text for the purpose of determining the library name. Is there is some other way for us to get the lib name?

abidh added a comment.Aug 8 2015, 1:42 AM

I'm not sure what the misunderstanding is, so I'll rephrase our desire. The VS debugger decorates code addresses with the name of the shared library from which the code originated (example above). We wish to use the load address and size of the shared library text for the purpose of determining the library name. Is there is some other way for us to get the lib name?

So what I understand is that you want to get the address range of the library so that you can decorate functions which lies within that range. Seems a valid use case to me. In the core lldb, you could use something like "target modules lookup" command for this purpose.

ki.stfu accepted this revision.Aug 10 2015, 1:33 AM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Aug 10 2015, 1:33 AM

Can you please check in in for me. Thanks.

Can you please check in in for me. Thanks.

Ok, tomorrow.

This revision was automatically updated to reflect the committed changes.