[Lldb-commits] [PATCH] D140358: [lldb-vscode] Add support for disassembly view

2023-01-17 Thread Enrico Loparco via Phabricator via lldb-commits 
eloparco added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2177
+  const auto max_instruction_size = g_vsc.target.GetMaximumOpcodeByteSize();
+  const auto bytes_offset = -instruction_offset * max_instruction_size;
+  auto start_addr = base_addr - bytes_offset;

clayborg wrote:
> eloparco wrote:
> > clayborg wrote:
> > > Just checked out your changes, and you are still just subtracting a value 
> > > from the start address and attempting to disassemble from memory which is 
> > > the problem. We need to take that subtracted address, and look it up as 
> > > suggested in previous code examples I posted. If you find a function to 
> > > symbol, ask those objects for their instructions. and then try to use 
> > > those. 
> > > 
> > > But basically for _any_ disassembly this is what I would recommend doing:
> > > - first resolve the "start_address" (no matter how you come up the 
> > > address) that want to disassemble into a SBAddress
> > > - check its section. If the section is valid and contains instructions, 
> > > call a function that will disassemble the address range for the section 
> > > that starts at "start_address" and ends at the end of the section. We can 
> > > call this "disassemble_code" as a function. More details on this below
> > > - If the section does not contain instructions, just read the bytes and 
> > > emit a lines like:
> > > ```
> > > 0x1000 .byte 0x12
> > > 0x1000 .byte 0x34
> > > ...
> > > ```
> > > 
> > > Now for the disassemble_code function. We know the address range for this 
> > > is in code. We then need to resolve the address passed to 
> > > "disassemble_code" into a SBAddress and ask that address for a SBFunction 
> > > or SBSymbol as I mentioned. Then we ask the SBFunction or SBSymbol for 
> > > all instructions that they contain, and then use any instructions that 
> > > fall into the range we have. If there is no SBFunction or SBSymbol, then 
> > > disassemble an instruction at a time and then see if the new address will 
> > > resolve to a function or symbol.
> > Tried my changes on a linux x86 machine and the loop `for (unsigned i = 0; 
> > i < max_instruction_size; i++) {` (L2190) takes care of the `start_address` 
> > possibly being in the middle of an instruction, so that's not a problem.  
> > The problem I faced is that it tries to read too far from `base_addr` and 
> > the `ReadMemory()` operation returns few instructions (without reaching 
> > `base_addr`). That was not happening on my macOS M1 (arm) machine. 
> > 
> > To solve, I changed the loop at L2190 to
> > ```
> > for (unsigned i = 0; i < bytes_offset; i++) {
> > auto sb_instructions =
> > _get_instructions_from_memory(start_addr + i, disassemble_bytes);
> > ```
> > and if `start_addr` is in `sb_instructions` we're done and can exit the 
> > loop. That worked.
> > 
> > Another similar thing that can be done is to start from `start_sbaddr` as 
> > you were saying, increment the address until a valid section is found. Then 
> > call `_get_instructions_from_memory()` passing the section start.
> > What do you think? Delegating the disassembling to `ReadMemory()` + 
> > `GetInstructions()` looks simpler to me than to manually iterate over 
> > sections and get instructions from symbols and functions.
> > Is there any shortcoming I'm not seeing?
> so your 
> ```
> for (unsigned i = 0; i < max_instruction_size; i++) { 
> ```
> disassembles and tries to make sure you make it back to the original base 
> address from the original disassemble packet? That can work but could a a bit 
> time consuming?
> 
> The main issue, as you saw on x86, is you don't know what is in memory. You 
> could have unreadable areas of memory when trying to disassemble. Also if you 
> do have good memory that does contain instructions, there can be padding in 
> between functions or even data between functions that the function accesses 
> that can't be correctly disassembled and could throw things off again. 
> 
> The memory regions are the safest way to traverse memory to see what you have 
> and would help you deal with holes in the memory. You can ask about a memory 
> region with:
> ```
> lldb::SBError SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr, 
> lldb::SBMemoryRegionInfo ®ion_info);
> ```
> If you ask about an invalid address, like zero on most platforms, it will 
> return a valid "region_info" with the bounds of the unreadable address range 
> filled in no read/write/execute permissions:
> ```
> (lldb) script
> Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
> >>> region = lldb.SBMemoryRegionInfo()
> >>> err = lldb.process.GetMemoryRegionInfo(0, region)
> [0x-0x0001 ---]
> ```
> So you could use the memory region info to your advantage here. If you have 
> execute permissions, disassemble as instructions, and if you don't emit 
> ".byte 0xXX" for each byte. If there are no permissions, you

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-01-17 Thread LJC via Phabricator via lldb-commits 
paperchalice created this revision.
paperchalice added reviewers: phosek, Ericson2314.
Herald added a subscriber: Enna1.
Herald added a project: All.
paperchalice requested review of this revision.
Herald added projects: clang, Sanitizers, LLDB, OpenMP, LLVM.
Herald added subscribers: llvm-commits, openmp-commits, lldb-commits, 
Sanitizers, cfe-commits.

