[Lldb-commits] [PATCH] D54452: [NativePDB] Add support for handling S_CONSTANT records

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added a comment.

LGTM too, thanks!


https://reviews.llvm.org/D54452



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.



Comment at: cmake/modules/AddLLVM.cmake:795
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS} FORCE)
 endmacro(add_llvm_executable name)

beanz wrote:
> sgraenitz wrote:
> > Would we want to pass `FORCE` to `add_llvm_executable` conditionally?
> I'm trying to think about the situations in which we need the `FORCE` option. 
> Since this is connecting as a post-build event it shouldn't be running unless 
> the target re-generates the output, so I'm not sure I understand why we ever 
> need it.
> 
> I did the git blame walk back to when the code was initially added in 
> `49dd98a03a`, and there is no explanation. I suspect debugserver doesn't 
> actually need the `--force` option because the author of the initial patch 
> probably hit errors when re-signing the pre-built binary in his build 
> directory.
> 
> Thoughts?
I think you are right, it shouldn't be necessary. In practice, though, I can 
imagine situations when we want to make sure this won't fail in any case.

The options are: remove entirely (most correct) OR allow enable per target 
(most flexible) OR allow enable globally.

What about the last one? I could add `LLVM_CODESIGNING_FORCE` which is OFF by 
default. In failsafe/debugging situations it could be turned ON globally. I 
could remove the `FORCE` parameter here and check the flag in `llvm_codesign` 
(similar to `LLVM_CODESIGNING_IDENTITY`).


Repository:
  rL LLVM

https://reviews.llvm.org/D54443



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54444: [CMake] Use extended llvm_codesign to pass entitlements for lldb-server

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.



Comment at: tools/lldb-server/CMakeLists.txt:50
+  if(LLDB_USE_ENTITLEMENTS AND NOT IOS)
+# Same entitlements file as used for lldb-server
+set(entitlements 
${LLDB_SOURCE_DIR}/resources/debugserver-macosx-entitlements.plist)

TODO: Fix comment "debugserver"


https://reviews.llvm.org/D5



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54444: [CMake] Use extended llvm_codesign to pass entitlements for lldb-server

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz updated this revision to Diff 173849.
sgraenitz added a comment.

unset not necessary, fix comment, globally set LLVM_CODESIGNING_IDENTITY from 
LLDB_CODESIGN_IDENTITY


https://reviews.llvm.org/D5

Files:
  CMakeLists.txt
  cmake/modules/AddLLDB.cmake
  tools/lldb-server/CMakeLists.txt


Index: tools/lldb-server/CMakeLists.txt
===
--- tools/lldb-server/CMakeLists.txt
+++ tools/lldb-server/CMakeLists.txt
@@ -42,6 +42,11 @@
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileELF)
 endif()
 
+if(LLDB_USE_ENTITLEMENTS AND NOT IOS)
+  # Same entitlements file as used for debugserver
+  set(entitlements 
${LLDB_SOURCE_DIR}/resources/debugserver-macosx-entitlements.plist)
+endif()
+
 add_lldb_tool(lldb-server INCLUDE_IN_SUITE
 Acceptor.cpp
 lldb-gdbserver.cpp
@@ -61,6 +66,9 @@
 
 LINK_COMPONENTS
   Support
+
+ENTITLEMENTS
+  ${entitlements}
 )
 
 target_link_libraries(lldb-server PRIVATE ${LLDB_SYSTEM_LIBS})
