[Lldb-commits] [lldb] d362882 - [LLDB][RISCV] Add RV32FC instruction support for EmulateInstructionRISCV

[via lldb-commits]

Author: Emmmer
Date: 2022-12-06T22:26:51+08:00
New Revision: d3628823c96f2d29ef8cea28f3b1b60ec670f318

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

LOG: [LLDB][RISCV] Add RV32FC instruction support for EmulateInstructionRISCV

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h
lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 7e6ce707cc41c..96d2c3fc1b004 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -497,13 +497,13 @@ static const InstrPattern PATTERNS[] = {
 
 // RVC (Compressed Instructions) //
 {"C_LWSP", 0xE003, 0x4002, DecodeC_LWSP},
-{"C_LDSP", 0xE003, 0x6002, DecodeC_LDSP},
+{"C_LDSP", 0xE003, 0x6002, DecodeC_LDSP, RV64 | RV128},
 {"C_SWSP", 0xE003, 0xC002, DecodeC_SWSP},
-{"C_SDSP", 0xE003, 0xE002, DecodeC_SDSP},
+{"C_SDSP", 0xE003, 0xE002, DecodeC_SDSP, RV64 | RV128},
 {"C_LW", 0xE003, 0x4000, DecodeC_LW},
-{"C_LD", 0xE003, 0x6000, DecodeC_LD},
+{"C_LD", 0xE003, 0x6000, DecodeC_LD, RV64 | RV128},
 {"C_SW", 0xE003, 0xC000, DecodeC_SW},
-{"C_SD", 0xE003, 0xE000, DecodeC_SD},
+{"C_SD", 0xE003, 0xE000, DecodeC_SD, RV64 | RV128},
 {"C_J", 0xE003, 0xA001, DecodeC_J},
 {"C_JR", 0xF07F, 0x8002, DecodeC_JR},
 {"C_JALR", 0xF07F, 0x9002, DecodeC_JALR},
@@ -512,11 +512,11 @@ static const InstrPattern PATTERNS[] = {
 {"C_LI", 0xE003, 0x4001, DecodeC_LI},
 {"C_LUI_ADDI16SP", 0xE003, 0x6001, DecodeC_LUI_ADDI16SP},
 {"C_ADDI", 0xE003, 0x1, DecodeC_ADDI},
-{"C_ADDIW", 0xE003, 0x2001, DecodeC_ADDIW},
+{"C_ADDIW", 0xE003, 0x2001, DecodeC_ADDIW, RV64 | RV128},
 {"C_ADDI4SPN", 0xE003, 0x0, DecodeC_ADDI4SPN},
-{"C_SLLI", 0xE003, 0x2, DecodeC_SLLI},
-{"C_SRLI", 0xEC03, 0x8001, DecodeC_SRLI},
-{"C_SRAI", 0xEC03, 0x8401, DecodeC_SRAI},
+{"C_SLLI", 0xE003, 0x2, DecodeC_SLLI, RV64 | RV128},
+{"C_SRLI", 0xEC03, 0x8001, DecodeC_SRLI, RV64 | RV128},
+{"C_SRAI", 0xEC03, 0x8401, DecodeC_SRAI, RV64 | RV128},
 {"C_ANDI", 0xEC03, 0x8801, DecodeC_ANDI},
 {"C_MV", 0xF003, 0x8002, DecodeC_MV},
 {"C_ADD", 0xF003, 0x9002, DecodeC_ADD},
@@ -524,8 +524,13 @@ static const InstrPattern PATTERNS[] = {
 {"C_OR", 0xFC63, 0x8C41, DecodeC_OR},
 {"C_XOR", 0xFC63, 0x8C21, DecodeC_XOR},
 {"C_SUB", 0xFC63, 0x8C01, DecodeC_SUB},
-{"C_SUBW", 0xFC63, 0x9C01, DecodeC_SUBW},
-{"C_ADDW", 0xFC63, 0x9C21, DecodeC_ADDW},
+{"C_SUBW", 0xFC63, 0x9C01, DecodeC_SUBW, RV64 | RV128},
+{"C_ADDW", 0xFC63, 0x9C21, DecodeC_ADDW, RV64 | RV128},
+// RV32FC //
+{"FLW", 0xE003, 0x6000, DecodeC_FLW, RV32},
+{"FSW", 0xE003, 0xE000, DecodeC_FSW, RV32},
+{"FLWSP", 0xE003, 0x6002, DecodeC_FLWSP, RV32},
+{"FSWSP", 0xE003, 0xE002, DecodeC_FSWSP, RV32},
 
 // RV32F (Extension for Single-Precision Floating-Point) //
 {"FLW", 0x707F, 0x2007, DecodeIType},
@@ -569,9 +574,16 @@ llvm::Optional 
EmulateInstructionRISCV::Decode(uint32_t inst) {
   // check whether the compressed encode could be valid
   uint16_t mask = try_rvc & 0b11;
   bool is_rvc = try_rvc != 0 && mask != 3;
+  uint8_t inst_type = RV64;
+
+  // if we have ArchSpec::eCore_riscv128 in the future,
+  // we also need to check it here
+  if (m_arch.GetCore() == ArchSpec::eCore_riscv32)
+inst_type = RV32;
 
   for (const InstrPattern &pat : PATTERNS) {
-if ((inst & pat.type_mask) == pat.eigen) {
+if ((inst & pat.type_mask) == pat.eigen &&
+(inst_type & pat.inst_type) != 0) {
   LLDB_LOGF(
   log, "EmulateInstructionRISCV::%s: inst(%x at %lx) was decoded to 
%s",
   __FUNCTION__, inst, m_addr, pat.name);

diff  --git a/lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h 
b/lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
index 83542109415b8..1f1d2853ab36d 100644
--- a/lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
+++ b/lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
@@ -299,6 +299,33 @@ RISCVInst DecodeC_ADDW(uint32_t inst) {
   auto rd = DecodeCA_RD(inst);
   return ADDW{rd, rd, DecodeCA_RS2(inst)};
 }
+RISCVInst DecodeC_FLW(uint32_t inst) {
+  uint16_t offset = ((inst << 1) & 0x40)   // imm[6]
+| ((inst >> 7) & 0x38) // imm[5:3]
+| ((inst >> 4) & 0x4); // imm[2]
+  return 

[Lldb-commits] [PATCH] D139390: [LLDB][RISCV] Add RV32FC instruction support for EmulateInstructionRISCV

[Emmmer S via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd3628823c96f: [LLDB][RISCV] Add RV32FC instruction support 
for EmulateInstructionRISCV (authored by Emmmer).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139390

Files:
  lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
  lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
  lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h
  lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp

Index: lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
===
--- lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
+++ lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp
@@ -27,8 +27,8 @@
   RegisterInfoPOSIX_riscv64::FPR fpr;
   uint8_t memory[1024] = {0};
 
-  RISCVEmulatorTester()
-  : EmulateInstructionRISCV(ArchSpec("riscv64-unknown-linux-gnu")) {
+  RISCVEmulatorTester(std::string triple = "riscv64-unknown-linux-gnu")
+  : EmulateInstructionRISCV(ArchSpec(triple)) {
 EmulateInstruction::SetReadRegCallback(ReadRegisterCallback);
 EmulateInstruction::SetWriteRegCallback(WriteRegisterCallback);
 EmulateInstruction::SetReadMemCallback(ReadMemoryCallback);
@@ -356,6 +356,26 @@
   }
 }
 
+class RISCVEmulatorTester32 : public RISCVEmulatorTester {
+public:
+  RISCVEmulatorTester32() : RISCVEmulatorTester("riscv32-unknown-linux-gnu") {}
+};
+
+TEST_F(RISCVEmulatorTester32, TestCDecodeRV32) {
+  std::vector tests = {
+  {0x6002, FLW{Rd{0}, Rs{2}, 0}},
+  {0xE006, FSW{Rs{2}, Rs{1}, 0}},
+  {0x6000, FLW{Rd{8}, Rs{8}, 0}},
+  {0xE000, FSW{Rs{8}, Rs{8}, 0}},
+  };
+
+  for (auto i : tests) {
+auto decode = this->Decode(i.inst);
+ASSERT_TRUE(decode.has_value());
+ASSERT_TRUE(compareInst(decode->decoded, i.inst_type));
+  }
+}
+
 // GEN_BRANCH_TEST(opcode, imm1, imm2, imm3):
 // It should branch for instruction `opcode imm1, imm2`
 // It should do nothing for instruction `opcode imm1, imm3`
Index: lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h
===
--- lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h
+++ lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h
@@ -236,6 +236,10 @@
 FCVT_S_WU, FMV_W_X, FCVT_L_S, FCVT_LU_S, FCVT_S_L, FCVT_S_LU, INVALID,
 EBREAK, RESERVED, HINT, NOP>;
 
+constexpr uint8_t RV32 = 1;
+constexpr uint8_t RV64 = 2;
+constexpr uint8_t RV128 = 4;
+
 struct InstrPattern {
   const char *name;
   /// Bit mask to check the type of a instruction (B-Type, I-Type, J-Type, etc.)
@@ -243,6 +247,8 @@
   /// Characteristic value after bitwise-and with type_mask.
   uint32_t eigen;
   RISCVInst (*decode)(uint32_t inst);
+  /// If not specified, the inst will be supported by all RV versions.
+  uint8_t inst_type = RV32 | RV64 | RV128;
 };
 
 struct DecodeResult {
Index: lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
===
--- lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
+++ lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h
@@ -299,6 +299,33 @@
   auto rd = DecodeCA_RD(inst);
   return ADDW{rd, rd, DecodeCA_RS2(inst)};
 }
+RISCVInst DecodeC_FLW(uint32_t inst) {
+  uint16_t offset = ((inst << 1) & 0x40)   // imm[6]
+| ((inst >> 7) & 0x38) // imm[5:3]
+| ((inst >> 4) & 0x4); // imm[2]
+  return FLW{DecodeCL_RD(inst), DecodeCL_RS1(inst), uint32_t(offset)};
+}
+
+RISCVInst DecodeC_FSW(uint32_t inst) {
+  uint16_t offset = ((inst << 1) & 0x40)   // imm[6]
+| ((inst >> 7) & 0x38) // imm[5:3]
+| ((inst >> 4) & 0x4); // imm[2]
+  return FSW{DecodeCS_RS1(inst), DecodeCS_RS2(inst), uint32_t(offset)};
+}
+
+RISCVInst DecodeC_FLWSP(uint32_t inst) {
+  auto rd = DecodeCI_RD(inst);
+  uint16_t offset = ((inst << 4) & 0xc0)// offset[7:6]
+| ((inst >> 7) & 0x20)  // offset[5]
+| ((inst >> 2) & 0x1c); // offset[4:2]
+  return FLW{rd, Rs{gpr_sp_riscv}, uint32_t(offset)};
+}
+
+RISCVInst DecodeC_FSWSP(uint32_t inst) {
+  uint16_t offset = ((inst >> 1) & 0xc0)// offset[7:6]
+| ((inst >> 7) & 0x3c); // offset[5:2]
+  return FSW{Rs{gpr_sp_riscv}, DecodeCSS_RS2(inst), uint32_t(offset)};
+}
 
 } // namespace lldb_private
 #endif // LLDB_SOURCE_PLUGINS_INSTRUCTION_RISCV_RISCVCINSTRUCTION_H
Index: lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
===
--- lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -497,13 +497,13 @@
 
 // RVC (Compressed Instructions) //
 {"C_LWSP", 0xE

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Looks mechanically fine. These classes are all a bit of a copy paste job right 
now, so if you were going to take inspiration riscv is your best bet.

On the subject of coding style, 
https://lldb.llvm.org/resources/contributing.html.

  Coding Style: LLDB’s code style differs from LLVM’s coding style. 
Unfortunately there is no document describing the differences. Please be 
consistent with the existing code.

Which isn't super helpful but in general look at something central like 
`lldb/source/Target/Process.cpp` and see what it does. The main difference 
you'll see is `variable_names_with_underscores_like_this` and `m_` prefix for 
class members.

For me, the style here looks fine.




Comment at: 
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:58
+   uint32_t reg_num) override;
+  lldb::addr_t ReadPC(bool *success);
+  bool WritePC(lldb::addr_t pc);

I think the older targets use this form but for riscv they went with 
`llvm::Optional ReadPC();` which I prefer over pointer plus 
addr_t.


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

https://reviews.llvm.org/D139158

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

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:765
+m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
+return clang_type.GetTypeName(/*BaseOnly*/ true);
+  }

Michael137 wrote:
> aeubanks wrote:
> > Michael137 wrote:
> > > Michael137 wrote:
> > > > Ok so what we're doing is:
> > > > 1. Create a `ClassTemplateSpecializationDecl` with an empty basename
> > > > 2. Return the type-name and since the basename is empty we end up with 
> > > > just the template arguments
> > > > 
> > > > Why can't we just call `GetTemplateParametersString(die)` instead of 
> > > > creating this function?
> > > Can confirm that this works locally. Though required moving that function 
> > > out of the DWARFASTParserClang, which seems OK
> > `GetTemplateParametersString` is specifically only used for 
> > `GetCPlusPlusQualifiedName`, which is used below
> > ```
> >   // For C++, we rely solely upon the one definition rule that says
> >   // only one thing can exist at a given decl context. We ignore the
> >   // file and line that things are declared on.
> >   std::string qualified_name = GetCPlusPlusQualifiedName(die);
> > ```
> > so it doesn't have to match clang's printing. but for the simple template 
> > name stuff, we are comparing clang-generated names, so everything needs to 
> > go through clang.
> > 
> > I've replaced `GetTemplateParametersString` with this code that goes 
> > through clang as well
> But didn't `GetTemplateParametersString` go through Clang's type printer too? 
> Probably missing something.
> What would be an example of the difference in output that could arise between 
> the new vs. old method?
one thing that I was aware of even initially implementing 
`GetTemplateParametersString` was spacing between two `>`, clang will add a 
space (unless you tell it not to), whereas the naive implementation didn't


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138834

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

[Lldb-commits] [PATCH] D139054: Delay calling ObjC class list read utility functions very early in process startup

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: lldb/tools/debugserver/source/RNBRemote.h:111-139
+prefix_reg_packets_with_tid,// 'QPrefixRegisterPacketsWithThreadID
+set_logging_mode,   // 'QSetLogging:'
+set_ignored_exceptions, // 'QSetIgnoredExceptions'
+set_max_packet_size,// 'QSetMaxPacketSize:'
+set_max_payload_size,   // 'QSetMaxPayloadSize:'
+set_environment_variable,   // 'QEnvironment:'
+set_environment_variable_hex,   // 'QEnvironmentHexEncoded:'

Nit, since you're touching this, why not align it with `'QStartNoAckMode'`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139054

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

[Lldb-commits] [PATCH] D139054: Delay calling ObjC class list read utility functions very early in process startup

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/tools/debugserver/source/RNBRemote.h:111-139
+prefix_reg_packets_with_tid,// 'QPrefixRegisterPacketsWithThreadID
+set_logging_mode,   // 'QSetLogging:'
+set_ignored_exceptions, // 'QSetIgnoredExceptions'
+set_max_packet_size,// 'QSetMaxPacketSize:'
+set_max_payload_size,   // 'QSetMaxPayloadSize:'
+set_environment_variable,   // 'QEnvironment:'
+set_environment_variable_hex,   // 'QEnvironmentHexEncoded:'

JDevlieghere wrote:
> Nit, since you're touching this, why not align it with `'QStartNoAckMode'`?
clang-format decided to go on a little adventure here, when I added a line to 
this table.  I didn't want to fight with it, so I just let it have its fun.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139054

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

[Lldb-commits] [PATCH] D139453: Switch to a different library-loaded notification function breakpoint in Darwin dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: jingham, JDevlieghere.
jasonmolenda added a project: LLDB.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

Darwin's dynamic linker, dyld, calls a no-op function whenever libraries are 
loaded or removed from a process.  There are two of these functions - one for 
older compatibility, which lldb is using, and actual one it calls first.

In a future patch, I'll change how lldb discovers the address of this function. 
 lldb is currently not using the "canonical" one that I'll discover that way, 
and their arguments are slightly different, so this patch lays the groundwork 
by switching to this canonical notification function with the same discovery 
mechanism.

The important difference between these two is that the old function (lldb 
currently uses) is passed an array of uint64_t addresses of binary mach-o 
headers.  In the new, canonical function, the headers are passed as an array of 
ptrsize -- so 32-bits, on a target like arm64_32 which is ILP32 (we don't have 
any genuine 32-bit targets that we still support).  Beyond that, it's only a 
different function name.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139453

Files:
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp


Index: lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===
--- lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -206,10 +206,15 @@
  lldb::user_id_t break_loc_id) {
   // Let the event know that the images have changed
   // DYLD passes three arguments to the notification breakpoint.
-  // Arg1: enum dyld_notify_mode mode - 0 = adding, 1 = removing, 2 = remove
-  // all Arg2: unsigned long icount- Number of shared libraries
-  // added/removed Arg3: uint64_t mach_headers[] - Array of load addresses
-  // of binaries added/removed
+  // 
+  // Arg1: enum dyld_notify_mode mode
+  // 0 = adding, 1 = removing, 2 = remove all, 3 = dyld moved
+  //
+  // Arg2: unsigned long count
+  // Number of shared libraries added/removed 
+  //
+  // Arg3: struct dyld_image_info mach_headers[]
+  // Array of load addresses of binaries added/removed
 
   DynamicLoaderMacOS *dyld_instance = (DynamicLoaderMacOS *)baton;
 
@@ -239,9 +244,10 @@
 ValueList argument_values;
 
 Value mode_value;// enum dyld_notify_mode { dyld_notify_adding=0,
- // dyld_notify_removing=1, dyld_notify_remove_all=2 };
+ // dyld_notify_removing=1, dyld_notify_remove_all=2,
+ // dyld_notify_dyld_moved=3 };
 Value count_value;   // unsigned long count
-Value headers_value; // uint64_t machHeaders[] (aka void*)
+Value headers_value; // struct dyld_image_info machHeaders[]
 
 CompilerType clang_void_ptr_type =
 clang_ast_context->GetBasicType(eBasicTypeVoid).GetPointerType();
@@ -270,6 +276,9 @@
 argument_values.PushValue(count_value);
 argument_values.PushValue(headers_value);
 
+// void lldb_image_notifier(enum dyld_image_mode mode, uint32_t infoCount,
+// const dyld_image_info info[])
+
 if (abi->GetArgumentValues(exe_ctx.GetThreadRef(), argument_values)) {
   uint32_t dyld_mode =
   argument_values.GetValueAtIndex(0)->GetScalar().UInt(-1);
@@ -283,11 +292,26 @@
   argument_values.GetValueAtIndex(2)->GetScalar().ULongLong(-1);
   if (header_array != static_cast(-1)) {
 std::vector image_load_addresses;
+
+// struct dyld_image_info_32 {
+// uint32_timageLoadAddress;
+// uint32_timageFilePath;
+// uint32_timageFileModDate;
+// };
+// struct dyld_image_info_64 {
+// uint64_timageLoadAddress;
+// uint64_timageFilePath;
+// uint64_timageFileModDate;
+// };
+
+int addr_size =
+process->GetTarget().GetArchitecture().GetAddressByteSize();
 for (uint64_t i = 0; i < image_infos_count; i++) {
   Status error;
-  addr_t addr = process->ReadUnsignedIntegerFromMemory(
-  header_array + (8 * i), 8, LLDB_INVALID_ADDRESS, error);
-  if (addr != LLDB_INVALID_ADDRESS) {
+  addr_t dyld_image_info = header_array + (3 * addr_size * i);
+  addr_t addr =
+  process->ReadPointerFromMemory(dyld_image_info, error);
+  if (error.Success()) {
 image_load_addresses.push_back(add

[Lldb-commits] [PATCH] D139453: Switch to a different library-loaded notification function breakpoint in Darwin dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

oops, slight misstatement from not reading the patch when I wrote the 
description.  The function we currently use has an array of uint64_t's, the one 
I am switching to is given an array of dyld_image_info objects, the first 
element has the ptrsize address of the mach-o header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139453

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

[Lldb-commits] [lldb] 5951b0b - Store OptTable::Info::Name as a StringRef

[via lldb-commits]

Author: serge-sans-paille
Date: 2022-12-06T22:51:28+01:00
New Revision: 5951b0bb23f3265bea16f28c2af9d278b9d829c6

URL: 
https://github.com/llvm/llvm-project/commit/5951b0bb23f3265bea16f28c2af9d278b9d829c6
DIFF: 
https://github.com/llvm/llvm-project/commit/5951b0bb23f3265bea16f28c2af9d278b9d829c6.diff

LOG: Store OptTable::Info::Name as a StringRef

This is a recommit of 8ae18303f97d5dcfaecc90b4d87effb2011ed82e,
with a few cleanups.

This avoids implicit conversion to StringRef at several points, which in
turns avoid redundant calls to strlen.

As a side effect, this greatly simplifies the implementation of
StrCmpOptionNameIgnoreCase.

It also eventually gives a consistent, humble speedup in compilation
time (timing updated since original commit).

https://llvm-compile-time-tracker.com/compare.php?from=de4b6a1bc64db33643f001ad45fae7b92b4a4688&to=c23a93d1292052b4be2fbe8c586fa31143d0c7ed&stat=instructions:u

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

Added: 


Modified: 
clang/lib/Driver/ToolChains/Gnu.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
llvm/include/llvm/ADT/StringRef.h
llvm/include/llvm/Option/OptTable.h
llvm/lib/Option/OptTable.cpp
llvm/unittests/Option/OptionMarshallingTest.cpp
llvm/utils/TableGen/OptParserEmitter.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/Gnu.cpp 
b/clang/lib/Driver/ToolChains/Gnu.cpp
index 4621850f13772..60d62e2b9c5c1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -331,8 +331,8 @@ static bool getStaticPIE(const ArgList &Args, const 
ToolChain &TC) {
   if (HasStaticPIE && Args.hasArg(options::OPT_nopie)) {
 const Driver &D = TC.getDriver();
 const llvm::opt::OptTable &Opts = D.getOpts();
-const char *StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
-const char *NoPIEName = Opts.getOptionName(options::OPT_nopie);
+StringRef StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
+StringRef NoPIEName = Opts.getOptionName(options::OPT_nopie);
 D.Diag(diag::err_drv_cannot_mix_options) << StaticPIEName << NoPIEName;
   }
   return HasStaticPIE;

diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp 
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index 9d89148616be1..fde098840be4b 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1055,7 +1055,7 @@ void 
PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
   // Only add the version-min options if we got a version from somewhere
   if (!version.empty() && sdk_type != XcodeSDK::Type::Linux) {
 #define OPTION(PREFIX, NAME, VAR, ...) 
\
-  const char *opt_##VAR = NAME;
\
+  llvm::StringRef opt_##VAR = NAME;
\
   (void)opt_##VAR;
 #include "clang/Driver/Options.inc"
 #undef OPTION

diff  --git a/llvm/include/llvm/ADT/StringRef.h 
b/llvm/include/llvm/ADT/StringRef.h
index 032f42a51ec0b..9fea390c2ed32 100644
--- a/llvm/include/llvm/ADT/StringRef.h
+++ b/llvm/include/llvm/ADT/StringRef.h
@@ -561,7 +561,8 @@ namespace llvm {
 /// \param N The number of characters to included in the substring. If N
 /// exceeds the number of characters remaining in the string, the string
 /// suffix (starting with \p Start) will be returned.
-[[nodiscard]] StringRef substr(size_t Start, size_t N = npos) const {
+[[nodiscard]] constexpr StringRef substr(size_t Start,
+ size_t N = npos) const {
   Start = std::min(Start, Length);
   return StringRef(Data + Start, std::min(N, Length - Start));
 }

diff  --git a/llvm/include/llvm/Option/OptTable.h 
b/llvm/include/llvm/Option/OptTable.h
index 07d9870f71b33..e884ebeb788c4 100644
--- a/llvm/include/llvm/Option/OptTable.h
+++ b/llvm/include/llvm/Option/OptTable.h
@@ -44,7 +44,7 @@ class OptTable {
 /// A null terminated array of prefix strings to apply to name while
 /// matching.
 const char *const *Prefixes;
-const char *Name;
+StringRef Name;
 const char *HelpText;
 const char *MetaVar;
 unsigned ID;
@@ -102,9 +102,7 @@ class OptTable {
   const Option getOption(OptSpecifier Opt) const;
 
   /// Lookup the name of the given option.
-  const char *getOptionName(OptSpecifier id) const {
-return getInfo(id).Name;
-  }
+  StringRef getOptionName(OptSpecifier id) const { return getInfo(id).Name; }
 
   /// Get the kind of the given option.
   unsigned getOptionKind(OptSpecifier id) const {
@@ -184,7 +182,7 @@ class OptTable {
   ///  takes
   ///
   /// \return true in success, and false in fail.
-  bool addValues(const char *Option, const char *Values);
+  bool addValues(StringRef Option, const char *Values);
 
   //

[Lldb-commits] [PATCH] D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

The idea of the patch seems fine to me. One thing that I thought about is 
creating a `PlatformSpec` struct that can contain all the needed configuration 
for `CreateInstance`/`GetOrCreate`. What do you think of this?




Comment at: lldb/source/API/SBDebugger.cpp:1501-1502
   PlatformList &platforms = m_opaque_sp->GetPlatformList();
-  if (PlatformSP platform_sp = platforms.GetOrCreate(platform_name_cstr))
+  if (PlatformSP platform_sp =
+  platforms.GetOrCreate(platform_name_cstr, nullptr))
 platforms.SetSelectedPlatform(platform_sp);

You could add a small comment indicating what parameter the `nullptr` is 
standing in for.



Comment at: lldb/source/API/SBPlatform.cpp:298
 
-  m_opaque_sp = Platform::Create(platform_name);
+  m_opaque_sp = Platform::Create(platform_name, nullptr, nullptr);
+}

Here too



Comment at: lldb/source/Interpreter/OptionGroupPlatform.cpp:13
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
 #include "lldb/Target/Platform.h"

Is it `OptionGroupPythonClassWithDict.h` you want to include here? It looks 
more like you'd want to include `ScriptedMetadata.h`



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:514
   process->GetTarget().GetDebugger().GetPlatformList().Create(
-  PlatformDarwinKernel::GetPluginNameStatic());
+  PlatformDarwinKernel::GetPluginNameStatic(), nullptr);
   if (platform_sp.get())

Add a small comment here for `nullptr` as well.



Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:973
   process->GetTarget().GetDebugger().GetPlatformList().Create(
-  PlatformDarwinKernel::GetPluginNameStatic());
+  PlatformDarwinKernel::GetPluginNameStatic(), nullptr);
   if (platform_sp)

Here too.



Comment at: lldb/source/Target/Platform.cpp:166-199
-// PlatformSP
-// Platform::FindPlugin (Process *process, ConstString plugin_name)
-//{
-//PlatformCreateInstance create_callback = nullptr;
-//if (plugin_name)
-//{
-//create_callback  =

Yipee! :)



Comment at: lldb/source/Target/Platform.cpp:2042-2043
   for (const ArchSpec &arch : archs) {
-if (PlatformSP platform = GetOrCreate(arch, process_host_arch, nullptr))
+if (PlatformSP platform =
+GetOrCreate(arch, process_host_arch, nullptr, nullptr))
   candidates.push_back(platform);

Here too



Comment at: lldb/source/Target/Platform.cpp:2081
 ArchSpec arch;
-PlatformSP platform_sp = create_callback(true, &arch);
+PlatformSP platform_sp = create_callback(true, &arch, &m_debugger, 
nullptr);
 if (platform_sp) {

Here too.



Comment at: lldb/source/Target/Process.cpp:2933
   platform_sp = GetTarget().GetDebugger().GetPlatformList().GetOrCreate(
-  target_arch, process_host_arch, &platform_arch);
+  target_arch, process_host_arch, &platform_arch, nullptr);
   if (platform_sp) {

Here too.



Comment at: lldb/source/Target/Target.cpp:1505-1506
 if (PlatformSP arch_platform_sp =
-GetDebugger().GetPlatformList().GetOrCreate(other, {},
-&platform_arch)) {
+GetDebugger().GetPlatformList().GetOrCreate(
+other, {}, &platform_arch, nullptr)) {
   SetPlatform(arch_platform_sp);

Here



Comment at: lldb/source/Target/TargetList.cpp:188
 if (PlatformSP platform_for_archs_sp =
-platform_list.GetOrCreate(archs, {}, candidates)) {
+platform_list.GetOrCreate(archs, {}, candidates, nullptr)) {
   platform_sp = platform_for_archs_sp;

Here



Comment at: lldb/source/Target/TargetList.cpp:221-222
 arch, {}, ArchSpec::CompatibleMatch, nullptr)) {
-  platform_sp = platform_list.GetOrCreate(arch, {}, &platform_arch);
+  platform_sp =
+  platform_list.GetOrCreate(arch, {}, &platform_arch, nullptr);
   if (platform_sp)

Here



Comment at: lldb/source/Target/TargetList.cpp:233
+  platform_sp = platform_list.GetOrCreate(platform_arch, {},
+  &fixed_platform_arch, nullptr);
   if (platform_sp)

H



Comment at: lldb/source/Target/TargetList.cpp:265
+  platform_sp = debugger.GetPlatformList().GetOrCreate(specified_arch, {},
+   &arch, nullptr);
   }

H


CHANGES SINCE LAST ACTION
  https:/

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/bindings/python/python-wrapper.swig:353-354
+  } else {
+error_string.assign("wrong number of arguments in __init__, should be 2 "
+"(not including self)");
+  }

I think the error string needs to be adjusted here. It should say `should be 1 
"` based on line 350.



Comment at: lldb/examples/python/scripted_process/scripted_platform.py:36-39
+pid = 420
+parent_pid = 42 (optional)
+uid = 0 (optional)
+gid = 0 (optional)

Each line here needs to have a comma at the end of it.



Comment at: lldb/examples/python/scripted_process/scripted_platform.py:43-46
+Dict: The processes represented as a dictionary, with at least the
+process ID, name, architecture. Optionally, the user can also
+provide the parent process ID and the user and group IDs.
+The dictionary can be empty.

I think you could add a little more information here. Based on the example 
below with `MyScriptedPlatform` it looks like the Dictionary maps `PID (int)` 
to `Process information (dict)`. However, what you've written here doesn't 
indicate that. You could change the example above to be something like:

```
processes = {
420: {
name: a.out,
arch: aarch64,
pid: 420,
parent_pid: 42 (optional),
uid: 0 (optional),
gid: 0 (optional),
},
}


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

https://reviews.llvm.org/D139250

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

[Lldb-commits] [PATCH] D139461: Fix dwarf5-lazy-dwo.c for the default c target not being c99.

[Mitch Phillips via Phabricator via lldb-commits]

hctim created this revision.
hctim added reviewers: Eric, jankratochvil, herhut.
Herald added a project: All.
hctim requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

My host compiler is clang version 15.0.0, which uses -std=c11 by
default. The test asserts that the language is 'c99', and so the test
fails locally.

Update the test to be explicit about compiling with 'c99'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139461

Files:
  lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c


Index: lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
===
--- lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
+++ lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
@@ -3,11 +3,11 @@
 // -gsplit-dwarf is supported only on Linux.
 // REQUIRES: system-linux
 
-// RUN: %clang_host %s -fno-standalone-debug -g \
+// RUN: %clang_host %s -fno-standalone-debug -g -std=c99 \
 // RUN:   -gdwarf-5 -gpubnames -gsplit-dwarf -c -o %t1.o -DONE
-// RUN: %clang_host %s -fno-standalone-debug -g \
+// RUN: %clang_host %s -fno-standalone-debug -g -std=c99 \
 // RUN:   -gdwarf-5 -gpubnames -gsplit-dwarf -c -o %t2.o -DTWO
-// RUN: %clang_host %t1.o %t2.o -o %t
+// RUN: %clang_host %t1.o %t2.o -o %t -std=c99
 // RUN: %lldb %t -o "log enable 'lldb' object" -o "settings set 
stop-line-count-before 0" \
 // RUN:   -o "b main" -o "run" -o "image lookup -n main -v" -b | FileCheck %s
 


Index: lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
===
--- lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
+++ lldb/test/Shell/SymbolFile/DWARF/dwarf5-lazy-dwo.c
@@ -3,11 +3,11 @@
 // -gsplit-dwarf is supported only on Linux.
 // REQUIRES: system-linux
 
-// RUN: %clang_host %s -fno-standalone-debug -g \
+// RUN: %clang_host %s -fno-standalone-debug -g -std=c99 \
 // RUN:   -gdwarf-5 -gpubnames -gsplit-dwarf -c -o %t1.o -DONE
-// RUN: %clang_host %s -fno-standalone-debug -g \
+// RUN: %clang_host %s -fno-standalone-debug -g -std=c99 \
 // RUN:   -gdwarf-5 -gpubnames -gsplit-dwarf -c -o %t2.o -DTWO
-// RUN: %clang_host %t1.o %t2.o -o %t
+// RUN: %clang_host %t1.o %t2.o -o %t -std=c99
 // RUN: %lldb %t -o "log enable 'lldb' object" -o "settings set stop-line-count-before 0" \
 // RUN:   -o "b main" -o "run" -o "image lookup -n main -v" -b | FileCheck %s
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/scripted_process/scripted_platform.py:43-46
+Dict: The processes represented as a dictionary, with at least the
+process ID, name, architecture. Optionally, the user can also
+provide the parent process ID and the user and group IDs.
+The dictionary can be empty.

bulbazord wrote:
> I think you could add a little more information here. Based on the example 
> below with `MyScriptedPlatform` it looks like the Dictionary maps `PID (int)` 
> to `Process information (dict)`. However, what you've written here doesn't 
> indicate that. You could change the example above to be something like:
> 
> ```
> processes = {
> 420: {
> name: a.out,
> arch: aarch64,
> pid: 420,
> parent_pid: 42 (optional),
> uid: 0 (optional),
> gid: 0 (optional),
> },
> }
Good point!


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

https://reviews.llvm.org/D139250

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

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo updated this revision to Diff 480619.
ayermolo added a comment.

fixed test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139379

Files:
  bolt/lib/Core/DebugData.cpp
  bolt/lib/Rewrite/DWARFRewriter.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
  llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h
  llvm/lib/DWP/DWP.cpp
  llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
  llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp
  llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
  llvm/test/DebugInfo/X86/debug-cu-index-unknown-section.s
  llvm/test/DebugInfo/X86/dwp-v2-cu-index.s
  llvm/test/DebugInfo/X86/dwp-v2-tu-index.s
  llvm/test/DebugInfo/X86/dwp-v5-cu-index.s
  llvm/test/DebugInfo/X86/dwp-v5-tu-index.s
  llvm/test/DebugInfo/dwarfdump-dwp.test
  llvm/test/tools/llvm-dwp/X86/debug_macro_v5.s
  llvm/test/tools/llvm-dwp/X86/info-v5.s
  llvm/test/tools/llvm-dwp/X86/loclists.s
  llvm/test/tools/llvm-dwp/X86/merge.test
  llvm/test/tools/llvm-dwp/X86/rnglists.s
  llvm/test/tools/llvm-dwp/X86/simple.test
  llvm/test/tools/llvm-dwp/X86/tu_units_v5.s
  llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

Index: llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
===
--- llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
+++ llvm/test/tools/llvm-dwp/X86/unknown-section-id.s
@@ -17,7 +17,7 @@
 # CHECK:  Index Signature INFO ABBREV
 # CHECK-NOT:  Unknown
 # CHECK:  -
-# CHECK-NEXT: 1 0x1122 [0x, 0x0014) [0x, 0x0009)
+# CHECK-NEXT: 1 0x1122 [0x, 0x0014) [0x, 0x0009)
 # CHECK-NOT:  [
 
 # CHECK:  .debug_tu_index contents:
@@ -25,7 +25,7 @@
 # CHECK:  Index Signature TYPES ABBREV
 # CHECK-NOT:  Unknown
 # CHECK:  -
-# CHECK-NEXT: 2 0x1133 [0x, 0x0019) [0x0009, 0x0014)
+# CHECK-NEXT: 2 0x1133 [0x, 0x0019) [0x0009, 0x0014)
 # CHECK-NOT:  [
 
 .section .debug_abbrev.dwo, "e", @progbits
Index: llvm/test/tools/llvm-dwp/X86/tu_units_v5.s
===
--- llvm/test/tools/llvm-dwp/X86/tu_units_v5.s
+++ llvm/test/tools/llvm-dwp/X86/tu_units_v5.s
@@ -13,9 +13,9 @@
 # CHECK: 0x001b: Type Unit: length = 0x0017, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_type, abbr_offset = 0x, addr_size = 0x08, name = '', type_signature = [[TUID2:.*]], type_offset = 0x0019 (next unit at 0x0036)
 # CHECK-DAG: .debug_tu_index contents:
 # CHECK: version = 5, units = 2, slots = 4
-# CHECK: Index Signature  INFO ABBREV
-# CHECK: 1 [[TUID1]]  [0x, 0x001b) [0x, 0x0010)
-# CHECK: 4 [[TUID2]]  [0x001b, 0x0036) [0x, 0x0010)
+# CHECK: Index Signature  INFO ABBREV
+# CHECK: 1 [[TUID1]]  [0x, 0x001b) [0x, 0x0010)
+# CHECK: 4 [[TUID2]]  [0x001b, 0x0036) [0x, 0x0010)
 
 .section	.debug_info.dwo,"e",@progbits
 .long	.Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
Index: llvm/test/tools/llvm-dwp/X86/simple.test
===
--- llvm/test/tools/llvm-dwp/X86/simple.test
+++ llvm/test/tools/llvm-dwp/X86/simple.test
@@ -28,9 +28,9 @@
 CHECK: DW_TAG_formal_parameter
 
 CHECK: .debug_info.dwo contents:
-CHECK: [[AOFF:0x[0-9a-f]*]]:
+CHECK: 0x[[#%.8x,AOFF:]]:
 CHECK-LABEL: Compile Unit: length = {{.*}}, version = 0x0004, abbr_offset =
-CHECK: 0x[[AAOFF]], addr_size = 0x08 (next unit at [[BOFF:.*]])
+CHECK: 0x[[AAOFF]], addr_size = 0x08 (next unit at 0x[[#%.8x,BOFF:]])
 CHECK: DW_TAG_compile_unit
 CHECK:   DW_AT_name {{.*}} "a.cpp"
 CHECK:   DW_AT_GNU_dwo_id {{.*}} ([[DWOA:.*]])
@@ -40,9 +40,9 @@
 NOTYP: DW_AT_name {{.*}} "foo"
 TYPES: DW_AT_signature {{.*}} ([[FOOSIG:.*]])
 
-CHECK: [[BOFF]]:
+CHECK: 0x[[#BOFF]]:
 CHECK-LABEL: Compile Unit: length = {{.*}}, version = 0x0004, abbr_offset =
-CHECK: 0x[[BAOFF]], addr_size = 0x08 (next unit at [[XOFF:.*]])
+CHECK: 0x[[BAOFF]], addr_size = 0x08 (next unit at 0x[[#%.8x,XOFF:]])
 CHECK:   DW_AT_name {{.*}} "b.cpp"
 CHECK:   DW_AT_GNU_dwo_id {{.*}} ([[DWOB:.*]])
 CHECK:   DW_TAG_structure_type
@@ -54,32 +54,32 @@
 
 NOTYP-NOT: .debug_types.dwo contents:
 TYPES-LABEL: .debug_types.dwo contents:
-TYPES: [[FOOUOFF:0x[0-9a-f]*]]:
+TYPES: 0x[[#%.8x,FOOUOFF:]]:
 TYPES-LABEL: Type Unit: length = 0x0020, format = DWARF32, version = 0x0004, abbr_offset =
-TYPES: 0x[[AAOFF]], addr_size = 0x08, name = 'foo', type_signature = [[FOOSIG]], type_offset = 0

[Lldb-commits] [PATCH] D139463: Fix breakpoint-command.test when no script interpreter is compiled in.

[Mitch Phillips via Phabricator via lldb-commits]

hctim created this revision.
hctim added reviewers: JDevlieghere, labath.
Herald added a project: All.
hctim requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

My local build is with -DLLVM_ENABLE_PROJECTS=lldb, but I don't compile
with -DLLDB_ENABLE_PYTHON=True or -DLLDB_ENABLE_LUA=True. This results
in there being no script interpreter.

The test lldb/test/Shell/Breakpoint/breakpoint-command.test has an
implicit dependency on a script interpreter being available.

This patch makes that dependency clear. If you have a script
interpreter, the test gets run, otherwise it gets skipped. This means
that folks (like me) who naively use -DLLVM_ENABLE_PROJECTS=lldb can
continue to run check-all without breakages.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139463

Files:
  lldb/source/Plugins/ScriptInterpreter/CMakeLists.txt
  lldb/test/CMakeLists.txt
  lldb/test/Shell/Breakpoint/breakpoint-command.test
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in


Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -28,6 +28,8 @@
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", 
"lldb-shell")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", 
"lldb-shell")
 
+config.has_script_interpreter = @LLDB_HAS_SCRIPT_INTERPRETER@
+
 import lit.llvm
 lit.llvm.initialize(lit_config, config)
 
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -135,6 +135,9 @@
 if config.have_lldb_server:
 config.available_features.add('lldb-server')
 
+if config.has_script_interpreter:
+config.available_features.add('script-interpreter')
+
 # NetBSD permits setting dbregs either if one is root
 # or if user_set_dbregs is enabled
 can_set_dbregs = True
Index: lldb/test/Shell/Breakpoint/breakpoint-command.test
===
--- lldb/test/Shell/Breakpoint/breakpoint-command.test
+++ lldb/test/Shell/Breakpoint/breakpoint-command.test
@@ -1,3 +1,4 @@
+# REQUIRES: script-interpreter
 # RUN: %build %p/Inputs/dummy-target.c -o %t.out
 # RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 
+ 126)"' -o 'r' | FileCheck %s
 
Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -177,7 +177,8 @@
   LLDB_HAS_LIBCXX
   LLDB_TOOL_LLDB_SERVER_BUILD
   LLDB_USE_SYSTEM_DEBUGSERVER
-  LLDB_IS_64_BITS)
+  LLDB_IS_64_BITS
+  LLDB_HAS_SCRIPT_INTERPRETER)
 
 # Configure the individual test suites.
 add_subdirectory(API)
Index: lldb/source/Plugins/ScriptInterpreter/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/CMakeLists.txt
@@ -1,8 +1,11 @@
+set(LLDB_HAS_SCRIPT_INTERPRETER FALSE)
 add_subdirectory(None)
 if (LLDB_ENABLE_PYTHON)
+  set(LLDB_HAS_SCRIPT_INTERPRETER TRUE)
   add_subdirectory(Python)
 endif()
 
 if (LLDB_ENABLE_LUA)
+  set(LLDB_HAS_SCRIPT_INTERPRETER TRUE)
   add_subdirectory(Lua)
 endif()


Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -28,6 +28,8 @@
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-shell")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-shell")
 
+config.has_script_interpreter = @LLDB_HAS_SCRIPT_INTERPRETER@
+
 import lit.llvm
 lit.llvm.initialize(lit_config, config)
 
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -135,6 +135,9 @@
 if config.have_lldb_server:
 config.available_features.add('lldb-server')
 
+if config.has_script_interpreter:
+config.available_features.add('script-interpreter')
+
 # NetBSD permits setting dbregs either if one is root
 # or if user_set_dbregs is enabled
 can_set_dbregs = True
Index: lldb/test/Shell/Breakpoint/breakpoint-command.test
===
--- lldb/test/Shell/Breakpoint/breakpoint-command.test
+++ lldb/test/Shell/Breakpoint/breakpoint-command.test
@@ -1,3 +1,4 @@
+# REQUIRES: script-interpreter
 # RUN: %build %p/Inputs/dummy-target.c -o %t.out
 # RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 + 126)"' -o 'r' | FileCheck %s
 
Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.

[Lldb-commits] [PATCH] D139251: [lldb/Interpreter] Introduce ScriptedPlatform{, Python}Interface

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptedPlatformInterface.h:14
+#include "lldb/Interpreter/ScriptedInterface.h"
+#include "lldb/Target/MemoryRegionInfo.h"
+

What is `MemoryRegionInfo.h` for?



Comment at: lldb/include/lldb/Interpreter/ScriptedPlatformInterface.h:36-42
+  virtual Status LaunchProcess() {
+return Status("ScriptedPlatform cannot launch process");
+  }
+
+  virtual Status KillProcess(lldb::pid_t pid) {
+return Status("ScriptedPlatform cannot kill process");
+  }

The status messages should probably refer to `ScriptedPlatformInterface` 
instead of `ScriptedPlatform` to be clearer.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPlatformPythonInterface.cpp:1-2
+//===-- ScriptedPlatformPythonInterface.cpp
+//===//
+//

This header is also broken.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPlatformPythonInterface.h:1-2
+//===-- ScriptedPlatformPythonInterface.h *- C++
+//-*-===//
+//

The header is a little broken here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139251

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

[Lldb-commits] [PATCH] D139252: [lldb/Plugins] Introduce Scripted Platform Plugin

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

There's some overlap in implementation between 
`ScriptedPlatform::GetProcessInfo` and `ScriptedPlatform::FindProcesses`. If 
you don't anticipate these diverging dramatically, it might make sense to 
extract out common functionality into something like 
`ScriptedPlatform::ExtractProcessInfoFromDict` (or something to that effect).




Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp:1
+#include "ScriptedPlatform.h"
+

This file needs a header.



Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp:66-67
+
+  if (!metadata)
+return {};
+

This was already checked above on line 60/61.



Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp:143
+
+  if (host_os == llvm::Triple::MacOSX) {
+// We can't use x86GetSupportedArchitectures() because it uses

This should be unconditionally true because you set `host_os` to 
`llvm::Triple::MacOSX` right above this.



Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h:1
+//===-- PlatformPOSIX.h -*- C++ 
-*-===//
+//

`PlatformPOSIX.h` -> `ScriptedPlatform.h`


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

https://reviews.llvm.org/D139252

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

[Lldb-commits] [PATCH] D139484: [lldb/test] Fix data racing issue in TestStackCoreScriptedProcess

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: JDevlieghere, bulbazord.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

This patch should fix an undeterministic error in TestStackCoreScriptedProcess.

In order to test both the multithreading capability and shared library
loading in Scripted Processes, the test would create multiple threads
that would take the same variable as a reference.

The first thread would alter the value and the second thread would
monitor the value until it gets altered. This assumed a certain ordering
regarding the `std::thread` spawning, however the ordering was not
always guaranteed at runtime.

To fix that, the test now makes use of a `std::condition_variable`
shared between the each thread. On the former, it will notify the other
thread when the variable gets initialized or updated and on the latter,
it will wait until the variable it receives a new notification.

This should fix the data racing issue while preserving the testing
coverage.

rdar://98678134

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139484

Files:
  lldb/test/API/functionalities/scripted_process/Makefile
  lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
  lldb/test/API/functionalities/scripted_process/baz.c
  lldb/test/API/functionalities/scripted_process/baz.cpp
  lldb/test/API/functionalities/scripted_process/baz.h
  lldb/test/API/functionalities/scripted_process/main.cpp

Index: lldb/test/API/functionalities/scripted_process/main.cpp
===
--- lldb/test/API/functionalities/scripted_process/main.cpp
+++ lldb/test/API/functionalities/scripted_process/main.cpp
@@ -1,10 +1,10 @@
 #include 
-#include 
 #include 
 
-extern "C" {
-int baz(int);
-}
+#include "baz.h"
+
+std::condition_variable cv;
+std::mutex mutex;
 
 int bar(int i) {
   int j = i * i;
@@ -13,23 +13,22 @@
 
 int foo(int i) { return bar(i); }
 
-void call_and_wait(int &n) {
-  std::cout << "waiting for computation!" << std::endl;
-  while (baz(n) != 42)
-;
-  std::cout << "finished computation!" << std::endl;
+void compute_pow(int &n) {
+  n = foo(n);
 }
 
-void compute_pow(int &n) { n = foo(n); }
+void call_and_wait(int &n, std::mutex& mutex, std::condition_variable& cv) {
+  cv.notify_one();
+  while (baz(n, mutex, cv) != 42) {
+cv.notify_one();
+  }
+}
 
 int main() {
   int n = 42;
-  std::mutex mutex;
-  std::unique_lock lock(mutex);
 
-  std::thread thread_1(call_and_wait, std::ref(n));
+  std::thread thread_1(call_and_wait, std::ref(n), std::ref(mutex), std::ref(cv));
   std::thread thread_2(compute_pow, std::ref(n));
-  lock.unlock();
 
   thread_1.join();
   thread_2.join();
Index: lldb/test/API/functionalities/scripted_process/baz.h
===
--- lldb/test/API/functionalities/scripted_process/baz.h
+++ lldb/test/API/functionalities/scripted_process/baz.h
@@ -1,3 +1,6 @@
 #pragma once
 
-int baz(int j);
+#include 
+#include 
+
+int baz(int j, std::mutex& mutex, std::condition_variable& cv);
Index: lldb/test/API/functionalities/scripted_process/baz.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/scripted_process/baz.cpp
@@ -0,0 +1,10 @@
+#include "baz.h"
+
+#include 
+
+int baz(int j, std::mutex& mutex, std::condition_variable& cv) {
+  std::unique_lock lock(mutex);
+  cv.wait(lock, [&j]{return j == 42 * 42;});
+  int k = sqrt(j);
+  return k; // break here
+}
Index: lldb/test/API/functionalities/scripted_process/baz.c
===
--- lldb/test/API/functionalities/scripted_process/baz.c
+++ /dev/null
@@ -1,8 +0,0 @@
-#include "baz.h"
-
-#include 
-
-int baz(int j) {
-  int k = sqrt(j);
-  return k; // break here
-}
Index: lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
@@ -17,7 +17,7 @@
 def create_stack_skinny_corefile(self, file):
 self.build()
 target, process, thread, _ = lldbutil.run_to_source_breakpoint(self, "// break here",
-   lldb.SBFileSpec("baz.c"))
+   lldb.SBFileSpec("baz.cpp"))
 self.assertTrue(process.IsValid(), "Process is invalid.")
 # FIXME: Use SBAPI to save the process corefile.
 self.runCmd("process save-core -s stack  " + file)
@@ -109,9 +109,9 @@
 self.assertTrue(func, "Invalid function.")
 
 self.assertIn("baz", frame.GetFunctionName())

[Lldb-commits] [PATCH] D139484: [lldb/test] Fix data racing issue in TestStackCoreScriptedProcess

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 480674.
mib added a comment.

Run `clang-format`


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

https://reviews.llvm.org/D139484

Files:
  lldb/test/API/functionalities/scripted_process/Makefile
  lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
  lldb/test/API/functionalities/scripted_process/baz.c
  lldb/test/API/functionalities/scripted_process/baz.cpp
  lldb/test/API/functionalities/scripted_process/baz.h
  lldb/test/API/functionalities/scripted_process/main.cpp

Index: lldb/test/API/functionalities/scripted_process/main.cpp
===
--- lldb/test/API/functionalities/scripted_process/main.cpp
+++ lldb/test/API/functionalities/scripted_process/main.cpp
@@ -1,10 +1,10 @@
 #include 
-#include 
 #include 
 
-extern "C" {
-int baz(int);
-}
+#include "baz.h"
+
+std::condition_variable cv;
+std::mutex mutex;
 
 int bar(int i) {
   int j = i * i;
@@ -13,23 +13,21 @@
 
 int foo(int i) { return bar(i); }
 
-void call_and_wait(int &n) {
-  std::cout << "waiting for computation!" << std::endl;
-  while (baz(n) != 42)
-;
-  std::cout << "finished computation!" << std::endl;
-}
-
 void compute_pow(int &n) { n = foo(n); }
 
+void call_and_wait(int &n, std::mutex &mutex, std::condition_variable &cv) {
+  cv.notify_one();
+  while (baz(n, mutex, cv) != 42) {
+cv.notify_one();
+  }
+}
+
 int main() {
   int n = 42;
-  std::mutex mutex;
-  std::unique_lock lock(mutex);
 
-  std::thread thread_1(call_and_wait, std::ref(n));
+  std::thread thread_1(call_and_wait, std::ref(n), std::ref(mutex),
+   std::ref(cv));
   std::thread thread_2(compute_pow, std::ref(n));
-  lock.unlock();
 
   thread_1.join();
   thread_2.join();
Index: lldb/test/API/functionalities/scripted_process/baz.h
===
--- lldb/test/API/functionalities/scripted_process/baz.h
+++ lldb/test/API/functionalities/scripted_process/baz.h
@@ -1,3 +1,6 @@
 #pragma once
 
-int baz(int j);
+#include 
+#include 
+
+int baz(int j, std::mutex &mutex, std::condition_variable &cv);
Index: lldb/test/API/functionalities/scripted_process/baz.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/scripted_process/baz.cpp
@@ -0,0 +1,10 @@
+#include "baz.h"
+
+#include 
+
+int baz(int j, std::mutex &mutex, std::condition_variable &cv) {
+  std::unique_lock lock(mutex);
+  cv.wait(lock, [&j] { return j == 42 * 42; });
+  int k = sqrt(j);
+  return k; // break here
+}
Index: lldb/test/API/functionalities/scripted_process/baz.c
===
--- lldb/test/API/functionalities/scripted_process/baz.c
+++ /dev/null
@@ -1,8 +0,0 @@
-#include "baz.h"
-
-#include 
-
-int baz(int j) {
-  int k = sqrt(j);
-  return k; // break here
-}
Index: lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py
@@ -17,7 +17,7 @@
 def create_stack_skinny_corefile(self, file):
 self.build()
 target, process, thread, _ = lldbutil.run_to_source_breakpoint(self, "// break here",
-   lldb.SBFileSpec("baz.c"))
+   lldb.SBFileSpec("baz.cpp"))
 self.assertTrue(process.IsValid(), "Process is invalid.")
 # FIXME: Use SBAPI to save the process corefile.
 self.runCmd("process save-core -s stack  " + file)
@@ -109,9 +109,9 @@
 self.assertTrue(func, "Invalid function.")
 
 self.assertIn("baz", frame.GetFunctionName())
-self.assertEqual(frame.vars.GetSize(), 2)
-self.assertEqual(int(frame.vars.GetFirstValueByName('j').GetValue()), 42 * 42)
+self.assertGreater(frame.vars.GetSize(), 0)
 self.assertEqual(int(frame.vars.GetFirstValueByName('k').GetValue()), 42)
+self.assertEqual(int(frame.vars.GetFirstValueByName('j').GetValue()), 42 * 42)
 
 corefile_dylib = self.get_module_with_name(corefile_target, 'libbaz.dylib')
 self.assertTrue(corefile_dylib, "Dynamic library libbaz.dylib not found.")
Index: lldb/test/API/functionalities/scripted_process/Makefile
===
--- lldb/test/API/functionalities/scripted_process/Makefile
+++ lldb/test/API/functionalities/scripted_process/Makefile
@@ -6,8 +6,8 @@
 
 all: libbaz.dylib a.out
 
-libbaz.dylib: baz.c
+libbaz.dylib: baz.cpp
 	$(MAKE) -f $(MAKEFILE_RULES) ARCH=$(ARCH) \
-		DYLIB_ONLY=YES DYLIB_NAME=baz DYLIB_C_SOURCES=baz.c
+		DYLIB_ONLY=YES DYLIB_NAME=baz DYLIB_CXX_SOURCES=baz.

[Lldb-commits] [PATCH] D139484: [lldb/test] Fix data racing issue in TestStackCoreScriptedProcess

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

I think that the idea you've laid out in the summary makes sense but the 
implementation seems off.
My understanding of `std::condition_variable` is that combined with 
`std::mutex` it is used to facilitate inter-thread synchronization. That would 
make sense if `thread_1` and `thread_2` both used a mutex and 
condition_variable to communicate with each other about the status of `n` but 
it looks like only `thread_1` actually uses and manages the 
mutex/condition_variable. Why is `call_and_wait` invoking `notify_one` when 
it's also what invokes `baz` (which is what is invoking `wait`)? This seems 
somewhat sketchy because the thread that is performing the notifications is the 
same thread that is performing the wait.

To accomplish your goal, I think it would probably look more like this:

1. `compute_pow` should acquire the lock on the mutex, modify `n`, unlock, and 
then invoke `cv.notify_one()`.
2. `call_and_wait` should only invoke `baz`. I don't think that the 
implementation of `baz` would require any modifications. Maybe `thread_2` could 
run `baz` directly instead?

Perhaps I'm mistaken about some details but I think this could probably be 
written in a clearer way.


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

https://reviews.llvm.org/D139484

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

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Lu Weining via Phabricator via lldb-commits]

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

In D139158#3974801 , @DavidSpickett 
wrote:

> Looks mechanically fine. These classes are all a bit of a copy paste job 
> right now, so if you were going to take inspiration riscv is your best bet.
>
> On the subject of coding style, 
> https://lldb.llvm.org/resources/contributing.html.
>
>   Coding Style: LLDB’s code style differs from LLVM’s coding style. 
> Unfortunately there is no document describing the differences. Please be 
> consistent with the existing code.
>
> Which isn't super helpful but in general look at something central like 
> `lldb/source/Target/Process.cpp` and see what it does. The main difference 
> you'll see is `variable_names_with_underscores_like_this` and `m_` prefix for 
> class members.
>
> For me, the style here looks fine.

Thanks for the clarification. Then LGTM for LoongArch related change.


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

https://reviews.llvm.org/D139158

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

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Hui Li via Phabricator via lldb-commits]

lh03061238 added a comment.

In D139158#3974801 , @DavidSpickett 
wrote:

> Looks mechanically fine. These classes are all a bit of a copy paste job 
> right now, so if you were going to take inspiration riscv is your best bet.
>
> On the subject of coding style, 
> https://lldb.llvm.org/resources/contributing.html.
>
>   Coding Style: LLDB’s code style differs from LLVM’s coding style. 
> Unfortunately there is no document describing the differences. Please be 
> consistent with the existing code.
>
> Which isn't super helpful but in general look at something central like 
> `lldb/source/Target/Process.cpp` and see what it does. The main difference 
> you'll see is `variable_names_with_underscores_like_this` and `m_` prefix for 
> class members.
>
> For me, the style here looks fine.

Thanks for your suggestion


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

https://reviews.llvm.org/D139158

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

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Hui Li via Phabricator via lldb-commits]

lh03061238 added inline comments.



Comment at: 
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:58
+   uint32_t reg_num) override;
+  lldb::addr_t ReadPC(bool *success);
+  bool WritePC(lldb::addr_t pc);

DavidSpickett wrote:
> I think the older targets use this form but for riscv they went with 
> `llvm::Optional ReadPC();` which I prefer over pointer plus 
> addr_t.
> I think the older targets use this form but for riscv they went with 
> `llvm::Optional ReadPC();` which I prefer over pointer plus 
> addr_t.

EmulateInstructionLoongArch is relatively simple Compared with riscv. If use 
llvm::Optional ReadPC(), There will be more type 
conversions here. I prefer to keep that definition for now, Considering the 
complexity of the code at this stage.

Thank you.


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

https://reviews.llvm.org/D139158

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

[Lldb-commits] [lldb] 4857b6f - Revert "Store OptTable::Info::Name as a StringRef"

[Douglas Yung via lldb-commits]

Author: Douglas Yung
Date: 2022-12-06T19:37:05-08:00
New Revision: 4857b6f8fff614c89ed35031c298df81394459af

URL: 
https://github.com/llvm/llvm-project/commit/4857b6f8fff614c89ed35031c298df81394459af
DIFF: 
https://github.com/llvm/llvm-project/commit/4857b6f8fff614c89ed35031c298df81394459af.diff

LOG: Revert "Store OptTable::Info::Name as a StringRef"

This reverts commit 5951b0bb23f3265bea16f28c2af9d278b9d829c6.

This is causing 24 test failures on the PS4 linux bot: 
https://lab.llvm.org/buildbot/#/builders/139/builds/32263

Added: 


Modified: 
clang/lib/Driver/ToolChains/Gnu.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
llvm/include/llvm/ADT/StringRef.h
llvm/include/llvm/Option/OptTable.h
llvm/lib/Option/OptTable.cpp
llvm/unittests/Option/OptionMarshallingTest.cpp
llvm/utils/TableGen/OptParserEmitter.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/Gnu.cpp 
b/clang/lib/Driver/ToolChains/Gnu.cpp
index 60d62e2b9c5c1..4621850f13772 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -331,8 +331,8 @@ static bool getStaticPIE(const ArgList &Args, const 
ToolChain &TC) {
   if (HasStaticPIE && Args.hasArg(options::OPT_nopie)) {
 const Driver &D = TC.getDriver();
 const llvm::opt::OptTable &Opts = D.getOpts();
-StringRef StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
-StringRef NoPIEName = Opts.getOptionName(options::OPT_nopie);
+const char *StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
+const char *NoPIEName = Opts.getOptionName(options::OPT_nopie);
 D.Diag(diag::err_drv_cannot_mix_options) << StaticPIEName << NoPIEName;
   }
   return HasStaticPIE;

diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp 
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index fde098840be4b..9d89148616be1 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1055,7 +1055,7 @@ void 
PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
   // Only add the version-min options if we got a version from somewhere
   if (!version.empty() && sdk_type != XcodeSDK::Type::Linux) {
 #define OPTION(PREFIX, NAME, VAR, ...) 
\
-  llvm::StringRef opt_##VAR = NAME;
\
+  const char *opt_##VAR = NAME;
\
   (void)opt_##VAR;
 #include "clang/Driver/Options.inc"
 #undef OPTION

diff  --git a/llvm/include/llvm/ADT/StringRef.h 
b/llvm/include/llvm/ADT/StringRef.h
index 9fea390c2ed32..032f42a51ec0b 100644
--- a/llvm/include/llvm/ADT/StringRef.h
+++ b/llvm/include/llvm/ADT/StringRef.h
@@ -561,8 +561,7 @@ namespace llvm {
 /// \param N The number of characters to included in the substring. If N
 /// exceeds the number of characters remaining in the string, the string
 /// suffix (starting with \p Start) will be returned.
-[[nodiscard]] constexpr StringRef substr(size_t Start,
- size_t N = npos) const {
+[[nodiscard]] StringRef substr(size_t Start, size_t N = npos) const {
   Start = std::min(Start, Length);
   return StringRef(Data + Start, std::min(N, Length - Start));
 }

diff  --git a/llvm/include/llvm/Option/OptTable.h 
b/llvm/include/llvm/Option/OptTable.h
index e884ebeb788c4..07d9870f71b33 100644
--- a/llvm/include/llvm/Option/OptTable.h
+++ b/llvm/include/llvm/Option/OptTable.h
@@ -44,7 +44,7 @@ class OptTable {
 /// A null terminated array of prefix strings to apply to name while
 /// matching.
 const char *const *Prefixes;
-StringRef Name;
+const char *Name;
 const char *HelpText;
 const char *MetaVar;
 unsigned ID;
@@ -102,7 +102,9 @@ class OptTable {
   const Option getOption(OptSpecifier Opt) const;
 
   /// Lookup the name of the given option.
-  StringRef getOptionName(OptSpecifier id) const { return getInfo(id).Name; }
+  const char *getOptionName(OptSpecifier id) const {
+return getInfo(id).Name;
+  }
 
   /// Get the kind of the given option.
   unsigned getOptionKind(OptSpecifier id) const {
@@ -182,7 +184,7 @@ class OptTable {
   ///  takes
   ///
   /// \return true in success, and false in fail.
-  bool addValues(StringRef Option, const char *Values);
+  bool addValues(const char *Option, const char *Values);
 
   /// Parse a single argument; returning the new argument and
   /// updating Index.

diff  --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp
index f23561b5d078d..ef4873eb7f9c4 100644
--- a/llvm/lib/Option/OptTable.cpp
+++ b/llvm/lib/Option/OptTable.cpp
@@ -36,23 +36,31 @@ namespace opt {
 // Ordering on Info. The ordering is *almost* case-insensitive lexicographic,
 // with an exception. '\0' comes at the end of the alphabet

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Amara Emerson via Phabricator via lldb-commits]

aemerson added a comment.

I just reverted this in 6e6be5f9504d 
 because 
it seems to have broken macOS builds:

  llvm/lib/Support/Compression.cpp:24:10: fatal error: 'zstd.h' file not found
  #include 
   ^~~~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D128465#3652525 , @ckissane wrote:

> In D128465#3651025 , @aemerson 
> wrote:
>
>> I just reverted this in 6e6be5f9504d 
>>  
>> because it seems to have broken macOS builds:
>>
>>   llvm/lib/Support/Compression.cpp:24:10: fatal error: 'zstd.h' file not 
>> found
>>   #include 
>>^~~~
>
> @aemerson Could you provide the output of your cmake command?
> (I can't easily reproduce because I don't have a mac on hand)

FWIW I hit this last night on my mac desktop - it looked like I had zstd 
installed by homebrew (as a dependency on something I installed), and cmake was 
able to find it in /opt/homebrew (somehow) but when Compression.cpp was built, 
nothing had added -I/opt/homebrew/include so the header wasn't found.  I hacked 
the FindZSTD.cmake to not find it, to finish what I was working on before 
bedtime.  This might not be related to what @aemerson saw though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

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

Does this address the macOS build failure?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane updated this revision to Diff 445623.
ckissane added a comment.

- remove extra cmake info


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

Files:
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -61,4 +61,42 @@
 }
 #endif
 
+#if LLVM_ENABLE_ZSTD
+static void testZstdCompression(StringRef Input, int Level) {
+  SmallVector Compressed;
+  SmallVector Uncompressed;
+  zstd::compress(arrayRefFromStringRef(Input), Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, toStringRef(Uncompressed));
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
+}
+
+TEST(CompressionTest, Zstd) {
+  testZstdCompression("", zstd::DefaultCompression);
+
+  testZstdCompression("hello, world!", zstd::NoCompression);
+  testZstdCompression("hello, world!", zstd::BestSizeCompression);
+  testZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  testZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i)
+BinaryData[i] = i & 255;
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  testZstdCompression(BinaryDataStr, zstd::NoCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  testZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
+#endif
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_curl = @LLVM_ENABLE_CURL@
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -20,6 +20,9 @@
 #if LLVM_ENABLE_ZLIB
 #include 
 #endif
+#if LLVM_ENABLE_ZSTD
+#include 
+#endif
 
 using namespace llvm;
 using namespace llvm::compression;
@@ -100,3 +103,65 @@
   llvm_unreachable("zlib::uncompress is unavailable");
 }
 #endif
+
+#if LLVM_ENABLE_ZSTD
+
+bool zstd::isAvailable() { return true; }
+
+void zstd::compress(ArrayRef Input,
+SmallVectorImpl &CompressedBuffer, int Level) {
+  unsigned long CompressedBufferSize = ::ZSTD_compressBound(Input.size());
+  CompressedBuffer.resize_for_overwrite(CompressedBufferSize);
+  unsigned long CompressedSize =
+  ::ZSTD_compress((char *)CompressedBuffer.data(), CompressedBufferSize,
+  (const char *)Input.data(), Input.size(), Level);
+  if (ZSTD_isError(CompressedSize))
+report_bad_alloc_error("Allocation failed");
+  // Tell MemorySanitizer that zstd output buffer is fully initialized.
+  // This avoids a false report when running LLVM with uninstrumented ZLib.
+  __msan_unpoison(CompressedBuffer.data(), CompressedSize);
+  if (CompressedSize < CompressedBuffer.size())
+CompressedBuffer.truncate(CompressedSize);
+}
+
+Error zstd::uncompress(ArrayRef Input, uint8_t *UncompressedBuffer,
+   size_t &UncompressedSize) {
+  const size_t Res =
+  ::ZSTD_decompress(UncompressedBuffer, UncompressedSize,
+(const uint8_t *)Input.data(), Input.size());
+  UncompressedSize =

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Leonard Chan via Phabricator via lldb-commits]

leonardchan accepted this revision.
leonardchan added a comment.

LGTM tested on mac on my end


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Cole Kissane 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 rGe939bf67e340: [llvm] add zstd to `llvm::compression` 
namespace (authored by ckissane).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

Files:
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -61,4 +61,42 @@
 }
 #endif
 
+#if LLVM_ENABLE_ZSTD
+static void testZstdCompression(StringRef Input, int Level) {
+  SmallVector Compressed;
+  SmallVector Uncompressed;
+  zstd::compress(arrayRefFromStringRef(Input), Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, toStringRef(Uncompressed));
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
+}
+
+TEST(CompressionTest, Zstd) {
+  testZstdCompression("", zstd::DefaultCompression);
+
+  testZstdCompression("hello, world!", zstd::NoCompression);
+  testZstdCompression("hello, world!", zstd::BestSizeCompression);
+  testZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  testZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i)
+BinaryData[i] = i & 255;
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  testZstdCompression(BinaryDataStr, zstd::NoCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  testZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
+#endif
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_curl = @LLVM_ENABLE_CURL@
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -20,6 +20,9 @@
 #if LLVM_ENABLE_ZLIB
 #include 
 #endif
+#if LLVM_ENABLE_ZSTD
+#include 
+#endif
 
 using namespace llvm;
 using namespace llvm::compression;
@@ -100,3 +103,65 @@
   llvm_unreachable("zlib::uncompress is unavailable");
 }
 #endif
+
+#if LLVM_ENABLE_ZSTD
+
+bool zstd::isAvailable() { return true; }
+
+void zstd::compress(ArrayRef Input,
+SmallVectorImpl &CompressedBuffer, int Level) {
+  unsigned long CompressedBufferSize = ::ZSTD_compressBound(Input.size());
+  CompressedBuffer.resize_for_overwrite(CompressedBufferSize);
+  unsigned long CompressedSize =
+  ::ZSTD_compress((char *)CompressedBuffer.data(), CompressedBufferSize,
+  (const char *)Input.data(), Input.size(), Level);
+  if (ZSTD_isError(CompressedSize))
+report_bad_alloc_error("Allocation failed");
+  // Tell MemorySanitizer that zstd output buffer is fully initialized.
+  // This avoids a false report when running LLVM with uninstrumented ZLib.
+  __msan_unpoison(CompressedBuffer.data(), CompressedSize);
+  if (CompressedSize < CompressedBuffer.size())
+CompressedBuffer.truncate(CompressedSize);
+}
+
+Error zstd::uncompress(ArrayRef Input, uint8_t *UncompressedBuffer,
+   size_t &UncompressedSize) {
+  const size_t Res =
+  ::ZSTD_decom

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Sergei Barannikov via Phabricator via lldb-commits]

barannikov88 added a comment.

@ckissane 
I took fresh sources of LLVM and when I run cmake I get the following warning:

  -- Looking for compress2
  -- Looking for compress2 - found
  CMake Warning at cmake/config-ix.cmake:149 (find_package):
By not providing "Findzstd.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "zstd", but
CMake did not find one.
  
Could not find a package configuration file provided by "zstd" with any of
the following names:
  
  zstdConfig.cmake
  zstd-config.cmake
  
Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
"zstd_DIR" to a directory containing one of the above files.  If "zstd"
provides a separate development package or SDK, be sure it has been
installed.
  Call Stack (most recent call first):
CMakeLists.txt:768 (include)

Ubuntu 22.04 LTS
cmake version 3.22.3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Petr Hosek via Phabricator via lldb-commits]

phosek added inline comments.



Comment at: llvm/cmake/config-ix.cmake:149
+  elseif(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
+find_package(zstd)
+  endif()

Since there's no `Findzstd.cmake` module shipped with CMake, and we don't 
provide in LLVM, we should use the config mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I am still seeing the

  zstdConfig.cmake
  zstd-config.cmake

warning. @ckissane @phosek will you fix it? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Alex Brachet via Phabricator via lldb-commits]

abrachet added a comment.

In D128465#3672515 , @MaskRay wrote:

> I am still seeing the
>
>   zstdConfig.cmake
>   zstd-config.cmake
>
> warning. @ckissane @phosek will you fix it? :)

I've quieted the warnings in 
https://github.com/llvm/llvm-project/commit/a71a01bc10d509bd4e746e7d277c4613ef886875,
 though I'm just now seeing @phosek's suggestion to use `CONFIG` I'm not sure 
what that does


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane updated this revision to Diff 445615.
ckissane added a comment.

- move try_find_dependency into it's own file
- add newline to end of TryFindDependencyMacro.cmake
- clarify comment in TryFindDependencyMacro.cmake


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

Files:
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/cmake/modules/TryFindDependencyMacro.cmake
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -61,4 +61,42 @@
 }
 #endif
 
+#if LLVM_ENABLE_ZSTD
+static void testZstdCompression(StringRef Input, int Level) {
+  SmallVector Compressed;
+  SmallVector Uncompressed;
+  zstd::compress(arrayRefFromStringRef(Input), Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, toStringRef(Uncompressed));
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
+}
+
+TEST(CompressionTest, Zstd) {
+  testZstdCompression("", zstd::DefaultCompression);
+
+  testZstdCompression("hello, world!", zstd::NoCompression);
+  testZstdCompression("hello, world!", zstd::BestSizeCompression);
+  testZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  testZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i)
+BinaryData[i] = i & 255;
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  testZstdCompression(BinaryDataStr, zstd::NoCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  testZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
+#endif
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_curl = @LLVM_ENABLE_CURL@
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -20,6 +20,9 @@
 #if LLVM_ENABLE_ZLIB
 #include 
 #endif
+#if LLVM_ENABLE_ZSTD
+#include 
+#endif
 
 using namespace llvm;
 using namespace llvm::compression;
@@ -100,3 +103,65 @@
   llvm_unreachable("zlib::uncompress is unavailable");
 }
 #endif
+
+#if LLVM_ENABLE_ZSTD
+
+bool zstd::isAvailable() { return true; }
+
+void zstd::compress(ArrayRef Input,
+SmallVectorImpl &CompressedBuffer, int Level) {
+  unsigned long CompressedBufferSize = ::ZSTD_compressBound(Input.size());
+  CompressedBuffer.resize_for_overwrite(CompressedBufferSize);
+  unsigned long CompressedSize =
+  ::ZSTD_compress((char *)CompressedBuffer.data(), CompressedBufferSize,
+  (const char *)Input.data(), Input.size(), Level);
+  if (ZSTD_isError(CompressedSize))
+report_bad_alloc_error("Allocation failed");
+  // Tell MemorySanitizer that zstd output buffer is fully initialized.
+  // This avoids a false report when running LLVM with uninstrumented ZLib.
+  __msan_unpoison(CompressedBuffer.data(), CompressedSize);
+  if (CompressedSize < CompressedBuffer.size())
+CompressedBuffer.truncate(CompressedSize);
+}
+
+Error zstd::uncompress(ArrayRef Input, uint8_t *UncompressedBuffer,
+   size_t &UncompressedSize) {
+  c

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/Support/Compression.h:48
+
+constexpr int NoCompression = -5;
+constexpr int BestSpeedCompression = 1;

I missed the values here. Why is -5 picked for NoCompression? What does it mean?

zstd.h says ZSTD_maxCLevel() is currently 22. The CLI program mentions 19. Why 
is BestSizeCompression 12?

ZSTD_CLEVEL_DEFAULT/the CLI CLI uses 3 for the default level. Why is 
DefaultCompression 5?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

___
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

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)

Just to check if your intention aligns with my understanding, removing the 
suffix here is fine because the destination is in the binary dir and not the 
final install destination?



Comment at: clang/runtime/CMakeLists.txt:90-92
+  -DCMAKE_INSTALL_BINDIR="${CMAKE_INSTALL_BINDIR}"
+  -DCMAKE_INSTALL_LIBDIR="${CMAKE_INSTALL_LIBDIR}"
+  
-DCMAKE_INSTALL_INCLUDEDIR="${CMAKE_INSTALL_INCLUDEDIR}"

nit: The indentation is wrong



Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:65
   set(output_dir "${LLVM_LIBRARY_OUTPUT_INTDIR}")
-  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib")
 else()

This is an install directory, so should this be something like
```
extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
```
instead?



Comment at: compiler-rt/cmake/base-config-ix.cmake:48
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_INSTALL_PATH lib/clang/${CLANG_VERSION})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."

This is an install path, so should it use 
`${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION}` instead?



Comment at: compiler-rt/cmake/base-config-ix.cmake:103
 ${COMPILER_RT_OUTPUT_DIR}/lib)
-  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" lib)
+  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" 
"${CMAKE_INSTALL_LIBDIR}")
   set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH

What is the result we expect here?
In case that CMAKE_INSTALL_LIBDIR is defined as lib64, this path will change.

In some cases it would have been `lib64/clang/14.0.0/lib`,
but with this patch it would be `lib/clang/14.0.0/lib64` if I understand 
correctly.



Comment at: lldb/source/API/CMakeLists.txt:116
 if(LLDB_ENABLE_PYTHON AND (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB) AND UNIX 
AND NOT APPLE)
-  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}")
+  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib")
 endif()

It looks to me like this path is used for installation, so should it have the 
(potential) suffix?
In AddLLVM.cmake, _install_rpath uses `${CMAKE_INSTALL_LIBDIR}`.



Comment at: mlir/cmake/modules/AddMLIRPython.cmake:412
 set_property(TARGET ${target} APPEND PROPERTY
-  INSTALL_RPATH 
"${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib${LLVM_LIBDIR_SUFFIX}")
+  INSTALL_RPATH "${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib")
   endif()

Same here as above, the rpath should probably use `${CMAKE_INSTALL_LIBDIR}`?


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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[John Ericson via Phabricator via lldb-commits]

Ericson2314 created this revision.
Ericson2314 added reviewers: sebastian-ne, beanz, compnerd, phosek.
Herald added subscribers: libc-commits, libcxx-commits, Enna1, bzcheeseman, 
ayermolo, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, 
msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, 
arpith-jacob, antiagainst, shauheen, rriddle, mehdi_amini, lebedev.ri, 
whisperity, mgorny.
Herald added a reviewer: bollu.
Herald added a reviewer: lebedev.ri.
Herald added a reviewer: ldionne.
Herald added a reviewer: sscalpone.
Herald added a reviewer: rafauler.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a reviewer: NoQ.
Herald added projects: libunwind, libc-project, Flang, All.
Herald added a reviewer: libunwind.
Ericson2314 requested review of this revision.
Herald added subscribers: llvm-commits, openmp-commits, lldb-commits, 
Sanitizers, cfe-commits, yota9, stephenneuendorffer, nicolasvasilache, 
jdoerfert, MaskRay.
Herald added projects: clang, Sanitizers, LLDB, libc++, OpenMP, libc++abi, 
MLIR, LLVM.
Herald added a reviewer: libc++.
Herald added a reviewer: libc++abi.

We held off on this before as `LLVM_LIBDIR_SUFFIX` conflicted with it.
Now we return this.

`LLVM_LIBDIR_SUFFIX` is kept as a deprecated way to set
`CMAKE_INSTALL_LIBDIR`. The other `*_LIBDIR_SUFFIX` are just removed
entirely.

I imagine this is too potentially-breaking to make LLVM 15. That's fine.
I have a more minimal version of this in the disto (NixOS) patches for
LLVM 15 (like previous versions). This more expansive version I will
test harder after the release is cut.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130586

Files:
  bolt/runtime/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/caches/Android-stage2.cmake
  clang/cmake/caches/Android.cmake
  clang/cmake/modules/AddClang.cmake
  clang/cmake/modules/CMakeLists.txt
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Headers/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  cmake/Modules/GNUInstallPackageDir.cmake
  compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  compiler-rt/cmake/base-config-ix.cmake
  compiler-rt/docs/BuildingCompilerRT.rst
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/cmake/modules/CMakeLists.txt
  libc/CMakeLists.txt
  libc/lib/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/docs/BuildingLibunwind.rst
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/cmake/modules/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/cmake/modules/LLDBGenerateConfig.cmake
  lldb/cmake/modules/LLDBStandalone.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/source/API/CMakeLists.txt
  lldb/source/Host/linux/HostInfoLinux.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/tools/intel-features/CMakeLists.txt
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddOCaml.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/docs/CMake.rst
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/llvm-shlib/CMakeLists.txt
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  llvm/utils/gn/secondary/lldb/include/lldb/Host/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  mlir/cmake/modules/AddMLIRPython.cmake
  mlir/cmake/modules/CMakeLists.txt
  mlir/test/CMakeLists.txt
  openmp/CMakeLists.txt
  openmp/README.rst
  polly/cmake/CMakeLists.txt
  polly/cmake/polly_macros.cmake
  polly/test/CMakeLists.txt
  pstl/CMakeLists.txt
  third-party/benchmark/src/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h

Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -68,7 +68,7 @@
 #define CLANG_SYSTEMZ_DEFAULT_ARCH "z10"
 
 /* Multilib suffix for libdir. */
-#define CLANG_LIBDIR_SUFFIX ""
+#define CLANG_INSTALL_LIBDIR_BASENAME "lib"
 
 /* Relative directory for resource files */
 #define CLANG_RESOURCE_DIR ""
Index: third-party/benchmark/src/CMakeLists.txt
===
--- third-party/benchmark/src/CMakeLists.txt
+++ third-party/benchmark/src/CMakeLists.txt
@@ -79,7 +79,7 @@
 configure_package_config_file (

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

[John Ericson via Phabricator via lldb-commits]

Ericson2314 added a comment.

Thanks so much @sebastian-ne for the thorough review!

Unlike the other patches rather than being cleaned up code that needs more 
testing, this is closer to the original patch we've been using for a while 
(tested but not cleaned up); sorry there were so many things to catch!




Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)

sebastian-ne wrote:
> Just to check if your intention aligns with my understanding, removing the 
> suffix here is fine because the destination is in the binary dir and not the 
> final install destination?
Yes exactly.

I really should write up the "rules" that I've (a) discovered from reading (b) 
invented somewhere. Any idea where?



Comment at: clang/runtime/CMakeLists.txt:90-92
+  -DCMAKE_INSTALL_BINDIR="${CMAKE_INSTALL_BINDIR}"
+  -DCMAKE_INSTALL_LIBDIR="${CMAKE_INSTALL_LIBDIR}"
+  
-DCMAKE_INSTALL_INCLUDEDIR="${CMAKE_INSTALL_INCLUDEDIR}"

sebastian-ne wrote:
> nit: The indentation is wrong
Oops, thanks.



Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:65
   set(output_dir "${LLVM_LIBRARY_OUTPUT_INTDIR}")
-  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib")
 else()

sebastian-ne wrote:
> This is an install directory, so should this be something like
> ```
> extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
> ```
> instead?
Indeed, thanks for catching!

`${CMAKE_INSTALL_FULL_LIBDIR}` also does this.



Comment at: compiler-rt/cmake/base-config-ix.cmake:48
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_INSTALL_PATH lib/clang/${CLANG_VERSION})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."

sebastian-ne wrote:
> This is an install path, so should it use 
> `${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION}` instead?
Yes. Thanks!



Comment at: compiler-rt/cmake/base-config-ix.cmake:103
 ${COMPILER_RT_OUTPUT_DIR}/lib)
-  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" lib)
+  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" 
"${CMAKE_INSTALL_LIBDIR}")
   set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH

sebastian-ne wrote:
> What is the result we expect here?
> In case that CMAKE_INSTALL_LIBDIR is defined as lib64, this path will change.
> 
> In some cases it would have been `lib64/clang/14.0.0/lib`,
> but with this patch it would be `lib/clang/14.0.0/lib64` if I understand 
> correctly.
Oh good point, yeah this double `lib` is very tricky I will fix and add comment.



Comment at: lldb/source/API/CMakeLists.txt:116
 if(LLDB_ENABLE_PYTHON AND (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB) AND UNIX 
AND NOT APPLE)
-  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}")
+  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib")
 endif()

sebastian-ne wrote:
> It looks to me like this path is used for installation, so should it have the 
> (potential) suffix?
> In AddLLVM.cmake, _install_rpath uses `${CMAKE_INSTALL_LIBDIR}`.
Yes. I'll do an `extend_path` to handle the absolute path case too.



Comment at: mlir/cmake/modules/AddMLIRPython.cmake:412
 set_property(TARGET ${target} APPEND PROPERTY
-  INSTALL_RPATH 
"${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib${LLVM_LIBDIR_SUFFIX}")
+  INSTALL_RPATH "${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib")
   endif()

sebastian-ne wrote:
> Same here as above, the rpath should probably use `${CMAKE_INSTALL_LIBDIR}`?
Agreed, and same extra comment on the fix as above too,


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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 447757.
Ericson2314 added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: Michael137, sstefan1, JDevlieghere, ormris.

Forgot to define `CLANG_INSTALL_LIBDIR_BASENAME`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

Files:
  bolt/runtime/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/caches/Android-stage2.cmake
  clang/cmake/caches/Android.cmake
  clang/cmake/modules/AddClang.cmake
  clang/cmake/modules/CMakeLists.txt
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Headers/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  cmake/Modules/GNUInstallPackageDir.cmake
  compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  compiler-rt/cmake/base-config-ix.cmake
  compiler-rt/docs/BuildingCompilerRT.rst
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/cmake/modules/CMakeLists.txt
  libc/CMakeLists.txt
  libc/lib/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/docs/BuildingLibunwind.rst
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/cmake/modules/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/cmake/modules/LLDBGenerateConfig.cmake
  lldb/cmake/modules/LLDBStandalone.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/source/API/CMakeLists.txt
  lldb/source/Host/linux/HostInfoLinux.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/tools/intel-features/CMakeLists.txt
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddOCaml.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/docs/CMake.rst
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/llvm-shlib/CMakeLists.txt
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  llvm/utils/gn/secondary/lldb/include/lldb/Host/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  mlir/cmake/modules/AddMLIRPython.cmake
  mlir/cmake/modules/CMakeLists.txt
  mlir/test/CMakeLists.txt
  openmp/CMakeLists.txt
  openmp/README.rst
  polly/cmake/CMakeLists.txt
  polly/cmake/polly_macros.cmake
  polly/test/CMakeLists.txt
  pstl/CMakeLists.txt
  third-party/benchmark/src/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h

Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -68,7 +68,7 @@
 #define CLANG_SYSTEMZ_DEFAULT_ARCH "z10"
 
 /* Multilib suffix for libdir. */
-#define CLANG_LIBDIR_SUFFIX ""
+#define CLANG_INSTALL_LIBDIR_BASENAME "lib"
 
 /* Relative directory for resource files */
 #define CLANG_RESOURCE_DIR ""
Index: third-party/benchmark/src/CMakeLists.txt
===
--- third-party/benchmark/src/CMakeLists.txt
+++ third-party/benchmark/src/CMakeLists.txt
@@ -79,7 +79,7 @@
 configure_package_config_file (
   ${PROJECT_SOURCE_DIR}/cmake/Config.cmake.in
   ${project_config}
-  INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}
+  INSTALL_DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}"
   NO_SET_AND_CHECK_MACRO
   NO_CHECK_REQUIRED_COMPONENTS_MACRO
 )
@@ -100,8 +100,8 @@
   install(
 TARGETS ${targets_to_export}
 EXPORT ${targets_export_name}
-ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
-LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
 RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
 INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -88,10 +88,10 @@
 install(EXPORT ParallelSTLTargets
 FILE ParallelSTLTargets.cmake
 NAMESPACE pstl::
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfig.cmake"
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(DIRECTORY include/

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

[John Ericson via Phabricator via lldb-commits]

Ericson2314 marked 6 inline comments as done.
Ericson2314 added inline comments.



Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:65
   set(output_dir "${LLVM_LIBRARY_OUTPUT_INTDIR}")
-  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib")
 else()

Ericson2314 wrote:
> sebastian-ne wrote:
> > This is an install directory, so should this be something like
> > ```
> > extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
> > ```
> > instead?
> Indeed, thanks for catching!
> 
> `${CMAKE_INSTALL_FULL_LIBDIR}` also does this.
Actually, it will do the `CMAKE_INSTALL_PREFIX` part by default (and the other 
branch is already a potentially relative path) so just 
`${CMAKE_INSTALL_LIBDIR}` will suffice.


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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 448053.
Ericson2314 marked an inline comment as done.
Ericson2314 added a comment.

Fix review comments. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

Files:
  bolt/runtime/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/caches/Android-stage2.cmake
  clang/cmake/caches/Android.cmake
  clang/cmake/modules/AddClang.cmake
  clang/cmake/modules/CMakeLists.txt
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Headers/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  cmake/Modules/GNUInstallPackageDir.cmake
  compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  compiler-rt/cmake/base-config-ix.cmake
  compiler-rt/docs/BuildingCompilerRT.rst
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/cmake/modules/CMakeLists.txt
  libc/CMakeLists.txt
  libc/lib/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/docs/BuildingLibunwind.rst
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/cmake/modules/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/cmake/modules/LLDBGenerateConfig.cmake
  lldb/cmake/modules/LLDBStandalone.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/source/API/CMakeLists.txt
  lldb/source/Host/linux/HostInfoLinux.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/tools/intel-features/CMakeLists.txt
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddOCaml.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/docs/CMake.rst
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/llvm-shlib/CMakeLists.txt
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  llvm/utils/gn/secondary/lldb/include/lldb/Host/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  mlir/cmake/modules/AddMLIRPython.cmake
  mlir/cmake/modules/CMakeLists.txt
  mlir/test/CMakeLists.txt
  openmp/CMakeLists.txt
  openmp/README.rst
  polly/cmake/CMakeLists.txt
  polly/cmake/polly_macros.cmake
  polly/test/CMakeLists.txt
  pstl/CMakeLists.txt
  third-party/benchmark/src/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h

Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -68,7 +68,7 @@
 #define CLANG_SYSTEMZ_DEFAULT_ARCH "z10"
 
 /* Multilib suffix for libdir. */
-#define CLANG_LIBDIR_SUFFIX ""
+#define CLANG_INSTALL_LIBDIR_BASENAME "lib"
 
 /* Relative directory for resource files */
 #define CLANG_RESOURCE_DIR ""
Index: third-party/benchmark/src/CMakeLists.txt
===
--- third-party/benchmark/src/CMakeLists.txt
+++ third-party/benchmark/src/CMakeLists.txt
@@ -79,7 +79,7 @@
 configure_package_config_file (
   ${PROJECT_SOURCE_DIR}/cmake/Config.cmake.in
   ${project_config}
-  INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}
+  INSTALL_DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}"
   NO_SET_AND_CHECK_MACRO
   NO_CHECK_REQUIRED_COMPONENTS_MACRO
 )
@@ -100,8 +100,8 @@
   install(
 TARGETS ${targets_to_export}
 EXPORT ${targets_export_name}
-ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
-LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
 RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
 INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -88,10 +88,10 @@
 install(EXPORT ParallelSTLTargets
 FILE ParallelSTLTargets.cmake
 NAMESPACE pstl::
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfig.cmake"
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(DIRECTORY include/
 DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 PATTERN "*.in"

[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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 448058.
Ericson2314 added a comment.

Add deprecation notice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

Files:
  bolt/runtime/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/caches/Android-stage2.cmake
  clang/cmake/caches/Android.cmake
  clang/cmake/modules/AddClang.cmake
  clang/cmake/modules/CMakeLists.txt
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Headers/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  cmake/Modules/GNUInstallPackageDir.cmake
  compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  compiler-rt/cmake/base-config-ix.cmake
  compiler-rt/docs/BuildingCompilerRT.rst
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/cmake/modules/CMakeLists.txt
  libc/CMakeLists.txt
  libc/lib/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/docs/BuildingLibcxx.rst
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/docs/BuildingLibunwind.rst
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/cmake/modules/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/cmake/modules/LLDBGenerateConfig.cmake
  lldb/cmake/modules/LLDBStandalone.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/source/API/CMakeLists.txt
  lldb/source/Host/linux/HostInfoLinux.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/tools/intel-features/CMakeLists.txt
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddOCaml.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/docs/CMake.rst
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/llvm-shlib/CMakeLists.txt
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  llvm/utils/gn/secondary/lldb/include/lldb/Host/BUILD.gn
  llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  mlir/cmake/modules/AddMLIRPython.cmake
  mlir/cmake/modules/CMakeLists.txt
  mlir/test/CMakeLists.txt
  openmp/CMakeLists.txt
  openmp/README.rst
  polly/cmake/CMakeLists.txt
  polly/cmake/polly_macros.cmake
  polly/test/CMakeLists.txt
  pstl/CMakeLists.txt
  third-party/benchmark/src/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h

Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -68,7 +68,7 @@
 #define CLANG_SYSTEMZ_DEFAULT_ARCH "z10"
 
 /* Multilib suffix for libdir. */
-#define CLANG_LIBDIR_SUFFIX ""
+#define CLANG_INSTALL_LIBDIR_BASENAME "lib"
 
 /* Relative directory for resource files */
 #define CLANG_RESOURCE_DIR ""
Index: third-party/benchmark/src/CMakeLists.txt
===
--- third-party/benchmark/src/CMakeLists.txt
+++ third-party/benchmark/src/CMakeLists.txt
@@ -79,7 +79,7 @@
 configure_package_config_file (
   ${PROJECT_SOURCE_DIR}/cmake/Config.cmake.in
   ${project_config}
-  INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}
+  INSTALL_DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}"
   NO_SET_AND_CHECK_MACRO
   NO_CHECK_REQUIRED_COMPONENTS_MACRO
 )
@@ -100,8 +100,8 @@
   install(
 TARGETS ${targets_to_export}
 EXPORT ${targets_export_name}
-ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
-LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
 RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
 INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -88,10 +88,10 @@
 install(EXPORT ParallelSTLTargets
 FILE ParallelSTLTargets.cmake
 NAMESPACE pstl::
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(FILES "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfig.cmake"
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
-DESTINATION lib/cmake/ParallelSTL)
+DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/ParallelSTL")
 install(DIRECTORY include/
 DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 PATTERN "*.in" EXCLUDE)
Index: polly/test/CMakeLists.txt
===

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

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)

Ericson2314 wrote:
> sebastian-ne wrote:
> > Just to check if your intention aligns with my understanding, removing the 
> > suffix here is fine because the destination is in the binary dir and not 
> > the final install destination?
> Yes exactly.
> 
> I really should write up the "rules" that I've (a) discovered from reading 
> (b) invented somewhere. Any idea where?
I think the commit message is a good place.



Comment at: llvm/CMakeLists.txt:59-60
+if (NOT DEFINED CMAKE_INSTALL_LIBDIR AND DEFINED LLVM_LIBDIR_SUFFIX)
+  message(WARNING "\"LLVM_LIBDIR_SUFFIX\" is deprecated. "
+  "Please set \"CMAKE_INSTALL_LIBDIR\" directly instead.")
+  set(CMAKE_INSTALL_LIBDIR "lib${LLVM_LIBDIR_SUFFIX}")

There’s also `message(DEPRECATION …)` :)


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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[John Ericson via Phabricator via lldb-commits]

Ericson2314 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}"
 

compnerd wrote:
> Does this not potentially break downstreams?
At the top of this file it says

```
/* This generated file is for internal use. Do not include it from headers. */
```


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] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Louis Dionne via Phabricator via lldb-commits]

ldionne accepted this revision as: libc++, libc++abi, ldionne.
ldionne added a comment.

I'm happy with this from the libc++/libc++abi side of things. You can ignore 
the failing CI job, it's been addressed.


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