This patch ensures all resources are installed into `CLANG_RESOURCE_DIR`.
Fixes: https://github.com/llvm/llvm-project/issues/59726


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141907

Files:
  clang/lib/Headers/CMakeLists.txt
  clang/lib/Tooling/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  cmake/Modules/GetClangResourceDir.cmake
  compiler-rt/cmake/base-config-ix.cmake
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/cmake/modules/LLVMExternalProjectUtils.cmake
  openmp/CMakeLists.txt

Index: openmp/CMakeLists.txt
===
--- openmp/CMakeLists.txt
+++ openmp/CMakeLists.txt
@@ -86,8 +86,8 @@
 if(${OPENMP_STANDALONE_BUILD})
   set(LIBOMP_HEADERS_INSTALL_PATH "${CMAKE_INSTALL_INCLUDEDIR}")
 else()
-  string(REGEX MATCH "[0-9]+" CLANG_VERSION ${PACKAGE_VERSION})
-  set(LIBOMP_HEADERS_INSTALL_PATH "${OPENMP_INSTALL_LIBDIR}/clang/${CLANG_VERSION}/include")
+  include(GetClangResourceDir)
+  get_clang_resource_dir(LIBOMP_HEADERS_INSTALL_PATH SUBDIR include)
 endif()
 
 # Build host runtime library, after LIBOMPTARGET variables are set since they are needed
Index: llvm/cmake/modules/LLVMExternalProjectUtils.cmake
===
--- llvm/cmake/modules/LLVMExternalProjectUtils.cmake
+++ llvm/cmake/modules/LLVMExternalProjectUtils.cmake
@@ -257,7 +257,11 @@
 if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR
  ${PACKAGE_VERSION})
-  set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(resource_dir ${LLVM_TOOLS_BINARY_DIR}/${CLANG_RESOURCE_DIR})
+  else()
+set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  endif()
   set(flag_types ASM C CXX MODULE_LINKER SHARED_LINKER EXE_LINKER)
   foreach(type ${flag_types})
 set(${type}_flag -DCMAKE_${type}_FLAGS=-resource-dir=${resource_dir})
Index: lldb/unittests/Expression/ClangParserTest.cpp
===
--- lldb/unittests/Expression/ClangParserTest.cpp
+++ lldb/unittests/Expression/ClangParserTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Basic/Version.h"
+#include "clang/Config/config.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
 #include "TestingSupport/SubsystemRAII.h"
@@ -37,12 +38,16 @@
 TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
 #if !defined(_WIN32)
   std::string path_to_liblldb = "/foo/bar/lib/";
-  std::string path_to_clang_dir =
-  "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING;
+  std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()
+  ? "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME
+"/clang/" CLANG_VERSION_MAJOR_STRING
+  : "/foo/bar/bin/" CLANG_RESOURCE_DIR;
 #else
   std::string path_to_liblldb = "C:\\foo\\bar\\lib";
   std::string path_to_clang_dir =