Index: cmake/modules/AddLLDB.cmake
===
--- cmake/modules/AddLLDB.cmake
+++ cmake/modules/AddLLDB.cmake
@@ -100,13 +100,13 @@
 function(add_lldb_executable name)
   cmake_parse_arguments(ARG
 "INCLUDE_IN_SUITE;GENERATE_INSTALL"
-""
+"ENTITLEMENTS"
 "LINK_LIBS;LINK_COMPONENTS"
 ${ARGN}
 )
 
   list(APPEND LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
-  add_llvm_executable(${name} ${ARG_UNPARSED_ARGUMENTS})
+  add_llvm_executable(${name} ${ARG_UNPARSED_ARGUMENTS} ENTITLEMENTS 
${ARG_ENTITLEMENTS})
 
   target_link_libraries(${name} PRIVATE ${ARG_LINK_LIBS})
   set_target_properties(${name} PROPERTIES
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -11,6 +11,12 @@
 include(LLDBConfig)
 include(AddLLDB)
 
+option(LLDB_USE_ENTITLEMENTS "When codesigning, use entitlements if available" 
ON)
+if(LLDB_CODESIGN_IDENTITY)
+  # In the future we may use LLVM_CODESIGNING_IDENTITY directly.
+  set(LLVM_CODESIGNING_IDENTITY ${LLDB_CODESIGN_IDENTITY})
+endif()
+
 # Define the LLDB_CONFIGURATION_xxx matching the build type
 if( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   add_definitions( -DLLDB_CONFIGURATION_DEBUG )


Index: tools/lldb-server/CMakeLists.txt
===
--- tools/lldb-server/CMakeLists.txt
+++ tools/lldb-server/CMakeLists.txt
@@ -42,6 +42,11 @@
   list(APPEND LLDB_PLUGINS lldbPluginObjectFileELF)
 endif()
 
+if(LLDB_USE_ENTITLEMENTS AND NOT IOS)
+  # Same entitlements file as used for debugserver
+  set(entitlements ${LLDB_SOURCE_DIR}/resources/debugserver-macosx-entitlements.plist)
+endif()
+
 add_lldb_tool(lldb-server INCLUDE_IN_SUITE
 Acceptor.cpp
 lldb-gdbserver.cpp
@@ -61,6 +66,9 @@
 
 LINK_COMPONENTS
   Support
+
+ENTITLEMENTS
+  ${entitlements}
 )
 
 target_link_libraries(lldb-server PRIVATE ${LLDB_SYSTEM_LIBS})
Index: cmake/modules/AddLLDB.cmake
===
--- cmake/modules/AddLLDB.cmake
+++ cmake/modules/AddLLDB.cmake
@@ -100,13 +100,13 @@
 function(add_lldb_executable name)
   cmake_parse_arguments(ARG
 "INCLUDE_IN_SUITE;GENERATE_INSTALL"
-""
+"ENTITLEMENTS"
 "LINK_LIBS;LINK_COMPONENTS"
 ${ARGN}
 )
 
   list(APPEND LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
-  add_llvm_executable(${name} ${ARG_UNPARSED_ARGUMENTS})
+  add_llvm_executable(${name} ${ARG_UNPARSED_ARGUMENTS} ENTITLEMENTS ${ARG_ENTITLEMENTS})
 
   target_link_libraries(${name} PRIVATE ${ARG_LINK_LIBS})
   set_target_properties(${name} PROPERTIES
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -11,6 +11,12 @@
 include(LLDBConfig)
 include(AddLLDB)
 
+option(LLDB_USE_ENTITLEMENTS "When codesigning, use entitlements if available" ON)
+if(LLDB_CODESIGN_IDENTITY)
+  # In the future we may use LLVM_CODESIGNING_IDENTITY directly.
+  set(LLVM_CODESIGNING_IDENTITY ${LLDB_CODESIGN_IDENTITY})
+endif()
+
 # Define the LLDB_CONFIGURATION_xxx matching the build type
 if( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   add_definitions( -DLLDB_CONFIGURATION_DEBUG )
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz updated this revision to Diff 173851.
sgraenitz added a comment.

Remove FORCE parameter from llvm_codesign and instead add global option 
LLVM_CODESIGNING_FORCE


Repository:
  rL LLVM

https://reviews.llvm.org/D54443

Files:
  CMakeLists.txt
  cmake/modules/AddLLVM.cmake


Index: cmake/modules/AddLLVM.cmake
===
--- cmake/modules/AddLLVM.cmake
+++ cmake/modules/AddLLVM.cmake
@@ -580,7 +580,7 @@
 
   if(ARG_SHARED OR ARG_MODULE)
 llvm_externalize_debuginfo(${name})
-llvm_codesign(${name})
+llvm_codesign(TARGET ${name})
   endif()
 endfunction()
 
@@ -708,7 +708,12 @@
 
 
 macro(add_llvm_executable name)
-  cmake_parse_arguments(ARG 
"DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH" "" 
"DEPENDS" ${ARGN})
+  cmake_parse_arguments(ARG
+
"DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH"
+"ENTITLEMENTS"
+"DEPENDS"
+${ARGN})
+
   llvm_process_sources( ALL_FILES ${ARG_UNPARSED_ARGUMENTS} )
 
   list(APPEND LLVM_COMMON_DEPENDS ${ARG_DEPENDS})
@@ -787,7 +792,7 @@
 target_link_libraries(${name} PRIVATE ${LLVM_PTHREAD_LIB})
   endif()
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
@@ -1626,7 +1631,16 @@
   endif()
 endfunction()
 
-function(llvm_codesign name)
+# Usage: llvm_codesign(TARGET name [ENTITLEMENTS file])
+#
+# Code-sign the given TARGET with the global LLVM_CODESIGNING_IDENTITY or skip
+# if undefined. Customize capabilities by passing a file path to ENTITLEMENTS.
+# Use LLVM_CODESIGNING_FORCE to avoid an error when replacing existing
+# signatures in failsafe/debugging situations.
+#
+function(llvm_codesign)
+  cmake_parse_arguments(ARG "" "TARGET;ENTITLEMENTS" "" ${ARGN})
+
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
   endif()
@@ -1642,12 +1656,20 @@
 OUTPUT_VARIABLE CMAKE_CODESIGN_ALLOCATE
   )
 endif()
+if(LLVM_CODESIGNING_FORCE)
+  set(PASS_FORCE --force)
+endif()
+if(DEFINED ARG_ENTITLEMENTS)
+  set(PASS_ENTITLEMENTS --entitlements ${ARG_ENTITLEMENTS})
+endif()
+
 add_custom_command(
-  TARGET ${name} POST_BUILD
+  TARGET ${ARG_TARGET} POST_BUILD
   COMMAND ${CMAKE_COMMAND} -E
   env CODESIGN_ALLOCATE=${CMAKE_CODESIGN_ALLOCATE}
   ${CMAKE_CODESIGN} -s ${LLVM_CODESIGNING_IDENTITY}
-  $
+  ${PASS_ENTITLEMENTS} ${PASS_FORCE}
+  $
 )
   endif()
 endfunction()
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -399,8 +399,11 @@
 option(LLVM_EXTERNALIZE_DEBUGINFO
   "Generate dSYM files and strip executables and libraries (Darwin Only)" OFF)
 
-option(LLVM_CODESIGNING_IDENTITY
-  "Sign executables and dylibs with the given identity (Darwin Only)" OFF)
+set(LLVM_CODESIGNING_IDENTITY "" CACHE STRING
+  "Sign executables and dylibs with the given identity or skip if empty 
(Darwin Only)")
+
+option(LLVM_CODESIGNING_FORCE
+  "Suppress errors when replacing existing signatures (Darwin Only)" OFF)
 
 # If enabled, verify we are on a platform that supports oprofile.
 if( LLVM_USE_OPROFILE )


Index: cmake/modules/AddLLVM.cmake
===
--- cmake/modules/AddLLVM.cmake
+++ cmake/modules/AddLLVM.cmake
@@ -580,7 +580,7 @@
 
   if(ARG_SHARED OR ARG_MODULE)
 llvm_externalize_debuginfo(${name})
-llvm_codesign(${name})
+llvm_codesign(TARGET ${name})
   endif()
 endfunction()
 
@@ -708,7 +708,12 @@
 
 
 macro(add_llvm_executable name)
-  cmake_parse_arguments(ARG "DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH" "" "DEPENDS" ${ARGN})
+  cmake_parse_arguments(ARG
+"DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH"
+"ENTITLEMENTS"
+"DEPENDS"
+${ARGN})
+
   llvm_process_sources( ALL_FILES ${ARG_UNPARSED_ARGUMENTS} )
 
   list(APPEND LLVM_COMMON_DEPENDS ${ARG_DEPENDS})
@@ -787,7 +792,7 @@
 target_link_libraries(${name} PRIVATE ${LLVM_PTHREAD_LIB})
   endif()
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS})
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
@@ -1626,7 +1631,16 @@
   endif()
 endfunction()
 
-function(llvm_codesign name)
+# Usage: llvm_codesign(TARGET name [ENTITLEMENTS file])
+#
+# Code-sign the given TARGET with the global LLVM_CODESIGNING_IDENTITY or skip
+# if undefined. Customize capabilities by passing a file path to ENTITLEMENTS.
+# Use LLVM_CODESIGNING_FORCE to avoid an error when replacing existing
+# signatures in failsafe/debugging situations.
+#
+function(llvm_codesign)
+  cmake_parse_arguments(ARG "" "TARGET;ENTITLEMENTS" "" ${ARGN})
+
   if(NOT LLVM_CODESIGNING_IDENTITY)
 return()
   endi

[Lldb-commits] [PATCH] D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.



Comment at: CMakeLists.txt:403
+set(LLVM_CODESIGNING_IDENTITY "" CACHE STRING
+  "Sign executables and dylibs with the given identity or skip if empty 
(Darwin Only)")
+

Identity should be a string right?



Comment at: cmake/modules/AddLLVM.cmake:795
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS} FORCE)
 endmacro(add_llvm_executable name)

sgraenitz wrote:
> beanz wrote:
> > sgraenitz wrote:
> > > Would we want to pass `FORCE` to `add_llvm_executable` conditionally?
> > I'm trying to think about the situations in which we need the `FORCE` 
> > option. Since this is connecting as a post-build event it shouldn't be 
> > running unless the target re-generates the output, so I'm not sure I 
> > understand why we ever need it.
> > 
> > I did the git blame walk back to when the code was initially added in 
> > `49dd98a03a`, and there is no explanation. I suspect debugserver doesn't 
> > actually need the `--force` option because the author of the initial patch 
> > probably hit errors when re-signing the pre-built binary in his build 
> > directory.
> > 
> > Thoughts?
> I think you are right, it shouldn't be necessary. In practice, though, I can 
> imagine situations when we want to make sure this won't fail in any case.
> 
> The options are: remove entirely (most correct) OR allow enable per target 
> (most flexible) OR allow enable globally.
> 
> What about the last one? I could add `LLVM_CODESIGNING_FORCE` which is OFF by 
> default. In failsafe/debugging situations it could be turned ON globally. I 
> could remove the `FORCE` parameter here and check the flag in `llvm_codesign` 
> (similar to `LLVM_CODESIGNING_IDENTITY`).
Patch updated


Repository:
  rL LLVM

https://reviews.llvm.org/D54443



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz created this revision.
sgraenitz added reviewers: xiaobai, beanz, vsk, JDevlieghere.
Herald added a subscriber: mgorny.

Use llvm_codesign to sign debugserver with entitlements. Make individual cases 
more explicit:

- default: build debugserver and sign with entitlements
- LLDB_NO_DEBUGSERVER: no debugserver in binaries, no path provided for tests
- LLVM_CODESIGNING_IDENTITY empty == LLDB_USE_SYSTEM_DEBUGSERVER:
  - on Darwin: copy system binary; skip debugserver tests, but provide path for 
other tests
  - on other platforms: same as LLDB_NO_DEBUGSERVER


https://reviews.llvm.org/D54476

Files:
  test/CMakeLists.txt
  tools/debugserver/CMakeLists.txt
  tools/debugserver/source/CMakeLists.txt
  unittests/tools/CMakeLists.txt

Index: unittests/tools/CMakeLists.txt
===
--- unittests/tools/CMakeLists.txt
+++ unittests/tools/CMakeLists.txt
@@ -1,5 +1,5 @@
 if(CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|Linux|NetBSD")
-  if ((CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_DEBUGSERVER) OR (NOT CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_LLDB_SERVER_BUILD))
+  if ((CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_TEST_DEBUGSERVER) OR (NOT CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_LLDB_SERVER_BUILD))
 # These tests are meant to test lldb-server/debugserver in isolation, and
 # don't provide any value if run against a server copied from somewhere.
   else()
Index: tools/debugserver/source/CMakeLists.txt
===
--- tools/debugserver/source/CMakeLists.txt
+++ tools/debugserver/source/CMakeLists.txt
@@ -94,32 +94,62 @@
 
 add_library(lldbDebugserverCommon ${lldbDebugserverCommonSources})
 
+# TODO: The LLDB target in Xcode has a build phase that explicitly deletes the
+# executable from the framework. IIUC the liblldb target could simply skip its
+# copy step to achieve the same.
+option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't try to codesign it" OFF)
+option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. Use the system's debugserver instead (Darwin only)." OFF)
+
+# Remember where our debugserver lives and whether or not we have to test it.
+set(DEBUGSERVER_PATH "" CACHE FILEPATH "Path to debugserver")
+set(SKIP_TEST_DEBUGSERVER OFF CACHE BOOL "Building the in-tree debugserver was skipped")
+
+if(LLDB_NO_DEBUGSERVER)
+  # No debugserver at all.
+  set(SKIP_TEST_DEBUGSERVER ON CACHE BOOL "" FORCE)
+elseif(LLDB_USE_SYSTEM_DEBUGSERVER OR NOT LLVM_CODESIGNING_IDENTITY)
+  # Use system debugserver (Darwin only).
+  set(SKIP_TEST_DEBUGSERVER ON CACHE BOOL "" FORCE)
+
+  if(CMAKE_HOST_APPLE)
+execute_process(
+  COMMAND xcode-select -p
+  OUTPUT_VARIABLE xcode_dev_dir)
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+
+if(EXISTS "${xcode_dev_dir}/../SharedFrameworks/LLDB.framework/")
+  set(lldb_framework_dir "${xcode_dev_dir}/../SharedFrameworks")
+elseif(EXISTS "${xcode_dev_dir}/Library/PrivateFrameworks/LLDB.framework/")
+  set(lldb_framework_dir "${xcode_dev_dir}/Library/PrivateFrameworks")
+else()
+  message(SEND_ERROR "Cannot find debugserver on system.")
+endif()
 
-set(LLDB_CODESIGN_IDENTITY "lldb_codesign"
-  CACHE STRING "Identity used for code signing. Set to empty string to skip the signing step.")
-
-if(NOT LLDB_CODESIGN_IDENTITY STREQUAL "")
-  set(DEBUGSERVER_PATH ${LLVM_RUNTIME_OUTPUT_INTDIR}/debugserver${CMAKE_EXECUTABLE_SUFFIX} CACHE PATH "Path to debugserver.")
-  set(SKIP_DEBUGSERVER OFF CACHE BOOL "Skip building the in-tree debug server")
-else()
-  execute_process(
-COMMAND xcode-select -p
-OUTPUT_VARIABLE XCODE_DEV_DIR)
-  string(STRIP ${XCODE_DEV_DIR} XCODE_DEV_DIR)
-  if(EXISTS "${XCODE_DEV_DIR}/../SharedFrameworks/LLDB.framework/")
-set(DEBUGSERVER_PATH
-  "${XCODE_DEV_DIR}/../SharedFrameworks/LLDB.framework/Resources/debugserver" CACHE PATH "Path to debugserver.")
-  elseif(EXISTS "${XCODE_DEV_DIR}/Library/PrivateFrameworks/LLDB.framework/")
-set(DEBUGSERVER_PATH
-  "${XCODE_DEV_DIR}/Library/PrivateFrameworks/LLDB.framework/Resources/debugserver" CACHE PATH "Path to debugserver.")
-  else()
-message(SEND_ERROR "Cannot find debugserver on system.")
+# TODO: Following the old behavior, DEBUGSERVER_PATH still points to the
+# original system binary, even if we copy it over. Keep this?
+set(DEBUGSERVER_PATH "${lldb_framework_dir}/LLDB.framework/Resources/debugserver" CACHE FILEPATH "" FORCE)
+
+# If we haven't built a signed debugserver. If possible, copy the one from
+# the system to make the built debugger functional on Darwin.
+#
+# TODO: IIUC the following condition would be the exact equivalent for the old
+#   behaviour, but it doesn't hurt to do it in both cases, right?
+#
+#   if(NOT LLVM_CODESIGNING_IDENTITY AND CMAKE_HOST_APPLE)
+#
+add_custom_target(debugserver
+  COMMAND ${CMAKE_COM

[Lldb-commits] [PATCH] D54352: [CMake] Explicit lldb_codesign function with application in debugserver and lldb-server

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz abandoned this revision.
sgraenitz added a comment.

Finally, debugserver with llvm_codesign: https://reviews.llvm.org/D54476


https://reviews.llvm.org/D54352



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54452: [NativePDB] Add support for handling S_CONSTANT records

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:329
 
+std::pair GetIntegralTypeInfo(TypeIndex ti, TpiStream &tpi) {
+  if (ti.isSimple()) {

lemo wrote:
> Just a suggestion: I'm not a big fan of returning std::pair<>. I'd use a 
> simple struct instead.
I have to concur on this, although the use of `std::tie` helps alleviate the 
pain i.e. no `second` and `first` it does feel more verbose on the calling side 
to declare two variables and then do the tie and it relies on the calling to 
choose meaningful names which can't be relied on if the use spreads and people 
are not as careful.


https://reviews.llvm.org/D54452



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: tools/debugserver/source/CMakeLists.txt:101
+option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't 
try to codesign it" OFF)
+option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. 
Use the system's debugserver instead (Darwin only)." OFF)
+

Should we emit and error if these variables are in an inconsistent state, e.g. 
when both are set? 



Comment at: tools/debugserver/source/CMakeLists.txt:118
+  OUTPUT_VARIABLE xcode_dev_dir)
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+

Why did you make this variable name lowercase? Does that have any semantic 
meaning?


https://reviews.llvm.org/D54476



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r346775 - Since ABI's now hold a process WP, they should be handed

[Jim Ingham via lldb-commits]

Author: jingham
Date: Tue Nov 13 10:18:32 2018
New Revision: 346775

URL: http://llvm.org/viewvc/llvm-project?rev=346775&view=rev
Log:
Since ABI's now hold a process WP, they should be handed
out one per process rather than keeping a single global instance.

Differential Revision: https://reviews.llvm.org/D54460

Modified:
lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
lldb/trunk/source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
lldb/trunk/source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Modified: lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp?rev=346775&r1=346774&r2=346775&view=diff
==
--- lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp Tue Nov 13 
10:18:32 2018
@@ -1323,16 +1323,13 @@ size_t ABIMacOSX_arm::GetRedZoneSize() c
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
 if ((arch_type == llvm::Triple::arm) ||
 (arch_type == llvm::Triple::thumb)) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABIMacOSX_arm(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABIMacOSX_arm(process_sp));
 }
   }
 

Modified: lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp?rev=346775&r1=346774&r2=346775&view=diff
==
--- lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp Tue Nov 13 
10:18:32 2018
@@ -1664,15 +1664,12 @@ size_t ABIMacOSX_arm64::GetRedZoneSize()
 
 ABISP
 ABIMacOSX_arm64::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
 if (arch_type == llvm::Triple::aarch64) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABIMacOSX_arm64(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABIMacOSX_arm64(process_sp));
 }
   }
 

Modified: lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp?rev=346775&r1=346774&r2=346775&view=diff
==
--- lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp Tue Nov 13 
10:18:32 2018
@@ -710,13 +710,10 @@ size_t ABIMacOSX_i386::GetRedZoneSize()
 
 ABISP
 ABIMacOSX_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec 
&arch) {
-  static ABISP g_abi_sp;
   if ((arch.GetTriple().getArch() == llvm::Triple::x86) &&
   (arch.GetTriple().isMacOSX() || arch.GetTriple().isiOS() ||
arch.GetTriple().isWatchOS())) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABIMacOSX_i386(process_sp));
-return g_abi_sp;
+return ABISP(new ABIMacOSX_i386(process_sp));
   }
   return ABISP();
 }

Modified: lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp?rev=346775&r1=346774&r2=346775&view=diff
==
--- lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp (original)
+++ lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp Tue Nov 13 10:18:32 
2018
@@ -1324,16 +1324,13 @@ size_t ABISysV_arm::GetRedZoneSize() con
 
 ABISP
 ABISysV_arm::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346775: Since ABI's now hold a process WP, they should 
be handed (authored by jingham, committed by ).
Herald added subscribers: llvm-commits, jrtc27.

Changed prior to commit:
  https://reviews.llvm.org/D54460?vs=173802&id=173877#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54460

