[Lldb-commits] [PATCH] D54731: [lit] Enable the use of custom user-defined lit commands

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I think the code to implement this is fine, but before we add this complexity 
to lit, I just wanted to know if other folks who work on the LLDB test suite 
are on board and want to use this approach to abstract over building apps for 
different targets. I see @stella.stamenova is, but I wanted to hear from other 
people involved in the LLDB lit test suite stuff. The `%compile %debug %opt %s` 
 lit substitution approach is limiting, but do people feel strongly that this 
is much better?




Comment at: 
llvm/utils/lit/tests/Inputs/shtest-keyword-command/keyword_helper.py:2
+
+def customCommand(line):
+  return ('STDOUT: ' + line, 'STDERR: ' + line)

Oh, the joys of multiprocessing and pickling...


https://reviews.llvm.org/D54731



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

[Lldb-commits] [PATCH] D34236: Delete ProcessLauncherPosix

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

`posix_spawn` is considered the preferred, efficient, canonical way to launch 
processes on Mac. Are you sure you want to remove support for it?


https://reviews.llvm.org/D34236



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

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 193191.
rnk marked 2 inline comments as done.
rnk added a comment.
Herald added subscribers: lldb-commits, cfe-commits, kadircet, arphaman, 
jkorous.
Herald added projects: clang, LLDB.

- fix one lldb usage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018

Files:
  clang-tools-extra/unittests/clangd/JSONTransportTests.cpp
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  llvm/include/llvm/DebugInfo/CodeView/CVRecord.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h
  llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h
  llvm/lib/DebugInfo/CodeView/AppendingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp
  llvm/lib/DebugInfo/CodeView/CVTypeVisitor.cpp
  llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp
  llvm/lib/DebugInfo/CodeView/GlobalTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/MergingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp
  llvm/lib/DebugInfo/CodeView/SymbolDumper.cpp
  llvm/lib/DebugInfo/CodeView/TypeDumpVisitor.cpp
  llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
  llvm/lib/DebugInfo/CodeView/TypeTableCollection.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
  llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
  llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp

Index: llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
===
--- llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
+++ llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
@@ -42,8 +42,6 @@
 }
 
 inline bool operator==(const CVType &R1, const CVType &R2) {
-  if (R1.Type != R2.Type)
-return false;
   if (R1.RecordData != R2.RecordData)
 return false;
   return true;
@@ -107,7 +105,7 @@
   GlobalState->Records.push_back(AR);
   GlobalState->Indices.push_back(Builder.writeLeafType(AR));
 
-  CVType Type(TypeLeafKind::LF_ARRAY, Builder.records().back());
+  CVType Type(Builder.records().back());
   GlobalState->TypeVector.push_back(Type);
 
   GlobalState->AllOffsets.push_back(
@@ -369,11 +367,10 @@
   TypeIndex IndexOne = Builder.writeLeafType(Modifier);
 
   // set up a type stream that refers to the above two serialized records.
-  std::vector TypeArray;
-  TypeArray.push_back(
-  CVType(static_cast(Class.Kind), Builder.records()[0]));
-  TypeArray.push_back(
-  CVType(static_cast(Modifier.Kind), Builder.records()[1]));
+  std::vector TypeArray = {
+  {Builder.records()[0]},
+  {Builder.records()[1]},
+  };
   BinaryItemStream ItemStream(llvm::support::little);
   ItemStream.setItems(TypeArray);
   VarStreamArray TypeStream(ItemStream);
Index: llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
@@ -224,7 +224,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}",
fmt_align(Index, AlignStyle::Right, Width),
-   formatTypeLeafKind(Record.Type), Record.length());
+   formatTypeLeafKind(Record.kind()), Record.length());
   if (Hashes) {
 std::string H;
 if (Index.toArrayIndex() >= HashValues.size()) {
Index: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
@@ -331,7 +331,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}]",
fmt_align(Offset, AlignStyle::Right, 6),
-   formatSymbolKind(Record.Type), Record.length());
+   formatSymbolKind(Record.kind()), Record.length());
   P.Indent();
   return Error::success();
 }
Index: llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
@@ -98,7 +98,7 @@
 
   CVType toCodeViewRecord(AppendingTypeTableBuilder &TS) const override {
 TS.writeLeafType(Record);
-return CVType(Kind, TS.records().back());
+return CVType(TS.records().back());
   }
 
   mutable T Record;
@@ -496,7 +496,7 @@
 Member.Member->writeTo(CRB);
   }
   TS.insertRecord(CRB);
-  return CVType(Kind, TS.records().back());
+  return CVType(TS.records().back());
 }
 
 void MappingTraits::mapping(IO &io, OneMethodRecord &Record) {
Index: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
===
--- llvm/lib/ObjectYAML/CodeVi

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 193388.
rnk marked 4 inline comments as done.
rnk added a comment.

- Add RecordPrefix ctor, remove all dummy RecordLen assignments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018

Files:
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  llvm/include/llvm/DebugInfo/CodeView/CVRecord.h
  llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h
  llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h
  llvm/lib/DebugInfo/CodeView/AppendingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp
  llvm/lib/DebugInfo/CodeView/CVTypeVisitor.cpp
  llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp
  llvm/lib/DebugInfo/CodeView/GlobalTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/MergingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp
  llvm/lib/DebugInfo/CodeView/SymbolDumper.cpp
  llvm/lib/DebugInfo/CodeView/TypeDumpVisitor.cpp
  llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
  llvm/lib/DebugInfo/CodeView/TypeTableCollection.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
  llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
  llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp

Index: llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
===
--- llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
+++ llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
@@ -42,8 +42,6 @@
 }
 
 inline bool operator==(const CVType &R1, const CVType &R2) {
-  if (R1.Type != R2.Type)
-return false;
   if (R1.RecordData != R2.RecordData)
 return false;
   return true;
@@ -107,7 +105,7 @@
   GlobalState->Records.push_back(AR);
   GlobalState->Indices.push_back(Builder.writeLeafType(AR));
 
-  CVType Type(TypeLeafKind::LF_ARRAY, Builder.records().back());
+  CVType Type(Builder.records().back());
   GlobalState->TypeVector.push_back(Type);
 
   GlobalState->AllOffsets.push_back(
@@ -369,11 +367,10 @@
   TypeIndex IndexOne = Builder.writeLeafType(Modifier);
 
   // set up a type stream that refers to the above two serialized records.
-  std::vector TypeArray;
-  TypeArray.push_back(
-  CVType(static_cast(Class.Kind), Builder.records()[0]));
-  TypeArray.push_back(
-  CVType(static_cast(Modifier.Kind), Builder.records()[1]));
+  std::vector TypeArray = {
+  {Builder.records()[0]},
+  {Builder.records()[1]},
+  };
   BinaryItemStream ItemStream(llvm::support::little);
   ItemStream.setItems(TypeArray);
   VarStreamArray TypeStream(ItemStream);
Index: llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
@@ -224,7 +224,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}",
fmt_align(Index, AlignStyle::Right, Width),
-   formatTypeLeafKind(Record.Type), Record.length());
+   formatTypeLeafKind(Record.kind()), Record.length());
   if (Hashes) {
 std::string H;
 if (Index.toArrayIndex() >= HashValues.size()) {
Index: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
@@ -331,7 +331,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}]",
fmt_align(Offset, AlignStyle::Right, 6),
-   formatSymbolKind(Record.Type), Record.length());
+   formatSymbolKind(Record.kind()), Record.length());
   P.Indent();
   return Error::success();
 }
Index: llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
@@ -98,7 +98,7 @@
 
   CVType toCodeViewRecord(AppendingTypeTableBuilder &TS) const override {
 TS.writeLeafType(Record);
-return CVType(Kind, TS.records().back());
+return CVType(TS.records().back());
   }
 
   mutable T Record;
@@ -496,7 +496,7 @@
 Member.Member->writeTo(CRB);
   }
   TS.insertRecord(CRB);