-  "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_MAJOR_STRING;
+  std::string(CLANG_RESOURCE_DIR).empty()
+  ? "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_MAJOR_STRING
+  : "C:\\foo\\bar\\bin\\" CLANG_RESOURCE_DIR;
 #endif
   EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -54,8 +54,11 @@
 
   static const llvm::StringRef kResourceDirSuffixes[] = {
   // LLVM.org's build of LLDB uses the clang resource directory placed
-  // in $install_dir/lib{,64}/clang/$clang_version.
-  CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING,
+  // in $install_dir/lib{,64}/clang/$clang_version or
+  // $install_dir/bin/$CLANG_RESOURCE_DIR
+  llvm::StringRef(CLANG_RESOURCE_DIR).empty()
+  ? CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING
+  : "bin/" CLANG_RESOURCE_DIR,
   // swift-lldb uses the clang resource directory copied from swift, which
   // by default is placed in $install_dir/lib{,64}/lldb/clang. LLDB places
   // it there, so we use LLDB_INSTALL_LIBDIR_

[Lldb-commits] [PATCH] D141910: [OpenMP][OMPIRBuilder]Move SIMD alignment calculation to LLVM Frontend

2023-01-17 Thread Dominik Adamski via Phabricator via lldb-commits 
domada created this revision.
domada added reviewers: dpalermo, skatrak, kiranktp, RogerV-AMD, NimishMishra, 
jsjodin.
Herald added subscribers: pmatos, asb, guansong, kbarton, hiraditya, 
jgravelle-google, sbc100, yaxunl, nemanjai, dschuff.
Herald added a project: All.
domada requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, sstefan1, 
aheejin.
Herald added a reviewer: jdoerfert.
Herald added projects: clang, LLDB, LLVM.

Currently default simd alignment is defined by Clang specific TargetInfo class. 
This class cannot be reused for LLVM Flang. That's why default simd alignment 
calculation has been moved to OMPIRBuilder which is common for Flang and Clang.

Previous attempt: https://reviews.llvm.org/D138496 was wrong because the 
default alignment depended on the number of built LLVM targets.

If we wanted to calculate the default alignment for PPC and we hadn't specified 
PPC LLVM target to build, then we would get 0 as the alignment because 
OMPIRBuilder couldn't create PPCTargetMachine object and it returned 0 as the 
default value. If PPC LLVM target had been built earlier, then OMPIRBuilder 
could have created PPCTargetMachine object and it would have returned 128.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141910

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/WebAssembly.h
  clang/lib/Basic/Targets/X86.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -3039,6 +3039,23 @@
   Builder.CreateBr(NewBlocks.front());
 }
 
+unsigned
+OpenMPIRBuilder::getOpenMPDefaultSimdAlign(const Triple &TargetTriple,
+   const StringMap &Features) {
+  if (TargetTriple.isX86()) {
+if (Features.lookup("avx512f"))
+  return 512;
+else if (Features.lookup("avx"))
+  return 256;
+return 128;
+  }
+  if (TargetTriple.isPPC())
+return 128;
+  if (TargetTriple.isWasm())
+return 128;
+  return 0;
+}
+
 void OpenMPIRBuilder::applySimd(CanonicalLoopInfo *CanonicalLoop,
 MapVector AlignedVars,
 Value *IfCond, OrderKind Order,
Index: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -502,6 +502,13 @@
ArrayRef Loops,
InsertPointTy ComputeIP);
 
+  /// Get the default alignment value for given target
+  ///
+  /// \param TargetTriple   Target triple
+  /// \param Features   StringMap which describes extra CPU features
+  static unsigned getOpenMPDefaultSimdAlign(const Triple &TargetTriple,
+const StringMap &Features);
+
 private:
   /// Modifies the canonical loop to be a statically-scheduled workshare loop.
   ///
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -500,8 +500,6 @@
   auto target_info = TargetInfo::CreateTargetInfo(
   m_compiler->getDiagnostics(), m_compiler->getInvocation().TargetOpts);
   if (log) {
-LLDB_LOGF(log, "Using SIMD alignment: %d",
-  target_info->getSimdDefaultAlign());
 LLDB_LOGF(log, "Target datalayout string: '%s'",
   target_info->getDataLayoutString());
 LLDB_LOGF(log, "Target ABI: '%s'", target_info->getABI().str().c_str());
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -400,9 +400,6 @@
 return false;
   }
 
-  SimdDefaultAlign =
-  hasFeature("avx512f") ? 512 : hasFeature("avx") ? 256 : 128;
-
   // FIXME: We should allow long double type on 32-bits to match with GCC.
   // This requires backend to be able to lower f80 without x87 first.
   if (!HasX87 && LongDoubleFormat == &llvm::APFloat::x87DoubleExtended())