Files:
  lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  lldb/trunk/source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  lldb/trunk/source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
  lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Index: lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
@@ -199,12 +199,9 @@
 
 ABISP
 ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
 if (arch.GetTriple().getArch() == llvm::Triple::x86) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_i386(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_i386(process_sp));
 }
   }
   return ABISP();
Index: lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
@@ -1016,11 +1016,8 @@
 
 ABISP
 ABISysV_hexagon::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::hexagon) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_hexagon(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_hexagon(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
@@ -1667,15 +1667,12 @@
 
 ABISP
 ABISysV_arm64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
 if (arch_type == llvm::Triple::aarch64) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_arm64(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_arm64(process_sp));
 }
   }
 
Index: lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) ||
   (arch_type == llvm::Triple::mipsel)) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_mips(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_mips(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
===
--- lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
+++ lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
@@ -202,11 +202,8 @@
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_s390x(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
Index: lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
===
--- lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
+++ lldb/trunk/source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
@@ -1323,16 +1323,13 @@
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, 

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB346775: Since ABI's now hold a process WP, they 
should be handed (authored by jingham, committed by ).

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460

Files:
  source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
  source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
  source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
  source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
  source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
  source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
  source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
  source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
  source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp
  source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp
  source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
  source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

Index: source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
===
--- source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
+++ source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
@@ -199,12 +199,9 @@
 
 ABISP
 ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
 if (arch.GetTriple().getArch() == llvm::Triple::x86) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_i386(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_i386(process_sp));
 }
   }
   return ABISP();
Index: source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
===
--- source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
+++ source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp
@@ -1016,11 +1016,8 @@
 
 ABISP
 ABISysV_hexagon::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::hexagon) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_hexagon(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_hexagon(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
===
--- source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
+++ source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
@@ -1667,15 +1667,12 @@
 
 ABISP
 ABISysV_arm64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
 if (arch_type == llvm::Triple::aarch64) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABISysV_arm64(process_sp));
-  return g_abi_sp;
+  return ABISP(new ABISysV_arm64(process_sp));
 }
   }
 
