[Lldb-commits] [PATCH] D52953: [lldb-mi] Implement -gdb-set breakpoint pending on/off

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: tools/lldb-mi/MICmdCmdGdbSet.cpp:447
+bool CMICmdCmdGdbSet::OptionFnBreakpoint(
+  const CMIUtilString::VecString_t 
) {
+bool bPending = false;

clang-format


Repository:
  rL LLVM

https://reviews.llvm.org/D52953



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

[Lldb-commits] [PATCH] D52953: [lldb-mi] Implement -gdb-set breakpoint pending on/off

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

And there is no reviewers set :)


Repository:
  rL LLVM

https://reviews.llvm.org/D52953



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

If we decide to optimize DWARF garbage collection, something generic will be 
cool.

This generic-abi thread has some discussion about that 
https://groups.google.com/d/msg/generic-abi/A-1rbP8hFCA/EDA7Sf3KBwAJ (e.g. 
using COMDAT but it seems challenging and it comes with its own costs)


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D54747#1342666 , @rocallahan wrote:

> Here are some results for the rusoto test in 
> https://github.com/rust-lang/rust/issues/56068#issue-382175735:
>
> | LLD   | Binary size in bytes |
> | LLD 6.0.1 | 43,791,192   |
> | LLD 8.0.0 1cfcf404f6b23943405bc3c5bb5940b10b914624   | 
> 43,861,056   |
> | LLD 8.0.0 1cfcf404f6b23943405bc3c5bb5940b10b914624 `--start-lib` | 
> 43,844,760   |
> | LLD 8.0.0 1cfcf404f6b23943405bc3c5bb5940b10b914624 + this patch  | 
> 6,281,488|
> |
>
> For `--start-lib` I wrapped `--start-lib`/`--end-lib` around all object files.
>
> As I expected `--start-lib` didn't make much difference. The dependencies 
> containing debuginfo that I want to be GCed away are already packaged into 
> static libraries before being passed to the final link.


The improvement of this patch looks really promising! Do you have numbers with 
ld.bfd and gold?




Comment at: lld/ELF/MarkLive.cpp:195
+return;
+  if (!Sec->File || !ObjFile::classof(Sec->File))
+return;

> `!ObjFile::classof(Sec->File)`

Can this happen?



Comment at: lld/ELF/MarkLive.cpp:199
+Sec->getFile()->HasLiveCodeOrData = true;
+  }
+}

The brace is redundant.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/ELF/MarkLive.cpp:192
+  Sec->Live = true;
+  if (Sec->kind() != SectionBase::Kind::Regular &&
+  Sec->kind() != SectionBase::Kind::Merge)

This check can be changed to `!isa && !isa`. 
But do you just want to exclude `EhInputSection`?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

@rocallahan I find that people are discussing a generic approach in D59553 



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D54747#1459175 , @rocallahan wrote:

> Updated results for the rusoto test in 
> https://github.com/rust-lang/rust/issues/56068#issue-382175735. The test 
> changed a bit because I'm using an updated Rust toolchain and `rusoto_core` 
> 0.37.0.
>
> | Linker | Size (bytes) | Real time (ms) |
> | GNU ld version 2.29.1-23.fc28  | 48,554,736   | 2234   |
> | GNU gold (version 2.29.1-23.fc28) 1.14 | 49,888,392   | 813|
> | lld-6.0.1-1.fc28.x86_64| 49,800,824   | 247|
> | lld d918d74461724a22cedd0b76dc1237392f295656
>   | 49,873,960   | 224|
> | lld d918d74461724a22cedd0b76dc1237392f295656 + this patch   
>   | 5,390,632| 158|
> |


@rocallahan
Can you also share the linker command line? You may execute `export 
LLD_REPRODUCE=/tmp/reproduce.tar` before the linking stage. After unpacking, 
there is a `response.txt` file that contains the full command line. I know 
little about Rust crates but I worry some component doesn't use static archives 
.a correctly. If you don't have .a but you have a bunch of .o files, you may 
place them inside a `--start-lib ... --end-lib` to get the archive semantic.

If all debug information of an object file is discarded, it sounds like if you 
switch the build system to use archives, the member in the archive will just 
get discarded, i.e.

If you do `ld.lld main.o a.o b.o c.o d1.o d2.o d3.o` before, you should 
probably switch to

`ld.lld main.o --start-lib a.o b.o c.o --end-lib --start-lib d1.o d2.o d3.o 
--end-lib`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D62593: DWARFDebugInfoEntry: delete unused Extract() and rename FastExtract() to Extract()

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB362049: DWARFDebugInfoEntry: delete unused Extract() and 
rename FastExtract() to… (authored by MaskRay, committed by ).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62593?vs=201916=202095#toc

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62593

Files:
  source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

Index: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -31,9 +31,12 @@
 using namespace std;
 extern int g_verbose;
 
-bool DWARFDebugInfoEntry::FastExtract(
-const DWARFDataExtractor _info_data, const DWARFUnit *cu,
-lldb::offset_t *offset_ptr) {
+// Extract a debug info entry for a given compile unit from the .debug_info and
+// .debug_abbrev data within the SymbolFileDWARF class starting at the given
+// offset
+bool DWARFDebugInfoEntry::Extract(const DWARFDataExtractor _info_data,
+  const DWARFUnit *cu,
+  lldb::offset_t *offset_ptr) {
   m_offset = *offset_ptr;
   m_parent_idx = 0;
   m_sibling_idx = 0;
@@ -198,168 +201,6 @@
   return false;
 }
 
-// Extract
-//
-// Extract a debug info entry for a given compile unit from the .debug_info and
-// .debug_abbrev data within the SymbolFileDWARF class starting at the given
-// offset
-bool DWARFDebugInfoEntry::Extract(const DWARFUnit *cu,
-  lldb::offset_t *offset_ptr) {
-  const DWARFDataExtractor _info_data = cu->GetData();
-  //const DWARFDataExtractor& debug_str_data =
-  //dwarf2Data->get_debug_str_data();
-  const uint32_t cu_end_offset = cu->GetNextUnitOffset();
-  lldb::offset_t offset = *offset_ptr;
-  //  if (offset >= cu_end_offset)
-  //  Log::Status("DIE at offset 0x%8.8x is beyond the end of the current
-  //  compile unit (0x%8.8x)", m_offset, cu_end_offset);
-  if ((offset < cu_end_offset) && debug_info_data.ValidOffset(offset)) {
-m_offset = offset;
-
-const uint64_t abbr_idx = debug_info_data.GetULEB128();
-lldbassert(abbr_idx <= UINT16_MAX);
-m_abbr_idx = abbr_idx;
-if (abbr_idx) {
-  const DWARFAbbreviationDeclaration *abbrevDecl =
-  cu->GetAbbreviations()->GetAbbreviationDeclaration(abbr_idx);
-
-  if (abbrevDecl) {
-m_tag = abbrevDecl->Tag();
-m_has_children = abbrevDecl->HasChildren();
-
-bool isCompileUnitTag = (m_tag == DW_TAG_compile_unit ||
- m_tag == DW_TAG_partial_unit);
-if (cu && isCompileUnitTag)
-  const_cast(cu)->SetBaseAddress(0);
-
-// Skip all data in the .debug_info for the attributes
-const uint32_t numAttributes = abbrevDecl->NumAttributes();
-for (uint32_t i = 0; i < numAttributes; ++i) {
-  DWARFFormValue form_value(cu);
-  dw_attr_t attr;
-  abbrevDecl->GetAttrAndFormValueByIndex(i, attr, form_value);
-  dw_form_t form = form_value.Form();
-
-  if (isCompileUnitTag &&
-  ((attr == DW_AT_entry_pc) || (attr == DW_AT_low_pc))) {
-if (form_value.ExtractValue(debug_info_data, )) {
-  if (attr == DW_AT_low_pc || attr == DW_AT_entry_pc)
-const_cast(cu)->SetBaseAddress(
-form_value.Address());
-}
-  } else {
-bool form_is_indirect = false;
-do {
-  form_is_indirect = false;
-  uint32_t form_size = 0;
-  switch (form) {
-  // Blocks if inlined data that have a length field and the data
-  // bytes inlined in the .debug_info
-  case DW_FORM_exprloc:
-  case DW_FORM_block:
-form_size = debug_info_data.GetULEB128();
-break;
-  case DW_FORM_block1:
-form_size = debug_info_data.GetU8();
-break;
-  case DW_FORM_block2:
-form_size = debug_info_data.GetU16();
-break;
-  case DW_FORM_block4:
-form_size = debug_info_data.GetU32();
-break;
-
-  // Inlined NULL terminated C-strings
-  case DW_FORM_string:
-debug_info_data.GetCStr();
-break;
-
-  // Compile unit address sized values
-  case DW_FORM_addr:
-form_size = cu->GetAddressByteSize();
-break;
-  case DW_FORM_ref_addr:
-if (cu->GetVersion() <= 2)
- 

[Lldb-commits] [PATCH] D64398: [LLDB] Fix FreeBSD build

[Fangrui Song via Phabricator via lldb-commits]

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

I think the description should mention that this is to fix FreeBSD build after 
the LaunchThread interface change.


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

https://reviews.llvm.org/D64398



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

[Lldb-commits] [PATCH] D64398: [LLDB] Fix FreeBSD build

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp:798
+  if (!m_operation_thread) {
+ Status e("Start launch thread failed");
+ error = e;

See D64163/r365226


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D64398



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

[Lldb-commits] [PATCH] D64398: [LLDB] Fix FreeBSD build

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D64398#1575713 , @devnexen wrote:

> By the way quick question, I did not commit since quite a time, is it still 
> subversion ?


Many people switched to the git monorepo github.com/llvm/llvm-project && `git 
llvm push` (I think it invokes svn under the hood)
The subversion repo still works.
I'm still using the git multirepo && `git svn dcommit`


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

https://reviews.llvm.org/D64398



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

[Lldb-commits] [PATCH] D67644: [ScriptInterpreter] Initialize globals when loading a scripting module.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/lit/Commands/Inputs/frame.py:3
+print(lldb.frame)
\ No newline at end of file


No newline at end of file


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67644



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

[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/common/LZMA.cpp:124
+   llvm::SmallVectorImpl ) {
+  if (InputBuffer.size() == 0)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),

empty



Comment at: lldb/source/Host/common/LZMA.cpp:129
+  uint64_t uncompressedSize = 0;
+  auto err = getUncompressedSize(InputBuffer, uncompressedSize);
+  if (err)

if (auto err = ...)
  return err;



Comment at: lldb/source/Host/common/LZMA.cpp:136
+  // Decompress xz buffer to buffer.
+  uint64_t memlimit(UINT64_MAX);
+  size_t inpos = 0;

= is more common



Comment at: lldb/source/Host/common/LZMA.cpp:138
+  size_t inpos = 0;
+  inpos = 0;
+  size_t outpos = 0;

stray `inpos = 0;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791



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

[Lldb-commits] [PATCH] D67520: Add pretty printing of Clang "bitfield" enums

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: source/Symbol/ClangASTContext.cpp:9529
+  // in `enum {A, B, ALL = A|B }` we visit ALL first.
+  std::stable_sort(
+  values.begin(), values.end(), [](const auto , const auto ) {

You can simply sort by magnitude and iterating the elements from the largest to 
the smallest.


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

https://reviews.llvm.org/D67520



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

[Lldb-commits] [PATCH] D65789: A more robust way of testing debug_line parser near the end of module

[Fangrui Song via Phabricator via lldb-commits]

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



Comment at: lit/SymbolFile/DWARF/Inputs/debug-line-basic.script:6
+  .shstrtab : { *(.shstrtab) }
+  .debug_info0 : { *(.debug_info  ) }
+  .debug_line0 : { *(.debug_line  ) }

`0` (output section address, `addrExpr` in lld) can be deleted. For 
non-SHF_ALLOC sections (`.shstrtab` `.debug_info` etc), the address expression 
is just ignored.

// lld/ELF/LinkerScript.cpp#L772
  if ((sec->flags & SHF_ALLOC) && sec->addrExpr)
setDot(sec->addrExpr, sec->location, false);


(I guess it was retrieved from `ld --verbose`)


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

https://reviews.llvm.org/D65789



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

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml:44
+# LOOKUP-LABEL: image lookup -a 0x1010
+# LOOKUP:   Address: {{.*}}.PT_LOAD[0]..data + 16)
+

Do you mind explaining more how you'd like to improve file-address-based 
lookups for PT_TLS?

> (lldb) image lookup -a 0x1010
>
>  Address: a.o[0x1010] (a.o.PT_LOAD[0]..tdata + 0)

This is the current output before the change.

Yes PT_TLS can be seen as a separate address space. At runtime a TLS block is 
allocated (mmap) for each thread and they access TLS through their thread 
pointer. The address will be very different from the PT_LOAD address.


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

https://reviews.llvm.org/D65282



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

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

> Are you referring to the "image lookup" command specifically, or is it a more 
> general question about the internals of lldb too?

Both:) This patch doesn't change the `Address: a.o[0x1010] 
(a.o.PT_LOAD[0]..tdata + 0)` output so I was puzzled what this patch intends to 
do.

> However, I can see how it might be interesting to be able to see the initial 
> value of a thread local variable much like we can display the initial value 
> of a global variable without launching a process. For this case, a flag to 
> Section::ContainsFileAddress saying "yes, I want to look up in thread-local 
> sections now" would suffice, but I don't know if this is the only use case...

Yes, inspecting the initial value of a thread-local variable is a use case. To 
that end, can this be done by introducing another member variable instead of 
overloading `m_sections_up` with a new purpose (adding `PT_TLS`)? If PT_TLS is 
recorded in a different variable, the change below can be deleted.

   bool Section::ContainsFileAddress(addr_t vm_addr) const {
 const addr_t file_addr = GetFileAddress();
  -  if (file_addr != LLDB_INVALID_ADDRESS) {
  +  if (file_addr != LLDB_INVALID_ADDRESS && !IsThreadSpecific()) {

(An adjacent pair of PT_LOAD segments can load the same file contents, e.g. 
PT_LOAD `[0x150, 0x1234)` and `[0x1234, 0x1800)` will transform to mmap calls 
with ranges `[0, 0x2000)` and `[0x1000, 0x2000)` at runtime if the runtime page 
size = 0x1000. They share one page in the file. If you ask what a specific 
offset in the file is mapped to, there can be multiple PT_LOAD segments 
(physical -> VMA is not unique). Fortunately the reverse mapping VMA -> 
physical offset can be treated as unique in practice 
(`[p_vaddr,p_vaddr+p_memsz)` ranges do not overlap).)




Comment at: lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml:68
+Flags:   [ SHF_ALLOC, SHF_WRITE, SHF_TLS ]
+Address: 0x1010
+AddressAlign:0x4

labath wrote:
> MaskRay wrote:
> > > .tbss 0x1000 NOBITS
> > >
> > > .tdata 0x1010 PROGBITS
> > 
> > Move .tdata before .tbss (0xff0) to make the example more realistic?
> > 
> > .tdata has a larger address than .tbss. I think this is impossible in 
> > ld.bfd, but you can make .tbss go before .tdata with a broken lld linker 
> > script.
> I'll change the order here. The thing I was trying to test here is that 
> addresses in .data are found regardless of whether it comes before or after a 
> tls section. I think already having two TLS segments is somewhat unrealistic, 
> and I could make it more real by splitting this into two tests, but it did 
> not seem necessary, as lldb does not care about details like this.
Multiple PT_TLS is unrealistic. None of glibc/musl/FreeBSD rtld supports more 
than 1 PT_TLS (they will just pick the last one and ignore the others).


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

https://reviews.llvm.org/D65282



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

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml:62
+Flags:   [ SHF_ALLOC, SHF_WRITE ]
+Address: 0x1000
+AddressAlign:0x4

`.data = .tbss = 0x1010` is a more realistic scenario.

Normally, a SHT_PROGBITS section may overlap with a SHT_NOBITS section, but two 
SHT_PROGBITS sections do not overlap (ld has an on-by-default check `ld 
--check-sections`). Linkers allocate file bytes for SHT_PROGBITS sections so 
their occupied bytes cannot be reused by other sections (without fixing 
addresses with a linker script).


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

https://reviews.llvm.org/D65282



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

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Fangrui Song via Phabricator via lldb-commits]

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

In D65282#1602293 , @labath wrote:

> In D65282#1602244 , @MaskRay wrote:
>
> > > Are you referring to the "image lookup" command specifically, or is it a 
> > > more general question about the internals of lldb too?
> >
> > Both:) This patch doesn't change the `Address: a.o[0x1010] 
> > (a.o.PT_LOAD[0]..tdata + 0)` output so I was puzzled what this patch 
> > intends to do.
>
>
> What do you mean by "doesn't change"? After this patch the addresses always 
> resolve to the .data section..
>
> > 
> > 
> >> However, I can see how it might be interesting to be able to see the 
> >> initial value of a thread local variable much like we can display the 
> >> initial value of a global variable without launching a process. For this 
> >> case, a flag to Section::ContainsFileAddress saying "yes, I want to look 
> >> up in thread-local sections now" would suffice, but I don't know if this 
> >> is the only use case...
> > 
> > Yes, inspecting the initial value of a thread-local variable is a use case. 
> > To that end, can this be done by introducing another member variable 
> > instead of overloading `m_sections_up` with a new purpose (adding 
> > `PT_TLS`)? If PT_TLS is recorded in a different variable, the change below 
> > can be deleted.
>
> I think that would be pretty significant departure from the current design of 
> lldb. Lldb expects that the "section list" of a module will contain all of 
> the module's sections (and before I started messing with these functions, it 
> did). This includes non-loadable sections like .debug_info et al. While one 
> could concieve a world where tls sections are in a special "tls" section 
> list, I am not sure this is actually useful -- if we're going to think of the 
> tls addresses as address spaces, then its reasonable to have more than two 
> address spaces one day (there are people interested in that), and so we 
> couldn't have a fixed set of section lists.


I see. If that would be a significant departure, the current approach should be 
the choice. I didn't non-SHF_ALLOC sections are also in the list. If 
.debug_info et all are in the list, I don't see any problem to have PT_TLS in 
the list since those PT_LOAD are already in the list.

>> 
>> 
>>bool Section::ContainsFileAddress(addr_t vm_addr) const {
>>  const addr_t file_addr = GetFileAddress();
>>   -  if (file_addr != LLDB_INVALID_ADDRESS) {
>>   +  if (file_addr != LLDB_INVALID_ADDRESS && !IsThreadSpecific()) {
>> 
>> 
>> (An adjacent pair of PT_LOAD segments can load the same file contents, e.g. 
>> PT_LOAD `[0x150, 0x1234)` and `[0x1234, 0x1800)` will transform to mmap 
>> calls with ranges `[0, 0x2000)` and `[0x1000, 0x2000)` at runtime if the 
>> runtime page size = 0x1000. They share one page in the file. If you ask what 
>> a specific offset in the file is mapped to, there can be multiple PT_LOAD 
>> segments (physical -> VMA is not unique). Fortunately the reverse mapping 
>> VMA -> physical offset can be treated as unique in practice 
>> (`[p_vaddr,p_vaddr+p_memsz)` ranges do not overlap).)
> 
> I am not 100% what you mean by this, but I think there's some confusion about 
> names of things here. In lldb terms, a "file address" is the "load address, 
> as it is written in the file. It is not the "physical offset within the 
> file", which lldb calls "file offset". Unfortunately, this terminology has 
> caused a lot of confusion in the past, but I don't know what would be the 
> best way to resolve this. How does lld call these things? I guess there's 
> less confusion there as lld does not have to care about real, memory, load 
> addresses...

Maybe we can refer to these things with ELF terminology: p_offset (offsets in 
the file)/p_vaddr (VMA)/p_paddr (LMA)...




Comment at: lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml:62
+Flags:   [ SHF_ALLOC, SHF_WRITE ]
+Address: 0x1000
+AddressAlign:0x4

labath wrote:
> MaskRay wrote:
> > `.data = .tbss = 0x1010` is a more realistic scenario.
> > 
> > Normally, a SHT_PROGBITS section may overlap with a SHT_NOBITS section, but 
> > two SHT_PROGBITS sections do not overlap (ld has an on-by-default check `ld 
> > --check-sections`). Linkers allocate file bytes for SHT_PROGBITS sections 
> > so their occupied bytes cannot be reused by other sections (without fixing 
> > addresses with a linker script).
> Interesting. I can that easily, but I'm wondering, do you know the reason for 
> that? Is it just how it falls out of the default linker processing of things, 
> or would something actually break if I assigned identical addresses to two 
> SHT_PROGBITS sections?
https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L2412

SHT_PROGBITS sections occupy space in the file but SHT_NOBITS sections 

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

> It turns out this was too aggressive because thread-local

sections typically will have file addresses which apear to overlap
regular data/code. This does not cause a problem at runtime because
thread-local sections are loaded into memory using special logic, but it
can cause problems for lldb when trying to lookup objects by their file
address.

Yes :) This can happen with .tbss (SHT_NOBITS) overlapping another section 
(usually .fini_array, but .got and .data are also possible).




Comment at: lit/Modules/ELF/PT_LOAD-overlap-PT_TLS.yaml:68
+Flags:   [ SHF_ALLOC, SHF_WRITE, SHF_TLS ]
+Address: 0x1010
+AddressAlign:0x4

> .tbss 0x1000 NOBITS
>
> .tdata 0x1010 PROGBITS

Move .tdata before .tbss (0xff0) to make the example more realistic?

.tdata has a larger address than .tbss. I think this is impossible in ld.bfd, 
but you can make .tbss go before .tdata with a broken lld linker script.


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

https://reviews.llvm.org/D65282



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

[Lldb-commits] [PATCH] D70883: [vscode.py] Make read_packet only return None when EOF

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:58
+while True:
+line = f.readline().decode("utf-8")
+if len(line) == 0:

Prefer single quotes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70883



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

[Lldb-commits] [PATCH] D68657: Update MinidumpYAML to use minidump::Exception for exception stream

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/ObjectYAML/MinidumpYAML.cpp:389
+  for (size_t Index = 0; Index < Exception.MaxParameters; ++Index) {
+SmallString<16> Name = formatv("Parameter {0}", Index);
+support::ulittle64_t  = Exception.ExceptionInformation[Index];

You may use `("Parameter " + Twine(Index)).str()` to get rid of the 
"llvm/Support/FormatVariadic.h" dependency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68657



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

[Lldb-commits] [PATCH] D68943: [llvm][yaml2obj] no more implicit ELF .symtab section

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D68943#1709998 , @grimar wrote:

> Ok, I was able to debug it finally.
>
> I think we should add a .symtab in a few more cases implicitly.
>  For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
>  i.e. it refers to .symtab by default and we should provide it.
>  There are other cases, like when Link just explicitly refers to .symtab.
>
> Here is a patch based on this one.
>  It is unpolished, but shows the idea. With it all yaml2obj tests pass.
>
> F10276544: patch.diff 


Nice! Can you post a differential for the yaml2obj change? I believe the 
differential can focus on lldb and remove yaml2obj changes (kwk will still get 
credited for the test cleanups).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943



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

[Lldb-commits] [PATCH] D68872: SBCommandReturnObject: change LLDB_RECORD_METHOD(..., FILE *, ...) to use LLDB_RECORD_DUMMY

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG56ee31964f5a: SBCommandReturnObject: change 
LLDB_RECORD_METHOD(..., FILE *, ...) to use… (authored by MaskRay).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D68872?vs=225155=225338#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68872

Files:
  lldb/source/API/SBCommandReturnObject.cpp


Index: lldb/source/API/SBCommandReturnObject.cpp
===
--- lldb/source/API/SBCommandReturnObject.cpp
+++ lldb/source/API/SBCommandReturnObject.cpp
@@ -116,7 +116,7 @@
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
   if (fh) {
 size_t num_bytes = GetOutputSize();
 if (num_bytes)
@@ -141,8 +141,7 @@
 }
 
 size_t SBCommandReturnObject::PutError(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
-
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
   if (fh) {
 size_t num_bytes = GetErrorSize();
 if (num_bytes)
@@ -255,31 +254,31 @@
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
- (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
+(FILE *), fh);
 
   SetImmediateOutputFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
- (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateErrorFile,
+(FILE *), fh);
 
   SetImmediateErrorFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh,
bool transfer_ownership) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
- (FILE *, bool), fh, transfer_ownership);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
+(FILE *, bool), fh, transfer_ownership);
   FileSP file = std::make_shared(fh, transfer_ownership);
   ref().SetImmediateOutputFile(file);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh,
   bool transfer_ownership) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
- (FILE *, bool), fh, transfer_ownership);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateErrorFile,
+(FILE *, bool), fh, transfer_ownership);
   FileSP file = std::make_shared(fh, transfer_ownership);
   ref().SetImmediateErrorFile(file);
 }


Index: lldb/source/API/SBCommandReturnObject.cpp
===
--- lldb/source/API/SBCommandReturnObject.cpp
+++ lldb/source/API/SBCommandReturnObject.cpp
@@ -116,7 +116,7 @@
 }
 
 size_t SBCommandReturnObject::PutOutput(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutOutput, (FILE *), fh);
   if (fh) {
 size_t num_bytes = GetOutputSize();
 if (num_bytes)
@@ -141,8 +141,7 @@
 }
 
 size_t SBCommandReturnObject::PutError(FILE *fh) {
-  LLDB_RECORD_METHOD(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
-
+  LLDB_RECORD_DUMMY(size_t, SBCommandReturnObject, PutError, (FILE *), fh);
   if (fh) {
 size_t num_bytes = GetErrorSize();
 if (num_bytes)
@@ -255,31 +254,31 @@
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
- (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
+(FILE *), fh);
 
   SetImmediateOutputFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateErrorFile(FILE *fh) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateErrorFile,
- (FILE *), fh);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateErrorFile,
+(FILE *), fh);
 
   SetImmediateErrorFile(fh, false);
 }
 
 void SBCommandReturnObject::SetImmediateOutputFile(FILE *fh,
bool transfer_ownership) {
-  LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetImmediateOutputFile,
- (FILE *, bool), fh, transfer_ownership);
+  LLDB_RECORD_DUMMY(void, SBCommandReturnObject, SetImmediateOutputFile,
+(FILE *, bool), fh, 

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D49466#1761044 , @thakis wrote:

> There's still one failing test on Windows after the fix attempt: 
> http://45.33.8.238/win/3052/step_6.txt
>
> Please take a look and revert if it's not an easy fix. (And please watch bots 
> after committing stuff.)


XFAIL'ed `clang/test/Preprocessor/file_test.c`. I didn't receive an email from 
http://45.33.8.238/win/3052/step_6.txt not sure if that was because the author 
email was @dankm's, not mine. If there is a console. please tell me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466



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

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The ugly path separator pattern `{{(/|)}}` appears in 60+ tests. Can we 
teach clang and other tools to

1. accept both `/` and `\` input
2. but only output `/`

on Windows? We can probably remove 
`llvm::sys::path::Style::{windows,posix,native}` from 
`include/llvm/Support/Path.h` and only keep the posix form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466



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

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Does this work on Windows?

  --- i/clang/test/Preprocessor/file_test.c
  +++ w/clang/test/Preprocessor/file_test.c
  @@ -1,8 +1,7 @@
  -// XFAIL: system-windows
   // RUN: %clang -E -ffile-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | 
FileCheck %s
   // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | 
FileCheck %s
   // RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH=empty -c -o - %s | 
FileCheck %s -check-prefix CHECK-EVIL
  -// RUN: %clang -E -fmacro-prefix-map=%p/= -c -o - %s | FileCheck %s 
--check-prefix CHECK-REMOVE
  +// RUN: %clang -E -fmacro-prefix-map=%/p/= -c -o - %/s | FileCheck %s 
--check-prefix CHECK-REMOVE

`startswith` is not ideal because `/t` will match `/tmp`. However, gcc appears 
to use something similar to `startswith`, see:

  % gcc -c -g -fdebug-prefix-map=/t=x /tmp/c/a.c -o /tmp/c/a.o
  % llvm-dwarfdump /tmp/c/a.o | grep -m 1 DW_AT_name
DW_AT_name("xmp/c/a.c")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466



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

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 231142.
MaskRay added a comment.

Minimize diff in Options.td
Properly rebase remapDIPath on top of D69213 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/test/Driver/debug-prefix-map.S
  clang/test/Driver/debug-prefix-map.c
  clang/test/Preprocessor/file_test.c
  clang/test/Preprocessor/file_test.h
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1230,7 +1230,9 @@
 TEST(Support, ReplacePathPrefix) {
   SmallString<64> Path1("/foo");
   SmallString<64> Path2("/old/foo");
+  SmallString<64> Path3("/oldnew/foo");
   SmallString<64> OldPrefix("/old");
+  SmallString<64> OldPrefixSep("/old/");
   SmallString<64> NewPrefix("/new");
   SmallString<64> NewPrefix2("/longernew");
   SmallString<64> EmptyPrefix("");
@@ -1250,6 +1252,33 @@
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
   EXPECT_EQ(Path, "/foo");
+  Path = Path2;
+  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix, true);
+  EXPECT_EQ(Path, "foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, false);
+  EXPECT_EQ(Path, "/newnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path1;
+  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/foo");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new");
+  Path = OldPrefixSep;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, false);
+  EXPECT_EQ(Path, "/old");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/new");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -496,27 +496,50 @@
   path.append(ext.begin(), ext.end());
 }
 
-void replace_path_prefix(SmallVectorImpl ,
+bool replace_path_prefix(SmallVectorImpl ,
  const StringRef , const StringRef ,
- Style style) {
+ Style style, bool strict) {
   if (OldPrefix.empty() && NewPrefix.empty())
-return;
+return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
-return;
+  StringRef OldPrefixDir;
+
+  if (!strict && OldPrefix.size() > OrigPath.size())
+return false;
+
+  // Ensure OldPrefixDir does not have a trailing separator.
+  if (!OldPrefix.empty() && is_separator(OldPrefix.back()))
+OldPrefixDir = parent_path(OldPrefix, style);
+  else
+OldPrefixDir = OldPrefix;
+
+  if (!OrigPath.startswith(OldPrefixDir))
+return false;
+
+  if (OrigPath.size() > OldPrefixDir.size())
+if (!is_separator(OrigPath[OldPrefixDir.size()], style) && strict)
+  return false;
 
   // If prefixes have the same size we can simply copy the new one over.
-  if (OldPrefix.size() == NewPrefix.size()) {
+  if (OldPrefixDir.size() == NewPrefix.size() && !strict) {
 llvm::copy(NewPrefix, Path.begin());
-return;
+return true;
   }
 
-  StringRef RelPath = OrigPath.substr(OldPrefix.size());
+  StringRef RelPath = OrigPath.substr(OldPrefixDir.size());
   SmallString<256> NewPath;
   path::append(NewPath, style, NewPrefix);
-  path::append(NewPath, style, RelPath);
+  if (!RelPath.empty()) {
+if (!is_separator(RelPath[0], style) || !strict)
+  path::append(NewPath, style, RelPath);
+else
+  path::append(NewPath, style, relative_path(RelPath, style));
+  }
+
   Path.swap(NewPath);
+
+  return true;
 }
 
 void native(const Twine , SmallVectorImpl , Style style) {
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -152,18 +152,33 @@
 ///
 /// @code
 ///   

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 231140.
MaskRay added a comment.

Add back remapDIPath that was unintentionally deleted by D69213 
, caught by a test.

Small adjustment of the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/test/Driver/debug-prefix-map.S
  clang/test/Driver/debug-prefix-map.c
  clang/test/Preprocessor/file_test.c
  clang/test/Preprocessor/file_test.h
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1230,7 +1230,9 @@
 TEST(Support, ReplacePathPrefix) {
   SmallString<64> Path1("/foo");
   SmallString<64> Path2("/old/foo");
+  SmallString<64> Path3("/oldnew/foo");
   SmallString<64> OldPrefix("/old");
+  SmallString<64> OldPrefixSep("/old/");
   SmallString<64> NewPrefix("/new");
   SmallString<64> NewPrefix2("/longernew");
   SmallString<64> EmptyPrefix("");
@@ -1250,6 +1252,33 @@
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
   EXPECT_EQ(Path, "/foo");
+  Path = Path2;
+  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix, true);
+  EXPECT_EQ(Path, "foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, false);
+  EXPECT_EQ(Path, "/newnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path1;
+  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/foo");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new");
+  Path = OldPrefixSep;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, false);
+  EXPECT_EQ(Path, "/old");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/new");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -496,27 +496,50 @@
   path.append(ext.begin(), ext.end());
 }
 
-void replace_path_prefix(SmallVectorImpl ,
+bool replace_path_prefix(SmallVectorImpl ,
  const StringRef , const StringRef ,
- Style style) {
+ Style style, bool strict) {
   if (OldPrefix.empty() && NewPrefix.empty())
-return;
+return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
-return;
+  StringRef OldPrefixDir;
+
+  if (!strict && OldPrefix.size() > OrigPath.size())
+return false;
+
+  // Ensure OldPrefixDir does not have a trailing separator.
+  if (!OldPrefix.empty() && is_separator(OldPrefix.back()))
+OldPrefixDir = parent_path(OldPrefix, style);
+  else
+OldPrefixDir = OldPrefix;
+
+  if (!OrigPath.startswith(OldPrefixDir))
+return false;
+
+  if (OrigPath.size() > OldPrefixDir.size())
+if (!is_separator(OrigPath[OldPrefixDir.size()], style) && strict)
+  return false;
 
   // If prefixes have the same size we can simply copy the new one over.
-  if (OldPrefix.size() == NewPrefix.size()) {
+  if (OldPrefixDir.size() == NewPrefix.size() && !strict) {
 llvm::copy(NewPrefix, Path.begin());
-return;
+return true;
   }
 
-  StringRef RelPath = OrigPath.substr(OldPrefix.size());
+  StringRef RelPath = OrigPath.substr(OldPrefixDir.size());
   SmallString<256> NewPath;
   path::append(NewPath, style, NewPrefix);
-  path::append(NewPath, style, RelPath);
+  if (!RelPath.empty()) {
+if (!is_separator(RelPath[0], style) || !strict)
+  path::append(NewPath, style, RelPath);
+else
+  path::append(NewPath, style, relative_path(RelPath, style));
+  }
+
   Path.swap(NewPath);
+
+  return true;
 }
 
 void native(const Twine , SmallVectorImpl , Style style) {
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ 

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6c92cdff7225: Initial implementation of -fmacro-prefix-map 
and -ffile-prefix-map (authored by dankm, committed by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/PreprocessorOptions.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/test/Driver/debug-prefix-map.S
  clang/test/Driver/debug-prefix-map.c
  clang/test/Preprocessor/file_test.c
  clang/test/Preprocessor/file_test.h
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1230,7 +1230,9 @@
 TEST(Support, ReplacePathPrefix) {
   SmallString<64> Path1("/foo");
   SmallString<64> Path2("/old/foo");
+  SmallString<64> Path3("/oldnew/foo");
   SmallString<64> OldPrefix("/old");
+  SmallString<64> OldPrefixSep("/old/");
   SmallString<64> NewPrefix("/new");
   SmallString<64> NewPrefix2("/longernew");
   SmallString<64> EmptyPrefix("");
@@ -1250,6 +1252,33 @@
   Path = Path2;
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
   EXPECT_EQ(Path, "/foo");
+  Path = Path2;
+  path::replace_path_prefix(Path, OldPrefix, EmptyPrefix, true);
+  EXPECT_EQ(Path, "foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, false);
+  EXPECT_EQ(Path, "/newnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path3;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path1;
+  path::replace_path_prefix(Path, EmptyPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/foo");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new");
+  Path = OldPrefixSep;
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, false);
+  EXPECT_EQ(Path, "/old");
+  Path = OldPrefix;
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix, true);
+  EXPECT_EQ(Path, "/new");
 }
 
 TEST_F(FileSystemTest, OpenFileForRead) {
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -496,27 +496,50 @@
   path.append(ext.begin(), ext.end());
 }
 
-void replace_path_prefix(SmallVectorImpl ,
+bool replace_path_prefix(SmallVectorImpl ,
  const StringRef , const StringRef ,
- Style style) {
+ Style style, bool strict) {
   if (OldPrefix.empty() && NewPrefix.empty())
-return;
+return false;
 
   StringRef OrigPath(Path.begin(), Path.size());
-  if (!OrigPath.startswith(OldPrefix))
-return;
+  StringRef OldPrefixDir;
+
+  if (!strict && OldPrefix.size() > OrigPath.size())
+return false;
+
+  // Ensure OldPrefixDir does not have a trailing separator.
+  if (!OldPrefix.empty() && is_separator(OldPrefix.back()))
+OldPrefixDir = parent_path(OldPrefix, style);
+  else
+OldPrefixDir = OldPrefix;
+
+  if (!OrigPath.startswith(OldPrefixDir))
+return false;
+
+  if (OrigPath.size() > OldPrefixDir.size())
+if (!is_separator(OrigPath[OldPrefixDir.size()], style) && strict)
+  return false;
 
   // If prefixes have the same size we can simply copy the new one over.
-  if (OldPrefix.size() == NewPrefix.size()) {
+  if (OldPrefixDir.size() == NewPrefix.size() && !strict) {
 llvm::copy(NewPrefix, Path.begin());
-return;
+return true;
   }
 
-  StringRef RelPath = OrigPath.substr(OldPrefix.size());
+  StringRef RelPath = OrigPath.substr(OldPrefixDir.size());
   SmallString<256> NewPath;
   path::append(NewPath, style, NewPrefix);
-  path::append(NewPath, style, RelPath);
+  if (!RelPath.empty()) {
+if (!is_separator(RelPath[0], style) || !strict)
+  path::append(NewPath, style, RelPath);
+else
+  path::append(NewPath, style, relative_path(RelPath, style));
+  }
+
   Path.swap(NewPath);
+
+  return true;
 }
 
 void native(const Twine , SmallVectorImpl , Style style) {
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ 

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D49466#1760765 , @dankm wrote:

> Ping?


The tests need fixing... I can commit it. Now that we've migrated to the llvm 
monorepo, the git commit message can retain the author info properly...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

This was probably due to:

Expected(Error Err)
: HasError(true)
  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
  // Expected is unchecked upon construction in Debug builds.
  , Unchecked(true)
  #endif
{
  assert(Err && "Cannot create Expected from Error success value.");
  new (getErrorStorage()) error_type(Err.takePayload());
}

The member initialization makes the members unchecked 
`m_operation_thread(nullptr)`. Adding something like 
`(void)!m_operation_thread` before assignment to `m_operation_thread` is 
probably better.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68645: MinidumpYAML: Add support for the memory info list stream

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:111
+
+  explicit MemoryInfoListStream(std::vector Infos)
+  : Stream(StreamKind::MemoryInfoList,

grimar wrote:
> Maybe be more explicit here, i.e.
> 
> ```
> std::vector &
> ```
> ?
Or let the constructor take `iterator_range` 
as the argument, and change the call site below.



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:116
+
+  MemoryInfoListStream()
+  : Stream(StreamKind::MemoryInfoList,

Move default constructor above.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68645



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D68723#1703322 , @devnexen wrote:

> @dim I LGTMed this but would it a real issue for you to take on a less 
> disruptive approach proposed by MaskRay ?


I think we should do what @labath suggested (changing Expected to Optional) if 
that is not very difficult.


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68723: Fix process launch failure on FreeBSD after r365761

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D68723#1703051 , @devnexen wrote:

> LGTM otherwise.


I don't think this change should be reverted. It can just be repaired by adding 
some `(void)!xxx;`


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

https://reviews.llvm.org/D68723



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

[Lldb-commits] [PATCH] D68943: [llvm][yaml2obj] no more implicit ELF .symtab section

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

This is not correct. A lot of yaml2obj tests fail with:

> yaml2obj: error: unknown section referenced: '.symtab' by YAML section 
> '.rela.text'

The problem is that .symtab can be the sh_link field of relocation sections, 
.llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look 
tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68943



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

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS AND UNIX

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: clayborg, JDevlieghere, jingham, labath.
Herald added subscribers: lldb-commits, kristof.beyls, mgorny.
Herald added a project: LLDB.

For -DBUILD_SHARED_LIBS=On builds, lib/liblldb.so depends on lib/*.so .
lib/python2.7/dist-packages/lldb/_lldb.so is a symlink to
lib/liblldb.so, which needs an additional rpath
`$ORIGIN/../../../../lib` to load. This fixes an import error from
lib/python2.7/dist-packages/lldb/__init__.py

  from . import _lldb
  ImportError: libLLVMAArch64CodeGen.so.10git: cannot open shared object file: 
No such file or directory


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71800

Files:
  lldb/source/API/CMakeLists.txt


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,19 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so .
+# Add an additional rpath \$ORIGIN/../../../../lib so that _lldb.so can be
+# loaded from Python.
+if(LLDB_ENABLE_PYTHON AND BUILD_SHARED_LIBS AND UNIX)
+  if(LLVM_INSTALL_PREFIX AND NOT (LLVM_INSTALL_PREFIX STREQUAL 
CMAKE_INSTALL_PREFIX))
+set(extra_libdir ${LLVM_LIBRARY_DIR})
+  elseif(LLVM_BUILD_LIBRARY_DIR)
+set(extra_libdir ${LLVM_LIBRARY_DIR})
+  endif()
+  set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+  set_target_properties(liblldb PROPERTIES INSTALL_RPATH "${_install_rpath}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS 
/bigobj)
 endif()


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,19 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so .
+# Add an additional rpath \$ORIGIN/../../../../lib so that _lldb.so can be
+# loaded from Python.
+if(LLDB_ENABLE_PYTHON AND BUILD_SHARED_LIBS AND UNIX)
+  if(LLVM_INSTALL_PREFIX AND NOT (LLVM_INSTALL_PREFIX STREQUAL CMAKE_INSTALL_PREFIX))
+set(extra_libdir ${LLVM_LIBRARY_DIR})
+  elseif(LLVM_BUILD_LIBRARY_DIR)
+set(extra_libdir ${LLVM_LIBRARY_DIR})
+  endif()
+  set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" "\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+  set_target_properties(liblldb PROPERTIES INSTALL_RPATH "${_install_rpath}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS AND UNIX

[Fangrui Song via Phabricator via lldb-commits]

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



Comment at: lldb/source/API/CMakeLists.txt:124
+  set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+  set_target_properties(liblldb PROPERTIES INSTALL_RPATH "${_install_rpath}")
+endif()

The extra_libdir logic is copied from `llvm/cmake/modules/AddLLVM.cmake` 
`llvm_setup_rpath`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71800



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

[Lldb-commits] [PATCH] D71857: [NFC] Fixes -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I really like the new feature. Thanks for making the efforts!

However, I am afraid I don't like some of the fixes here. You can replace 
`const auto` with `const auto &` and call that a fix... IMHO if the type is not 
obvious, `const ConcreteType &` will be better.

In addition, I think there are some false positives, e.g. the diagnostic may 
ask me to replace `const std::pair` with `const std::pair &` when it is clear that the type is trivial and cheap to copy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71857



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

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS or LLVM_LINK_LLVM_DYLIB on *nix

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Ping:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71800



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

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS or LLVM_LINK_LLVM_DYLIB on *nix

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 235170.
MaskRay marked 3 inline comments as done.
MaskRay retitled this revision from "[CMake] Add $ORIGIN/../../../../lib to 
rpath if BUILD_SHARED_LIBS AND UNIX" to "[CMake] Add $ORIGIN/../../../../lib to 
rpath if BUILD_SHARED_LIBS or LLVM_LINK_LLVM_DYLIB on *nix".
MaskRay edited the summary of this revision.
MaskRay added a comment.

Make -DLLVM_LINK_LLVM_DYLIB=ON work


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71800

Files:
  lldb/source/API/CMakeLists.txt


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,14 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
+# which depends on lib/libLLVM*.so (BUILD_SHARED_LIBS) or lib/libLLVM-10git.so
+# (LLVM_LINK_LLVM_DYLIB). Add an additional rpath $ORIGIN/../../../../lib so
+# that _lldb.so can be loaded from Python.
+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}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS 
/bigobj)
 endif()


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,14 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
+# which depends on lib/libLLVM*.so (BUILD_SHARED_LIBS) or lib/libLLVM-10git.so
+# (LLVM_LINK_LLVM_DYLIB). Add an additional rpath $ORIGIN/../../../../lib so
+# that _lldb.so can be loaded from Python.
+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}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS or LLVM_LINK_LLVM_DYLIB on *nix

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/API/CMakeLists.txt:117
+# loaded from Python.
+if(LLDB_ENABLE_PYTHON AND BUILD_SHARED_LIBS AND UNIX)
+  if(LLVM_INSTALL_PREFIX AND NOT (LLVM_INSTALL_PREFIX STREQUAL 
CMAKE_INSTALL_PREFIX))

labath wrote:
> I think this would work on APPLE (which is subsumed by UNIX). Though I don't 
> know if anyone there actually uses BUILD_SHARED_LIBS...
I assume APPLE -DBUILD_SHARED_LIBS=On works as-is. I will add `AND NOT APPLE`.



Comment at: lldb/source/API/CMakeLists.txt:124
+  set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+  set_target_properties(liblldb PROPERTIES INSTALL_RPATH "${_install_rpath}")
+endif()

labath wrote:
> MaskRay wrote:
> > The extra_libdir logic is copied from `llvm/cmake/modules/AddLLVM.cmake` 
> > `llvm_setup_rpath`.
> Instead of copying that you should do something like `set_property(TARGET 
> liblldb APPEND PROPERTY INSTALL_RPATH "whatever")` to just add the new entry.
> 
Thanks for the suggestion! Will apply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71800



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

[Lldb-commits] [PATCH] D71857: [NFC] Fixes -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D71857#1804307 , @Mordante wrote:

> In D71857#1800663 , @MaskRay wrote:
>
> > I think there is a false positive.
> >
> > https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622
> >
> >   for (const std::pair ts : isd->thunkSections)
> >   
>
>
> Removing the `const` like L1648 will also solve the issue.


Yes, both `std::pair` and `auto` work, but `const std::pair` doesn't. std::pair 
has an unfortunate user-defined copy assignment operator 
(https://eel.is/c++draft/pairs#pair-16 "Assigns p.first to first and p.second 
to second." Does this wording mandate a user-defined function?) I will drop 
`const` for this lld file.

For other user-defined types. I think loosening the diagnostic to allow 
trivially copyable types will be nice. Types that take only a few words (say. 
1~3) are likely faster with a copy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71857



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

[Lldb-commits] [PATCH] D71800: [CMake] Add $ORIGIN/../../../../lib to rpath if BUILD_SHARED_LIBS or LLVM_LINK_LLVM_DYLIB on *nix

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbfebd7b8a67: [CMake] Add $ORIGIN/../../../../lib to rpath 
if BUILD_SHARED_LIBS or… (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71800

Files:
  lldb/source/API/CMakeLists.txt


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,14 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
+# which depends on lib/libLLVM*.so (BUILD_SHARED_LIBS) or lib/libLLVM-10git.so
+# (LLVM_LINK_LLVM_DYLIB). Add an additional rpath $ORIGIN/../../../../lib so
+# that _lldb.so can be loaded from Python.
+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}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS 
/bigobj)
 endif()


Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -111,6 +111,14 @@
   ${option_install_prefix}
 )
 
+# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
+# which depends on lib/libLLVM*.so (BUILD_SHARED_LIBS) or lib/libLLVM-10git.so
+# (LLVM_LINK_LLVM_DYLIB). Add an additional rpath $ORIGIN/../../../../lib so
+# that _lldb.so can be loaded from Python.
+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}")
+endif()
+
 if (MSVC)
   set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to disable readability-identifier-naming

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG71269a1f172c: [lldb] Add .clang-tidy with customization to 
disable readability-identifier… (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75810

Files:
  lldb/.clang-tidy


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,2 @@
+# Checks enabled in the top-level .clang-tidy minus 
readability-identifier-naming
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,2 @@
+# Checks enabled in the top-level .clang-tidy minus readability-identifier-naming
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to readability-identifier-naming.{Function, Member, Parameter, Variable}Case

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: JDevlieghere, jingham, labath.
Herald added subscribers: lldb-commits, aheejin.
Herald added a project: LLDB.
MaskRay added a comment.

https://github.com/google/llvm-premerge-checks/issues/142 This should suppress 
the annoying `clang-tidy linux` diagnostics in `Diff Detail - Build Status`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75810

Files:
  lldb/.clang-tidy


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,21 @@
+# Almost identical to the top-level .clang-tidy, except that
+# - FunctionCase uses CamelCase
+# - {Member,Parameter,Variable}Case use lower_case
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+CheckOptions:
+  - key: readability-identifier-naming.ClassCase
+value:   CamelCase
+  - key: readability-identifier-naming.EnumCase
+value:   CamelCase
+  - key: readability-identifier-naming.FunctionCase
+value:   CamelCase
+  - key: readability-identifier-naming.MemberCase
+value:   lower_case
+  - key: readability-identifier-naming.ParameterCase
+value:   lower_case
+  - key: readability-identifier-naming.UnionCase
+value:   CamelCase
+  - key: readability-identifier-naming.VariableCase
+value:   lower_case
+  - key: readability-identifier-naming.IgnoreMainLikeFunctions
+value:   1


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,21 @@
+# Almost identical to the top-level .clang-tidy, except that
+# - FunctionCase uses CamelCase
+# - {Member,Parameter,Variable}Case use lower_case
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+CheckOptions:
+  - key: readability-identifier-naming.ClassCase
+value:   CamelCase
+  - key: readability-identifier-naming.EnumCase
+value:   CamelCase
+  - key: readability-identifier-naming.FunctionCase
+value:   CamelCase
+  - key: readability-identifier-naming.MemberCase
+value:   lower_case
+  - key: readability-identifier-naming.ParameterCase
+value:   lower_case
+  - key: readability-identifier-naming.UnionCase
+value:   CamelCase
+  - key: readability-identifier-naming.VariableCase
+value:   lower_case
+  - key: readability-identifier-naming.IgnoreMainLikeFunctions
+value:   1
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to readability-identifier-naming.{Function, Member, Parameter, Variable}Case

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

https://github.com/google/llvm-premerge-checks/issues/142 This should suppress 
the annoying `clang-tidy linux` diagnostics in `Diff Detail - Build Status`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75810



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

[Lldb-commits] [PATCH] D75810: [lldb] Add .clang-tidy with customization to disable readability-identifier-naming

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 249139.
MaskRay retitled this revision from "[lldb] Add .clang-tidy with customization 
to readability-identifier-naming.{Function,Member,Parameter,Variable}Case" to 
"[lldb] Add .clang-tidy with customization to disable 
readability-identifier-naming".
MaskRay added a comment.

Disable readability-identifier-naming


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75810

Files:
  lldb/.clang-tidy


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,2 @@
+# Checks enabled in the top-level .clang-tidy minus 
readability-identifier-naming
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'


Index: lldb/.clang-tidy
===
--- /dev/null
+++ lldb/.clang-tidy
@@ -0,0 +1,2 @@
+# Checks enabled in the top-level .clang-tidy minus readability-identifier-naming
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:102
+
+Streamer.emitIntValue(ELFAttrs::Format_Version, 1);
+  }

`emitInt8`



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:113
+
+  Streamer.emitIntValue(VendorHeaderSize + TagHeaderSize + ContentsSize, 4);
+  Streamer.emitBytes(CurrentVendor);

emitInt32



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:115
+  Streamer.emitBytes(CurrentVendor);
+  Streamer.emitIntValue(0, 1); // '\0'
+

emitInt8



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:42
+Arch = (Twine(Arch) + "_m2p0").str();
+
+  if (STI.hasFeature(RISCV::FeatureStdExtA))

These empty lines seem unneeded.



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:44
+  if (STI.hasFeature(RISCV::FeatureStdExtA))
+Arch = (Twine(Arch) + "_a2p0").str();
+

Just `Arch += "_a2p0";`



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:93
+   StringRef String) {
+  OS << "\t.attribute\t" << Attribute << ", \"" << String << "\"\n";
+}

Does `String` need to be escaped?



Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:186
+void RISCVAsmPrinter::emitEndOfAsmFile(Module ) {
+  MCTargetStreamer  = *OutStreamer->getTargetStreamer();
+  RISCVTargetStreamer  = static_cast(TS);

`TS` can be inlined.



Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:195
+  MCTargetStreamer  = *OutStreamer->getTargetStreamer();
+  RISCVTargetStreamer  = static_cast(TS);
+

TS can be inlined.



Comment at: llvm/test/CodeGen/RISCV/attributes.ll:25
+
+define i32 @addi(i32 %a) nounwind {
+  %1 = add i32 %a, 1

Delete `nounwind` (unneeded detail)



Comment at: llvm/test/CodeGen/RISCV/attributes.ll:29
+}
+

Delete empty line



Comment at: llvm/test/MC/RISCV/attribute-arch.s:38
+# CHECK: attribute  5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
+

Delete empty line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The code generally looks good. For unittests, I think we can either make 
llvm-readobj -A canonical or the unittests canonical. If we decide to place 
tests on one place, we should delete most tests on the other side.

My current preference is that we use more of unittests and leave the minimum to 
`test/llvm-readobj/ELF/{ARM,RISCV}/`




Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1689
+/// parseDirectiveAttribute
+///  ::= .attribute int, int [, "str"]
+///  ::= .attribute Tag_name, int [, "str"]

No space before `[`



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1723
+
+  StringRef StringValue = "";
+  int64_t IntegerValue = 0;

Delete `= ""`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D77186: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D77186#1955451 , @wallace wrote:

> address comments


@wallace labath made comments after the differential had been approved. So it 
looks like he may have further questions. It was probably better waiting for an 
explicit ack instead of being in a haste.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77186



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

[Lldb-commits] [PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I remember the patch caused many failures yesterday. The new diff is good.




Comment at: llvm/unittests/Support/ELFAttributeParserTest.cpp:16
+
+TagNameMap emptyTagNameMap;
+

`static const`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023



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

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

commit fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 should be 
associated with `Differential Revision: https://reviews.llvm.org/D76111`.

The revert of fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 should 
explain why fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 was 
reverted.

Please don't make a commit whose description contains just one word `fix`.

If you are unsure, upload a new diff to trigger Harbormaster, check its test 
status on 3 Linux/Windows/macOS, instead of committing and reverting like 
playing a game. Every commit has a permanent record which will be shared among 
thousands of developers and can increase the size of the repository. Please be 
careful.

`Subscribers:` and `Tags:` lines are generally considered useless. You can 
strip them with:

  arcfilter () {
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: 
/,"");print}' | git commit --amend -F -
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



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

[Lldb-commits] [PATCH] D86857: [Test] Simplify DWARF test cases. NFC.

[Fangrui Song via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86857

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

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Support/MemoryBuffer.cpp:460
 
-  if (shouldUseMmap(FD, FileSize, MapSize, Offset, RequiresNullTerminator,
-PageSize, IsVolatile)) {
+  if (false) {
 std::error_code EC;

I guess this was unintended. Fixed in 03f1516d6075f42dce95bcf9fde3f6fde97abd35


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86996

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Fangrui Song via Phabricator via lldb-commits]

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

LGTM. Worth mentioning that this will be in POSIX issue 8 
https://www.austingroupbugs.net/bug_view_page.php?bug_id=508




Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

labath wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > labath wrote:
> > > > mgorny wrote:
> > > > > labath wrote:
> > > > > > mgorny wrote:
> > > > > > > I would really feel better with a real error handling here. It 
> > > > > > > shouldn't be hard to use `ErrorOr` here.
> > > > > > Yeah, but what are you going to do with that value? Pass it to the 
> > > > > > caller? The existing callers are ignoring the error return anyway, 
> > > > > > and I don't want to add error handling everywhere as afaict, this 
> > > > > > function can't fail unless the user messes up the master state 
> > > > > > (which is not something I want to support).
> > > > > I get your point but I've literally wasted days because of missing 
> > > > > error handling, so I'd really preferred if we wouldn't make it even 
> > > > > worse. Though I guess `assert` is good enough.
> > > > In some ways it's even better because it will point you straight to the 
> > > > place where the assumption is violated, whereas a propagated logic 
> > > > error can manifest itself much farther away (or not at all). :)
> > > If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
> > > use-of-uninitialized-value error.
> > ... in a -DLLVM_ENABLE_ASSERTIONS=off build.
> > 
> > This probably still needs some protection.
> What kind of protection did you have it mind? Initialize the buffer to an 
> empty string?
In the case of a non-zero return value, `buf[0] = '\0'` is probably sufficient 
to avoid an uninitialized value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/tools/lldb-server/CMakeLists.txt:61
 LINK_COMPONENTS
   Support
 )

Otherwise it fails in a BUILD_SHARED_LIBS=on build because the -Wl,-z,defs 
linker option requires a module to have fully specified dependencies


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89477

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

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:404
+  unsigned MissingArgCount;
+  opt::InputArgList Args = Opts.ParseArgs(makeArrayRef(argv + 2, argc - 2),
+  MissingArgIndex, MissingArgCount);

Consider using `parseArgs` I added in D83639


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89477

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

[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:369
+
+  If no target is selected a startup, the lldb-server can be directed to launch
+  or attach a process by the LLDB client.

`a startup` -> `at startup`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89477

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

MaskRay wrote:
> labath wrote:
> > mgorny wrote:
> > > labath wrote:
> > > > mgorny wrote:
> > > > > I would really feel better with a real error handling here. It 
> > > > > shouldn't be hard to use `ErrorOr` here.
> > > > Yeah, but what are you going to do with that value? Pass it to the 
> > > > caller? The existing callers are ignoring the error return anyway, and 
> > > > I don't want to add error handling everywhere as afaict, this function 
> > > > can't fail unless the user messes up the master state (which is not 
> > > > something I want to support).
> > > I get your point but I've literally wasted days because of missing error 
> > > handling, so I'd really preferred if we wouldn't make it even worse. 
> > > Though I guess `assert` is good enough.
> > In some ways it's even better because it will point you straight to the 
> > place where the assumption is violated, whereas a propagated logic error 
> > can manifest itself much farther away (or not at all). :)
> If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
> use-of-uninitialized-value error.
... in a -DLLVM_ENABLE_ASSERTIONS=off build.

This probably still needs some protection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > I would really feel better with a real error handling here. It 
> > > > shouldn't be hard to use `ErrorOr` here.
> > > Yeah, but what are you going to do with that value? Pass it to the 
> > > caller? The existing callers are ignoring the error return anyway, and I 
> > > don't want to add error handling everywhere as afaict, this function 
> > > can't fail unless the user messes up the master state (which is not 
> > > something I want to support).
> > I get your point but I've literally wasted days because of missing error 
> > handling, so I'd really preferred if we wouldn't make it even worse. Though 
> > I guess `assert` is good enough.
> In some ways it's even better because it will point you straight to the place 
> where the assumption is violated, whereas a propagated logic error can 
> manifest itself much farther away (or not at all). :)
If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
use-of-uninitialized-value error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D85968: [lldb] Forcefully complete a type when adding nested classes

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s:7
 
-# RUN: llvm-mc --triple x86_64-pc-linux %s --filetype=obj > %t
-# RUN: %lldb %t -o "expr a" -o exit 2>&1 | FileCheck %s --check-prefix=EXPR
-# RUN: %lldb %t -o "target var a" -o exit 2>&1 | FileCheck %s 
--check-prefix=VAR
-
-# EXPR: incomplete type 'A' where a complete type is required
+# RUN: rm -rf %t
+# RUN: split-file %s %t

In many cases `rm -rf %t` is not needed. `split-file` will unlink the output if 
it was originally a file.



Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_declaration-with-children.s:9
+# RUN: split-file %s %t
+# RUN: llvm-mc --triple x86_64-pc-linux %t/asm --filetype=obj > %t.o
+# RUN: %lldb -o "settings set interpreter.stop-command-source-on-error false" \

No need to change now: `-o` is recommended (I think the arguments are (1) for 
aesthetic value (2) when llvm-mc fails, don't leave an empty file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85968

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Target/TraceSettingsParser.cpp:123
+  if (schema.empty()) {
+std::ostringstream schema_builder;
+schema_builder << "{\n \"trace\": ";

This requires `sstream`. Fixed.



Comment at: lldb/source/Target/TraceSettingsParser.cpp:128
+std::string plugin_schema(GetPluginSchema());
+plugin_schema = std::regex_replace(plugin_schema, std::regex("\n"), "\n  
");
+schema_builder << plugin_schema << ",\n";

The replacement here looks strange.

`` implementations tend to be large, slow and buggy. It'd be best if 
 can be avoided. (llvm has llvm/Support/Regex.h)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D80755: Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I haven't been paying close attention to this patch, but allowing 20 bytes 
makes sense. In GNU ld and gold, `--build-id` is `--build-id=sha1` (20 bytes). 
The 3 linkers (plus LLD) don't have a way to produce a build ID longer than 20 
bytes.


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

https://reviews.llvm.org/D80755



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

[Lldb-commits] [PATCH] D83957: [lldb/DWARF] Don't get confused by line sequences with tombstone values

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1040
   std::unique_ptr sequence =
   LineTable::CreateLineSequenceContainer();
   std::vector> sequences;

While here, delete the initialization and move `sequence = 
LineTable::CreateLineSequenceContainer()` to the top of the loop body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83957



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

[Lldb-commits] [PATCH] D85169: [test] Exit with an error if no tests are run.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:1043
+if configuration.suite.countTestCases() == 0:
+print("error: did not discover any tests.")
+exitTestSuite(1)

JDevlieghere wrote:
> Maybe `did not discover any (matching) tests` to make it clear that this 
> might be the result of user input?
`sys.stderr.write` or `print(..., file=sys.stderr)` (which appends a newline)

Trailing full stops are not recommended for error messages if the project does 
not already have the convention 
http://llvm.org/docs/CodingStandards.html#error-and-warning-messages

Looking around, several messages do not have a full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85169

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

[Lldb-commits] [PATCH] D85248: [test] Support git commit version ids for clang.

[Fangrui Song via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85248

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

[Lldb-commits] [PATCH] D85100: [ELF] Allow sections after a non-SHF_ALLOC section to be covered by PT_LOAD

[Fangrui Song via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa6db64ef4a99: [ELF] Allow sections after a non-SHF_ALLOC 
section to be covered by PT_LOAD (authored by MaskRay).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D85100?vs=283277=283618#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85100

Files:
  lld/ELF/LinkerScript.cpp
  lld/ELF/Writer.cpp
  lld/test/ELF/linkerscript/compress-debug-sections-custom.s
  lld/test/ELF/linkerscript/sections.s
  lld/test/ELF/linkerscript/symbols-non-alloc.test
  lldb/test/Shell/SymbolFile/DWARF/Inputs/debug-line-basic.script

Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/debug-line-basic.script
===
--- lldb/test/Shell/SymbolFile/DWARF/Inputs/debug-line-basic.script
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/debug-line-basic.script
@@ -2,11 +2,11 @@
   text PT_LOAD;
 }
 SECTIONS {
-  .shstrtab : { *(.shstrtab) }
-  .debug_info   : { *(.debug_info  ) }
-  .debug_line   : { *(.debug_line  ) }
-  .debug_str: { *(.debug_str   ) }
-  .debug_abbrev : { *(.debug_abbrev) }
+  .shstrtab 0 : { *(.shstrtab) }
+  .debug_info   0 : { *(.debug_info  ) }
+  .debug_line   0 : { *(.debug_line  ) }
+  .debug_str0 : { *(.debug_str   ) }
+  .debug_abbrev 0 : { *(.debug_abbrev) }
 
   . = 0x201000;
   .text : { *(.text .text.f) } :text
Index: lld/test/ELF/linkerscript/symbols-non-alloc.test
===
--- lld/test/ELF/linkerscript/symbols-non-alloc.test
+++ lld/test/ELF/linkerscript/symbols-non-alloc.test
@@ -1,14 +1,17 @@
 # REQUIRES: x86
+## The address of a symbol assignment after a non-SHF_ALLOC section equals the
+## end address of the section.
+
 # RUN: echo '.section .nonalloc,""; .quad 0' \
 # RUN:   | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %t
 # RUN: ld.lld -o %t2 --script %s %t
 # RUN: llvm-objdump --section-headers -t %t2 | FileCheck %s
 
 # CHECK: Sections:
-# CHECK:  .nonalloc 0008 
+# CHECK:  .nonalloc 0008 0120
 
 # CHECK: SYMBOL TABLE:
-# CHECK:  0008 g .nonalloc  Sym
+# CHECK:  0128 g .nonalloc  Sym
 
 SECTIONS {
   . = SIZEOF_HEADERS;
Index: lld/test/ELF/linkerscript/sections.s
===
--- lld/test/ELF/linkerscript/sections.s
+++ lld/test/ELF/linkerscript/sections.s
@@ -25,8 +25,10 @@
 # SEC-DEFAULT: 7 .shstrtab 003b {{[0-9a-f]*}}
 # SEC-DEFAULT: 8 .strtab   0008 {{[0-9a-f]*}}
 
-# Sections are put in order specified in linker script, other than alloc
-# sections going first.
+## Sections are placed in the order specified by the linker script. .data has
+## a PT_LOAD segment, even if it is preceded by a non-alloc section. To
+## allow this, place non-alloc orphan sections at the end and advance
+## location counters for non-alloc non-orphan sections.
 # RUN: echo "SECTIONS { \
 # RUN:  .bss : { *(.bss) } \
 # RUN:  other : { *(other) } \
@@ -34,20 +36,27 @@
 # RUN:  .symtab : { *(.symtab) } \
 # RUN:  .strtab : { *(.strtab) } \
 # RUN:  .data : { *(.data) } \
-# RUN:  .text : { *(.text) } }" > %t.script
-# RUN: ld.lld -o %t3 --script %t.script %t
-# RUN: llvm-objdump --section-headers %t3 | \
-# RUN:   FileCheck -check-prefix=SEC-ORDER %s
+# RUN:  .text : { *(.text) } }" > %t3.lds
+# RUN: ld.lld -o %t3a -T %t3.lds %t
+# RUN: llvm-readelf -S -l %t3a | FileCheck --check-prefix=SEC-ORDER %s
+# RUN: ld.lld -o %t3b -T %t3.lds --unique %t
+# RUN: llvm-readelf -S -l %t3b | FileCheck --check-prefix=SEC-ORDER %s
 
-#   Idx Name  Size
-# SEC-ORDER: 1 .bss  0002 {{[0-9a-f]*}} BSS
-# SEC-ORDER: 2 other 0003 {{[0-9a-f]*}} DATA
-# SEC-ORDER: 3 .shstrtab 003b {{[0-9a-f]*}}
-# SEC-ORDER: 4 .symtab   0030 {{[0-9a-f]*}}
-# SEC-ORDER: 5 .strtab   0008 {{[0-9a-f]*}}
-# SEC-ORDER: 6 .comment  0008 {{[0-9a-f]*}}
-# SEC-ORDER: 7 .data 0020 {{[0-9a-f]*}} DATA
-# SEC-ORDER: 8 .text 000e {{[0-9a-f]*}} TEXT
+# SEC-ORDER:   [Nr] Name  Type Address  OffSize   ES Flg
+# SEC-ORDER:   [ 0]   NULL  00 00 00
+# SEC-ORDER-NEXT:  [ 1] .bss  NOBITS    001000 02 00  WA
+# SEC-ORDER-NEXT:  [ 2] other PROGBITS 0002 001002 03 00  WA
+# SEC-ORDER-NEXT:  [ 3] .shstrtab STRTAB   0005 001005 3b 00
+# SEC-ORDER-NEXT:  [ 4] .symtab   SYMTAB   0040 001040 30 18
+# SEC-ORDER-NEXT:  [ 5] .strtab   STRTAB   

[Lldb-commits] [PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

[Fangrui Song via Phabricator via lldb-commits]

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

Looks like a good cleanup. Hope someone with more CMake-fu to confirm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454



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

[Lldb-commits] [PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Hi, your git commit contains extra Phabricator tags. You can drop `Reviewers:` 
`Subscribers:` `Tags:` and the text `Summary:` from the git commit with the 
following script:

  arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: 
/,"");print}' | git commit --amend --date=now -F -
  }

`Reviewed By: ` is considered important by some people. Please keep the tag. 
(`--date=now` is my personal preference (author dates are usually not useful. 
Using committer dates can make log almost monotonic in time))

`llvm/utils/git/pre-push.py` can validate the message does not include unneeded 
tags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[Lldb-commits] [PATCH] D79147: Switch to using -debug-info-kind=constructor as default (from =limited)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Hi, your git commit contains extra Phabricator tags. You can drop `Reviewers:` 
`Subscribers:` `Tags:` and the text `Summary:` from the git commit with the 
following script:

  arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: 
/,"");print}' | git commit --amend --date=now -F -
  }

`Reviewed By: ` is considered important by some people. Please keep the tag. 
(`--date=now` is my personal preference (author dates are usually not useful. 
Using committer dates can make log almost monotonic in time))

`llvm/utils/git/pre-push.py` can validate the message does not include unneeded 
tags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79147



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

[Lldb-commits] [PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a subscriber: mehdi_amini.
MaskRay added a comment.

In D82477#2145967 , @clayborg wrote:

> In D82477#2145565 , @MaskRay wrote:
>
> > Hi, your git commit contains extra Phabricator tags. You can drop 
> > `Reviewers:` `Subscribers:` `Tags:` and the text `Summary:` from the git 
> > commit with the following script:
> >
> >   arcfilter () {
> >   arc amend
> >   git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
> > /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ 
> > {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
> >   }
> >   
> >
> > `Reviewed By: ` is considered important by some people. Please keep the 
> > tag. (`--date=now` is my personal preference (author dates are usually not 
> > useful. Using committer dates can make log almost monotonic in time))
> >
> > `llvm/utils/git/pre-push.py` can validate the message does not include 
> > unneeded tags.
>
>
> Can we modify this script to remove the unneeded tags instead of detecting it?


@mehdi_amini ^^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[Lldb-commits] [PATCH] D81200: [vscode] set default values for terminateDebuggee for the disconnect request

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Hi, your git commit contains extra Phabricator tags. You can drop `Reviewers:` 
`Subscribers:` `Tags:` and the text `Summary:` from the git commit with the 
following script:

  arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: 
/,"");print}' | git commit --amend --date=now -F -
  }

`Reviewed By: ` is considered important by some people. Please keep the tag. (I 
have updated my script to use `--date=now` (setting author date to committer 
date))

`https://reviews.llvm.org/D80978` contains a git pre-push hook to automate this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81200



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

[Lldb-commits] [PATCH] D82238: lldb whitelist -> allowed

[Fangrui Song via Phabricator via lldb-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82238



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

[Lldb-commits] [PATCH] D68541: Implement 'up' and 'down' shortcuts in lldb gui

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

What is the state of the patch? Does lldb support cgdb-style u/d/f/b etc now?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68541



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

[Lldb-commits] [PATCH] D84401: [lldb] Add SectionList::ContainsFileAddressRange

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Core/Section.cpp:570
+  while (file_addr < end) {
+SectionSP sect_sp = FindSectionContainingFileAddress(file_addr);
+if (!sect_sp)

I am a bit concerned about the potential large time complexity 
here.FindSectionContainingFileAddress is O(|sections|) and this loop usually 
executes once but can take a lot of iterations in a pessimistic case. See my 
comment in D84402.



Comment at: lldb/unittests/Core/SectionTest.cpp:43
+Flags:   [ SHF_ALLOC ]
+Address: 0x0031
+Size:0x000f

Might be useful leaving a comment that there is a gap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84401



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

[Lldb-commits] [PATCH] D84402: [lldb/DWARF] Add more line table validation

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

For ELF, there are non-pic cases (i.e. -no-pie) and pic cases (-pie or 
-shared). I think it is sufficient just testing -pie (image base is zero). If 
filtering for -pie works, filter for -no-pie or -shared should work as well.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1044
   for (const llvm::DWARFDebugLine::Sequence  : line_table->Sequences) {
+if (!list.ContainsFileAddressRange(seq.LowPC, seq.HighPC - seq.LowPC))
+  continue;

clayborg wrote:
> labath wrote:
> > clayborg wrote:
> > > dblaikie wrote:
> > > > Could you specifically look for/propagate tombstones here, to reduce 
> > > > the risk of large functions overlapping with the valid address range, 
> > > > even when they're tombstoned to zero? (because zero+large function size 
> > > > could still end up overlapping with valid code)
> > > > 
> > > > To do that well, I guess it'd have to be implemented at a lower-layer, 
> > > > inside the line table state machine - essentially dropping all lines 
> > > > derived from a "set address" operation that specifies a tombstone.
> > > Just checking if the section lists contains an address doesn't help weed 
> > > out addresses that were tombstoned to zero since our PT_LOAD[0] will 
> > > almost always contain zero for shared libraries. It might be nice to make 
> > > a list of addresses that come from sections with read + execute 
> > > permissions and store that in SymbolFileDWARF one time at startup. Then 
> > > these searches will be faster as we are looking in less ranges, and most 
> > > likely will not contain address zero. This code will catch the -1 and -2 
> > > tombstones, but most linkers I have run into use zero and the tombstone. 
> > > 
> > > If our algorithm only checks sections with no subsections and then makes 
> > > a list of file addresses for and section ranges for those, we should have 
> > > a great list. The entire PT_LOAD[0] will usually be mapped read + 
> > > execute, so we can't just check top level sections for ELF. Mach-o also 
> > > has this issue __TEXT in mac is also mapped read + execute and usually 
> > > contains zero for shared libraries, but since the sections must come 
> > > after the mach-o header, the sections within the __TEXT segment have 
> > > correct permissions and would work, just like they would for ELF.
> > You're right -- this would not handle shared libraries with base zero.
> > 
> > I am slightly uneasy about requiring executable permissions for all line 
> > tables. While it does not seem terribly useful to have line tables for 
> > non-executable code, if someone does have a line table for it for whatever 
> > reason (maybe he wants to make it executable at runtime?) it would be a 
> > shame not to display it. Also the choice of using section rather than 
> > segment permissions feels slightly arbitrary (although I could make a case 
> > for it), as it's the segment permissions which will actually define the 
> > runtime memory permissions.
> > 
> > Since this is really about filtering out (near) zero addresses, how about 
> > we make that explicit? Find the lowest executable (section) address and 
> > reject anything below that? Additionally, I'd also reject all addresses 
> > which are completely outside of the module range, as those not going to get 
> > used for anything, and they are generating bogus line-table dumps.
> > 
> > What do you think?
> > 
> > David: The -1 tombstones are already sort of handled in llvm (and in lldb 
> > since D83957). They are "handled" in the sense that the all sequences with 
> > and end PC lower than the start PC are rejected (and line sequences 
> > starting with (unsigned)-1 will definitely wrap). This is trying to do 
> > something about the zero tombstones.
> > Since this is really about filtering out (near) zero addresses, how about 
> > we make that explicit? Find the lowest executable (section) address and 
> > reject anything below that? Additionally, I'd also reject all addresses 
> > which are completely outside of the module range, as those not going to get 
> > used for anything, and they are generating bogus line-table dumps.
> > 
> > What do you think?
> 
> That will work for me. My main goal is to get anything that should have been 
> dead stripped out from appearing in results for line lookups or function 
> lookups. The quicker we can short circuit these cases the better for 
> performance. We can also use this when we try to lookup functions and don't 
> return any matches for functions whose start address falls below this value.
> 
> 
There is a concern about ContainsFileAddressRange's performance.

FindSectionContainingFileAddress iterates all sections and can be slow when the 
number of sections is large.

>  if someone does have a line table for it for whatever reason (maybe he wants 
> to make it executable at runtime?) it would be a shame not to display it.

+1

Instead of a [lowpc,highpc) range 

[Lldb-commits] [PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG53880b8cb9c6: [CMake] Make `intrinsics_gen` dependency 
unconditional. (authored by michele.scandale, committed by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454

Files:
  clang/CMakeLists.txt
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/Frontend/CMakeLists.txt
  clang/tools/clang-fuzzer/handle-llvm/CMakeLists.txt
  clang/tools/clang-import-test/CMakeLists.txt
  clang/tools/clang-offload-bundler/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/driver/CMakeLists.txt
  lld/COFF/CMakeLists.txt
  lld/Common/CMakeLists.txt
  lld/ELF/CMakeLists.txt
  lld/MinGW/CMakeLists.txt
  lld/lib/Core/CMakeLists.txt
  lld/wasm/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/source/Expression/CMakeLists.txt
  lldb/source/Plugins/ExpressionParser/Clang/CMakeLists.txt
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt

Index: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
===
--- lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
+++ lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/CMakeLists.txt
@@ -1,8 +1,3 @@
-if(NOT LLDB_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
-
 add_lldb_library(lldbPluginRenderScriptRuntime PLUGIN
   RenderScriptRuntime.cpp
   RenderScriptExpressionOpts.cpp
@@ -10,7 +5,7 @@
   RenderScriptScriptGroup.cpp
 
   DEPENDS
-  ${tablegen_deps}
+  intrinsics_gen
 
   LINK_LIBS
 lldbBreakpoint
Index: lldb/source/Plugins/ExpressionParser/Clang/CMakeLists.txt
===
--- lldb/source/Plugins/ExpressionParser/Clang/CMakeLists.txt
+++ lldb/source/Plugins/ExpressionParser/Clang/CMakeLists.txt
@@ -1,7 +1,3 @@
-if(NOT LLDB_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
 add_lldb_library(lldbPluginExpressionParserClang
   ASTResultSynthesizer.cpp
   ASTStructExtractor.cpp
@@ -29,7 +25,7 @@
   NameSearchContext.cpp
 
   DEPENDS
-  ${tablegen_deps}
+  intrinsics_gen
 
   LINK_LIBS
 lldbCore
Index: lldb/source/Expression/CMakeLists.txt
===
--- lldb/source/Expression/CMakeLists.txt
+++ lldb/source/Expression/CMakeLists.txt
@@ -1,7 +1,3 @@
-if(NOT LLDB_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
 add_lldb_library(lldbExpression
   DiagnosticManager.cpp
   DWARFExpression.cpp
@@ -18,7 +14,7 @@
   UtilityFunction.cpp
 
   DEPENDS
-  ${tablegen_deps}
+  intrinsics_gen
 
   LINK_LIBS
 lldbCore
Index: lldb/CMakeLists.txt
===
--- lldb/CMakeLists.txt
+++ lldb/CMakeLists.txt
@@ -64,7 +64,7 @@
 # some of these generated headers. This approach is copied from Clang's main
 # CMakeLists.txt, so it should kept in sync the code in Clang which was added
 # in llvm-svn 308844.
-if(LLVM_ENABLE_MODULES AND NOT LLDB_BUILT_STANDALONE)
+if(LLVM_ENABLE_MODULES)
   list(APPEND LLVM_COMMON_DEPENDS intrinsics_gen)
 endif()
 
Index: lld/wasm/CMakeLists.txt
===
--- lld/wasm/CMakeLists.txt
+++ lld/wasm/CMakeLists.txt
@@ -2,10 +2,6 @@
 tablegen(LLVM Options.inc -gen-opt-parser-defs)
 add_public_tablegen_target(WasmOptionsTableGen)
 
-if(NOT LLD_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
 add_lld_library(lldWasm
   Driver.cpp
   InputChunks.cpp
@@ -37,5 +33,5 @@
 
   DEPENDS
   WasmOptionsTableGen
-  ${tablegen_deps}
+  intrinsics_gen
   )
Index: lld/lib/Core/CMakeLists.txt
===
--- lld/lib/Core/CMakeLists.txt
+++ lld/lib/Core/CMakeLists.txt
@@ -1,7 +1,3 @@
-if(NOT LLD_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
 add_lld_library(lldCore
   DefinedAtom.cpp
   Error.cpp
@@ -24,5 +20,5 @@
   ${LLVM_PTHREAD_LIB}
 
   DEPENDS
-  ${tablegen_deps}
+  intrinsics_gen
   )
Index: lld/MinGW/CMakeLists.txt
===
--- lld/MinGW/CMakeLists.txt
+++ lld/MinGW/CMakeLists.txt
@@ -2,10 +2,6 @@
 tablegen(LLVM Options.inc -gen-opt-parser-defs)
 add_public_tablegen_target(MinGWOptionsTableGen)
 
-if(NOT LLD_BUILT_STANDALONE)
-  set(tablegen_deps intrinsics_gen)
-endif()
-
 add_lld_library(lldMinGW
   Driver.cpp
 
@@ -19,5 +15,5 @@
 
   DEPENDS
   MinGWOptionsTableGen
-  ${tablegen_deps}
+  intrinsics_gen
 )
Index: lld/ELF/CMakeLists.txt
===
--- lld/ELF/CMakeLists.txt
+++ lld/ELF/CMakeLists.txt
@@ -2,10 +2,6 @@
 tablegen(LLVM Options.inc -gen-opt-parser-defs)
 add_public_tablegen_target(ELFOptionsTableGen)
 
-if(NOT 

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The number of changed tests is large. Is it worth moving the 
`IO.mapOptional("Length", Unit.Length);` change to a separate patch to make the 
refactoring more focused? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



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

[Lldb-commits] [PATCH] D83957: [lldb/DWARF] Don't get confused by line sequences with tombstone values

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

> Recently, lld has started debug info resolving relocations to 
> garbage-collected symbols as -1

Just mention D81784 :)

> (instead of 0).

This is not accurate. It resolves the relocation to `r_addend`. In many cases, 
r_addend is 0. However, for the following, a non-zero addend is possible.

  __attribute__((section(".text.x"))) void f1() { }
  __attribute__((section(".text.x"))) void f2() { } // DW_AT_low_pc has a 
non-zero addend


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83957



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

[Lldb-commits] [PATCH] D84623: Remove HAVE_VCS_VERSION_INC, not needed

[Fangrui Song 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 rG9bb9b737c557: Remove HAVE_VCS_VERSION_INC, not needed 
(authored by hlopko, committed by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84623

Files:
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Version.cpp
  lld/Common/CMakeLists.txt
  lld/Common/Version.cpp
  lldb/source/CMakeLists.txt
  lldb/source/lldb.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
  llvm/utils/gn/secondary/lld/Common/BUILD.gn

Index: llvm/utils/gn/secondary/lld/Common/BUILD.gn
===
--- llvm/utils/gn/secondary/lld/Common/BUILD.gn
+++ llvm/utils/gn/secondary/lld/Common/BUILD.gn
@@ -42,5 +42,4 @@
 "Timer.cpp",
 "Version.cpp",
   ]
-  defines = [ "HAVE_VCS_VERSION_INC" ]  # For Version.cpp
 }
Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -103,5 +103,4 @@
 "XRayInstr.cpp",
 "XRayLists.cpp",
   ]
-  defines = [ "HAVE_VCS_VERSION_INC" ]  # For Version.cpp
 }
Index: lldb/source/lldb.cpp
===
--- lldb/source/lldb.cpp
+++ lldb/source/lldb.cpp
@@ -13,9 +13,7 @@
 
 #include "clang/Basic/Version.h"
 
-#ifdef HAVE_VCS_VERSION_INC
 #include "VCSVersion.inc"
-#endif
 
 static const char *GetLLDBRevision() {
 #ifdef LLDB_REVISION
Index: lldb/source/CMakeLists.txt
===
--- lldb/source/CMakeLists.txt
+++ lldb/source/CMakeLists.txt
@@ -26,9 +26,6 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-set_property(SOURCE lldb.cpp APPEND PROPERTY
- COMPILE_DEFINITIONS "HAVE_VCS_VERSION_INC")
-
 list(APPEND lldbBase_SOURCES ${version_inc})
 
 if(LLDB_VERSION_STRING)
Index: lld/Common/Version.cpp
===
--- lld/Common/Version.cpp
+++ lld/Common/Version.cpp
@@ -12,9 +12,7 @@
 
 #include "lld/Common/Version.h"
 
-#ifdef HAVE_VCS_VERSION_INC
 #include "VCSVersion.inc"
-#endif
 
 // Returns a version string, e.g.:
 // lld 9.0.0 (https://github.com/llvm/llvm-project.git 9efdd7ac5e914d3c9fa1ef)
Index: lld/Common/CMakeLists.txt
===
--- lld/Common/CMakeLists.txt
+++ lld/Common/CMakeLists.txt
@@ -26,9 +26,6 @@
   PROPERTIES GENERATED TRUE
   HEADER_FILE_ONLY TRUE)
 
-set_property(SOURCE Version.cpp APPEND PROPERTY
-  COMPILE_DEFINITIONS "HAVE_VCS_VERSION_INC")
-
 add_lld_library(lldCommon
   Args.cpp
   DWARF.cpp
Index: clang/lib/Basic/Version.cpp
===
--- clang/lib/Basic/Version.cpp
+++ clang/lib/Basic/Version.cpp
@@ -17,9 +17,7 @@
 #include 
 #include 
 
-#ifdef HAVE_VCS_VERSION_INC
 #include "VCSVersion.inc"
-#endif
 
 namespace clang {
 
Index: clang/lib/Basic/CMakeLists.txt
===
--- clang/lib/Basic/CMakeLists.txt
+++ clang/lib/Basic/CMakeLists.txt
@@ -33,9 +33,6 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-set_property(SOURCE Version.cpp APPEND PROPERTY
- COMPILE_DEFINITIONS "HAVE_VCS_VERSION_INC")
-
 add_clang_library(clangBasic
   Attributes.cpp
   Builtins.cpp
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84623: Remove HAVE_VCS_VERSION_INC, not needed

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.
Herald added a subscriber: dexonsmith.

Thanks! Assuming you don't have a commit bit, I'll commit on your behalf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84623

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

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Is this good? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

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

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive rule to the only test

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 317985.
MaskRay retitled this revision from "Makefile.rules: Avoid redundant .d 
generation and make restart" to "Makefile.rules: Avoid redundant .d generation 
(make restart) and inline archive rule to the only test".
MaskRay edited the summary of this revision.
MaskRay added a comment.

Use $(AR)

Remove more ARCHIVE_NAME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/functionalities/archives/Makefile

Index: lldb/test/API/functionalities/archives/Makefile
===
--- lldb/test/API/functionalities/archives/Makefile
+++ lldb/test/API/functionalities/archives/Makefile
@@ -1,7 +1,13 @@
-C_SOURCES := main.c
-
+C_SOURCES := main.c a.c b.c
+EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
-ARCHIVE_NAME := libfoo.a
-ARCHIVE_C_SOURCES := a.c b.c
+
+all: a.out
+
+a.out: main.o libfoo.a
+	$(LD) $(LDFLAGS) $^ -o $@
+
+libfoo.a: a.o b.o
+	$(AR) $(ARFLAGS) $@ $^
 
 include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -26,15 +26,13 @@
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
 # USE_PRIVATE_MODULE_CACHE := YES
-#
-# And test/functionalities/archives/Makefile:
-# MAKE_DSYM := NO
-# ARCHIVE_NAME := libfoo.a
-# ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
+# Suppress built-in suffix rules. We explicitly define rules for %.o.
+.SUFFIXES:
+
 SRCDIR := $(shell dirname $(firstword $(MAKEFILE_LIST)))
 BUILDDIR := $(shell pwd)
 MAKEFILE_RULES := $(lastword $(MAKEFILE_LIST))
@@ -477,42 +475,6 @@
 	endif
 endif
 
-#--
-# Check if we have any C source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_C_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_C_SOURCES:.c=.o))
-endif
-
-#--
-# Check if we have any C++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_CXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_CXX_SOURCES:.cpp=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-endif
-
-#--
-# Check if we have any ObjC source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJC_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJC_SOURCES:.m=.o))
-	LDFLAGS +=-lobjc
-endif
-
-#--
-# Check if we have any ObjC++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJCXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJCXX_SOURCES:.mm=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-	ifeq "$(findstring lobjc,$(LDFLAGS))" ""
-		LDFLAGS +=-lobjc
-	endif
-endif
-
 ifeq ($(findstring clang, $(CXX)), clang)
 	CXXFLAGS += --driver-mode=g++
 endif
@@ -534,8 +496,8 @@
 #--
 ifneq "$(DYLIB_NAME)" ""
 ifeq "$(DYLIB_ONLY)" ""
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
-	$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
+$(EXE) : $(OBJECTS) $(DYLIB_FILENAME)
+	$(LD) $(OBJECTS) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -543,8 +505,8 @@
 EXE = $(DYLIB_FILENAME)
 endif
 else
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
-	$(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+$(EXE) : $(OBJECTS)
+	$(LD) $(OBJECTS) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -566,19 +528,6 @@
 endif
 endif
 
-#--
-# Make the archive
-#--
-ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-	$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-	$(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
-endif
-
 #--
 # Make the dylib
 #--
@@ -628,12 +577,22 @@
 # Make the precompiled header and compile C++ sources against it
 

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 317983.
MaskRay added a comment.

Inline archive rule


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/functionalities/archives/Makefile

Index: lldb/test/API/functionalities/archives/Makefile
===
--- lldb/test/API/functionalities/archives/Makefile
+++ lldb/test/API/functionalities/archives/Makefile
@@ -1,7 +1,13 @@
-C_SOURCES := main.c
-
+C_SOURCES := main.c a.c b.c
+EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
-ARCHIVE_NAME := libfoo.a
-ARCHIVE_C_SOURCES := a.c b.c
+
+all: a.out
+
+a.out: main.o libfoo.a
+	$(LD) $(LDFLAGS) $^ -o $@
+
+libfoo.a: a.o b.o
+	ar $(ARFLAGS) $@ $^
 
 include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -26,15 +26,13 @@
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
 # USE_PRIVATE_MODULE_CACHE := YES
-#
-# And test/functionalities/archives/Makefile:
-# MAKE_DSYM := NO
-# ARCHIVE_NAME := libfoo.a
-# ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
+# Suppress built-in suffix rules. We explicitly define rules for %.o.
+.SUFFIXES:
+
 SRCDIR := $(shell dirname $(firstword $(MAKEFILE_LIST)))
 BUILDDIR := $(shell pwd)
 MAKEFILE_RULES := $(lastword $(MAKEFILE_LIST))
@@ -477,42 +475,6 @@
 	endif
 endif
 
-#--
-# Check if we have any C source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_C_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_C_SOURCES:.c=.o))
-endif
-
-#--
-# Check if we have any C++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_CXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_CXX_SOURCES:.cpp=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-endif
-
-#--
-# Check if we have any ObjC source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJC_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJC_SOURCES:.m=.o))
-	LDFLAGS +=-lobjc
-endif
-
-#--
-# Check if we have any ObjC++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJCXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJCXX_SOURCES:.mm=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-	ifeq "$(findstring lobjc,$(LDFLAGS))" ""
-		LDFLAGS +=-lobjc
-	endif
-endif
-
 ifeq ($(findstring clang, $(CXX)), clang)
 	CXXFLAGS += --driver-mode=g++
 endif
@@ -534,7 +496,7 @@
 #--
 ifneq "$(DYLIB_NAME)" ""
 ifeq "$(DYLIB_ONLY)" ""
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
+$(EXE) : $(OBJECTS) $(DYLIB_FILENAME)
 	$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
@@ -566,19 +528,6 @@
 endif
 endif
 
-#--
-# Make the archive
-#--
-ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-	$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-	$(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
-endif
-
 #--
 # Make the dylib
 #--
@@ -628,12 +577,22 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
 	$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-	$(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
-#endif
+endif
+
+%.o: %.c %.d
+	$(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.cpp %.d $(PCH_OUTPUT)
+	$(CXX) $(PCHFLAGS) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.m %.d
+	$(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.mm %.d
+	$(CXX) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
 
 

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D94890#2510627 , @JDevlieghere 
wrote:

> In D94890#2510446 , @MaskRay wrote:
>
>> Is this good? :)
>
> I think you forgot to update the patch?

Ah, looks like you want to do inline (`ARCHIVE_OBJECTS`) into the test in this 
patch... Done (it is a bit tricky).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

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

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive rule to the only test

[Fangrui Song 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 rG6afdf13ae4cc: Makefile.rules: Avoid redundant .d generation 
(make restart) and inline archive… (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/functionalities/archives/Makefile

Index: lldb/test/API/functionalities/archives/Makefile
===
--- lldb/test/API/functionalities/archives/Makefile
+++ lldb/test/API/functionalities/archives/Makefile
@@ -1,7 +1,14 @@
-C_SOURCES := main.c
-
+C_SOURCES := main.c a.c b.c
+EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
-ARCHIVE_NAME := libfoo.a
-ARCHIVE_C_SOURCES := a.c b.c
+
+all: a.out
+
+a.out: main.o libfoo.a
+	$(LD) $(LDFLAGS) $^ -o $@
+
+libfoo.a: a.o b.o
+	$(AR) $(ARFLAGS) $@ $^
+	$(RM) $^
 
 include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -26,15 +26,13 @@
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
 # USE_PRIVATE_MODULE_CACHE := YES
-#
-# And test/functionalities/archives/Makefile:
-# MAKE_DSYM := NO
-# ARCHIVE_NAME := libfoo.a
-# ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
+# Suppress built-in suffix rules. We explicitly define rules for %.o.
+.SUFFIXES:
+
 SRCDIR := $(shell dirname $(firstword $(MAKEFILE_LIST)))
 BUILDDIR := $(shell pwd)
 MAKEFILE_RULES := $(lastword $(MAKEFILE_LIST))
@@ -477,42 +475,6 @@
 	endif
 endif
 
-#--
-# Check if we have any C source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_C_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_C_SOURCES:.c=.o))
-endif
-
-#--
-# Check if we have any C++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_CXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_CXX_SOURCES:.cpp=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-endif
-
-#--
-# Check if we have any ObjC source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJC_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJC_SOURCES:.m=.o))
-	LDFLAGS +=-lobjc
-endif
-
-#--
-# Check if we have any ObjC++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJCXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJCXX_SOURCES:.mm=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-	ifeq "$(findstring lobjc,$(LDFLAGS))" ""
-		LDFLAGS +=-lobjc
-	endif
-endif
-
 ifeq ($(findstring clang, $(CXX)), clang)
 	CXXFLAGS += --driver-mode=g++
 endif
@@ -534,8 +496,8 @@
 #--
 ifneq "$(DYLIB_NAME)" ""
 ifeq "$(DYLIB_ONLY)" ""
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
-	$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
+$(EXE) : $(OBJECTS) $(DYLIB_FILENAME)
+	$(LD) $(OBJECTS) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -543,8 +505,8 @@
 EXE = $(DYLIB_FILENAME)
 endif
 else
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
-	$(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+$(EXE) : $(OBJECTS)
+	$(LD) $(OBJECTS) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -566,19 +528,6 @@
 endif
 endif
 
-#--
-# Make the archive
-#--
-ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-	$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-	$(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
-endif
-
 #--
 # Make the dylib
 #--
@@ -628,12 +577,22 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive rule to the only test

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 318011.
MaskRay marked an inline comment as done.
MaskRay added a comment.

$(RM) a.o b.o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/test/API/functionalities/archives/Makefile

Index: lldb/test/API/functionalities/archives/Makefile
===
--- lldb/test/API/functionalities/archives/Makefile
+++ lldb/test/API/functionalities/archives/Makefile
@@ -1,7 +1,14 @@
-C_SOURCES := main.c
-
+C_SOURCES := main.c a.c b.c
+EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
-ARCHIVE_NAME := libfoo.a
-ARCHIVE_C_SOURCES := a.c b.c
+
+all: a.out
+
+a.out: main.o libfoo.a
+	$(LD) $(LDFLAGS) $^ -o $@
+
+libfoo.a: a.o b.o
+	$(AR) $(ARFLAGS) $@ $^
+	$(RM) $^
 
 include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -26,15 +26,13 @@
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
 # USE_PRIVATE_MODULE_CACHE := YES
-#
-# And test/functionalities/archives/Makefile:
-# MAKE_DSYM := NO
-# ARCHIVE_NAME := libfoo.a
-# ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
+# Suppress built-in suffix rules. We explicitly define rules for %.o.
+.SUFFIXES:
+
 SRCDIR := $(shell dirname $(firstword $(MAKEFILE_LIST)))
 BUILDDIR := $(shell pwd)
 MAKEFILE_RULES := $(lastword $(MAKEFILE_LIST))
@@ -477,42 +475,6 @@
 	endif
 endif
 
-#--
-# Check if we have any C source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_C_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_C_SOURCES:.c=.o))
-endif
-
-#--
-# Check if we have any C++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_CXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_CXX_SOURCES:.cpp=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-endif
-
-#--
-# Check if we have any ObjC source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJC_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJC_SOURCES:.m=.o))
-	LDFLAGS +=-lobjc
-endif
-
-#--
-# Check if we have any ObjC++ source files for archive
-#--
-ifneq "$(strip $(ARCHIVE_OBJCXX_SOURCES))" ""
-	ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJCXX_SOURCES:.mm=.o))
-	CXX = $(call cxx_compiler,$(CC))
-	LD = $(call cxx_linker,$(CC))
-	ifeq "$(findstring lobjc,$(LDFLAGS))" ""
-		LDFLAGS +=-lobjc
-	endif
-endif
-
 ifeq ($(findstring clang, $(CXX)), clang)
 	CXXFLAGS += --driver-mode=g++
 endif
@@ -534,8 +496,8 @@
 #--
 ifneq "$(DYLIB_NAME)" ""
 ifeq "$(DYLIB_ONLY)" ""
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
-	$(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
+$(EXE) : $(OBJECTS) $(DYLIB_FILENAME)
+	$(LD) $(OBJECTS) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -543,8 +505,8 @@
 EXE = $(DYLIB_FILENAME)
 endif
 else
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
-	$(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+$(EXE) : $(OBJECTS)
+	$(LD) $(OBJECTS) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
 	$(CODESIGN) -s - "$(EXE)"
 endif
@@ -566,19 +528,6 @@
 endif
 endif
 
-#--
-# Make the archive
-#--
-ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-	$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-	$(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
-endif
-
 #--
 # Make the dylib
 #--
@@ -628,12 +577,22 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
 	$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-	$(CXX) 

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: dblaikie, labath, mgorny, rupprecht.
Herald added a subscriber: krytarowski.
MaskRay requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

On my Debian machine, system libc++/libc++abi is not installed (`libc++1-9 
libc++abi-9`),
21 check-lldb-api tests fail because -stdlib=libc++ linked executables cannot
find runtime libc++.so.1 at runtime.

Use the `-Wl,-rpath,$(LLVM_LIBS_DIR)` mechanism in
`packages/Python/lldbsuite/test/make/Makefile.rules` (D58630 
 for NetBSD) to
allow such tests compile/link with fresh libc++ built beside lldb.
(A system libc++.so.1 is not guaranteed to match fresh libc++ header files.)

Some tweaks to the existing NetBSD rule when generalizing:

- Drop `-L$(LLVM_LIBS_DIR)` since Clang driver adds it correctly.
- Add `-stdlib=libc++` only for `USE_LIBCPP`.

Also, drop `-isystem /usr/include/c++/v1` introduced in D9426 
. It is not needed
by Clang driver. GCC using libc++ requires more setup.

I don't find any test needing `-Wl,-rpath` in 
`test/Shell/helper/{build,toolchain}.py` (D58630 
 for NetBSD added them).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94888

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -281,11 +281,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-   ifeq ($(OS),NetBSD)
-   LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-   endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
ifneq (,$(filter YES,$(ENABLE_THREADS)))
LDFLAGS += -pthread
@@ -395,21 +390,18 @@
 
 ifeq (1,$(USE_LIBCPP))
CXXFLAGS += -DLLDB_USING_LIBCPP
-   ifeq "$(OS)" "Linux"
-   ifneq (,$(findstring clang,$(CC)))
-   CXXFLAGS += -stdlib=libc++
-   LDFLAGS += -stdlib=libc++
-   else
-   CXXFLAGS += -isystem /usr/include/c++/v1
-   LDFLAGS += -lc++
-   endif
-   else ifeq "$(OS)" "Android"
+   ifeq "$(OS)" "Android"
# Nothing to do, this is already handled in
# Android.rules.
else
CXXFLAGS += -stdlib=libc++
LDFLAGS += -stdlib=libc++
endif
+   ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+   ifneq (,$(LLVM_LIBS_DIR))
+   LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+   endif
+   endif
 endif
 
 #--


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -281,11 +281,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-	ifeq ($(OS),NetBSD)
-		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-	endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
 	ifneq (,$(filter YES,$(ENABLE_THREADS)))
 		LDFLAGS += -pthread
@@ -395,21 +390,18 @@
 
 ifeq (1,$(USE_LIBCPP))
 	CXXFLAGS += -DLLDB_USING_LIBCPP
-	ifeq "$(OS)" "Linux"
-		ifneq (,$(findstring clang,$(CC)))
-			CXXFLAGS += -stdlib=libc++
-			LDFLAGS += -stdlib=libc++
-		else
-			CXXFLAGS += -isystem /usr/include/c++/v1
-			LDFLAGS += -lc++
-		endif
-	else ifeq "$(OS)" "Android"
+	ifeq "$(OS)" "Android"
 		# Nothing to do, this is already handled in
 		# Android.rules.
 	else
 		CXXFLAGS += -stdlib=libc++
 		LDFLAGS += -stdlib=libc++
 	endif
+	ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+		ifneq (,$(LLVM_LIBS_DIR))
+			LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+		endif
+	endif
 endif
 
 #--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

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

Take an example when `CXX_SOURCES` is main.cpp.

main.d is a included file. make will rebuild main.d, re-executes itself [1] to 
read
in the new main.d file, then rebuild main.o, finally link main.o into a.out.
main.cpp is parsed twice in this process.

This patch merges .d generation into .o generation [2], writes explicit rules
for .c/.m and deletes suffix rules for %.m and %.o. Since a target can be
satisfied by either of .c/.cpp/.m/.mm, we use double-colon rules [3].

Since suffix rules are disabled, the implicit rule for archive member targets is
no long available [4]. Rewrite and simplify the archive rule.

[1]: https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html
[2]: http://make.mad-scientist.net/papers/advanced-auto-dependency-generation/
[3]: https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html
[4]: https://www.gnu.org/software/make/manual/html_node/Archive-Update.html

ObjC/ObjCXX tests only run on macOS. I don't have testing environment.  Hope
someone can do it for me.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -570,13 +570,8 @@
 # Make the archive
 #--
 ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-   $(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-   $(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach 
ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
+$(ARCHIVE_NAME): $(ARCHIVE_OBJECTS)
+   $(AR) $(ARFLAGS) $@ $^
 endif
 
 #--
@@ -628,12 +623,24 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-   $(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
-#endif
+endif
+
+.SUFFIXES:
+
+%.o:: %.c %.d
+   $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o:: %.cpp %.d $(PCH_OUTPUT)
+   $(CXX) $(PCHFLAGS) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o:: %.m %.d
+   $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o:: %.mm %.d
+   $(CXX) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
 
 #--
 # Automatic variables based on items already entered. Below we create
@@ -641,43 +648,21 @@
 # that end with .c with .o, and we also create a list of prerequisite
 # files by replacing all .c files with .d.
 #--
-PREREQS := $(OBJECTS:.o=.d)
+PREREQS := $(OBJECTS:.o=.d) $(ARCHIVE_OBJECTS:.o=.d)
 DWOS := $(OBJECTS:.o=.dwo) $(ARCHIVE_OBJECTS:.o=.dwo)
 ifneq "$(DYLIB_NAME)" ""
DYLIB_PREREQS := $(DYLIB_OBJECTS:.o=.d)
DYLIB_DWOS := $(DYLIB_OBJECTS:.o=.dwo)
 endif
 
-#--
-# Rule for Generating Prerequisites Automatically using .d files and
-# the compiler -MM option. The -M option will list all system headers,
-# and the -MM option will list all non-system dependencies.
-#--
-%.d: %.c
-   $(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.cpp
-   @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.m
-   @$(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.mm
-   @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
+# Don't error if a .d file is deleted.
+$(PREREQS) $(DYLIB_PREREQS): ;
 
 #--
 # Include all of the makefiles for each source file so we don't have
 # to manually track all of the prerequisites for each source file.
 #--
-sinclude $(PREREQS)
-ifneq "$(DYLIB_NAME)" ""
-   sinclude $(DYLIB_PREREQS)
-endif
-
-# Define a suffix rule for .mm -> .o
-.SUFFIXES: .mm .o
-.mm.o:
-   $(CXX) $(CXXFLAGS) -c $<
+include $(wildcard $(PREREQS) $(DYLIB_PREREQS))
 
 .PHONY: clean
 dsym:  $(DSYM)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 318414.
MaskRay marked an inline comment as done.
MaskRay added a comment.

Adopt rupprecht's suggestion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -279,11 +279,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-   ifeq ($(OS),NetBSD)
-   LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-   endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
ifneq (,$(filter YES,$(ENABLE_THREADS)))
LDFLAGS += -pthread
@@ -393,21 +388,18 @@
 
 ifeq (1,$(USE_LIBCPP))
CXXFLAGS += -DLLDB_USING_LIBCPP
-   ifeq "$(OS)" "Linux"
-   ifneq (,$(findstring clang,$(CC)))
-   CXXFLAGS += -stdlib=libc++
-   LDFLAGS += -stdlib=libc++
-   else
-   CXXFLAGS += -isystem /usr/include/c++/v1
-   LDFLAGS += -lc++
-   endif
-   else ifeq "$(OS)" "Android"
+   ifeq "$(OS)" "Android"
# Nothing to do, this is already handled in
# Android.rules.
else
CXXFLAGS += -stdlib=libc++
LDFLAGS += -stdlib=libc++
endif
+   ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+   ifneq (,$(LLVM_LIBS_DIR))
+   LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+   endif
+   endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,15 +761,15 @@
 return True, "libc++ always present"
 
 if platform == "linux":
-if os.path.isdir("/usr/include/c++/v1"):
-return True, "Headers found, let's hope they work"
 with tempfile.NamedTemporaryFile() as f:
 cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", 
f.name, "-"]
 p = subprocess.Popen(cmd, stdin=subprocess.PIPE, 
stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
 _, stderr = p.communicate("#include \nint main() {}")
-if not p.returncode:
-return True, "Compiling with -stdlib=libc++ works"
-return False, "Compiling with -stdlib=libc++ fails with the error: 
%s" % stderr
+if p.returncode != 0:
+return False, "Compiling with -stdlib=libc++ fails with the 
error: %s" % stderr
+if subprocess.call([f.name]) != 0:
+return False, "Compiling with -stdlib=libc++ works, but fails 
to run"
+return True, "Compiling with -stdlib=libc++ works"
 
 return False, "Don't know how to build with libc++ on %s" % platform
 


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -279,11 +279,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-	ifeq ($(OS),NetBSD)
-		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-	endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
 	ifneq (,$(filter YES,$(ENABLE_THREADS)))
 		LDFLAGS += -pthread
@@ -393,21 +388,18 @@
 
 ifeq (1,$(USE_LIBCPP))
 	CXXFLAGS += -DLLDB_USING_LIBCPP
-	ifeq "$(OS)" "Linux"
-		ifneq (,$(findstring clang,$(CC)))
-			CXXFLAGS += -stdlib=libc++
-			LDFLAGS += -stdlib=libc++
-		else
-			CXXFLAGS += -isystem /usr/include/c++/v1
-			LDFLAGS += -lc++
-		endif
-	else ifeq "$(OS)" "Android"
+	ifeq "$(OS)" "Android"
 		# Nothing to do, this is already handled in
 		# Android.rules.
 	else
 		CXXFLAGS += -stdlib=libc++
 		LDFLAGS += -stdlib=libc++
 	endif
+	ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+		ifneq (,$(LLVM_LIBS_DIR))
+			LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+		endif
+	endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,15 +761,15 @@
 return True, "libc++ always present"
 
 if platform == 

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 318546.
MaskRay added a comment.

Drop fancy check in dotest.py (restore the previous behavior)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -279,11 +279,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-   ifeq ($(OS),NetBSD)
-   LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-   endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
ifneq (,$(filter YES,$(ENABLE_THREADS)))
LDFLAGS += -pthread
@@ -393,21 +388,18 @@
 
 ifeq (1,$(USE_LIBCPP))
CXXFLAGS += -DLLDB_USING_LIBCPP
-   ifeq "$(OS)" "Linux"
-   ifneq (,$(findstring clang,$(CC)))
-   CXXFLAGS += -stdlib=libc++
-   LDFLAGS += -stdlib=libc++
-   else
-   CXXFLAGS += -isystem /usr/include/c++/v1
-   LDFLAGS += -lc++
-   endif
-   else ifeq "$(OS)" "Android"
+   ifeq "$(OS)" "Android"
# Nothing to do, this is already handled in
# Android.rules.
else
CXXFLAGS += -stdlib=libc++
LDFLAGS += -stdlib=libc++
endif
+   ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+   ifneq (,$(LLVM_LIBS_DIR))
+   LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+   endif
+   endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,8 +761,6 @@
 return True, "libc++ always present"
 
 if platform == "linux":
-if os.path.isdir("/usr/include/c++/v1"):
-return True, "Headers found, let's hope they work"
 with tempfile.NamedTemporaryFile() as f:
 cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", 
f.name, "-"]
 p = subprocess.Popen(cmd, stdin=subprocess.PIPE, 
stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -279,11 +279,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-	ifeq ($(OS),NetBSD)
-		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-	endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
 	ifneq (,$(filter YES,$(ENABLE_THREADS)))
 		LDFLAGS += -pthread
@@ -393,21 +388,18 @@
 
 ifeq (1,$(USE_LIBCPP))
 	CXXFLAGS += -DLLDB_USING_LIBCPP
-	ifeq "$(OS)" "Linux"
-		ifneq (,$(findstring clang,$(CC)))
-			CXXFLAGS += -stdlib=libc++
-			LDFLAGS += -stdlib=libc++
-		else
-			CXXFLAGS += -isystem /usr/include/c++/v1
-			LDFLAGS += -lc++
-		endif
-	else ifeq "$(OS)" "Android"
+	ifeq "$(OS)" "Android"
 		# Nothing to do, this is already handled in
 		# Android.rules.
 	else
 		CXXFLAGS += -stdlib=libc++
 		LDFLAGS += -stdlib=libc++
 	endif
+	ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+		ifneq (,$(LLVM_LIBS_DIR))
+			LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+		endif
+	endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,8 +761,6 @@
 return True, "libc++ always present"
 
 if platform == "linux":
-if os.path.isdir("/usr/include/c++/v1"):
-return True, "Headers found, let's hope they work"
 with tempfile.NamedTemporaryFile() as f:
 cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.name, "-"]
 p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 317599.
MaskRay added a comment.

Delete `if os.path.isdir("/usr/include/c++/v1"):`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -281,11 +281,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-   ifeq ($(OS),NetBSD)
-   LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-   endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
ifneq (,$(filter YES,$(ENABLE_THREADS)))
LDFLAGS += -pthread
@@ -395,21 +390,18 @@
 
 ifeq (1,$(USE_LIBCPP))
CXXFLAGS += -DLLDB_USING_LIBCPP
-   ifeq "$(OS)" "Linux"
-   ifneq (,$(findstring clang,$(CC)))
-   CXXFLAGS += -stdlib=libc++
-   LDFLAGS += -stdlib=libc++
-   else
-   CXXFLAGS += -isystem /usr/include/c++/v1
-   LDFLAGS += -lc++
-   endif
-   else ifeq "$(OS)" "Android"
+   ifeq "$(OS)" "Android"
# Nothing to do, this is already handled in
# Android.rules.
else
CXXFLAGS += -stdlib=libc++
LDFLAGS += -stdlib=libc++
endif
+   ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+   ifneq (,$(LLVM_LIBS_DIR))
+   LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+   endif
+   endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,8 +761,6 @@
 return True, "libc++ always present"
 
 if platform == "linux":
-if os.path.isdir("/usr/include/c++/v1"):
-return True, "Headers found, let's hope they work"
 with tempfile.NamedTemporaryFile() as f:
 cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", 
f.name, "-"]
 p = subprocess.Popen(cmd, stdin=subprocess.PIPE, 
stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -281,11 +281,6 @@
 LD = $(CC)
 LDFLAGS ?= $(CFLAGS)
 LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
-ifneq (,$(LLVM_LIBS_DIR))
-	ifeq ($(OS),NetBSD)
-		LDFLAGS += -L$(LLVM_LIBS_DIR) -Wl,-rpath,$(LLVM_LIBS_DIR)
-	endif
-endif
 ifeq (,$(filter $(OS), Windows_NT Android Darwin))
 	ifneq (,$(filter YES,$(ENABLE_THREADS)))
 		LDFLAGS += -pthread
@@ -395,21 +390,18 @@
 
 ifeq (1,$(USE_LIBCPP))
 	CXXFLAGS += -DLLDB_USING_LIBCPP
-	ifeq "$(OS)" "Linux"
-		ifneq (,$(findstring clang,$(CC)))
-			CXXFLAGS += -stdlib=libc++
-			LDFLAGS += -stdlib=libc++
-		else
-			CXXFLAGS += -isystem /usr/include/c++/v1
-			LDFLAGS += -lc++
-		endif
-	else ifeq "$(OS)" "Android"
+	ifeq "$(OS)" "Android"
 		# Nothing to do, this is already handled in
 		# Android.rules.
 	else
 		CXXFLAGS += -stdlib=libc++
 		LDFLAGS += -stdlib=libc++
 	endif
+	ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))
+		ifneq (,$(LLVM_LIBS_DIR))
+			LDFLAGS += -Wl,-rpath,$(LLVM_LIBS_DIR)
+		endif
+	endif
 endif
 
 #--
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -761,8 +761,6 @@
 return True, "libc++ always present"
 
 if platform == "linux":
-if os.path.isdir("/usr/include/c++/v1"):
-return True, "Headers found, let's hope they work"
 with tempfile.NamedTemporaryFile() as f:
 cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.name, "-"]
 p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay updated this revision to Diff 317382.
MaskRay edited the summary of this revision.
MaskRay added a comment.

Don't use double-colon. 
https://lists.gnu.org/archive/html/help-make/2021-01/msg00016.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -570,13 +570,8 @@
 # Make the archive
 #--
 ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-   $(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-   $(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach 
ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
+$(ARCHIVE_NAME): $(ARCHIVE_OBJECTS)
+   $(AR) $(ARFLAGS) $@ $^
 endif
 
 #--
@@ -628,12 +623,24 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-   $(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
-#endif
+endif
+
+.SUFFIXES:
+
+%.o: %.c %.d
+   $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.cpp %.d $(PCH_OUTPUT)
+   $(CXX) $(PCHFLAGS) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.m %.d
+   $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.mm %.d
+   $(CXX) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
 
 #--
 # Automatic variables based on items already entered. Below we create
@@ -641,43 +648,21 @@
 # that end with .c with .o, and we also create a list of prerequisite
 # files by replacing all .c files with .d.
 #--
-PREREQS := $(OBJECTS:.o=.d)
+PREREQS := $(OBJECTS:.o=.d) $(ARCHIVE_OBJECTS:.o=.d)
 DWOS := $(OBJECTS:.o=.dwo) $(ARCHIVE_OBJECTS:.o=.dwo)
 ifneq "$(DYLIB_NAME)" ""
DYLIB_PREREQS := $(DYLIB_OBJECTS:.o=.d)
DYLIB_DWOS := $(DYLIB_OBJECTS:.o=.dwo)
 endif
 
-#--
-# Rule for Generating Prerequisites Automatically using .d files and
-# the compiler -MM option. The -M option will list all system headers,
-# and the -MM option will list all non-system dependencies.
-#--
-%.d: %.c
-   $(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.cpp
-   @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.m
-   @$(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.mm
-   @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
+# Don't error if a .d file is deleted.
+$(PREREQS) $(DYLIB_PREREQS): ;
 
 #--
 # Include all of the makefiles for each source file so we don't have
 # to manually track all of the prerequisites for each source file.
 #--
-sinclude $(PREREQS)
-ifneq "$(DYLIB_NAME)" ""
-   sinclude $(DYLIB_PREREQS)
-endif
-
-# Define a suffix rule for .mm -> .o
-.SUFFIXES: .mm .o
-.mm.o:
-   $(CXX) $(CXXFLAGS) -c $<
+include $(wildcard $(PREREQS) $(DYLIB_PREREQS))
 
 .PHONY: clean
 dsym:  $(DSYM)


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -570,13 +570,8 @@
 # Make the archive
 #--
 ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-	$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-	$(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
+$(ARCHIVE_NAME): $(ARCHIVE_OBJECTS)
+	$(AR) $(ARFLAGS) $@ $^
 endif
 
 #--
@@ -628,12 +623,24 @@
 # Make the precompiled header and compile C++ sources against it
 #--
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
 	$(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-	$(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
-#endif
+endif
+

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D94890#2507856 , @JDevlieghere 
wrote:

> In D94890#2507241 , @MaskRay wrote:
>
>> In D94890#2505988 , @labath wrote:
>>
>>> Looks like a nice cleanup. The only part I am not sure of is the part about 
>>> removing `$(RM) $(ARCHIVE_OBJECTS)`. Is that necessary?
>>> I'm not sure why is that line there, but if I had to guess, I would say 
>>> it's to ensure that lldb (on macos) reads debug info from the archive file 
>>> instead of the original .o files. If it's not required, it may be better to 
>>> leave it in. Otherwise, someone from Apple should say whether that is ok 
>>> (testing archives is only really interesting on fruity platforms).
>>
>> I can add back it under the `ifeq "$(OS)" "Darwin"` guard if Apple folks 
>> think it is useful.
>
> It looks like we have only one test on llvm.org (+ one additional test in the 
> Swift fork) that's using this. My vote is to just remove this together with 
> the `ARCHIVE_C_SOURCES`, `ARCHIVE_CXX_SOURCES`, `ARCHIVE_OBJC_SOURCES` and 
> `ARCHIVE_OBJCXX_SOURCES` and inline it in that one test.

Agree.

" (+ one additional test in the Swift fork)" --- Sounds like this can be a 
separate patch which should Swift folks a heads-up. I don't know how to test 
Swift and probably someone else can do it:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

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

[Lldb-commits] [PATCH] D94890: Makefile.rules: Avoid redundant .d generation and make restart

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D94890#2505988 , @labath wrote:

> Looks like a nice cleanup. The only part I am not sure of is the part about 
> removing `$(RM) $(ARCHIVE_OBJECTS)`. Is that necessary?
> I'm not sure why is that line there, but if I had to guess, I would say it's 
> to ensure that lldb (on macos) reads debug info from the archive file instead 
> of the original .o files. If it's not required, it may be better to leave it 
> in. Otherwise, someone from Apple should say whether that is ok (testing 
> archives is only really interesting on fruity platforms).

I can add back it under the `ifeq "$(OS)" "Darwin"` guard if Apple folks think 
it is useful.

> BTW, have you measured any speedups from these improvements?

I don't notice a difference in `check-lldb-api`'s `Testing Time: `. I think the 
time is probably dominated by running lldb and Python..

If the user builds `-DBUILD_SHARED_LIBS=on` clang and thus has a significant 
startup overhead, I guess there may be some differences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94890

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

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D94888#2505992 , @labath wrote:

> It looks like this is removing the ability to build libc++ tests with gcc (as 
> it does not have the `-stdlib` option). While having that ability would be 
> nice, I don't believe there's anyone currently using that configuration, so 
> it shouldn't stand in the way of other things. But we should also update the 
> python detection code then (in `canRunLibcxxTests` in 
> `packages/Python/lldbsuite/test/dotest.py` -- I guess you just need to remove 
> the `if os.path.isdir("/usr/include/c++/v1"):` blurb)
>
> As for testing against the system libc++ with clang, I guess that should 
> still work, as the extra rpath will be just ignored in that case...

I do not know whether the following few lines D9426 
 were intentional.

CXXFLAGS += -isystem /usr/include/c++/v1
LDFLAGS += -lc++

For a proper setup, I think more stuff is needed. `/usr/include/c++/v1` 
probably works for many Linux distributions but the choice doesn't look nice. 
When libc++ is built with lldb, it probably use the libc++ include directory 
instead. LDFLAGS will thus need a specific `-L`.

I think deleting the code until someone complains is fine? :)

> You seem to be removing more than adding there.

Can you kindly test this on NetBSD? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94888

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

[Lldb-commits] [PATCH] D95119: Prefer /usr/bin/env xxx over /usr/bin/xxx where xxx = perl, python, awk

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

`#!/usr/bin/env perl -w` (2 arguments) unfortunately does not work on Linux and 
many other systems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95119

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

[Lldb-commits] [PATCH] D95119: Prefer /usr/bin/env xxx over /usr/bin/xxx where xxx = perl, python, awk

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Several `llvm/utils/` scripts can probably just be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95119

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

[Lldb-commits] [PATCH] D101462: [MC] Untangle MCContext and MCObjectFileInfo

[Fangrui Song 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 rG632ebc4ab437: [MC] Untangle MCContext and MCObjectFileInfo 
(authored by flip1995, committed by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101462

Files:
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/tools/driver/cc1as_main.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/source/Plugins/Instruction/MIPS/EmulateInstructionMIPS.cpp
  lldb/source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/include/llvm/MC/MCObjectFileInfo.h
  llvm/lib/CodeGen/MachineModuleInfo.cpp
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/DWARFLinker/DWARFStreamer.cpp
  llvm/lib/MC/MCAsmStreamer.cpp
  llvm/lib/MC/MCContext.cpp
  llvm/lib/MC/MCDisassembler/Disassembler.cpp
  llvm/lib/MC/MCMachOStreamer.cpp
  llvm/lib/MC/MCObjectFileInfo.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/COFFAsmParser.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/MC/MCStreamer.cpp
  llvm/lib/MC/MCWinCOFFStreamer.cpp
  llvm/lib/Object/ModuleSymbolTable.cpp
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXTargetStreamer.cpp
  llvm/lib/Target/TargetLoweringObjectFile.cpp
  llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
  llvm/tools/llvm-dwp/llvm-dwp.cpp
  llvm/tools/llvm-exegesis/lib/Analysis.cpp
  llvm/tools/llvm-exegesis/lib/LlvmState.cpp
  llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
  llvm/tools/llvm-jitlink/llvm-jitlink.cpp
  llvm/tools/llvm-mc-assemble-fuzzer/llvm-mc-assemble-fuzzer.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-ml/Disassembler.cpp
  llvm/tools/llvm-ml/llvm-ml.cpp
  llvm/tools/llvm-objdump/MachODump.cpp
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/tools/llvm-profgen/ProfiledBinary.cpp
  llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp
  llvm/tools/sancov/sancov.cpp
  llvm/unittests/CodeGen/MachineInstrTest.cpp
  llvm/unittests/CodeGen/MachineOperandTest.cpp
  llvm/unittests/CodeGen/TestAsmPrinter.cpp
  llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp
  llvm/unittests/MC/DwarfLineTables.cpp
  llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
  mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp

Index: mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
===
--- mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
+++ mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
@@ -169,8 +169,8 @@
   mai->setRelaxELFRelocations(true);
 
   llvm::MCObjectFileInfo mofi;
-  llvm::MCContext ctx(mai.get(), mri.get(), , , );
-  mofi.InitMCObjectFileInfo(triple, false, ctx, false);
+  llvm::MCContext ctx(triple, mai.get(), mri.get(), , , );
+  mofi.initMCObjectFileInfo(ctx, /*PIC=*/false, /*LargeCodeModel=*/false);
 
   SmallString<128> cwd;
   if (!llvm::sys::fs::current_path(cwd))
Index: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
===
--- llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
+++ llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
@@ -112,9 +112,9 @@
 SrcMgr.AddNewSourceBuffer(std::move(Buffer), SMLoc());
 EXPECT_EQ(Buffer, nullptr);
 
-Ctx.reset(
-new MCContext(MUPMAI.get(), MRI.get(), , , ));
-MOFI.InitMCObjectFileInfo(Triple, false, *Ctx, false);
+Ctx.reset(new MCContext(Triple, MUPMAI.get(), MRI.get(), , STI.get(),
+, ));
+MOFI.initMCObjectFileInfo(*Ctx, /*PIC=*/false, /*LargeCodeModel=*/false);
 
 Str.reset(TheTarget->createNullStreamer(*Ctx));
 
Index: llvm/unittests/MC/DwarfLineTables.cpp
===
--- llvm/unittests/MC/DwarfLineTables.cpp
+++ llvm/unittests/MC/DwarfLineTables.cpp
@@ -21,7 +21,7 @@
 
 namespace {
 struct Context {
-  const char *Triple = "x86_64-pc-linux";
+  const char *TripleName = "x86_64-pc-linux";
   std::unique_ptr MRI;
   std::unique_ptr MAI;
   std::unique_ptr Ctx;
@@ -33,14 +33,15 @@
 
 // If we didn't build x86, do not run the test.
 std::string Error;
-const Target *TheTarget = TargetRegistry::lookupTarget(Triple, Error);
+const Target *TheTarget = TargetRegistry::lookupTarget(TripleName, Error);
 if (!TheTarget)
   return;
 
-MRI.reset(TheTarget->createMCRegInfo(Triple));
+MRI.reset(TheTarget->createMCRegInfo(TripleName));
 MCTargetOptions MCOptions;
-MAI.reset(TheTarget->createMCAsmInfo(*MRI, Triple, MCOptions));
-Ctx = std::make_unique(MAI.get(), MRI.get(), nullptr);
+MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCOptions));
+Ctx =