[Lldb-commits] [PATCH] D159142: [lldb] Add support for recognizing swift ast sections in object files

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

LGTM with @kastiglione's comments addressed.




Comment at: lldb/source/Core/Section.cpp:153
+  case eSectionTypeSwiftModules:
+return "swift-modules";
   }

kastiglione wrote:
> I wonder if this should be "swiftmodules". I have never seen it spelled with 
> a hyphen.
Is it actually more than one module?  If not, `swift-module` would make sense 
given the DWARF cases - it is a "Swift Module" converted to lower kebab case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159142

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

[Lldb-commits] [PATCH] D157059: [lldb][PECOFF] Exclude alignment padding when reading section data

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

LGTM outside of the PE header check.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1035
+  // llvm::object::COFFObjectFile::getSectionSize().
+  if (m_binary->getDOSHeader())
+return std::min(section->GetByteSize(), section->GetFileSize());

Can we use `m_binary->getPEHeader() || m_binary->getPE32Header()` here instead? 
 We are technically ensuring that this is a linked (PE) binary rather than an 
object file.  While a DOS header is present, it is not an absolute requirement 
(theoretically, it is practically never going to change).


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

https://reviews.llvm.org/D157059

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5014830ede78: ObjectFile: introduce a COFF object file 
plugin (authored by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D149987?vs=520102&id=520207#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
  lldb/test/Shell/ObjectFile/COFF/basic.yaml

Index: lldb/test/Shell/ObjectFile/COFF/basic.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/COFF/basic.yaml
@@ -0,0 +1,234 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Plugin name: COFF
+# CHECK: Architecture: x86_64-unknown-windows-msvc
+
+# CHECK: Executable: false
+# CHECK: Stripped: false
+# CHECK: Type: object file
+# CHECK: Strata: user
+
+# CHECK: Name: .text
+# CHECK: Type: code
+
+# CHECK: Name: .data
+# CHECK: Type: data
+
+# CHECK: Name: .bss
+# CHECK: Type: zero-fill
+
+# CHECK: Name: .rdata
+# CHECK: Type: data
+
+# CHECK: Name: .debug_abbrev
+# CHECK: Type: dwarf-abbrev
+
+# CHECK: Name: .debug_info
+# CHECK: Type: dwarf-info
+
+# CHECK: Name: .debug_str
+# CHECK: Type: dwarf-str
+
+# CHECK: Name: .debug_line
+# CHECK: Type: dwarf-line
+
+--- !COFF
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+Alignment:   4
+SectionData: ''
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+  - Name:.bss
+Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+SizeOfRawData:   0
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 31343A34313A303700
+  - Name:.debug_abbrev
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 011101250E1305030E10171B0E023400030E49133F193A0B3B0B021803010149130421004913370B0526004913062400030E3E0B0B0B072400030E0B0B3E0B00
+  - Name:.debug_info
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4F00040008010C0032003400023F00330001010903033F00044B0009000544000649000601074E00080700
+Relocations:
+  - VirtualAddress:  6
+SymbolName:  .debug_abbrev
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  12
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  18
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  22
+SymbolName:  .debug_line
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  26
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  31
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  43
+SymbolName:  timestamp
+Type:IMAGE_REL_AMD64_ADDR64
+  - VirtualAddress:  69
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  76
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - Name:.debug_str
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4170706C6520636C616E672076657273696F6E2031342E302E332028636C616E672D313430332E302E32322E31342E3129002D002F7661722F656D7074790074696D657374616D700063686172005F5F41525241595F53495A455F545950455F5F00
+  - Name:.debug_line
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 250004001F00010101FB0E0D0001010101000101003C737464696E3E00
+  - Name:.llvm_addrsig
+Characteristics: [ IMAGE_SCN_LNK_REMOVE ]
+Alignment:   1
+SectionData: ''
+symbols:
+  - Name:.text
+Value:   0
+SectionNumber:   1
+SimpleType:  IM

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 520102.
compnerd marked an inline comment as done.
compnerd added a comment.

Address feedback


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

https://reviews.llvm.org/D149987

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
  lldb/test/Shell/ObjectFile/COFF/basic.yaml

Index: lldb/test/Shell/ObjectFile/COFF/basic.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/COFF/basic.yaml
@@ -0,0 +1,234 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Plugin name: COFF
+# CHECK: Architecture: x86_64-unknown-windows-msvc
+
+# CHECK: Executable: false
+# CHECK: Stripped: false
+# CHECK: Type: object file
+# CHECK: Strata: user
+
+# CHECK: Name: .text
+# CHECK: Type: code
+
+# CHECK: Name: .data
+# CHECK: Type: data
+
+# CHECK: Name: .bss
+# CHECK: Type: zero-fill
+
+# CHECK: Name: .rdata
+# CHECK: Type: data
+
+# CHECK: Name: .debug_abbrev
+# CHECK: Type: dwarf-abbrev
+
+# CHECK: Name: .debug_info
+# CHECK: Type: dwarf-info
+
+# CHECK: Name: .debug_str
+# CHECK: Type: dwarf-str
+
+# CHECK: Name: .debug_line
+# CHECK: Type: dwarf-line
+
+--- !COFF
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+Alignment:   4
+SectionData: ''
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+  - Name:.bss
+Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+SizeOfRawData:   0
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 31343A34313A303700
+  - Name:.debug_abbrev
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 011101250E1305030E10171B0E023400030E49133F193A0B3B0B021803010149130421004913370B0526004913062400030E3E0B0B0B072400030E0B0B3E0B00
+  - Name:.debug_info
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4F00040008010C0032003400023F00330001010903033F00044B0009000544000649000601074E00080700
+Relocations:
+  - VirtualAddress:  6
+SymbolName:  .debug_abbrev
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  12
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  18
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  22
+SymbolName:  .debug_line
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  26
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  31
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  43
+SymbolName:  timestamp
+Type:IMAGE_REL_AMD64_ADDR64
+  - VirtualAddress:  69
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  76
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - Name:.debug_str
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4170706C6520636C616E672076657273696F6E2031342E302E332028636C616E672D313430332E302E32322E31342E3129002D002F7661722F656D7074790074696D657374616D700063686172005F5F41525241595F53495A455F545950455F5F00
+  - Name:.debug_line
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 250004001F00010101FB0E0D0001010101000101003C737464696E3E00
+  - Name:.llvm_addrsig
+Characteristics: [ IMAGE_SCN_LNK_REMOVE ]
+Alignment:   1
+SectionData: ''
+symbols:
+  - Name:.text
+Value:   0
+SectionNumber:   1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
+SectionDefinition:
+  Length:  0
+  Number

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd marked 4 inline comments as done.
compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp:203
+.Case(".debug_pubnames", eSectionTypeDWARFDebugPubNames)
+.Case(".debug_pubtypes", eSectionTypeDWARFDebugPubTypes)
+.Case(".debug_str", eSectionTypeDWARFDebugStr)

aprantl wrote:
> Can these be correct? They seem too long.
They do exceed the COFF limit of 8 characters.  However, this is what is 
actually emitted with a tweak to indicate the long name.  This is why I cannot 
simply use the `Section->Name` and need to extract the long name through the 
generic `SectionRef`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 519984.
compnerd added a comment.

Address feedback, add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
  lldb/test/Shell/ObjectFile/COFF/basic.yaml

Index: lldb/test/Shell/ObjectFile/COFF/basic.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/COFF/basic.yaml
@@ -0,0 +1,234 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Plugin name: COFF
+# CHECK: Architecture: x86_64-unknown-windows-msvc
+
+# CHECK: Executable: false
+# CHECK: Stripped: false
+# CHECK: Type: object file
+# CHECK: Strata: user
+
+# CHECK: Name: .text
+# CHECK: Type: code
+
+# CHECK: Name: .data
+# CHECK: Type: data
+
+# CHECK: Name: .bss
+# CHECK: Type: zero-fill
+
+# CHECK: Name: .rdata
+# CHECK: Type: data
+
+# CHECK: Name: .debug_abbrev
+# CHECK: Type: dwarf-abbrev
+
+# CHECK: Name: .debug_info
+# CHECK: Type: dwarf-info
+
+# CHECK: Name: .debug_str
+# CHECK: Type: dwarf-str
+
+# CHECK: Name: .debug_line
+# CHECK: Type: dwarf-line
+
+--- !COFF
+header:
+  Machine: IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:.text
+Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+Alignment:   4
+SectionData: ''
+  - Name:.data
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+  - Name:.bss
+Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+Alignment:   4
+SectionData: ''
+SizeOfRawData:   0
+  - Name:.rdata
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 31343A34313A303700
+  - Name:.debug_abbrev
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 011101250E1305030E10171B0E023400030E49133F193A0B3B0B021803010149130421004913370B0526004913062400030E3E0B0B0B072400030E0B0B3E0B00
+  - Name:.debug_info
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4F00040008010C0032003400023F00330001010903033F00044B0009000544000649000601074E00080700
+Relocations:
+  - VirtualAddress:  6
+SymbolName:  .debug_abbrev
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  12
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  18
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  22
+SymbolName:  .debug_line
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  26
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  31
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  43
+SymbolName:  timestamp
+Type:IMAGE_REL_AMD64_ADDR64
+  - VirtualAddress:  69
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - VirtualAddress:  76
+SymbolName:  .debug_str
+Type:IMAGE_REL_AMD64_SECREL
+  - Name:.debug_str
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 4170706C6520636C616E672076657273696F6E2031342E302E332028636C616E672D313430332E302E32322E31342E3129002D002F7661722F656D7074790074696D657374616D700063686172005F5F41525241595F53495A455F545950455F5F00
+  - Name:.debug_line
+Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+Alignment:   1
+SectionData: 250004001F00010101FB0E0D0001010101000101003C737464696E3E00
+  - Name:.llvm_addrsig
+Characteristics: [ IMAGE_SCN_LNK_REMOVE ]
+Alignment:   1
+SectionData: ''
+symbols:
+  - Name:.text
+Value:   0
+SectionNumber:   1
+SimpleType:  IMAGE_SYM_TYPE_NULL
+ComplexType: IMAGE_SYM_DTYPE_NULL
+StorageClass:IMAGE_SYM_CLASS_STATIC
+SectionDefinition:
+  Length:  0
+  

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp:103-104
+
+  return new ObjectFileCOFF(module_sp, data_sp, data_offset, file, file_offset,
+length);
+}

bulbazord wrote:
> compnerd wrote:
> > bulbazord wrote:
> > > Reading the implementation of the constructor, it looks like the 
> > > constructor can fail to initialize correctly (specifically `m_object` may 
> > > not be correctly populated). What are callers supposed to do in the way 
> > > of validation here? Maybe there is further validation we can do in this 
> > > function so that the constructor is only invoked if we're absolutely sure 
> > > it will work?
> > There isn't much you can do IMO.  The `new` can fail just as well - at 
> > which point, what do we do?  The constructor should only really fail if the 
> > return type from libLLVMObject has suddenly changed into an invalid type.  
> > That cast really cannot fail in a way that we can recover from.
> `createBinary` and the subsequent cast may not fail in a recoverable fashion 
> but doing it in the constructor means that whatever is trying to create an 
> object gets back an `ObjectFileCOFF` object even if it wasn't initialized 
> correctly. If you did that work in `CreateInstance`, you could return 
> `nullptr` if `createBinary` and that cast failed.
Ah, sinking that into this seems plausible, and then the constructor takes the 
binary?  WFM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp:103-104
+
+  return new ObjectFileCOFF(module_sp, data_sp, data_offset, file, file_offset,
+length);
+}

bulbazord wrote:
> Reading the implementation of the constructor, it looks like the constructor 
> can fail to initialize correctly (specifically `m_object` may not be 
> correctly populated). What are callers supposed to do in the way of 
> validation here? Maybe there is further validation we can do in this function 
> so that the constructor is only invoked if we're absolutely sure it will work?
There isn't much you can do IMO.  The `new` can fail just as well - at which 
point, what do we do?  The constructor should only really fail if the return 
type from libLLVMObject has suddenly changed into an invalid type.  That cast 
really cannot fail in a way that we can recover from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149987

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

[Lldb-commits] [PATCH] D149987: ObjectFile: introduce a COFF object file plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: aprantl, kastiglione.
compnerd added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
compnerd requested review of this revision.

Windows uses COFF as an object file format and PE/COFF as an executable
file format.  They are subtly different and certain elements of a COFF
file may not be present in an executable.  Introduce a new plugin to add
support for the COFF object file format which is required to support
loading of modules built with `-gmodules`.  This is motivated by Swift
which serialises debugging information into a PCM which is a COFF object
file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149987

Files:
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
  lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h

Index: lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
===
--- /dev/null
+++ lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.h
@@ -0,0 +1,108 @@
+//===-- ObjectFileCOFF.h -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_COFF_OBJECTFILECOFF_H
+#define LLDB_SOURCE_PLUGINS_OBJECTFILE_COFF_OBJECTFILECOFF_H
+
+#include "lldb/Symbol/ObjectFile.h"
+
+#include "llvm/Object/COFF.h"
+
+class ObjectFileCOFF : public lldb_private::ObjectFile {
+  std::unique_ptr m_object;
+  lldb_private::UUID m_uuid;
+
+  ObjectFileCOFF(const lldb::ModuleSP &module_sp, lldb::DataBufferSP data_sp,
+ lldb::offset_t data_offset, const lldb_private::FileSpec *file,
+ lldb::offset_t file_offset, lldb::offset_t length);
+
+public:
+  ~ObjectFileCOFF() override;
+
+  static void Initialize();
+  static void Terminate();
+
+  static llvm::StringRef GetPluginNameStatic() { return "COFF"; }
+  static llvm::StringRef GetPluginDescriptionStatic() {
+return "COFF Object File Reader";
+  }
+
+  static lldb_private::ObjectFile *
+  CreateInstance(const lldb::ModuleSP &module_sp, lldb::DataBufferSP data_sp,
+ lldb::offset_t data_offset, const lldb_private::FileSpec *file,
+ lldb::offset_t file_offset, lldb::offset_t length);
+
+  static lldb_private::ObjectFile *
+  CreateMemoryInstance(const lldb::ModuleSP &module_sp,
+   lldb::WritableDataBufferSP data_sp,
+   const lldb::ProcessSP &process_sp, lldb::addr_t header);
+
+  static size_t GetModuleSpecifications(const lldb_private::FileSpec &file,
+lldb::DataBufferSP &data_sp,
+lldb::offset_t data_offset,
+lldb::offset_t file_offset,
+lldb::offset_t length,
+lldb_private::ModuleSpecList &specs);
+
+  // LLVM RTTI support
+  static char ID;
+  bool isA(const void *ClassID) const override {
+return ClassID == &ID || ObjectFile::isA(ClassID);
+  }
+  static bool classof(const ObjectFile *obj) { return obj->isA(&ID); }
+
+  // PluginInterface protocol
+  llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
+
+  // ObjectFile protocol
+  void Dump(lldb_private::Stream *stream) override;
+
+  uint32_t GetAddressByteSize() const override;
+
+  uint32_t GetDependentModules(lldb_private::FileSpecList &specs) override {
+return 0;
+  }
+
+  bool IsExecutable() const override {
+// COFF is an object file format only, it cannot host an executable.
+return false;
+  }
+
+  lldb_private::ArchSpec GetArchitecture() override;
+
+  void CreateSections(lldb_private::SectionList &) override;
+
+  void ParseSymtab(lldb_private::Symtab &) override;
+
+  bool IsStripped() override {
+// FIXME(compnerd) see if there is a good way to identify a /Z7 v /Zi or /ZI
+// build.
+return false;
+  }
+
+  lldb_private::UUID GetUUID() override { return m_uuid; }
+
+  lldb::ByteOrder GetByteOrder() const override {
+// Microsoft always uses little endian.
+return lldb::ByteOrder::eByteOrderLittle;
+  }
+
+  bool ParseHeader() override;
+
+  lldb_private::ObjectFile::Type CalculateType() override {
+// COFF is an object file format only, it cannot host an executable.
+return lldb_private::ObjectFile::eTypeObjectFile;
+  }
+
+  lldb_private::ObjectFile::Strata CalculateStrata() override {
+// FIXME(compnerd) the object file may correspond to a kernel image.
+return lldb_private::ObjectFile::eStrataUser;
+  }
+};
+
+#