Index: source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
===
--- source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
+++ source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp
@@ -556,13 +556,10 @@
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) ||
   (arch_type == llvm::Triple::mipsel)) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_mips(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_mips(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
===
--- source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
+++ source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
@@ -202,11 +202,8 @@
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-if (!g_abi_sp)
-  g_abi_sp.reset(new ABISysV_s390x(process_sp));
-return g_abi_sp;
+return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
Index: source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
===
--- source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
+++ source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp
@@ -1323,16 +1323,13 @@
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
 if ((arch_type == llvm::Triple::arm) ||
 (arch_type == llvm::Triple::thumb)) {
-  if (!g_abi_sp)
-g_abi_sp.reset(new ABIMacOSX_arm(process_sp));
-  return g_abi_sp;
+  return A

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.



Comment at: tools/debugserver/source/CMakeLists.txt:128-133
+# TODO: Following the old behavior, DEBUGSERVER_PATH still points to the
+# original system binary, even if we copy it over. Keep this?
+set(DEBUGSERVER_PATH 
"${lldb_framework_dir}/LLDB.framework/Resources/debugserver" CACHE FILEPATH "" 
FORCE)
+
+# If we haven't built a signed debugserver. If possible, copy the one from
+# the system to make the built debugger functional on Darwin.

I'm pretty sure that if you do not copy debugserver it will still find the one 
from your `${lldb_framework_dir}` when you run lldb anyway. Do you know why we 
copy it over?


https://reviews.llvm.org/D54476



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r346780 - Add GDB remote packet reproducer.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Tue Nov 13 11:18:16 2018
New Revision: 346780

URL: http://llvm.org/viewvc/llvm-project?rev=346780&view=rev
Log:
Add GDB remote packet reproducer.

Added:
lldb/trunk/include/lldb/Utility/Reproducer.h
lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/

lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/

lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/Makefile

lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/TestGdbRemoteReproducer.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/reproducer/gdb-remote/main.c
lldb/trunk/source/Commands/CommandObjectReproducer.cpp
lldb/trunk/source/Commands/CommandObjectReproducer.h

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationHistory.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationHistory.h

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.h
lldb/trunk/source/Utility/Reproducer.cpp
Modified:
lldb/trunk/include/lldb/API/SBDebugger.h
lldb/trunk/include/lldb/Core/Debugger.h
lldb/trunk/include/lldb/Host/HostInfoBase.h
lldb/trunk/scripts/interface/SBDebugger.i
lldb/trunk/source/API/SBDebugger.cpp
lldb/trunk/source/Commands/CMakeLists.txt
lldb/trunk/source/Core/Debugger.cpp
lldb/trunk/source/Host/common/HostInfoBase.cpp
lldb/trunk/source/Interpreter/CommandInterpreter.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/CMakeLists.txt
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/trunk/source/Utility/CMakeLists.txt
lldb/trunk/tools/driver/Driver.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

Modified: lldb/trunk/include/lldb/API/SBDebugger.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBDebugger.h?rev=346780&r1=346779&r2=346780&view=diff
==
--- lldb/trunk/include/lldb/API/SBDebugger.h (original)
+++ lldb/trunk/include/lldb/API/SBDebugger.h Tue Nov 13 11:18:16 2018
@@ -109,7 +109,7 @@ public:
  const char *archname);
 
   lldb::SBTarget CreateTarget(const char *filename);
-  
+
   lldb::SBTarget GetDummyTarget();
 
   // Return true if target is deleted from the target list of the debugger.
@@ -226,6 +226,10 @@ public:
 
   void SetPrompt(const char *prompt);
 
+  const char *GetReproducerPath() const;
+
+  void SetReproducerPath(const char *reproducer);
+
   lldb::ScriptLanguage GetScriptLanguage() const;
 
   void SetScriptLanguage(lldb::ScriptLanguage script_lang);

Modified: lldb/trunk/include/lldb/Core/Debugger.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Debugger.h?rev=346780&r1=346779&r2=346780&view=diff
==
--- lldb/trunk/include/lldb/Core/Debugger.h (original)
+++ lldb/trunk/include/lldb/Core/Debugger.h Tue Nov 13 11:18:16 2018
@@ -261,6 +261,11 @@ public:
   void SetPrompt(llvm::StringRef p);
   void SetPrompt(const char *) = delete;
 
+  llvm::StringRef GetReproducerPath() const;
+
+  void SetReproducerPath(llvm::StringRef p);
+  void SetReproducerPath(const char *) = delete;
+
   bool GetUseExternalEditor() const;
 
   bool SetUseExternalEditor(bool use_external_editor_p);

Modified: lldb/trunk/include/lldb/Host/HostInfoBase.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/HostInfoBase.h?rev=346780&r1=346779&r2=346780&view=diff
==
--- lldb/trunk/include/lldb/Host/HostInfoBase.h (original)
+++ lldb/trunk/include/lldb/Host/HostInfoBase.h Tue Nov 13 11:18:16 2018
@@ -93,6 +93,12 @@ public:
   /// FileSpec is filled in.
   static FileSpec GetGlobalTempDir();
 
+  /// Returns the reproducer temporary directory. This directory will **not**
+  /// be automatically cleaned up when this process exits, but might be removed
+  /// by the reproducer generator. Only the directory member of the FileSpec is
+  /// filled in.
+  static FileSpec GetReproducerTempDir();
+
   //---
   /// If the triple does not specify 

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Okay, I think I'll keep what Jason did and just make them per-process.  As Greg 
said, they are very cheap to make, since they just take a Process SP and get 
the weak pointer from it.  None of them have any other state.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r346783 - [Cocoa] Implement formatter for the new NSDate representation.

[Davide Italiano via lldb-commits]

Author: davide
Date: Tue Nov 13 11:43:43 2018
New Revision: 346783

URL: http://llvm.org/viewvc/llvm-project?rev=346783&view=rev
Log:
[Cocoa] Implement formatter for the new NSDate representation.



Modified:
lldb/trunk/source/Plugins/Language/ObjC/Cocoa.cpp

Modified: lldb/trunk/source/Plugins/Language/ObjC/Cocoa.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Language/ObjC/Cocoa.cpp?rev=346783&r1=346782&r2=346783&view=diff
==
--- lldb/trunk/source/Plugins/Language/ObjC/Cocoa.cpp (original)
+++ lldb/trunk/source/Plugins/Language/ObjC/Cocoa.cpp Tue Nov 13 11:43:43 2018
@@ -742,6 +742,60 @@ bool lldb_private::formatters::NSURLSumm
   return false;
 }
 
+/// Bias value for tagged pointer exponents.
+/// Recommended values:
+/// 0x3e3: encodes all dates between distantPast and distantFuture
+///   except for the range within about 1e-28 second of the reference date.
+/// 0x3ef: encodes all dates for a few million years beyond distantPast and
+///   distantFuture, except within about 1e-25 second of the reference date.
+const int TAGGED_DATE_EXPONENT_BIAS = 0x3ef;
+
+typedef union {
+  struct {
+uint64_t fraction:52;  // unsigned
+uint64_t exponent:11;  // signed
+uint64_t sign:1;
+  };
+  uint64_t i;
+  double d;
+} DoubleBits;
+typedef union {
+  struct {
+uint64_t fraction:52;  // unsigned
+uint64_t exponent:7;   // signed
+uint64_t sign:1;
+uint64_t unused:4;  // placeholder for pointer tag bits
+  };
+  uint64_t i;
+} TaggedDoubleBits;
+
+static uint64_t decodeExponent(uint64_t exp) {
+  int64_t exp7 = exp;
+  // Tagged exponent field is 7-bit signed. Sign-extend the value to 64 bits
+  // before performing arithmetic.
+  int64_t exp11 = ((exp7 << 57) >> 57) + TAGGED_DATE_EXPONENT_BIAS;
+  return exp11;
+}
+
+static uint64_t decodeTaggedTimeInterval(uint64_t encodedTimeInterval) {
+  if (encodedTimeInterval == 0)
+return 0.0;
+  if (encodedTimeInterval == std::numeric_limits::max())
+return -0.0;
+
+  TaggedDoubleBits encodedBits = { .i = encodedTimeInterval };
+  DoubleBits decodedBits;
+
+  // Sign and fraction are represented exactly.
+  // Exponent is encoded.
+  assert(encodedBits.unused == 0);
+  decodedBits.sign = encodedBits.sign;
+  decodedBits.fraction = encodedBits.fraction;
+  decodedBits.exponent = decodeExponent(encodedBits.exponent);
+
+  return decodedBits.d;
+}
+
 bool lldb_private::formatters::NSDateSummaryProvider(
 ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   ProcessSP process_sp = valobj.GetProcessSP();
@@ -781,9 +835,9 @@ bool lldb_private::formatters::NSDateSum
   if (class_name.IsEmpty())
 return false;
 
+  uint64_t info_bits = 0, value_bits = 0;
   if ((class_name == g_NSDate) || (class_name == g___NSDate) ||
   (class_name == g___NSTaggedDate)) {
-uint64_t info_bits = 0, value_bits = 0;
 if (descriptor->GetTaggedPointerInfo(&info_bits, &value_bits)) {
   date_value_bits = ((value_bits << 8) | (info_bits << 4));
   memcpy(&date_value, &date_value_bits, sizeof(date_value_bits));
@@ -813,6 +867,14 @@ bool lldb_private::formatters::NSDateSum
 stream.Printf("0001-12-30 00:00:00 +");
 return true;
   }
+
+  // Accomodate for the __NSTaggedDate format introduced in Foundation 1600.
+  if (class_name == g___NSTaggedDate) {
+auto *runtime = 
llvm::dyn_cast_or_null(process_sp->GetObjCLanguageRuntime());
+if (runtime && runtime->GetFoundationVersion() >= 1600)
+  date_value = decodeTaggedTimeInterval(value_bits << 4);
+  }
+
   // this snippet of code assumes that time_t == seconds since Jan-1-1970 this
   // is generally true and POSIXly happy, but might break if a library vendor
   // decides to get creative


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

As for testing, Jason was careful to use a WP, so nothing crashes.  The 
process's SP doesn't resolve and then you get the default setting of any 
process specific setting that uses the ABI.  Since there aren't any of those 
yet - this currently has no observable consequences.  But there will be soon, 
and we can use that to test that we are getting this process's settings easily 
once that gets checked in.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.



Comment at: tools/debugserver/source/CMakeLists.txt:101
+option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't 
try to codesign it" OFF)
+option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. 
Use the system's debugserver instead (Darwin only)." OFF)
+

JDevlieghere wrote:
> Should we emit and error if these variables are in an inconsistent state, 
> e.g. when both are set? 
Yes, could do that.



Comment at: tools/debugserver/source/CMakeLists.txt:118
+  OUTPUT_VARIABLE xcode_dev_dir)
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+

JDevlieghere wrote:
> Why did you make this variable name lowercase? Does that have any semantic 
> meaning?
LLVM tends to use lowercase names for local/non-cached variables, e.g. 
`update_src_props` in:
https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddLLVM.cmake

I didn't find any guidelines on conventions for CMake, but I thought it's a 
good idea. It's a little surprising to mix in such changes to patches that also 
have semantic changes. Maybe I should continue to use uppercase names for now 
and add a subsequent patch (with a proper title) that does the change for the 
whole file. What do you think?



Comment at: tools/debugserver/source/CMakeLists.txt:128-133
+# TODO: Following the old behavior, DEBUGSERVER_PATH still points to the
+# original system binary, even if we copy it over. Keep this?
+set(DEBUGSERVER_PATH 
"${lldb_framework_dir}/LLDB.framework/Resources/debugserver" CACHE FILEPATH "" 
FORCE)
+
+# If we haven't built a signed debugserver. If possible, copy the one from
+# the system to make the built debugger functional on Darwin.

xiaobai wrote:
> I'm pretty sure that if you do not copy debugserver it will still find the 
> one from your `${lldb_framework_dir}` when you run lldb anyway. Do you know 
> why we copy it over?
Yes this is what I mean with "old behaviour" here. `DEBUGSERVER_PATH` still 
points to the original system binary and gets passed to tests, e.g. in 
`test/CMakeLists.txt` above.


https://reviews.llvm.org/D54476



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r346787 - [NativePDB] Add support for S_CONSTANT records.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Tue Nov 13 12:07:57 2018
New Revision: 346787

URL: http://llvm.org/viewvc/llvm-project?rev=346787&view=rev
Log:
[NativePDB] Add support for S_CONSTANT records.

clang-cl does not emit these, but MSVC does, so we need to be able to
handle them.

Because clang-cl does not generate them, it was a bit hard to write a
test. So what I had to do was get an PDB file with some S_CONSTANT
records in using cl and link, dump it using llvm-pdbutil dump -globals
-sym-data to get the bytes of the records, generate the same object file
using clang-cl but with -S to emit an assembly file, and replace all the
S_LDATA32 records with the bytes of the S_CONSTANT records. This way, we
can compile the file using llvm-mc and link it with lld-link.

Differential Revision: https://reviews.llvm.org/D54452

Added:
lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit
lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s
lldb/trunk/lit/SymbolFile/NativePDB/s_constant.cpp
Modified:
lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Added: lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit?rev=346787&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit Tue Nov 13 
12:07:57 2018
@@ -0,0 +1,25 @@
+target variable GlobalLUEA
+target variable GlobalLUEB
+target variable GlobalLUEC
+
+target variable GlobalLSEA
+target variable GlobalLSEB
+target variable GlobalLSEC
+
+target variable GlobalUEA
+target variable GlobalUEB
+target variable GlobalUEC
+
+target variable GlobalSEA
+target variable GlobalSEB
+target variable GlobalSEC
+
+target variable GlobalSUEA
+target variable GlobalSUEB
+target variable GlobalSUEC
+
+target variable GlobalSSEA
+target variable GlobalSSEB
+target variable GlobalSSEC
+
+quit

Added: lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s?rev=346787&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s Tue Nov 13 12:07:57 
2018
@@ -0,0 +1,971 @@
+   .text
+   .def @feat.00;
+   .scl3;
+   .type   0;
+   .endef
+   .globl  @feat.00
+.set @feat.00, 0
+   .intel_syntax noprefix
+   .def main;
+   .scl2;
+   .type   32;
+   .endef
+   .globl  main# -- Begin function main
+   .p2align4, 0x90
+main:   # @main
+.Lfunc_begin0:
+   .cv_func_id 0
+   .cv_file1 
"D:\\src\\llvm-mono\\lldb\\lit\\SymbolFile\\NativePDB\\s_constant.cpp" 
"7F1DA683A9B72A1360C1FDEDD7550E06" 1
+   .cv_loc 0 1 79 0# 
D:\src\llvm-mono\lldb\lit\SymbolFile\NativePDB\s_constant.cpp:79:0
+.seh_proc main
+# %bb.0:# %entry
+   sub rsp, 24
+   .seh_stackalloc 24
+   .seh_endprologue
+   xor eax, eax
+   mov dword ptr [rsp + 20], 0
+   mov qword ptr [rsp + 8], rdx
+   mov dword ptr [rsp + 4], ecx
+.Ltmp0:
+   .cv_loc 0 1 80 0# 
D:\src\llvm-mono\lldb\lit\SymbolFile\NativePDB\s_constant.cpp:80:0
+   add rsp, 24
+   ret
+.Ltmp1:
+.Lfunc_end0:
+   .seh_handlerdata
+   .text
+   .seh_endproc
+# -- End function
+   .section.rdata,"dr"
+   .p2align3   # @GlobalLUEA
+GlobalLUEA:
+   .quad   0   # 0x0
+
+   .p2align3   # @GlobalLUEB
+GlobalLUEB:
+   .quad   1000# 0x3e8
+
+   .p2align3   # @GlobalLUEC
+GlobalLUEC:
+   .quad   -16 # 0xfff0
+
+   .p2align3   # @GlobalLSEA
+GlobalLSEA:
+   .quad   0   # 0x0
+
+   .p2align3   # @GlobalLSEB
+GlobalLSEB:
+   .quad   9223372036854775000 # 0x7cd8
+
+   .p2align3   # @GlobalLSEC
+GlobalLSEC:
+   .quad   -9223372036854775000# 0x8328
+
+   .p2align2   # @GlobalUEA
+GlobalUEA:
+   .long   0   # 0x0
+
+   .p2align2   # @GlobalUEB
+GlobalUEB:
+   .long   1000# 0x3e8
+
+   .p2align2   # @GlobalUEC
+GlobalUEC:
+   .long   429400  # 0

[Lldb-commits] [lldb] r346786 - [NativePDB] Improved support for nested type reconstruction.

[Zachary Turner via lldb-commits]

Author: zturner
Date: Tue Nov 13 12:07:32 2018
New Revision: 346786

URL: http://llvm.org/viewvc/llvm-project?rev=346786&view=rev
Log:
[NativePDB] Improved support for nested type reconstruction.

In a previous patch, we pre-processed the TPI stream in order to build
the reverse mapping from nested type -> parent type so that we could
accurately reconstruct a DeclContext hierarchy.

However, there were some issues. An LF_NESTTYPE record is really just a
typedef, so although it happens to be used to indicate the name of the
nested type and referring to the global record which defines the type,
it is also used for every other kind of nested typedef. When we rebuild
the DeclContext hierarchy, we want it to be as accurate as possible,
which means that if we have something like:

  struct A {
struct B {};
using C = B;
  };

We don't want to create two CXXRecordDecls in the AST each with the
exact same definition. We just want to create one for B and then
define C as an alias to B. Previously, however, it would not be able
to distinguish between the two cases and it would treat A::B and
A::C as being two classes each with separate definitions. We address
the first half of improving the pre-processing logic so that only
actual definitions are treated this way.

Later, in a followup patch, we can handle the case of nested
typedefs since we're already going to be enumerating the field list
anyway and this patch introduces the general framework for
distinguishing between the two cases.

Differential Revision: https://reviews.llvm.org/D54357

Added:
lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit
lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp
Modified:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Added: lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit?rev=346786&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit Tue Nov 13 
12:07:32 2018
@@ -0,0 +1,12 @@
+settings set auto-one-line-summaries false
+
+target variable -T GlobalA
+target variable -T GlobalB
+target variable -T GlobalC
+target variable -T GlobalD
+target variable -T GlobalE
+target variable -T GlobalF
+target variable -T GlobalG
+target variable -T GlobalH
+
+target modules dump ast

Added: lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp?rev=346786&view=auto
==
--- lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp (added)
+++ lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp Tue Nov 13 12:07:32 
2018
@@ -0,0 +1,139 @@
+// clang-format off
+// REQUIRES: lld
+
+// Test various interesting cases for AST reconstruction.
+// RUN: clang-cl /Z7 /GS- /GR- -Xclang -fkeep-static-consts /c /Fo%t.obj -- %s
+// RUN: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- 
%t.obj
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
+// RUN: %p/Inputs/nested-types.lldbinit 2>&1 | FileCheck %s
+
+struct S {
+  struct NestedStruct {
+int A = 0;
+int B = 1;
+  };
+  int C = 2;
+  int D = 3;
+};
+struct T {
+  using NestedTypedef = int;
+  using NestedTypedef2 = S;
+
+  struct NestedStruct {
+int E = 4;
+int F = 5;
+  };
+
+  using NestedStructAlias = NestedStruct;
+  using NST = S::NestedStruct;
+
+  NestedTypedef NT = 4;
+
+  using U = struct {
+int G = 6;
+int H = 7;
+  };
+};
+
+template
+class U {
+public:
+  // See llvm.org/pr39607.  clang-cl currently doesn't emit an important debug
+  // info record for nested template instantiations, so we can't reconstruct
+  // a proper DeclContext hierarchy for these.  As such, U::V will show 
up
+  // in the global namespace.
+  template
+  struct V {
+Param I = 8;
+Param J = 9;
+
+using W = T::NestedTypedef;
+using X = U;
+  };
+
+  struct W {
+Param M = 12;
+Param N = 13;
+  };
+  Param K = 10;
+  Param L = 11;
+  using Y = V;
+  using Z = V;
+};
+
+constexpr S GlobalA;
+constexpr S::NestedStruct GlobalB;
+constexpr T GlobalC;
+constexpr T::NestedStruct GlobalD;
+constexpr T::U GlobalE;
+constexpr U GlobalF;
+constexpr U::V GlobalG;
+constexpr U::W GlobalH;
+
+
+int main(int argc, char **argv) {
+  return 0;
+}
+
+
+
+// CHECK: (lldb) target variable -T GlobalA
+// CHECK: (const S) GlobalA = {
+// CHECK:   (int) C = 2
+// CHECK:   (int) D = 3
+// CHECK: }
+// CHECK: (lldb) target variable -T GlobalB
+// CHECK: (const S::NestedStruct) GlobalB = {
+// CHECK:   (int) A = 0
+// CHECK:   (int) B = 1
+// CHECK: }
+// CHECK: (lldb) target variable -T GlobalC
+// CHECK: (const T) GlobalC = {
+// CHECK:   (int) N

[Lldb-commits] [PATCH] D54357: [NativePDB] Improved support for nested type reconstruction

[Zachary Turner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346786: [NativePDB] Improved support for nested type 
reconstruction. (authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54357?vs=173447&id=173902#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54357

Files:
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/nested-types.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  llvm/trunk/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
  llvm/trunk/lib/DebugInfo/PDB/Native/TpiStream.cpp

Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -535,6 +535,49 @@
   lldbassert(m_clang);
 }
 
+static llvm::Optional
+GetNestedTagRecord(const NestedTypeRecord &Record, const CVTagRecord &parent,
+   TpiStream &tpi) {
+  // An LF_NESTTYPE is essentially a nested typedef / using declaration, but it
+  // is also used to indicate the primary definition of a nested class.  That is
+  // to say, if you have:
+  // struct A {
+  //   struct B {};
+  //   using C = B;
+  // };
+  // Then in the debug info, this will appear as:
+  // LF_STRUCTURE `A::B` [type index = N]
+  // LF_STRUCTURE `A`
+  //   LF_NESTTYPE [name = `B`, index = N]
+  //   LF_NESTTYPE [name = `C`, index = N]
+  // In order to accurately reconstruct the decl context hierarchy, we need to
+  // know which ones are actual definitions and which ones are just aliases.
+
+  // If it's a simple type, then this is something like `using foo = int`.
+  if (Record.Type.isSimple())
+return llvm::None;
+
+  // If it's an inner definition, then treat whatever name we have here as a
+  // single component of a mangled name.  So we can inject it into the parent's
+  // mangled name to see if it matches.
+  CVTagRecord child = CVTagRecord::create(tpi.getType(Record.Type));
+  std::string qname = parent.asTag().getUniqueName();
+  if (qname.size() < 4 || child.asTag().getUniqueName().size() < 4)
+return llvm::None;
+
+  // qname[3] is the tag type identifier (struct, class, union, etc).  Since the
+  // inner tag type is not necessarily the same as the outer tag type, re-write
+  // it to match the inner tag type.
+  qname[3] = child.asTag().getUniqueName()[3];
+  std::string piece = Record.Name;
+  piece.push_back('@');
+  qname.insert(4, std::move(piece));
+  if (qname != child.asTag().UniqueName)
+return llvm::None;
+
+  return std::move(child);
+}
+
 void SymbolFileNativePDB::PreprocessTpiStream() {
   LazyRandomTypeCollection &types = m_index->tpi().typeCollection();
 
@@ -552,19 +595,27 @@
 
 struct ProcessTpiStream : public TypeVisitorCallbacks {
   ProcessTpiStream(PdbIndex &index, TypeIndex parent,
+   const CVTagRecord &parent_cvt,
llvm::DenseMap &parents)
-  : index(index), parents(parents), parent(parent) {}
+  : index(index), parents(parents), parent(parent),
+parent_cvt(parent_cvt) {}
 
   PdbIndex &index;
   llvm::DenseMap &parents;
   TypeIndex parent;
+  const CVTagRecord &parent_cvt;
 
   llvm::Error visitKnownMember(CVMemberRecord &CVR,
NestedTypeRecord &Record) override {
+llvm::Optional tag =
+GetNestedTagRecord(Record, parent_cvt, index.tpi());
+if (!tag)
+  return llvm::ErrorSuccess();
+
 parents[Record.Type] = parent;
-CVType child = index.tpi().getType(Record.Type);
-if (!IsForwardRefUdt(child))
+if (!tag->asTag().isForwardRef())
   return llvm::ErrorSuccess();
+
 llvm::Expected full_decl =
 index.tpi().findFullDeclForForwardRef(Record.Type);
 if (!full_decl) {
@@ -577,7 +628,7 @@
 };
 
 CVType field_list = m_index->tpi().getType(tag.asTag().FieldList);
-ProcessTpiStream process(*m_index, *ti, m_parent_types);
+ProcessTpiStream process(*m_index, *ti, tag, m_parent_types);
 llvm::Error error = visitMemberRecordStream(field_list.data(), process);
 if (error)
   llvm::consumeError(std::move(error));
@@ -792,6 +843,16 @@
   return {OS.getBuffer()};
 }
 
+static bool
+AnyScopesHaveTemplateParams(llvm::ArrayRef scopes) {
+  for (llvm::ms_demangle::Node *n : scopes) {
+auto *idn = static_cast(n);
+if (idn->TemplateParams)
+  return true;
+  }
+  return false;
+}
+
 std::pair
 SymbolFileNativePDB::CreateDeclInfoForType(const TagRecord &record,
TypeIndex ti) {
@@ -817,6 +878,14 @@
 if (scopes.empty())
   return {context, uname};
 
+// If there is no p

[Lldb-commits] [PATCH] D54452: [NativePDB] Add support for handling S_CONSTANT records

[Zachary Turner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346787: [NativePDB] Add support for S_CONSTANT records. 
(authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54452?vs=173775&id=173903#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54452

Files:
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/s_constant.s
  lldb/trunk/lit/SymbolFile/NativePDB/nested-types.cpp
  lldb/trunk/lit/SymbolFile/NativePDB/s_constant.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -201,6 +201,8 @@
   lldb::TypeSP CreateType(PdbSymUid type_uid);
   lldb::TypeSP CreateAndCacheType(PdbSymUid type_uid);
   lldb::VariableSP CreateGlobalVariable(PdbSymUid var_uid);
+  lldb::VariableSP CreateConstantSymbol(PdbSymUid var_uid,
+const llvm::codeview::CVSymbol &cvs);
 
   llvm::BumpPtrAllocator m_allocator;
 
Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -253,6 +253,29 @@
   }
 }
 
+static bool IsSimpleTypeSignedInteger(SimpleTypeKind kind) {
+  switch (kind) {
+  case SimpleTypeKind::Int128:
+  case SimpleTypeKind::Int64:
+  case SimpleTypeKind::Int64Quad:
+  case SimpleTypeKind::Int32:
+  case SimpleTypeKind::Int32Long:
+  case SimpleTypeKind::Int16:
+  case SimpleTypeKind::Int16Short:
+  case SimpleTypeKind::Float128:
+  case SimpleTypeKind::Float80:
+  case SimpleTypeKind::Float64:
+  case SimpleTypeKind::Float32:
+  case SimpleTypeKind::Float16:
+  case SimpleTypeKind::NarrowCharacter:
+  case SimpleTypeKind::SignedCharacter:
+  case SimpleTypeKind::SByte:
+return true;
+  default:
+return false;
+  }
+}
+
 static size_t GetTypeSizeForSimpleKind(SimpleTypeKind kind) {
   switch (kind) {
   case SimpleTypeKind::Boolean128:
@@ -303,6 +326,35 @@
   }
 }
 
+std::pair GetIntegralTypeInfo(TypeIndex ti, TpiStream &tpi) {
+  if (ti.isSimple()) {
+SimpleTypeKind stk = ti.getSimpleKind();
+return {GetTypeSizeForSimpleKind(stk), IsSimpleTypeSignedInteger(stk)};
+  }
+
+  CVType cvt = tpi.getType(ti);
+  switch (cvt.kind()) {
+  case LF_MODIFIER: {
+ModifierRecord mfr;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, mfr));
+return GetIntegralTypeInfo(mfr.ModifiedType, tpi);
+  }
+  case LF_POINTER: {
+PointerRecord pr;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, pr));
+return GetIntegralTypeInfo(pr.ReferentType, tpi);
+  }
+  case LF_ENUM: {
+EnumRecord er;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, er));
+return GetIntegralTypeInfo(er.UnderlyingType, tpi);
+  }
+  default:
+assert(false && "Type is not integral!");
+return {0, false};
+  }
+}
+
 static llvm::StringRef GetSimpleTypeName(SimpleTypeKind kind) {
   switch (kind) {
   case SimpleTypeKind::Boolean128:
@@ -557,10 +609,15 @@
   if (Record.Type.isSimple())
 return llvm::None;
 
+  CVType cvt = tpi.getType(Record.Type);
+
+  if (!IsTagRecord(cvt))
+return llvm::None;
+
   // If it's an inner definition, then treat whatever name we have here as a
   // single component of a mangled name.  So we can inject it into the parent's
   // mangled name to see if it matches.
-  CVTagRecord child = CVTagRecord::create(tpi.getType(Record.Type));
+  CVTagRecord child = CVTagRecord::create(cvt);
   std::string qname = parent.asTag().getUniqueName();
   if (qname.size() < 4 || child.asTag().getUniqueName().size() < 4)
 return llvm::None;
@@ -971,23 +1028,25 @@
 
 lldb::TypeSP SymbolFileNativePDB::CreateTagType(PdbSymUid type_uid,
 const EnumRecord &er) {
-  llvm::StringRef name = DropNameScope(er.getName());
-
-  clang::DeclContext *decl_context = m_clang->GetTranslationUnitDecl();
+  const PdbTypeSymId &tid = type_uid.asTypeSym();
+  TypeIndex ti(tid.index);
+  clang::DeclContext *decl_context = nullptr;
+  std::string uname;
+  std::tie(decl_context, uname) = CreateDeclInfoForType(er, ti);
 
   Declaration decl;
   TypeSP underlying_type = GetOrCreateType(er.UnderlyingType);
   CompilerType enum_ct = m_clang->CreateEnumerationType(
-  name.str().c_str(), decl_context, decl,
-  underlying_type->GetFullCompilerType(), er.isScoped());
+  uname.c_str(), decl_context, decl, underlying_type->Get

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: tools/debugserver/source/CMakeLists.txt:118
+  OUTPUT_VARIABLE xcode_dev_dir)
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+

sgraenitz wrote:
> JDevlieghere wrote:
> > Why did you make this variable name lowercase? Does that have any semantic 
> > meaning?
> LLVM tends to use lowercase names for local/non-cached variables, e.g. 
> `update_src_props` in:
> https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddLLVM.cmake
> 
> I didn't find any guidelines on conventions for CMake, but I thought it's a 
> good idea. It's a little surprising to mix in such changes to patches that 
> also have semantic changes. Maybe I should continue to use uppercase names 
> for now and add a subsequent patch (with a proper title) that does the change 
> for the whole file. What do you think?
Cool, I wasn't aware of the convention. I think either is fine, whatever you 
prefer. 


https://reviews.llvm.org/D54476



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I think we can close this after https://reviews.llvm.org/rL344945?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign

[Chris Bieneman via Phabricator via lldb-commits]

beanz added inline comments.



Comment at: CMakeLists.txt:403
+set(LLVM_CODESIGNING_IDENTITY "" CACHE STRING
+  "Sign executables and dylibs with the given identity or skip if empty 
(Darwin Only)")
+

sgraenitz wrote:
> Identity should be a string right?
Yep, makes sense.



Comment at: cmake/modules/AddLLVM.cmake:795
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS} FORCE)
 endmacro(add_llvm_executable name)

sgraenitz wrote:
> sgraenitz wrote:
> > beanz wrote:
> > > sgraenitz wrote:
> > > > Would we want to pass `FORCE` to `add_llvm_executable` conditionally?
> > > I'm trying to think about the situations in which we need the `FORCE` 
> > > option. Since this is connecting as a post-build event it shouldn't be 
> > > running unless the target re-generates the output, so I'm not sure I 
> > > understand why we ever need it.
> > > 
> > > I did the git blame walk back to when the code was initially added in 
> > > `49dd98a03a`, and there is no explanation. I suspect debugserver doesn't 
> > > actually need the `--force` option because the author of the initial 
> > > patch probably hit errors when re-signing the pre-built binary in his 
> > > build directory.
> > > 
> > > Thoughts?
> > I think you are right, it shouldn't be necessary. In practice, though, I 
> > can imagine situations when we want to make sure this won't fail in any 
> > case.
> > 
> > The options are: remove entirely (most correct) OR allow enable per target 
> > (most flexible) OR allow enable globally.
> > 
> > What about the last one? I could add `LLVM_CODESIGNING_FORCE` which is OFF 
> > by default. In failsafe/debugging situations it could be turned ON 
> > globally. I could remove the `FORCE` parameter here and check the flag in 
> > `llvm_codesign` (similar to `LLVM_CODESIGNING_IDENTITY`).
> Patch updated
My gut is to just remove forcing entirely, and only add it back if we actually 
need it. Short of post-build steps being incorrectly implemented in a CMake 
generator, I can't imagine a situation where it would be needed.


Repository:
  rL LLVM

https://reviews.llvm.org/D54443



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via lldb-commits]