Index: clang/lib/Basic/Targets/WebAssembly.h
===
--- clang/lib/Basic/Targets/WebAssembly.h
+++ clang/lib/Basic/Targets/WebAssembly.h
@@ -49,7 +49,6 @@
 SuitableAlign = 128;
 LargeArrayMinWidth = 128;
 LargeArrayAlign = 128;
-SimdDefaultAlign = 128;

[Lldb-commits] [PATCH] D140839: [lldb] Enable TestFrameFormatNameWithArgs in case of cross compilation

2023-01-17 Thread Ayush Sahay via Phabricator via lldb-commits 
asahay added inline comments.



Comment at: lldb/test/Shell/helper/build.py:29
 default='host',
-choices=['32', '64', 'host'],
 help='Specify the architecture to target.')

Michael137 wrote:
> Why was this needed?
> 
> Other than that, LGTM
IIUC, //~/lldb/test/Shell/helper/toolchain.py// supplies LLDB's bitness (and 
not the target architecture) as //arch// to 
//~/lldb/test/Shell/helper/build.py// unconditionally. However, we may require 
the target architecture too to customize the compile and link commands (we did 
in our downstream project, at least). Perhaps, we'd like both, LLDB's bitness 
and the target architecture to be supplied via separate options but I thought 
that lifting the restriction in question might've sufficed until we'd 
provisioned support for the same. Let me know if we'd like it handled (or, not 
dealt with at all) in this change itself, though, please.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140839/new/

https://reviews.llvm.org/D140839

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

[Lldb-commits] [PATCH] D140839: [lldb] Enable TestFrameFormatNameWithArgs in case of cross compilation

2023-01-17 Thread Michael Buch via Phabricator via lldb-commits 
Michael137 added inline comments.



Comment at: lldb/test/Shell/helper/build.py:29
 default='host',
-choices=['32', '64', 'host'],
 help='Specify the architecture to target.')

asahay wrote:
> Michael137 wrote:
> > Why was this needed?
> > 
> > Other than that, LGTM
> IIUC, //~/lldb/test/Shell/helper/toolchain.py// supplies LLDB's bitness (and 
> not the target architecture) as //arch// to 
> //~/lldb/test/Shell/helper/build.py// unconditionally. However, we may 
> require the target architecture too to customize the compile and link 
> commands (we did in our downstream project, at least). Perhaps, we'd like 
> both, LLDB's bitness and the target architecture to be supplied via separate 
> options but I thought that lifting the restriction in question might've 
> sufficed until we'd provisioned support for the same. Let me know if we'd 
> like it handled (or, not dealt with at all) in this change itself, though, 
> please.
Ah I see. So you're saying currently the build scripts conflate `-m32`/`-m64` 
with the `-march=<...>` flags? Seems like a separate option for bitness vs. 
arch would be the way to go

This current patch could be split into a stack of 3:
1. Add `-std` flag
2. Switch the test case to use `%build`

and an independent patch

3. Handle the `--arch` confusion


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140839/new/

https://reviews.llvm.org/D140839

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

2023-01-17 Thread Erich Keane via Phabricator via lldb-commits 
erichkeane added a comment.

In D131858#4054348 , @v.g.vassilev 
wrote:

> In D131858#4052031 , @erichkeane 
> wrote:
>
>> In D131858#4052026 , @v.g.vassilev 
>> wrote:
>>
>>> Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman 
>>> and @erichkeane for the efforts saving @mizvekov work over the summer. I 
>>> believe he has sporadic access to internet and soon he will be back to 
>>> normal. Great example of team work here!!
>>
>> Note we don't yet have the evidence that this is savable yet, we should know 
>> in the next day or so.  At minimum, at least 1 of his patches needs to be 
>> reverted due to the memory regression.
>
> I see that https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6584/ 
> picked up your revert and seems successful. Then the next build is green as 
> well and then starts failing for a reason that’s unrelated to this patch.

Yep!  So only 1 of the patches (D136566  
needs reverting/further attention).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131858/new/

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

2023-01-17 Thread Vassil Vassilev via Phabricator via lldb-commits 
v.g.vassilev added a comment.

In D131858#4058807 , @erichkeane 
wrote:

> In D131858#4054348 , @v.g.vassilev 
> wrote:
>
>> In D131858#4052031 , @erichkeane 
>> wrote:
>>
>>> In D131858#4052026 , 
>>> @v.g.vassilev wrote:
>>>
 Thanks a lot @rsmith for providing a fix and thanks a lot @aaron.ballman 
 and @erichkeane for the efforts saving @mizvekov work over the summer. I 
 believe he has sporadic access to internet and soon he will be back to 
 normal. Great example of team work here!!
>>>
>>> Note we don't yet have the evidence that this is savable yet, we should 
>>> know in the next day or so.  At minimum, at least 1 of his patches needs to 
>>> be reverted due to the memory regression.
>>
>> I see that https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6584/ 
>> picked up your revert and seems successful. Then the next build is green as 
>> well and then starts failing for a reason that’s unrelated to this patch.
>
> Yep!  So only 1 of the patches (D136566  
> needs reverting/further attention).

Awesome!!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131858/new/

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D141629: Run address expression argument values through ABI::FixCodeAddress to strip TBI/pointer auth bytes on AArch64

2023-01-17 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

I am looking at this, just taking some time to get a PAC enabled environment 
going again. At first glance it seems like a better way to do things. I just 
want to run the test from my patch against this one.

I can port the test to AArch64 Linux though that's not much closer to being run 
on a bot either. At least these tests are pretty simple, less likely to rot in 
the meantime.
(we could do with a "qemu-system-aarch64 max features" bot but unlikely I'll 
have time to set one up)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141629/new/

https://reviews.llvm.org/D141629

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

[Lldb-commits] [lldb] 7ef075a - [lldb] Only allow SymbolFiles to construct Types

2023-01-17 Thread Augusto Noronha via lldb-commits 

Author: Augusto Noronha
Date: 2023-01-17T10:30:29-08:00
New Revision: 7ef075a6d7a2bccbeedb9b5a1689e8545d5753e9

URL: 
https://github.com/llvm/llvm-project/commit/7ef075a6d7a2bccbeedb9b5a1689e8545d5753e9
DIFF: 
https://github.com/llvm/llvm-project/commit/7ef075a6d7a2bccbeedb9b5a1689e8545d5753e9.diff

LOG: [lldb] Only allow SymbolFiles to construct Types

SymbolFiles should own Types by keeping them in their TypeList. This
patch privates the Type constructor to guarantee that every created Type
is kept in the SymbolFile's type list.

Added: 