[Lldb-commits] [PATCH] D149397: Host: generalise `GetXcodeSDKPath`

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGade3c6a6a88e: Host: generalise `GetXcodeSDKPath` (authored 
by compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D149397?vs=517764&id=517959#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149397

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -57,7 +57,8 @@
 #if defined(__APPLE__)
 TEST_F(HostInfoTest, GetXcodeSDK) {
   auto get_sdk = [](std::string sdk, bool error = false) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK(std::move(sdk))});
 if (!error) {
   EXPECT_TRUE((bool)sdk_path_or_err);
   return *sdk_path_or_err;
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -124,7 +124,8 @@
   }
 
   // Use the default SDK as a fallback.
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+  auto sdk_path_or_err =
+  HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK::GetAnyMacOS()});
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
   toString(sdk_path_or_err.takeError()));
Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -284,7 +284,8 @@
   std::string secondary) {
   llvm::StringRef sdk;
   auto get_sdk = [&](std::string sdk) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK(std::move(sdk))});
 if (!sdk_path_or_err) {
   Debugger::ReportError("Error while searching for Xcode SDK: " +
 toString(sdk_path_or_err.takeError()));
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -338,7 +338,8 @@
   }
 }
 
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+auto sdk_path_or_err =
+HostInfo::GetSDKRoot(SDKOptions{XcodeSDK::GetAnyMacOS()});
 if (!sdk_path_or_err) {
   Log *log = GetLog(LLDBLog::Host);
   LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
@@ -519,7 +520,7 @@
   return path;
 }
 
-llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+llvm::Expected HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
   struct ErrorOrPath {
 std::string str;
 bool is_error;
@@ -530,6 +531,11 @@
   std::lock_guard guard(g_sdk_path_mutex);
   LLDB_SCOPED_TIMER();
 
+  if (!options.XcodeSDK)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "XCodeSDK not specified");
+  XcodeSDK sdk = *options.XcodeSDK;
+
   auto key = sdk.GetString();
   auto it = g_sdk_path.find(key);
   if (it != g_sdk_path.end()) {
Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -1607,8 +1607,8 @@
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
   llvm::StringRef sysroot) {
-  XcodeSDK sdk(sdk_name.str());
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+  auto sdk_path_or_err =
+  HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk_name.str()});
 
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
Index: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -31,7 +31,7 @@
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
-  static llvm::Expected GetXcodeSDKPath(Xco

[Lldb-commits] [PATCH] D149397: Host: generalise `GetXcodeSDKPath`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: JDevlieghere, kastiglione, aprantl.
compnerd added a project: LLDB.
Herald added a project: All.
compnerd requested review of this revision.

This generalises the `GetXcodeSDKPath` hook to a `GetSDKRoot` path which
will be re-used for the Windows support to compute a language specific
SDK path on the platform.  Because there may be other options that we
wish to use to compute the SDK path, sink the `XcodeSDK` parameter into
a structure which can pass a disaggregated set of options.  Furthermore,
optionalise the parameter as Xcode is not available for all platforms.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149397

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -57,7 +57,7 @@
 #if defined(__APPLE__)
 TEST_F(HostInfoTest, GetXcodeSDK) {
   auto get_sdk = [](std::string sdk, bool error = false) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err = HostInfo::GetSDKRoot(SDKOptions{XcodeSDK(std::move(sdk))});
 if (!error) {
   EXPECT_TRUE((bool)sdk_path_or_err);
   return *sdk_path_or_err;
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -124,7 +124,7 @@
   }
 
   // Use the default SDK as a fallback.
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+  auto sdk_path_or_err = HostInfo::GetSDKRoot(HostInfo::SDKOptions{XcodeSDK::GetAnyMacOS()});
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
   toString(sdk_path_or_err.takeError()));
Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -284,7 +284,7 @@
   std::string secondary) {
   llvm::StringRef sdk;
   auto get_sdk = [&](std::string sdk) -> llvm::StringRef {
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+auto sdk_path_or_err = HostInfo::GetSDKRoot(SDKOptions{XcodeSDK(std::move(sdk))});
 if (!sdk_path_or_err) {
   Debugger::ReportError("Error while searching for Xcode SDK: " +
 toString(sdk_path_or_err.takeError()));
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -338,7 +338,7 @@
   }
 }
 
-auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+auto sdk_path_or_err = HostInfo::GetSDKRoot(SDKOptions{XcodeSDK::GetAnyMacOS()});
 if (!sdk_path_or_err) {
   Log *log = GetLog(LLDBLog::Host);
   LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
@@ -519,7 +519,7 @@
   return path;
 }
 
-llvm::Expected HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+llvm::Expected HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
   struct ErrorOrPath {
 std::string str;
 bool is_error;
@@ -539,7 +539,7 @@
 else
   return it->second.str;
   }
-  auto path_or_err = GetXcodeSDK(sdk);
+  auto path_or_err = GetXcodeSDK(options.XcodeSDK);
   if (!path_or_err) {
 std::string error = toString(path_or_err.takeError());
 g_sdk_path.insert({key, {error, true}});
Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -1607,8 +1607,7 @@
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
   llvm::StringRef sysroot) {
-  XcodeSDK sdk(sdk_name.str());
-  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+  auto sdk_path_or_err = HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk_name.str()});
 
   if (!sdk_path_or_err) {
 Debugger::ReportError("Error while searching for Xcode SDK: " +
Index: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -31,7 +31

[Lldb-commits] [PATCH] D146297: [lldb][gnustep][PDB] Parse ObjC base classes and recognize NSObject type

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:399
 // Such symbols will be handled here.
 
 // Ignore unnamed-tag UDTs.

Does it make sense to drop a note here that the 0-length UDT is used as a 
marker for the NSObject root?  Although, shouldn't `NSObject`'s `isa` be 
`NSObject`?  Or does GNUStep not do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146297

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

[Lldb-commits] [PATCH] D147669: PECOFF: consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

@sgraenitz done :)


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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 513614.
compnerd added a comment.

Address feedback


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

https://reviews.llvm.org/D147669

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -863,10 +863,14 @@
   for (const auto &entry : m_binary->export_directories()) {
 llvm::StringRef sym_name;
 if (auto err = entry.getSymbolName(sym_name)) {
-  LLDB_LOG(log,
-   "ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
-   "table entry name: {0}",
-   llvm::fmt_consume(std::move(err)));
+  if (log)
+log->Format(
+__FILE__, __func__,
+"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
+"table entry name: {0}",
+llvm::fmt_consume(std::move(err)));
+  else
+llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -884,10 +888,13 @@
   // it in symtab and make a note using the symbol name.
   llvm::StringRef forwarder_name;
   if (auto err = entry.getForwardTo(forwarder_name)) {
-LLDB_LOG(log,
- "ObjectFilePECOFF::AppendFromExportTable - failed to get "
- "forwarder name of forwarder export '{0}': {1}",
- sym_name, llvm::fmt_consume(std::move(err)));
+if (log)
+  log->Format(__FILE__, __func__,
+  "ObjectFilePECOFF::AppendFromExportTable - failed to get 
"
+  "forwarder name of forwarder export '{0}': {1}",
+  sym_name, llvm::fmt_consume(std::move(err)));
+else
+  llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = 
{symbol.GetDisplayName().GetStringRef(),
@@ -899,10 +906,13 @@
 
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
-  LLDB_LOG(log,
-   "ObjectFilePECOFF::AppendFromExportTable - failed to get "
-   "address of export entry '{0}': {1}",
-   sym_name, llvm::fmt_consume(std::move(err)));
+  if (log)
+log->Format(__FILE__, __func__,
+"ObjectFilePECOFF::AppendFromExportTable - failed to get "
+"address of export entry '{0}': {1}",
+sym_name, llvm::fmt_consume(std::move(err)));
+  else
+llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -863,10 +863,14 @@
   for (const auto &entry : m_binary->export_directories()) {
 llvm::StringRef sym_name;
 if (auto err = entry.getSymbolName(sym_name)) {
-  LLDB_LOG(log,
-   "ObjectFilePECOFF::AppendFromExportTable - failed to get export "
-   "table entry name: {0}",
-   llvm::fmt_consume(std::move(err)));
+  if (log)
+log->Format(
+__FILE__, __func__,
+"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
+"table entry name: {0}",
+llvm::fmt_consume(std::move(err)));
+  else
+llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -884,10 +888,13 @@
   // it in symtab and make a note using the symbol name.
   llvm::StringRef forwarder_name;
   if (auto err = entry.getForwardTo(forwarder_name)) {
-LLDB_LOG(log,
- "ObjectFilePECOFF::AppendFromExportTable - failed to get "
- "forwarder name of forwarder export '{0}': {1}",
- sym_name, llvm::fmt_consume(std::move(err)));
+if (log)
+  log->Format(__FILE__, __func__,
+  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
+  "forwarder name of forwarder export '{0}': {1}",
+  sym_name, llvm::fmt_consume(std::move(err)));
+else
+  llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
@@ -899,10 +906,13 @@
 
 uint32_t function_rva;
 if (auto err = entry.getExportRVA(function_rva)) {
-  LLDB_LOG(log,
-   "ObjectFilePECOFF::AppendFromExportTable - failed to get "
-   "address of export entry '{0}': {1}",
-   sym_name, llvm::fmt_consume(std::move(err)));
+  if (log)
+log->Form

[Lldb-commits] [PATCH] D147669: PECOFF: consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 511706.
compnerd retitled this revision from "PECOFF: enforce move semantics and 
consume errors properly" to "PECOFF: consume errors properly".
compnerd edited the summary of this revision.

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

https://reviews.llvm.org/D147669

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -867,6 +867,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -888,6 +889,7 @@
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = 
{symbol.GetDisplayName().GetStringRef(),
@@ -903,6 +905,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -867,6 +867,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -888,6 +889,7 @@
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
@@ -903,6 +905,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D147669#4251441 , @compnerd wrote:

> In D147669#4249968 , @sgraenitz 
> wrote:
>
>> First of all, yes the existing code is incorrect. Thanks for looking into 
>> this. However, I am afraid this isn't the right solution.
>
>
>
>> You probably mean `LLVM_ENABLE_ABI_BREAKING_CHECKS`? For `Error` this 
>> enables logic to make sure the error was checked: 
>> https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724
>>  I think your observation is a side effect of the issue you look at and has 
>> nothing to do with compiler optimizations like NRVO. If logging is off, the 
>> error isn't consumed.
>
> Yes, I did indeed mean checks and not changes.  I had originally tried with 
> only the consumption in the non-logging case, but that still seemed to abort 
> similarly.  Adding the `std::move` on the assignment for the conditional was 
> also required for some reason.

Hmm, now I'm wondering if I had run the wrong binary, because I do remember 
that I somehow ended up running the wrong binary once or twice.  Let's go with 
removing the confusing `std::move` in the `if` because that _shouldn't_ be 
needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D147669#4249968 , @sgraenitz wrote:

> First of all, yes the existing code is incorrect. Thanks for looking into 
> this. However, I am afraid this isn't the right solution.



> You probably mean `LLVM_ENABLE_ABI_BREAKING_CHECKS`? For `Error` this enables 
> logic to make sure the error was checked: 
> https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724
>  I think your observation is a side effect of the issue you look at and has 
> nothing to do with compiler optimizations like NRVO. If logging is off, the 
> error isn't consumed.

Yes, I did indeed mean checks and not changes.  I had originally tried with 
only the consumption in the non-logging case, but that still seemed to abort 
similarly.  Adding the `std::move` on the assignment for the conditional was 
also required for some reason.

I do wonder if we should keep the consume to be unconditional or not as 
@alvinhochun points out that `llvm::Error` does guarantee this behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> compnerd wrote:
> > bulbazord wrote:
> > > If logging is enabled, we are moving from the same object twice, no?
> > > 
> > > I think we should rethink the LLDB_LOG macro when it comes to errors 
> > > :/
> > Yeah ... I was worried about that.
> I should mention that at least on MSVC I _can_ get away with it:
> 
> ```
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found 
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
>  ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
> name: RVA 0x0 for export ordinal table not found
> ```
https://godbolt.org/z/nj4r7K8hb

UBSAN also seems to indicate it is permissible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> bulbazord wrote:
> > If logging is enabled, we are moving from the same object twice, no?
> > 
> > I think we should rethink the LLDB_LOG macro when it comes to errors :/
> Yeah ... I was worried about that.
I should mention that at least on MSVC I _can_ get away with it:

```
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found 
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
 ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry 
name: RVA 0x0 for export ordinal table not found
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

bulbazord wrote:
> If logging is enabled, we are moving from the same object twice, no?
> 
> I think we should rethink the LLDB_LOG macro when it comes to errors :/
Yeah ... I was worried about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: bulbazord, sgraenitz, labath.
compnerd added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
compnerd requested review of this revision.

Ensure that we explicitly indicate that we would like the move semantics
in the assignment.  With MSVC 14.35.32215, the assignment would not
trigger a NVRO and copy constructor would be invoked, which resulted in
a check failure under `LLVM_ENABLE_ABI_BREAKING_CHANGES`.  The error
path is meant to log and continue on the failure but would instead abort
when built with assertions.

Secondary to the assignment cast check, we would not ensure that the
error is consumed in the case that logging is disabled.  Ensure that we
properly drop the error on the floor or we would re-trigger the checked
failure.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147669

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -865,11 +865,12 @@
   // Read each export table entry, ordered by ordinal instead of by name.
   for (const auto &entry : m_binary->export_directories()) {
 llvm::StringRef sym_name;
-if (auto err = entry.getSymbolName(sym_name)) {
+if (auto err = std::move(entry.getSymbolName(sym_name))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -886,11 +887,12 @@
   // Forwarder exports are redirected by the loader transparently, but keep
   // it in symtab and make a note using the symbol name.
   llvm::StringRef forwarder_name;
-  if (auto err = entry.getForwardTo(forwarder_name)) {
+  if (auto err = std::move(entry.getForwardTo(forwarder_name))) {
 LLDB_LOG(log,
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = 
{symbol.GetDisplayName().GetStringRef(),
@@ -901,11 +903,12 @@
 }
 
 uint32_t function_rva;
-if (auto err = entry.getExportRVA(function_rva)) {
+if (auto err = std::move(entry.getExportRVA(function_rva))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -865,11 +865,12 @@
   // Read each export table entry, ordered by ordinal instead of by name.
   for (const auto &entry : m_binary->export_directories()) {
 llvm::StringRef sym_name;
-if (auto err = entry.getSymbolName(sym_name)) {
+if (auto err = std::move(entry.getSymbolName(sym_name))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -886,11 +887,12 @@
   // Forwarder exports are redirected by the loader transparently, but keep
   // it in symtab and make a note using the symbol name.
   llvm::StringRef forwarder_name;
-  if (auto err = entry.getForwardTo(forwarder_name)) {
+  if (auto err = std::move(entry.getForwardTo(forwarder_name))) {
 LLDB_LOG(log,
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
@@ -901,11 +903,12 @@
 }
 
 uint32_t function_rva;
-if (auto err = entry.getExportRVA(function_rva)) {
+if (auto err = std::move(entry.getExportRVA(function_rva))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name

[Lldb-commits] [PATCH] D146536: SymbolFile: ensure that we have a value before invoking `getBitWidth`

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG16b7cf245ec0: SymbolFile: ensure that we have a value before 
invoking `getBitWidth` (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146536

Files:
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1299,6 +1299,15 @@
   // Query the symbol's value as the variable initializer if valid.
   if (member_comp_type.IsConst()) {
 auto value = member->getValue();
+if (value.Type == llvm::pdb::Empty) {
+  LLDB_LOG(GetLog(LLDBLog::AST),
+   "Class '{0}' has member '{1}' of type '{2}' with an unknown 
"
+   "constant size.",
+   record_type.GetTypeName(), member_name,
+   member_comp_type.GetTypeName());
+  continue;
+}
+
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
 unsigned constant_width = value.getBitWidth();


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1299,6 +1299,15 @@
   // Query the symbol's value as the variable initializer if valid.
   if (member_comp_type.IsConst()) {
 auto value = member->getValue();
+if (value.Type == llvm::pdb::Empty) {
+  LLDB_LOG(GetLog(LLDBLog::AST),
+   "Class '{0}' has member '{1}' of type '{2}' with an unknown "
+   "constant size.",
+   record_type.GetTypeName(), member_name,
+   member_comp_type.GetTypeName());
+  continue;
+}
+
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
 unsigned constant_width = value.getBitWidth();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146536: SymbolFile: ensure that we have a value before invoking `getBitWidth`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 507330.
compnerd added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Address feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146536

Files:
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1299,6 +1299,15 @@
   // Query the symbol's value as the variable initializer if valid.
   if (member_comp_type.IsConst()) {
 auto value = member->getValue();
+if (value.Type == llvm::pdb::Empty) {
+  LLDB_LOG(GetLog(LLDBLog::AST),
+   "Class '{0}' has member '{1}' of type '{2}' with an unknown 
"
+   "constant size.",
+   record_type.GetTypeName(), member_name,
+   member_comp_type.GetTypeName());
+  continue;
+}
+
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
 unsigned constant_width = value.getBitWidth();


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1299,6 +1299,15 @@
   // Query the symbol's value as the variable initializer if valid.
   if (member_comp_type.IsConst()) {
 auto value = member->getValue();
+if (value.Type == llvm::pdb::Empty) {
+  LLDB_LOG(GetLog(LLDBLog::AST),
+   "Class '{0}' has member '{1}' of type '{2}' with an unknown "
+   "constant size.",
+   record_type.GetTypeName(), member_name,
+   member_comp_type.GetTypeName());
+  continue;
+}
+
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
 unsigned constant_width = value.getBitWidth();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D146536: SymbolFile: ensure that we have a value before invoking `getBitWidth`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D146536#4210062 , @sgraenitz wrote:

> Thanks for tackling this! Yes, I hit it as well during development and 
> wondered what is the right thing to do. The effect of the proposed change 
> will be log messages like this right?
>
>   ... which resolves to a constant value of mismatched width (-1 bits). 
> Ignoring constant.

No, it should be something like 4294967295 bits.  Assuming that you have that 
log enabled.

> I think it would be great to log the actual issue (constant width not 
> accessible) and bail out if `value.Type == llvm::pdb::Empty`. Even before 
> requesting `qual_type`. What do you think?

Sure, I can test that and see if there are any adverse effects from that.




Comment at: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:1307
+unsigned constant_width =
+value.Type == llvm::pdb::Empty ? - 1 : value.getBitWidth();
 

sgraenitz wrote:
> Is the `-1` intended to express an invalid value? Maybe this should become a 
> signed `int` then?
Yeah, by means of it being impossibly large.  I suppose I could do an explicit 
cast (i.e. `static_cast(-1)` to make this clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146536

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

[Lldb-commits] [PATCH] D146536: SymbolFile: ensure that we have a value before invoking `getBitWidth`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: rnk, sgraenitz.
Herald added a project: All.
compnerd requested review of this revision.
Herald added a project: LLDB.

Ensure that the variant returned by `member->getValue()` has a value and
is not `Empty`.  Failure to do so will trigger an assertion failure in
`llvm::pdb::Variant::getBitWidth()`.  This can occur when the `static`
member is a forward declaration.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146536

Files:
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1301,7 +1301,10 @@
 auto value = member->getValue();
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
-unsigned constant_width = value.getBitWidth();
+// If we have an incomplete type, the value may be empty, which will
+// trigger an assertion failure on `getBitWidth`.
+unsigned constant_width =
+value.Type == llvm::pdb::Empty ? - 1 : value.getBitWidth();
 
 if (qual_type->isIntegralOrEnumerationType()) {
   if (type_width >= constant_width) {


Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1301,7 +1301,10 @@
 auto value = member->getValue();
 clang::QualType qual_type = decl->getType();
 unsigned type_width = m_ast.getASTContext().getIntWidth(qual_type);
-unsigned constant_width = value.getBitWidth();
+// If we have an incomplete type, the value may be empty, which will
+// trigger an assertion failure on `getBitWidth`.
+unsigned constant_width =
+value.Type == llvm::pdb::Empty ? - 1 : value.getBitWidth();
 
 if (qual_type->isIntegralOrEnumerationType()) {
   if (type_width >= constant_width) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX);
+  llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR);
   if (!llvm::sys::fs::exists(LibPath)) {

Ericson2314 wrote:
> compnerd wrote:
> > serge-sans-paille wrote:
> > > Ericson2314 wrote:
> > > > mgorny wrote:
> > > > > Well, one thing I immediately notice is that technically 
> > > > > `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many 
> > > > > locations you are assuming it will always be relative. Not saying 
> > > > > that's a blocker but I think you should add checks for that into 
> > > > > `CMakeLists.txt`.
> > > > Yes I was working on this, and I intend to use `CMAKE_INSTALL_LIBDIR` 
> > > > with an absolute path.
> > > > 
> > > > OK if this isn't supported initially but we should ovoid regressing 
> > > > where possible.
> > > Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the 
> > > library install dir or (b) for the location where build artifact as 
> > > stored in CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless 
> > > we derive it from (a) but I can see a lot of complication there for the 
> > > testing step. Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a 
> > > path and not a directory component?
> > > 
> > I think that is absolutely reasonable.  There is the 
> > `CMAKE_INSTALL_FULL_LIBDIR` which should be the relatively absolute path 
> > (it is relative to `DESTDIR`).  The `CMAKE_INSTALL_LIBDIR` should be the 
> > relative component which is added to `CMAKE_INSTALL_PREFIX`.
> Please do not do this. In NixOS we uses absolute paths for these which are 
> *not* within `CMAKE_INSTALL_PREFIX` and I would very much like that to 
> continue to work, and have put a lot of effort into it.
> 
> (I am sorry I have been a bit AWAL on LLVM things in general but hopefully 
> will have more time as we head into the new year.)
Why can NixOS not use `CMAKE_INSTALL_FULL_LIBDIR`?  That is computed to 
`${CMAKE_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR}` only if it is not already 
defined to an absolute path.


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

https://reviews.llvm.org/D137337

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

[Lldb-commits] [PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX);
+  llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR);
   if (!llvm::sys::fs::exists(LibPath)) {

serge-sans-paille wrote:
> Ericson2314 wrote:
> > mgorny wrote:
> > > Well, one thing I immediately notice is that technically 
> > > `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many locations 
> > > you are assuming it will always be relative. Not saying that's a blocker 
> > > but I think you should add checks for that into `CMakeLists.txt`.
> > Yes I was working on this, and I intend to use `CMAKE_INSTALL_LIBDIR` with 
> > an absolute path.
> > 
> > OK if this isn't supported initially but we should ovoid regressing where 
> > possible.
> Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the 
> library install dir or (b) for the location where build artifact as stored in 
> CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless we derive it 
> from (a) but I can see a lot of complication there for the testing step. 
> Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a path and not a 
> directory component?
> 
I think that is absolutely reasonable.  There is the 
`CMAKE_INSTALL_FULL_LIBDIR` which should be the relatively absolute path (it is 
relative to `DESTDIR`).  The `CMAKE_INSTALL_LIBDIR` should be the relative 
component which is added to `CMAKE_INSTALL_PREFIX`.


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

https://reviews.llvm.org/D137337

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

[Lldb-commits] [PATCH] D133890: [CMake] Do these replacements to make use of D132608

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

I don't see anything wrong with this change per se, but I'm conflicted on the 
name of the variable.  These are not standard variables but are encroaching on 
the CMake namespace.  What happens if upstream decides to use these names?  I 
think that we should keep the names in the `LLVM_` namespace.  However, I 
realize that the variable itself is not being touched here and this is a 
mechanical replacement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133890

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

[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: cmake/Modules/GNUBinaryDirs.cmake:3
+  get_filename_component(CMAKE_LIBDIR_BASENAME "${CMAKE_INSTALL_LIBDIR}" NAME)
+endif()
+

Should this perhaps be moved further down near the usage?



Comment at: cmake/Modules/GNUBinaryDirs.cmake:6
+if (NOT DEFINED CMAKE_BINARY_BINDIR)
+  set(CMAKE_BINARY_BINDIR "${CMAKE_BINARY_BINDIR}/bin")
+endif()

arichardson wrote:
> I find this name a bit confusing, maybe something like `CMAKE_BUILD_BINDIR` 
> (and the same for _LIBDIR/_INCLUDEDIR would be less surprising? That way it's 
> clear that we are referring to binaries/libraries/includes in the build 
> output (and it mirrors CMAKE_INSTALL_INCLUDEDIR, etc.) 
I think you mean `${CMAKE_BINARY_DIR}/bin` as this is in a block where 
`CMAKE_BINARY_BINDIR` is undefined.



Comment at: cmake/Modules/GNUBinaryDirs.cmake:10
+if (NOT DEFINED CMAKE_BINARY_INCLUDEDIR)
+  set(CMAKE_BINARY_INCLUDEDIR "${CMAKE_BINARY_DIR}/inc")
+endif()

Is the default spelling for include not `include` not `inc`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132608

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: clang/include/clang/Config/config.h.cmake:57
+/* Multilib basename for libdir. */
+#define CLANG_INSTALL_LIBDIR_BASENAME "${CLANG_INSTALL_LIBDIR_BASENAME}"
 

Does this not potentially break downstreams?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

Thanks @thakis - 906e60b9f923464cba0f71a9205846550752162f 
 should 
have addressed that from a few days ago (sorry about not mentioning that here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77287

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

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
compnerd marked an inline comment as done.
Closed by commit rGf1585a4b47cc: Windows: support `DoLoadImage` (authored by 
compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D77287?vs=390249&id=391847#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/CMakeLists.txt
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -44,6 +44,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target &target,
@@ -71,6 +80,15 @@
  BreakpointSite *bp_site) override;
 
   std::vector m_supported_architectures;
+
+private:
+  std::unique_ptr
+  MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &context,
+   lldb_private::Status &status);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -19,10 +19,20 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Expression/DiagnosticManager.h"
+#include "lldb/Expression/FunctionCaller.h"
+#include "lldb/Expression/UserExpression.h"
+#include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/ConvertUTF.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -151,6 +161,283 @@
   return error;
 }
 
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_image) {
+  DiagnosticManager diagnostics;
+
+  if (loaded_image)
+loaded_image->Clear();
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread) {
+error.SetErrorString("LoadLibrary error: no thread available to invoke LoadLibrary");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  ExecutionContext context;
+  thread->CalculateExecutionContext(context);
+
+  Status status;
+  UtilityFunction *loader =
+  process->GetLoadImageUtilityFunction(this, [&]() -> std::unique_ptr {
+return MakeLoadImageUtilityFunction(context, status);
+  });
+  if (loader == nullptr)
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  FunctionCaller *invocation = loader->GetFunctionCaller();
+  if (!invocation) {
+error.SetErrorString("LoadLibrary error: could not get function caller");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  /* Convert name */
+  llvm::SmallVector name;
+  if (!llvm::convertUTF8ToUTF16String(remote_file.GetPath(), name)) {
+error.SetErrorString("Loa

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd marked an inline comment as done.
compnerd added inline comments.



Comment at: lldb/source/Plugins/Platform/Windows/PlatformWindows.h:47-51
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;

JDevlieghere wrote:
> I know it's an `override`, but what a horrible interface...
I completely agree.



Comment at: lldb/test/Shell/Process/Windows/process_load.cpp:3
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s

JDevlieghere wrote:
> compnerd wrote:
> > JDevlieghere wrote:
> > > We should probably have a `lit.local.cfg` in the Windows directory with 
> > > 
> > > ```
> > > if 'system-windows' not in config.available_features:
> > >   config.unsupported = True
> > > 
> > > ```
> > I think that's a good idea, but, should be a separate change - it isn't 
> > related to the load/unload functionality.
> Fair enough
I already pushed that change, so it's already taken care of, but it is not part 
of the set of changes :)


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

https://reviews.llvm.org/D77287

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

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 390249.
compnerd added a comment.

Add missing null-terminators.


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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/CMakeLists.txt
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -44,6 +44,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target &target,
@@ -71,6 +80,15 @@
  BreakpointSite *bp_site) override;
 
   std::vector m_supported_architectures;
+
+private:
+  std::unique_ptr
+  MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &context,
+   lldb_private::Status &status);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -19,10 +19,19 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Expression/DiagnosticManager.h"
+#include "lldb/Expression/FunctionCaller.h"
+#include "lldb/Expression/UserExpression.h"
+#include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/ConvertUTF.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -151,6 +160,283 @@
   return error;
 }
 
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_image) {
+  DiagnosticManager diagnostics;
+
+  if (loaded_image)
+loaded_image->Clear();
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread) {
+error.SetErrorString("LoadLibrary error: no thread available to invoke LoadLibrary");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  ExecutionContext context;
+  thread->CalculateExecutionContext(context);
+
+  Status status;
+  UtilityFunction *loader =
+  process->GetLoadImageUtilityFunction(this, [&]() -> std::unique_ptr {
+return MakeLoadImageUtilityFunction(context, status);
+  });
+  if (loader == nullptr)
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  FunctionCaller *invocation = loader->GetFunctionCaller();
+  if (!invocation) {
+error.SetErrorString("LoadLibrary error: could not get function caller");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  /* Convert name */
+  llvm::SmallVector name;
+  if (!llvm::convertUTF8ToUTF16String(remote_file.GetPath(), name)) {
+error.SetErrorString("LoadLibrary error: could not convert path to UCS2");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+  name.emplace_back(L'\0');
+
+  /* Inject name paramter into inferior */
+  lldb::addr_t injected_name =
+  process->AllocateMemory(name.size() * sizeof(llvm::UTF16)

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 390246.
compnerd added a comment.
Herald added a subscriber: mgorny.

Fix build rules


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/CMakeLists.txt
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -44,6 +44,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target &target,
@@ -71,6 +80,15 @@
  BreakpointSite *bp_site) override;
 
   std::vector m_supported_architectures;
+
+private:
+  std::unique_ptr
+  MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &context,
+   lldb_private::Status &status);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -19,10 +19,19 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Expression/DiagnosticManager.h"
+#include "lldb/Expression/FunctionCaller.h"
+#include "lldb/Expression/UserExpression.h"
+#include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/ConvertUTF.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -151,6 +160,281 @@
   return error;
 }
 
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_image) {
+  DiagnosticManager diagnostics;
+
+  if (loaded_image)
+loaded_image->Clear();
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread) {
+error.SetErrorString("LoadLibrary error: no thread available to invoke LoadLibrary");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  ExecutionContext context;
+  thread->CalculateExecutionContext(context);
+
+  Status status;
+  UtilityFunction *loader =
+  process->GetLoadImageUtilityFunction(this, [&]() -> std::unique_ptr {
+return MakeLoadImageUtilityFunction(context, status);
+  });
+  if (loader == nullptr)
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  FunctionCaller *invocation = loader->GetFunctionCaller();
+  if (!invocation) {
+error.SetErrorString("LoadLibrary error: could not get function caller");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  /* Convert name */
+  llvm::SmallVector name;
+  if (!llvm::convertUTF8ToUTF16String(remote_file.GetPath(), name)) {
+error.SetErrorString("LoadLibrary error: could not convert path to UCS2");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  /* Inject name paramter into inferior */
+  lldb::addr_t injected_name =
+  process->AllocateMemory(na

[Lldb-commits] [PATCH] D77287: Windows: support `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 390245.
compnerd retitled this revision from "Windows: add very basic support for 
`DoLoadImage`" to "Windows: support `DoLoadImage`".
compnerd edited the summary of this revision.
compnerd added a comment.

This is a more complete implementation that allows for error reporting and use 
of UCS-2 as that is needed for the search path alteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -44,6 +44,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target &target,
@@ -71,6 +80,15 @@
  BreakpointSite *bp_site) override;
 
   std::vector m_supported_architectures;
+
+private:
+  std::unique_ptr
+  MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &context,
+   lldb_private::Status &status);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -19,10 +19,19 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Expression/DiagnosticManager.h"
+#include "lldb/Expression/FunctionCaller.h"
+#include "lldb/Expression/UserExpression.h"
+#include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/ConvertUTF.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -151,6 +160,281 @@
   return error;
 }
 
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_image) {
+  DiagnosticManager diagnostics;
+
+  if (loaded_image)
+loaded_image->Clear();
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread) {
+error.SetErrorString("LoadLibrary error: no thread available to invoke LoadLibrary");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  ExecutionContext context;
+  thread->CalculateExecutionContext(context);
+
+  Status status;
+  UtilityFunction *loader =
+  process->GetLoadImageUtilityFunction(this, [&]() -> std::unique_ptr {
+return MakeLoadImageUtilityFunction(context, status);
+  });
+  if (loader == nullptr)
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  FunctionCaller *invocation = loader->GetFunctionCaller();
+  if (!invocation) {
+error.SetErrorString("LoadLibrary error: could not get function caller");
+return LLDB_INVALID_IMAGE_TOKEN;
+  }
+
+  /* Convert name */
+  llvm::SmallVector name;
+  if (!llvm::convertUTF8ToUTF16String(remote_file.GetPath(), name)) {
+error.SetErrorString("

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: llvm/CMakeLists.txt:289
+set(LLVM_TOOLS_INSTALL_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
+"Path for binary subdirectory (defaults to 'bin')")
 mark_as_advanced(LLVM_TOOLS_INSTALL_DIR)





Comment at: llvm/docs/CMake.rst:270
+  Defaults to ``share/man``.
+
 .. _LLVM-related variables:

I'm kinda torn on this.  The variables here are all CMake standard and we 
should redirect users to CMake.  Although, this is following the existing 
pattern, so seems reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

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

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: llvm/CMakeLists.txt:589
 CACHE STRING "Doxygen-generated HTML documentation install directory")
-set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "share/doc/llvm/ocaml-html"
+set(LLVM_INSTALL_OCAMLDOC_HTML_DIR 
"${CMAKE_INSTALL_DOCDIR}/${project}/ocaml-html"
 CACHE STRING "OCamldoc-generated HTML documentation install directory")

Why the change from `llvm` -> `${project}`?  (not that it really makes a 
difference)



Comment at: llvm/cmake/modules/AddSphinxTarget.cmake:77
 if (CMAKE_INSTALL_MANDIR)
-  set(INSTALL_MANDIR ${CMAKE_INSTALL_MANDIR}/)
+  set(INSTALL_MANDIR "${CMAKE_INSTALL_MANDIR}/")
 else()

Nit: trailing slash is unnecessary, `CMAKE_INSTALL_MANDIR` should be defined, 
and if not, you do not want installation into `/` anyway.



Comment at: llvm/cmake/modules/CMakeLists.txt:3
 
-set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm)
+set(LLVM_INSTALL_PACKAGE_DIR lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm CACHE STRING 
"Path for CMake subdirectory (defaults to 'cmake/llvm')")
 set(llvm_cmake_builddir "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")

Why is this variable being put into the cache now?



Comment at: llvm/cmake/modules/LLVMInstallSymlink.cmake:7
   set(DESTDIR $ENV{DESTDIR})
-  set(bindir "${DESTDIR}${CMAKE_INSTALL_PREFIX}/${outdir}/")
+  set(bindir "${DESTDIR}${outdir}/")
 

Nit: trailing slash shouldn't be there.



Comment at: llvm/tools/llvm-config/llvm-config.cpp:361
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);

Why the temporary `StringRef`?  Can you not just initialize `Path` with the 
literal?



Comment at: llvm/tools/llvm-config/llvm-config.cpp:366
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);

Similar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

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

[Lldb-commits] [PATCH] D104176: [libunwind] Define and use portable macro for checking for presence of ASAN

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe03be2efe564: unwind: allow building with GCC (authored by 
compnerd).

Changed prior to commit:
  https://reviews.llvm.org/D104176?vs=351711&id=351743#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104176

Files:
  libunwind/src/libunwind.cpp


Index: libunwind/src/libunwind.cpp
===
--- libunwind/src/libunwind.cpp
+++ libunwind/src/libunwind.cpp
@@ -16,7 +16,13 @@
 
 #include 
 
-#if __has_feature(address_sanitizer)
+// Define the __has_feature extension for compilers that do not support it so
+// that we can later check for the presence of ASan in a compiler-neutral way.
+#if !defined(__has_feature)
+#define __has_feature(feature) 0
+#endif
+
+#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
 #include 
 #endif
 
@@ -187,7 +193,7 @@
 /// Resume execution at cursor position (aka longjump).
 _LIBUNWIND_HIDDEN int __unw_resume(unw_cursor_t *cursor) {
   _LIBUNWIND_TRACE_API("__unw_resume(cursor=%p)", static_cast(cursor));
-#if __has_feature(address_sanitizer)
+#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
   // Inform the ASan runtime that now might be a good time to clean stuff up.
   __asan_handle_no_return();
 #endif


Index: libunwind/src/libunwind.cpp
===
--- libunwind/src/libunwind.cpp
+++ libunwind/src/libunwind.cpp
@@ -16,7 +16,13 @@
 
 #include 
 
-#if __has_feature(address_sanitizer)
+// Define the __has_feature extension for compilers that do not support it so
+// that we can later check for the presence of ASan in a compiler-neutral way.
+#if !defined(__has_feature)
+#define __has_feature(feature) 0
+#endif
+
+#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
 #include 
 #endif
 
@@ -187,7 +193,7 @@
 /// Resume execution at cursor position (aka longjump).
 _LIBUNWIND_HIDDEN int __unw_resume(unw_cursor_t *cursor) {
   _LIBUNWIND_TRACE_API("__unw_resume(cursor=%p)", static_cast(cursor));
-#if __has_feature(address_sanitizer)
+#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
   // Inform the ASan runtime that now might be a good time to clean stuff up.
   __asan_handle_no_return();
 #endif
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D104176: [libunwind] Define and use portable macro for checking for presence of ASAN

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

LGTM; do you need someone to commit this on your behalf?


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

https://reviews.llvm.org/D104176

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

This is a pretty straightforward cleanup now, which adds additional 
functionality by deferring work to CMake.  There are a couple of minor points 
about inconsistent quoting but this seems good otherwise.




Comment at: clang-tools-extra/clang-doc/tool/CMakeLists.txt:26
 install(FILES ../assets/index.js
-  DESTINATION share/clang
+  DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
   COMPONENT clang-doc)

Why are these quoted but other uses not?



Comment at: flang/CMakeLists.txt:442
   install(DIRECTORY include/flang
-DESTINATION include
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 COMPONENT flang-headers

Why is this quoted but other uses not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: clang/cmake/modules/AddClang.cmake:127
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})

Ericson2314 wrote:
> compnerd wrote:
> > Ericson2314 wrote:
> > > compnerd wrote:
> > > > For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` 
> > > > sometimes already deals with the bit suffix, so you can end up with two 
> > > > instances of `32` or `64`.  I think that cleaning that up separately, 
> > > > while focusing on the details of cleaning up how to handle 
> > > > `LLVM_LIBDIR_SUFFIX` is the right thing to do.  The same applies to the 
> > > > rest of the patch.
> > > https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257
> > >  Hmm I see what you mean. So you are saying `s/${ 
> > > CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/` 
> > > everywhere?
> > Yes, that is what I was referring to.  I'm suggesting that you do *not* 
> > make that change instead.  That needs a much more involved change to clean 
> > up the use of `${LLVM_LIBDIR_SUFFIX}`.  I think that this change is already 
> > extremely large as is, and folding more into it is not going to help.
> So you are saying II should back all of these out into 
> `lib${LLVM_LIBDIR_SUFFIX}` as they were before, for now?
> 
> Yes I don't want to make this bigger either, and would rather be on the hook 
> for follow-up work than have this one be too massive to get over the finish 
> line.
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: clang/cmake/modules/AddClang.cmake:127
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})

Ericson2314 wrote:
> compnerd wrote:
> > For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` 
> > sometimes already deals with the bit suffix, so you can end up with two 
> > instances of `32` or `64`.  I think that cleaning that up separately, while 
> > focusing on the details of cleaning up how to handle `LLVM_LIBDIR_SUFFIX` 
> > is the right thing to do.  The same applies to the rest of the patch.
> https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257
>  Hmm I see what you mean. So you are saying `s/${ 
> CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/` 
> everywhere?
Yes, that is what I was referring to.  I'm suggesting that you do *not* make 
that change instead.  That needs a much more involved change to clean up the 
use of `${LLVM_LIBDIR_SUFFIX}`.  I think that this change is already extremely 
large as is, and folding more into it is not going to help.



Comment at: compiler-rt/cmake/base-config-ix.cmake:72
+  set(COMPILER_RT_INSTALL_PATH "" CACHE PATH
+"Prefix where built compiler-rt artifacts should be installed, comes 
before CMAKE_INSTALL_PREFIX.")
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit 
tests." OFF)

Ericson2314 wrote:
> compnerd wrote:
> > Please don't change the descriptions of the options as part of the 
> > `GNUInstallDirs` handling.  The change to `COMPILER_RT_INSTALL_PATH` looks 
> > incorrect.  Can you explain this change please?
> I tried explain this https://reviews.llvm.org/D99484#2655885 and the original 
> description about prefixes. The GNU install dir variables are allowed to be 
> absolute paths (and not necessary with the installation prefix) (and I needed 
> that for the NixOS patch), so always prepending `COMPILER_RT_INSTALL_PATH` as 
> is done doesn't work if that is `CMAKE_INSTALL_PREFIX` by default.
> 
> If you do `git grep '_PREFIX ""'` and also `git grep GRPC_INSTALL_PATH` you 
> will see that many similar variables also were already empty by default. I 
> agree this thing is a bit ugly, but by that argument I am not doing a new 
> hack for `GNUInstallDIrs` but rather applying an existing ideom consistently 
> in all packages.
Sure, but the *descriptions* of the options are needed to changed for changing 
the value.

That said, I'm not convinced that this change is okay.  It changes the way that 
compiler-rt can be built and used when building a toolchain image.  The 
handling of the install prefix in compiler-rt is significantly different from 
the other parts of LLVM, and so, I think that it may need to be done as a 
separate larger effort.



Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 

Ericson2314 wrote:
> compnerd wrote:
> > Does this need to come here?  Why not push this to after the if block 
> > completes?  The same applies through out the rest of the patch.
> It might be fine here. But I was worried that in some of these cases code 
> included in those blocks might refer to the `GNUInstallDirs` variables.
> 
> Originally I had `GNUInstallDirs` only included in the conditional block 
> after `project(...)`, but then the combined build test failed because nothing 
> including `GnuInstallDirs` in that case. If there is an "entrypoint" 
> CMakeLists boilerplate that combined builds should always use, I think the 
> best thing would be to change it back and then additionally put 
> `GNUInstallDirs` there.
Unified builds always use `llvm/CMakeLists.txt`.  However, both should continue 
to work, and so you will need to add this in the subprojects as well.



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
   install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
-DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+DESTINATION 
${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
 COMPONENT cxx-headers

Ericson2314 wrote:
> compnerd wrote:
> > @ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used?  Can altering 
> > the value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?
> It is sometimes modified to be per target when multiple targets are being 
> used at once. All things `CMAKE_INSTALL_*` are globally scoped so in general 
> the combination builds are quite awkward.
> 
> (Having worked on Meson, I am really missing 
> https://mesonbuild.com/Subprojects.html which is exactly what's needed to do 
> this without these bespoke idioms that never work well enough . Alas...)
I don't think that bringing up other build systems is par

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: clang/cmake/modules/AddClang.cmake:127
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})

For the initial change, Id leave this off.  `CMAKE_INSTALL_LIBDIR` sometimes 
already deals with the bit suffix, so you can end up with two instances of `32` 
or `64`.  I think that cleaning that up separately, while focusing on the 
details of cleaning up how to handle `LLVM_LIBDIR_SUFFIX` is the right thing to 
do.  The same applies to the rest of the patch.



Comment at: compiler-rt/cmake/base-config-ix.cmake:72
+  set(COMPILER_RT_INSTALL_PATH "" CACHE PATH
+"Prefix where built compiler-rt artifacts should be installed, comes 
before CMAKE_INSTALL_PREFIX.")
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit 
tests." OFF)

Please don't change the descriptions of the options as part of the 
`GNUInstallDirs` handling.  The change to `COMPILER_RT_INSTALL_PATH` looks 
incorrect.  Can you explain this change please?



Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
 

Does this need to come here?  Why not push this to after the if block 
completes?  The same applies through out the rest of the patch.



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
   install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
-DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+DESTINATION 
${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
 COMPONENT cxx-headers

@ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used?  Can altering the 
value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[Lldb-commits] [PATCH] D95185: lldb: repair the standalone build for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added a reviewer: JDevlieghere.
compnerd added a project: LLDB.
Herald added a subscriber: mgorny.
compnerd requested review of this revision.
Herald added a subscriber: lldb-commits.

The previous code path only happened to work incidentally.  The
`file(MAKE_DIRECTORY)` is executed early, without the generator
expression being evaluated.  The result is that the literal value is
being treated as a path.  However, on Windows `:` is not a valid
character for a file name.  This would cause the operation to fail.  The
subsequent commands are delayed until runtime, and the operations will
expand the value at generation time yielding the correct result.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95185

Files:
  lldb/source/API/CMakeLists.txt


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -207,13 +207,12 @@
   # When building the LLDB framework, this isn't necessary as there we copy 
everything we need into
   # the framework (including the Clang resourece directory).
   if(NOT LLDB_BUILD_FRAMEWORK)
-set(LLDB_CLANG_RESOURCE_DIR_PARENT "$/clang")
-file(MAKE_DIRECTORY "${LLDB_CLANG_RESOURCE_DIR_PARENT}")
+get_target_property(liblldb_TARGET_FILE_DIR liblldb TARGET_FILE_DIR)
+file(MAKE_DIRECTORY "${liblldb_TARGET_FILE_DIR}/clang")
 add_custom_command(TARGET liblldb POST_BUILD
-  COMMENT "Linking Clang resource dir into LLDB build directory: 
${LLDB_CLANG_RESOURCE_DIR_PARENT}"
-  COMMAND ${CMAKE_COMMAND} -E make_directory 
"${LLDB_CLANG_RESOURCE_DIR_PARENT}"
-  COMMAND ${CMAKE_COMMAND} -E create_symlink 
"${LLDB_EXTERNAL_CLANG_RESOURCE_DIR}"
-  
"${LLDB_CLANG_RESOURCE_DIR_PARENT}/${LLDB_CLANG_RESOURCE_DIR_NAME}"
+  COMMENT "Linking Clang resource dir into LLDB build directory: 
${liblldb_TARGET_FILE_DIR}/clang"
+  COMMAND ${CMAKE_COMMAND} -E make_directory 
"${liblldb_TARGET_FILE_DIR}/clang"
+  COMMAND ${CMAKE_COMMAND} -E create_symlink 
"${LLDB_EXTERNAL_CLANG_RESOURCE_DIR}" 
"${liblldb_TARGET_FILE_DIR}/clang/${LLDB_CLANG_RESOURCE_DIR_NAME}"
 )
   endif()
 endif()


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -207,13 +207,12 @@
   # When building the LLDB framework, this isn't necessary as there we copy everything we need into
   # the framework (including the Clang resourece directory).
   if(NOT LLDB_BUILD_FRAMEWORK)
-set(LLDB_CLANG_RESOURCE_DIR_PARENT "$/clang")
-file(MAKE_DIRECTORY "${LLDB_CLANG_RESOURCE_DIR_PARENT}")
+get_target_property(liblldb_TARGET_FILE_DIR liblldb TARGET_FILE_DIR)
+file(MAKE_DIRECTORY "${liblldb_TARGET_FILE_DIR}/clang")
 add_custom_command(TARGET liblldb POST_BUILD
-  COMMENT "Linking Clang resource dir into LLDB build directory: ${LLDB_CLANG_RESOURCE_DIR_PARENT}"
-  COMMAND ${CMAKE_COMMAND} -E make_directory "${LLDB_CLANG_RESOURCE_DIR_PARENT}"
-  COMMAND ${CMAKE_COMMAND} -E create_symlink "${LLDB_EXTERNAL_CLANG_RESOURCE_DIR}"
-  "${LLDB_CLANG_RESOURCE_DIR_PARENT}/${LLDB_CLANG_RESOURCE_DIR_NAME}"
+  COMMENT "Linking Clang resource dir into LLDB build directory: ${liblldb_TARGET_FILE_DIR}/clang"
+  COMMAND ${CMAKE_COMMAND} -E make_directory "${liblldb_TARGET_FILE_DIR}/clang"
+  COMMAND ${CMAKE_COMMAND} -E create_symlink "${LLDB_EXTERNAL_CLANG_RESOURCE_DIR}" "${liblldb_TARGET_FILE_DIR}/clang/${LLDB_CLANG_RESOURCE_DIR_NAME}"
 )
   endif()
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88181: Utility: ignore OS version on non-Darwin targets in `ArchSpec`

[Saleem Abdulrasool via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG92d42b32a9b7: Utility: ignore OS version on non-Darwin 
targets in `ArchSpec` (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88181

Files:
  lldb/source/Utility/ArchSpec.cpp


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -1426,7 +1426,8 @@
 if ((user_specified_triple.getVendor() != llvm::Triple::UnknownVendor) ||
 TripleVendorWasSpecified()) {
   const unsigned unspecified = 0;
-  if (user_specified_triple.getOSMajorVersion() != unspecified) {
+  if (!user_specified_triple.isOSDarwin() ||
+  user_specified_triple.getOSMajorVersion() != unspecified) {
 user_triple_fully_specified = true;
   }
 }


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -1426,7 +1426,8 @@
 if ((user_specified_triple.getVendor() != llvm::Triple::UnknownVendor) ||
 TripleVendorWasSpecified()) {
   const unsigned unspecified = 0;
-  if (user_specified_triple.getOSMajorVersion() != unspecified) {
+  if (!user_specified_triple.isOSDarwin() ||
+  user_specified_triple.getOSMajorVersion() != unspecified) {
 user_triple_fully_specified = true;
   }
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88181: Utility: ignore OS version on non-Darwin targets in `ArchSpec`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

Correct, this is just upstreaming the original ArchSpec change that we found on 
Swift.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88181

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

[Lldb-commits] [PATCH] D88181: Utility: ignore OS version on non-Darwin targets in `ArchSpec`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: JDevlieghere, kastiglione.
compnerd added a project: LLDB.
compnerd requested review of this revision.

The OS version field is generally not very helpful for non-Darwin
targets.  On Linux, it identifies the kernel version which moves
out-of-sync with the userspace.  On Windows, this field actually ends up
corresponding to the Visual Studio toolset version instead of the OS
version.  Consider non-Darwin targets without an OS version to be fully
specified.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88181

Files:
  lldb/source/Utility/ArchSpec.cpp


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -1426,7 +1426,8 @@
 if ((user_specified_triple.getVendor() != llvm::Triple::UnknownVendor) ||
 TripleVendorWasSpecified()) {
   const unsigned unspecified = 0;
-  if (user_specified_triple.getOSMajorVersion() != unspecified) {
+  if (!user_specified_triple.isOSDarwin() ||
+  user_specified_triple.getOSMajorVersion() != unspecified) {
 user_triple_fully_specified = true;
   }
 }


Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -1426,7 +1426,8 @@
 if ((user_specified_triple.getVendor() != llvm::Triple::UnknownVendor) ||
 TripleVendorWasSpecified()) {
   const unsigned unspecified = 0;
-  if (user_specified_triple.getOSMajorVersion() != unspecified) {
+  if (!user_specified_triple.isOSDarwin() ||
+  user_specified_triple.getOSMajorVersion() != unspecified) {
 user_triple_fully_specified = true;
   }
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84691: [CMake] Move find_package(ZLIB) to LLVMConfig

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

Thanks, this is a great idea!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84691



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

[Lldb-commits] [PATCH] D81501: [lldb/CMake] Make it possible to build against Python 2 with CMake > 3.12

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

Would be nice to remove this entirely in favour of CMake's builtin support for 
Python Interpeter and Libraries.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D81501



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

[Lldb-commits] [PATCH] D78972: Treat hasWeakODRLinkage symbols as external in REPL computations

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

WeakODR requires that the symbol actually be discarded if not referenced.  This 
will preserve the symbol even if unreferenced will it not?  One approach might 
be to just create a `DenseMap` and check for any references and mark is as 
preserved otherwise just drop it.  However, it _is_ ODR, which means that if it 
ever gets instantiated, we are well within our rights to give you the second 
definition rather than the first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78972



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

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

@labath - can this get merged so that I can rebase and get D77287 
 merged?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77662



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 255581.

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -49,6 +49,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target *target,
@@ -73,6 +82,10 @@
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -20,7 +20,10 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
@@ -308,6 +311,111 @@
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+ const char *expression,
+ ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+  R"(
+  // libloaderapi.h
+
+  // WINBASEAPI BOOL WINAPI FreeLibrary(HMODULE);
+  extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeLibrary(void *hLibModule);
+
+  // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+  extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+)";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+Status result = loader->CanLoadImage();
+if (result.Fail())
+  return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result = UserExpression::Evaluate(
+  context, options, expression, kLoaderDecls, value, error);
+  if (result != eExpressionCompleted)
+return error;
+
+  if (value->GetError().Fail())
+return value->GetError();
+
+  return Status();
+}
+
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+   

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 255580.

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h

Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -49,6 +49,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target *target,
@@ -73,6 +82,10 @@
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -20,7 +20,10 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
@@ -308,6 +311,111 @@
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+ const char *expression,
+ ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+  R"(
+  // libloaderapi.h
+
+  // WINBASEAPI BOOL WINAPI FreeLibrary(HMODULE);
+  extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeLibrary(void *hLibModule);
+
+  // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+  extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+)";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+Status result = loader->CanLoadImage();
+if (result.Fail())
+  return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result = UserExpression::Evaluate(
+  context, options, expression, kLoaderDecls, value, error);
+  if (result != eExpressionCompleted)
+return error;
+
+  if (value->GetError().Fail())
+return value->GetError();
+
+  return Status();
+}
+
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_path) {
+  if (loaded_path)
+loaded_path->Clear();
+
+  StreamString expression;
+  expression.Printf("LoadLibraryA(\"%s\")", remote_file.GetPath().c_str());
+
+  ValueObjectSP value;
+  Status result =
+  EvaluateLoaderExpression(process, expression.GetData(), value);
+  if (result.Fail())
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  Scalar scalar;
+  if (value->ResolveValue(scalar)) {
+lldb::addr_t address = scalar.ULongLong();
+if (address == 0)
+  return LLDB_INVALID_IMAGE_TOKEN;
+return proce

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

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

https://reviews.llvm.org/D77287



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D77287#1963242 , @labath wrote:

> In D77287#1960390 , @compnerd wrote:
>
> > I think that the basic test is sufficient for this.
>
>
> That test does not seem to be exercising the "unload" part of this patch. It 
> would also be nice to run some basic command like "image list" to verify that 
> the loaded binary is indeed listed.
>
> (I don't really have a hard objection to this being a lit test, but it does 
> sound to me like at that point, this will be reimplementing TestLoadUnload.py)
>
> > I think that the path sanitizing and conversion should be a subsequent 
> > change.
>
> Why is that? The need for sanitation is a direct consequence of how you've 
> chosen to implement this patch -- the posix version of this function does not 
> do sanitation, but it does not need to, as it does not embed the library name 
> into the compiled expression. I can see how the support for wide strings 
> might be considered a separate feature, but given that supporting that is a 
> matter of adding a single `L` to the compiled expression, I don't see a 
> problem with including that here.


Actually, it changes the APIs used and the path that this goes down on the 
Windows side, so it has a much broader impact than it appears.


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

https://reviews.llvm.org/D77287



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd marked an inline comment as done.
compnerd added inline comments.



Comment at: lldb/test/Shell/Process/Windows/process_load.cpp:3
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s

JDevlieghere wrote:
> We should probably have a `lit.local.cfg` in the Windows directory with 
> 
> ```
> if 'system-windows' not in config.available_features:
>   config.unsupported = True
> 
> ```
I think that's a good idea, but, should be a separate change - it isn't related 
to the load/unload functionality.


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

https://reviews.llvm.org/D77287



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 254883.

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/test/Shell/Process/Windows/process_load.cpp

Index: lldb/test/Shell/Process/Windows/process_load.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/process_load.cpp
@@ -0,0 +1,12 @@
+// clang-format off
+
+// REQUIRES: system-windows
+// RUN: %build --compiler=clang-cl -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -o "b main" -o "process launch" -o "process load kernel32.dll" | FileCheck %s
+
+int main(int argc, char *argv[]) {
+  return 0;
+}
+
+// CHECK: "Loading "kernel32.dll"...ok{{.*}}
+// CHECK: Image 0 loaded.
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -49,6 +49,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target *target,
@@ -73,6 +82,10 @@
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -20,7 +20,10 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
@@ -306,6 +309,111 @@
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+ const char *expression,
+ ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+  R"(
+  // libloaderapi.h
+
+  // WINBASEAPI BOOL WINAPI FreeModule(HMODULE);
+  extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeModule(void *hLibModule);
+
+  // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+  extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+)";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+Status result = loader->CanLoadImage();
+if (result.Fail())
+  return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result = UserExpression::Evaluate(
+  context, options, expression, kLoaderDecls, value, error);
+  if (result != eExpressionCompleted)
+return error;
+
+  if (value->GetError().Fail())
+return value->GetError();
+
+  return Status();
+}
+
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+ 

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

Okay, thanks to some help from @JDevlieghere I was able to get a test going for 
this.  I think that the basic test is sufficient for this.  I think that the 
path sanitizing and conversion should be a subsequent change.


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

https://reviews.llvm.org/D77287



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 254631.

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

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h

Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -49,6 +49,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target *target,
@@ -73,6 +82,10 @@
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -20,7 +20,10 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
@@ -306,6 +309,111 @@
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+ const char *expression,
+ ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+  R"(
+  // libloaderapi.h
+
+  // WINBASEAPI BOOL WINAPI FreeModule(HMODULE);
+  extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeModule(void *hLibModule);
+
+  // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+  extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+)";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+Status result = loader->CanLoadImage();
+if (result.Fail())
+  return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result = UserExpression::Evaluate(
+  context, options, expression, kLoaderDecls, value, error);
+  if (result != eExpressionCompleted)
+return error;
+
+  if (value->GetError().Fail())
+return value->GetError();
+
+  return Status();
+}
+
+uint32_t PlatformWindows::DoLoadImage(Process *process,
+  const FileSpec &remote_file,
+  const std::vector *paths,
+  Status &error, FileSpec *loaded_path) {
+  if (loaded_path)
+loaded_path->Clear();
+
+  StreamString expression;
+  expression.Printf("LoadLibraryA(\"%s\")", remote_file.GetPath().c_str());
+
+  ValueObjectSP value;
+  Status result =
+  EvaluateLoaderExpression(process, expression.GetData(), value);
+  if (result.Fail())
+return LLDB_INVALID_IMAGE_TOKEN;
+
+  Scalar scalar;
+  if (value->ResolveValue(scalar)) {
+lldb::addr_t address = scalar.ULongLong();
+if (address == 0)
+  return LLDB_INVALID_IMAGE_TOKEN;
+return process

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

Thanks for the hint about the string conversion, however, I think that it's 
going to complicate things as the string is going to be a mixture of UTF-8 and 
UTF-16 content.

As to the testing, `functionalities/load_using_paths/TestLoadUsingPaths.py` is 
not applicable to Windows.  In fact, I don't really see a good way to really 
test much of this outside the context of the swift REPL which forces the 
loading of a DLL which is in fact how I discovered this.  If there is an easy 
way to ensure that the dll that is needed is in the user's `PATH`, then I 
suppose creating an empty dll and loading that is theoretically possible, but 
that too can have a lot of flakiness due to dependencies to build and all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77287



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

[Lldb-commits] [PATCH] D77287: Windows: add very basic support for `DoLoadImage`

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: JDevlieghere, xiaobai.
compnerd added a project: LLDB.

Add some very basic support for `DoLoadImage` and `UnloadImage` for Windows.  
This was previously not implemented and would result in a failure at runtime 
that is hard to detect.

This implementation is extremely limited but serves as a starting point for 
proper support for loading a library.  Ideally, the user input would be 
converted from UTF-8 to UTF-16.  This requires additional heap allocations and 
conversion logic.  Error recovery there requires additional allocations both 
from the local heap and the global heap.

This support enables the use of LLDB's Swift REPL on Windows.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77287

Files:
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.h

Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.h
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.h
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.h
@@ -49,6 +49,15 @@
 
   lldb_private::Status DisconnectRemote() override;
 
+  uint32_t DoLoadImage(lldb_private::Process *process,
+   const lldb_private::FileSpec &remote_file,
+   const std::vector *paths,
+   lldb_private::Status &error,
+   lldb_private::FileSpec *loaded_path) override;
+
+  lldb_private::Status UnloadImage(lldb_private::Process *process,
+   uint32_t image_token) override;
+
   lldb::ProcessSP DebugProcess(lldb_private::ProcessLaunchInfo &launch_info,
lldb_private::Debugger &debugger,
lldb_private::Target *target,
@@ -73,6 +82,10 @@
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformWindows);
+
+  lldb_private::Status EvaluateLoaderExpression(lldb_private::Process *process,
+const char *expression,
+lldb::ValueObjectSP &value);
 };
 
 } // namespace lldb_private
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -16,11 +16,13 @@
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Expression/UserExpression.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/Status.h"
 
@@ -306,6 +308,112 @@
   return error;
 }
 
+Status PlatformWindows::EvaluateLoaderExpression(Process *process,
+ const char *expression,
+ ValueObjectSP &value) {
+  // FIXME(compnerd) `-fdeclspec` is not passed to the clang instance?
+  static const char kLoaderDecls[] =
+  R"(
+  // libloaderapi.h
+
+  // WINBASEAPI BOOL WINAPI FreeModule(HMODULE);
+  extern "C" /* __declspec(dllimport) */ BOOL __stdcall FreeModule(void *hLibModule);
+
+  // WINBASEAPI HMODULE WINAPI LoadLibraryA(LPCSTR);
+  extern "C" /* __declspec(dllimport) */ void * __stdcall LoadLibraryA(const char *);
+)";
+
+  if (DynamicLoader *loader = process->GetDynamicLoader()) {
+Status result = loader->CanLoadImage();
+if (result.Fail())
+  return result;
+  }
+
+  ThreadSP thread = process->GetThreadList().GetExpressionExecutionThread();
+  if (!thread)
+return Status("selected thread is invalid");
+
+  StackFrameSP frame = thread->GetStackFrameAtIndex(0);
+  if (!frame)
+return Status("frame 0 is invalid");
+
+  ExecutionContext context;
+  frame->CalculateExecutionContext(context);
+
+  EvaluateExpressionOptions options;
+  options.SetUnwindOnError(true);
+  options.SetIgnoreBreakpoints(true);
+  options.SetExecutionPolicy(eExecutionPolicyAlways);
+  options.SetLanguage(eLanguageTypeC_plus_plus);
+  // LoadLibrary{A,W}/FreeLibrary cannot raise exceptions which we can handle.
+  // They may potentially throw SEH exceptions which we do not know how to
+  // handle currently.
+  options.SetTrapExceptions(false);
+  options.SetTimeout(process->GetUtilityExpressionTimeout());
+
+  Status error;
+  ExpressionResults result =
+  UserExpression::Evaluate(context, options, expression, kLoaderDecls,
+   value, error);
+  if (result != eExpressionCompleted)
+return error;
+
+  if (value->GetError().Fail())
+return value->GetError();
+
+  return Status();
+}
+
+uint32_t
+PlatformWindows:

[Lldb-commits] [PATCH] D73289: [lldb/Test] Disallow using substituted binaries in shell test.

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/test/Shell/helper/toolchain.py:24
+   warning.format(execName)))
+
+

Wow, that took a couple of reads to figure out what was being formatted where.  
I wish that there was a nicer way to write that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D73289



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

[Lldb-commits] [PATCH] D73067: [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

Do we need to worry about ordering of the plugins?




Comment at: lldb/source/Plugins/Platform/CMakeLists.txt:9
   add_subdirectory(MacOSX)
-#elseif (CMAKE_SYSTEM_NAME MATCHES "Windows")
+elseif (CMAKE_SYSTEM_NAME MATCHES "Windows")
   add_subdirectory(Windows)

I would use `STREQUAL` rather than `MATCHES`.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D73067



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

[Lldb-commits] [PATCH] D72290: [lldb/CMake] Use LLDB's autodetection logic for libxml2

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/cmake/modules/FindLibXml28.cmake:14
+if (APPLE)
+  set(LIBXML2_LIBRARIES xml2)
+endif()

labath wrote:
> JDevlieghere wrote:
> > kwk wrote:
> > > labath wrote:
> > > > Why is this under `if(APPLE)` ?
> > > To me this looks as if this module file `FileLibXml28.cmake` is only 
> > > relevant for Apple. In line 10 it falls back to the standard CMake 
> > > find-module file (`find_package(LibXml2 QUIET)`). Is this correct?
> > I'm not sure, I just kept the old behavior. I'm pretty sure it doesn't 
> > matter. Do you prefer to make it unconditional or just remove it 
> > altogether? 
> Ah, right, I think I now see where this is coming from. I don't think it 
> makes sense to keep this if xml is disabled then linking against it is 
> useless. Ideally, I'd just remove it...
Why the custom module for this?  Why not use `find_package(LibXml2 2.8 QUIET)` 
and ensure that libxml2 is >= 2.8?


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

https://reviews.llvm.org/D72290



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

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

GIT 68a235d07f9 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70764



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

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

GIT 2046d72e916 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69535



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

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

@stella.stamenova ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69535



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

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D70764#1767395 , @JDevlieghere 
wrote:

> Having one canonical variable controlling zlib support seems indeed desirable.
>
> In D70519#1754618 , @labath wrote:
>
> > With this patch, what is the output of `llvm-config --system-libs` ?
>
>
> @compnerd What's the answer to this for this patch?


Sorry, didn't see the question.  From my local build:

   $ ./bin/llvm-config --system-libs
  /usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm 
/usr/lib/x86_64-linux-gnu/libxml2.so


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70764



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

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

@labath I think you are misunderstanding the patch.  This is not autoselecting 
the dependencies.  It is simply doing that based on an existing option that we 
have - `LLVM_ENABLE_ZLIB`.  We could always search for zlib and override the 
results with `LLVM_ENABLE_ZLIB` as well.  The current build will continue to 
just work - zlib is used only for the compressed debug sections (which requires 
the user to opt-in).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70764



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

[Lldb-commits] [PATCH] D70764: build: reduce CMake handling for zlib

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: beanz, smeenai.
Herald added subscribers: lldb-commits, Sanitizers, hiraditya, mgorny.
Herald added projects: clang, Sanitizers, LLDB, LLVM.

Rather than handling zlib handling manually, use `find_package` from CMake to 
find zlib properly.  Use this to normalize the `LLVM_ENABLE_ZLIB`, `HAVE_ZLIB`, 
`HAVE_ZLIB_H`.  Furthermore, require zlib if `LLVM_ENABLE_ZLIB` is set to 
`YES`, which requires the distributor to explicitly select whether zlib is 
enabled or not.  This simplifies the CMake handling and usage in the rest of 
the tooling.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70764

Files:
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  compiler-rt/test/lit.common.configured.in
  lld/test/CMakeLists.txt
  lld/test/lit.site.cfg.py.in
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  llvm/cmake/config-ix.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CRC.cpp
  llvm/lib/Support/Compression.cpp
  llvm/test/CMakeLists.txt
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp

Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -21,7 +21,7 @@
 
 namespace {
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if LLVM_ENABLE_ZLIB
 
 void TestZlibCompression(StringRef Input, int Level) {
   SmallString<32> Compressed;
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -33,7 +33,7 @@
 config.host_ldflags = '@HOST_LDFLAGS@'
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
-config.have_zlib = @HAVE_LIBZ@
+config.have_zlib = @LLVM_ENABLE_ZLIB@
 config.have_libxar = @HAVE_LIBXAR@
 config.have_dia_sdk = @LLVM_ENABLE_DIA_SDK@
 config.enable_ffi = @LLVM_ENABLE_FFI@
Index: llvm/test/CMakeLists.txt
===
--- llvm/test/CMakeLists.txt
+++ llvm/test/CMakeLists.txt
@@ -1,12 +1,12 @@
 llvm_canonicalize_cmake_booleans(
   BUILD_SHARED_LIBS
   HAVE_LIBXAR
-  HAVE_LIBZ
   HAVE_OCAMLOPT
   HAVE_OCAML_OUNIT
   LLVM_ENABLE_DIA_SDK
   LLVM_ENABLE_FFI
   LLVM_ENABLE_THREADS
+  LLVM_ENABLE_ZLIB
   LLVM_INCLUDE_GO_TESTS
   LLVM_LIBXML2_ENABLED
   LLVM_LINK_LLVM_DYLIB
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -17,13 +17,13 @@
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_ZLIB_H
+#if LLVM_ENABLE_ZLIB
 #include 
 #endif
 
 using namespace llvm;
 
-#if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
+#if LLVM_ENABLE_ZLIB
 static Error createError(StringRef Err) {
   return make_error(Err, inconvertibleErrorCode());
 }
Index: llvm/lib/Support/CRC.cpp
===
--- llvm/lib/Support/CRC.cpp
+++ llvm/lib/Support/CRC.cpp
@@ -25,7 +25,7 @@
 
 using namespace llvm;
 
-#if LLVM_ENABLE_ZLIB == 0 || !HAVE_ZLIB_H
+#if !LLVM_ENABLE_ZLIB
 
 static const uint32_t CRCTable[256] = {
 0x, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -1,7 +1,4 @@
-set(system_libs)
-if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
-  set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
-endif()
+set(system_libs ${ZLIB_LIBRARY})
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   # advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -109,9 +109,6 @@
 /* Define to 1 if you have the `pthread_setname_np' function. */
 #cmakedefine HAVE_PTHREAD_SETNAME_NP ${HAVE_PTHREAD_SETNAME_NP}
 
-/* Define to 1 if you have the `z' library (-lz). */
-#cmakedefine HAVE_LIBZ ${HAVE_LIBZ}
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_LINK_H ${HAVE_LINK_H}
 
@@ -226,9 +223,6 @@
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_VALGRIND_VALGRIND_H ${HAVE_VALGRIND_VALGRIND_H}
 
-/* Define to 1 if you have the  header file. */
-#cmakedefine HAVE_ZLIB_H ${HAVE_ZLIB_H}
-
 /* Have host's _alloca */
 #cmakedefine HAVE__ALLOCA ${HAVE__ALLOCA}
 
Index: llv

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 230658.
compnerd added a comment.

Use @labath's suggestion of bumping minimum required version for Windows


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69535

Files:
  lldb/CMakeLists.txt
  lldb/cmake/modules/LLDBConfig.cmake

Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -190,105 +190,15 @@
   set(${OUT_VERSION_VARNAME}  ${PYTHON_VERSION_OUTPUT} PARENT_SCOPE)
 endfunction()

-function(find_python_libs_windows)
-  if ("${PYTHON_HOME}" STREQUAL "")
-message(WARNING "LLDB embedded Python on Windows requires specifying a value for PYTHON_HOME.  Python support disabled.")
-set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
-return()
-  endif()
-
-  file(TO_CMAKE_PATH "${PYTHON_HOME}/Include" PYTHON_INCLUDE_DIR)
-
-  if(EXISTS "${PYTHON_INCLUDE_DIR}/patchlevel.h")
-file(STRINGS "${PYTHON_INCLUDE_DIR}/patchlevel.h" python_version_str
- REGEX "^#define[ \t]+PY_VERSION[ \t]+\"[^\"]+\"")
-string(REGEX REPLACE "^#define[ \t]+PY_VERSION[ \t]+\"([^\"+]+)[+]?\".*" "\\1"
- PYTHONLIBS_VERSION_STRING "${python_version_str}")
-message(STATUS "Found Python library version ${PYTHONLIBS_VERSION_STRING}")
-string(REGEX REPLACE "([0-9]+)[.]([0-9]+)[.][0-9]+" "python\\1\\2" PYTHONLIBS_BASE_NAME "${PYTHONLIBS_VERSION_STRING}")
-unset(python_version_str)
-  else()
-message(WARNING "Unable to find ${PYTHON_INCLUDE_DIR}/patchlevel.h, Python installation is corrupt.")
-message(WARNING "Python support will be disabled for this build.")
-set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
-return()
-  endif()
-
-  file(TO_CMAKE_PATH "${PYTHON_HOME}" PYTHON_HOME)
-  # TODO(compnerd) when CMake Policy `CMP0091` is set to NEW, we should use
-  # if(CMAKE_MSVC_RUNTIME_LIBRARY MATCHES MultiThreadedDebug)
-  if(NOT DEFINED CMAKE_BUILD_TYPE)
-# Multi-target generator was selected (like Visual Studio or Xcode) where no concrete build type was passed
-# Lookup for both debug and release python installations
-find_python_libs_windows_helper(TRUE  PYTHON_DEBUG_EXE   PYTHON_DEBUG_LIB   PYTHON_DEBUG_DLL   PYTHON_DEBUG_VERSION_STRING)
-find_python_libs_windows_helper(FALSE PYTHON_RELEASE_EXE PYTHON_RELEASE_LIB PYTHON_RELEASE_DLL PYTHON_RELEASE_VERSION_STRING)
-if(LLDB_DISABLE_PYTHON)
-  set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
-  return()
-endif()
-
-# We should have been found both debug and release python here
-# Now check that their versions are equal
-if(NOT PYTHON_DEBUG_VERSION_STRING STREQUAL PYTHON_RELEASE_VERSION_STRING)
-  message(FATAL_ERROR "Python versions for debug (${PYTHON_DEBUG_VERSION_STRING}) and release (${PYTHON_RELEASE_VERSION_STRING}) are different."
-  "Python installation is corrupted")
-endif ()
-
-set(PYTHON_EXECUTABLE $<$:${PYTHON_DEBUG_EXE}>$<$>:${PYTHON_RELEASE_EXE}>)
-set(PYTHON_LIBRARY$<$:${PYTHON_DEBUG_LIB}>$<$>:${PYTHON_RELEASE_LIB}>)
-set(PYTHON_DLL$<$:${PYTHON_DEBUG_DLL}>$<$>:${PYTHON_RELEASE_DLL}>)
-set(PYTHON_VERSION_STRING ${PYTHON_RELEASE_VERSION_STRING})
-  else()
-# Lookup for concrete python installation depending on build type
-if (CMAKE_BUILD_TYPE STREQUAL Debug)
-  set(LOOKUP_DEBUG_PYTHON TRUE)
-else()
-  set(LOOKUP_DEBUG_PYTHON FALSE)
-endif()
-find_python_libs_windows_helper(${LOOKUP_DEBUG_PYTHON} PYTHON_EXECUTABLE PYTHON_LIBRARY PYTHON_DLL PYTHON_VERSION_STRING)
-if(LLDB_DISABLE_PYTHON)
-  set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
-  return()
-endif()
-  endif()
-
-  if(PYTHON_VERSION_STRING)
-string(REPLACE "." ";" PYTHON_VERSION_PARTS "${PYTHON_VERSION_STRING}")
-list(GET PYTHON_VERSION_PARTS 0 PYTHON_VERSION_MAJOR)
-list(GET PYTHON_VERSION_PARTS 1 PYTHON_VERSION_MINOR)
-list(GET PYTHON_VERSION_PARTS 2 PYTHON_VERSION_PATCH)
-  else()
-unset(PYTHON_VERSION_MAJOR)
-unset(PYTHON_VERSION_MINOR)
-unset(PYTHON_VERSION_PATCH)
-  endif()
-
-  # Set the same variables as FindPythonInterp and FindPythonLibs.
-  set(PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}"  CACHE PATH "")
-  set(PYTHON_LIBRARY"${PYTHON_LIBRARY}" CACHE PATH "")
-  set(PYTHON_DLL"${PYTHON_DLL}" CACHE PATH "")
-  set(PYTHON_INCLUDE_DIR"${PYTHON_INCLUDE_DIR}" CACHE PATH "")
-  set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}"  PARENT_SCOPE)
-  set(PYTHON_VERSION_STRING "${PYTHON_VERSION_STRING}"  PARENT_SCOPE)
-  set(PYTHON_VERSION_MAJOR  "${PYTHON_VERSION_MAJOR}"   PARENT_SCOPE)
-  set(PYTHON_VERSION_MINOR  "${PYTHON_VERSION_MINOR}"   PARENT_SCOPE)
-  set(PYTHON_VERSION_PATCH  "${PYTHON_VERSION_PATCH}"   PARENT_SCOPE)
-
-  message(STATUS 

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd marked an inline comment as done.
compnerd added a comment.

Yeah, doing an incremental rollout makes sense.




Comment at: lldb/cmake/modules/LLDBConfig.cmake:225
+  if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.13 AND CMAKE_SYSTEM_NAME STREQUAL 
Windows)
+find_package(Python3 COMPONENTS Interpreter Development REQUIRED)
+if(Python3_VERSION VERSION_LESS 3.5)

labath wrote:
> What if I use a single-config generator and build a release configuration. Is 
> the "development" thingy still required?
Yes, it is - the component is the headers for building.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69535



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

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

The reason for bringing this back up as a Windows specific thing is that 
currently, there is no good way to build LLDB with python support without 
having to specify additional details on *just* windows because the windows path 
is doing something special.  This is trying to bring the windows path to parity 
with the Linux path.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D69535



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

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: xiaobai, stella.stamenova.
Herald added subscribers: JDevlieghere, mgorny.
Herald added a project: LLDB.

If we have a new enough CMake, use `find_package(Python3)`.  This version is 
able to check both 32-bit and 64-bit versions and will setup everything 
properly without the user needing to specify `PYTHON_HOME`.  This enables 
building lldb's python bindings on Windows under Azure's CI again.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D69535

Files:
  lldb/cmake/modules/LLDBConfig.cmake


Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -221,34 +221,43 @@
 endfunction(find_python_libs_windows)
 
 if (NOT LLDB_DISABLE_PYTHON)
-  if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
-find_python_libs_windows()
-
-if (NOT LLDB_RELOCATABLE_PYTHON)
-  file(TO_CMAKE_PATH "${PYTHON_HOME}" LLDB_PYTHON_HOME)
-  add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
+  if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.13 AND CMAKE_SYSTEM_NAME STREQUAL 
Windows)
+find_package(Python3 COMPONENTS Interpreter Development REQUIRED)
+if(Python3_VERSION VERSION_LESS 3.5)
+  message(FATAL_ERROR "Python 3.5 or newer is required (found: 
${Python3_VERSION})")
 endif()
+set(PYTHON_LIBRARY ${Python3_LIBRARIES})
+include_directories(${Python3_INCLUDE_DIRS})
   else()
-find_package(PythonInterp REQUIRED)
-find_package(PythonLibs REQUIRED)
-  endif()
+if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
+  find_python_libs_windows()
+
+  if (NOT LLDB_RELOCATABLE_PYTHON)
+file(TO_CMAKE_PATH "${PYTHON_HOME}" LLDB_PYTHON_HOME)
+add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
+  endif()
+else()
+  find_package(PythonInterp REQUIRED)
+  find_package(PythonLibs REQUIRED)
+endif()
 
-  if (NOT CMAKE_CROSSCOMPILING)
-string(REPLACE "." ";" pythonlibs_version_list 
${PYTHONLIBS_VERSION_STRING})
-list(GET pythonlibs_version_list 0 pythonlibs_major)
-list(GET pythonlibs_version_list 1 pythonlibs_minor)
-
-# Ignore the patch version. Some versions of macOS report a different patch
-# version for the system provided interpreter and libraries.
-if (NOT PYTHON_VERSION_MAJOR VERSION_EQUAL pythonlibs_major OR
-NOT PYTHON_VERSION_MINOR VERSION_EQUAL pythonlibs_minor)
-  message(FATAL_ERROR "Found incompatible Python interpreter 
(${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR})"
-  " and Python libraries 
(${pythonlibs_major}.${pythonlibs_minor})")
+if (NOT CMAKE_CROSSCOMPILING)
+  string(REPLACE "." ";" pythonlibs_version_list 
${PYTHONLIBS_VERSION_STRING})
+  list(GET pythonlibs_version_list 0 pythonlibs_major)
+  list(GET pythonlibs_version_list 1 pythonlibs_minor)
+
+  # Ignore the patch version. Some versions of macOS report a different 
patch
+  # version for the system provided interpreter and libraries.
+  if (NOT PYTHON_VERSION_MAJOR VERSION_EQUAL pythonlibs_major OR
+  NOT PYTHON_VERSION_MINOR VERSION_EQUAL pythonlibs_minor)
+message(FATAL_ERROR "Found incompatible Python interpreter 
(${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR})"
+" and Python libraries 
(${pythonlibs_major}.${pythonlibs_minor})")
+  endif()
 endif()
-  endif()
 
-  if (PYTHON_INCLUDE_DIR)
-include_directories(${PYTHON_INCLUDE_DIR})
+if (PYTHON_INCLUDE_DIR)
+  include_directories(${PYTHON_INCLUDE_DIR})
+endif()
   endif()
 endif()
 


Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -221,34 +221,43 @@
 endfunction(find_python_libs_windows)
 
 if (NOT LLDB_DISABLE_PYTHON)
-  if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
-find_python_libs_windows()
-
-if (NOT LLDB_RELOCATABLE_PYTHON)
-  file(TO_CMAKE_PATH "${PYTHON_HOME}" LLDB_PYTHON_HOME)
-  add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
+  if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.13 AND CMAKE_SYSTEM_NAME STREQUAL Windows)
+find_package(Python3 COMPONENTS Interpreter Development REQUIRED)
+if(Python3_VERSION VERSION_LESS 3.5)
+  message(FATAL_ERROR "Python 3.5 or newer is required (found: ${Python3_VERSION})")
 endif()
+set(PYTHON_LIBRARY ${Python3_LIBRARIES})
+include_directories(${Python3_INCLUDE_DIRS})
   else()
-find_package(PythonInterp REQUIRED)
-find_package(PythonLibs REQUIRED)
-  endif()
+if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
+  find_python_libs_windows()
+
+  if (NOT LLDB_RELOCATABLE_PYTHON)
+file(TO_CMAKE_PATH "${PYTHON_HOME}" LLDB_PYTHON_HOME)
+add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
+  en

[Lldb-commits] [PATCH] D68614: [LLDB] Remove standalone build dep on llvm-strip

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lit/CMakeLists.txt:64
   )
+if(NOT LLDB_BUILT_STANDALONE)
+  list(APPEND LLDB_TEST_DEPS llvm-strip)

JDevlieghere wrote:
> xiaobai wrote:
> > why not `if(TARGET llvm-strip)`? I think that expresses the intent more 
> > cleanly (and conforms to the existing pattern).
> I actually ran into an issue with that today, we had the following code in 
> the top-level CMake list:
> 
> ```  
> if(TARGET dsymutil)
> add_lldb_test_dependency(dsymutil)
>   endif()
> ```
> Nevertheless, `ninja lldb-test-deps` didn't build dsymutil.
> 
> ```
> $ /V/J/l/build-ra> ninja lldb-test-deps
> [1/1] Python script sym-linking LLDB Python API
> $ /V/J/l/build-ra> ninja dsymutil
> [1/1] Linking CXX executable bin/dsymutil
> ```
My slight preference for this is to actually have a good way to identify that 
this can be simplified once unified builds are the only supported build style.  
I suppose a comment along with the `if(TARGET)` check would work too.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68614



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

[Lldb-commits] [PATCH] D67954: [LLDB] [Windows] Initial support for ARM64 debugging

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

Honestly, this is just setting up the register context for ARM64.  I dont think 
that there is much of a test for this.  I mean, I suppose you could test this 
by instantiating the context and trying to read it through the interface.  But, 
I question the value of such a test.  Whether you go with the in-process or 
out-of-process approach and whether you are doing DWARF of CodeView debugging 
this is going to be needed.  As to running the test suite - you can 
cross-compile and run the tests remotely.




Comment at: 
lldb/source/Plugins/Process/Windows/Common/arm64/RegisterContextWindows_arm64.cpp:57
+gpr_x21, gpr_x22, gpr_x23, gpr_x24, gpr_x25, gpr_x26, gpr_x27,
+gpr_x28, gpr_fp,  gpr_lr,  gpr_sp,  gpr_pc,  gpr_cpsr};
+

It formats better if you add a trailing comma to the list (`gpr_cpsr,`)



Comment at: 
lldb/source/Plugins/Process/Windows/Common/arm64/RegisterContextWindows_arm64.cpp:64
+fpu_v21, fpu_v22, fpu_v23, fpu_v24, fpu_v25,  fpu_v26, fpu_v27,
+fpu_v28, fpu_v29, fpu_v30, fpu_v31, fpu_fpsr, fpu_fpcr};
+

Similar



Comment at: 
lldb/source/Plugins/Process/Windows/Common/arm64/RegisterContextWindows_arm64.cpp:70
+{"Floating Point Registers", "fpu",
+ llvm::array_lengthof(g_fpu_reg_indices), g_fpu_reg_indices}};
+

Similar


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67954



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

[Lldb-commits] [PATCH] D67911: [LLDB] [Windows] Add missing ifdefs to fix building for non-x86 architectures

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

I think that `printf` is quite an amazing notification :-)




Comment at: 
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:87
   case 4:
 #if defined(__x86_64__) || defined(_M_AMD64)
   case 8:

mstorsjo wrote:
> compnerd wrote:
> > Should this line not also include `|| defined(__aarch64__) || 
> > defined(_M_ARM64)`?
> I guess it should. Or this might be one of the few places where checking for 
> just `_WIN64` might be easiest?
Yeah, I think that `_WIN64` is probably the easiest thing indeed.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67911



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

[Lldb-commits] [PATCH] D67913: [LLDB] [Windows] Map COFF ARM machine ids to the right triplet architectures

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

Nit: `triple`, not `triplet`.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67913



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

[Lldb-commits] [PATCH] D67912: [LLDB] [PECOFF] Recognize arm64 executables

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:202
+  case MachineArm64:
+spec.SetTriple("aarch64-pc-windows");
+specs.Append(module_spec);

Please use `aarch64-unknown-windows`.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67912



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

[Lldb-commits] [PATCH] D67911: [LLDB] [Windows] Add missing ifdefs to fix building for non-x86 architectures

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

What do you think of adding some sort of notification that hardware breakpoints 
are currently unsupported and that we are falling back to software breakpoints 
for the `#else` case?




Comment at: 
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:87
   case 4:
 #if defined(__x86_64__) || defined(_M_AMD64)
   case 8:

Should this line not also include `|| defined(__aarch64__) || 
defined(_M_ARM64)`?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67911



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

[Lldb-commits] [PATCH] D67863: [LLDB] Cast -1 (as invalid socket) to the socket type before comparing

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/source/Host/common/Socket.cpp:479
   }
-  NativeSocket fd = llvm::sys::RetryAfterSignal(-1, ::accept4,
+  NativeSocket fd = llvm::sys::RetryAfterSignal((NativeSocket) -1, ::accept4,
   sockfd, addr, addrlen, flags);

Could you use C++ style casts please?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67863



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

[Lldb-commits] [PATCH] D66858: POSIX DYLD: add workaround for android L loader

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd created this revision.
compnerd added reviewers: davide, xiaobai.
Herald added subscribers: abidh, srhines.
Herald added a project: LLDB.

In certain cases, the loader does not report the base address of the DSO.
Attempt to infer it from the loaded address of the object file.  This was
originally added in the Swift fork of LLDB, this simply is upstreaming the
workaround for the system loader.  Unfortunately, without a recent test case, it
is difficult to construct a test case for this.  However, this problem is also
something which is worked around in the unwinder and has been seen multiple
times previously.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66858

Files:
  source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp


Index: source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -550,6 +550,18 @@

   UpdateBaseAddrIfNecessary(entry, file_path);

+  // On Android L (5.0, 5.1) the base address for the lirbary is sometimes not
+  // reported.  Attempt to discover it based on the load address of the object
+  // file.
+  if (entry.base_addr == 0) {
+lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
+bool is_loaded = false;
+Error error =
+m_process->GetFileLoadAddress(entry.file_spec, is_loaded, load_addr);
+if (error.Success() && is_loaded)
+  entry.base_addr = load_addr;
+  }
+
   return true;
 }



Index: source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
===
--- source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -550,6 +550,18 @@

   UpdateBaseAddrIfNecessary(entry, file_path);

+  // On Android L (5.0, 5.1) the base address for the lirbary is sometimes not
+  // reported.  Attempt to discover it based on the load address of the object
+  // file.
+  if (entry.base_addr == 0) {
+lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
+bool is_loaded = false;
+Error error =
+m_process->GetFileLoadAddress(entry.file_spec, is_loaded, load_addr);
+if (error.Success() && is_loaded)
+  entry.base_addr = load_addr;
+  }
+
   return true;
 }

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

[Lldb-commits] [PATCH] D66445: Explicitly Cast Constants to DWORD

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r369788


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66445



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

[Lldb-commits] [PATCH] D66448: Include "windows" Instead of "Windows"

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r369307


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66448



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

[Lldb-commits] [PATCH] D65798: [lldb][CMake] Infer `Clang_DIR` if not passed explicitly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/cmake/modules/LLDBStandalone.cmake:6
+  # next to LLVM's module directory.
+  set(Clang_DIR ${LLVM_DIR}/../clang)
+  message(STATUS "Inferred Clang_DIR: ${Clang_DIR}")

sgraenitz wrote:
> compnerd wrote:
> > What happens in the standalone clang build scenario?  Can I ask what is the 
> > motivation for this change?  I think it is better to require that the user 
> > pass the path, as that is an explicit dependency of LLDB.
> I don't think there's any side-effects on Clang standalone builds. Is that 
> what you mean with "standalone clang build scenario"?
> 
> I would like top prevent people from writing custom build scripts on top of 
> CMake. Passing a number of very similar paths to CMake, e.g. each time we 
> want to generate a Xcode project for development, this option seems to become 
> compelling quickly. This patch makes standalone configurations simpler. 
> Basically, it provides a default value. I doesn't cut down functionality.
> 
> You can still explicitly pass any path you want. This branch will then not be 
> taken.
I think that the build fragmentation has caused a larger problem, and I would 
like to avoid that.  The standalone build scenario is:

build/llvm
build/clang
build/lldb

In this case, `../clang` does not exist (`../../clang/lib/cmake/clang` does).  
I think what I would suggest instead is adding a cache file that has the 
configuration parameters setup already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65798



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

[Lldb-commits] [PATCH] D65798: [lldb][CMake] Infer `Clang_DIR` if not passed explicitly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/cmake/modules/LLDBStandalone.cmake:6
+  # next to LLVM's module directory.
+  set(Clang_DIR ${LLVM_DIR}/../clang)
+  message(STATUS "Inferred Clang_DIR: ${Clang_DIR}")

What happens in the standalone clang build scenario?  Can I ask what is the 
motivation for this change?  I think it is better to require that the user pass 
the path, as that is an explicit dependency of LLDB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65798



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

[Lldb-commits] [PATCH] D65409: [ProcessWindows] Choose a register context file by prepocessor

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: Common/CMakeLists.txt:28
+target_sources(lldbPluginProcessWindowsCommon PRIVATE
+  x86/RegisterContextWindows_x86.cpp)

At this point, I would say its better to just merge it into the main source 
list.



Comment at: Common/x64/RegisterContextWindows_x64.h:47
+
+#endif // defined(__x86_64__) || defined(__amd64__) || defined(_M_X64) || 
defined(_M_AMD64)

Can you push this inside the include guards please?



Comment at: Common/x86/RegisterContextWindows_x86.h:51
+
+#endif // defined(__i386__) || defined(_M_IX86)

Can you sink the check here inside the include guards please?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D65409



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

[Lldb-commits] [PATCH] D64806: [CMake] Always build debugserver on Darwin and allow tests to use the system's one

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:292
+  else()
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+set(subdir "LLDB.framework/Resources/debugserver")

Can you add a comment explaining that you want to strip leading whitespace?  
Alternatively, if its trailing whitespace, please use 
`OUTPUT_STRIP_TRAILING_WHITESPACE` in the `execute_process` on L288 please.



Comment at: lldb/cmake/modules/AddLLDB.cmake:293
+string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+set(subdir "LLDB.framework/Resources/debugserver")
+set(path_shared "${xcode_dev_dir}/../SharedFrameworks/${subdir}")

I think that `subdir` is a misleading name.  This is the path to debugserver, 
`subdir` should be `LLDB.framework/Resources`.  I don't really have an opinion 
on renaming the variable or composing the variable, that choice is yours :-)



Comment at: lldb/tools/debugserver/source/CMakeLists.txt:258
   endif()
-endif()
+#endif()

sgraenitz wrote:
> Removing the `if() ... endif()` would bloat the diff even more. Would do it 
> in a follow-up NFC commit.
Sure, although ignoring whitespace also is helpful :-)



Comment at: lldb/unittests/CMakeLists.txt:83
   add_subdirectory(debugserver)
 endif()

Shouldn't debugserver not be available always?  It doesn't matter if it isn't 
being used.  Furthermore, elision from the distribution targets will also 
prevent the unnecessary build so there is no need to worry about the default 
builds being longer than necessary.



Comment at: lldb/unittests/tools/lldb-server/CMakeLists.txt:16
 
-if(DEBUGSERVER_PATH)
-  add_definitions(-DLLDB_SERVER="${DEBUGSERVER_PATH}" 
-DLLDB_SERVER_IS_DEBUGSERVER=1)
+if(LLDB_CAN_USE_DEBUGSERVER)
+  if(LLDB_USE_SYSTEM_DEBUGSERVER)

Why is this being checked in `lldb-server`?



Comment at: lldb/unittests/tools/lldb-server/CMakeLists.txt:20
+  else()
+set(debugserver_path $)
+  endif()

Should this not be `debugserver`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64806



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

[Lldb-commits] [PATCH] D64159: [Core] Generalize ValueObject::MaybeCalculateCompleteType

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.

Seems that all the comments have been addressed and this is purely code motion. 
 LGTM


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

https://reviews.llvm.org/D64159



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

[Lldb-commits] [PATCH] D64599: [LanguageRuntime] Move CPPLanguageRuntime into a plugin

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:55
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
+#include "Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h"
 

xiaobai wrote:
> JDevlieghere wrote:
> > What's the benefit of making this a separate plugin, as compared to making 
> > it part of `Plugins/Language/CPlusPlus`? 
> I view LanguageRuntimes as distinct from Languages and thus I think they 
> should go into their own plugins. However, I'm not against moving this to 
> `Plugins/Language/CPlusPlus` if you think it would make more sense to do so 
> for another reason (e.g. less plugins overall?)
We do need the abstraction since there are multiple C++ runtimes: c++, stdc++, 
MSVCPRT, stlport, etc.  Each one is slightly different.  Furthermore, libstdc++ 
supported the GNU and Solaris ABIs, libc++ only does itanium, MSVCPRT only does 
MSVC ABI.  So, we need to have some layer to differentiate between the various 
ABIs and just general C++ language support.


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

https://reviews.llvm.org/D64599



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

[Lldb-commits] [PATCH] D63622: [Target] Hoist LanguageRuntime::GetDeclVendor

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: include/lldb/Target/LanguageRuntime.h:137
 
+  virtual DeclVendor *GetDeclVendor() { return nullptr; }
+

Can this not be `const`?  Seems like retrieving the vendor should not mutate 
the runtime.



Comment at: source/API/SBTarget.cpp:1850
+// Didn't find the type in the symbols; Try the loaded language runtimes
+if (ProcessSP process_sp = target_sp->GetProcessSP()) {
+  for (auto *runtime : process_sp->GetLanguageRuntimes()) {

`auto` should be fine, the type is spelt out.




Comment at: source/API/SBTarget.cpp:1851
+if (ProcessSP process_sp = target_sp->GetProcessSP()) {
+  for (auto *runtime : process_sp->GetLanguageRuntimes()) {
+if (auto vendor = runtime->GetDeclVendor()) {

Can this not be `const auto *runtime`?



Comment at: source/API/SBTarget.cpp:1912
+// Try the loaded language runtimes
+if (ProcessSP process_sp = target_sp->GetProcessSP()) {
+  for (auto *runtime : process_sp->GetLanguageRuntimes()) {

`auto` should be fine, the type is spelt out in the method.


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

https://reviews.llvm.org/D63622



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

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: source/Plugins/Process/Utility/RegisterContextWindows_i386.cpp:40
+
+// clang-format off
+#define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4)   
\

I believe that this bounds the range, and needs to be re-enabled.  Why not 
permit clang-format to reflow `DEFINE_GPR`?



Comment at: source/Plugins/Process/Utility/RegisterContextWindows_i386.cpp:76
+  default:
+assert(false && "Unhandled target architecture.");
+return nullptr;

Could you use `llvm_unreachable` instead please?  Same throughout.



Comment at: source/Plugins/Process/Utility/RegisterContextWindows_i386.cpp:84
+  case llvm::Triple::x86:
+return static_cast(sizeof(g_register_infos_i386) /
+ sizeof(g_register_infos_i386[0]));

Why not `llvm::array_lengthof`?



Comment at: source/Plugins/Process/Utility/RegisterContextWindows_wow64.h:14
+
+class RegisterContextWindows_wow64
+: public lldb_private::RegisterInfoInterface {

Could you spell this `WoW64` to match the Microsoft naming scheme for the 
"proper" noun?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63165



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

[Lldb-commits] [PATCH] D63052: [Target] Remove Process::GetObjCLanguageRuntime

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: include/lldb/Target/ObjCLanguageRuntime.h:202
 
+  static ObjCLanguageRuntime *GetObjCLanguageRuntime(Process &process) {
+return llvm::cast_or_null(

I think it would be nice to just call this `Get` (and we could have equivalents 
in the other languages).  It makes the uses less verbose and repetitive.



Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:515
   if (process_sp && lang_opts.ObjC) {
-if (process_sp->GetObjCLanguageRuntime()) {
-  if (process_sp->GetObjCLanguageRuntime()->GetRuntimeVersion() ==
+if (ObjCLanguageRuntime::GetObjCLanguageRuntime(*process_sp)) {
+  if (ObjCLanguageRuntime::GetObjCLanguageRuntime(*process_sp)

Why not create a variable?

`if (const auto *runtime = ObjCLanguageRuntime::Get(*process))`


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

https://reviews.llvm.org/D63052



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::GetCompilerTypeFromPersistentDecl

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a subscriber: clayborg.
compnerd added a comment.
This revision is now accepted and ready to land.

Would be nice to get someone like @clayborg to chime in, but, I think that 
@labath also seems to think that this is fine.




Comment at: source/Commands/CommandObjectMemory.cpp:476
+for (auto lang : Language::GetSupportedLanguages()) {
+  if (PersistentExpressionState *persistent_vars =
+  target->GetPersistentExpressionStateForLanguage(lang)) {

I think that `auto` would be fine given that the 
`GetPersistentExpressionStateForLanguage` spells out the return type.



Comment at: source/Commands/CommandObjectMemory.cpp:481
+lookup_type_name)) {
+  clang_ast_type = *type;
+  break;

This seems wrong now.  You iterate over all the languages, but always set the 
`clang_ast_type`?  The type may be a Swift AST type or Clang AST type (or 
something more exotic like a rust AST type).  We should rename that to 
`m_ast_type` as per the LLDB style.  But, that makes sense to do as a follow up 
change.


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62812: [llvm] [CodeView] Move Triple::ArchType → CPUType mapping from LLDB

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeView.h:145
+  switch (ArchType) {
+  case Triple::ArchType::aarch64:
+return CPUType::ARM64;

I that `aarch64_be` and `aarch64_32` should be included in this.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeView.h:147
+return CPUType::ARM64;
+  default:
+return CPUType::X64;

This is still wrong.  `x86` and `armv7` are totally legitimate values.  
Furthermore, x86 is still far more prevalent on Windows.


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

https://reviews.llvm.org/D62812



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

This should get the build working again, so lets get this fixed, we can improve 
it later




Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

TomTan wrote:
> TomTan wrote:
> > compnerd wrote:
> > > aganea wrote:
> > > > Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be 
> > > > handled here as well?
> > > I think that we should use a `switch` to cover the targets.  `/Oy` will 
> > > allow FPO on x86 as well.  There is also WoA (ARM32).
> > Seems no, aarch64_be or aarch64_32 is not supported by CodeView or Windows.
> Ok. It makes sense to use switch/case. CodeView doesn't support WoA (ARM32) 
> so no need to add case for it here.
Huh?  How does `cl` generate that then?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::SetCompilerTypeFromPersistentDecl

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: source/Commands/CommandObjectMemory.cpp:479
+if (persistent_vars->SetCompilerTypeFromPersistentDecl(
+lookup_type_name, clang_ast_type))
+  break;

Why is the parameter `clang_ast_type` and not based on 
`persistent_vars->getKind()`?



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp:58
+ConstString type_name, CompilerType &compiler_type) {
+  if (clang::TypeDecl *tdecl = llvm::dyn_cast_or_null(
+  GetPersistentDecl(type_name))) {

NIT: `decl` would be a nicer name


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

aganea wrote:
> Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled here 
> as well?
I think that we should use a `switch` to cover the targets.  `/Oy` will allow 
FPO on x86 as well.  There is also WoA (ARM32).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62771: [LLDBRegisterNum] Update function call llvm::codeview::getRegisterNames(CPUType) in lldb

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a comment.

Generally, `clang-format` the changes, it will catch the formatting things.




Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:28
+  llvm::codeview::CPUType cpu;
+  switch(arch_type) {
+case llvm::Triple::ArchType::aarch64:

Space after the `switch`.



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:35
+default:
+  cpu = llvm::codeview::CPUType::X64;
+  }

Please add a `break` after this.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62771



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