In D33035#833876, @clayborg wrote:

I was out on vacation, sorry for the delay. Looks good.

Hi Greg .. Can you please review the changes? Please let me know if something needs to be changed. Thanks :)

Changes to make a C++ API work in python modules

• Added a new python typemap to handle it

In D33035#816337, @clayborg wrote:

I am not sure how sensitive typemaps are to the names of the arguments? Maybe we can just have you use different names and then the two typemaps won't collide? You can also write some manual code to do the remapping without typemaps.

Regardless of whether you are going to change the API or not, you will need to write something to make it work in python.

const uint8_t * ptdecoder_private:: Instruction::GetRawBytes() const;

What does swig do right now when the above code is converted to python? It needs to make a python string from this and the GetRawBytesSize()...

In D33035#803801, @clayborg wrote:

Much better on the API with Swig. Just a few inlined questions around GetRawBytes.

Removed std::vector<> from public APIs

In D33035#799656, @clayborg wrote:

In D33035#799404, @abhishek.aggarwal wrote:

In D33035#799058, @clayborg wrote:

So std::vector shouldn't be used in a public API. You should make a class that wraps your objects. LLDB's public API has lldb::SBInstruction and lldb::SBInstructionList as examples. std::vector on other systems compiles differently for debug and release builds and things will crash.

Hi Greg .. Does it go for the tools built on top of LLDB (e.g. in my case where feature library is not a part of liblldb shared lib but built on top of it)? If yes, then I will proceed to remove std::vector from C++ public API of the tool and create a new class for it.

It really comes down to the issue of how stable you want this API to be. If this API is a "you are expected to rebuild any tools you make against this API every time we release a new version" then you can really do anything you want. If you want a stable API to build against, you could choose to follow the LLDB public API rules: no inheritance, fixed number of member variables that never change (which usually means a std::shared_ptr<T>, std::unique_ptr<T> or a T*), and no virtual functions. This effectively renders the C++ API into a C API, but it does force you to have your public API classes have an internal implementation (T) and then the class that you export for your API be the public facing API that everyone uses externally. The nice thing about this is that swig has a really easy time creating all of the script bindings for you if you do it this way. So it really depends on what your future use case of this API is.

If you need this via swig for internal kind of stuff, then use a typemap where you map std::vector<T> to a list() with T instances in it in python.

I want to provide a python interface for the tool's C++ public API as well so that the API can be used in python modules as well. Therefore, I think typemapping to list() will not solve the problem. Am I right?

I believe it would fix your issue for you. You write the glue code that can translate any argument (std::vector<ptdecoder::PTInstruction>) into something more python like like a list of PTInstruction python objects. We do this in a lot of places with LLDB.

I am a bit confused here as well. Are we exporting a plug-in specific python bindings for the Intel PT data? It seems like it would be nice to wrap this API into the SBTrace or other lldb interface? Am I not understanding this correctly?

Probably, I didn't understand this comment of yours. We are not exporting python binding for Intel PT data. It is a python binding for Tool's C++ API and this Tool is built on top of LLDB. Did I answer your question? Please let me know if I misunderstood your comment.

Now I understand. I didn't realize this was a stand alone tool. The main thing to figure out is how stable you want this C++ API that you are exporting to be. If that isn't an issue, feel free to use anything you want in that API. If stability is an issue you want to solve and you want to vend a C++ API, you might think about adopting LLDB's public API rules to help ensure stability.

In D33035#799058, @clayborg wrote:

So std::vector shouldn't be used in a public API. You should make a class that wraps your objects. LLDB's public API has lldb::SBInstruction and lldb::SBInstructionList as examples. std::vector on other systems compiles differently for debug and release builds and things will crash.

Hi Greg .. Does it go for the tools built on top of LLDB (e.g. in my case where feature library is not a part of liblldb shared lib but built on top of it)? If yes, then I will proceed to remove std::vector from C++ public API of the tool and create a new class for it.

If you need this via swig for internal kind of stuff, then use a typemap where you map std::vector<T> to a list() with T instances in it in python.

I want to provide a python interface for the tool's C++ public API as well so that the API can be used in python modules as well. Therefore, I think typemapping to list() will not solve the problem. Am I right?

I am a bit confused here as well. Are we exporting a plug-in specific python bindings for the Intel PT data? It seems like it would be nice to wrap this API into the SBTrace or other lldb interface? Am I not understanding this correctly?

Probably, I didn't understand this comment of yours. We are not exporting python binding for Intel PT data. It is a python binding for Tool's C++ API and this Tool is built on top of LLDB. Did I answer your question? Please let me know if I misunderstood your comment.

In D33035#798885, @labath wrote:

In D33035#798857, @abhishek.aggarwal wrote:

Hi Pavel .. I could remove all exception handling code from source files. However, I am still wondering how to disable exception handling while providing python functions for C++ APIs of the features. Can you suggest something here? The whole problem is occurring because of the use of vector<> as an argument to one of the C++ APIs. To provide a python interface for these APIs, I am forced to include std_vector.i (please see tools/intel-features/intel-pt/interface/PTDecoder.i file) which is introducing exception handling code.

Hm... that's a bit of a drag. I guess the SB API never ran into this problem because it always provides it's own vector-like classes (SBModuleList, SBFileSpecList, etc.). I guess the most canonical way would be to follow that example and have your own class for a list of instructions. However, that is a bit annoying, so I do see a case for making code generated by swig as an exception to the rule.

Hi Pavel .. I could remove all exception handling code from source files. However, I am still wondering how to disable exception handling while providing python functions for C++ APIs of the features. Can you suggest something here? The whole problem is occurring because of the use of vector<> as an argument to one of the C++ APIs. To provide a python interface for these APIs, I am forced to include std_vector.i (please see tools/intel-features/intel-pt/interface/PTDecoder.i file) which is introducing exception handling code.

Removed exception handling code

Thanks for your review Pavel. My comments are inlined. Let me know your opinion :)

Fixed one minor thing in CMakeFile

Cmake files related changes

• Using lldb's cmake functions insted of vanilla ones for cmake files

Hi Pavel .. I have made the changes you suggested. My apologies for misinterpreting your previous comments but during written communications, it is sometimes difficult to interpret everything correctly. I have tried following LLDB's coding conventions and guidelines. Please let me know if I still missed things that you would have liked to see in this patch. Thanks for your patience :)

Changes after feedback

Hi Pavel .. My comments are inlined. Please let me know if you have more concerns. I am submitting the patch with all the proposed changes.

Single feature library development for all intel specific features

In D33035#754640, @labath wrote:

I don't really like that we are adding a public shared library for every tiny intel feature. Could we at least merge this "plugin" with the existing "intel-mpx plugin" to create one "intel support" library?

Also, adding an external dependency probably deserves a discussion on lldb-dev.

Updating D33434: Added new API to SBStructuredData class

• Removed inferior from test case (not required)
• fixed enum scope

Thanks for your suggestions. I have made changes according to feedback and submitting it here.

My comments are inlined. Please let me know if something still needs to be changed.

Updating D33434: Added new API to SBStructuredData class

Abandoning this change due to formatting problem with commit message.

#Updating D33034: Tool for using Intel(R) Processor Trace hardware feature

2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. Commit message formatting changes

Already merged it in LLDb repo. So closing this revision.

Removed get() for shared_ptr

Changes based on review

New patch according to review feedback

Hi Greg

source/Plugins/Process/FreeBSD/FreeBSDThread.cpp will not compile for FreeBSD. In Line 576, bp_id is undefined. Please replace it with bp_site_sp->GetID()

Comments Inlined.

Hi Pavel and Tamas

Added Loggings

Hello Greg

Removed assert if ptrace API fails

Hello Jason

Removed log and inserted statement terminator

Used Assert instead of If condition

I thought that the patch can be landed even if one of the reviewers accepts it. Is it so that the patch requires approval from all the reviewers? If that is the case then I am extremely sorry to land it without your approval.

A test was laready present where registers were loaded from lldb command terminal with specific values and then read to compare the values. However, the test still passed because lldb wrote the register values at wrong offsets, but then it read the register values from the same wrong offsets also.

In D12677#242006, @ovyalov wrote:

I reverted the CL because it was causing TestRegisters.test_fp_special_purpose_register_read to fail on OSX:

• stop reason = EXC_BREAKPOINT
• "register read ftag" yields 0x80 instead of expected 0x8000

Couldn't land this patch by arc due to merge conflicts. Hence merging it manually and closing this revision.

Clang/GCC generate different assembly for same inferior.

In D12592#239288, @tfiala wrote:

Overall LGTM. I'd put the offset calculation into an inline function.

It sounds like you gave it a test on a 32-bit process running against a 64-bit OS? (I'm not sure if we're still using the x86_64 register contexts on 32-bit processes running on a 64-bit host these days, but it would be a good check).

Created a data member in the class to store byte offset of fctrl

Followed up on Jason's comments

If I understood you correctly, you want to add arch.GetTriple().isiOS() also along with arch.GetTriple().isMacOSX(). Am I right ? If yes then I will do it.