Modified: 
lldb/include/lldb/Symbol/SymbolFile.h
lldb/include/lldb/Symbol/SymbolFileOnDemand.h
lldb/include/lldb/Symbol/Type.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/unittests/Symbol/TestTypeSystemClang.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/SymbolFile.h 
b/lldb/include/lldb/Symbol/SymbolFile.h
index 4b5499304664b..1be3323f715be 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -412,6 +412,15 @@ class SymbolFile : public PluginInterface {
   virtual bool GetDebugInfoHadFrameVariableErrors() const = 0;
   virtual void SetDebugInfoHadFrameVariableErrors() = 0;
 
+  virtual lldb::TypeSP
+  MakeType(lldb::user_id_t uid, ConstString name,
+   std::optional byte_size, SymbolContextScope *context,
+   lldb::user_id_t encoding_uid,
+   Type::EncodingDataType encoding_uid_type, const Declaration &decl,
+   const CompilerType &compiler_qual_type,
+   Type::ResolveState compiler_type_resolve_state,
+   uint32_t opaque_payload = 0) = 0;
+
 protected:
   void AssertModuleLock();
 
@@ -492,6 +501,26 @@ class SymbolFileCommon : public SymbolFile {
  m_debug_info_had_variable_errors = true;
   }
 
+  /// This function is used to create types that belong to a SymbolFile. The
+  /// symbol file will own a strong reference to the type in an internal type
+  /// list.
+  lldb::TypeSP MakeType(lldb::user_id_t uid, ConstString name,
+std::optional byte_size,
+SymbolContextScope *context,
+lldb::user_id_t encoding_uid,
+Type::EncodingDataType encoding_uid_type,
+const Declaration &decl,
+const CompilerType &compiler_qual_type,
+Type::ResolveState compiler_type_resolve_state,
+uint32_t opaque_payload = 0) override {
+ lldb::TypeSP type_sp (new Type(
+ uid, this, name, byte_size, context, encoding_uid,
+ encoding_uid_type, decl, compiler_qual_type,
+ compiler_type_resolve_state, opaque_payload));
+ m_type_list.Insert(type_sp);
+ return type_sp;
+  }
+
 protected:
   virtual uint32_t CalculateNumCompileUnits() = 0;
   virtual lldb::CompUnitSP ParseCompileUnitAtIndex(uint32_t idx) = 0;

diff  --git a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h 
b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
index 1f5b0c0902b3c..b615356246c64 100644
--- a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
+++ b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
@@ -227,6 +227,20 @@ class SymbolFileOnDemand : public lldb_private::SymbolFile 
{
 return m_sym_file_impl->SetDebugInfoHadFrameVariableErrors();
   }
 
+  lldb::TypeSP MakeType(lldb::user_id_t uid, ConstString name,
+std::optional byte_size,
+SymbolContextScope *context,
+lldb::user_id_t encoding_uid,
+Type::EncodingDataType encoding_uid_type,
+const Declaration &decl,
+const CompilerType &compiler_qual_type,
+Type::ResolveState compiler_type_resolve_state,
+uint32_t opaque_payload = 0) override {
+return m_sym_file_impl->MakeType(
+uid, name, byte_size, context, encoding_uid, encoding_uid_type, decl,
+compiler_qual_type, compiler_type_resolve_state, opaque_payload);
+  }
+
 private:
   Log *GetLog() const { return ::lldb_private::GetLog(LLDBLog::OnDemand); }
 

diff  --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h
index d04a0fecd421f..fa494172219c9 100644
--- a/lldb/include/lldb/Symbol/Type.h
+++ b/lldb/include/lldb/Symbol/Type.h
@@ -22,6 +22,7 @@
 #include 
 
 namespace lldb_private {
+class SymbolFileCommon;
 
 /// CompilerContext allows an array of these items to be passed to perform
 /// detailed lookups in SymbolVendor and SymbolFile functions.
@@ -101,16 +102,6 @@ class Type : public std::enable_shared_fro

[Lldb-commits] [PATCH] D141702: [lldb/crashlog] Make module loading use Scripted Process affordance

2023-01-17 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib added a comment.

ping @JDevlieghere @bulbazord :p


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141702/new/

https://reviews.llvm.org/D141702

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

[Lldb-commits] [PATCH] D141702: [lldb/crashlog] Make module loading use Scripted Process affordance

2023-01-17 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:421
   Target &target = GetTarget();
+  bool force_lookup = m_scripted_metadata.GetClassName().contains("CrashLog");
 

This seems inverted: rather than the generic scripted process implementation 
having to know about a concrete implementation, why not make this a capability 
that the crashlog can opt into?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141702/new/

https://reviews.llvm.org/D141702

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

[Lldb-commits] [PATCH] D141702: [lldb/crashlog] Make module loading use Scripted Process affordance

2023-01-17 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord requested changes to this revision.
bulbazord added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:421
   Target &target = GetTarget();
+  bool force_lookup = m_scripted_metadata.GetClassName().contains("CrashLog");
 

JDevlieghere wrote:
> This seems inverted: rather than the generic scripted process implementation 
> having to know about a concrete implementation, why not make this a 
> capability that the crashlog can opt into?
Instead of relying on the class name being `CrashLog`, why not ask the scripted 
metadata for its capabilities? Something like `bool force_lookup = 
m_scripted_metadata.ShouldForceSymbolFileLookup();` or something to this effect?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141702/new/

https://reviews.llvm.org/D141702

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

[Lldb-commits] [PATCH] D141972: Delay loading of qProcessInfo-informed binaries until later in the Process setup

2023-01-17 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

I handle two types of binary specification in qProcessInfo -- the 
`main-binary-{uuid,address,slide}` and `binary-addresses` -- so the remote stub 
can tell lldb to discover/load binaries when we connect.

I did this loading in `ProcessGDBRemote::DoConnectRemote()` which is very early 
in process setup, e.g. we don't have threads yet.  This caused problems for one 
group that has a python script in the dSYM and need a more fully set up process.

This patch moves this binary loading from `DoConnectRemote()` to 
`DidLaunchOrAttach()` where we're nearly done setting up the Process.  There's 
a method, `MaybeLoadExecutableModule` which reads the load address of the main 
binary from the remote stub (maybge on linux or something) and sets the load 
address in the Target; I added my new `LoadStubBinaries()` method next to that 
one in `DidLaunchOrAttach()`. I originally thought to simply add the code to 
`MaybeLoadExecutableModule` but then the method was doing two completely 
unrelated things and it would only serve to confuse people in the future I 
think.

I don't know of a particularly good reviewer on this one, but I'll add Jim 
without thinking of a better choice offhand.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141972

Files:
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -374,6 +374,7 @@
   bool UpdateThreadIDList();
 
   void DidLaunchOrAttach(ArchSpec &process_arch);
+  void LoadStubBinaries();
   void MaybeLoadExecutableModule();
 
   Status ConnectToDebugserver(llvm::StringRef host_port);
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -555,58 +555,6 @@
 }
   }
 
-  // The remote stub may know about the "main binary" in
-  // the context of a firmware debug session, and can
-  // give us a UUID and an address/slide of where the
-  // binary is loaded in memory.
-  UUID standalone_uuid;
-  addr_t standalone_value;
-  bool standalone_value_is_offset;
-  if (m_gdb_comm.GetProcessStandaloneBinary(
-  standalone_uuid, standalone_value, standalone_value_is_offset)) {
-ModuleSP module_sp;
-
-if (standalone_uuid.IsValid()) {
-  const bool force_symbol_search = true;
-  const bool notify = true;
-  DynamicLoader::LoadBinaryWithUUIDAndAddress(
-  this, llvm::StringRef(), standalone_uuid, standalone_value,
-  standalone_value_is_offset, force_symbol_search, notify);
-}
-  }
-
-  // The remote stub may know about a list of binaries to
-  // force load into the process -- a firmware type situation
-  // where multiple binaries are present in virtual memory,
-  // and we are only given the addresses of the binaries.
-  // Not intended for use with userland debugging when we
-  // a DynamicLoader plugin that knows how to find the loaded
-  // binaries and will track updates as binaries are added.
-
-  std::vector bin_addrs = m_gdb_comm.GetProcessStandaloneBinaries();
-  if (bin_addrs.size()) {
-UUID uuid;
-const bool value_is_slide = false;
-for (addr_t addr : bin_addrs) {
-  const bool notify = true;
-  // First see if this is a special platform
-  // binary that may determine the DynamicLoader and
-  // Platform to be used in this Process/Target in the
-  // process of loading it.
-  if (GetTarget()
-  .GetDebugger()
-  .GetPlatformList()
-  .LoadPlatformBinaryAndSetup(this, addr, notify))
-continue;
-
-  const bool force_symbol_search = true;
-  // Second manually load this binary into the Target.
-  DynamicLoader::LoadBinaryWithUUIDAndAddress(
-  this, llvm::StringRef(), uuid, addr, value_is_slide,
-  force_symbol_search, notify);
-}
-  }
-
   const StateType state = SetThreadStopInfo(response);
   if (state != eStateInvalid) {
 SetPrivateState(state);
@@ -1007,6 +955,7 @@
 }
   }
 
+  LoadStubBinaries();
   MaybeLoadExecutableModule();
 
   // Find out which StructuredDataPlugins are supported by the d

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

2023-01-17 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

In D138618#4004866 , @labath wrote:

> I think that the main reason we've arrived at such different conclusions is 
> that I treat the "user IDs of DIEs" and and "user IDs of symbol files" as 
> essentially two different things (namespaces if you will). Obviously, one 
> needs the symbol file ID in order to compute the DIERef ID, but theoretically 
> those two can use completely different encodings. With this take on things, I 
> stand by my assertion that DIERef->user_id conversions are tightly 
> controlled. The symbol file ID computations are a mess.
>
> You, if I understand correctly, see the ID of a symbol file as a special case 
> of an all-encompassing user id -- essentially a user_id (or a DIERef) 
> pointing to the first byte of the symbol file. with this world view, the 
> entirety of user ID computation is a mess. :)
> I can definitely see the appeal of viewing the world that way. It's nice and 
> uniform and unambiguous (since you can't have a DIE at offset zero) -- it's 
> just not the view I had when I was writing this code a couple of years ago. 
> :) And it has the disadvantage of obscuring the DIERef->user_id transition 
> (for DIEs at least), and that's what I'm weight against the other advantages 
> of that approach.

