[PATCH] D159435: [NFC] remove unneded header includes

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This breaks the LLDB (modules) build: 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59738/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159435

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

[PATCH] D158071: [clang] Remove rdar links; NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I had a cursory look and the handful of radar links I opened and read through 
didn't have any context that wasn't already in the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158071

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

[PATCH] D154329: [lldb] Replace llvm::writeFileAtomically with llvm::writeToOutput API.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D154329/new/

https://reviews.llvm.org/D154329

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

[PATCH] D118754: [DebugInfo] Always emit `.debug_names` with dwarf 5 for Apple platforms

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe0d57295bf6a: [DebugInfo] Always emit `.debug_names` with 
DWARF 5 for Apple platforms (authored by JDevlieghere).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118754

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-names.c
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
  llvm/lib/IR/DebugInfoMetadata.cpp
  llvm/test/DebugInfo/Inputs/name-table-kind-apple-4.ll
  llvm/test/DebugInfo/Inputs/name-table-kind-apple-5.ll
  llvm/test/DebugInfo/accel-tables-apple.ll

Index: llvm/test/DebugInfo/accel-tables-apple.ll
===
--- /dev/null
+++ llvm/test/DebugInfo/accel-tables-apple.ll
@@ -0,0 +1,55 @@
+; Verify the emission of accelerator tables for nameTableKind: Apple
+
+; Darwin has the apple tables unless we specifically tune for gdb
+; RUN: llc -mtriple=x86_64-apple-darwin12 -filetype=obj < %S/Inputs/name-table-kind-apple-5.ll \
+; RUN:   | llvm-readobj --sections - | FileCheck --check-prefix=DEBUG_NAMES %s
+; RUN: llc -mtriple=x86_64-apple-darwin12 -filetype=obj < %S/Inputs/name-table-kind-apple-4.ll \
+; RUN:   | llvm-readobj --sections - | FileCheck --check-prefix=APPLE %s
+
+; APPLE-NOT: debug_names
+; APPLE-NOT: debug{{.*}}pub
+; APPLE: apple_names
+; APPLE-NOT: debug_names
+; APPLE-NOT: debug{{.*}}pub
+
+; DEBUG_NAMES-NOT: apple_names
+; DEBUG_NAMES-NOT: pubnames
+; DEBUG_NAMES: debug_names
+; DEBUG_NAMES-NOT: apple_names
+; DEBUG_NAMES-NOT: pubnames
+
+@var = thread_local global i32 0, align 4, !dbg !0
+
+; Function Attrs: norecurse nounwind readnone uwtable
+define void @_Z3funv() local_unnamed_addr #0 !dbg !11 {
+  ret void, !dbg !14
+}
+
+; Function Attrs: norecurse uwtable
+define weak_odr hidden ptr @_ZTW3var() local_unnamed_addr #1 {
+  ret ptr @var
+}
+
+attributes #0 = { norecurse nounwind readnone uwtable }
+attributes #1 = { norecurse uwtable }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7, !8, !9}
+!llvm.ident = !{!10}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "var", scope: !2, file: !3, line: 1, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 7.0.0 (trunk 322268) (llvm/trunk 322267)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: Apple)
+!3 = !DIFile(filename: "debugger-tune.cpp", directory: "/tmp")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !{i32 2, !"Dwarf Version", i32 4}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{!"clang version 7.0.0 (trunk 322268) (llvm/trunk 322267)"}
+!11 = distinct !DISubprogram(name: "fun", linkageName: "_Z3funv", scope: !3, file: !3, line: 2, type: !12, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !2, retainedNodes: !4)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DILocation(line: 2, column: 13, scope: !11)
+
Index: llvm/test/DebugInfo/Inputs/name-table-kind-apple-5.ll
===
--- /dev/null
+++ llvm/test/DebugInfo/Inputs/name-table-kind-apple-5.ll
@@ -0,0 +1,35 @@
+@var = thread_local global i32 0, align 4, !dbg !0
+
+; Function Attrs: norecurse nounwind readnone uwtable
+define void @_Z3funv() local_unnamed_addr #0 !dbg !11 {
+  ret void, !dbg !14
+}
+
+; Function Attrs: norecurse uwtable
+define weak_odr hidden ptr @_ZTW3var() local_unnamed_addr #1 {
+  ret ptr @var
+}
+
+attributes #0 = { norecurse nounwind readnone uwtable }
+attributes #1 = { norecurse uwtable }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7, !8, !9}
+!llvm.ident = !{!10}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "var", scope: !2, file: !3, line: 1, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 7.0.0 (trunk 322268) (llvm/trunk 322267)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: Apple)
+!3 = !DIFile(filename: "debugger-tune.cpp", directory: "/tmp")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !{i32 2, !"Dwarf Version", i32 5}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{!"clang version 7.0.0 (trunk 322268) (llvm/trunk 322267)"}
+!11 = distinct !DISubprogram(name: "fun", linkageName: 

[PATCH] D136290: [clang] Disable assertion that can "easily happen"

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG97b91307b00e: [clang] Disable assertion that can 
easily happen (authored by JDevlieghere).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136290

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2669,12 +2669,12 @@
 }
 
 unsigned ASTWriter::getSubmoduleID(Module *Mod) {
+  unsigned ID = getLocalOrImportedSubmoduleID(Mod);
   // FIXME: This can easily happen, if we have a reference to a submodule that
   // did not result in us loading a module file for that submodule. For
   // instance, a cross-top-level-module 'conflict' declaration will hit this.
-  unsigned ID = getLocalOrImportedSubmoduleID(Mod);
-  assert((ID || !Mod) &&
- "asked for module ID for non-local, non-imported module");
+  // assert((ID || !Mod) &&
+  //"asked for module ID for non-local, non-imported module");
   return ID;
 }
 


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2669,12 +2669,12 @@
 }
 
 unsigned ASTWriter::getSubmoduleID(Module *Mod) {
+  unsigned ID = getLocalOrImportedSubmoduleID(Mod);
   // FIXME: This can easily happen, if we have a reference to a submodule that
   // did not result in us loading a module file for that submodule. For
   // instance, a cross-top-level-module 'conflict' declaration will hit this.
-  unsigned ID = getLocalOrImportedSubmoduleID(Mod);
-  assert((ID || !Mod) &&
- "asked for module ID for non-local, non-imported module");
+  // assert((ID || !Mod) &&
+  //"asked for module ID for non-local, non-imported module");
   return ID;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D131422/new/

https://reviews.llvm.org/D131422

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

[PATCH] D131020: Reland "[lldb/Fuzzer] Add fuzzer for expression evaluator"

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D131020

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

[PATCH] D131020: Reland "[lldb/Fuzzer] Add fuzzer for expression evaluator"

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt:51-56
+add_custom_target(fuzz-lldb-expression
+  COMMENT "Running the LLDB expression evaluator fuzzer..."
+  WORKING_DIRECTORY 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/expression-artifacts
+  COMMAND $ 
-artifact_prefix=expression- -reduce_inputs=0
+  USES_TERMINAL
+  )

Shouldn't this be setting the `LLDB_FUZZER_TARGET`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131020

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D112374#3665989 , @mizvekov wrote:

> If anyone wants to take a look at the new changes to lldb tests, be my guest. 
> Otherwise I will try to land this again soon. It might well be that we figure 
> out some other in-tree user is affected, but I'd rather do that sooner than 
> later.

Please allow @teemperor some time to take a look at the llvm changes before 
landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

[Jonas Devlieghere via Phabricator via cfe-commits]

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

A few nits about naming, but otherwise this LGTM.




Comment at: 
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp:47
+DEFINE_BINARY_PROTO_FUZZER(const clang_fuzzer::Function ) {
+  auto S = clang_fuzzer::FunctionToString(input);
+





Comment at: 
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp:51-52
+  // This will be used as the path for the object file to create a target from
+  std::string rawpath = originalargv[2];
+  StringRef objpath = rawpath.erase(0, 2);
+





Comment at: 
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp:59
+  // Create a breakpoint on the only line in the program
+  SBBreakpoint bp = target.BreakpointCreateByLocation(objpath.str().c_str(), 
1);
+





Comment at: 
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp:62
+  // Create launch info and error for launching the process
+  SBLaunchInfo li = target.GetLaunchInfo();
+  SBError error;




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

https://reviews.llvm.org/D129377

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a subscriber: teemperor.
JDevlieghere added a comment.

In D112374#3653702 , @mizvekov wrote:

> @JDevlieghere I spent a lot of time trying to get this test running on my 
> machine to no avail. I think lldb build and test setup is quite convoluted, 
> fragile and antiquated. It uses many deprecated CMake features, It fails to 
> properly link to system libraries it needs like librt. And I just kept 
> patching these problems up and more kept coming. At some point I just gave up.

I'm sorry to hear you're having trouble building LLDB. The LLDB website has 
quite an elaborate guide with instructions in how to build LLDB: 
https://lldb.llvm.org/resources/build.html, including specific instructions on 
Windows. Windows is not my main platform, but I've successfully built LLDB 
there in the past following those instructions. I'm not sure what you feel is 
"convoluted, fragile and antiquated" about our build and test setup, as it's 
fairly similar to the rest of LLVM. I'd be happy to hear your suggestions on 
how we could improve things.

> And even this test itself requires the whole libcxx it seems, which is 
> another difficult thing to get building outside of CI.
>
> I think pre-commit CI is essential here.
>
> So I would be happy to help sort this lldb bug, but you have to help me help 
> you.

I'm happy to help out. I personally don't know if we should go with (1) or (2), 
both sound reasonable in their own way. I'd like @teemperor, who's the author 
of the feature and the affected tests, to weigh in.

> I think reverting it with no prior notification was unreasonable, and landing 
> this back as it was, no changes, is reasonable.
>
> Do you agree? Please let me know soon otherwise.

I don't. I think reverting your change was well within the guidelines outlined 
by LLVM's patch reversion policy: 
https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

Additionally, I think you could've given me a little bit more time to catch up 
on the discussion here. The code review policy and practices 
(https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) 
recommend pinging every few days to once per week depending on how urgent the 
patch is.

By relanding, you broke the bots again 
(https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45354/#showFailuresLink)
 and I'm forced to revert this change a second time. Please refrain from 
landing this again until we've settled on a way forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/cxx_proto.proto:1
+//===-- cxx_proto.proto - Protobuf description of C++ 
-===//
+//

Do we still need a copy of this for LLDB?


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

https://reviews.llvm.org/D129377

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This breaks all the LLDB tests that import the std module:

  import-std-module/array.TestArrayFromStdModule.py
  import-std-module/deque-basic.TestDequeFromStdModule.py
  
import-std-module/deque-dbg-info-content.TestDbgInfoContentDequeFromStdModule.py
  import-std-module/forward_list.TestForwardListFromStdModule.py
  
import-std-module/forward_list-dbg-info-content.TestDbgInfoContentForwardListFromStdModule.py
  import-std-module/list.TestListFromStdModule.py
  import-std-module/list-dbg-info-content.TestDbgInfoContentListFromStdModule.py
  import-std-module/queue.TestQueueFromStdModule.py
  import-std-module/stack.TestStackFromStdModule.py
  import-std-module/vector.TestVectorFromStdModule.py
  import-std-module/vector-bool.TestVectorBoolFromStdModule.py
  
import-std-module/vector-dbg-info-content.TestDbgInfoContentVectorFromStdModule.py
  import-std-module/vector-of-vectors.TestVectorOfVectorsFromStdModule.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45301/

Given that the bot has been red for 14 hours I went ahead and reverted this 
change. Please keep an eye on this bot when relanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D128059#3646953 , @cor3ntin wrote:

> In D128059#3646695 , @JDevlieghere 
> wrote:
>
>> I had to revert this because it breaks a bunch of LLDB tests: 
>> https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45288/#showFailuresLink.
>>  It looks like this causes an error when building some SDK modules.
>
> Thanks (and sorry for the annoyance)

No worries at all. Thanks for looking into the issue!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128059

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

[PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/cmake/modules/ProtobufMutator.cmake:4-5
+  set (PBM_PREFIX clang_protobuf_mutator)
+elseif(${CMAKE_CURRENT_SOURCE_DIR} MATCHES "lldb")
+  set (PBM_PREFIX lldb_protobuf_mutator)
+endif()

cassanova wrote:
> mib wrote:
> > If feels wrong to me that the clang protobuf cmake module knows about lldb.
> > 
> > May be we should just have 2 separate files for clang and lldb
> My preferred solution to this was just creating a target called 
> ${LLVM_VARIABLE_THAT_TELLS_YOU_THE_SUBPROJECT_NAME}_protobuf_mutator to avoid 
> using if-statements and direct strings but it looks like clang and lldb 
> aren't defined as subprojects unless they're being built standalone.
> 
> Also in the event that some other subproject wanted to use this library then 
> this solution just gets messier. Having 2 different CMake module files for 
> the clang and lldb sides each or putting protobuf mutator in a more central 
> location is another way to approach this
I agree with Ismail. I think we can fix this issue by making it possible to set 
the `PBM_PREFIX` in the scope that includes `ProtobufMutator.cmake` like this:

In `clang/cmake/modules/ProtobufMutator.cmake`:

```
if (NOT PBM_PREFIX) 
  set (PBM_PREFIX protobuf_mutator)
endif()
```

In `lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt`:

```
  set (PBM_PREFIX lldb_protobuf_mutator)
  include(ProtobufMutator)
```

That said, I think an even better way to do this is by using a cached variable 
that keep track of whether we've already included the external project. That 
way we don't have to create two different targets that are essentially the 
same. If I'm following the code correctly, all the variables that we rely on 
can be computed without the `ExternalProject_Add`. I imagine something like:

```
if (NOT PBM_PROJECT_ADDED) 
  ExternalProject_Add(${PBM_PREFIX}
PREFIX ${PBM_PREFIX}
GIT_REPOSITORY https://github.com/google/libprotobuf-mutator.git
GIT_TAG master
CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
 -DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
BUILD_BYPRODUCTS ${PBM_LIB_PATH} ${PBM_FUZZ_LIB_PATH}
UPDATE_COMMAND ""
INSTALL_COMMAND ""
)
set(PBM_PROJECT_ADDED TRUE CACHE BOOL
"Whether the ProtoBufMutator external project was added")
endif()
```

I think that should preclude us from creating the target twice.


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

https://reviews.llvm.org/D129377

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

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I had to revert this because it breaks a bunch of LLDB tests: 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45288/#showFailuresLink.
 It looks like this causes an error when building some SDK modules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128059

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

[PATCH] D129456: [lldb] Add image dump pcm-info command

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

[bikeshedding] `pcm-info` seems a little odd. Do we have other command that end 
with `-info`? `target modules dump` already sounds like you're about to dump 
some kind of "info". What about just `pcm`? [/bikeshedding]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129456

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D126189#3643045 , @iains wrote:

> In D126189#3643021 , @JDevlieghere 
> wrote:
>
>> In D126189#3643001 , @iains wrote:
>>
>>> In D126189#3642992 , 
>>> @JDevlieghere wrote:
>>>
 In D126189#3642820 , @iains 
 wrote:

> JFTR, I did not get any notification from green dragon (which is odd, 
> AFAIR it's sent email before) or I would have looked right away  - kicked 
> off a build will take a look as soon as that's done.

 Yes, the bot was already red because of a different issue.

 FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f 
  and 
 4328b960176f4394416093e640ad4265bde65ad7 
  
 locally and I'm still getting a linker error about missing symbols:
>>>
>>> those refs come up as 'bad object' for me .. can you identify the upstream 
>>> changes?
>>
>> That's odd, both Github and Phab think those are the canonical hashes:
>>
>> https://github.com/llvm/llvm-project/commit/ac507102d258b6fc0cb57eb60c9dfabd57ff562f
>> https://github.com/llvm/llvm-project/commit/4328b960176f4394416093e640ad4265bde65ad7
>
> pilot error (pasto...)
>
   Undefined symbols for architecture arm64:
 "vtable for std::__1::format_error", referenced from:
 std::__1::format_error::format_error(char const*) in main.o
 std::__1::format_error::format_error(std::__1::basic_string>>> std::__1::char_traits, std::__1::allocator > const&) in main.o
>>>
>>> That seems also unrelated to the modules code, but I could always be 
>>> surprised :)
>
> OK - so if I cannot figure out what is happening in the next couple or hours, 
> I can revert those two commits (that's 9PM for me so I probably cannot do 
> much more today).

Thanks, if the bot reproduces the other issue I mentioned I'll bisect it down 
to see if there's another commit that's causing issues. I should've mentioned 
that that failures only happens when building with `-gmodules`, so it does 
sound somewhat related.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

The review shows the diff with the previous iteration of the patch. Can you 
generate a diff with the current top-of-tree?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129377

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D126189#3643001 , @iains wrote:

> In D126189#3642992 , @JDevlieghere 
> wrote:
>
>> In D126189#3642820 , @iains wrote:
>>
>>> JFTR, I did not get any notification from green dragon (which is odd, AFAIR 
>>> it's sent email before) or I would have looked right away  - kicked off a 
>>> build will take a look as soon as that's done.
>>
>> Yes, the bot was already red because of a different issue.
>>
>> FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f 
>>  and 
>> 4328b960176f4394416093e640ad4265bde65ad7 
>>  
>> locally and I'm still getting a linker error about missing symbols:
>
> those refs come up as 'bad object' for me .. can you identify the upstream 
> changes?

That's odd, both Github and Phab think those are the canonical hashes:

https://github.com/llvm/llvm-project/commit/ac507102d258b6fc0cb57eb60c9dfabd57ff562f
https://github.com/llvm/llvm-project/commit/4328b960176f4394416093e640ad4265bde65ad7

>>   Undefined symbols for architecture arm64:
>> "vtable for std::__1::format_error", referenced from:
>> std::__1::format_error::format_error(char const*) in main.o
>> std::__1::format_error::format_error(std::__1::basic_string> std::__1::char_traits, std::__1::allocator > const&) in main.o
>
> That seems also unrelated to the modules code, but I could always be 
> surprised :)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D126189#3642820 , @iains wrote:

> JFTR, I did not get any notification from green dragon (which is odd, AFAIR 
> it's sent email before) or I would have looked right away  - kicked off a 
> build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f 
 and 
4328b960176f4394416093e640ad4265bde65ad7 
 locally 
and I'm still getting a linker error about missing symbols:

  Undefined symbols for architecture arm64:
"vtable for std::__1::format_error", referenced from:
std::__1::format_error::format_error(char const*) in main.o
std::__1::format_error::format_error(std::__1::basic_string, std::__1::allocator > const&) in main.o


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D126189#3642777 , @iains wrote:

> In D126189#3642762 , @JDevlieghere 
> wrote:
>
>> This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:
>>
>>   Undefined symbols for architecture x86_64:
>> "__ZGIW10std_config", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>> "__ZGIW4span", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>> "__ZGIW5array", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>> "__ZGIW5stdio", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>> "__ZGIW6string", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>> "__ZGIW6vector", referenced from:
>> __GLOBAL__sub_I_main.cpp in main.o
>>
>> https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/
>
> ack, (I develop on macOS so usually catch these things)...
> do you need me to revert the commit - or can we try to find out what's broken 
> and fix it?

If you think you'll be able to get to the bottom of this quickly then I don't 
think we need to revert, but the bot has been red for a while (which means that 
other issues can get buried) so I'd like to get it green ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

  Undefined symbols for architecture x86_64:
"__ZGIW10std_config", referenced from:
__GLOBAL__sub_I_main.cpp in main.o
"__ZGIW4span", referenced from:
__GLOBAL__sub_I_main.cpp in main.o
"__ZGIW5array", referenced from:
__GLOBAL__sub_I_main.cpp in main.o
"__ZGIW5stdio", referenced from:
__GLOBAL__sub_I_main.cpp in main.o
"__ZGIW6string", referenced from:
__GLOBAL__sub_I_main.cpp in main.o
"__ZGIW6vector", referenced from:
__GLOBAL__sub_I_main.cpp in main.o

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This change triggers an assertion when building an LLDB test case:

  UNREACHABLE executed at 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaTemplate.cpp:1726!
  Assertion failed: (InstantiatingSpecializations.empty() && "failed to clean 
up an InstantiatingTemplate?"), function ~Sema, file 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/Sema.cpp,
 line 458.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.Program arguments: 
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang -fmodules 
-gmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
 -gmodules -fcxx-modules -std=c++11 -g -O0 -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
 -arch x86_64 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make
 -include 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h
 -fno-limit-debug-info -fmodules -gmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
 -gmodules -fcxx-modules -DLLDB_USING_LIBCPP -stdlib=libc++ -std=c++20 
--driver-mode=g++ -MT main.o -MD -MP -MF main.d -c -o main.o 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp
  1.
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:1:2:
 current parser token 'include'
  Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
  0  clang0x000106bc157e 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
  1  clang0x000106bc0258 llvm::sys::RunSignalHandlers() 
+ 248
  2  clang0x000106bc0962 
llvm::sys::CleanupOnSignal(unsigned long) + 210
  3  clang0x000106adb82a (anonymous 
namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) + 106
  4  clang0x000106adba2e 
CrashRecoverySignalHandler(int) + 110
  5  libsystem_platform.dylib 0x7fff67b405fd _sigtramp + 29
  6  libsystem_platform.dylib 0x7ffeeab5b460 _sigtramp + 
18446744071612509824
  7  libsystem_c.dylib0x7fff67a12808 abort + 120
  8  libsystem_c.dylib0x7fff67a11ac6 err + 0
  9  clang0x000108a1325c clang::Sema::~Sema() + 5660
  10 clang0x000107698e41 
clang::CompilerInstance::~CompilerInstance() + 625
  11 clang0x0001076a7fe5 
compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, 
llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, 
llvm::function_ref, llvm::function_ref) + 4517
  12 clang0x0001076a9b16 
compileModuleAndReadASTImpl(clang::CompilerInstance&, clang::SourceLocation, 
clang::SourceLocation, clang::Module*, llvm::StringRef) + 1238
  13 clang0x0001076a370b 
compileModuleAndReadAST(clang::CompilerInstance&, clang::SourceLocation, 
clang::SourceLocation, clang::Module*, llvm::StringRef) + 1947
  14 clang0x0001076a2b97 
clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, 
clang::SourceLocation, clang::SourceLocation, bool) + 3783
  15 clang0x0001076a3aa7 
clang::CompilerInstance::loadModule(clang::SourceLocation, 
llvm::ArrayRef>, 
clang::Module::NameVisibilityKind, bool) + 695
  16 clang0x000109b8a630 
clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, 
clang::Token&, clang::Token&, clang::SourceLocation, 
clang::detail::SearchDirIteratorImpl, clang::FileEntry const*) + 7792
  17 clang0x000109b822e1 
clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, 
clang::Token&, clang::detail::SearchDirIteratorImpl, clang::FileEntry 
const*) + 177
  18 clang0x000109b82f8c 
clang::Preprocessor::HandleDirective(clang::Token&) + 2604
  19 clang0x000109b50877 
clang::Lexer::LexTokenInternal(clang::Token&, bool) + 6199
  20 clang0x000109b4cf15 

[PATCH] D127684: [NFC] Use `https` instead of `http` in the `LLVM.org` URLs

[Jonas Devlieghere via Phabricator via cfe-commits]

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

+1 on breaking this down per sub-project and creating separate patches for 
review.

I sampled the patch and about a quarter or so of the links I tried didn't work, 
even before the change. There's no point in generating churn by changing those, 
but there's also little value in keeping them around, and fixing them is beyond 
the scope of this patch. I'm inclined to say let's leave them be, but I'm 
curious to hear what others think about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127684

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

[PATCH] D115438: [lldb] Remove unused lldb.cpp

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115438

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

[PATCH] D113743: [RFC][clang][DebugInfo] Allow function-local statics and types to be scoped within a lexical block

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Hey Kristina, this broke TestSetData.py on GreenDragon: 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39089/

Since the bot has been red for several hours I went ahead and reverted your 
change in 4cb79294e8df8c91ae15264d1014361815d34a53 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113743

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

[PATCH] D112767: [clang][driver] Fix multiarch output name with -Wl arg

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D112767/new/

https://reviews.llvm.org/D112767

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

[PATCH] D109128: [VFS] Use original path when falling back to external FS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180
 
+  if (ExternalFS)
+ExternalFS->setCurrentWorkingDirectory(Path);
+

I'm pretty sure there was a reason we stopped doing this. There should be some 
discussion about that in my original patch. 



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2012
+  if (auto Result = ExternalFS->status(CanonicalPath)) {
+return Result.get().copyWithNewName(Result.get(), OriginalPath);
+  } else {

Can we abstract this in a function similar to `getRedirectedFileStatus`, 
something like `getOrginialFileStatus` or `getNonCanonicalizedFileStatus` and 
have a comment explaining what it does and why? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

[PATCH] D111269: [clang][Driver] Make multiarch output file basenames reproducible

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D111269/new/

https://reviews.llvm.org/D111269

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

[PATCH] D110041: [clang] Use portable "#!/usr/bin/env bash" shebang for tools and utils.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D110041/new/

https://reviews.llvm.org/D110041

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

[PATCH] D109128: [VFS] Use original path when falling back to external FS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Yes, Keith and I came to the same conclusion yesterday. I was worried about 
tracking both paths at all times, but I like your suggestion of only changing 
the path when requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

[PATCH] D109128: [VFS] Use original path when falling back to external FS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Keith and I discussed this offline. My suggestion was to do the following:

1. Check the overlay for the canonicalized path
2. Check the fall-through for the canonicalized path
3. Check the fall-through for the original path

If I understand correctly, this patch does that, but swaps 2 and 3. What's the 
motivation for trying the non-canonical path first? If we treat the current 
working directory as a property directory (which is what the canonicalization 
is all about), then it seems like we should exhaust those options first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

[PATCH] D109128: [VFS] Use original path when falling back to external FS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I'm sure I'm missing something, but after rereading the patch several times I 
still don't see the functional change. It just looks like it's renaming every 
instance of `Path` to `CanonicalPath` and `Path_` to `Path`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109128

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

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:59
+MIRSampleLoader(
+std::make_unique(FileName, RemappingFileName)) {
+LowBit = getFSPassBitBegin(P);

You're instantiating a forward-declared type. This breaks the modules build: 
https://green.lab.llvm.org/green/job/lldb-cmake/34450/console



Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:289
+
+bool MIRProfileLoaderPass::runOnMachineFunction(MachineFunction ) {
+  if (!MIRSampleLoader->isValid())

Why is this outside the `llvm` namespace? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

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

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Adrian said "Let's do it!" on the mailing list so LGTM for Darin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:402-403
 case DW_AT_type:
-  type = form_value;
+  if (!type.IsValid())
+type = form_value;
   break;

What's the purpose of this? Do we expect to see the type attribute more than 
once? 



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:900-901
+  const auto *ft = qt->getAs();
+  TypeSystemClang *ts =
+  llvm::dyn_cast_or_null(function_type.GetTypeSystem());
+  ast.adjustDeducedFunctionResultType(

You're doing `dyn_cast_or_null` but then below you're dereferencing `ts` 
unconditionally? 



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1337-1339
+TypeSP ret_type = dwarf->FindTypeForAutoReturnForDIE(
+die, ConstString(attrs.mangled_name));
+if (ret_type) {

LLVM likes to put these variables in the if-clause, which I personally really 
like because it conveys the scope without hurting readability. 



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1341-1344
+  auto *function_decl = llvm::dyn_cast_or_null(
+  GetCachedClangDeclContextForDIE(die));
+
+  if (function_decl)





Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2672-2675
+  TypeSP type_sp;
+
+  if (!name)
+return type_sp;

I know this pattern is common in LLDB, but I really dislike it because you have 
to backtrack all the way to the beginning of the function to know if `type_sp` 
has been modified in any way. When I write code like, this I tend to use 
`return {};` to make it clear I'm returning a default constructed instance of 
the return type. That also makes it clear where we're actually returning a 
non-default instance by just looking at the `return`s. 



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2718
+
+  type_sp = func_type->shared_from_this();
+}




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

https://reviews.llvm.org/D105564

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

[PATCH] D100901: [CMake][llvm] avoid conflict w/ (and use when available) new builtin check_linker_flag

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100901

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D45639#2706605 , @ldionne wrote:

> In D45639#2706589 , @dexonsmith 
> wrote:
>
>> I'm not sure I'm totally following, but just want to double check that the 
>> tests won't somehow use the libc++ from the SDK instead of ToT? I guess the 
>> test uses `-nostdinc++` or something?
>>
>> Assuming that's doing what we want... I also wanted to highlight this adds a 
>> dependency on bot's host clang working with the ToT libc++ headers. That's 
>> probably fine -- I assume we keep the bot's Xcode relatively up-to-date -- 
>> but I know @ldionne at some point was trying to avoid requiring new libc++ 
>> to work with older clangs.

The simulator tests are now using both headers and library from the SDK.

> LLDB should either use the libc++ headers from the SDK *and* the dylib from 
> the SDK, or build libc++ from scratch and test against both the trunk headers 
> and the trunk dylib built for their target, not for the host as they seem to 
> do right now. I'm following offline with @JDevlieghere to try and disentangle 
> that.

As discussed on Slack on Wednesday, lldb is doing the former for the simulator 
tests. All other tests are building for the host and are using the just-built 
headers and library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D45639#2706364 , @JDevlieghere 
wrote:

> Given that these tests are macOS specific and already require a specific SDK, 
> I'll just update them to use the compiler from the SDK instead of the 
> just-built one.

Done in 5d1c43f333c2327be61604dc90ea675f0d1e6913 
 and 
reverted my revert (i.e. re-landed your change in 
6331680ad2ad000fdaf7e72f3c1880c7908ffa25 
).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Given that these tests are macOS specific and already require a specific SDK, 
I'll just update them to use the compiler from the SDK instead of the 
just-built one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D45639#2705919 , @phosek wrote:

> In D45639#2705702 , @ldionne wrote:
>
>> In D45639#2703913 , @JDevlieghere 
>> wrote:
>>
>>> This breaks `TestAppleSimulatorOSType.py ` on GreenDragon. First failed 
>>> build: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31346/
>>> [...]
>>>
>>> Based on your description above, shouldn't it prefer the libc++ form the 
>>> sysroot?
>>
>> Actually, it's the other way around. In the discussion above, we say it's 
>> the toolchain path first, then anything provided with `-L`, and then the 
>> sysroot. Here you have a dylib in the toolchain root (assuming that's what 
>> `jenkins/workspace/lldb-cmake/lldb-build` is), so it's using that. It seems 
>> that the library built in the toolchain root is built for x68_64 (probably 
>> your host), whereas you'd need it to be built for the target (i386 
>> simulator).
>
> Correct, that's exactly what's happening here. Since this bot is only running 
> `check-debuginfo` and `check-lldb`, would it make sense to stop building 
> libcxx altogether (that is drop `libcxx;libcxxabi` from 
> `LLVM_ENABLE_PROJECTS`)? That way the compiler should pick up the one inside 
> the sysroot since that's the only one available.

No, the bot is also meant to catch changes to libc++ breaking LLDB (or at least 
making sure we update the corresponding data formatters). I don't think that 
really matters for the simulator tests though. So if the toolchain takes 
precedence over `-L` too, how can I use the just-built compiler and make sure 
we continue to use the libc++ from the SDK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

@phosek I've reverted this in 05eeed9691aeb3e0316712195b998e9078cdceb0 
 to turn 
the bot green again. Happy to help you look into this tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This breaks `TestAppleSimulatorOSType.py ` on GreenDragon. First failed build: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31346/

  /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang  main.o 
-g -O0 -fno-builtin -isysroot 
"/Applications/Xcode.app/Contents/Developer/Platforms/WatchSimulator.platform/Developer/SDKs/WatchSimulator6.2.sdk"
 -arch i386  
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/tools/lldb-server
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make
 -include 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h
  -fno-limit-debug-info -target i386-apple-watchos6.2-simulator 
-mwatchos-simulator-version-min=6.2 -D__STDC_LIMIT_MACROS 
-D__STDC_FORMAT_MACROS   --driver-mode=g++ -o "test_simulator_platform_watchos"
  ld: warning: ignoring file 
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lib/libc++.dylib, 
building for watchOS Simulator-i386 but attempting to link with file built for 
macOS-x86_64

Based on your description above, shouldn't it prefer the libc++ form the 
sysroot?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45639

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

[PATCH] D100086: Include `llvm-config` and `not` in AppleClang toolchains.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D100086/new/

https://reviews.llvm.org/D100086

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D95119#2584709 , @haampie wrote:

> Should this be merged?

Do you have commit access? If not I can land this for you.


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

https://reviews.llvm.org/D95119

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

[PATCH] D96363: Mark output as text if it is really text

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks. LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96363

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

[PATCH] D96363: Mark output as text if it is really text

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM with two inline nits.




Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:188
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(true, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
   if (!OS) return;





Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:273
 OutputStream =
-CI.createDefaultOutputFile(true, getCurrentFileOrBufferName());
+CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
 if (!OutputStream)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96363

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

[PATCH] D96427: Support multi-configuration generators correctly in several config files

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM. I'm surprised the `dsymutil` one slipped through the cracks, we have a 
bot that should (?) be testing this configuration: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96427

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

[PATCH] D96049: [Timer] On macOS count number of executed instructions

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Seems useful! LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96049

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Thanks for brining this up on the mailing list, I think that's a good place to 
discuss!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Jonas Devlieghere via Phabricator via cfe-commits]

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

In D95635#2529027 , @ctetreau wrote:

> In D95635#2528851 , @JDevlieghere 
> wrote:
>
>>> However, the project claims to require 3.6 or greater, and 3.6 features are 
>>> being used.
>>
>> Can you link to where it says that? I'm not opposed to making 3.6 the 
>> minimally required version, but at least for LLDB we don't have that 
>> requirement and we have documentation mentioning Python 3.5.
>
>
>
> In D95635#2528883 , @yln wrote:
>
>> In D95635#2528851 , @JDevlieghere 
>> wrote:
>>
 However, the project claims to require 3.6 or greater, and 3.6 features 
 are being used.
>>>
>>> Can you link to where it says that? I'm not opposed to making 3.6 the 
>>> minimally required version, but at least for LLDB we don't have that 
>>> requirement and we have documentation mentioning Python 3.5.
>>
>> For LLVM: https://llvm.org/docs/GettingStarted.html#software
>
> This is where I saw it. I got bit by @yln's cleanup patch. I can reduce the 
> minimum version for LLDB if that's the officially required version, but llvm 
> at the very least advertises 3.6

That page says "known to work" which is not exactly the same a minimally 
supported. In a unified build there's also no way to say LLVM requires Python 
3.6 but LLDB only requires Python 3.5 because we use the same variables and we 
need to keep that in sync as to not run the test suite under one Python version 
while linking against another.

I see the patch got reverted because not all the bots have Python 3.6. What's 
the lowest Python version that they're running? Is there a reason 3.6 should be 
the minimally supported version?

Again, I'm not opposed to this, but it warrants a bit more discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM, unless @MaskRay has any other concerns.


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

https://reviews.llvm.org/D95119

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

[PATCH] D95635: [CMake] Actually require python 3.6 or greater

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

> However, the project claims to require 3.6 or greater, and 3.6 features are 
> being used.

Can you link to where it says that? I'm not opposed to making 3.6 minimally 
required version, but at least for LLDB we don't have that requirement and we 
have documentation mentioning Python 3.5.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95635

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Oops, didn't mean to accept.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95119

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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

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

In D95119#2525925 , @MaskRay wrote:

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

Not sure if there's a similar thing for `awk`, but I think `use warnings` is 
preferred nowadays over `perl -w`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95119

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

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Jonas Devlieghere via Phabricator via cfe-commits]

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

In D94844#2524854 , @nathawes wrote:

> @JDevlieghere and @dexonsmith would you mind taking another look?
>
> I ended up changing the 'lookup' functions to drop the ExternalRedirect out 
> param and supply that via a new 'LookupResult' return value that also gives 
> the matched Entry. Hopefully that avoids any confusion over where it's 
> computed vs used. It also served as a good home for the duplicated code and 
> the logic in the old SetExternalRedirect closure, as Jonas had mentioned. 
> While adding the extra directory iteration tests Duncan suggested I noticed I 
> wasn't respecting the 'use-external-names' setting in the return items, so 
> I've also added a new directory iterator implementation 
> (RedirectingFSDirRemapIterImpl). That one's used to wrap the iterator the 
> external file system gives for the directory being mapped to, and updates the 
> paths in the items it returns to refer to the virtual directory's path 
> instead, before passing them along.

I like the new approach, it adds an indirection but nicely abstracts how we get 
the `ExternalRedirect`. I've left an inline comment about a copy I think we can 
avoid, but other than that this LGTM.




Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:748
+  bool shouldFallBackToExternalFS(std::error_code EC,
+  Optional Result = None) const;
 

While I prefer `Optional` over a pointer, this would now incur a copy, so I 
think it's better to pass a `LookupResult*` here, or maybe even a 
`RemapEntry*`. 


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

https://reviews.llvm.org/D94844

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

[PATCH] D95279: Support: Remove duplicated code in {File,clang::ModulesDependency}Collector, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM, thanks for cleaning this up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95279

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

[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Overall this looks good. I wonder if abstracting the ExternalRedirect as a 
small wrapper class around a SmallString would help. There's a few operations 
that are repeated, like the example below, and it could also house the logic 
that's currently in the lambda.

  if (ExternalRedirect.empty()) {
assert(RE->getKind() == RedirectingFileSystem::EK_File);
ExternalRedirect = RE->getExternalContentsPath();
  }

One thing I found tricky during the review was differentiating between places 
where the ExternalRedirect was computed and uses. Looking at the signatures the 
`lookupPath` functions are computing it and `status` is consuming it, although 
the latter then changes the StringRef (which I guess shouldn't persist?). I 
don't know if the wrapper could alleviate this as well.




Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:564
 ///
+/// Re-mapped directories are represented as
+/// /// \verbatim

I think we should expand on this a bit more to contrast a directory-remapped 
directory vs a file-remapped directory (for a lack of better term). 
Specifically, an empty file-remapped directory could be confusing, as it 
behaves quite differently but looks pretty similar in YAML. 

```
{
  'type': 'file',
  'name': ,
  'external-contents': 
}
```

```
{
  'type': 'directory',
  'name': ,
  'contents': []
}
```

Looking at the two, I wonder if there would be any benefit to changing the 
`type` in the yaml to match the `NameKind`.






Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:624
+
+/// whether to use the external path as the name for this file or 
directory.
+bool useExternalName(bool GlobalUseExternalName) const {

nit: s/whether/Whether/



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:776-778
   /// Looks up \p Path in \c Roots.
-  ErrorOr lookupPath(const Twine ) const;
+  ErrorOr lookupPath(const Twine ,
+  SmallVectorImpl ) const;

dexonsmith wrote:
> It's not entirely clear what the extra parameters do / when they're filled in 
> / etc; can you add a bit more documentation?
> 
> Also, I see that the public caller ignores the `ExternalRedirect` parameter. 
> I wonder if this should be renamed to `lookupPathWithName` or 
> `lookupPathImpl`, leaving behind a `lookupPath` wrapper that creates the 
> ignored buffer for the public interface... no strong opinion though.
+1.


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

https://reviews.llvm.org/D94844

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

[PATCH] D94374: [CMake] Remove dead code setting policies to NEW

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

LGTM for LLDB


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94374

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

[PATCH] D91317: Support: Add RedirectingFileSystem::create from simple list of redirections

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM with the other patches. Thanks!


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

https://reviews.llvm.org/D91317

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

[PATCH] D92888: ADT: Allow IntrusiveRefCntPtr construction from std::unique_ptr, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92888

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

[PATCH] D91317: Support: Add RedirectingFileSystem::create from simple list of redirections

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:735
+  /// Redirect each of the remapped files from first to second.
+  static std::unique_ptr
+  create(ArrayRef> RemappedFiles,

 - Do you need to know that this is a `RedirectingFileSystem` or can it be a 
`FileSystem`? 
 - Is there a reason this should be a `unique_ptr` instead of an 
`IntrusiveRefCntPtr` (which is more prevalent across the VFS, although I'm not 
sure why)


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

https://reviews.llvm.org/D91317

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

[PATCH] D92597: ARCMigrate: Initialize fields in EditEntry inline, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92597

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

[PATCH] D90890: Frontend: Change ComputePreambleBounds to take MemoryBufferRef, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D90890

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

[PATCH] D89836: Change Module::ASTFile and ModuleFile::File => Optional, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D89836

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

[PATCH] D89835: ModuleManager: Simplify lookupModuleFile by only setting the out parameter once, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D89835

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

[PATCH] D89913: SourceManager: Encapsulate line number mapping into SrcMgr::LineOffsetMapping

[Jonas Devlieghere via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/SourceManager.h:118
+  private:
+unsigned *Storage = nullptr;
+  };

I guess it's implicit in the implementation, but maybe it's worth adding a 
comment describing the layout (first element is the size, elements at index i 
are at index i+1. 


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

https://reviews.llvm.org/D89913

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

[PATCH] D89580: SourceManager: Fix an SLocEntry memory regression introduced with FileEntryRef

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:293
   X.ContentAndKind.setInt(FileCharacter);
-  X.Filename = Filename;
+  const_cast(Con).Filename = Filename;
   return X;

Would it possibly make more sense to drop the `const` qualifier from the 
argument? 


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

https://reviews.llvm.org/D89580

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

[PATCH] D89554: SourceManager: Clarify that FileInfo always has a ContentCache, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM if Shafik is happy


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

https://reviews.llvm.org/D89554

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

[PATCH] D89763: [Apple-stage2] Install FileCheck and yaml2obj in the toolchain

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG27a909a24f99: [Apple-stage2] Install FileCheck and yaml2obj 
in the toolchain (authored by JDevlieghere).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D89763?vs=299370=299460#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89763

Files:
  clang/cmake/caches/Apple-stage2.cmake


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -58,6 +58,13 @@
   llvm-size
   CACHE STRING "")
 
+set(LLVM_BUILD_UTILS ON CACHE BOOL "")
+set(LLVM_INSTALL_UTILS ON CACHE BOOL "")
+set(LLVM_TOOLCHAIN_UTILITIES
+  FileCheck
+  yaml2obj
+  CACHE STRING "")
+
 set(LLVM_DISTRIBUTION_COMPONENTS
   clang
   LTO
@@ -66,6 +73,7 @@
   cxx-headers
   Remarks
   ${LLVM_TOOLCHAIN_TOOLS}
+  ${LLVM_TOOLCHAIN_UTILITIES}
   CACHE STRING "")
 
 # test args


Index: clang/cmake/caches/Apple-stage2.cmake
===
--- clang/cmake/caches/Apple-stage2.cmake
+++ clang/cmake/caches/Apple-stage2.cmake
@@ -58,6 +58,13 @@
   llvm-size
   CACHE STRING "")
 
+set(LLVM_BUILD_UTILS ON CACHE BOOL "")
+set(LLVM_INSTALL_UTILS ON CACHE BOOL "")
+set(LLVM_TOOLCHAIN_UTILITIES
+  FileCheck
+  yaml2obj
+  CACHE STRING "")
+
 set(LLVM_DISTRIBUTION_COMPONENTS
   clang
   LTO
@@ -66,6 +73,7 @@
   cxx-headers
   Remarks
   ${LLVM_TOOLCHAIN_TOOLS}
+  ${LLVM_TOOLCHAIN_UTILITIES}
   CACHE STRING "")
 
 # test args
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89429: clang/Basic: Replace SourceManager::getMemoryBufferForFile, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks for the explanation!


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

https://reviews.llvm.org/D89429

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

[PATCH] D89761: Split out llvm/Support/FileSystem/UniqueID.h and clang/Basic/FileEntry.h, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Basic/FileEntry.h:33
+
+using llvm::Optional;
+using llvm::StringRef;

Won't this now make `llvm::Optional` visible as `clang::Optional` everywhere 
this header is included? Isn't this considered bad practice in a header? 


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

https://reviews.llvm.org/D89761

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

[PATCH] D89398: Lexer: Update the Lexer to use MemoryBufferRef, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/include/clang/Lex/Lexer.h:145
   /// outlive it, so it doesn't take ownership of either of them.
-  Lexer(FileID FID, const llvm::MemoryBuffer *InputFile, Preprocessor );
+  Lexer(FileID FID, const llvm::MemoryBufferRef , Preprocessor );
 

dexonsmith wrote:
> dexonsmith wrote:
> > JDevlieghere wrote:
> > > Should `MemoryBufferRef`s be passed by value like `StringRef`s?
> > I don't think it absolutely needs to be, but it could, it's just two 
> > `StringRef`s. Passing by reference here prevents adding an include 
> > unnecessarily.
> That said, I don't feel strongly, happy to change to by-value if you think 
> that's better.
Thanks! I was mostly asking from a "policy" perspective, but avoiding an 
include seems as good of a motivation as any.


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

https://reviews.llvm.org/D89398

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

[PATCH] D89430: clang/Basic: Remove SourceManager::getBufferPointer, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Thanks!


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

https://reviews.llvm.org/D89430

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

[PATCH] D89398: Lexer: Update the Lexer to use MemoryBufferRef, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Lex/Lexer.h:145
   /// outlive it, so it doesn't take ownership of either of them.
-  Lexer(FileID FID, const llvm::MemoryBuffer *InputFile, Preprocessor );
+  Lexer(FileID FID, const llvm::MemoryBufferRef , Preprocessor );
 

Should `MemoryBufferRef`s be passed by value like `StringRef`s?


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

https://reviews.llvm.org/D89398

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

[PATCH] D89430: clang/Basic: Remove SourceManager::getBufferPointer, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/Basic/SourceManager.cpp:167
 
 Buffer.setInt(Buffer.getInt() | InvalidFlag);
+return None;

Orthogonal to this patch but an idea for a follow-up: this seems lightly error 
prone and repetitive. We could put it in an `llvm::scope_exit` and then release 
it before the final return on line `200`.


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

https://reviews.llvm.org/D89430

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

[PATCH] D89429: clang/Basic: Replace SourceManager::getMemoryBufferForFile, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:705
+llvm::Optional
+SourceManager::getMemoryBufferForFileOrNone(const FileEntry *File) {
   const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);

Would it make sense to rename this to the slightly less verbose 
`getMemoryBufferOrNone` now that it takes just the File as an argument? Or 
maybe even `getBufferOrNone` like the method in `ContentCache`. 


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

https://reviews.llvm.org/D89429

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

[PATCH] D88497: [objc] Fix memory leak in CGObjCMac.cpp

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Thanks for updating the comment in dsymutil!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88497

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

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87243

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

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D87243#2261687 , @kastiglione wrote:

> If an LLVM install disabled `LLVM_ENABLE_WARNINGS`, should other builds 
> inherit that? I would think no, but is there a precedent for that that to be 
> the case?

Yes, most of the `LLVM_ENABLE_*` options work that way. I guess you could argue 
that some of those are different because LLVM is the one "detecting" whether 
they should be on, such as `LLVM_ENABLE_ZLIB`. But 
`LLVM_ENABLE_EXPENSIVE_CHECKS` and `LLVM_ENABLE_ASSERTIONS` seem conceptually 
very similar to `LLVM_ENABLE_WARNINGS` so (in my opinion) it should behave the 
same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87243

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

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

You need to add `LLVM_ENABLE_WARNINGS` to `LLVMConfig.cmake.in` so that the 
standalone builds know what value was set in the LLVM build. I think with the 
current patch the other projects won't inherit the value and just default to 
`ON`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87243

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

[PATCH] D85231: Protect against filenames with no extension at all.

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

Can you add a test that exercises this code path?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85231

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

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D84572/new/

https://reviews.llvm.org/D84572

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

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/include/clang/Driver/Options.td:693
 def e : JoinedOrSeparate<["-"], "e">, Group;
+def external_dsym_dir : JoinedOrSeparate<["-"], "external-dsym-dir">,
+  Flags<[DriverOption, RenderAsInput]>,

Could we drop the `external` prefix or did you pick this for consistency with 
other flags? It seems rather specific for the externalize debug info scenario 
while the functionality seems generic enough by itself. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84572



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

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[Jonas Devlieghere via Phabricator via cfe-commits]

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

When you don’t pass any specific options to the linker, it’s going to generate 
a temporary `lto.o` file in `/tmp` and delete it after the link. When 
`dsymutil` will try to read that, it won't be there anymore. Unless I'm missing 
I don't think this is going to work. If it turns out I'm mistaken please add 
that situation as a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84565



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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D83454#2142719 , @michele.scandale 
wrote:

> I tested locally the standalone build of Clang with D83426 
> .
>  I just want to make sure that we all agree this is right way moving forward.


Yep, with the target exported this is the right thing to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454



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

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

LGTM. I verified this works for the build-tree standalone build of LLDB, but 
please still keep an eye on 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/ after you 
land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83454



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks Petr, LGTM


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

https://reviews.llvm.org/D79219



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

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa1f4e48c4aca: [clang][docs] Add note about using `-flto` 
with `-g` on macOS (authored by phil-blain, committed by JDevlieghere).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82733

Files:
  clang/docs/CommandGuide/clang.rst


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -474,6 +474,16 @@
   optimization. With "thin", :doc:`ThinLTO <../ThinLTO>`
   compilation is invoked instead.
 
+  .. note::
+
+ On Darwin, when using :option:`-flto` along with :option:`-g` and
+ compiling and linking in separate steps, you also need to pass
+ ``-Wl,-object_path_lto,.o`` at the linking step to instruct 
the
+ ld64 linker not to delete the temporary object file generated during Link
+ Time Optimization (this flag is automatically passed to the linker by 
Clang
+ if compilation and linking are done in a single step). This allows 
debugging
+ the executable as well as generating the ``.dSYM`` bundle using 
:manpage:`dsymutil(1)`.
+
 Driver Options
 ~~
 


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -474,6 +474,16 @@
   optimization. With "thin", :doc:`ThinLTO <../ThinLTO>`
   compilation is invoked instead.
 
+  .. note::
+
+ On Darwin, when using :option:`-flto` along with :option:`-g` and
+ compiling and linking in separate steps, you also need to pass
+ ``-Wl,-object_path_lto,.o`` at the linking step to instruct the
+ ld64 linker not to delete the temporary object file generated during Link
+ Time Optimization (this flag is automatically passed to the linker by Clang
+ if compilation and linking are done in a single step). This allows debugging
+ the executable as well as generating the ``.dSYM`` bundle using :manpage:`dsymutil(1)`.
+
 Driver Options
 ~~
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM. Thank you for adding this!

In D82733#2121414 , @phil-blain wrote:

> I tested the sphinx build for the `man` and `html` targets. Any other targets 
> I should check ?


No, that sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82733



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

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/docs/CommandGuide/clang.rst:477
 
+  Note: on Darwin, when using :option:`-flto` along with :option:`-g` and
+  compiling and linking in separate steps, you also need to pass

Any reason not to use the sphinx note directive (`.. note::` )?

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#paragraph-level-markup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82733



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

[PATCH] D80770: [diagtool] Install diagtool when LLVM_INSTALL_TOOLCHAIN_ONLY is ON.

[Jonas Devlieghere via Phabricator via cfe-commits]

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

Thanks, Volodymyr!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80770



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: llvm/cmake/config-ix.cmake:514
+  if(ZLIB_FOUND)
+set(LLVM_ENABLE_ZLIB "YES" CACHE STRING
+  "Use zlib for compression/decompression if available. Can be ON, OFF, or 
FORCE_ON"

phosek wrote:
> JDevlieghere wrote:
> > If I understand correctly, after running the configuration phase with 
> > LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 
> Correct, we used `FORCE_ON` above when invoking `find_package` setting 
> `REQUIRED` which makes the check fail if the library is missing. Recording 
> this information is important for LLVM consumers because it'll be stored in 
> `LLVMConfig.cmake` and AFAIU we don't want to propagate `FORCE_ON` there.
I get that. My worry is that if for whatever reason the library disappears 
(system upgrade, package removal, ...) this will silently disable ZLIB support 
because now LLVM_ENABLE_ZLIB just equals on. This might sound far fetched, but 
happens all the time with "internal installs". This is why I prefer the 
ON/OFF/Auto approach because it doesn't update the cache variable, but would 
set the internal variable to either ON or OFF.


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

https://reviews.llvm.org/D79219



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

I'm in favor of this change. I'm not too happy with how this works in CMake, 
I've expressed similar concerns when the FORCE_ON approach was suggested in 
D71306 .

I really like what we ended up with in LLDB. The TL;DR is that we have 3 
values: ON, OFF and Auto. The later will resolve to either ON or OFF depending 
on whether the dependency was found. The "clever" part is that we use shadowing 
to avoid having another CMake variable without overwriting the cache.

Here's what it looks like in LLDB: 
https://github.com/llvm/llvm-project/blob/3df40007e6317955176dff0fa9e0b029dc4a11d1/lldb/cmake/modules/LLDBConfig.cmake#L26

Maybe we can consider something similar here? I'd be more than happy to hoist 
the corresponding CMake into llvm.

(edit: added myself as a reviewer so this shows up in my review queue)




Comment at: llvm/cmake/config-ix.cmake:514
+  if(ZLIB_FOUND)
+set(LLVM_ENABLE_ZLIB "YES" CACHE STRING
+  "Use zlib for compression/decompression if available. Can be ON, OFF, or 
FORCE_ON"

If I understand correctly, after running the configuration phase with 
LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219



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

[PATCH] D78762: build: use `find_package(Python3)` if available

[Jonas Devlieghere via Phabricator via cfe-commits]

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

I talked to Saleem and he cleared up some of my concerns. Given that the 
community seems to have agreed to support only Python 3, this change seems a 
lot more reasonable. My earlier comment was made under the impression that we 
were going to continue supporting Python 2. With that in mind, the burden 
should fall on LLDB if we want to continue supporting it. Python 2 will be a 
"special case" then, rather than another first class citizen. With all that 
said, this LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78762



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

[PATCH] D78762: build: use `find_package(Python3)` if available

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D78762#2002390 , @compnerd wrote:

> @JDevlieghere I think that adding another mechanism for finding the python 
> executable is not the right approach.  You already have the variables that 
> you are talking about, you just need to specify it in triplicate if you want 
> compatibility across all the versions, but specifying `-DPython3_EXECUTABLE=` 
> gives you the control over the executable for python3.  If you want to use 
> python2, `-DPython3_EXECUTABLE=` and `-DPython2_EXECUTABLE=` will ensure that 
> python2 is always used.  If you are using an older CMake, specifying 
> `-DPython3_EXECUTABLE=`, `-DPython2_EXECUTABLE=` and `-DPYTHON_EXECUTABLE=` 
> will ensure that the specified version is used.  I'm not sure what the 
> scenario is that you believe will be made more difficult with this approach.


It only //works// because you're setting Python3_EXECUTABLE to 
Python2_EXECUTABLE.

  set(Python3_EXECUTABLE ${Python2_EXECUTABLE})

This will be completely opaque to lldb and we'll have to struggle again to 
reverse engineer which Python is used, leaving us in the same situation as 
today. How are we going to choose between Python 2 and Python 3 based on that 
variable? What I envision is to find Python once, like we do with other 
dependencies in the LLVM project, and then use that (interpreter, libraries, 
include paths) in LLDB. This has to work for in-tree-builds as well as 
out-of-tree builds (I'm not a fan but hey they're still supported).

In D78762#2005407 , @ldionne wrote:

> In D78762#2002305 , @JDevlieghere 
> wrote:
>
> > My suggestion is to have 4 new CMake variable that abstract over this: 
> > `LLVM_PYTHON_EXECUTABLE`, `LLVM_PYTHON_LIBRARIES`, 
> > `LLVM_PYTHON_INCLUDE_DIRS` and finally `LLVM_PYTHON_HINT`.  We can then 
> > populate the first three depending on what machinery we want to use, i.e. 
> > the old CMake way of finding Python (until we bump the requirement across 
> > llvm), as well as the new `FindPython3` and `FindPython2`. Similarly for 
> > the `HINT` variable, having our own abstraction means we don't clobber any 
> > of the variables used internally by CMake.
> >
> > What do you think?
>
>
> This is not better IMHO since it assumes that all subprojects are using the 
> `LLVM_meow` variable to refer to the Python executable.


This patch is already changing the variable. How is this different/worse from 
using `Python3_EXECUTABLE` and having it **maybe** point to a Python 2 
interpreter?




Comment at: clang/CMakeLists.txt:154
+message(WARNING "Python3 not found, using python2 as a fallback")
+find_package(Python3 COMPONENTS Interpreter REQUIRED)
+if(Python2_VERSION VERSION_LESS 2.7)

Python2?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78762



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

[PATCH] D78762: build: use `find_package(Python3)` if available

[Jonas Devlieghere via Phabricator via cfe-commits]

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

I would strongly prefer to do this differently. While we hope to drop Python 2 
support in LLDB as soon as possible, we are not there yet. This patch as it 
stands will make it really hard for us to keep doing so (even internally) if 
upstream decides to drop support. I have looked into this change myself a while 
ago with these limitations in mind, and believe we can achieve the same in a 
way that would make this easier.

My suggestion is to have 4 new CMake variable that abstract over this: 
`LLVM_PYTHON_EXECUTABLE`, `LLVM_PYTHON_LIBRARIES`, `LLVM_PYTHON_INCLUDE_DIRS` 
and finally `LLVM_PYTHON_HINT`.  We can then populate the first three depending 
on what machinery we want to use, i.e. the old CMake way of finding Python 
(until we bump the requirement across llvm), as well as the new `FindPython3` 
and `FindPython2`. Similarly for the `HINT` variable, having our own 
abstraction means we don't clobber any of the variables used internally by 
CMake.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78762



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

[PATCH] D78807: Fix gendered documentation

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78807



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

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

In D75360#1937556 , @Szelethus wrote:

> In D75360#1937415 , @JDevlieghere 
> wrote:
>
> > It appears this broke the modules build: 
> > http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12905/console
> >
> > Can you please take a look?
>
>
> Yup, on it.


Oh, I'm sorry, I just reverted it like a minute ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75360



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