I think it must be related to setting up the environment in which to run
clang.  In all other projects we call llvm_config.use_clang() which is in
llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path
of a clang we are trying to use, we skip this function in LLDB's lit
configuration files.  But there is also a lot of other logic in that
function, so perhaps it's some of that logic that's necessary.

On Mon, Nov 12, 2018 at 9:02 AM Aleksandr Urakov via Phabricator <
revi...@reviews.llvm.org> wrote:

> aleksandr.urakov added a comment.
>
> But all compiles without errors if I run this manually:
>
>   clang-cl -m32 /Z7 /c /GS-
> C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o
> C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj
>
>
> Repository:
>   rLLDB LLDB
>
> https://reviews.llvm.org/D54009
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

I think it must be related to setting up the environment in which to run
clang.  In all other projects we call llvm_config.use_clang() which is in
llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path
of a clang we are trying to use, we skip this function in LLDB's lit
configuration files.  But there is also a lot of other logic in that
function, so perhaps it's some of that logic that's necessary.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54009



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Add the module lock where necessary and assert that we own it.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

yep


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r346812 - Fix a bug in the parsing of the LC_BUILD_VERSION Mach-O load command.

[Adrian Prantl via lldb-commits]

Author: adrian
Date: Tue Nov 13 15:14:37 2018
New Revision: 346812