FWIW: User IDs of symbol files is not part of any API. It was added to 
SymbolFileDWARF to allow us to identify .o files for mac without dSYM and was 
used by the fission code that Pavel wrote as well. Since this is internal, it 
doesn't matter at all how we make or use the IDs. No public interface will ever 
expect a SymboFile to have a lldb::user_id_t. Therefore it is just down to how 
to use these IDs within the DWARF symbol plug-ins for both mac with .o files 
and for fission with .dwo files.

Things that are created by the DWARF, like lldb_private::CompileUnit, 
lldb_private::Function, lldb_private::Block, and lldb_private::Type do have 
lldb::user_id_t and they are expected to make IDs that make sense for the 
individual SymbolFile plug-in to be able to easily match up with something in 
the DWARF. All of these objects have DIEs in the DWARF, so we must be able to 
make a lldb::user_id_t that allows us to easily answer more questions about 
something at a later date in the DWARF. Like we can make a 
lldb_private::CompileUnit without making any functions, blocks or types. If we 
are later asked to find all functions for a compile unit, we should be able to 
take the lldb::user_id_t of the compile unit and easily do this.

So how DIERef is used is solely up to the DWARF symbol file plug-in.

So we could just assign the user IDs of each SymbolFileDWARF to be the index of 
the .o file for mac, or the index of the .dwo file for fission. It doesn't 
really matter. As long as we can easily take a user_id_t from a virtual 
interface and track it back to the DIE we care about.

> In D138618#4002747 , @ayermolo 
> wrote:
>
>> I guess main issue with GetUID is that we rely on an internal state of 
>> SymbolFileDWARF to
>>
>> 1. figure out if we are dealing with dwo or oso with check for 
>> GetDebugMapSymfile
>> 2. get extra data GetDwoNum(), and GetID()
>>
>> We can either push that logic on the caller side of things (not I deal I 
>> would think) and remove GetUID, or extend the constructor to be a bit more 
>> explicit. This way all the bit settings are still consolidated, but the 
>> logic of when to create what is still hidden in GetDebugMapSymfile.
>>
>> What do you think?
>
> I'm not entirely sure what you mean by that, but I think either of those 
> could be fine. Essentally, what I'm trying to achieve is to make sure is that 
> if the DIERef<->user_id conversion is trivial, then it is always valid to 
> perform it (i.e. there are no partially constructed DIERefs). Ideally, there 
> wouldn't be partially constructed DIERefs in any case, but that is not as 
> important if one is forced to provide that additional information in order to 
> do the conversion.
>
> However, I also want to throw out this alternative 
> . This one goes in the completely opposite 
> direction. Instead of centralizing the conversions, it federates it (which is 
> I think is roughly what I had in mind when I worked on this last time). There 
> is no single place which controls the conversion, but there are multiple 
> **disjoint** places which do that:
>
> - one for the OSO case. This includes the following problematic lines you've 
> listed:
>
>> GetOSOIndexFromUserID
>> GetUID (1/2)
>> Encode/Decode UID (1/2)
>> return DecodedUID{
>>
>>   *dwarf, {std::nullopt, DIERef::Section::DebugInfo, dw_offset_t(uid)}};
>
> - one for the DWO case:
>
>> GetUID (1/2)
>> Encode/Decode UID (1/2)
>
> - one for Symbol File IDs (which is does a +1 on the internal index -- 
> bacause the main object file has ID 0)
>
>> oso_symfile->SetID(((uint64_t)m_cu_idx + 1ull) << 32ul

[Lldb-commits] [PATCH] D141972: Delay loading of qProcessInfo-informed binaries until later in the Process setup

2023-01-17 Thread Jim Ingham via Phabricator via lldb-commits 
jingham added a comment.

This seems okay to me, I can't see why you would be required to fill in the 
"main binary" or "extra binaries" when we're just stopping for the attach.  We 
shouldn't need them till we've decided we're launched or attached and need to 
print any info about that.

But given that it was at one point important to call LoadStubBinaries AFTER you 
had a process, you should leave a comment saying why it is important to wait 
till you have a process before reading in more binaries.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141972/new/

https://reviews.llvm.org/D141972

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