-  return CVType(Kind, TS.records().back());
+  return CVType(TS.records().back());
 }
 
 void MappingTraits::mapping(IO &io, OneMethodRecord &Record) {
Index: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
@@ -248,7

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 193390.
rnk marked an inline comment as done.
rnk added a comment.

- one more change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018

Files:
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  llvm/include/llvm/DebugInfo/CodeView/CVRecord.h
  llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h
  llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h
  llvm/lib/DebugInfo/CodeView/AppendingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp
  llvm/lib/DebugInfo/CodeView/CVTypeVisitor.cpp
  llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp
  llvm/lib/DebugInfo/CodeView/GlobalTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/MergingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp
  llvm/lib/DebugInfo/CodeView/SymbolDumper.cpp
  llvm/lib/DebugInfo/CodeView/TypeDumpVisitor.cpp
  llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
  llvm/lib/DebugInfo/CodeView/TypeTableCollection.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
  llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
  llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp

Index: llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
===
--- llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
+++ llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
@@ -42,8 +42,6 @@
 }
 
 inline bool operator==(const CVType &R1, const CVType &R2) {
-  if (R1.Type != R2.Type)
-return false;
   if (R1.RecordData != R2.RecordData)
 return false;
   return true;
@@ -107,7 +105,7 @@
   GlobalState->Records.push_back(AR);
   GlobalState->Indices.push_back(Builder.writeLeafType(AR));
 
-  CVType Type(TypeLeafKind::LF_ARRAY, Builder.records().back());
+  CVType Type(Builder.records().back());
   GlobalState->TypeVector.push_back(Type);
 
   GlobalState->AllOffsets.push_back(
@@ -369,11 +367,10 @@
   TypeIndex IndexOne = Builder.writeLeafType(Modifier);
 
   // set up a type stream that refers to the above two serialized records.
-  std::vector TypeArray;
-  TypeArray.push_back(
-  CVType(static_cast(Class.Kind), Builder.records()[0]));
-  TypeArray.push_back(
-  CVType(static_cast(Modifier.Kind), Builder.records()[1]));
+  std::vector TypeArray = {
+  {Builder.records()[0]},
+  {Builder.records()[1]},
+  };
   BinaryItemStream ItemStream(llvm::support::little);
   ItemStream.setItems(TypeArray);
   VarStreamArray TypeStream(ItemStream);
Index: llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
@@ -224,7 +224,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}",
fmt_align(Index, AlignStyle::Right, Width),
-   formatTypeLeafKind(Record.Type), Record.length());
+   formatTypeLeafKind(Record.kind()), Record.length());
   if (Hashes) {
 std::string H;
 if (Index.toArrayIndex() >= HashValues.size()) {
Index: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
@@ -331,7 +331,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}]",
fmt_align(Offset, AlignStyle::Right, 6),
-   formatSymbolKind(Record.Type), Record.length());
+   formatSymbolKind(Record.kind()), Record.length());
   P.Indent();
   return Error::success();
 }
Index: llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
@@ -98,7 +98,7 @@
 
   CVType toCodeViewRecord(AppendingTypeTableBuilder &TS) const override {
 TS.writeLeafType(Record);
-return CVType(Kind, TS.records().back());
+return CVType(TS.records().back());
   }
 
   mutable T Record;
@@ -496,7 +496,7 @@
 Member.Member->writeTo(CRB);
   }
   TS.insertRecord(CRB);
-  return CVType(Kind, TS.records().back());
+  return CVType(TS.records().back());
 }
 
 void MappingTraits::mapping(IO &io, OneMethodRecord &Record) {
Index: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
@@ -248,7 +248,7 @@
 uint8_t *Buffer = Allocator.Al

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template  class CVRecord {

aganea wrote:
> aganea wrote:
> > To add to what you said in a comment above, do you think that if we could 
> > add `assert(Data.size() == ((RecordPrefix 
> > *)RecordData.data())->RecordPrefix + 2)` at relevant places below; and then 
> > after a while we could simply switch to `RecordPrefix *`, once issues are 
> > ironed out?
> I didn't mean now.
Eventually, yes, I think it's possible.



Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+RecordPrefix *Prefix = reinterpret_cast(&PreBytes[0]);
+Prefix->RecordLen = 0;
+Prefix->RecordKind = uint16_t(Sym.Kind);

aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > Should be 2.
> > I could say 2, but this is the seralizer, so really it just gets 
> > overwritten. Do you think I should leave it uninitialized, 2, -1, 0?
> I'd say "2" because you want as much as possible to keep data coherent in 
> time. Regardless of what comes after. I think the code should be correct 
> before being optimal. If someone looks for `RecordPrefix` and then tries to 
> understand how it works, it'd be nice if the code displays the same, correct, 
> behavior everywhere.
> 
> But then, you can argue that `RecordPrefix.RecordLen` is simply a constrait, 
> tied to the amount data that comes after. And maybe the calculation logic for 
> that `.RecordLen` field should be hidden inside the `RecordPrefix` class. In 
> that case, to avoid screwing the init order, ideally you could simply say:
> ```
> RecordPrefix Prefix{uint16_t(Sym.Kind)};
> CVSymbol Result(&Prefix);
> ```
> ...and let `RecordPrefix` (or `CVSymbol`) handle sizing?
Makes sense. I'll try to standardize on 2, and I like the logic that the data 
becomes trustworthy.



Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+  static CVSymbol Tombstone(
+  ArrayRef(DenseMapInfo::getTombstoneKey(), 1));
   return Tombstone;

aganea wrote:
> I suppose you've chosen 1 because that's a invalid record size? Comment maybe?
Actually, the reason I did this was to avoid ambiguity between the `T* begin, 
T* end` ctor and the `T* base, size_t count` ctor. If I use zero, it's 
ambiguous. Implicit null strikes again. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk marked 2 inline comments as done.
rnk added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 193526.
rnk added a comment.

- final updates


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018

Files:
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  llvm/include/llvm/DebugInfo/CodeView/CVRecord.h
  llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h
  llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h
  llvm/lib/DebugInfo/CodeView/AppendingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp
  llvm/lib/DebugInfo/CodeView/CVTypeVisitor.cpp
  llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp
  llvm/lib/DebugInfo/CodeView/GlobalTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/MergingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp
  llvm/lib/DebugInfo/CodeView/SymbolDumper.cpp
  llvm/lib/DebugInfo/CodeView/TypeDumpVisitor.cpp
  llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
  llvm/lib/DebugInfo/CodeView/TypeTableCollection.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
  llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
  llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp

Index: llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
===
--- llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
+++ llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
@@ -42,8 +42,6 @@
 }
 
 inline bool operator==(const CVType &R1, const CVType &R2) {
-  if (R1.Type != R2.Type)
-return false;
   if (R1.RecordData != R2.RecordData)
 return false;
   return true;
@@ -107,7 +105,7 @@
   GlobalState->Records.push_back(AR);
   GlobalState->Indices.push_back(Builder.writeLeafType(AR));
 
-  CVType Type(TypeLeafKind::LF_ARRAY, Builder.records().back());
+  CVType Type(Builder.records().back());
   GlobalState->TypeVector.push_back(Type);
 
   GlobalState->AllOffsets.push_back(
@@ -369,11 +367,10 @@
   TypeIndex IndexOne = Builder.writeLeafType(Modifier);
 
   // set up a type stream that refers to the above two serialized records.
-  std::vector TypeArray;
-  TypeArray.push_back(
-  CVType(static_cast(Class.Kind), Builder.records()[0]));
-  TypeArray.push_back(
-  CVType(static_cast(Modifier.Kind), Builder.records()[1]));
+  std::vector TypeArray = {
+  {Builder.records()[0]},
+  {Builder.records()[1]},
+  };
   BinaryItemStream ItemStream(llvm::support::little);
   ItemStream.setItems(TypeArray);
   VarStreamArray TypeStream(ItemStream);
Index: llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
@@ -224,7 +224,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}",
fmt_align(Index, AlignStyle::Right, Width),
-   formatTypeLeafKind(Record.Type), Record.length());
+   formatTypeLeafKind(Record.kind()), Record.length());
   if (Hashes) {
 std::string H;
 if (Index.toArrayIndex() >= HashValues.size()) {
Index: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
@@ -331,7 +331,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}]",
fmt_align(Offset, AlignStyle::Right, 6),
-   formatSymbolKind(Record.Type), Record.length());
+   formatSymbolKind(Record.kind()), Record.length());
   P.Indent();
   return Error::success();
 }
Index: llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
@@ -98,7 +98,7 @@
 
   CVType toCodeViewRecord(AppendingTypeTableBuilder &TS) const override {
 TS.writeLeafType(Record);
-return CVType(Kind, TS.records().back());
+return CVType(TS.records().back());
   }
 
   mutable T Record;
@@ -496,7 +496,7 @@
 Member.Member->writeTo(CRB);
   }
   TS.insertRecord(CRB);
