[Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables
ted added a comment. We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, hexagon-clang, etc. The test infrastructure is smart enough to pick up hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; will it be smart enough to run an in-tree hexagon-clang? @labath, we run on Windows using hexagon-clang and hexagon-clang++; don't forget the embedded cases when choosing compilers and running tests. I'm all for removing redundant variables. https://reviews.llvm.org/D39215 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables
zturner added a comment. In https://reviews.llvm.org/D39215#905259, @ted wrote: > We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, > hexagon-clang, etc. The test infrastructure is smart enough to pick up > hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; > will it be smart enough to run an in-tree hexagon-clang? > > @labath, we run on Windows using hexagon-clang and hexagon-clang++; don't > forget the embedded cases when choosing compilers and running tests. > > I'm all for removing redundant variables. I haven't checked the logic for deducing the cxx from the c, and vice versa. But if it's as simple as just adding or removing ++ from the end of the executable name, it should work. https://reviews.llvm.org/D39215 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
Author: labath Date: Tue Oct 24 09:07:50 2017 New Revision: 316451 URL: http://llvm.org/viewvc/llvm-project?rev=316451&view=rev Log: Revert "[lldbtests] Handle errors instead of crashing." The commit breaks the case where you specify just a filename to the compiler. Previously, it would look up the compiler in your path, now it complains that the compiler is not found. One of the lldb buildbots is depending on this. It seems like a nice feature to have, as it means less typing and being able to avoid hard-coding the system compiler path in the bot config. This reverts commit r316393. Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=316451&r1=316450&r2=316451&view=diff == --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original) +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Tue Oct 24 09:07:50 2017 @@ -50,11 +50,7 @@ from ..support import seven def is_exe(fpath): -"""Returns true if fpath is an executable. - Exits with an error code if the specified path is invalid""" -if not os.path.exists(fpath): -print(fpath + " is not a valid path, exiting") -sys.exit(-1) +"""Returns true if fpath is an executable.""" return os.path.isfile(fpath) and os.access(fpath, os.X_OK) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
HI, I'm a little curious of what bot this breaks. FWIW, I don't think the bot should rely on this behaviour, and more importantly, I haven't received any failmail. Thanks, -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D39199: [lldbtests] Handle errors instead of crashing
labath added a comment. Davide, I reverted this because it breaks the case when you just specify a filename as a compiler (with the expectation that we will look it up in the path). I think this is a good thing to have, and our buildbot was using that behavior. I like the direction this change was going in, but I couldn't find a simple way to fix this use case, as is_exe is being used from a number of places, so I reverted the change for now. Repository: rL LLVM https://reviews.llvm.org/D39199 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
(and thanks for reverting, BTW :) On Tue, Oct 24, 2017 at 9:10 AM, Davide Italiano wrote: > HI, I'm a little curious of what bot this breaks. > FWIW, I don't think the bot should rely on this behaviour, and more > importantly, I haven't received any failmail. > > Thanks, > > -- > Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
I think there's something like lit.util.which(), or a similar function in lldb test utilities. Seems like we could solve this by writing the function: ``` def is_exe(fpath): if not os.path.exists(fpath): fpath = lit.util.which(fpath) if not (fpath and os.path.exists(fpath)): sys.exit(-1) return is_exe(fpath) return os.path.isfile(fpath) and os.access(fpath, os.X_OK) ``` On Tue, Oct 24, 2017 at 9:08 AM Pavel Labath via lldb-commits < lldb-commits@lists.llvm.org> wrote: > Author: labath > Date: Tue Oct 24 09:07:50 2017 > New Revision: 316451 > > URL: http://llvm.org/viewvc/llvm-project?rev=316451&view=rev > Log: > Revert "[lldbtests] Handle errors instead of crashing." > > The commit breaks the case where you specify just a filename to the > compiler. Previously, it would look up the compiler in your path, now it > complains that the compiler is not found. One of the lldb buildbots is > depending on this. It seems like a nice feature to have, as it means > less typing and being able to avoid hard-coding the system compiler path > in the bot config. > > This reverts commit r316393. > > Modified: > lldb/trunk/packages/Python/lldbsuite/test/dotest.py > > Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=316451&r1=316450&r2=316451&view=diff > > == > --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original) > +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Tue Oct 24 > 09:07:50 2017 > @@ -50,11 +50,7 @@ from ..support import seven > > > def is_exe(fpath): > -"""Returns true if fpath is an executable. > - Exits with an error code if the specified path is invalid""" > -if not os.path.exists(fpath): > -print(fpath + " is not a valid path, exiting") > -sys.exit(-1) > +"""Returns true if fpath is an executable.""" > return os.path.isfile(fpath) and os.access(fpath, os.X_OK) > > > > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
On Tue, Oct 24, 2017 at 9:12 AM, Zachary Turner via lldb-commits wrote: > I think there's something like lit.util.which(), or a similar function in > lldb test utilities. Seems like we could solve this by writing the > function: > > ``` > def is_exe(fpath): >if not os.path.exists(fpath): >fpath = lit.util.which(fpath) >if not (fpath and os.path.exists(fpath)): > sys.exit(-1) > return is_exe(fpath) > >return os.path.isfile(fpath) and os.access(fpath, os.X_OK) > ``` > Yes, makes sense. Pavel, WDYT? Also, If you give me a link to the bot I'll take a look at the conf and try to reproduce (still puzzled as I haven't received the blame mail). Thanks, -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables
On 24 October 2017 at 08:58, Ted Woodward via Phabricator wrote: > ted added a comment. > > We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, > hexagon-clang, etc. The test infrastructure is smart enough to pick up > hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; > will it be smart enough to run an in-tree hexagon-clang? > > @labath, we run on Windows using hexagon-clang and hexagon-clang++; don't > forget the embedded cases when choosing compilers and running tests. For the embedded case, I assume have to run dotest manually anyway (?) If that's true, then nothing will change for you, as we're not touching that part at all. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
The breaking build is this one: < http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14775> The blame email was sent (I know because I got it, as I also had a commit in the same build). Is it possible you overlooked it? ``` def is_exe(fpath): if not os.path.exists(fpath): fpath = lit.util.which(fpath) if not (fpath and os.path.exists(fpath)): sys.exit(-1) return is_exe(fpath) return os.path.isfile(fpath) and os.access(fpath, os.X_OK) ``` === There is a which function in dotest.py -- the thing that made the fix un-obvious is that it is implemented in terms of the is_exe function :D. So it will require a bit of refactoring to achieve this, which i did not want to do in a hurry. On 24 October 2017 at 09:10, Davide Italiano wrote: > HI, I'm a little curious of what bot this breaks. > FWIW, I don't think the bot should rely on this behaviour, and more > importantly, I haven't received any failmail. > > Thanks, > > -- > Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
On Tue, Oct 24, 2017 at 9:25 AM, Pavel Labath wrote: > The breaking build is this one: < > http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14775> > > The blame email was sent (I know because I got it, as I also had a > commit in the same build). Is it possible you overlooked it? > > ``` > def is_exe(fpath): >if not os.path.exists(fpath): >fpath = lit.util.which(fpath) >if not (fpath and os.path.exists(fpath)): > sys.exit(-1) > return is_exe(fpath) > >return os.path.isfile(fpath) and os.access(fpath, os.X_OK) > ``` > === > > There is a which function in dotest.py -- the thing that made the fix > un-obvious is that it is implemented in terms of the is_exe function > :D. > > So it will require a bit of refactoring to achieve this, which i did > not want to do in a hurry. > Now that I have the link I realized gmail put it to spam, go figure. Anyway, thanks. I think the whole dotest.py requires a little bit of love and I'll make sure to test with the same conf in the bot that broke before putting up another review :) Thanks, -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
Fun fact, there are 13 implementations in tree of is_exe (and probably which). Maybe we should try replacing all them with the one from lit? Or is there some hidden dependency I'm missing? [davide@cupiditate lldb]$ grep -R 'def is_exe' * packages/Python/lldbsuite/test/dotest.py:def is_exe(fpath): packages/Python/lldbsuite/test/lldbtest.py:def is_exe(fpath): packages/Python/lldbsuite/test/benchmarks/disassembly/TestDisassembly.py:def is_exe(fpath): packages/Python/lldbsuite/test/lldbutil.py:def is_exe(fpath): scripts/Xcode/build-llvm.py:def is_executable(path): test/testcases/dotest.py:def is_exe(fpath): test/testcases/lldbtest.py:def is_exe(fpath): test/testcases/benchmarks/disassembly/TestDisassembly.py:def is_exe(fpath): test/testcases/lldbutil.py:def is_exe(fpath): utils/test/llvm-mc-shell.py:def is_exe(fpath): utils/test/disasm.py:def is_exe(fpath): utils/test/run-until-faulted.py:def is_exe(fpath): utils/lui/lldbutil.py:def is_exe(fpath): [davide@cupiditate lldb]$ grep -R 'def is_exe' * |wc -l 13 On Tue, Oct 24, 2017 at 9:30 AM, Davide Italiano wrote: > On Tue, Oct 24, 2017 at 9:25 AM, Pavel Labath wrote: >> The breaking build is this one: < >> http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14775> >> >> The blame email was sent (I know because I got it, as I also had a >> commit in the same build). Is it possible you overlooked it? >> >> ``` >> def is_exe(fpath): >>if not os.path.exists(fpath): >>fpath = lit.util.which(fpath) >>if not (fpath and os.path.exists(fpath)): >> sys.exit(-1) >> return is_exe(fpath) >> >>return os.path.isfile(fpath) and os.access(fpath, os.X_OK) >> ``` >> === >> >> There is a which function in dotest.py -- the thing that made the fix >> un-obvious is that it is implemented in terms of the is_exe function >> :D. >> >> So it will require a bit of refactoring to achieve this, which i did >> not want to do in a hurry. >> > > Now that I have the link I realized gmail put it to spam, go figure. > Anyway, thanks. I think the whole dotest.py requires a little bit of > love and I'll make sure to test with the same conf in the bot that > broke before putting up another review :) > > Thanks, > > -- > Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
Actually there's fewer, I think `test/testcases` is a symlink. But there's more than one, for sure. We should standardize on the one in lldbutil.py On Tue, Oct 24, 2017 at 9:33 AM Davide Italiano wrote: > Fun fact, there are 13 implementations in tree of is_exe (and probably > which). Maybe we should try replacing all them with the one from lit? > Or is there some hidden dependency I'm missing? > > [davide@cupiditate lldb]$ grep -R 'def is_exe' * > packages/Python/lldbsuite/test/dotest.py:def is_exe(fpath): > packages/Python/lldbsuite/test/lldbtest.py:def is_exe(fpath): > > packages/Python/lldbsuite/test/benchmarks/disassembly/TestDisassembly.py:def > is_exe(fpath): > packages/Python/lldbsuite/test/lldbutil.py:def is_exe(fpath): > scripts/Xcode/build-llvm.py:def is_executable(path): > test/testcases/dotest.py:def is_exe(fpath): > test/testcases/lldbtest.py:def is_exe(fpath): > test/testcases/benchmarks/disassembly/TestDisassembly.py:def is_exe(fpath): > test/testcases/lldbutil.py:def is_exe(fpath): > utils/test/llvm-mc-shell.py:def is_exe(fpath): > utils/test/disasm.py:def is_exe(fpath): > utils/test/run-until-faulted.py:def is_exe(fpath): > utils/lui/lldbutil.py:def is_exe(fpath): > [davide@cupiditate lldb]$ grep -R 'def is_exe' * |wc -l > 13 > > On Tue, Oct 24, 2017 at 9:30 AM, Davide Italiano > wrote: > > On Tue, Oct 24, 2017 at 9:25 AM, Pavel Labath wrote: > >> The breaking build is this one: < > >> > http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14775 > > > >> > >> The blame email was sent (I know because I got it, as I also had a > >> commit in the same build). Is it possible you overlooked it? > >> > >> ``` > >> def is_exe(fpath): > >>if not os.path.exists(fpath): > >>fpath = lit.util.which(fpath) > >>if not (fpath and os.path.exists(fpath)): > >> sys.exit(-1) > >> return is_exe(fpath) > >> > >>return os.path.isfile(fpath) and os.access(fpath, os.X_OK) > >> ``` > >> === > >> > >> There is a which function in dotest.py -- the thing that made the fix > >> un-obvious is that it is implemented in terms of the is_exe function > >> :D. > >> > >> So it will require a bit of refactoring to achieve this, which i did > >> not want to do in a hurry. > >> > > > > Now that I have the link I realized gmail put it to spam, go figure. > > Anyway, thanks. I think the whole dotest.py requires a little bit of > > love and I'll make sure to test with the same conf in the bot that > > broke before putting up another review :) > > > > Thanks, > > > > -- > > Davide > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r316451 - Revert "[lldbtests] Handle errors instead of crashing."
Reported https://bugs.llvm.org/show_bug.cgi?id=35061 so I don't forget. On Tue, Oct 24, 2017 at 9:35 AM, Zachary Turner wrote: > Actually there's fewer, I think `test/testcases` is a symlink. But there's > more than one, for sure. We should standardize on the one in lldbutil.py > > On Tue, Oct 24, 2017 at 9:33 AM Davide Italiano > wrote: >> >> Fun fact, there are 13 implementations in tree of is_exe (and probably >> which). Maybe we should try replacing all them with the one from lit? >> Or is there some hidden dependency I'm missing? >> >> [davide@cupiditate lldb]$ grep -R 'def is_exe' * >> packages/Python/lldbsuite/test/dotest.py:def is_exe(fpath): >> packages/Python/lldbsuite/test/lldbtest.py:def is_exe(fpath): >> >> packages/Python/lldbsuite/test/benchmarks/disassembly/TestDisassembly.py:def >> is_exe(fpath): >> packages/Python/lldbsuite/test/lldbutil.py:def is_exe(fpath): >> scripts/Xcode/build-llvm.py:def is_executable(path): >> test/testcases/dotest.py:def is_exe(fpath): >> test/testcases/lldbtest.py:def is_exe(fpath): >> test/testcases/benchmarks/disassembly/TestDisassembly.py:def >> is_exe(fpath): >> test/testcases/lldbutil.py:def is_exe(fpath): >> utils/test/llvm-mc-shell.py:def is_exe(fpath): >> utils/test/disasm.py:def is_exe(fpath): >> utils/test/run-until-faulted.py:def is_exe(fpath): >> utils/lui/lldbutil.py:def is_exe(fpath): >> [davide@cupiditate lldb]$ grep -R 'def is_exe' * |wc -l >> 13 >> >> On Tue, Oct 24, 2017 at 9:30 AM, Davide Italiano >> wrote: >> > On Tue, Oct 24, 2017 at 9:25 AM, Pavel Labath wrote: >> >> The breaking build is this one: < >> >> >> >> http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14775> >> >> >> >> The blame email was sent (I know because I got it, as I also had a >> >> commit in the same build). Is it possible you overlooked it? >> >> >> >> ``` >> >> def is_exe(fpath): >> >>if not os.path.exists(fpath): >> >>fpath = lit.util.which(fpath) >> >>if not (fpath and os.path.exists(fpath)): >> >> sys.exit(-1) >> >> return is_exe(fpath) >> >> >> >>return os.path.isfile(fpath) and os.access(fpath, os.X_OK) >> >> ``` >> >> === >> >> >> >> There is a which function in dotest.py -- the thing that made the fix >> >> un-obvious is that it is implemented in terms of the is_exe function >> >> :D. >> >> >> >> So it will require a bit of refactoring to achieve this, which i did >> >> not want to do in a hurry. >> >> >> > >> > Now that I have the link I realized gmail put it to spam, go figure. >> > Anyway, thanks. I think the whole dotest.py requires a little bit of >> > love and I'll make sure to test with the same conf in the bot that >> > broke before putting up another review :) >> > >> > Thanks, >> > >> > -- >> > Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D39246: Fix LLVM_LINK_LLVM_DYLIB build (pr35053)
labath created this revision.
Herald added a subscriber: mgorny.
r316368 broke this build when it introduced a reference to a pthread
function to the Utility module. This caused cmake to generate an
incorrect link line (wrong order of libs) because it did not see the
dependency from Utility to the system libraries. Instead these libraries
were being manually added to each final target.
This changes moves the dependency management from the individual targets
to the lldbUtility module, which is consistent with how llvm does it.
The final targets will pick up these libraries as they will be a part of
the link interface of the module.
Technically, some of these dependencies could go into the host module,
as that's where most of the os-specific code is, but I did not try to
investigate which ones.
https://reviews.llvm.org/D39246
Files:
cmake/LLDBDependencies.cmake
scripts/CMakeLists.txt
source/API/CMakeLists.txt
source/Utility/CMakeLists.txt
tools/argdumper/CMakeLists.txt
tools/driver/CMakeLists.txt
tools/intel-features/CMakeLists.txt
tools/lldb-server/CMakeLists.txt
unittests/CMakeLists.txt
Index: unittests/CMakeLists.txt
===
--- unittests/CMakeLists.txt
+++ unittests/CMakeLists.txt
@@ -11,8 +11,6 @@
list(APPEND LLVM_COMPILE_FLAGS -include ${LLDB_GTEST_COMMON_INCLUDE})
endif ()
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
if (LLDB_BUILT_STANDALONE)
# Build the gtest library needed for unittests, if we have LLVM sources
# handy.
@@ -46,7 +44,7 @@
POST_BUILD
COMMAND "${CMAKE_COMMAND}" -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/Inputs)
- target_link_libraries(${test_name} ${ARG_LINK_LIBS} ${LLDB_SYSTEM_LIBS})
+ target_link_libraries(${test_name} ${ARG_LINK_LIBS})
endfunction()
function(add_unittest_inputs test_name inputs)
Index: tools/lldb-server/CMakeLists.txt
===
--- tools/lldb-server/CMakeLists.txt
+++ tools/lldb-server/CMakeLists.txt
@@ -24,41 +24,6 @@
include_directories(../../source)
-set(LLDB_SYSTEM_LIBS)
-if (NOT LLDB_DISABLE_LIBEDIT)
- list(APPEND LLDB_SYSTEM_LIBS edit)
-endif()
-if (NOT LLDB_DISABLE_CURSES)
- list(APPEND LLDB_SYSTEM_LIBS ${CURSES_LIBRARIES})
- if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
-list(APPEND LLDB_SYSTEM_LIBS ${TERMINFO_LIBS})
- endif()
-endif()
-
-if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
-list(APPEND LLDB_SYSTEM_LIBS atomic)
-endif()
-
-# On FreeBSD/NetBSD backtrace() is provided by libexecinfo, not libc.
-if (CMAKE_SYSTEM_NAME MATCHES "FreeBSD" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD")
- list(APPEND LLDB_SYSTEM_LIBS execinfo)
-endif()
-
-if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
- list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
-endif()
-
-list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
-
-if (LLVM_BUILD_STATIC)
- if (NOT LLDB_DISABLE_PYTHON)
-list(APPEND LLDB_SYSTEM_LIBS python2.7 util)
- endif()
- if (NOT LLDB_DISABLE_CURSES)
-list(APPEND LLDB_SYSTEM_LIBS gpm)
- endif()
-endif()
-
set(LLDB_PLUGINS)
if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
Index: tools/intel-features/CMakeLists.txt
===
--- tools/intel-features/CMakeLists.txt
+++ tools/intel-features/CMakeLists.txt
@@ -1,5 +1,3 @@
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
# Flags to control each individual feature
option(LLDB_BUILD_INTEL_MPX "Enable Building of Intel(R) Memory Protection Extensions" ON)
option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool" OFF)
@@ -63,7 +61,6 @@
# Add link dependencies for python wrapper
if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)
add_dependencies(lldbIntelFeatures intel-features-swig_wrapper)
- target_link_libraries(lldbIntelFeatures PRIVATE ${LLDB_SYSTEM_LIBS})
endif()
install(TARGETS lldbIntelFeatures
Index: tools/driver/CMakeLists.txt
===
--- tools/driver/CMakeLists.txt
+++ tools/driver/CMakeLists.txt
@@ -1,5 +1,3 @@
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
if ((CMAKE_SYSTEM_NAME MATCHES "Windows") OR
(CMAKE_SYSTEM_NAME MATCHES "NetBSD" ))
# These targets do not have getopt support, so they rely on the one provided by
Index: tools/argdumper/CMakeLists.txt
===
--- tools/argdumper/CMakeLists.txt
+++ tools/argdumper/CMakeLists.txt
@@ -1,9 +1,6 @@
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
add_lldb_tool(lldb-argdumper INCLUDE_IN_FRAMEWORK
argdumper.cpp
LINK_LIBS
lldbUtility
)
-
Index: source/Utility/CMakeLists.txt
===
--- source/Utility/CMakeLists.txt
+++ source/Utility/CMakeLists.txt
@@ -1,3 +1,44 @@
+set(LLDB_SYSTEM_LIBS)
+
+# Windows-only libraries
+if ( CMAKE_SY
[Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables
labath updated this revision to Diff 120102.
labath added a comment.
This removes the LLDB_TEST_CLANG and LLDB_TEST_COMPILER settings.
I've decided to keep the separate C and CXX variables because:
a) that's consistent with other cmake settings
b) in case of gcc, it's not easy to figure out the c++ compiler name
(potentially need to strip executable suffix and version number), and I did
not
want to implement one more dodgy heuristic.
https://reviews.llvm.org/D39215
Files:
CMakeLists.txt
lit/CMakeLists.txt
lit/lit.site.cfg.in
test/CMakeLists.txt
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -27,10 +27,6 @@
list(APPEND LLDB_TEST_DEPS lldb-mi)
endif()
-if ("${LLDB_TEST_COMPILER}" STREQUAL "")
-string(REGEX REPLACE ".*ccache\ +" "" LLDB_TEST_COMPILER ${CMAKE_C_COMPILER} ${CMAKE_C_COMPILER_ARG1})
-endif()
-
# The default architecture with which to compile test executables is the default LLVM target
# architecture, which itself defaults to the host architecture.
string(TOLOWER "${LLVM_TARGET_ARCH}" LLDB_DEFAULT_TEST_ARCH)
@@ -43,10 +39,6 @@
${LLDB_DEFAULT_TEST_ARCH}
CACHE STRING "Specify the architecture to run LLDB tests as (x86|x64). Determines whether tests are compiled with -m32 or -m64")
-if(LLDB_TEST_CLANG)
- set(LLDB_TEST_COMPILER $)
-endif()
-
# Users can override LLDB_TEST_USER_ARGS to specify arbitrary arguments to pass to the script
set(LLDB_TEST_USER_ARGS
""
@@ -60,7 +52,7 @@
-S nm
-u CXXFLAGS
-u CFLAGS
- -C ${LLDB_TEST_COMPILER}
+ -C ${LLDB_TEST_C_COMPILER}
)
if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
Index: lit/lit.site.cfg.in
===
--- lit/lit.site.cfg.in
+++ lit/lit.site.cfg.in
@@ -10,22 +10,8 @@
config.lldb_tools_dir = "@LLVM_RUNTIME_OUTPUT_INTDIR@"
config.target_triple = "@TARGET_TRIPLE@"
config.python_executable = "@PYTHON_EXECUTABLE@"
-config.cc = "@CMAKE_C_COMPILER@"
-config.cxx = "@CMAKE_CXX_COMPILER@"
-
-test_c_compiler = "@LLDB_TEST_C_COMPILER@"
-test_cxx_compiler = "@LLDB_TEST_CXX_COMPILER@"
-test_clang = "@LLDB_TEST_CLANG@".lower()
-test_clang = test_clang == "on" or test_clang == "true" or test_clang == "1"
-
-if len(test_c_compiler) > 0:
- config.cc = test_c_compiler
-if len(test_c_compiler) > 0:
- config.cxx = test_cxx_compiler
-
-if test_clang:
- config.cc = 'clang'
- config.cxx = 'clang++'
+config.cc = "@LLDB_TEST_C_COMPILER@"
+config.cxx = "@LLDB_TEST_CXX_COMPILER@"
# Support substitution of the tools and libs dirs with user parameters. This is
# used when we can't determine the tool dir at configuration time.
Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -11,10 +11,6 @@
set(ENABLE_SHARED 0)
endif(BUILD_SHARED_LIBS)
-option(LLDB_TEST_CLANG "Use in-tree clang when testing lldb" Off)
-set(LLDB_TEST_C_COMPILER "" CACHE STRING "C compiler to use when testing LLDB")
-set(LLDB_TEST_CXX_COMPILER "" CACHE STRING "C++ compiler to use when testing LLDB")
-
configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg)
@@ -41,10 +37,7 @@
list(APPEND LLDB_TEST_DEPS debugserver)
endif()
-if(LLDB_TEST_CLANG)
- if(LLDB_TEST_C_COMPILER OR LLDB_TEST_CXX_COMPILER)
-message(SEND_ERROR "Cannot override LLDB_TEST__COMPILER and set LLDB_TEST_CLANG.")
- endif()
+if(TARGET clang)
list(APPEND LLDB_TEST_DEPS clang)
endif()
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -62,6 +62,22 @@
option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests."
${LLVM_INCLUDE_TESTS})
if(LLDB_INCLUDE_TESTS)
+ if (TARGET clang)
+set(LLDB_DEFAULT_TEST_C_COMPILER "${LLVM_BINARY_DIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
+set(LLDB_DEFAULT_TEST_CXX_COMPILER "${LLVM_BINARY_DIR}/bin/clang++${CMAKE_EXECUTABLE_SUFFIX}")
+ else()
+set(LLDB_DEFAULT_TEST_C_COMPILER "")
+set(LLDB_DEFAULT_TEST_CXX_COMPILER "")
+ endif()
+
+ set(LLDB_TEST_C_COMPILER "${LLDB_DEFAULT_TEST_C_COMPILER}" CACHE PATH "C Compiler to use for building LLDB test inferiors")
+ set(LLDB_TEST_CXX_COMPILER "${LLDB_DEFAULT_TEST_CXX_COMPILER}" CACHE PATH "C++ Compiler to use for building LLDB test inferiors")
+
+ if (("${LLDB_TEST_C_COMPILER}" STREQUAL "") OR
+ ("${LLDB_TEST_CXX_COMPILER}" STREQUAL ""))
+message(FATAL_ERROR "LLDB test compilers not specified. Tests will not run")
+ endif()
+
add_subdirectory(test)
add_subdirectory(unittests)
add_subdirectory(lit)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D39246: Fix LLVM_LINK_LLVM_DYLIB build (pr35053)
sylvestre.ledru added a comment. This fixed the issue, thanks! https://reviews.llvm.org/D39246 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
labath added a comment. I'm back now, and I'd like to try to push this patch to completion. After re-reading the discussion, I got the impression we have mostly reached a consensus here. A small issue remained about how to guarantee that the Architecture plugin and the ArchSpec object are in sync. Several versions were thrown around, with no clear conclusion. Does anyone hav objections to how this part is implemented in the last version of the patch? If not, I'd like to put this in... https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
clayborg accepted this revision. clayborg added a comment. Looks fine. We can iterate on this. https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
Looks fine. We can start with this. I was thinking it would be nice to lazily populate m_plugin_up, but then we would need to add a bit to see if we already tried to look for it, so the current approach will work fine. > On Oct 24, 2017, at 1:22 PM, Pavel Labath via Phabricator > wrote: > > labath added a comment. > > I'm back now, and I'd like to try to push this patch to completion. > > After re-reading the discussion, I got the impression we have mostly reached > a consensus here. A small issue remained about how to guarantee that the > Architecture plugin and the ArchSpec object are in sync. Several versions > were thrown around, with no clear conclusion. > > Does anyone hav objections to how this part is implemented in the last > version of the patch? If not, I'd like to put this in... > > > https://reviews.llvm.org/D31172 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner added a comment.
I know you're doing things the way it's always been done, but I want to start
questioning some long-standing practices :) So I'm not picking on you
specifically, but anywhere we can migrate towards something better
incrementally, I think we should do so.
Comment at: include/lldb/Target/Target.h:1213-1214
+ public:
+explicit Arch(const ArchSpec &spec);
+const Arch &operator=(const ArchSpec &spec);
+
Add a move constructor maybe?
Comment at: include/lldb/Target/Target.h:1217
+const ArchSpec &GetSpec() const { return m_spec; }
+Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
Can this return a reference instead of a pointer?
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
Why do we need a `ConstString` and a `std::string` here? I don't think any
plugin ever has a dynamically generated name or description, they exclusively
originate from string literals. So, with that in mind, can both of these just
be `StringRef`?
Comment at: source/Core/PluginManager.cpp:318-323
+ std::lock_guard guard(g_architecture_mutex);
+ for (const auto &instances : GetArchitectureInstances()) {
+if (auto plugin_up = instances.create_callback(arch))
+ return plugin_up;
+ }
+ return nullptr;
These mutexes have always bothered me. Are people really registering and
unregistering plugins dynamically? It seems to me all of this always happens
at debugger startup. Are the mutexes necessary?
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:21-23
+ConstString ArchitectureArm::GetPluginNameStatic() {
+ return ConstString("arm");
+}
Again, doesn't make much sense to me why these need to be const-ified. They're
all string literals anyway, this is just introducing unnecessary locking and
contention on the global string pool.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.h:26
+
+ void OverrideStopInfo(Thread &thread) override;
+
Can you forward declare `Thread` in this file?
Comment at: source/Target/Target.cpp:90-95
+ m_mutex(), m_arch(target_arch),
+ m_images(this), m_section_load_history(), m_breakpoint_list(false),
+ m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
+ m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged,
this),
+ m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
+ m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
Can probably delete all of these initializations that are just invoking default
constructors, and move the rest to inline class member initialization.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
labath added inline comments.
Comment at: include/lldb/Target/Target.h:1217
+const ArchSpec &GetSpec() const { return m_spec; }
+Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
zturner wrote:
> Can this return a reference instead of a pointer?
It can't, because the architecture plugin is not always present (currently we
only have the arm one, and I currently don't see a use for others). We could
consider a no-op plugin, which gets used if no specific plugin is present. This
would even avoid the need for this class, as then the plugin could be
responsible for returning the archspec.
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
zturner wrote:
> Why do we need a `ConstString` and a `std::string` here? I don't think any
> plugin ever has a dynamically generated name or description, they exclusively
> originate from string literals. So, with that in mind, can both of these
> just be `StringRef`?
They, could be, but I don't see a way to enforce that the names come from
string literals, which makes me wonder if the usage of StringRef is a win in
this case...
IIRC, the main reason I made this ConstString (instead of std::string, like the
description), is that this name eventually makes it's way to the
PluginInterface virtual method (which cannot be changed without touching every
plugin).
Comment at: source/Core/PluginManager.cpp:318-323
+ std::lock_guard guard(g_architecture_mutex);
+ for (const auto &instances : GetArchitectureInstances()) {
+if (auto plugin_up = instances.create_callback(arch))
+ return plugin_up;
+ }
+ return nullptr;
zturner wrote:
> These mutexes have always bothered me. Are people really registering and
> unregistering plugins dynamically? It seems to me all of this always happens
> at debugger startup. Are the mutexes necessary?
I'm pretty sure they aren't needed. I was just following what the other plugins
do.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
labath wrote:
> zturner wrote:
> > Why do we need a `ConstString` and a `std::string` here? I don't think any
> > plugin ever has a dynamically generated name or description, they
> > exclusively originate from string literals. So, with that in mind, can
> > both of these just be `StringRef`?
> They, could be, but I don't see a way to enforce that the names come from
> string literals, which makes me wonder if the usage of StringRef is a win in
> this case...
>
> IIRC, the main reason I made this ConstString (instead of std::string, like
> the description), is that this name eventually makes it's way to the
> PluginInterface virtual method (which cannot be changed without touching
> every plugin).
I don't think we need to enforce it. Documentation is good enough. If someone
accepts a `StringRef` to a function, you, as the caller of the function, cannot
enforce what they do with it. They could store a pointer to it. That doesn't
mean you should never pass in a `std::string` and let the implicit conversion
happen, it just means you have to follow the contract. Same could happen if
your function took a `const std::string&`.
Documenting the top level interface to say "The memory for this string is
assumed to live for the life of the program" should be sufficient.
If someone needs to do something funky and construct a name dynamically, they
can make their plugin contain an `llvm::StringSaver` and get a stable pointer
that way.
Comment at: source/Core/PluginManager.cpp:318-323
+ std::lock_guard guard(g_architecture_mutex);
+ for (const auto &instances : GetArchitectureInstances()) {
+if (auto plugin_up = instances.create_callback(arch))
+ return plugin_up;
+ }
+ return nullptr;
labath wrote:
> zturner wrote:
> > These mutexes have always bothered me. Are people really registering and
> > unregistering plugins dynamically? It seems to me all of this always
> > happens at debugger startup. Are the mutexes necessary?
> I'm pretty sure they aren't needed. I was just following what the other
> plugins do.
So, follow up question... Can we remove it?
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
clayborg added inline comments.
Comment at: include/lldb/Target/Target.h:1217
+const ArchSpec &GetSpec() const { return m_spec; }
+Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
zturner wrote:
> Can this return a reference instead of a pointer?
No, most architectures don't have one of these plug-ins. It isn't mandatory, so
NULL is just fine.
Comment at: source/Core/PluginManager.cpp:281
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
zturner wrote:
> Why do we need a `ConstString` and a `std::string` here? I don't think any
> plugin ever has a dynamically generated name or description, they exclusively
> originate from string literals. So, with that in mind, can both of these
> just be `StringRef`?
ConstString makes for faster lookups. Many plugins can ask for a plug-in by
name, so ConstString works well in that regard.
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
We need "std::string" since it owns the storage. Most people do init this with
a const string, but they don't need to. ConstString doesn't work here because
we never will search for a description. StringRef doesn't own the storage, so I
would rather avoid StringRef unless you can guarantee no one can construct a
StringRef with local data.
Comment at: source/Core/PluginManager.cpp:323
+ }
+ return nullptr;
+}
You are probably correct. I don't really mind the thread safety. We would need
to worry about this more if people could write external plug-ins, but that
isn't the case here. I would just leave it personally.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
One time at startup. No threads contending yet. Asking for plug-in by name is
made fast for later. I would leave this.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.h:26
+
+ void OverrideStopInfo(Thread &thread) override;
+
zturner wrote:
> Can you forward declare `Thread` in this file?
use lldb-forward.h
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
zturner wrote:
> labath wrote:
> > clayborg wrote:
> > > zturner wrote:
> > > > Why do we need a `ConstString` and a `std::string` here? I don't think
> > > > any plugin ever has a dynamically generated name or description, they
> > > > exclusively originate from string literals. So, with that in mind, can
> > > > both of these just be `StringRef`?
> > > ConstString makes for faster lookups. Many plugins can ask for a plug-in
> > > by name, so ConstString works well in that regard.
> > They, could be, but I don't see a way to enforce that the names come from
> > string literals, which makes me wonder if the usage of StringRef is a win
> > in this case...
> >
> > IIRC, the main reason I made this ConstString (instead of std::string, like
> > the description), is that this name eventually makes it's way to the
> > PluginInterface virtual method (which cannot be changed without touching
> > every plugin).
> I don't think we need to enforce it. Documentation is good enough. If
> someone accepts a `StringRef` to a function, you, as the caller of the
> function, cannot enforce what they do with it. They could store a pointer to
> it. That doesn't mean you should never pass in a `std::string` and let the
> implicit conversion happen, it just means you have to follow the contract.
> Same could happen if your function took a `const std::string&`.
> Documenting the top level interface to say "The memory for this string is
> assumed to live for the life of the program" should be sufficient.
>
> If someone needs to do something funky and construct a name dynamically, they
> can make their plugin contain an `llvm::StringSaver` and get a stable pointer
> that way.
I'm not convinced it makes for faster lookups, because while true that it is
"just" a pointer comparison, it is a pointer comparison that involves taking a
global mutex.
In the case of `StringRef` constructed from a string literal (which as I
mentioned, is I believe 100% of usage today), `StringRef` will be faster,
because it will pass an unguarded pointer comparison. Only if the pointer
comparisons fail does `StringRef` fall back to `memcmp` (or at least, it
*should* work that way, if it doesn't we can make it)
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
clayborg wrote:
> We need "std::string" since it owns the storage. Most people do init this
> with a const string, but they don't need to. ConstString doesn't work here
> because we never will search for a description. StringRef doesn't own the
> storage, so I would rather avoid StringRef unless you can guarantee no one
> can construct a StringRef with local data.
But why do you need to own the storage of a string literal? The binary already
owns that storage. Just document in the API that it needs to be stable
storage, since that covers 100% of existing use cases. And if someone needs
something else they can use an `llvm::StringSaver`
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> One time at startup. No threads contending yet. Asking for plug-in by name is
> made fast for later. I would leave this.
Is asking for a plugin by name something that happens in a hot path?
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
clayborg added inline comments.
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
zturner wrote:
> clayborg wrote:
> > We need "std::string" since it owns the storage. Most people do init this
> > with a const string, but they don't need to. ConstString doesn't work here
> > because we never will search for a description. StringRef doesn't own the
> > storage, so I would rather avoid StringRef unless you can guarantee no one
> > can construct a StringRef with local data.
> But why do you need to own the storage of a string literal? The binary
> already owns that storage. Just document in the API that it needs to be
> stable storage, since that covers 100% of existing use cases. And if someone
> needs something else they can use an `llvm::StringSaver`
Why not just use a std::string then. Both std::string and StringRef will have
similar compare times.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
zturner wrote:
> clayborg wrote:
> > One time at startup. No threads contending yet. Asking for plug-in by name
> > is made fast for later. I would leave this.
> Is asking for a plugin by name something that happens in a hot path?
No. If we are talking about making the code safer, I would rather go with
something that can guarantee safety. The StringRef has the possibility of going
wrong if someone uses a local string. So unless we can guarantee it I don't see
a reason to change.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > One time at startup. No threads contending yet. Asking for plug-in by
> > > name is made fast for later. I would leave this.
> > Is asking for a plugin by name something that happens in a hot path?
> No. If we are talking about making the code safer, I would rather go with
> something that can guarantee safety. The StringRef has the possibility of
> going wrong if someone uses a local string. So unless we can guarantee it I
> don't see a reason to change.
I agree with you that it leaves open the possibility of something going wrong,
but why isn't "don't use a local string" a sufficient answer to this. I
thought (perhaps mistakenly) that we agreed that if you document your
assumptions, you can then assume that people will follow them. This isn't user
input.
It's worth mentioning that strings are one of LLDB's biggest memory hogs, and
getting as much stuff out of the global string pool as possible is a win from a
memory perspective. So, I don't think "I'm not really sure if anyone will ever
do the wrong thing here, so let's just be safe" is a good long term strategy.
Instead, we shoudl look at each case and say "can we tell users to do something
reasonable here", and if so we should do that.
There was a talk at cppcon a few weeks ago from someone at backtrace.io who had
some insightful metrics on debugger performance memory consumption, and LLDB
had ~2x the memory consumption of GDB.
So, I think this isn't a pretend problem, and that we should be more vigilant
about mindlessly constifying all strings "just in case". There has to be a
strong technical argument for it (i.e. millions of duplicated strings
originating from user input, a hot-path, etc).
That said, this is a pretty uninteresting case, so I won't push it here, but
unnecessary use of `ConstString` this is something I'm actively interested in
reducing.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
clayborg added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
zturner wrote:
> clayborg wrote:
> > zturner wrote:
> > > clayborg wrote:
> > > > One time at startup. No threads contending yet. Asking for plug-in by
> > > > name is made fast for later. I would leave this.
> > > Is asking for a plugin by name something that happens in a hot path?
> > No. If we are talking about making the code safer, I would rather go with
> > something that can guarantee safety. The StringRef has the possibility of
> > going wrong if someone uses a local string. So unless we can guarantee it I
> > don't see a reason to change.
> I agree with you that it leaves open the possibility of something going
> wrong, but why isn't "don't use a local string" a sufficient answer to this.
> I thought (perhaps mistakenly) that we agreed that if you document your
> assumptions, you can then assume that people will follow them. This isn't
> user input.
>
> It's worth mentioning that strings are one of LLDB's biggest memory hogs, and
> getting as much stuff out of the global string pool as possible is a win from
> a memory perspective. So, I don't think "I'm not really sure if anyone will
> ever do the wrong thing here, so let's just be safe" is a good long term
> strategy. Instead, we shoudl look at each case and say "can we tell users to
> do something reasonable here", and if so we should do that.
>
> There was a talk at cppcon a few weeks ago from someone at backtrace.io who
> had some insightful metrics on debugger performance memory consumption, and
> LLDB had ~2x the memory consumption of GDB.
>
> So, I think this isn't a pretend problem, and that we should be more vigilant
> about mindlessly constifying all strings "just in case". There has to be a
> strong technical argument for it (i.e. millions of duplicated strings
> originating from user input, a hot-path, etc).
>
> That said, this is a pretty uninteresting case, so I won't push it here, but
> unnecessary use of `ConstString` this is something I'm actively interested in
> reducing.
We mmap everything. Many people don't know how to correctly say which programs
are using too much memory. I don't care if we mmap 20GB of data, I care if we
page that all into real memory. Not saying that the people that presented were
making errors, but it could be interesting to see their data to see what they
found and see if their issues are real ones.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
jasonmolenda added a comment. > There was a talk at cppcon a few weeks ago from someone at backtrace.io who > had some insightful metrics on debugger performance memory consumption, and > LLDB had ~2x the memory consumption of GDB. I haven't seen the paper, but my guess is that this is on linux and lldb wasn't using any accelerator tables for the dwarf of their inferior test program. gdb was designed before there were any accelerator tables, so in their absence, it will create "partial symbol tables" with the address ranges and globally visible names of each CU. When details are needed for a specific CU, the psymtab is expanded into a symbol table, symtab. DWARF is scanned on initial load to create the psymtab names, and then when the symtab is created, it is ingested a CU at a time. nb: my knowledge of lldb is a decade old at this point, I have not kept up with the project. lldb was designed with the assumption that we could get these global symbol names from accelerator tables - we would map them into our address space and search for names in them. When we are interested in a specific type or function or variable, we will read only the DWARF related to that type/function/variable. In the absence of accelerator tables, I am willing to believe that lldb exhibits poor memory and startup performance -- that was not something we (at apple) concentrated on the performance of. It's a completely valid area for future work in lldb -- adopting to using accelerator tables other than apple's vendor specific ones. I suspect, in an absence of concrete information, that the measurement of lldb's memory use being primarily strings, and the perf issues mentioned in this paper, are all due to this non-performant code path on Linux. If we see these memory issues when there are accelerator tables, then that's a big problem and I'll be shocked if we can actually be out-performed by gdb. They were the base line that we designed ourselves to do better than. We can expend a lot of energy on making string tables smaller, and of course I wouldn't object to people working in that direction. But I think the real solution is to get lldb on non-darwin platforms to be using the accelerator tables that exist and allowing the full design of lldb to work as is intended. My two cents. https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
zturner accepted this revision. zturner added a comment. I will ping them for some numbers and more details of their test setup. Regardless, I didn't mean to derail the code review. But, I really really want to reach a point where we can stop falling back on the "we need to be safe even in the presence of stuff that is clearly not user input" argument. I understand the concerns, but I don't think this is a reasonable path forward for the project. If it's not user input, if we own it, then we can make whatever assumption we want that leads to the best performance and memory usage characteristics. As I said this is a pretty uninteresting case, and probably makes no difference since we're talking about 30-40 strings in the whole program, so I'll leave it at that. But I will probably push on this harder in the future if it's in some other area of code that has a larger impact. https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316526 - Remove some dead code from ClangExpressionDeclMap.cpp
Author: sas
Date: Tue Oct 24 15:56:05 2017
New Revision: 316526
URL: http://llvm.org/viewvc/llvm-project?rev=316526&view=rev
Log:
Remove some dead code from ClangExpressionDeclMap.cpp
Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp?rev=316526&r1=316525&r2=316526&view=diff
==
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
(original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Tue Oct 24 15:56:05 2017
@@ -1540,17 +1540,17 @@ void ClangExpressionDeclMap::FindExterna
// a generic
// data symbol, and -- if it is found -- treat it as a variable.
Status error;
-
+
const Symbol *data_symbol =
m_parser_vars->m_sym_ctx.FindBestGlobalDataSymbol(name, error);
-
+
if (!error.Success()) {
const unsigned diag_id =
m_ast_context->getDiagnostics().getCustomDiagID(
clang::DiagnosticsEngine::Level::Error, "%0");
m_ast_context->getDiagnostics().Report(diag_id) << error.AsCString();
}
-
+
if (data_symbol) {
std::string warning("got name from symbols: ");
warning.append(name.AsCString());
@@ -1565,49 +1565,6 @@ void ClangExpressionDeclMap::FindExterna
}
}
-// static opaque_compiler_type_t
-// MaybePromoteToBlockPointerType
-//(
-//ASTContext *ast_context,
-//opaque_compiler_type_t candidate_type
-//)
-//{
-//if (!candidate_type)
-//return candidate_type;
-//
-//QualType candidate_qual_type =
QualType::getFromOpaquePtr(candidate_type);
-//
-//const PointerType *candidate_pointer_type =
-//dyn_cast(candidate_qual_type);
-//
-//if (!candidate_pointer_type)
-//return candidate_type;
-//
-//QualType pointee_qual_type = candidate_pointer_type->getPointeeType();
-//
-//const RecordType *pointee_record_type =
-//dyn_cast(pointee_qual_type);
-//
-//if (!pointee_record_type)
-//return candidate_type;
-//
-//RecordDecl *pointee_record_decl = pointee_record_type->getDecl();
-//
-//if (!pointee_record_decl->isRecord())
-//return candidate_type;
-//
-//if
-//
(!pointee_record_decl->getName().startswith(llvm::StringRef("__block_literal_")))
-//return candidate_type;
-//
-//QualType generic_function_type =
-//ast_context->getFunctionNoProtoType(ast_context->UnknownAnyTy);
-//QualType block_pointer_type =
-//ast_context->getBlockPointerType(generic_function_type);
-//
-//return block_pointer_type.getAsOpaquePtr();
-//}
-
bool ClangExpressionDeclMap::GetVariableValue(VariableSP &var,
lldb_private::Value
&var_location,
TypeFromUser *user_type,
@@ -1647,7 +1604,6 @@ bool ClangExpressionDeclMap::GetVariable
"There is no AST context for the current execution context");
return false;
}
- // var_clang_type = MaybePromoteToBlockPointerType (ast, var_clang_type);
DWARFExpression &var_location_expr = var->LocationExpression();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316527 - Remove some unused function calls from ClangUserExpression.cpp
Author: sas Date: Tue Oct 24 16:01:33 2017 New Revision: 316527 URL: http://llvm.org/viewvc/llvm-project?rev=316527&view=rev Log: Remove some unused function calls from ClangUserExpression.cpp Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp?rev=316527&r1=316526&r2=316527&view=diff == --- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp (original) +++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp Tue Oct 24 16:01:33 2017 @@ -347,7 +347,6 @@ bool ClangUserExpression::Parse(Diagnost // ApplyObjcCastHack(m_expr_text); - // ApplyUnicharHack(m_expr_text); std::string prefix = m_expr_prefix; ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D19604: Allow ObjectFilePECOFF to initialize with ARM binaries.
sas updated this revision to Diff 120146.
sas added a comment.
Herald added a subscriber: kristof.beyls.
Rebase.
https://reviews.llvm.org/D19604
Files:
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -136,6 +136,11 @@
spec.SetTriple("i686-pc-windows");
specs.Append(ModuleSpec(file, spec));
}
+else if (coff_header.machine == MachineArmNt)
+{
+ spec.SetTriple("arm-pc-windows");
+ specs.Append(ModuleSpec(file, spec));
+}
}
}
}
Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -136,6 +136,11 @@
spec.SetTriple("i686-pc-windows");
specs.Append(ModuleSpec(file, spec));
}
+else if (coff_header.machine == MachineArmNt)
+{
+ spec.SetTriple("arm-pc-windows");
+ specs.Append(ModuleSpec(file, spec));
+}
}
}
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316529 - [ExpressionParser] Garbage-collect dead code. NFCI.
Author: davide
Date: Tue Oct 24 16:29:01 2017
New Revision: 316529
URL: http://llvm.org/viewvc/llvm-project?rev=316529&view=rev
Log:
[ExpressionParser] Garbage-collect dead code. NFCI.
Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp?rev=316529&r1=316528&r2=316529&view=diff
==
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
(original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
Tue Oct 24 16:29:01 2017
@@ -138,15 +138,6 @@ bool ClangUtilityFunction::Install(Diagn
}
}
-#if 0
- // jingham: look here
-StreamFile logfile ("/tmp/exprs.txt", "a");
-logfile.Printf ("0x%16.16" PRIx64 ": func = %s, source =\n%s\n",
-m_jit_start_addr,
-m_function_name.c_str(),
-m_function_text.c_str());
-#endif
-
DeclMap()->DidParse();
ResetDeclMap();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316530 - [FreeBSD] Remove more dead code. NFCI.
Author: davide
Date: Tue Oct 24 16:31:53 2017
New Revision: 316530
URL: http://llvm.org/viewvc/llvm-project?rev=316530&view=rev
Log:
[FreeBSD] Remove more dead code. NFCI.
Modified:
lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
Modified: lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp?rev=316530&r1=316529&r2=316530&view=diff
==
--- lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp (original)
+++ lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp Tue Oct 24
16:31:53 2017
@@ -825,32 +825,6 @@ uint32_t ProcessFreeBSD::UpdateThreadLis
return m_thread_list.GetSize(false);
}
-#if 0
-bool
-ProcessFreeBSD::UpdateThreadList(ThreadList &old_thread_list, ThreadList
&new_thread_list)
-{
-Log *log (ProcessPOSIXLog::GetLogIfAllCategoriesSet (POSIX_LOG_THREAD));
-if (log && log->GetMask().Test(POSIX_LOG_VERBOSE))
-log->Printf ("ProcessFreeBSD::%s() (pid = %" PRIi64 ")", __FUNCTION__,
GetID());
-
-bool has_updated = false;
-// Update the process thread list with this new thread.
-// FIXME: We should be using tid, not pid.
-assert(m_monitor);
-ThreadSP thread_sp (old_thread_list.FindThreadByID (GetID(), false));
-if (!thread_sp) {
-thread_sp.reset(CreateNewFreeBSDThread(*this, GetID()));
-has_updated = true;
-}
-
-if (log && log->GetMask().Test(POSIX_LOG_VERBOSE))
-log->Printf ("ProcessFreeBSD::%s() updated pid = %" PRIi64,
__FUNCTION__, GetID());
-new_thread_list.AddThread(thread_sp);
-
-return has_updated; // the list has been updated
-}
-#endif
-
ByteOrder ProcessFreeBSD::GetByteOrder() const {
// FIXME: We should be able to extract this value directly. See comment in
// ProcessFreeBSD().
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316532 - Allow ObjectFilePECOFF to initialize with ARM binaries.
Author: sas
Date: Tue Oct 24 16:40:59 2017
New Revision: 316532
URL: http://llvm.org/viewvc/llvm-project?rev=316532&view=rev
Log:
Allow ObjectFilePECOFF to initialize with ARM binaries.
Summary: This is required to start debugging WinPhone ARM targets.
Reviewers: compnerd, zturner, omjavaid
Reviewed By: compnerd
Subscribers: jasonmolenda, aemerson, rengolin, lldb-commits
Differential Revision: https://reviews.llvm.org/D19604
Modified:
lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
Modified: lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp?rev=316532&r1=316531&r2=316532&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp Tue Oct 24
16:40:59 2017
@@ -136,6 +136,11 @@ size_t ObjectFilePECOFF::GetModuleSpecif
spec.SetTriple("i686-pc-windows");
specs.Append(ModuleSpec(file, spec));
}
+else if (coff_header.machine == MachineArmNt)
+{
+ spec.SetTriple("arm-pc-windows");
+ specs.Append(ModuleSpec(file, spec));
+}
}
}
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D19604: Allow ObjectFilePECOFF to initialize with ARM binaries.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL316532: Allow ObjectFilePECOFF to initialize with ARM
binaries. (authored by sas).
Repository:
rL LLVM
https://reviews.llvm.org/D19604
Files:
lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
Index: lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -136,6 +136,11 @@
spec.SetTriple("i686-pc-windows");
specs.Append(ModuleSpec(file, spec));
}
+else if (coff_header.machine == MachineArmNt)
+{
+ spec.SetTriple("arm-pc-windows");
+ specs.Append(ModuleSpec(file, spec));
+}
}
}
}
Index: lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -136,6 +136,11 @@
spec.SetTriple("i686-pc-windows");
specs.Append(ModuleSpec(file, spec));
}
+else if (coff_header.machine == MachineArmNt)
+{
+ spec.SetTriple("arm-pc-windows");
+ specs.Append(ModuleSpec(file, spec));
+}
}
}
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r316533 - Fix a compile warning on linux
Author: sas
Date: Tue Oct 24 16:46:00 2017
New Revision: 316533
URL: http://llvm.org/viewvc/llvm-project?rev=316533&view=rev
Log:
Fix a compile warning on linux
Can't cast directly between a pointer to function and a pointer to
object.
Modified:
lldb/trunk/source/API/SBDebugger.cpp
Modified: lldb/trunk/source/API/SBDebugger.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBDebugger.cpp?rev=316533&r1=316532&r2=316533&view=diff
==
--- lldb/trunk/source/API/SBDebugger.cpp (original)
+++ lldb/trunk/source/API/SBDebugger.cpp Tue Oct 24 16:46:00 2017
@@ -72,7 +72,7 @@ static llvm::sys::DynamicLibrary LoadPlu
// TODO: mangle this differently for your system - on OSX, the first
// underscore needs to be removed and the second one stays
LLDBCommandPluginInit init_func =
-(LLDBCommandPluginInit)dynlib.getAddressOfSymbol(
+(LLDBCommandPluginInit)(uintptr_t)dynlib.getAddressOfSymbol(
"_ZN4lldb16PluginInitializeENS_10SBDebuggerE");
if (init_func) {
if (init_func(debugger_sb))
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