URL: http://llvm.org/viewvc/llvm-project?rev=346812&view=rev
Log:
Fix a bug in the parsing of the LC_BUILD_VERSION Mach-O load command.

LC_BUILD_VERSION records are of variable length. The original code
would use uninitialized memory when the size of a record was exactly 24.

rdar://problem/46032185

Added:
lldb/trunk/lit/Modules/lc_build_version_notools.yaml
  - copied, changed from r346787, 
lldb/trunk/lit/Modules/lc_build_version.yaml
Modified:
lldb/trunk/lit/Modules/lc_build_version.yaml
lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Modified: lldb/trunk/lit/Modules/lc_build_version.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/lc_build_version.yaml?rev=346812&r1=346811&r2=346812&view=diff
==
--- lldb/trunk/lit/Modules/lc_build_version.yaml (original)
+++ lldb/trunk/lit/Modules/lc_build_version.yaml Tue Nov 13 15:14:37 2018
@@ -1,6 +1,6 @@
 # RUN: yaml2obj %s > %t.out
 # RUN: lldb-test symbols %t.out | FileCheck %s
-# REQUIRES: darwin
+# REQUIRES: system-darwin
 # Test that the deployment target is parsed from the load commands.
 # CHECK: x86_64-apple-macosx10.14.0
 --- !mach-o

Copied: lldb/trunk/lit/Modules/lc_build_version_notools.yaml (from r346787, 
lldb/trunk/lit/Modules/lc_build_version.yaml)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/lc_build_version_notools.yaml?p2=lldb/trunk/lit/Modules/lc_build_version_notools.yaml&p1=lldb/trunk/lit/Modules/lc_build_version.yaml&r1=346787&r2=346812&rev=346812&view=diff
==
--- lldb/trunk/lit/Modules/lc_build_version.yaml (original)
+++ lldb/trunk/lit/Modules/lc_build_version_notools.yaml Tue Nov 13 15:14:37 
2018
@@ -1,6 +1,6 @@
 # RUN: yaml2obj %s > %t.out
 # RUN: lldb-test symbols %t.out | FileCheck %s
-# REQUIRES: darwin
+# REQUIRES: system-darwin
 # Test that the deployment target is parsed from the load commands.
 # CHECK: x86_64-apple-macosx10.14.0
 --- !mach-o
@@ -10,7 +10,7 @@ FileHeader:
   cpusubtype:  0x8003
   filetype:0x0002
   ncmds:   14
-  sizeofcmds:  744
+  sizeofcmds:  738
   flags:   0x00200085
   reserved:0x
 LoadCommands:
@@ -119,14 +119,11 @@ LoadCommands:
 cmdsize: 24
 uuid:8F41E140-23B9-3720-AC28-4E7AF9D159BA
   - cmd: LC_BUILD_VERSION
-cmdsize: 32
+cmdsize: 24
 platform:1
 minos:   658944
 sdk: 658944
-ntools:  1
-Tools:   
-  - tool:3
-version: 26738944
+ntools:  0
   - cmd: LC_SOURCE_VERSION
 cmdsize: 16
 version: 0

Modified: lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp?rev=346812&r1=346811&r2=346812&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp Tue Nov 13 
15:14:37 2018
@@ -5027,24 +5027,28 @@ bool ObjectFileMachO::GetArchitecture(co
 const lldb::offset_t cmd_offset = offset;
 if (data.GetU32(&offset, &load_cmd, 2) == NULL)
   break;
-
-if (load_cmd.cmd == llvm::MachO::LC_BUILD_VERSION) {
-  struct build_version_command build_version;
-  if (load_cmd.cmdsize != sizeof(build_version))
+do {
+  if (load_cmd.cmd == llvm::MachO::LC_BUILD_VERSION) {
+struct build_version_command build_version;
+if (load_cmd.cmdsize < sizeof(build_version)) {
+  // Malformed load command.
+  break;
+}
 if (data.ExtractBytes(cmd_offset, sizeof(build_version),
   data.GetByteOrder(), &build_version) == 0)
-  continue;
-  MinOS min_os(build_version.minos);
-  OSEnv os_env(build_version.platform);
-  if (os_env.os_type.empty())
-continue;
-  os << os_env.os_type << min_os.major_version << '.'
- << min_os.minor_version << '.' << min_os.patch_version;
-  triple.setOSName(os.str());
-  if (!os_env.environment.empty())
-triple.setEnvironmentName(os_env.environment);
-  return true;
-}
+  break;
+MinOS min_os(build_version.minos);
+OSEnv os_env(build_version.platform);
+if (os_env.os_type.empty())
+  break;
+os << os_env.os_type << min_os.major_version << '.'
+   << min_os.minor_version << '.' << min_os.patch_version;
+tr

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Stella Stamenova via lldb-commits]

I took a brief look and I have a question about the usage of clang (rather than 
clang-cl).

In general I would agree that we have an exact path of clang (or gcc) that we 
are trying to use and they’re specified by using %cc and %cxx in the test 
files, but there are a number of test files that simply use clang e.g.:

SymbolFile\DWARF\find-variable-dwo.cpp:3:// RUN: clang %s -g -gsplit-dwarf -c 
-emit-llvm -o - --target=x86_64-pc-linux -DONE

In this case, are we not going to pick up whatever clang happens to be in the 
path instead of one that was explicitly specified? Is this intentional?

Thanks,
-Stella

From: Zachary Turner 
Sent: Tuesday, November 13, 2018 2:46 PM
To: reviews+d54009+public+0e164460da8f1...@reviews.llvm.org
Cc: Stella Stamenova ; pa...@labath.sk; 
chris.biene...@me.com; dccitali...@gmail.com; aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; abidh@gmail.com; teempe...@gmail.com; 
ki.s...@gmail.com; mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com; 
lldb-commits@lists.llvm.org; l...@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

I think it must be related to setting up the environment in which to run clang. 
 In all other projects we call llvm_config.use_clang() which is in 
llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path of a 
clang we are trying to use, we skip this function in LLDB's lit configuration 
files.  But there is also a lot of other logic in that function, so perhaps 
it's some of that logic that's necessary.

On Mon, Nov 12, 2018 at 9:02 AM Aleksandr Urakov via Phabricator 
mailto:revi...@reviews.llvm.org>> wrote:
aleksandr.urakov added a comment.

But all compiles without errors if I run this manually:

  clang-cl -m32 /Z7 /c /GS- 
C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o 
C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54009


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r346812 - Fix a bug in the parsing of the LC_BUILD_VERSION Mach-O load command.

[Davide Italiano via lldb-commits]

Thanks!
On Tue, Nov 13, 2018 at 3:17 PM Adrian Prantl via lldb-commits
 wrote:
>
> Author: adrian
> Date: Tue Nov 13 15:14:37 2018
> New Revision: 346812
>
> URL: http://llvm.org/viewvc/llvm-project?rev=346812&view=rev
> Log:
> Fix a bug in the parsing of the LC_BUILD_VERSION Mach-O load command.
>
> LC_BUILD_VERSION records are of variable length. The original code
> would use uninitialized memory when the size of a record was exactly 24.
>
> rdar://problem/46032185
>
> Added:
> lldb/trunk/lit/Modules/lc_build_version_notools.yaml
>   - copied, changed from r346787, 
> lldb/trunk/lit/Modules/lc_build_version.yaml
> Modified:
> lldb/trunk/lit/Modules/lc_build_version.yaml
> lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
>
> Modified: lldb/trunk/lit/Modules/lc_build_version.yaml
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/lc_build_version.yaml?rev=346812&r1=346811&r2=346812&view=diff
> ==
> --- lldb/trunk/lit/Modules/lc_build_version.yaml (original)
> +++ lldb/trunk/lit/Modules/lc_build_version.yaml Tue Nov 13 15:14:37 2018
> @@ -1,6 +1,6 @@
>  # RUN: yaml2obj %s > %t.out
>  # RUN: lldb-test symbols %t.out | FileCheck %s
> -# REQUIRES: darwin
> +# REQUIRES: system-darwin
>  # Test that the deployment target is parsed from the load commands.
>  # CHECK: x86_64-apple-macosx10.14.0
>  --- !mach-o
>
> Copied: lldb/trunk/lit/Modules/lc_build_version_notools.yaml (from r346787, 
> lldb/trunk/lit/Modules/lc_build_version.yaml)
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/lc_build_version_notools.yaml?p2=lldb/trunk/lit/Modules/lc_build_version_notools.yaml&p1=lldb/trunk/lit/Modules/lc_build_version.yaml&r1=346787&r2=346812&rev=346812&view=diff
> ==
> --- lldb/trunk/lit/Modules/lc_build_version.yaml (original)
> +++ lldb/trunk/lit/Modules/lc_build_version_notools.yaml Tue Nov 13 15:14:37 
> 2018
> @@ -1,6 +1,6 @@
>  # RUN: yaml2obj %s > %t.out
>  # RUN: lldb-test symbols %t.out | FileCheck %s
> -# REQUIRES: darwin
> +# REQUIRES: system-darwin
>  # Test that the deployment target is parsed from the load commands.
>  # CHECK: x86_64-apple-macosx10.14.0
>  --- !mach-o
> @@ -10,7 +10,7 @@ FileHeader:
>cpusubtype:  0x8003
>filetype:0x0002
>ncmds:   14
> -  sizeofcmds:  744
> +  sizeofcmds:  738
>flags:   0x00200085
>reserved:0x
>  LoadCommands:
> @@ -119,14 +119,11 @@ LoadCommands:
>  cmdsize: 24
>  uuid:8F41E140-23B9-3720-AC28-4E7AF9D159BA
>- cmd: LC_BUILD_VERSION
> -cmdsize: 32
> +cmdsize: 24
>  platform:1
>  minos:   658944
>  sdk: 658944
> -ntools:  1
> -Tools:
> -  - tool:3
> -version: 26738944
> +ntools:  0
>- cmd: LC_SOURCE_VERSION
>  cmdsize: 16
>  version: 0
>
> Modified: lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp?rev=346812&r1=346811&r2=346812&view=diff
> ==
> --- lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (original)
> +++ lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp Tue Nov 
> 13 15:14:37 2018
> @@ -5027,24 +5027,28 @@ bool ObjectFileMachO::GetArchitecture(co
>  const lldb::offset_t cmd_offset = offset;
>  if (data.GetU32(&offset, &load_cmd, 2) == NULL)
>break;
> -
> -if (load_cmd.cmd == llvm::MachO::LC_BUILD_VERSION) {
> -  struct build_version_command build_version;
> -  if (load_cmd.cmdsize != sizeof(build_version))
> +do {
> +  if (load_cmd.cmd == llvm::MachO::LC_BUILD_VERSION) {
> +struct build_version_command build_version;
> +if (load_cmd.cmdsize < sizeof(build_version)) {
> +  // Malformed load command.
> +  break;
> +}
>  if (data.ExtractBytes(cmd_offset, sizeof(build_version),
>data.GetByteOrder(), &build_version) == 0)
> -  continue;
> -  MinOS min_os(build_version.minos);
> -  OSEnv os_env(build_version.platform);
> -  if (os_env.os_type.empty())
> -continue;
> -  os << os_env.os_type << min_os.major_version << '.'
> - << min_os.minor_version << '.' << min_os.patch_version;
> -  triple.setOSName(os.str());
> -  if (!os_env.environment.empty())
> -triple.setEnvironmentName(os_env.environment);
> -  return true;
> -}
> +  break;
> +MinOS mi

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via lldb-commits]

I believe that is correct, and perhaps part of the problem.  In other
projects we call llvm_config.use_clang(), and that actually explicitly
creates a substitution so that if someone writes "clang" they'll get an
error that says "use %clang instead".  Because we have the exact path, that
isn't happening here.  Perhaps the proper fix is to add a keyword argument
to llvm_config.use_clang() so that it can be called as
llvm_config.use_clang(path=p).  This way we can get the proper
substitutions created and we would find out about this.

I can play around with this some, but does it sound reasonable to you?  I'm
not sure how to hit all the edge cases because everything is already
working for me (for some reason), so it won't be obvious if I fix the
problem for those people for whom it's broken.

On Tue, Nov 13, 2018 at 3:16 PM Stella Stamenova 
wrote:

> I took a brief look and I have a question about the usage of clang (rather
> than clang-cl).
>
>
>
> In general I would agree that we have an exact path of clang (or gcc) that
> we are trying to use and they’re specified by using %cc and %cxx in the
> test files, but there are a number of test files that simply use clang e.g.:
>
>
>
> SymbolFile\DWARF\find-variable-dwo.cpp:3:// RUN: clang %s -g -gsplit-dwarf
> -c -emit-llvm -o - --target=x86_64-pc-linux -DONE
>
>
>
> In this case, are we not going to pick up whatever clang happens to be in
> the path instead of one that was explicitly specified? Is this intentional?
>
>
>
> Thanks,
>
> -Stella
>
>
>
> *From:* Zachary Turner 
> *Sent:* Tuesday, November 13, 2018 2:46 PM
> *To:* reviews+d54009+public+0e164460da8f1...@reviews.llvm.org
> *Cc:* Stella Stamenova ; pa...@labath.sk;
> chris.biene...@me.com; dccitali...@gmail.com;
> aleksandr.ura...@jetbrains.com; jdevliegh...@apple.com;
> abidh@gmail.com; teempe...@gmail.com; ki.s...@gmail.com;
> mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com;
> lldb-commits@lists.llvm.org; l...@inglorion.net
> *Subject:* Re: [PATCH] D54009: Refactor LLDB lit configuration files
>
>
>
> I think it must be related to setting up the environment in which to run
> clang.  In all other projects we call llvm_config.use_clang() which is in
> llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path
> of a clang we are trying to use, we skip this function in LLDB's lit
> configuration files.  But there is also a lot of other logic in that
> function, so perhaps it's some of that logic that's necessary.
>
>
>
> On Mon, Nov 12, 2018 at 9:02 AM Aleksandr Urakov via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
> aleksandr.urakov added a comment.
>
> But all compiles without errors if I run this manually:
>
>   clang-cl -m32 /Z7 /c /GS-
> C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o
> C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj
>
>
> Repository:
>   rLLDB LLDB
>
> https://reviews.llvm.org/D54009
> 
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Stella Stamenova via lldb-commits]

I am not sure if that’s the right solution for a couple of reasons:

  1.  As far as I can tell only clang calls use_clang (and only lld calls 
use_lld), while the other projects such as lld and llvm rely on the environment 
to be setup correctly
  2.  Lld also has tests that invoke clang-cl and they pass – while the ones in 
LLDB do not, so the invocation of use_clang is not necessary for the tests to 
pass (maybe?)
  3.  LLDB allows us to specify whether to use gcc or clang as well as the path 
and it can also have a test compiler specified via LLDB_USE_TEST_*_COMPILER, so 
we should first decide what scenarios we want to support before trying to make 
this work and possibly making it even more confusing and complicated

Do you know what the answer for 3) is? What compilers are valid to specify for 
the lit/suite/unittests via the various parameters?

Thanks,
-Stella


From: Zachary Turner 
Sent: Tuesday, November 13, 2018 3:27 PM
To: Stella Stamenova 
Cc: reviews+d54009+public+0e164460da8f1...@reviews.llvm.org; pa...@labath.sk; 
chris.biene...@me.com; dccitali...@gmail.com; aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; abidh@gmail.com; teempe...@gmail.com; 
ki.s...@gmail.com; mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com; 
lldb-commits@lists.llvm.org; l...@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

I believe that is correct, and perhaps part of the problem.  In other projects 
we call llvm_config.use_clang(), and that actually explicitly creates a 
substitution so that if someone writes "clang" they'll get an error that says 
"use %clang instead".  Because we have the exact path, that isn't happening 
here.  Perhaps the proper fix is to add a keyword argument to 
llvm_config.use_clang() so that it can be called as 
llvm_config.use_clang(path=p).  This way we can get the proper substitutions 
created and we would find out about this.

I can play around with this some, but does it sound reasonable to you?  I'm not 
sure how to hit all the edge cases because everything is already working for me 
(for some reason), so it won't be obvious if I fix the problem for those people 
for whom it's broken.

On Tue, Nov 13, 2018 at 3:16 PM Stella Stamenova 
mailto:sti...@microsoft.com>> wrote:
I took a brief look and I have a question about the usage of clang (rather than 
clang-cl).

In general I would agree that we have an exact path of clang (or gcc) that we 
are trying to use and they’re specified by using %cc and %cxx in the test 
files, but there are a number of test files that simply use clang e.g.:

SymbolFile\DWARF\find-variable-dwo.cpp:3:// RUN: clang %s -g -gsplit-dwarf -c 
-emit-llvm -o - --target=x86_64-pc-linux -DONE

In this case, are we not going to pick up whatever clang happens to be in the 
path instead of one that was explicitly specified? Is this intentional?

Thanks,
-Stella

From: Zachary Turner mailto:ztur...@google.com>>
Sent: Tuesday, November 13, 2018 2:46 PM
To: 
reviews+d54009+public+0e164460da8f1...@reviews.llvm.org
Cc: Stella Stamenova mailto:sti...@microsoft.com>>; 
pa...@labath.sk; 
chris.biene...@me.com; 
dccitali...@gmail.com; 
aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; 
abidh@gmail.com; 
teempe...@gmail.com; 
ki.s...@gmail.com; 
mgo...@gentoo.org; 
d...@su-root.co.uk; 
jfbast...@apple.com; 
lldb-commits@lists.llvm.org; 
l...@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

I think it must be related to setting up the environment in which to run clang. 
 In all other projects we call llvm_config.use_clang() which is in 
llvm/utils/lit/lit/llvm/config.py, but because here we have an exact path of a 
clang we are trying to use, we skip this function in LLDB's lit configuration 
files.  But there is also a lot of other logic in that function, so perhaps 
it's some of that logic that's necessary.

On Mon, Nov 12, 2018 at 9:02 AM Aleksandr Urakov via Phabricator 
mailto:revi...@reviews.llvm.org>> wrote:
aleksandr.urakov added a comment.

But all compiles without errors if I run this manually:

  clang-cl -m32 /Z7 /c /GS- 
C:\Work\llvm\tools\lldb\lit\SymbolFile\PDB/Inputs/SimpleTypesTest.cpp /o 
C:\Work\llvm\build_x86\tools\lldb\lit\SymbolFile\PDB\Output/SimpleTypesTest.cpp.enums.obj


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54009

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Stella Stamenova via lldb-commits]

Actually, I take 2) back. Lld doesn’t call use_clang, but it only references 
clang-cl, it doesn’t expect it to execute.