-  return CVType(Kind, TS.records().back());
+  return CVType(TS.records().back());
 }
 
 void MappingTraits::mapping(IO &io, OneMethodRecord &Record) {
Index: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
@@ -248,7 +248,7 @@
 uint8_t *Buffer = Allocator.Allocate(TotalLen);
 ::memcpy(Buffer, 

[Lldb-commits] [PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLD357658: [codeview] Remove Type member from CVRecord 
(authored by rnk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60018?vs=193526&id=193633#toc

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D60018

Files:
  COFF/PDB.cpp


Index: COFF/PDB.cpp
===
--- COFF/PDB.cpp
+++ COFF/PDB.cpp
@@ -759,7 +759,7 @@
   if (Kind == SymbolKind::S_GPROC32_ID || Kind == SymbolKind::S_LPROC32_ID) {
 SmallVector Refs;
 auto Content = RecordData.drop_front(sizeof(RecordPrefix));
-CVSymbol Sym(Kind, RecordData);
+CVSymbol Sym(RecordData);
 discoverTypeIndicesInSymbol(Sym, Refs);
 assert(Refs.size() == 1);
 assert(Refs.front().Count == 1);
@@ -959,7 +959,7 @@
 MutableArrayRef RecordBytes;
 if (NeedsRealignment) {
   RecordBytes = copyAndAlignSymbol(Sym, AlignedSymbolMem);
-  Sym = CVSymbol(Sym.kind(), RecordBytes);
+  Sym = CVSymbol(RecordBytes);
 } else {
   // Otherwise, we can actually mutate the symbol directly, since we
   // copied it to apply relocations.
@@ -983,7 +983,7 @@
 // An object file may have S_xxx_ID symbols, but these get converted to
 // "real" symbols in a PDB.
 translateIdSymbols(RecordBytes, TMerger.getIDTable());
-Sym = CVSymbol(symbolKind(RecordBytes), RecordBytes);
+Sym = CVSymbol(RecordBytes);
 
 // If this record refers to an offset in the object file's string 
table,
 // add that item to the global PDB string table and re-write the index.


Index: COFF/PDB.cpp
===
--- COFF/PDB.cpp
+++ COFF/PDB.cpp
@@ -759,7 +759,7 @@
   if (Kind == SymbolKind::S_GPROC32_ID || Kind == SymbolKind::S_LPROC32_ID) {
 SmallVector Refs;
 auto Content = RecordData.drop_front(sizeof(RecordPrefix));
-CVSymbol Sym(Kind, RecordData);
+CVSymbol Sym(RecordData);
 discoverTypeIndicesInSymbol(Sym, Refs);
 assert(Refs.size() == 1);
 assert(Refs.front().Count == 1);
@@ -959,7 +959,7 @@
 MutableArrayRef RecordBytes;
 if (NeedsRealignment) {
   RecordBytes = copyAndAlignSymbol(Sym, AlignedSymbolMem);
-  Sym = CVSymbol(Sym.kind(), RecordBytes);
+  Sym = CVSymbol(RecordBytes);
 } else {
   // Otherwise, we can actually mutate the symbol directly, since we
   // copied it to apply relocations.
@@ -983,7 +983,7 @@
 // An object file may have S_xxx_ID symbols, but these get converted to
 // "real" symbols in a PDB.
 translateIdSymbols(RecordBytes, TMerger.getIDTable());
-Sym = CVSymbol(symbolKind(RecordBytes), RecordBytes);
+Sym = CVSymbol(RecordBytes);
 
 // If this record refers to an offset in the object file's string table,
 // add that item to the global PDB string table and re-write the index.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D119044: Add a case for Rust in LLDB's PDB reader

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk 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/D119044/new/

https://reviews.llvm.org/D119044

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

[Lldb-commits] [PATCH] D129807: [LLDB][NativePDB] Add MSInheritanceAttr when completing CXXRecordDecl

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:492
   if (pr.isPointerToMember()) {
 MemberPointerInfo mpi = pr.getMemberInfo();
 GetOrCreateType(mpi.ContainingType);

I think you want to make code changes here to look at `mpi.Representation`.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:762
 
 VariableSP SymbolFileNativePDB::CreateGlobalVariable(PdbGlobalSymId var_id) {
   CVSymbol sym = m_index->symrecords().readRecord(var_id.offset);

This codepath seems specific to global variable creation. Are there other ways 
to hit this issue through other codepaths, such as local variables or non-type 
template parameters?

I would consider moving this logic closer to the logic which creates the AST 
member pointer type. Any time LLDB loads a member pointer type, it should check 
the inheritance kind, and then check if the class already has an 
MSInheritanceAttr, and add one if it is missing.

If the existing attribute doesn't match the attribute you want to add, that 
represents an ODR violation in the user program. I'm not sure what LLDB should 
do in that case.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:319
+  spelling = clang::MSInheritanceAttr::Keyword_virtual_inheritance;
+else if (bases.size() > 1)
+  spelling = clang::MSInheritanceAttr::Keyword_multiple_inheritance;

We shouldn't try to reimplement the frontend calculation logic, we should just 
trust the debug info. Users can use the [pointers_to_members 
pragma](https://docs.microsoft.com/en-us/cpp/preprocessor/pointers-to-members?view=msvc-170)
 to force the compiler to use different representations, so this calculation 
won't always be right.

Also, I don't think this case handles multiple inheritance via indirect bases. 
Consider:
```
struct A { int a; };
struct B { int b; };
struct C : A, B { int c; };
struct D : C { int d; };
```

If we need this logic in LLDB, try calling 
`CXXRecordDecl::calculateInheritanceModel`.


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

https://reviews.llvm.org/D129807

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

[Lldb-commits] [PATCH] D129807: [LLDB][NativePDB] Add MSInheritanceAttr when creating pointer type that is a pointer to member.

[Reid Kleckner via Phabricator via lldb-commits]

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

Thanks!




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:840
+spelling =
+clang::MSInheritanceAttr::Spelling::Keyword_multiple_inheritance;
+break;

This should be virtual, not multiple, right?



Comment at: lldb/test/Shell/SymbolFile/NativePDB/ast-types.cpp:111
+MITYPE *mp7 = nullptr;
+VI2TYPE *mp8 = nullptr;
+

It's worth adding tests for data member pointers as well as function member 
pointers, in particular ones that use -1 as the null representation. Try this:
```
int SI::*mp9 = nullptr;
```

Because offset zero is a valid offset, -1 is used to represent null in this 
case.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129807

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

[Lldb-commits] [PATCH] D130942: [LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754
+  m_ast.getASTContext(),
+  
clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance));
+}

I'm concerned that this isn't really the right fix. Changing the inheritance 
model changes the size of the member pointer representation, so the 
consequences of getting the wrong one could affect expression evaluation 
results. Zequan suggested guessing the inheritance model from the class 
definition, but we really ought to write down the inheritance model in the 
DWARF when using the MS ABI. This probably needs its own DWARF attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

[Lldb-commits] [PATCH] D130942: [LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754
+  m_ast.getASTContext(),
+  
clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance));
+}

mstorsjo wrote:
> rnk wrote:
> > I'm concerned that this isn't really the right fix. Changing the 
> > inheritance model changes the size of the member pointer representation, so 
> > the consequences of getting the wrong one could affect expression 
> > evaluation results. Zequan suggested guessing the inheritance model from 
> > the class definition, but we really ought to write down the inheritance 
> > model in the DWARF when using the MS ABI. This probably needs its own DWARF 
> > attribute.
> FWIW, there’s two use cases at play here:
> 
> 1. The executable actually is using the itanium ABI, but is being 
> (mis)interpreted with the MSVC ABI. When this happens, we currently crash, 
> which is a terrible user experience. In this case, there’s probably no good 
> solution (we can’t really get it right at this point) - but pretty much 
> anything is better than crashing. Before D127048, we always interpreted 
> everything with the MSVC C++ ABI; now we probably are getting it right in a 
> majority of cases at least.
> 
> 2. People intentionally using dwarf debug into with MSVC ABI. Not very 
> common, but something that some people do use, as far as I know. Here we at 
> least have a chance of getting right.
I see, I thought we already knew we were in use case 2 not 1. Having this as a 
workaround to not crash seems fine, but we really ought to warn when LLDB 
encounters these conditions:
1. MS C++ ABI is in use
2. DWARF is in use
3. A C++ source file is in use

Using DWARF for C code in the MS ABI model is fine, no need to warn in that 
case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130942

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

[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp:843-846
+std::map offset_to_size;
+// Get the size of each fields if it's udt.
+if (!FindMembersSize::GetMemberSizesForUdt(result.type, index.tpi(), 0,
+   offset_to_size))

I'm a bit worried about performance. This code runs for every `S_LOCAL` record. 
So, every time we encounter a `std::string` local variable, we walk over the 
entire string class hierarchy field list to compute this map, which we may or 
may not need later.

This code is pretty thorough, but can we reduce the scope of this patch by 
ignoring subfield records stored in memory, since they lack size information? I 
think that would make it easier to review and test. Just focus on variables in 
registers, and subfields in registers, since those are easiest to test and 
understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130796

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

[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk added a comment.

Looks good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130796

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

[Lldb-commits] [PATCH] D121967: [LLDB][NativePDB] Create inline function decls

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1123
+
+  PdbTypeSymId func_id(inline_site.Inlinee, true);
+  if (clang::Decl *decl = TryGetDecl(func_id))

Please add a comment about what the inlinee is, and what this lookup does. 
Basically, this lookup will find function declarations from previously inlined 
call sites.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1153
+  StringIdRecord sir;
+  cantFail(
+  TypeDeserializer::deserializeAs(parent_cvt, sir));

We shouldn't use cantFail if it isn't implied by local invariants. You should 
check if the parent scope is in fact a string first, otherwise this code will 
crash on invalid PDBs.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1155
+  TypeDeserializer::deserializeAs(parent_cvt, sir));
+  parent = GetOrCreateNamespaceDecl(sir.String.data(), *parent);
+}

Can these be nested? You may need to split these on `::` and create  or look up 
nested namespace decls.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1158
+
+CVType func_type_cvt = m_index.tpi().getType(func_ti);
+if (func_type_cvt.kind() == LF_PROCEDURE) {

This only requires `func_ti` as an input. Does it need to be part of the case? 
Should we compute param_count for methods too?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1163
+  TypeDeserializer::deserializeAs(func_type_cvt, pr));
+  param_count = pr.getParameterCount();
+}

This is overwritten later, so I think you can remove this if block.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1174
+  func_ct = ToCompilerType(func_qt);
+  const clang::FunctionProtoType *func_type =
+  llvm::dyn_cast(func_qt);

I don't know LLDB style, but LLVM style recommends using `auto` when casting so 
you do not need to repeat the casted type. This could be:
  const auto *func_type = llvm::dyn_cast(func_qt);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121967

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

[Lldb-commits] [PATCH] D121967: [LLDB][NativePDB] Create inline function decls

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1124
+  // Inlinee is the id index to the function id record that is inlined.
+  PdbTypeSymId func_id(inline_site.Inlinee, true);
+  // Look up the function decl by the id index to see if we have created a

inline_site is unused from here on in. Really, this function gets or creates a 
FunctionDecl from an LF_FUNC_ID record. Inline sites happen to point to one of 
those. Instead of naming this `GetOrCreateInlinedFunctionDecl`, could you name 
this `GetOrCreateFunctionDeclFromId` and restructure it accordingly? I think 
that will be more useful in the future if and when we have to make decls from 
other ids.

I believe S_*PROC32 records do not reference these records, so I don't think 
there is a way to share the codepaths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121967

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

[Lldb-commits] [PATCH] D125844: [LLDB][NativePDB] Check for missing type info to avoid crash.

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk 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/D125844/new/

https://reviews.llvm.org/D125844

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

[Lldb-commits] [PATCH] D125509: [LLDB][NFC] Decouple dwarf location table from DWARFExpression.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

@labath, ping, I see you just got back from vacation and this is a lot of code, 
but any high level design feedback would be helpful to know if this is the 
right direction.




Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:180
 if (!Loc)
-  return Callback(Loc.takeError());
-if (*Loc)
+  consumeError(Loc.takeError());
+else if (*Loc)

Did this change cause the presubmit test failure? 
DebugInfoDWARFTests/DWARFDie::getLocations looks related.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509

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

[Lldb-commits] [PATCH] D152189: [LLDB][PDB] Fix age field in UUID in PDB file.

[Reid Kleckner via Phabricator via lldb-commits]

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

Wow. Well, I'm not going to argue otherwise. :)

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152189

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

[Lldb-commits] [PATCH] D110885: [NFC][AttributeList] Replace index_begin/end with an iterator

[Reid Kleckner via Phabricator via lldb-commits]

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

lgtm




Comment at: llvm/include/llvm/IR/Attributes.h:854
 
-  /// Use these to iterate over the valid attribute indices.
-  unsigned index_begin() const { return AttributeList::FunctionIndex; }
-  unsigned index_end() const { return getNumAttrSets() - 1; }
+  struct index_iterator {
+unsigned NumAttrSets;

This deserves a comment just for consistency with the rest of these top-level 
members, even though it's a boring implementation detail of indexes().



Comment at: llvm/include/llvm/IR/Attributes.h:863
+  int_wrapper &operator++() {
+++i;
+return *this;

It's probably worth a comment that this operation is expected to overflow & 
wrap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110885

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

[Lldb-commits] [PATCH] D110885: [NFC][AttributeList] Replace index_begin/end with an iterator

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I had another idle thought on this matter: The unsigned overflow here really 
isn't that weird. It's the same thing as iterating over the range [-1, 0, 
`#args+1`]. We could update all these APIs to traffic in plain signed ints, and 
then there would be no wrapping, just normal index math.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110885

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

[Lldb-commits] [PATCH] D111715: [WIP] [lldb] change name demangling to be consistent between windows and linx

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk added a comment.

lgtm

This flag seems reasonable. Microsoft's UnDecorateName API has quite a few of 
these kinds of things:
https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-undecoratesymbolname

I don't think it makes sense to implement them all, but clearly, people have 
use cases for this kind of stuff. I think "name only" might be a useful 
addition at some point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111715

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

[Lldb-commits] [PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Thanks for the review! I uploaded it a bit quickly because I wanted to kick off 
Bazel presubmit testing, since I haven't replicated that locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111454

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

[Lldb-commits] [PATCH] D113930: [LLDB][NativePDB] Fix function decl creation for class methods

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1019
+  if (parent->isRecord()) {
+struct ProcessOneMethodRecord : public TypeVisitorCallbacks {
+  ProcessOneMethodRecord(PdbIndex &m_index, TypeSystemClang &m_clang,

As a style thing, this class is quite large, I think I would prefer to see this 
at global scope in an anonymous namespace.



Comment at: lldb/test/Shell/SymbolFile/NativePDB/find-functions.cpp:89
+// FIND-OVERLOAD: FuncType: id = {{.*}}, compiler_type = "int (void)"
+// FIND-OVERLOAD: FuncType: id = {{.*}}, compiler_type = "int (char)"
+// FIND-OVERLOAD: FuncType: id = {{.*}}, compiler_type = "int (char, int, ...)"

I guess the test isn't able to differentiate whether these methods are static, 
private, etc. =/

We can overlook that for now, but I think now is the time to invest in better 
testing tools. The lldb-test program can do whatever you need it to, I don't 
think changing it requires updating that many tests, I think it's something you 
should look into.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113930

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

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1198
 
   clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
   if (!parent)

Should the check be done before GetParentDeclContext or be done inside 
GetParentDeclContext so that we don't do wasted work computing a parent that 
won't be used for dynamic initializers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

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

[Lldb-commits] [PATCH] D133461: [LLDB][NativePDB] Set block address range.

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk 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/D133461/new/

https://reviews.llvm.org/D133461

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

[Lldb-commits] [PATCH] D134066: [LLDB][NativePDB] Forcefully complete a record type it has incomplete type debug info.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:681
+auto &ts = llvm::cast(*ct.GetTypeSystem());
+ts.GetMetadata(&tag)->SetIsForcefullyCompleted();
+  }

Is this what we do for DWARF? The same kind of situation seems like it can 
arise, where clang requires a type to be complete, but the information is 
missing, and we still try to proceed with evaluation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134066

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

[Lldb-commits] [PATCH] D136209: [LLDB][NativePDB] Fix parameter size for member functions LF_MFUNCTION

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Right, so `cantFail` must be an assertions-only check. That's not great. These 
deserialization APIs have really, really bad ergonomics and could use a lot of 
improvement if you want to look into that.

It would be really nice if, for example, this worked more like dyn_cast. 
Consider this kind of code:

  CVType signature;
  NewTypeMatcher d(signature);
  if (auto *sig = d.getAs()) {
 ... handle ProcedureRecord
  } else if (auto *sig = d.getAs()) {
... handle MemberFunctionRecord
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136209

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

[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

@Michael137, are you comfortable approving this, or can you reroute to a 
reviewer who can help us with this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134378

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

[Lldb-commits] [PATCH] D134066: [LLDB][NativePDB] Forcefully complete a record type if it has empty debug info and is required to have complete type.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Following the direction of the DWARF plugin sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134066

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

[Lldb-commits] [PATCH] D75279: Avoid SourceManager.h include in RawCommentList.h, add missing incs

[Reid Kleckner via Phabricator via lldb-commits]

rnk created this revision.
rnk added reviewers: aaron.ballman, hans.
Herald added a reviewer: teemperor.
Herald added subscribers: lldb-commits, arphaman.
Herald added projects: clang, LLDB.

SourceManager.h includes FileManager.h, which is expensive due to
dependencies on LLVM FS headers.

Remove dead BeforeThanCompare specialization.

Sink ASTContext::addComment to cpp file.

This reduces the time to compile a file that does nothing but include
ASTContext.h from ~3.4s to ~2.8s for me.

Saves these includes:

  219 -../clang/include/clang/Basic/SourceManager.h
  204 -../clang/include/clang/Basic/FileSystemOptions.h
  204 -../clang/include/clang/Basic/FileManager.h
  165 -../llvm/include/llvm/Support/VirtualFileSystem.h
  164 -../llvm/include/llvm/Support/SourceMgr.h
  164 -../llvm/include/llvm/Support/SMLoc.h
  161 -../llvm/include/llvm/Support/Path.h
  141 -../llvm/include/llvm/ADT/BitVector.h
  128 -../llvm/include/llvm/Support/MemoryBuffer.h
  124 -../llvm/include/llvm/Support/FileSystem.h
  124 -../llvm/include/llvm/Support/Chrono.h
  124 -.../MSVCSTL/include/stack
  122 -../llvm/include/llvm-c/Types.h
  122 -../llvm/include/llvm/Support/NativeFormatting.h
  122 -../llvm/include/llvm/Support/FormatProviders.h
  122 -../llvm/include/llvm/Support/CBindingWrapping.h
  122 -.../MSVCSTL/include/xtimec.h
  122 -.../MSVCSTL/include/ratio
  122 -.../MSVCSTL/include/chrono
  121 -../llvm/include/llvm/Support/FormatVariadicDetails.h
  118 -../llvm/include/llvm/Support/MD5.h
  109 -.../MSVCSTL/include/deque
  105 -../llvm/include/llvm/Support/Host.h
  105 -../llvm/include/llvm/Support/Endian.h


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75279

Files:
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTDumper.h
  clang/include/clang/AST/RawCommentList.h
  clang/lib/ARCMigrate/TransProtectedScope.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DataCollection.cpp
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/RawCommentList.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Analysis/CloneDetection.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/SanitizerMetadata.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/FileIndexRecord.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Tooling/ASTDiff/ASTDiff.cpp
  clang/lib/Tooling/Core/Lookup.cpp
  clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
  clang/lib/Tooling/Transformer/SourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Utility/Log.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecordLayout.h"
+#include "clang/Basic/SourceManager.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangUtil.h"
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
Index: clang/lib/Tooling/Transformer/SourceCode.cpp
===
--- clang/lib/Tooling/Transformer/SourceCode.cpp
+++ clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -17,6 +17,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/Errc.h"
 
Index: clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
Index: clang/lib/Tooling/Core/Lookup.cpp
===
--- clang/lib/Tooling/Core/Lookup.cpp
+++ clang/lib/Tooling/Core/Lookup.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
Index: clang/lib/Tooling/ASTDiff/ASTDiff.cpp
=

[Lldb-commits] [PATCH] D75279: Avoid SourceManager.h include in RawCommentList.h, add missing incs

[Reid Kleckner via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG86565c130942: Avoid SourceManager.h include in 
RawCommentList.h, add missing incs (authored by rnk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75279

Files:
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTDumper.h
  clang/include/clang/AST/RawCommentList.h
  clang/lib/ARCMigrate/TransProtectedScope.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DataCollection.cpp
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/RawCommentList.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Analysis/CloneDetection.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/SanitizerMetadata.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/FileIndexRecord.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Tooling/ASTDiff/ASTDiff.cpp
  clang/lib/Tooling/Core/Lookup.cpp
  clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
  clang/lib/Tooling/Transformer/SourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Utility/Log.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecordLayout.h"
+#include "clang/Basic/SourceManager.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangUtil.h"
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
Index: clang/lib/Tooling/Transformer/SourceCode.cpp
===
--- clang/lib/Tooling/Transformer/SourceCode.cpp
+++ clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -17,6 +17,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/Errc.h"
 
Index: clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
Index: clang/lib/Tooling/Core/Lookup.cpp
===
--- clang/lib/Tooling/Core/Lookup.cpp
+++ clang/lib/Tooling/Core/Lookup.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
Index: clang/lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- clang/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ clang/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -11,9 +11,9 @@
 //===--===//
 
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
-
 #include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/PriorityQueue.h"
 
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -8,6 +8,7 @@
 //  This file implements C++ template instantiation for declarations.
 //
 //===--===/
+
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
@@ -19,6 +20,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Template.h"
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/li

[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 247479.
rnk added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- fixes on Linux


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406

Files:
  clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
  clang-tools-extra/clangd/Format.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/ModuleMap.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Basic/SanitizerBlacklist.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/XRayLists.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPCallbacks.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/libclang/CXSourceLocation.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Index: lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
===
--- lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
+++ lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Utility/AnsiTerminal.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringSet.h"
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -9,6 +9,7 @@
 #include "ClangExpressionSourceCode.h"
 
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
Index: clang/tools/libclang/CXSourceLocation.cpp
===
--- clang/tools/libclang/CXSourceLocation.cpp
+++ clang/tools/libclang/CXSourceLocation.cpp
@@ -10,13 +10,14 @@
 //
 //===--===//
 
-#include "clang/Frontend/ASTUnit.h"
+#include "CXSourceLocation.h"
 #include "CIndexer.h"
 #include "CLog.h"
 #include "CXLoadedDiagnostic.h"
-#include "CXSourceLocation.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Format.h"
 
Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSupport.cpp
@@ -14,6 +14,7 @@
 
 #include "TestSupport.h"
 #include "clang/Basic/DiagnosticError.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
Index: clang/tools/clang-import-test/clang-import-test.cpp
===
--- clang/tools/clang-import-test/clang-import-test.cpp
+++ clang/tools/clang-import-test/clang-import-test.cpp
@@ -11,6 +11,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExternalASTMerger.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TargetInfo.h"
Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
+#include "clang/Basic

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk created this revision.
rnk added reviewers: aaron.ballman, hans.
Herald added projects: clang, LLDB.
Herald added a subscriber: lldb-commits.

Module.h takes 86ms to parse, mostly parsing the class itself. Avoid it
if possible. ASTContext.h depends on ExternalASTSource.h.

A few NFC changes were needed to make this possible:

- Move ASTSourceDescriptor to Module.h. This needs Module to be complete, and 
seems more related to modules and AST files than external AST sources.
- Move "import complete" bit from Module* pointer int pair to NextLocalImport 
pointer. Required because PointerIntPair requires Module to be 
complete, and now it may not be.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75784

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -40,6 +40,10 @@
 class DWARFASTParserClang;
 class PDBASTParser;
 
+namespace clang {
+class FileManager;
+}
+
 namespace lldb_private {
 
 class ClangASTMetadata;
Index: lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 #define LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 
+#include "clang/Basic/Module.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Sema/Sema.h"
@@ -71,7 +72,7 @@
 return m_Source->getModule(ID);
   }
 
-  llvm::Optional
+  llvm::Optional
   getSourceDescriptor(unsigned ID) override {
 return m_Source->getSourceDescriptor(ID);
   }
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1968,8 +1968,8 @@
 
 void ASTDeclReader::VisitImportDecl(ImportDecl *D) {
   VisitDecl(D);
-  D->ImportedAndComplete.setPointer(readModule());
-  D->ImportedAndComplete.setInt(Record.readInt());
+  D->ImportedModule = readModule();
+  D->setImportComplete(Record.readInt());
   auto *StoredLocs = D->getTrailingObjects();
   for (unsigned I = 0, N = Record.back(); I != N; ++I)
 StoredLocs[I] = readSourceLocation();
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8491,10 +8491,10 @@
   return (I - PCHModules.end()) << 1;
 }
 
-llvm::Optional
+llvm::Optional
 ASTReader::getSourceDescriptor(unsigned ID) {
   if (const Module *M = getSubmodule(ID))
-return ExternalASTSource::ASTSourceDescriptor(*M);
+return ASTSourceDescriptor(*M);
 
   // If there is only a single PCH, return it instead.
   // Chained PCH are not supported.
@@ -8503,8 +8503,8 @@
 ModuleFile &MF = ModuleMgr.getPrimaryModule();
 StringRef ModuleName = llvm::sys::path::filename(MF.OriginalSourceFileName);
 StringRef FileName = llvm::sys::path::filename(MF.FileName);
-return ASTReader::ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
-  MF.Signature);
+return ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
+   MF.Signature);
   }
   return None;
 }
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -20,6 +20,7 @@
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeOrdering.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
@@ -60,7 +61,7 @@
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
-  ExternalASTSource::ASTSourceDescriptor PCHDescriptor;
+  ASTSourceDescriptor PCHDescriptor;
   SourceLocation CurLoc;
   llvm::MDNode *CurInlinedAt 

[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406



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

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

In D75784#1910688 , @aprantl wrote:

> This will no doubt also need some patches to the Swift compiler, but given 
> the NFC-ness this hopefully should be fine.


True. I could leave behind a typedef for the ASTSourceDescriptor if it matters.

Any concerns, or does someone mind approving?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Thanks!

In D75784#1917181 , @aprantl wrote:

> To avoid bot breakage, I would recommend testing -DLLVM_ENABLE_MODULES=1 
> stage2 build works before landing this, as it is notoriously sensitive to 
> header reshuffling.


Good idea, I confirmed this passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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

[Lldb-commits] [PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
rnk marked an inline comment as done.
Closed by commit rGc915cb957dc3: Avoid including Module.h from 
ExternalASTSource.h (authored by rnk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Basic/Module.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -40,6 +40,10 @@
 class DWARFASTParserClang;
 class PDBASTParser;
 
+namespace clang {
+class FileManager;
+}
+
 namespace lldb_private {
 
 class ClangASTMetadata;
Index: lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 #define LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_ASTUTILS_H
 
+#include "clang/Basic/Module.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Sema/Sema.h"
@@ -71,7 +72,7 @@
 return m_Source->getModule(ID);
   }
 
-  llvm::Optional
+  llvm::Optional
   getSourceDescriptor(unsigned ID) override {
 return m_Source->getSourceDescriptor(ID);
   }
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1968,8 +1968,8 @@
 
 void ASTDeclReader::VisitImportDecl(ImportDecl *D) {
   VisitDecl(D);
-  D->ImportedAndComplete.setPointer(readModule());
-  D->ImportedAndComplete.setInt(Record.readInt());
+  D->ImportedModule = readModule();
+  D->setImportComplete(Record.readInt());
   auto *StoredLocs = D->getTrailingObjects();
   for (unsigned I = 0, N = Record.back(); I != N; ++I)
 StoredLocs[I] = readSourceLocation();
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8491,10 +8491,10 @@
   return (I - PCHModules.end()) << 1;
 }
 
-llvm::Optional
+llvm::Optional
 ASTReader::getSourceDescriptor(unsigned ID) {
   if (const Module *M = getSubmodule(ID))
-return ExternalASTSource::ASTSourceDescriptor(*M);
+return ASTSourceDescriptor(*M);
 
   // If there is only a single PCH, return it instead.
   // Chained PCH are not supported.
@@ -8503,8 +8503,8 @@
 ModuleFile &MF = ModuleMgr.getPrimaryModule();
 StringRef ModuleName = llvm::sys::path::filename(MF.OriginalSourceFileName);
 StringRef FileName = llvm::sys::path::filename(MF.FileName);
-return ASTReader::ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
-  MF.Signature);
+return ASTSourceDescriptor(ModuleName, MF.OriginalDir, FileName,
+   MF.Signature);
   }
   return None;
 }
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -20,6 +20,7 @@
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeOrdering.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
@@ -60,7 +61,7 @@
   llvm::DIBuilder DBuilder;
   llvm::DICompileUnit *TheCU = nullptr;
   ModuleMap *ClangModuleMap = nullptr;
-  ExternalASTSource::ASTSourceDescriptor PCHDescriptor;
+  ASTSourceDescriptor PCHDescriptor;
   SourceLocation CurLoc;
   llvm::MDNode *CurInlinedAt = nullptr;
   llvm::DIType *VTablePtrType = nullptr;
@@ -379,9 +380,7 @@
   /// When generating debug information for a clang module or
   /// precompiled header, this module map will be used to determine
   /// the module of origin of each Decl.
-  void setPCHDescriptor(ExternalASTSource::ASTSourceDescriptor PCH) {
-PCHDescriptor = PCH;
-  }
+  void setPCHDescriptor(ASTS

[Lldb-commits] [PATCH] D75406: Avoid including FileManager.h from SourceManager.h

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe08464fb4504: Avoid including FileManager.h from 
SourceManager.h (authored by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D75406?vs=247479&id=249752#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406

Files:
  clang-tools-extra/clang-include-fixer/find-all-symbols/FindAllMacros.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
  clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
  clang-tools-extra/clangd/Format.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/include/clang/Lex/DirectoryLookup.h
  clang/include/clang/Lex/ModuleMap.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/AST/ExternalASTSource.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Basic/SanitizerBlacklist.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/XRayLists.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPCallbacks.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/libclang/CXSourceLocation.cpp
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/Frontend/CompilerInstanceTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp

Index: lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
===
--- lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
+++ lldb/source/Plugins/Language/ClangCommon/ClangHighlighter.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Utility/AnsiTerminal.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringSet.h"
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -9,6 +9,7 @@
 #include "ClangExpressionSourceCode.h"
 
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
Index: clang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- clang/unittests/Frontend/CompilerInstanceTest.cpp
+++ clang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "llvm/Support/FileSystem.h"
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -8,6 +8,7 @@
 
 #include 
 
+#include "clang/Basic/FileManager.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
Index: clang/tools/libclang/CXSourceLocation.cpp
===
--- clang/tools/libclang/CXSourceLocation.cpp
+++ clang/tools/libclang/CXSourceLocation.cpp
@@ -10,13 +10,14 @@
 //
 //===--===//
 
-#include "clang/Frontend/ASTUnit.h"
+#include "CXSourceLocation.h"
 #include "CIndexer.h"
 #include "CLog.h"
 #include "CXLoadedDiagnostic.h"
-#include "CXSourceLocation.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Format.h"
 
Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSuppo

[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

This still feels a bit messy and I don't totally understand how it all fits 
together, but I'm super supportive of getting rid of the inline asm diagnostic 
handler and standardizing on DiagnosticHandler/DiagnosticInfo.




Comment at: llvm/include/llvm/MC/MCContext.h:33
 #include "llvm/Support/MD5.h"
+#include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"

MCContext.h is popular, let's try not to leak SourceMgr.h as an include. It 
probably brings in tons of FS headers that most files don't need.



Comment at: llvm/include/llvm/MC/MCContext.h:76
+std::function &)>;
 

It doesn't feel right to use MDNode, an IR type, from MC. Is there a way to 
make this opaque?



Comment at: llvm/include/llvm/MC/MCContext.h:386
+  if (!InlineSrcMgr)
+InlineSrcMgr.reset(new SourceMgr());
+}

This would need to be out of line to get by with a forward decl of SourceMgr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97449

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

In D99426#2653725 , @MaskRay wrote:

> This touches a lot of files. I am a bit worried that it would not be easy for 
> a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

Right, we need to separate text-ness for System/Z EBCDIC re-encoding from 
Windows CRLF translation. I think it's best to have a separate, positive CRLF 
bit, and to make that spelling longer than OF_Text. I think, in general, more 
problems are caused by extra unintended CRLF than by missing carriage returns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D70281: Fix -Wunused-result warnings in LLDB

[Reid Kleckner via Phabricator via lldb-commits]

rnk created this revision.
rnk added a reviewer: amccarth.
Herald added a project: LLDB.

Three uses of try_lock intentionally ignore the result. Make that
explicit with a void cast.

Add what appears to be a missing return in the clang expression parser
code. It's a functional change, but presumably the right one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70281

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp


Index: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -166,7 +166,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // Ignore the result, do it anyway.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   LLDB_LOGF(log,
@@ -308,7 +308,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // Ignore the result, do it anyway.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
@@ -395,7 +395,7 @@
 Target &target = m_process->GetTarget();
 std::unique_lock api_lock(target.GetAPIMutex(),
 std::defer_lock);
-api_lock.try_lock();
+(void)api_lock.try_lock(); // Ignore the result, do it anyway.
 auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
 StructuredData::DictionarySP thread_info_dict =
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -312,7 +312,7 @@
   if (m_exe_ctx.GetTargetPtr())
 return m_exe_ctx.GetTargetPtr();
   else if (m_sym_ctx.target_sp)
-m_sym_ctx.target_sp.get();
+return m_sym_ctx.target_sp.get();
   return nullptr;
 }
 


Index: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -166,7 +166,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // Ignore the result, do it anyway.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   LLDB_LOGF(log,
@@ -308,7 +308,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // Ignore the result, do it anyway.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
@@ -395,7 +395,7 @@
 Target &target = m_process->GetTarget();
 std::unique_lock api_lock(target.GetAPIMutex(),
 std::defer_lock);
-api_lock.try_lock();
+(void)api_lock.try_lock(); // Ignore the result, do it anyway.
 auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
 StructuredData::DictionarySP thread_info_dict =
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -312,7 +312,7 @@
   if (m_exe_ctx.GetTargetPtr())
 return m_exe_ctx.GetTargetPtr();
   else if (m_sym_ctx.target_sp)
-m_sym_ctx.target_sp.get();
+return m_sym_ctx.target_sp.get();
   return nullptr;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70281: Fix -Wunused-result warnings in LLDB

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4d23764dddc2: Fix -Wunused-result warnings in LLDB (authored 
by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D70281?vs=229421&id=229667#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70281

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
  lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp


Index: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -166,7 +166,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // See above.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   LLDB_LOGF(log,
@@ -308,7 +308,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // See above.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
@@ -395,7 +395,7 @@
 Target &target = m_process->GetTarget();
 std::unique_lock api_lock(target.GetAPIMutex(),
 std::defer_lock);
-api_lock.try_lock();
+(void)api_lock.try_lock(); // See above.
 auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
 StructuredData::DictionarySP thread_info_dict =
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -312,7 +312,7 @@
   if (m_exe_ctx.GetTargetPtr())
 return m_exe_ctx.GetTargetPtr();
   else if (m_sym_ctx.target_sp)
-m_sym_ctx.target_sp.get();
+return m_sym_ctx.target_sp.get();
   return nullptr;
 }
 


Index: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -166,7 +166,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // See above.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   LLDB_LOGF(log,
@@ -308,7 +308,7 @@
   Target &target = m_process->GetTarget();
   std::unique_lock api_lock(target.GetAPIMutex(),
   std::defer_lock);
-  api_lock.try_lock();
+  (void)api_lock.try_lock(); // See above.
   auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
@@ -395,7 +395,7 @@
 Target &target = m_process->GetTarget();
 std::unique_lock api_lock(target.GetAPIMutex(),
 std::defer_lock);
-api_lock.try_lock();
+(void)api_lock.try_lock(); // See above.
 auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
 StructuredData::DictionarySP thread_info_dict =
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -312,7 +312,7 @@
   if (m_exe_ctx.GetTargetPtr())
 return m_exe_ctx.GetTargetPtr();
   else if (m_sym_ctx.target_sp)
-m_sym_ctx.target_sp.get();
+return m_sym_ctx.target_sp.get();
   return nullptr;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65249: [NFC] use C++11 in AlignOf.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I still think this concern applies:
https://reviews.llvm.org/D64417#1576675

I'm not sure how to track down an 19.11 release to test if we can pass 
std::aligned_storage values through function calls on x86. We might be better 
off raising the minimum to 19.14, which is a more recent MSVC release in the 
2017 track. I don't think it's too much to ask developers to use the most 
recent version of the 2017 compiler, they won't have to change IDEs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249



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

[Lldb-commits] [PATCH] D65249: [NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[Reid Kleckner via Phabricator via lldb-commits]

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

I have concerns that some of the patches that you landed prior to this will 
cause issues with old versions of MSVC, but in isolation, this is fine, and if 
anyone complains, we can address it on a case-by-case basis. lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249



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

[Lldb-commits] [PATCH] D65826: Add support for deterministically linked binaries on macOS to lldb.

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

Code seems fine, I think this is probably the right fix, but this probably 
deserves a test along the lines of what's in the commit message. I'm not 
familiar with LLDB's test suite, so I can't give much guidance there.

Let's see if @JDevlieghere has input.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D65826



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

[Lldb-commits] [PATCH] D61292: Include inlined functions when figuring out a contiguous address range

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I think TestLineEntry.cpp didn't get added to the commit, so I removed it in 
r360076 to try to fix the build.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61292



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

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

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I think there's an issue with the whole idea of dropping .debug_info from 
objects without live sections. Consider:

  // a.cpp
  int main() {
return f();
  }
  // b.cpp
  struct Foo {
int x, y;
  };
  int f() {
volatile Foo var;
var.x = 13;
var.y = 42;
return var.x + var.y;
  }

When compiled and linked with thinlto, `f` is imported into the a.cpp TU, but 
the full definition of the `Foo` type remains in the b.cpp TU, because 
importing the full type would be expensive and wasteful.

I used these commands to show the change in behavior before and after this 
change:

  $ clang -O2 -flto=thin -c -ffunction-sections a.cpp b.cpp -g 
  # before
  $ ninja lld
  [1 processes, 3/3 @ 0.8/s : 3.582s ] Linking CXX executable bin/lld
  $ clang -Wl,--gc-sections -flto=thin  -fuse-ld=lld a.o b.o  -o t.exe
  $ gdb -ex 'b foo' -ex r -ex s -ex 'p var' -batch t.exe
  ...
  Breakpoint 1 at 0x201168: file a.cpp, line 2.
  
  Breakpoint 1, foo () at b.cpp:7
  7 var.y = 42;
  8 return var.x + var.y;
  $1 = {x = 13, y = 42}
  # after
  $ ninja lld 
  $ clang -Wl,--gc-sections -flto=thin  -fuse-ld=lld a.o b.o  -o t.exe 
  $ gdb -ex 'b foo' -ex r -ex s -ex 'p var' -batch t.exe
  ...
  Breakpoint 1 at 0x201168: file a.cpp, line 2.
  
  Breakpoint 1, foo () at b.cpp:7
  7 var.y = 42;
  8 return var.x + var.y;
  $1 = 

So, before `Foo` had a complete type, but now it does not.

I can't seem to construct an example where LLD will throw away useful debug 
info without thinlto, but Clang often makes assumptions that other object files 
will provide certain bits of debug info as a size optimization.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54747



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

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

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

There's another use case worth considering here, which is -fmodules-debuginfo. 
In that situation, a standalone object file is emitted that contains nothing 
but DWARF .debug_info sections, and the object is fed to the linker. If the 
linker drops it with --gc-sections, it's not going to work. Here's a transcript 
of me trying to use this feature:
https://reviews.llvm.org/P8150

Note that t.o (the normal object) only contains a forward declaration of Foo, 
and the complete definition is provided in the debug-info-only module object 
file, t2.o in this case.

I think, before this change, passing an object with only .debug_info in it (and 
--gc-sections) used to retain that debug info. I think we need a way to keep 
doing that going forward. I think that might mean we need a new flag for this 
new behavior, or we should revert this patch altogether, and recommend that 
people who want smaller debug info use tools like dsymutil, dwz, DWP, or other 
format aware things that can remove unreferenced or duplicate DWARF.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54747



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

[Lldb-commits] [PATCH] D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

You broke my build. =/ I got this output:

  CMake Error at C:/src/llvm-project/lldb/cmake/modules/LLDBConfig.cmake:201 
(message):
Found incompatible Python interpreter (3.7.3) and Python libraries ()

I'll mess with it a bit I guess.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64812



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

[Lldb-commits] [PATCH] D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

In D64812#1588055 , @rnk wrote:

> You broke my build. =/ I got this output:
>
>   CMake Error at C:/src/llvm-project/lldb/cmake/modules/LLDBConfig.cmake:201 
> (message):
> Found incompatible Python interpreter (3.7.3) and Python libraries ()
>
>
> I'll mess with it a bit I guess.


Fixed that in r366247. Apparently Windows has totally custom Python version 
detection logic.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64812



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

[Lldb-commits] [PATCH] D64536: Adding inline comments to code view type records

[Reid Kleckner via Phabricator via lldb-commits]

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

lgtm, sorry this fell out of my inbox.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64536



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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I see. We should give that constant materialization a location. It looks like 
it is coming from a phi node. The IR looks like this:

%6 = icmp ne i32 %5, 0, !dbg !11
br i1 %6, label %7, label %10, !dbg !12
  
  7:; preds = %2
%8 = load i32, i32* %4, align 4, !dbg !13
%9 = icmp ne i32 %8, 0, !dbg !13
br label %10
  
  10:   ; preds = %7, %2
%11 = phi i1 [ false, %2 ], [ %9, %7 ], !dbg !14    Probably where the 
zero comes from
%12 = zext i1 %11 to i32, !dbg !11
ret i32 %12, !dbg !15

The PHI node has location !14, which is a line 0 location. Is there a reason we 
give this PHI a line 0 location, when it's built by the frontend for the 
conditional operator? IMO we should use the location of the `br` instruction, 
which will be the location of the conditional operator (`&&` at the source 
level).

Paul has already shown that flushing the local value map improves debug info 
quality in general. If we can't fix all the gdb test suite failures, IMO we 
should consider XFAILing them and moving on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D79447: [Debug][CodeView] Emit fully qualified names for globals

[Reid Kleckner via Phabricator via lldb-commits]

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

lgtm, modulo ifdef adjustment




Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3125-3126
   } else {
 // FIXME: Currently this only emits the global variables in the IR 
metadata.
 // This should also emit enums and static data members.
 const DIExpression *DIE = CVGV.GVInfo.get();

aganea wrote:
> akhuang wrote:
> > rnk wrote:
> > > @akhuang, can we remove this FIXME? I think we addressed this simply by 
> > > changing clang to emit this metadata.
> > Yep- thanks for catching that!
> Can I leave the comment for 'enum'? There are still a few things missing in 
> the debug stream, in regards to enums (see other comment).
I'd leave the FIXME out, as you have it now. I don't think we're getting value 
from it here. At this point, it's in the frontend's hands to decide if the enum 
needs emitting, so the code fix would be far from this comment.



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2989
+const std::vector> &UDTs) {
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  size_t OriginalSize = UDTs.size();

This probably needs to be `#ifndef NDEBUG`, or a build with asserts but without 
ABI breaking checks (blech) will break. Besides, it's not an ABI breaking check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79447



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

[Lldb-commits] [PATCH] D79447: [Debug][CodeView] Emit fully qualified names for globals

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I will take a look and try to reland this, since I requested the assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79447



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

[Lldb-commits] [PATCH] D70442: [LLDB] Force message formatting to English

[Reid Kleckner via Phabricator via lldb-commits]

rnk accepted this revision.
rnk 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/D70442/new/

https://reviews.llvm.org/D70442



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

[Lldb-commits] [PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk created this revision.
rnk added reviewers: bkramer, klimek, rsmith.
Herald added subscribers: lldb-commits, kbarton, mgorny, nemanjai.
Herald added projects: clang, LLDB.

The only part of ASTContext.h that requires most AST types to be
complete is the parent map. Nothing in Clang proper uses the ParentMap,
so split it out into its own class. Make ASTContext own the
ParentMapContext so there is still a one-to-one relationship.

After this change, 562 fewer files depend on ASTTypeTraits.h, and 66
fewer depend on TypeLoc.h:

  $ diff -u deps-before.txt deps-after.txt | \
grep '^[-+] ' | sort | uniq -c | sort -nr | less
  562 -../clang/include/clang/AST/ASTTypeTraits.h
  340 +../clang/include/clang/AST/ParentMapContext.h
   66 -../clang/include/clang/AST/TypeLocNodes.def
   66 -../clang/include/clang/AST/TypeLoc.h
   15 -../clang/include/clang/AST/TemplateBase.h
  ...

I computed deps-before.txt and deps-after.txt with `ninja -t deps`.

This removes a common and key dependency on TemplateBase.h and
TypeLoc.h.

This also has the effect of breaking the ParentMap RecursiveASTVisitor
instantiation into its own file, which roughly halves the compilation
time of ASTContext.cpp (29.75s -> 17.66s). The new file takes 13.8s to
compile.

I left behind forwarding methods for getParents(), but clients will need
to include a new header to make them work:

  #include "clang/AST/ParentMapContext.h"

I noticed that this parent map functionality is unfortunately duplicated
in ParentMap.h, which only works for Stmt nodes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71313

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ParentMapContext.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Linkage.h
  clang/lib/AST/ParentMapContext.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/Tooling/ASTDiff/ASTDiff.cpp
  clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  lldb/include/lldb/Symbol/ClangASTContext.h

Index: lldb/include/lldb/Symbol/ClangASTContext.h
===
--- lldb/include/lldb/Symbol/ClangASTContext.h
+++ lldb/include/lldb/Symbol/ClangASTContext.h
@@ -21,6 +21,7 @@
 #include 
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/ExternalASTMerger.h"
 #include "clang/AST/TemplateBase.h"
 #include "llvm/ADT/APSInt.h"
Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -15,6 +15,7 @@
 
 #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
Index: clang/lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- clang/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ clang/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/PriorityQueue.h"
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -16,6 +16,7 @@
 
 #include "CGValue.h"
 #include "EHScopeStack.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Type.h"
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/LLVM.h"
@@ -222,7 +223,8 @@
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  auto N = Finder->getASTContext().traverseIgnored(DynNode);
+  auto N =
+  Finder->getASTContext().getParentMapContext().traverseIgnored(DynNode);
 
   if (RestrictKind.isB

[Lldb-commits] [PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk planned changes to this revision.
rnk marked an inline comment as done.
rnk added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:653
   template  DynTypedNodeList getParents(const NodeT &Node) {
 return getParents(ast_type_traits::DynTypedNode::create(Node));
   }

Turns out this only worked for me locally because of delayed template parsing. 
I will have to think harder about how to preserve the API while not requiring 
complete types here. I could make an overload set, but I worry it will be 
inconveniently large.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71313



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

[Lldb-commits] [PATCH] D72855: Make LLVM_APPEND_VC_REV=OFF affect clang, lld, and lldb as well.

[Reid Kleckner via Phabricator via lldb-commits]

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

lgtm




Comment at: llvm/cmake/modules/LLVMConfig.cmake.in:81
 
+set(LLVM_APPEND_VC_REV "@LLVM_APPEND_VC_REV@")
+

This here should make the standalone build configs work, I think.


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

https://reviews.llvm.org/D72855



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

[Lldb-commits] [PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk updated this revision to Diff 240050.
rnk added a comment.
This revision is now accepted and ready to land.

- Move things around while retaining API compatibility for getParents


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71313

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ParentMapContext.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Linkage.h
  clang/lib/AST/ParentMapContext.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/Tooling/ASTDiff/ASTDiff.cpp
  clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  lldb/include/lldb/Symbol/TypeSystemClang.h

Index: lldb/include/lldb/Symbol/TypeSystemClang.h
===
--- lldb/include/lldb/Symbol/TypeSystemClang.h
+++ lldb/include/lldb/Symbol/TypeSystemClang.h
@@ -21,6 +21,7 @@
 #include 
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/TemplateBase.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -15,6 +15,7 @@
 
 #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
Index: clang/lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- clang/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ clang/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/PriorityQueue.h"
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -16,6 +16,7 @@
 
 #include "CGValue.h"
 #include "EHScopeStack.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Type.h"
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/LLVM.h"
@@ -237,7 +238,8 @@
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  auto N = Finder->getASTContext().traverseIgnored(DynNode);
+  auto N =
+  Finder->getASTContext().getParentMapContext().traverseIgnored(DynNode);
 
   if (RestrictKind.isBaseOf(N.getNodeKind()) &&
   Implementation->dynMatches(N, Finder, Builder)) {
@@ -256,7 +258,8 @@
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  auto N = Finder->getASTContext().traverseIgnored(DynNode);
+  auto N =
+  Finder->getASTContext().getParentMapContext().traverseIgnored(DynNode);
 
   assert(RestrictKind.isBaseOf(N.getNodeKind()));
   if (Implementation->dynMatches(N, Finder, Builder)) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -143,11 +143,14 @@
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode && Finder->getASTContext().getTraversalKind() ==
-  ast_type_traits::TK_IgnoreUnlessSpelledInSource)
+  if (LambdaNode &&
+  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
+  ast_type_traits::TK_IgnoreUnlessSpelledInSource)
 StmtToTraverse = LambdaNode;
   else
-StmtToTraverse = Finder->getASTContext().traverseIgnored(ExprNode);
+StmtToTraverse =
+

[Lldb-commits] [PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

[Reid Kleckner via Phabricator via lldb-commits]

rnk added a comment.

I think I can land this version, but I'll wait until tomorrow so I'll be around 
to debug fallout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71313



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

[Lldb-commits] [PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a81daaa8b58: [AST] Split parent map traversal logic into 
ParentMapContext.h (authored by rnk).

Changed prior to commit:
  https://reviews.llvm.org/D71313?vs=240050&id=240283#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71313

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/ParentMapContext.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Linkage.h
  clang/lib/AST/ParentMapContext.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/Tooling/ASTDiff/ASTDiff.cpp
  clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  lldb/include/lldb/Symbol/TypeSystemClang.h

Index: lldb/include/lldb/Symbol/TypeSystemClang.h
===
--- lldb/include/lldb/Symbol/TypeSystemClang.h
+++ lldb/include/lldb/Symbol/TypeSystemClang.h
@@ -21,6 +21,7 @@
 #include 
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/TemplateBase.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -15,6 +15,7 @@
 
 #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
Index: clang/lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- clang/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ clang/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/PriorityQueue.h"
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -16,6 +16,7 @@
 
 #include "CGValue.h"
 #include "EHScopeStack.h"
+#include "clang/AST/ASTFwd.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/GlobalDecl.h"
 #include "clang/AST/Type.h"
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/LLVM.h"
@@ -237,7 +238,8 @@
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  auto N = Finder->getASTContext().traverseIgnored(DynNode);
+  auto N =
+  Finder->getASTContext().getParentMapContext().traverseIgnored(DynNode);
 
   if (RestrictKind.isBaseOf(N.getNodeKind()) &&
   Implementation->dynMatches(N, Finder, Builder)) {
@@ -256,7 +258,8 @@
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  auto N = Finder->getASTContext().traverseIgnored(DynNode);
+  auto N =
+  Finder->getASTContext().getParentMapContext().traverseIgnored(DynNode);
 
   assert(RestrictKind.isBaseOf(N.getNodeKind()));
   if (Implementation->dynMatches(N, Finder, Builder)) {
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -143,11 +143,14 @@
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode && Finder->getASTContext().getTraversalKind() ==
-  ast_type_traits::TK_IgnoreUnlessSpelledInSource)
+  if (LambdaNode &&
+  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
+  ast_type_traits::TK_IgnoreUnlessSpelledInSource)
 StmtToTraverse = LambdaNode;
   else
-StmtToTravers

[Lldb-commits] [PATCH] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener

[Reid Kleckner via Phabricator via lldb-commits]

rnk created this revision.
rnk added reviewers: JDevlieghere, amccarth.
Herald added a project: LLDB.

This appears to be a real bug caught by -Wunused-value. std::find_if
doesn't modify the underlying collection, it just returns an iterator
pointing to the matching element.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74010

Files:
  lldb/source/Utility/Broadcaster.cpp


Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -406,7 +406,7 @@
   listener_collection::iterator iter = m_listeners.begin(),
 end_iter = m_listeners.end();
 
-  std::find_if(iter, end_iter, predicate);
+  iter = std::find_if(iter, end_iter, predicate);
   if (iter != end_iter)
 m_listeners.erase(iter);
 


Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -406,7 +406,7 @@
   listener_collection::iterator iter = m_listeners.begin(),
 end_iter = m_listeners.end();
 
-  std::find_if(iter, end_iter, predicate);
+  iter = std::find_if(iter, end_iter, predicate);
   if (iter != end_iter)
 m_listeners.erase(iter);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener

[Reid Kleckner via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG50d2d33b8ef5: Fix BroadcasterManager::RemoveListener to 
really remove the listener (authored by rnk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74010

Files:
  lldb/source/Utility/Broadcaster.cpp


Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -406,7 +406,7 @@
   listener_collection::iterator iter = m_listeners.begin(),
 end_iter = m_listeners.end();
 
-  std::find_if(iter, end_iter, predicate);
+  iter = std::find_if(iter, end_iter, predicate);
   if (iter != end_iter)
 m_listeners.erase(iter);
 


Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -406,7 +406,7 @@
   listener_collection::iterator iter = m_listeners.begin(),
 end_iter = m_listeners.end();
 
-  std::find_if(iter, end_iter, predicate);
+  iter = std::find_if(iter, end_iter, predicate);
   if (iter != end_iter)
 m_listeners.erase(iter);
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits