abhishek.aggarwal added a comment.
Hi Greg .. Can you please review the changes? Please let me know if something
needs to be changed. Thanks :)
https://reviews.llvm.org/D33035
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
abhishek.aggarwal added a comment.
In https://reviews.llvm.org/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
clayborg added a comment.
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
abhishek.aggarwal added a comment.
In https://reviews.llvm.org/D33035#803801, @clayborg wrote:
> Much better on the API with Swig. Just a few inlined questions around
> GetRawBytes.
Hi Greg .. Thanks for feedback. My comments are inlined.
Comment at:
clayborg added a comment.
Much better on the API with Swig. Just a few inlined questions around
GetRawBytes.
Comment at: tools/intel-features/intel-pt/PTDecoder.h:54-55
+
+ // Get raw bytes of the instruction
+ const uint8_t *GetRawBytes() const;
+
In the
abhishek.aggarwal updated this revision to Diff 105434.
abhishek.aggarwal added a comment.
Removed std::vector<> from public APIs
https://reviews.llvm.org/D33035
Files:
tools/CMakeLists.txt
tools/intel-features/CMakeLists.txt
tools/intel-features/README.txt
clayborg added a comment.
In https://reviews.llvm.org/D33035#799404, @abhishek.aggarwal wrote:
> In https://reviews.llvm.org/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
abhishek.aggarwal added a comment.
In https://reviews.llvm.org/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
clayborg added a comment.
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
labath added a comment.
In https://reviews.llvm.org/D33035#798925, @abhishek.aggarwal wrote:
> In https://reviews.llvm.org/D33035#798885, @labath wrote:
>
> > 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
abhishek.aggarwal added a comment.
In https://reviews.llvm.org/D33035#798885, @labath wrote:
> In https://reviews.llvm.org/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
labath added a comment.
In https://reviews.llvm.org/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
abhishek.aggarwal added a comment.
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
abhishek.aggarwal updated this revision to Diff 105163.
abhishek.aggarwal added a comment.
Removed exception handling code
https://reviews.llvm.org/D33035
Files:
tools/CMakeLists.txt
tools/intel-features/CMakeLists.txt
tools/intel-features/README.txt
abhishek.aggarwal added inline comments.
Comment at: tools/intel-features/intel-pt/Decoder.cpp:411
+std::string image_path(image_complete_path, path_length);
+try {
+ readExecuteSectionInfos.emplace_back(
labath wrote:
>
labath added inline comments.
Comment at: tools/intel-features/intel-pt/Decoder.cpp:411
+std::string image_path(image_complete_path, path_length);
+try {
+ readExecuteSectionInfos.emplace_back(
abhishek.aggarwal wrote:
> labath wrote:
>
abhishek.aggarwal added a comment.
Thanks for your review Pavel. My comments are inlined. Let me know your opinion
:)
Comment at: tools/intel-features/intel-pt/Decoder.cpp:411
+std::string image_path(image_complete_path, path_length);
+try {
+
abhishek.aggarwal updated this revision to Diff 104430.
abhishek.aggarwal added a comment.
Fixed one minor thing in CMakeFile
https://reviews.llvm.org/D33035
Files:
tools/CMakeLists.txt
tools/intel-features/CMakeLists.txt
tools/intel-features/README.txt
abhishek.aggarwal updated this revision to Diff 104376.
abhishek.aggarwal added a comment.
Cmake files related changes
- Using lldb's cmake functions insted of vanilla ones for cmake files
https://reviews.llvm.org/D33035
Files:
tools/CMakeLists.txt
tools/intel-features/CMakeLists.txt
abhishek.aggarwal added a comment.
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
labath added a comment.
Hi Abhishek,
Thank you for incorporating the changes. I still see some room for improvement
in the cmake files. I realize that this is something most people are not
familiar with, and is not exactly glamorous work, but it's still part of our
codebase and we should make
abhishek.aggarwal updated this revision to Diff 103924.
abhishek.aggarwal added a comment.
Changes after feedback
- Created static libs for each hardware feature to combine it to generate
single shared library
- Removed error codes decoding logic and directly using error strings
abhishek.aggarwal added a comment.
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.
Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+ LIBRARY
labath added inline comments.
Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+ LIBRARY DESTINATION bin)
"bin" sounds wrong here. Shouldn't this go ti lib${LLVM_LIBDIR_SUFFIX}?
To properly handle DLL targets (i don't know
abhishek.aggarwal added a subscriber: ravitheja.
abhishek.aggarwal added inline comments.
Comment at: tools/intel-pt/CMakeLists.txt:53
+
+if (NOT LLDB_DISABLE_PYTHON)
+ target_link_libraries(lldbIntelPT PRIVATE
labath wrote:
> All of this needs to go away. I
labath added a comment.
In https://reviews.llvm.org/D33035#767029, @abhishek.aggarwal wrote:
> In https://reviews.llvm.org/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"
abhishek.aggarwal added a comment.
In https://reviews.llvm.org/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
abhishek.aggarwal added inline comments.
Comment at: tools/CMakeLists.txt:8
add_subdirectory(lldb-mi)
+option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool"
OFF)
+if (LLDB_BUILD_INTEL_PT)
clayborg wrote:
> Can we default this to
labath added a comment.
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
krytarowski added a comment.
In https://reviews.llvm.org/D33035#751280, @clayborg wrote:
> In https://reviews.llvm.org/D33035#751264, @krytarowski wrote:
>
> > Can we please use the generic process plugin code, not the specific Linux
> > one? If the generic code is insufficient - we should
clayborg added a comment.
In https://reviews.llvm.org/D33035#751264, @krytarowski wrote:
> Can we please use the generic process plugin code, not the specific Linux
> one? If the generic code is insufficient - we should adjust it and alter
> instances of it (Linux and BSD).
There should be
krytarowski added a comment.
Can we please use a generic process plugin code, not the specific Linux one? If
the generic code is insufficient - we should adjust it and alter instances of
it (Linux and BSD).
https://reviews.llvm.org/D33035
___
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So we don't want clients of SBStructuredData to have to text scrape and write
manual decoders. See my inlined comments on the minimal modifications we need
to make to
33 matches
Mail list logo