From: Stella Stamenova
Sent: Tuesday, November 13, 2018 3:47 PM
To: 'Zachary Turner' 
Cc: reviews+d54009+public+0e164460da8f1...@reviews.llvm.org; pa...@labath.sk; 
chris.biene...@me.com; dccitali...@gmail.com; aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; abidh@gmail.com; teempe...@gmail.com; 
ki.s...@gmail.com; mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com; 
lldb-commits@lists.llvm.org; l...@inglorion.net
Subject: RE: [PATCH] D54009: Refactor LLDB lit configuration files

I am not sure if that’s the right solution for a couple of reasons:

  1.  As far as I can tell only clang calls use_clang (and only lld calls 
use_lld), while the other projects such as lld and llvm rely on the environment 
to be setup correctly
  2.  Lld also has tests that invoke clang-cl and they pass – while the ones in 
LLDB do not, so the invocation of use_clang is not necessary for the tests to 
pass (maybe?)
  3.  LLDB allows us to specify whether to use gcc or clang as well as the path 
and it can also have a test compiler specified via LLDB_USE_TEST_*_COMPILER, so 
we should first decide what scenarios we want to support before trying to make 
this work and possibly making it even more confusing and complicated

Do you know what the answer for 3) is? What compilers are valid to specify for 
the lit/suite/unittests via the various parameters?

Thanks,
-Stella


From: Zachary Turner mailto:ztur...@google.com>>
Sent: Tuesday, November 13, 2018 3:27 PM
To: Stella Stamenova mailto:sti...@microsoft.com>>
Cc: 
reviews+d54009+public+0e164460da8f1...@reviews.llvm.org;
 pa...@labath.sk; 
chris.biene...@me.com; 
dccitali...@gmail.com; 
aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; 
abidh@gmail.com; 
teempe...@gmail.com; 
ki.s...@gmail.com; 
mgo...@gentoo.org; 
d...@su-root.co.uk; 
jfbast...@apple.com; 
lldb-commits@lists.llvm.org; 
l...@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

I believe that is correct, and perhaps part of the problem.  In other projects 
we call llvm_config.use_clang(), and that actually explicitly creates a 
substitution so that if someone writes "clang" they'll get an error that says 
"use %clang instead".  Because we have the exact path, that isn't happening 
here.  Perhaps the proper fix is to add a keyword argument to 
llvm_config.use_clang() so that it can be called as 
llvm_config.use_clang(path=p).  This way we can get the proper substitutions 
created and we would find out about this.

I can play around with this some, but does it sound reasonable to you?  I'm not 
sure how to hit all the edge cases because everything is already working for me 
(for some reason), so it won't be obvious if I fix the problem for those people 
for whom it's broken.

On Tue, Nov 13, 2018 at 3:16 PM Stella Stamenova 
mailto:sti...@microsoft.com>> wrote:
I took a brief look and I have a question about the usage of clang (rather than 
clang-cl).

In general I would agree that we have an exact path of clang (or gcc) that we 
are trying to use and they’re specified by using %cc and %cxx in the test 
files, but there are a number of test files that simply use clang e.g.:

SymbolFile\DWARF\find-variable-dwo.cpp:3:// RUN: clang %s -g -gsplit-dwarf -c 
-emit-llvm -o - --target=x86_64-pc-linux -DONE

In this case, are we not going to pick up whatever clang happens to be in the 
path instead of one that was explicitly specified? Is this intentional?

Thanks,
-Stella

From: Zachary Turner mailto:ztur...@google.com>>
Sent: Tuesday, November 13, 2018 2:46 PM
To: 
reviews+d54009+public+0e164460da8f1...@reviews.llvm.org
Cc: Stella Stamenova mailto:sti...@microsoft.com>>; 
pa...@labath.sk; 
chris.biene...@me.com; 
dccitali...@gmail.com; 
aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; 
abidh@gmail.com; 
teempe...@gmail.com; 
ki.s...@gmail.com; 
mgo...@gentoo.org; 
d...@su-root.co.uk; 
jfbast...@apple.com; 
lldb-commits@lists.llvm.org; 
l...@inglorion.net
S

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via lldb-commits]

On Tue, Nov 13, 2018 at 3:47 PM Stella Stamenova 
wrote:

> I am not sure if that’s the right solution for a couple of reasons:
>
>1. As far as I can tell only clang calls use_clang (and only lld calls
>use_lld), while the other projects such as lld and llvm rely on the
>environment to be setup correctly
>2. Lld also has tests that invoke clang-cl and they pass – while the
>ones in LLDB do not, so the invocation of use_clang is not necessary for
>the tests to pass (maybe?)
>3. LLDB allows us to specify whether to use gcc or clang as well as
>the path and it can also have a test compiler specified via
>LLDB_USE_TEST_*_COMPILER, so we should first decide what scenarios we want
>to support before trying to make this work and possibly making it even more
>confusing and complicated
>
>
>
> Do you know what the answer for 3) is? What compilers are valid to specify
> for the lit/suite/unittests via the various parameters?
>

For the unit tests, I don't think we ever specify a compiler, or we don't
ever read the value.  Because a unit test shouldn't be compiling anything,
it's a different kind of test.

For the dotest suite, specifying the compiler is important and it can
definitely be gcc, but I don't think this uses the same method of going
through config.cc.  In fact, I'm not sure how it determines what compiler
to use at the moment, as it's been a number of years since I've looked at
the dotest suite.

For the lit tests, I'm inclined to say we should keep things simple and
only support clang for now, and add support for new compilers such as gcc
if and when someone actually wants it.  Otherwise YAGNI.

Definitely that time will come, but it doesn't make sense to support it
immediately if nobody is using it today and nobody is planning to enable it
immediately.

So I'm tempted to say that perhaps we should just call
llvm_config.use_clang() and llvm_config.use_lld() and ignore
LLDB_TEST_COMPILER, which in my experience has only been a source of
unnecessary complexity that never actually gets used in practice.

>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via lldb-commits]

Ok so for dotest, it seems to be ignoring the config.cc and config.cxx
entirely.  So we can theoretically do whatever we want with it, or change
around the directory structure so that it's more like:

lldb
* lit
* * Dotest
* * Unit
* * Tests

and put the config.cc / config.cxx logic under Tests.  That's a large
change though and probably not worth making such a large change right away.

dotest tests manually construct the command line directly in CMake via this
`LLDB_DOTEST_ARGS_PROPERTY` global property, and then in
lldb/lit/Suite/lit.cfg we have this line:

dotest_cmd = [config.dotest_path, '-q']
dotest_cmd.extend(config.dotest_args_str.split(';'))


So pretty much everythign the parent lit file has done is totally ignored.

With that in mind, **for the lit tests only** I propose dropping support
for non-clang compilers and ignoring LLDB_TEST_C_COMPILER and
LLDB_TEST_CXX_COMPILER (you can still have a custom path to clang
executable via an environment variable, which is consistent with how
clang's test suite works).

Note that when you run ninja check-lldb-lit you will now get messages that
tell you the exact path to the clang executable, so you can see what the
PATH resolution is doing.

On Tue, Nov 13, 2018 at 4:02 PM Zachary Turner  wrote:

> On Tue, Nov 13, 2018 at 3:47 PM Stella Stamenova 
> wrote:
>
>> I am not sure if that’s the right solution for a couple of reasons:
>>
>>1. As far as I can tell only clang calls use_clang (and only lld
>>calls use_lld), while the other projects such as lld and llvm rely on the
>>environment to be setup correctly
>>2. Lld also has tests that invoke clang-cl and they pass – while the
>>ones in LLDB do not, so the invocation of use_clang is not necessary for
>>the tests to pass (maybe?)
>>3. LLDB allows us to specify whether to use gcc or clang as well as
>>the path and it can also have a test compiler specified via
>>LLDB_USE_TEST_*_COMPILER, so we should first decide what scenarios we want
>>to support before trying to make this work and possibly making it even 
>> more
>>confusing and complicated
>>
>>
>>
>> Do you know what the answer for 3) is? What compilers are valid to
>> specify for the lit/suite/unittests via the various parameters?
>>
>
> For the unit tests, I don't think we ever specify a compiler, or we don't
> ever read the value.  Because a unit test shouldn't be compiling anything,
> it's a different kind of test.
>
> For the dotest suite, specifying the compiler is important and it can
> definitely be gcc, but I don't think this uses the same method of going
> through config.cc.  In fact, I'm not sure how it determines what compiler
> to use at the moment, as it's been a number of years since I've looked at
> the dotest suite.
>
> For the lit tests, I'm inclined to say we should keep things simple and
> only support clang for now, and add support for new compilers such as gcc
> if and when someone actually wants it.  Otherwise YAGNI.
>
> Definitely that time will come, but it doesn't make sense to support it
> immediately if nobody is using it today and nobody is planning to enable it
> immediately.
>
> So I'm tempted to say that perhaps we should just call
> llvm_config.use_clang() and llvm_config.use_lld() and ignore
> LLDB_TEST_COMPILER, which in my experience has only been a source of
> unnecessary complexity that never actually gets used in practice.
>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Stella Stamenova via lldb-commits]

The plan for the lit tests sounds reasonable to me. I would also remove 
LLDB_TEST_C/CXX_COMPILER entirely so that we can reduce confusion since they’re 
only used for the lit tests, right?

My only concern is that I’ve been told that there are people who will build 
lldb with a different compiler than the tests – so the properties for 
LLDB_TEST_C/CXX_COMPILER might actually be used especially in cases where clang 
is not built alongside lldb.

Thanks,
-Stella

From: Zachary Turner 
Sent: Tuesday, November 13, 2018 4:16 PM
To: Stella Stamenova 
Cc: reviews+d54009+public+0e164460da8f1...@reviews.llvm.org; pa...@labath.sk; 
chris.biene...@me.com; dccitali...@gmail.com; aleksandr.ura...@jetbrains.com; 
jdevliegh...@apple.com; abidh@gmail.com; teempe...@gmail.com; 
ki.s...@gmail.com; mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com; 
lldb-commits@lists.llvm.org; l...@inglorion.net
Subject: Re: [PATCH] D54009: Refactor LLDB lit configuration files

Ok so for dotest, it seems to be ignoring the config.cc and config.cxx 
entirely.  So we can theoretically do whatever we want with it, or change 
around the directory structure so that it's more like:

lldb
* lit
* * Dotest
* * Unit
* * Tests

and put the config.cc / config.cxx logic under Tests.  That's a large change 
though and probably not worth making such a large change right away.

dotest tests manually construct the command line directly in CMake via this 
`LLDB_DOTEST_ARGS_PROPERTY` global property, and then in lldb/lit/Suite/lit.cfg 
we have this line:

dotest_cmd = [config.dotest_path, '-q']
dotest_cmd.extend(config.dotest_args_str.split(';'))


So pretty much everythign the parent lit file has done is totally ignored.

With that in mind, **for the lit tests only** I propose dropping support for 
non-clang compilers and ignoring LLDB_TEST_C_COMPILER and 
LLDB_TEST_CXX_COMPILER (you can still have a custom path to clang executable 
via an environment variable, which is consistent with how clang's test suite 
works).

Note that when you run ninja check-lldb-lit you will now get messages that tell 
you the exact path to the clang executable, so you can see what the PATH 
resolution is doing.

On Tue, Nov 13, 2018 at 4:02 PM Zachary Turner 
mailto:ztur...@google.com>> wrote:
On Tue, Nov 13, 2018 at 3:47 PM Stella Stamenova 
mailto:sti...@microsoft.com>> wrote:
I am not sure if that’s the right solution for a couple of reasons:

  1.  As far as I can tell only clang calls use_clang (and only lld calls 
use_lld), while the other projects such as lld and llvm rely on the environment 
to be setup correctly
  2.  Lld also has tests that invoke clang-cl and they pass – while the ones in 
LLDB do not, so the invocation of use_clang is not necessary for the tests to 
pass (maybe?)
  3.  LLDB allows us to specify whether to use gcc or clang as well as the path 
and it can also have a test compiler specified via LLDB_USE_TEST_*_COMPILER, so 
we should first decide what scenarios we want to support before trying to make 
this work and possibly making it even more confusing and complicated

Do you know what the answer for 3) is? What compilers are valid to specify for 
the lit/suite/unittests via the various parameters?

For the unit tests, I don't think we ever specify a compiler, or we don't ever 
read the value.  Because a unit test shouldn't be compiling anything, it's a 
different kind of test.

For the dotest suite, specifying the compiler is important and it can 
definitely be gcc, but I don't think this uses the same method of going through 
config.cc.  In fact, I'm not sure how it determines what compiler to use at the 
moment, as it's been a number of years since I've looked at the dotest suite.

For the lit tests, I'm inclined to say we should keep things simple and only 
support clang for now, and add support for new compilers such as gcc if and 
when someone actually wants it.  Otherwise YAGNI.

Definitely that time will come, but it doesn't make sense to support it 
immediately if nobody is using it today and nobody is planning to enable it 
immediately.

So I'm tempted to say that perhaps we should just call llvm_config.use_clang() 
and llvm_config.use_lld() and ignore LLDB_TEST_COMPILER, which in my experience 
has only been a source of unnecessary complexity that never actually gets used 
in practice.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

[Zachary Turner via lldb-commits]

use_clang() already will fall back on searching the environment variable
'CLANG' to find a path to it.

self.config.clang = self.use_llvm_tool(
'clang', search_env='CLANG', required=required)

But we could make this environment variable a parameter to use_clang() if
we wanted to.  As long as we can agree that we don't need to worry about
gcc -- at least for now -- then it should all simplify down quite a bit.
And AFAICT, there's nobody using gcc with the lit tests right now, so it
just adds unnecessary complexity.  And if and when we do have people using
it, there is even more work to be done.

If someone only wants a clang that isn't the just-built clang (for example
a release version to make sure the tests run faster), all they need to do
is set the environment variable 'CLANG' and it should be fine.

Since the lit suite is still very new and developing, I'm not too concerned
about regressing a feature (especially one with zero users), because the
important thing to me is that it's designed right so that the feature can
grow in organically and not be "forced" in with a subpar implementation.

On Tue, Nov 13, 2018 at 4:23 PM Stella Stamenova 
wrote:

> The plan for the lit tests sounds reasonable to me. I would also remove
> LLDB_TEST_C/CXX_COMPILER entirely so that we can reduce confusion since
> they’re only used for the lit tests, right?
>
>
>
> My only concern is that I’ve been told that there are people who will
> build lldb with a different compiler than the tests – so the properties for
> LLDB_TEST_C/CXX_COMPILER might actually be used especially in cases where
> clang is not built alongside lldb.
>
>
>
> Thanks,
>
> -Stella
>
>
>
> *From:* Zachary Turner 
> *Sent:* Tuesday, November 13, 2018 4:16 PM
>
>
> *To:* Stella Stamenova 
> *Cc:* reviews+d54009+public+0e164460da8f1...@reviews.llvm.org;
> pa...@labath.sk; chris.biene...@me.com; dccitali...@gmail.com;
> aleksandr.ura...@jetbrains.com; jdevliegh...@apple.com;
> abidh@gmail.com; teempe...@gmail.com; ki.s...@gmail.com;
> mgo...@gentoo.org; d...@su-root.co.uk; jfbast...@apple.com;
> lldb-commits@lists.llvm.org; l...@inglorion.net
> *Subject:* Re: [PATCH] D54009: Refactor LLDB lit configuration files
>
>
>
> Ok so for dotest, it seems to be ignoring the config.cc and config.cxx
> entirely.  So we can theoretically do whatever we want with it, or change
> around the directory structure so that it's more like:
>
>
>
> lldb
>
> * lit
>
> * * Dotest
>
> * * Unit
>
> * * Tests
>
>
>
> and put the config.cc / config.cxx logic under Tests.  That's a large
> change though and probably not worth making such a large change right away.
>
>
>
> dotest tests manually construct the command line directly in CMake via
> this `LLDB_DOTEST_ARGS_PROPERTY` global property, and then in
> lldb/lit/Suite/lit.cfg we have this line:
>
>
>
> dotest_cmd = [config.dotest_path, '-q']
>
> dotest_cmd.extend(config.dotest_args_str.split(';'))
>
>
>
>
>
> So pretty much everythign the parent lit file has done is totally
> ignored.
>
>
>
> With that in mind, **for the lit tests only** I propose dropping support
> for non-clang compilers and ignoring LLDB_TEST_C_COMPILER and
> LLDB_TEST_CXX_COMPILER (you can still have a custom path to clang
> executable via an environment variable, which is consistent with how
> clang's test suite works).
>
>
>
> Note that when you run ninja check-lldb-lit you will now get messages that
> tell you the exact path to the clang executable, so you can see what the
> PATH resolution is doing.
>
>
>
> On Tue, Nov 13, 2018 at 4:02 PM Zachary Turner  wrote:
>
> On Tue, Nov 13, 2018 at 3:47 PM Stella Stamenova 
> wrote:
>
> I am not sure if that’s the right solution for a couple of reasons:
>
>1. As far as I can tell only clang calls use_clang (and only lld calls
>use_lld), while the other projects such as lld and llvm rely on the
>environment to be setup correctly
>2. Lld also has tests that invoke clang-cl and they pass – while the
>ones in LLDB do not, so the invocation of use_clang is not necessary for
>the tests to pass (maybe?)
>3. LLDB allows us to specify whether to use gcc or clang as well as
>the path and it can also have a test compiler specified via
>LLDB_USE_TEST_*_COMPILER, so we should first decide what scenarios we want
>to support before trying to make this work and possibly making it even more
>confusing and complicated
>
>
>
> Do you know what the answer for 3) is? What compilers are valid to specify
> for the lit/suite/unittests via the various parameters?
>
>
>
> For the unit tests, I don't think we ever specify a compiler, or we don't
> ever read the value.  Because a unit test shouldn't be compiling anything,
> it's a different kind of test.
>
>
>
> For the dotest suite, specifying the compiler is important and it can
> definitely be gcc, but I don't think this uses the same method of going
> through config.cc.  In fact, I'm not sure how it determines what compiler
>