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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

import-std-module test changes look good to me, thanks for fixing that up.

And yes, ideally the tests should never use any libc++ internal names (and LLDB 
should never print them for those tests). So I think not having those in the 
here is a step in the right direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D116155: [clang][AST][ASTImporter] Set record to complete during import of its members.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I'm really sorry @martong , but I no longer work on LLDB (or the ASTImporter) 
and I'm not really in the loop regarding LLDB development.

(Having said that, I still happy to answer questions about my own 
patches/reviews that I did in the past in case there are questions about that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116155

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

[PATCH] D112804: [ASTImporter] Remove ASTNodeImporter::IsStructuralMatch overload for EnumConstantDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG01b3bd3992b4: [ASTImporter] Remove 
ASTNodeImporter::IsStructuralMatch overload for… (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112804

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -956,6 +956,48 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceEnumConstantTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithSameValues) {
+  auto t = makeNamedDecls("enum foo { foo = 1 };", "enum foo { foo = 1 };",
+  Lang_C89);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+   EnumConstantsWithDifferentValues) {
+  auto t =
+  makeNamedDecls("enum e { foo = 1 };", "enum e { foo = 2 };", Lang_C89);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+   EnumConstantsWithDifferentExprsButSameValues) {
+  auto t = makeNamedDecls("enum e { foo = 1 + 1 };", "enum e { foo = 2 };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest,
+   EnumConstantsWithDifferentSignedness) {
+  auto t = makeNamedDecls("enum e : unsigned { foo = 1 };",
+  "enum e : int { foo = 1 };", Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithDifferentWidth) {
+  auto t = makeNamedDecls("enum e : short { foo = 1 };",
+  "enum e : int { foo = 1 };", Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumConstantTest, EnumConstantsWithDifferentName) {
+  auto t =
+  makeDecls("enum e { foo = 1 };", "enum e { bar = 1 };",
+  Lang_CXX11, enumConstantDecl());
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceTemplateTest : StructuralEquivalenceTest {};
 
 TEST_F(StructuralEquivalenceTemplateTest, ExactlySameTemplates) {
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1591,6 +1591,26 @@
   return true;
 }
 
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
+ EnumConstantDecl *D1,
+ EnumConstantDecl *D2) {
+  const llvm::APSInt  = D1->getInitVal();
+  const llvm::APSInt  = D2->getInitVal();
+  if (FromVal.isSigned() != ToVal.isSigned())
+return false;
+  if (FromVal.getBitWidth() != ToVal.getBitWidth())
+return false;
+  if (FromVal != ToVal)
+return false;
+
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+return false;
+
+  // Init expressions are the most expensive check, so do them last.
+  return IsStructurallyEquivalent(Context, D1->getInitExpr(),
+  D2->getInitExpr());
+}
+
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  EnumDecl *D1, EnumDecl *D2) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -468,7 +468,6 @@
 bool hasSameVisibilityContextAndLinkage(T *Found, T *From);
 
 bool IsStructuralMatch(Decl *From, Decl *To, bool Complain = true);
-bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl *ToEC);
 ExpectedDecl VisitDecl(Decl *D);
 ExpectedDecl VisitImportDecl(ImportDecl *D);
 ExpectedDecl VisitEmptyDecl(EmptyDecl *D);
@@ -2182,16 +2181,6 @@
   return Ctx.IsEquivalent(From, To);
 }
 
-bool ASTNodeImporter::IsStructuralMatch(EnumConstantDecl *FromEC,
-EnumConstantDecl *ToEC) {
-  const llvm::APSInt  = FromEC->getInitVal();
-  const llvm::APSInt  = ToEC->getInitVal();
-
-  return FromVal.isSigned() == ToVal.isSigned() &&
- FromVal.getBitWidth() == ToVal.getBitWidth() &&
- FromVal == ToVal;
-}
-
 ExpectedDecl ASTNodeImporter::VisitDecl(Decl *D) {
   Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
 << D->getDeclKindName();

[PATCH] D112796: [ASTImporter] Remove redundant IsStructuralMatch overloads

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG96808c69a13c: [ASTImporter] Remove redundant 
IsStructuralMatch overloads (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112796

Files:
  clang/lib/AST/ASTImporter.cpp

Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -467,18 +467,8 @@
 template 
 bool hasSameVisibilityContextAndLinkage(T *Found, T *From);
 
-bool IsStructuralMatch(Decl *From, Decl *To, bool Complain);
-bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
-   bool Complain = true);
-bool IsStructuralMatch(VarDecl *FromVar, VarDecl *ToVar,
-   bool Complain = true);
-bool IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToRecord);
+bool IsStructuralMatch(Decl *From, Decl *To, bool Complain = true);
 bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl *ToEC);
-bool IsStructuralMatch(FunctionTemplateDecl *From,
-   FunctionTemplateDecl *To);
-bool IsStructuralMatch(FunctionDecl *From, FunctionDecl *To);
-bool IsStructuralMatch(ClassTemplateDecl *From, ClassTemplateDecl *To);
-bool IsStructuralMatch(VarTemplateDecl *From, VarTemplateDecl *To);
 ExpectedDecl VisitDecl(Decl *D);
 ExpectedDecl VisitImportDecl(ImportDecl *D);
 ExpectedDecl VisitEmptyDecl(EmptyDecl *D);
@@ -2178,68 +2168,17 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(Decl *From, Decl *To, bool Complain) {
-  StructuralEquivalenceContext Ctx(
-  Importer.getFromContext(), Importer.getToContext(),
-  Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
-  false, Complain);
-  return Ctx.IsEquivalent(From, To);
-}
-
-bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
-RecordDecl *ToRecord, bool Complain) {
   // Eliminate a potential failure point where we attempt to re-import
   // something we're trying to import while completing ToRecord.
-  Decl *ToOrigin = Importer.GetOriginalDecl(ToRecord);
+  Decl *ToOrigin = Importer.GetOriginalDecl(To);
   if (ToOrigin) {
-auto *ToOriginRecord = dyn_cast(ToOrigin);
-if (ToOriginRecord)
-  ToRecord = ToOriginRecord;
+To = ToOrigin;
   }
 
-  StructuralEquivalenceContext Ctx(Importer.getFromContext(),
-   ToRecord->getASTContext(),
-   Importer.getNonEquivalentDecls(),
-   getStructuralEquivalenceKind(Importer),
-   false, Complain);
-  return Ctx.IsEquivalent(FromRecord, ToRecord);
-}
-
-bool ASTNodeImporter::IsStructuralMatch(VarDecl *FromVar, VarDecl *ToVar,
-bool Complain) {
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
   false, Complain);
-  return Ctx.IsEquivalent(FromVar, ToVar);
-}
-
-bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
-  // Eliminate a potential failure point where we attempt to re-import
-  // something we're trying to import while completing ToEnum.
-  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
-if (auto *ToOriginEnum = dyn_cast(ToOrigin))
-ToEnum = ToOriginEnum;
-
-  StructuralEquivalenceContext Ctx(
-  Importer.getFromContext(), Importer.getToContext(),
-  Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
-  return Ctx.IsEquivalent(FromEnum, ToEnum);
-}
-
-bool ASTNodeImporter::IsStructuralMatch(FunctionTemplateDecl *From,
-FunctionTemplateDecl *To) {
-  StructuralEquivalenceContext Ctx(
-  Importer.getFromContext(), Importer.getToContext(),
-  Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
-  false, false);
-  return Ctx.IsEquivalent(From, To);
-}
-
-bool ASTNodeImporter::IsStructuralMatch(FunctionDecl *From, FunctionDecl *To) {
-  StructuralEquivalenceContext Ctx(
-  Importer.getFromContext(), Importer.getToContext(),
-  Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
-  false, false);
   return Ctx.IsEquivalent(From, To);
 }
 
@@ -2253,24 +2192,6 @@
  FromVal == ToVal;
 }
 
-bool ASTNodeImporter::IsStructuralMatch(ClassTemplateDecl *From,
-ClassTemplateDecl *To) {
-  StructuralEquivalenceContext Ctx(Importer.getFromContext(),
-   Importer.getToContext(),

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D105168#3071152 , @craig.topper 
wrote:

> In D105168#3069499 , @teemperor 
> wrote:
>
>> This broke the modules build:
>>
>>   While building module 'LLVM_Utils' imported from 
>> lvm/lib/Support/TargetParser.cpp:14:
>>   In file included from :209:
>>   llvm/include/llvm/Support/RISCVISAInfo.h:15:10: fatal error: could not 
>> build module 'LLVM_Option'
>>   #include "llvm/Option/ArgList.h"
>>^~~
>>   llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build 
>> module 'LLVM_Utils'
>>   #include "llvm/Support/TargetParser.h"
>>^
>>
>> `Support` headers can't include `Option` headers (as `Option` depends on 
>> `Support` already, so that would be a cyclic dependency). I believe folks 
>> here are in the US time zone, so I went ahead and replaced the header 
>> include with a forward decl to unbreak the bots (see 
>> de4d2f80b75e2a1e4b0ac5c25e20f20839633688 
>>  )
>>
>> FWIW, I think there is a better place than `Support` for this new class. 
>> `Support` is a dependency of nearly every single LLVM component, but this 
>> class seems to be used by a handful of files. Could we maybe move this to 
>> some other/new part of LLVM (and make the few components that need it depend 
>> on that)?
>
> Thanks for the header fix. I think we also need to fix the library circular 
> dependency that still exists. The Support library should not use anything 
> from the Option library. Maybe we can partition the function some way that 
> moves the MakeArgStr back into the clang driver code.
>
> I don't know if we have a better library for this new class. I think Support 
> is the only common library between the clang driver and LLVM's MC layer. I 
> supposed we could create a new library, but that might be a hard sell for one 
> file.

IIRC there are some other target-specific classes in Support for the same 
reason. Maybe we can make something such as `TargetSupport` or `SharedTarget`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

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

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This broke the modules build:

  While building module 'LLVM_Utils' imported from 
lvm/lib/Support/TargetParser.cpp:14:
  In file included from :209:
  llvm/include/llvm/Support/RISCVISAInfo.h:15:10: fatal error: could not build 
module 'LLVM_Option'
  #include "llvm/Option/ArgList.h"
   ^~~
  llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build module 
'LLVM_Utils'
  #include "llvm/Support/TargetParser.h"
   ^

`Support` headers can't include `Option` headers (as `Option` depends on 
`Support` already, so that would be a cyclic dependency). I believe folks here 
are in the US time zone, so I went ahead and replaced the header include with a 
forward decl to unbreak the bots (see de4d2f80b75e2a1e4b0ac5c25e20f20839633688 
 )

FWIW, I think there is a better place than `Support` for this new class. 
`Support` is a dependency of nearly every single LLVM component, but this class 
seems to be used by a handful of files. Could we maybe move this to some 
other/new part of LLVM (and make the few components that need it depend on 
that)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

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

[PATCH] D102923: [clang][lex] Remark on search path usage

[Raphael Isemann via Phabricator via cfe-commits]

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

This seems to have broken compiling some versions of Foundation/Dispatch with 
-fmodules enabled (which breaks the LLDB test suite when it tries to compile 
any Objective-C tests). Can you take a look and/or revert? I mailed you the 
version/compiler errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102923

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

[PATCH] D111387: [NFC] [Clang] Remove pre-computed complex float types

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

Would be nice to have the motivation for this in the commit message (IIUC this 
is based on the suggestion from D109948  ). 
Patch itself LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111387

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

[PATCH] D108919: [clan-repl] Install clang-repl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This is essentially what D106813  was 
supposed to do, so this is also LGTM.

You probably want to file a bugreport for @tstellar to cherry-pick this to the 
release. The biggest impact from this seems to be that it increases the release 
size and maybe that some maintainers want to package this separately (or not at 
all). It's probably worth pointing this out when making the cherry-pick bug 
report.


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

https://reviews.llvm.org/D108919

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

[PATCH] D108407: [CodeGen][WIP] Avoid generating Record layouts for pointee types

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I'm mostly putting this up to get some early feedback if anyone sees a problem 
with using opaque types here (e.g. it breaks some optimizations, etc.). If it 
does, it would still be nice if we could at least make this happen on some 
opt-in bases as it would be very beneficial for improving the performance of 
LLDB.




Comment at: llvm/include/llvm/IR/Instructions.h:1176
  ->isOpaqueOrPointeeTypeMatches(ResultElementType));
+  assert(PointeeType->isSized());
   init(Ptr, IdxList, NameStr);

)This change and the one below slipped in by accident, that was more of a 
debugging help that I wanted to put up as a separate patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108407

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

[PATCH] D108407: [CodeGen][WIP] Avoid generating Record layouts for pointee types

[Raphael Isemann via Phabricator via cfe-commits]

teemperor created this revision.
teemperor added reviewers: dblaikie, rjmccall, rsmith, v.g.vassilev.
teemperor added a project: clang.
Herald added a subscriber: dexonsmith.
teemperor requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits.
Herald added a project: LLVM.

This is a WIP patch that tries to avoid creating a RecordLayout in Clang and 
instead just emit an opaque structure type
as if we only had a forward declarations. The main motivation for this patch is 
actually just supporting a use case in LLDB
where laying out types can be very expensive as it usually triggers parsing of 
debug information.

The changes in this patch can be summarized as:

- `CodeGenTypes::ConvertRecordDeclType` (and related funcs) have a new 
parameter that tells us if we need the definition. It's currently only set to 
false for Clang pointer types.
- There are a few new places where I added (temporary) calls to 
`ConvertTypeForMem()` on some pointee types. The reason is that the code after 
is usually creating GEP instructions where we need a non-opaque source type. We 
can't do this automatically from the GEP factory methods as they would need to 
know the clang::Type to automatically do this (and they only have the 
llvm::Type that can't be mapped back to a clang::Type from what I can see, but 
that might be incorrect).
- A few test that needed to be adjusted as they relied on e.g. `Foo *x` to be 
enough to force `Foo` to be laid out/emitted.

There are still about a dozen more tests failing but from what I can see they 
all just need to be adjusted to force specific types to be emitted. I'll fix 
those up once there is consensus that this patch is going in the right 
direction.

Some benchmarks: I did a stage2 build of LLVM+Clang with my patch and those are 
the stats:

  current ToT Clang:
  2232421 - total amount of struct types created
94911 - of which are opaque struct types
  
  with this patch:
  1715074 - total amount of struct types created (-23%)
   173127 - of which are opaque struct types (+82%)

I built a part of Clang (the last 300 source files in the 
compile_commands.json) and the average time on my 64 core machine changes like 
this (as per hyperfine):

  Benchmark #1: parallel --progress -j63 -a ToT-clang
Time (mean ± σ): 27.703 s ±  0.168 s[User: 1434.619 s, System: 
66.687 s]
Range (min … max):   27.459 s … 27.891 s10 runs
   
  Benchmark #2: parallel --progress -j63 -a with-patch
Time (mean ± σ): 27.439 s ±  0.111 s[User: 1427.739 s, System: 
66.220 s]
Range (min … max):   27.300 s … 27.625 s10 runs
   
  Summary
'parallel --progress -j63 -a with-patch' ran
  1.01 ± 0.01 times faster than 'parallel --progress -j63 -a ToT-clang'


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108407

Files:
  clang/include/clang/AST/Type.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h
  clang/test/CodeGen/c11atomics.c
  clang/test/CodeGenCXX/class-layout.cpp
  clang/test/CodeGenCXX/pr18962.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp
  llvm/include/llvm/IR/Instructions.h

Index: llvm/include/llvm/IR/Instructions.h
===
--- llvm/include/llvm/IR/Instructions.h
+++ llvm/include/llvm/IR/Instructions.h
@@ -1173,6 +1173,7 @@
   ResultElementType(getIndexedType(PointeeType, IdxList)) {
   assert(cast(getType()->getScalarType())
  ->isOpaqueOrPointeeTypeMatches(ResultElementType));
+  assert(PointeeType->isSized());
   init(Ptr, IdxList, NameStr);
 }
 
@@ -1187,6 +1188,7 @@
   ResultElementType(getIndexedType(PointeeType, IdxList)) {
   assert(cast(getType()->getScalarType())
  ->isOpaqueOrPointeeTypeMatches(ResultElementType));
+  assert(PointeeType->isSized());
   init(Ptr, IdxList, NameStr);
 }
 
Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -148,6 +148,6 @@
 
 
 // The warnings are emitted when the layout of the structs is computed, so we have to use them.
-void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*,
-   S14*, S15*, S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*,
-   S26*, S27*){}
+void f(S1, S2, S3, S4, S5, S6, S7, S8, S9, S10, S11, S12, S13,
+   S14, S15, S16, S17, S18, S19, S20, S21, S22, S23, S24, S25,
+   S26, S27){}
Index: clang/test/CodeGenCXX/pr18962.cpp
===
--- clang/test/CodeGenCXX/pr18962.cpp
+++ clang/test/CodeGenCXX/pr18962.cpp
@@ -27,6 +27,5 @@
 // We end up using an opaque type for 'append' to avoid circular references.
 // CHECK: %class.A = 

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor updated this revision to Diff 364385.
teemperor added a comment.

- Make setter propagate 'deserialize' to the node dumper (thanks Volodymyr!)


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

https://reviews.llvm.org/D80878

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterObjCTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,182 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// An ExternalASTSource that records which DeclContexts were queried so far.
+class RecordingExternalASTSource : public ExternalASTSource {
+public:
+  llvm::DenseSet QueriedDCs;
+  ~RecordingExternalASTSource() override = default;
+
+  void FindExternalLexicalDecls(const DeclContext *DC,
+llvm::function_ref,
+SmallVectorImpl &) override {
+QueriedDCs.insert(DC);
+  }
+};
+
+class ASTDumpTestBase : public ::testing::Test {
+protected:
+  ASTDumpTestBase(clang::ExternalASTSource *Source)
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins, TU_Complete) {
+Ctxt.setExternalSource(Source);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class NoDeserializeTest : public ASTDumpTestBase {
+protected:
+  NoDeserializeTest() : ASTDumpTestBase(new TrappingExternalASTSource()) {}
+};
+
+/// Tests which declarations Decl::dump deserializes;
+class DeserializeTest : public ASTDumpTestBase {
+protected:
+  DeserializeTest()
+  : ASTDumpTestBase(Recorder = new RecordingExternalASTSource()) {}
+  RecordingExternalASTSource *Recorder;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This is technically in the "Waiting on Review" queue, that's why I also added 
another reviewer :). The original revision got accepted and then I reverted and 
updated the patch/test to honor the `Deserialize` value (which was broken in 
the first version). I guess the fact that this got reviewed, landed, reverted 
and back into review caused some confusion

Also obligatory "friendly ping" on this :)


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

https://reviews.llvm.org/D80878

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

[PATCH] D106813: [clang-repl] Build and install clang-repl by default

[Raphael Isemann via Phabricator via cfe-commits]

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

I think my concerns from D96033  are all 
addressed by now, so this LGTM. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D106813

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

[PATCH] D106266: [C++4OpenCL] Add run line standard aliases clc++1.0 and CLC++1.0

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Quick note to prevent some confusion: I saw this patch and realized that the 
LLDB change was only necessary because some Clang code got copy pasted into 
LLDB. I removed that copy in https://reviews.llvm.org/D106537 so if you see 
merge conflicts while merging this, you can just drop the LLDB changes as they 
shouldn't be necessary once my patch landed. Thanks!


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

https://reviews.llvm.org/D106266

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

[PATCH] D105926: [PowerPC] Extra test case for LDARX

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Sorry for raising an unrelated topic here, but I can't reach @Conanap directly 
via the mail from the git commits: @Conanap could you please create the git 
branches for your patches in your own Github fork instead of the main LLVM 
repo? LLVM's policy is to have working branches in everyone's private fork 
(even though I don't think we explicitly tell people that when they get commit 
access). I'll go ahead and delete your created branches end of next week, but 
let me know if I should wait a bit longer with that. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105926

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

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

[Raphael Isemann via Phabricator via cfe-commits]

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

I'm really sorry, I don't understand what kind of review is expected here given 
the state of this patch.

- The patch **still** doesn't include the tests that I asked for in my previous 
comment: https://reviews.llvm.org/D105564#2871703
- I applied the patch locally and **again** LLDB just crashes on the first 
corner case that I test.
- I genuinely can't comprehend why my crash reproducer got copied one-to-one 
into this patch as a real test case. It's just meant to make it straightforward 
to reproduce the crash I ran into, but the 'test' itself does completely bogus 
things like checking for the *wrong* return value, declaring dead code, all the 
identifiers are just 'bla', etc...

I think it's part of a good review to actually compile and poke around at the 
code that I'm requested to review. It's also fine if I run into some weird 
corner cases while doing some manual testing (one could argue that's the whole 
point of the review. I'm always happy when someone invests the time to do that 
for my patches). But this patch feels like every reviewer either has to just 
hope this just works or IKEA-style assemble their own tests at home. This is 
fine for an RFC/WIP patch, but this is clearly aiming to be accepted at the 
moment.

Given how long everyone's review queue is, it seems frankly unfair to other 
people to postpone reviewing their patches and instead spend time repeatedly 
reviewing this patch by manually compiling and testing it. I'll send this patch 
back and I'm happy to review it once it's actually ready, but at the moment 
it's sadly not.

Some notes on what I expect to happen to this patch before this goes back in 
anyone's review queue:

- Get enough (documented) tests that a reasonable person can look at this patch 
and believe it could work in production.
- Obviously fix any bugs uncovered by the tests.
- I don't think the current approach will always correctly resolve the return 
type (which is perfectly fine), but if that happens we shouldn't crash. IIRC 
the reason why my original patch set the C++14 flag in the expr evaluator is 
because it makes using these raw 'auto' types an error diagnostic instead of a 
crash. In any case, this should be tested then. Just to be clear: I think 
getting this to work in every case is out of scope for this patch so I think 
it's fine if those cases are at just documented/tested.




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

shafik wrote:
> JDevlieghere wrote:
> > What's the purpose of this? Do we expect to see the type attribute more 
> > than once? 
> Good question, the first iteration was done by @teemperor and then I modified 
> it heavily but I did not dig  into this change to deeply but I was pretty 
> sure when we first talked about there was a good reason for it.
`ParsedDWARFTypeAttributes` iterates not just over the attributes in the 
current DIE, but also follows `DW_AT_specification` and then iterates the 
attributes there. But the `DW_AT_specification` of the definition points to the 
declaration with the `auto` type, so without this change we would first assign 
`type` to the real type from the definition and then when we iterate over the 
attributes of the declaration we would overwrite the real type with the `auto` 
type.

In short: without this change `ParsedDWARFTypeAttributes(definition_die).type` 
would be `auto` instead of the real type.


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

https://reviews.llvm.org/D105564

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

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

[Raphael Isemann via Phabricator via cfe-commits]

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

I guess Phabricator just sent this back to review automatically when you 
updated the diff? Just mark this as 'needs review' again when this is ready for 
review. I'll send this back in the meantime to clear my review queue.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:899
+  clang::QualType qt = ClangUtil::GetQualType(function_type);
+  const auto *ft = qt->getAs();
+  TypeSystemClang *ts =

unused variable



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:919
   // Parse the function children for the parameters
-
   DWARFDIE decl_ctx_die;

unrelated change?


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

https://reviews.llvm.org/D105564

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

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

[Raphael Isemann via Phabricator via cfe-commits]

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

I think this looks good, thanks for fixing this! I believe there should be a 
few more tests for all the corner cases we can run into here (those tests can 
all be just Python API tests, no need for having them all in assembly format). 
Also I think lambdas where triggering the original issue, so that would also be 
nice to have in a test.

I started writing some example tests but this is crashing for me with:

  Unexpected undeduced type!
  UNREACHABLE executed at 
/home/teemperor/work/ci/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp:624!
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
  Stack dump:
  0.Program arguments: ./bin/lldb 
././lldb-test-build.noindex/lang/cpp/auto_return/TestCppAutoReturn.test_dwarf/a.out
   #0 0x55f540b5e611 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
(./bin/lldb+0x34611)
   #1 0x55f540b5c6b0 llvm::sys::RunSignalHandlers() (./bin/lldb+0x326b0)
   #2 0x55f540b5efb6 SignalHandler(int) (./bin/lldb+0x34fb6)
   #3 0x7fe3c3484870 __restore_rt (/usr/lib/libpthread.so.0+0x13870)
   #4 0x7fe3bd2e3d22 raise (/usr/lib/libc.so.6+0x3cd22)
   #5 0x7fe3bd2cd862 abort (/usr/lib/libc.so.6+0x26862)
   #6 0x7fe3becf0df1 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x146adf1)
   #7 0x7fe3bf7cbdda 
clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1f45dda)
   #8 0x7fe3bf7caf7e 
clang::CodeGen::CodeGenTypes::ConvertTypeForMem(clang::QualType, bool) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1f44f7e)
   #9 0x7fe3bf951dd8 
clang::CodeGen::CodeGenModule::getOrCreateStaticVarDecl(clang::VarDecl const&, 
llvm::GlobalValue::LinkageTypes) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20cbdd8)
  #10 0x7fe3bf950b10 
clang::CodeGen::CodeGenFunction::EmitStaticVarDecl(clang::VarDecl const&, 
llvm::GlobalValue::LinkageTypes) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20cab10)
  #11 0x7fe3bf9508f3 
clang::CodeGen::CodeGenFunction::EmitVarDecl(clang::VarDecl const&) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20ca8f3)
  #12 0x7fe3bf950458 clang::CodeGen::CodeGenFunction::EmitDecl(clang::Decl 
const&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20ca458)
  #13 0x7fe3bf6f887c 
clang::CodeGen::CodeGenFunction::EmitDeclStmt(clang::DeclStmt const&) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e7287c)
  #14 0x7fe3bf6ee2d8 
clang::CodeGen::CodeGenFunction::EmitSimpleStmt(clang::Stmt const*, 
llvm::ArrayRef) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e682d8)
  #15 0x7fe3bf6eda89 clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt 
const*, llvm::ArrayRef) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e67a89)
  #16 0x7fe3bf6f96b0 
clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt
 const&, bool, clang::CodeGen::AggValueSlot) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e736b0)
  #17 0x7fe3bf7585b4 
clang::CodeGen::CodeGenFunction::EmitFunctionBody(clang::Stmt const*) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ed25b4)
  #18 0x7fe3bf7591de 
clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, 
llvm::Function*, clang::CodeGen::CGFunctionInfo const&) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ed31de)
  #19 0x7fe3bf77a51f 
clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, 
llvm::GlobalValue*) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef451f)
  #20 0x7fe3bf77222d 
clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, 
llvm::GlobalValue*) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1eec22d)
  #21 0x7fe3bf7773bb 
clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef13bb)
  #22 0x7fe3bf77e039 
clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef8039)
  #23 0x7fe3bf6ab280 (anonymous 
namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e25280)
  #24 0x7fe3bf549632 
lldb_private::ASTResultSynthesizer::HandleTopLevelDecl(clang::DeclGroupRef) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1cc3632)
  #25 0x7fe3c018a558 clang::ParseAST(clang::Sema&, bool, bool) 
(/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x2904558)
  #26 0x7fe3bf55d493 
lldb_private::ClangExpressionParser::ParseInternal(lldb_private::DiagnosticManager&,
 clang::CodeCompleteConsumer*, unsigned int, unsigned int) 

[PATCH] D104918: [clang-repl] Implement partial translation units and error recovery.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

@v.g.vassilev For LLDB you need to change the 
`TypeSystemClang::SetExternalSource` function to this (it just moves the 
`setHasExternalLexicalStorage` one line up. You can just add that change and 
the compilation fix to this commit.

  void TypeSystemClang::SetExternalSource(
  llvm::IntrusiveRefCntPtr _source_up) {
ASTContext  = getASTContext();
ast.getTranslationUnitDecl()->setHasExternalLexicalStorage(true);
ast.setExternalSource(ast_source_up);
  }

The problem is that a few ASTs in LLDB change the ExternalSource that is 
originally set, but the LazyGenerationalUpdatePtr remembers the initial 
ExternalSource and tries to access it to complete the redecl chain (but the 
original ExternalSource just got replaced and deleted). The better fix is to 
not even create that original ExternalSource, but I can fix that in a follow up 
as that seems out of scope for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104918

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

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:164
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+return {};

`getParent()` will assert when the MethodDecl is not a in a Record (which is 
the only way this can return null) so you can remove that check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

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

[PATCH] D103750: [analyzer][WIP] Handle std::make_unique for SmartPtrModeling

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D103750#2812613 , @RedDocMD wrote:

> How do I set the C++ standard while running a test?

If I understand your language correctly, then see my inline comment.




Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:4
 // RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 

You set the standard here via the `-std` arg. If you want different standards 
you need to have multiple `RUN:` lines that all pass a different standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

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

[PATCH] D102903: [clang] Fix Wnested-anon-types in ABIArgInfo

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGebd25fde5e04: [clang] Fix Wnested-anon-types in ABIArgInfo 
(authored by teemperor).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D102903?vs=346942=346960#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102903

Files:
  clang/include/clang/CodeGen/CGFunctionInfo.h


Index: clang/include/clang/CodeGen/CGFunctionInfo.h
===
--- clang/include/clang/CodeGen/CGFunctionInfo.h
+++ clang/include/clang/CodeGen/CGFunctionInfo.h
@@ -93,15 +93,17 @@
 llvm::Type *PaddingType; // canHavePaddingType()
 llvm::Type *UnpaddedCoerceAndExpandType; // isCoerceAndExpand()
   };
+  struct DirectAttrInfo {
+unsigned Offset;
+unsigned Align;
+  };
+  struct IndirectAttrInfo {
+unsigned Align;
+unsigned AddrSpace;
+  };
   union {
-struct {
-  unsigned Offset;
-  unsigned Align;
-} DirectAttr;  // isDirect() || isExtend()
-struct {
-  unsigned Align;
-  unsigned AddrSpace;
-} IndirectAttr;// isIndirect()
+DirectAttrInfo DirectAttr; // isDirect() || isExtend()
+IndirectAttrInfo IndirectAttr; // isIndirect()
 unsigned AllocaFieldIndex; // isInAlloca()
   };
   Kind TheKind;


Index: clang/include/clang/CodeGen/CGFunctionInfo.h
===
--- clang/include/clang/CodeGen/CGFunctionInfo.h
+++ clang/include/clang/CodeGen/CGFunctionInfo.h
@@ -93,15 +93,17 @@
 llvm::Type *PaddingType; // canHavePaddingType()
 llvm::Type *UnpaddedCoerceAndExpandType; // isCoerceAndExpand()
   };
+  struct DirectAttrInfo {
+unsigned Offset;
+unsigned Align;
+  };
+  struct IndirectAttrInfo {
+unsigned Align;
+unsigned AddrSpace;
+  };
   union {
-struct {
-  unsigned Offset;
-  unsigned Align;
-} DirectAttr;  // isDirect() || isExtend()
-struct {
-  unsigned Align;
-  unsigned AddrSpace;
-} IndirectAttr;// isIndirect()
+DirectAttrInfo DirectAttr; // isDirect() || isExtend()
+IndirectAttrInfo IndirectAttr; // isIndirect()
 unsigned AllocaFieldIndex; // isInAlloca()
   };
   Kind TheKind;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:243
+  // If b is a get() expression, then we can return a note
+  if (auto Report = bugReportOnGet(Node, BRC, RHS)) {
+return Report;

LLVM-code style mandates no curly braces around single-line ifs.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:256
+BugReporterContext , const Stmt *S, const SVal InnerPtrVal) {
+  if (const auto *DS = llvm::dyn_cast(S)) {
+const Decl *D = DS->getSingleDecl();

(I think this was already pointed out, but early-exits are the way to go in 
LLVM.

```
const auto *DS = llvm::dyn_cast(S));
if (!DS)
  return nullptr;
const Decl *D = DS->getSingleDecl();
const auto *VD = llvm::dyn_cast(D);
if (!VD)
  return nullptr;


``



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179-180
+  // P.get()
+  if (const auto *InnerCastExpr =
+  llvm::dyn_cast(Sub)) {
+Sub = InnerCastExpr->getSubExpr();

RedDocMD wrote:
> vsavchenko wrote:
> > I think it's better to `IgnoreParensAndCasts` instead of manual traversal.
> What is IgnoreParensAndCasts`? I didn't find it in the source code anywhere 
> (ripgrep, that is).
Just a typo, the actual name is `IgnoreParenCasts` (Expr::IgnoreParenCasts)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

(Tabbed on the Submit button instead of finishing that quote block...)

So I assume the stage2- targets are just invoking some ninja invocations in 
sequence?

Anyway, what I think it would be helpful to see what link jobs were in 
progress. But I guess even with 32 jobs you could end up with enough memory in 
32 linker invocations that processes start get OOM killed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96033

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D96033#2768940 , @phosek wrote:

> In D96033#2767884 , @teemperor wrote:
>
>> In D96033#2766502 , @phosek wrote:
>>
>>> In D96033#2766372 , @v.g.vassilev 
>>> wrote:
>>>
 In D96033#2766332 , @phosek wrote:

> We've started seeing `LLVM ERROR: out of memory` on our 2-stage LTO Linux 
> builders after this change landed. It looks like linking `clang-repl` 
> always fails on our bot, but I've also seen OOM when linking 
> `ClangCodeGenTests` and `FrontendTests`. Do you have any idea why this 
> could be happening? We'd appreciate any help since our bots have been 
> broken for several days now.

 Ouch. Are the bot logs public? If not maybe a stacktrace could be useful. 
 `clang-repl` combines a lot of libraries across llvm and clang that 
 usually are compiled separately. For instance we put in memory most of the 
 clang frontend, the backend and the JIT. Could it be we are hitting some 
 real limit?
>>>
>>> Yes, they are, see 
>>> https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64, but 
>>> there isn't much information in there unfortunately. It's possible that 
>>> we're hitting some limit, but these bots use 32-core instances with 128GB 
>>> RAM which I'd hope is enough even for the LTO build.
>>
>> I think the specs are fine for just building with LTO, but I am not sure if 
>> that's enough to for the worst case when running `ninja -j320` with an LTO 
>> build (which is what your job is doing). Can you try limiting your link jobs 
>> to something like 16 or 32 (e.g., `-DLLVM_PARALLEL_LINK_JOBS=32`)
>>
>> (FWIW, your go build script also crashes with OOM errors so you really are 
>> running low on memory on that node)`
>
> `-j320` is only used for the first stage compiler which uses distributed 
> compilation and no LTO, the second stage which uses LTO and where we see this 
> issue uses Ninja default, so `-j32` in this case.

I admit I don't really know the CI system on your node, but I assumed you're 
using `-j320` from this output which I got by clicking on "execution details" 
on the aborted stage of this build 
:

  Executing command [
'/b/s/w/ir/x/w/cipd/ninja',
'-j320',
'stage2-check-clang',
'stage2-check-lld',
'stage2-check-llvm',
'stage2-check-polly',
  ]
  escaped for shell: /b/s/w/ir/x/w/cipd/ninja -j320 stage2-check-clang 
stage2-check-lld stage2-check-llvm stage2-check-polly
  in dir /b/s/w/ir/x/w/staging/llvm_build
  at time 2021-05-18T20:53:37.215574


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96033

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D96033#2766502 , @phosek wrote:

> In D96033#2766372 , @v.g.vassilev 
> wrote:
>
>> In D96033#2766332 , @phosek wrote:
>>
>>> We've started seeing `LLVM ERROR: out of memory` on our 2-stage LTO Linux 
>>> builders after this change landed. It looks like linking `clang-repl` 
>>> always fails on our bot, but I've also seen OOM when linking 
>>> `ClangCodeGenTests` and `FrontendTests`. Do you have any idea why this 
>>> could be happening? We'd appreciate any help since our bots have been 
>>> broken for several days now.
>>
>> Ouch. Are the bot logs public? If not maybe a stacktrace could be useful. 
>> `clang-repl` combines a lot of libraries across llvm and clang that usually 
>> are compiled separately. For instance we put in memory most of the clang 
>> frontend, the backend and the JIT. Could it be we are hitting some real 
>> limit?
>
> Yes, they are, see 
> https://luci-milo.appspot.com/p/fuchsia/builders/prod/clang-linux-x64, but 
> there isn't much information in there unfortunately. It's possible that we're 
> hitting some limit, but these bots use 32-core instances with 128GB RAM which 
> I'd hope is enough even for the LTO build.

I think the specs are fine for just building with LTO, but I am not sure if 
that's enough to for the worst case when running `ninja -j320` with an LTO 
build (which is what your job is doing). Can you try limiting your link jobs to 
something like 16 or 32 (e.g., `-DLLVM_PARALLEL_LINK_JOBS=32`)

(FWIW, your go build script also crashes with OOM errors so you really are 
running low on memory on that node)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96033

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

[PATCH] D99946: [DebugInfo] Fix DWARF expressions for __block vars that are not on the heap

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG888ce70af288: [DebugInfo] Fix DWARF expressions for __block 
vars that are not on the heap (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99946

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-block-expr.c


Index: clang/test/CodeGen/debug-info-block-expr.c
===
--- clang/test/CodeGen/debug-info-block-expr.c
+++ clang/test/CodeGen/debug-info-block-expr.c
@@ -1,9 +1,55 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -emit-llvm -o - %s | 
FileCheck %s
+// RUN: %clang_cc1 -DDEAD_CODE -fblocks -debug-info-kind=limited -emit-llvm -o 
- %s | FileCheck %s
+
+typedef void (^BlockTy)();
+void escapeFunc(BlockTy);
+typedef void (^BlockTy)();
+void noEscapeFunc(__attribute__((noescape)) BlockTy);
+
+// Verify that the desired DIExpression are generated for escaping (i.e, not
+// 'noescape') blocks.
+void test_escape_func() {
+// CHECK-LABEL: void @test_escape_func
+// CHECK: call void @llvm.dbg.declare({{.*}}metadata ![[ESCAPE_VAR:[0-9]+]], 
metadata !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref, 
DW_OP_plus_uconst, {{[0-9]+}}){{.*}})
+  __block int escape_var;
+// Blocks in dead code branches still capture __block variables.
+#ifdef DEAD_CODE
+  if (0)
+#endif
+  escapeFunc(^{ (void)escape_var; });
+}
+
+// Verify that the desired DIExpression are generated for noescape blocks.
+void test_noescape_func() {
+// CHECK-LABEL: void @test_noescape_func
+// CHECK: call void @llvm.dbg.declare({{.*}}metadata ![[NOESCAPE_VAR:[0-9]+]], 
metadata !DIExpression())
+  __block int noescape_var;
+  noEscapeFunc(^{ (void)noescape_var; });
+}
+
 // Verify that the desired DIExpression are generated for blocks.
+void test_local_block() {
+// CHECK-LABEL: void @test_local_block
+// CHECK: call void @llvm.dbg.declare({{.*}}metadata ![[BLOCK_VAR:[0-9]+]], 
metadata !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref, 
DW_OP_plus_uconst, {{[0-9]+}}){{.*}})
+  __block int block_var;
 
-void test() {
-// CHECK: call void @llvm.dbg.declare({{.*}}!DIExpression(DW_OP_plus_uconst, 
{{[0-9]+}}, DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}){{.*}})
-  __block int i;
+// CHECK-LABEL: @__test_local_block_block_invoke
 // CHECK: call void @llvm.dbg.declare({{.*}}!DIExpression(DW_OP_deref, 
DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}, 
DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}){{.*}})
-  ^ { i = 1; }();
+  ^ { block_var = 1; }();
+}
+
+// Verify that the desired DIExpression are generated for __block vars not used
+// in any block.
+void test_unused() {
+// CHECK-LABEL: void @test_unused
+// CHECK: call void @llvm.dbg.declare({{.*}}metadata ![[UNUSED_VAR:[0-9]+]], 
metadata !DIExpression())
+  __block int unused_var;
+// Use i (not inside a block).
+  ++unused_var;
 }
+
+// CHECK: ![[ESCAPE_VAR]] = !DILocalVariable(name: "escape_var"
+// CHECK: ![[NOESCAPE_VAR]] = !DILocalVariable(name: "noescape_var"
+// CHECK: ![[BLOCK_VAR]] = !DILocalVariable(name: "block_var"
+// CHECK: ![[UNUSED_VAR]] = !DILocalVariable(name: "unused_var"
+
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4303,7 +4303,9 @@
   auto *Scope = cast(LexicalBlockStack.back());
   StringRef Name = VD->getName();
   if (!Name.empty()) {
-if (VD->hasAttr()) {
+// __block vars are stored on the heap if they are captured by a block that
+// can escape the local scope.
+if (VD->isEscapingByref()) {
   // Here, we need an offset *into* the alloca.
   CharUnits offset = CharUnits::fromQuantity(32);
   Expr.push_back(llvm::dwarf::DW_OP_plus_uconst);


Index: clang/test/CodeGen/debug-info-block-expr.c
===
--- clang/test/CodeGen/debug-info-block-expr.c
+++ clang/test/CodeGen/debug-info-block-expr.c
@@ -1,9 +1,55 @@
 // RUN: %clang_cc1 -fblocks -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DDEAD_CODE -fblocks -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+typedef void (^BlockTy)();
+void escapeFunc(BlockTy);
+typedef void (^BlockTy)();
+void noEscapeFunc(__attribute__((noescape)) BlockTy);
+
+// Verify that the desired DIExpression are generated for escaping (i.e, not
+// 'noescape') blocks.
+void test_escape_func() {
+// CHECK-LABEL: void @test_escape_func
+// CHECK: call void @llvm.dbg.declare({{.*}}metadata ![[ESCAPE_VAR:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}){{.*}})
+  __block int escape_var;
+// Blocks in dead 

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Just to be clear, I am not sure when/if D101950 
 is ready. It's mostly blocked by me not 
having the time to work on it, so depending how my schedule is this might land 
soon or far in the future. Just linked it here as this (temporary) workaround 
would be on the list of things I would revert when D101950 
 lands. But in the meantime I'm ok with this 
workaround patch as I said above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101236

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

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

By the way, I set up a WIP patch for changing LLDB's import away from the 
current MinimalImport of records to something that is more in line with how 
Clang works: https://reviews.llvm.org/D101950


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101236

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

(Obviously this should still wait on Richard & John as I think they still have 
unaddressed objections)


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

https://reviews.llvm.org/D96033

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

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

I believe everything I pointed out so far has been resolved. I still have one 
more nit about the unit test though (just to not make it fail on some Windows 
setups).

FWIW, given that this is in quite the raw state at the moment, I wonder if we 
should actually keep it out of the list of installed binaries until it reaches 
some kind of MVP state. Once this landed this otherwise gets shipped as part of 
the next clang release and shows up in people's shells.




Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:86
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}

You still need a `#ifdef GTEST_HAS_DEATH_TEST` around this (death tests aren't 
supported on some specific setups)


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

https://reviews.llvm.org/D96033

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

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Sorry for the delay. I think all my points have been resolved beside the 
insertion SourceLoc.




Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184
+  SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID())
+  .getLocWithOffset(InputCount + 2);
+

v.g.vassilev wrote:
> teemperor wrote:
> > The `+ 2` here is probably not what you want. This will just give you a 
> > pointer into Clang's source buffers and will eventually point to random 
> > source buffers (or worse) once InputCount is large enough.
> > 
> > I feel like the proper solution is to just use the StartOfFile Loc and 
> > don't add any offset to it. I think Clang should still be able to figure 
> > out a reasonable ordering for overload candidates etc. 
> > 
> > (I thought I already commented on this line before, but I can't see my 
> > comment or any replies on Phab so I'm just commenting again).
> > The `+ 2` here is probably not what you want. This will just give you a 
> > pointer into Clang's source buffers and will eventually point to random 
> > source buffers (or worse) once InputCount is large enough.
> 
> Indeed.
> 
> > 
> > I feel like the proper solution is to just use the StartOfFile Loc and 
> > don't add any offset to it. I think Clang should still be able to figure 
> > out a reasonable ordering for overload candidates etc. 
> 
> That particular part of the input processing has been causing a lot of 
> troubles for cling over the years. If we use the StartOfFile each new line 
> will appear before the previous which can be problematic for as you say 
> diagnostics but also template instantiations.
> 
> Cling ended up solving this by introducing a virtual file with impossible to 
> allocate size of `1U << 15U` 
> (https://github.com/vgvassilev/cling/blob/da1bb78f3dea4d2bf19b383aeb1872e9f2b117ad/lib/Interpreter/CIFactory.cpp#L1516-L1527
>  and 
> https://github.com/vgvassilev/cling/blob/da1bb78f3dea4d2bf19b383aeb1872e9f2b117ad/lib/Interpreter/IncrementalParser.cpp#L394)
>  Then we are save to get Loc + 1 (I do not remember how that + 2 came 
> actually) and you should be still safe.
> 
> I wonder if that's something we should do here? 
> 
> > 
> > (I thought I already commented on this line before, but I can't see my 
> > comment or any replies on Phab so I'm just commenting again).
> 
> 
I think my point then is: If we would change Clang's behaviour to consider the 
insertion order in the include tree when deciding the SourceLocation order, 
wouldn't that fix the problem? IIUC that's enough to make this case work the 
intended way without requiring some fake large source file. It would also make 
this work in other projects such as LLDB.

So IMHO we could just use StartOfFile as the loc here and then consider the 
wrong ordering as a Clang bug (and add a FIXME for that here).


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

https://reviews.llvm.org/D96033

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

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Not sure what the backtrace is for the `clang::FieldDecl::isZeroSize` crash but 
what might relevant:

1. The ASTImporter test isn't using the 'Minimal' import mode that LLDB is 
using. In the tests we are importing all declarations directly. In LLDB we use 
the 'minimal' mode where we try to import declarations lazily (but the setup 
for that is wrong, hence why it's crashing for you).
2. The ASTImporter tests aren't generating code for imported nodes (at least to 
my knowledge?), so you can't reproduce CG crashes here.

If the assert here is actually for the FieldDecl not having a complete type 
then I think the actual problem is point 1.

Looking at the code we do actually get a request to complete the type before 
the assert:

  const RecordDecl *RD = RT->getDecl()->getDefinition();
  if (!RD) {
assert(isInvalidDecl() && "valid field has incomplete type");
return false;
  }

The call to `getDefinition` is expected to pull in the definition but this 
won't happen in the current LLDB implementation (as we don't properly implement 
that interface). In the past we worked around this issue in the same way as in 
this patch, so IMHO this is fine as a temporary solution until we finally fix 
the LLDB Decl completion. But I guess it's also a question of how much LLDB 
workarounds the ASTImporter maintainers want to have in their code until this 
happens.

(Side note: I think `isInvalidDecl` is also true when we run into semantic 
errors and just mark the Decl as being malformed, so I'm not sure the assert 
text can be fully trusted here. But if eagerly completing the type fixes the 
issue then I guess it's actually the incomplete type that causes the assert to 
trigger)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101236

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

[PATCH] D99740: Avoid calling ParseCommandLineOptions in BackendUtil if possible

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG60854c328d87: Avoid calling ParseCommandLineOptions in 
BackendUtil if possible (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99740

Files:
  clang/lib/CodeGen/BackendUtil.cpp


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -871,7 +871,15 @@
 BackendArgs.push_back("-limit-float-precision");
 BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
   }
+  // Check for the default "clang" invocation that won't set any cl::opt 
values.
+  // Skip trying to parse the command line invocation to avoid the issues
+  // described below.
+  if (BackendArgs.size() == 1)
+return;
   BackendArgs.push_back(nullptr);
+  // FIXME: The command line parser below is not thread-safe and shares a 
global
+  // state, so this call might crash or overwrite the options of another Clang
+  // instance in the same process.
   llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
 BackendArgs.data());
 }


Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -871,7 +871,15 @@
 BackendArgs.push_back("-limit-float-precision");
 BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
   }
+  // Check for the default "clang" invocation that won't set any cl::opt values.
+  // Skip trying to parse the command line invocation to avoid the issues
+  // described below.
+  if (BackendArgs.size() == 1)
+return;
   BackendArgs.push_back(nullptr);
+  // FIXME: The command line parser below is not thread-safe and shares a global
+  // state, so this call might crash or overwrite the options of another Clang
+  // instance in the same process.
   llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
 BackendArgs.data());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99512: [ObjC][CodeGen] Fix missing debug info in situations where an instance and class property have the same identifier

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1cbba533ec93: [ObjC][CodeGen] Fix missing debug info in 
situations where an instance and… (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99512

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-property-class-instance-same-name.m


Index: clang/test/CodeGenObjC/debug-info-property-class-instance-same-name.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-property-class-instance-same-name.m
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -S -emit-llvm -debug-info-kind=limited %s -o - | FileCheck 
%s
+
+// Both properties should be emitted as having a class and an instance property
+// with the same name is allowed.
+@interface I1
+// CHECK: !DIObjCProperty(name: "p1"
+// CHECK-SAME:line: [[@LINE+1]]
+@property int p1;
+// CHECK: !DIObjCProperty(name: "p1"
+// CHECK-SAME:line: [[@LINE+1]]
+@property(class) int p1;
+@end
+
+@implementation I1
+@synthesize p1;
+@end
+
+void foo(I1 *iptr) {}
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2740,16 +2740,26 @@
 EltTys.push_back(PropertyNode);
   };
   {
-llvm::SmallPtrSet PropertySet;
+// Use 'char' for the isClassProperty bit as DenseSet requires space for
+// empty/tombstone keys in the data type (and bool is too small for that).
+typedef std::pair IsClassAndIdent;
+/// List of already emitted properties. Two distinct class and instance
+/// properties can share the same identifier (but not two instance
+/// properties or two class properties).
+llvm::DenseSet PropertySet;
+/// Returns the IsClassAndIdent key for the given property.
+auto GetIsClassAndIdent = [](const ObjCPropertyDecl *PD) {
+  return std::make_pair(PD->isClassProperty(), PD->getIdentifier());
+};
 for (const ObjCCategoryDecl *ClassExt : ID->known_extensions())
   for (auto *PD : ClassExt->properties()) {
-PropertySet.insert(PD->getIdentifier());
+PropertySet.insert(GetIsClassAndIdent(PD));
 AddProperty(PD);
   }
 for (const auto *PD : ID->properties()) {
   // Don't emit duplicate metadata for properties that were already in a
   // class extension.
-  if (!PropertySet.insert(PD->getIdentifier()).second)
+  if (!PropertySet.insert(GetIsClassAndIdent(PD)).second)
 continue;
   AddProperty(PD);
 }


Index: clang/test/CodeGenObjC/debug-info-property-class-instance-same-name.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-property-class-instance-same-name.m
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -S -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s
+
+// Both properties should be emitted as having a class and an instance property
+// with the same name is allowed.
+@interface I1
+// CHECK: !DIObjCProperty(name: "p1"
+// CHECK-SAME:line: [[@LINE+1]]
+@property int p1;
+// CHECK: !DIObjCProperty(name: "p1"
+// CHECK-SAME:line: [[@LINE+1]]
+@property(class) int p1;
+@end
+
+@implementation I1
+@synthesize p1;
+@end
+
+void foo(I1 *iptr) {}
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2740,16 +2740,26 @@
 EltTys.push_back(PropertyNode);
   };
   {
-llvm::SmallPtrSet PropertySet;
+// Use 'char' for the isClassProperty bit as DenseSet requires space for
+// empty/tombstone keys in the data type (and bool is too small for that).
+typedef std::pair IsClassAndIdent;
+/// List of already emitted properties. Two distinct class and instance
+/// properties can share the same identifier (but not two instance
+/// properties or two class properties).
+llvm::DenseSet PropertySet;
+/// Returns the IsClassAndIdent key for the given property.
+auto GetIsClassAndIdent = [](const ObjCPropertyDecl *PD) {
+  return std::make_pair(PD->isClassProperty(), PD->getIdentifier());
+};
 for (const ObjCCategoryDecl *ClassExt : ID->known_extensions())
   for (auto *PD : ClassExt->properties()) {
-PropertySet.insert(PD->getIdentifier());
+PropertySet.insert(GetIsClassAndIdent(PD));
 AddProperty(PD);
   }
 for (const auto *PD : ID->properties()) {
   // Don't emit duplicate metadata for properties that were already in a
   // class extension.
-  if (!PropertySet.insert(PD->getIdentifier()).second)
+  if (!PropertySet.insert(GetIsClassAndIdent(PD)).second)
 

[PATCH] D99162: [ASTImporter] Split out Objective-C related unit tests

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG274907c0a4d6: [ASTImporter] Split out Objective-C related 
unit tests (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99162

Files:
  clang/unittests/AST/ASTImporterObjCTest.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -8,6 +8,7 @@
   ASTContextParentMapTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
+  ASTImporterObjCTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterODRStrategiesTest.cpp
   ASTImporterVisibilityTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5615,59 +5615,6 @@
 2u);
 }
 
-TEST_P(ASTImporterOptionSpecificTestBase, ImplicitlyDeclareSelf) {
-  Decl *FromTU = getTuDecl(R"(
-   __attribute__((objc_root_class))
-   @interface Root
-   @end
-   @interface C : Root
- -(void)method;
-   @end
-   @implementation C
- -(void)method {}
-   @end
-   )",
-   Lang_OBJCXX, "input.mm");
-  auto *FromMethod = LastDeclMatcher().match(
-  FromTU, namedDecl(hasName("method")));
-  ASSERT_TRUE(FromMethod);
-  auto ToMethod = Import(FromMethod, Lang_OBJCXX);
-  ASSERT_TRUE(ToMethod);
-
-  // Both methods should have their implicit parameters.
-  EXPECT_TRUE(FromMethod->getSelfDecl() != nullptr);
-  EXPECT_TRUE(ToMethod->getSelfDecl() != nullptr);
-}
-
-TEST_P(ASTImporterOptionSpecificTestBase, ObjPropertyNameConflict) {
-  // Tests that properties that share the same name are correctly imported.
-  // This is only possible with one instance and one class property.
-  Decl *FromTU = getTuDecl(R"(
-   @interface DupProp{}
-   @property (class) int prop;
-   @property int prop;
-   @end
-   )",
-   Lang_OBJCXX, "input.mm");
-  auto *FromClass = FirstDeclMatcher().match(
-  FromTU, namedDecl(hasName("DupProp")));
-  auto ToClass = Import(FromClass, Lang_OBJCXX);
-  ASSERT_TRUE(ToClass);
-  // We should have one class and one instance property.
-  ASSERT_EQ(
-  1, std::distance(ToClass->classprop_begin(), ToClass->classprop_end()));
-  ASSERT_EQ(1,
-std::distance(ToClass->instprop_begin(), ToClass->instprop_end()));
-  for (clang::ObjCPropertyDecl *prop : ToClass->properties()) {
-// All properties should have a getter and a setter.
-ASSERT_TRUE(prop->getGetterMethodDecl());
-ASSERT_TRUE(prop->getSetterMethodDecl());
-// The getters/setters should be able to find the right associated property.
-ASSERT_EQ(prop->getGetterMethodDecl()->findPropertyDecl(), prop);
-ASSERT_EQ(prop->getSetterMethodDecl()->findPropertyDecl(), prop);
-  }
-}
-
 struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
Index: clang/unittests/AST/ASTImporterObjCTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTImporterObjCTest.cpp
@@ -0,0 +1,89 @@
+//===- unittest/AST/ASTImporterObjCTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests for the correct import of AST nodes related to Objective-C and
+// Objective-C++.
+//
+//===--===//
+
+#include "clang/AST/DeclContextInternals.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+#include "ASTImporterFixtures.h"
+
+using namespace clang::ast_matchers;
+using namespace clang;
+
+namespace {
+struct ImportObjCDecl : ASTImporterOptionSpecificTestBase {};
+} // namespace
+
+TEST_P(ImportObjCDecl, ImplicitlyDeclareSelf) {
+  Decl *FromTU = getTuDecl(R"(
+   __attribute__((objc_root_class))
+   @interface Root
+   @end
+  

[PATCH] D99077: [ASTImporter] Fix import of ObjCPropertyDecl that share the same name

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe421a74108ee: [ASTImporter] Fix import of ObjCPropertyDecl 
that share the same name (authored by teemperor).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99077

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5639,6 +5639,35 @@
   EXPECT_TRUE(ToMethod->getSelfDecl() != nullptr);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ObjPropertyNameConflict) {
+  // Tests that properties that share the same name are correctly imported.
+  // This is only possible with one instance and one class property.
+  Decl *FromTU = getTuDecl(R"(
+   @interface DupProp{}
+   @property (class) int prop;
+   @property int prop;
+   @end
+   )",
+   Lang_OBJCXX, "input.mm");
+  auto *FromClass = FirstDeclMatcher().match(
+  FromTU, namedDecl(hasName("DupProp")));
+  auto ToClass = Import(FromClass, Lang_OBJCXX);
+  ASSERT_TRUE(ToClass);
+  // We should have one class and one instance property.
+  ASSERT_EQ(
+  1, std::distance(ToClass->classprop_begin(), ToClass->classprop_end()));
+  ASSERT_EQ(1,
+std::distance(ToClass->instprop_begin(), ToClass->instprop_end()));
+  for (clang::ObjCPropertyDecl *prop : ToClass->properties()) {
+// All properties should have a getter and a setter.
+ASSERT_TRUE(prop->getGetterMethodDecl());
+ASSERT_TRUE(prop->getSetterMethodDecl());
+// The getters/setters should be able to find the right associated 
property.
+ASSERT_EQ(prop->getGetterMethodDecl()->findPropertyDecl(), prop);
+ASSERT_EQ(prop->getSetterMethodDecl()->findPropertyDecl(), prop);
+  }
+}
+
 struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5066,6 +5066,11 @@
   auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
   for (auto *FoundDecl : FoundDecls) {
 if (auto *FoundProp = dyn_cast(FoundDecl)) {
+  // Instance and class properties can share the same name but are 
different
+  // declarations.
+  if (FoundProp->isInstanceProperty() != D->isInstanceProperty())
+continue;
+
   // Check property types.
   if (!Importer.IsStructurallyEquivalent(D->getType(),
  FoundProp->getType())) {


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5639,6 +5639,35 @@
   EXPECT_TRUE(ToMethod->getSelfDecl() != nullptr);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ObjPropertyNameConflict) {
+  // Tests that properties that share the same name are correctly imported.
+  // This is only possible with one instance and one class property.
+  Decl *FromTU = getTuDecl(R"(
+   @interface DupProp{}
+   @property (class) int prop;
+   @property int prop;
+   @end
+   )",
+   Lang_OBJCXX, "input.mm");
+  auto *FromClass = FirstDeclMatcher().match(
+  FromTU, namedDecl(hasName("DupProp")));
+  auto ToClass = Import(FromClass, Lang_OBJCXX);
+  ASSERT_TRUE(ToClass);
+  // We should have one class and one instance property.
+  ASSERT_EQ(
+  1, std::distance(ToClass->classprop_begin(), ToClass->classprop_end()));
+  ASSERT_EQ(1,
+std::distance(ToClass->instprop_begin(), ToClass->instprop_end()));
+  for (clang::ObjCPropertyDecl *prop : ToClass->properties()) {
+// All properties should have a getter and a setter.
+ASSERT_TRUE(prop->getGetterMethodDecl());
+ASSERT_TRUE(prop->getSetterMethodDecl());
+// The getters/setters should be able to find the right associated property.
+ASSERT_EQ(prop->getGetterMethodDecl()->findPropertyDecl(), prop);
+ASSERT_EQ(prop->getSetterMethodDecl()->findPropertyDecl(), prop);
+  }
+}
+
 struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ 

[PATCH] D98951: [clang][ASTImporter] Add import API for 'const Type *' (NFC).

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: whisperity.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98951

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

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

It seems we already hit this issue before and decided to add a workaround: 
https://reviews.llvm.org/D86823


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97850

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

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Sorry for the delay. Yes, I meant the `--vg` flag which is I think the only 
valid way to test this from your description. That it can't be 
deterministically tested without this (or in configurations that don't run 
valgrind) is fine IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97849

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

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

If I understand this correctly, then the inode caching logic in FileManager 
isn't just breaking PCMs but all file operations:

  TEST_F(FileManagerTest, InodeReuse) {
{
  std::ofstream myfile;
  myfile.open("a.cpp");
  myfile << "content\n";
}
llvm::ErrorOr fe1 = manager.getFile("a.cpp");
EXPECT_TRUE(fe1);
const FileEntry *f1 = *fe1;
remove("a.cpp");
{
  std::ofstream myfile;
  myfile.open("b.cpp");
  myfile << "different content\n";
}
llvm::ErrorOr fe2 = manager.getFile("b.cpp");
EXPECT_TRUE(fe2);
const FileEntry *f2 = *fe2;
EXPECT_NE(f2->getSize(), f1->getSize());
EXPECT_NE(f2->getUniqueID().getFile(), f1->getUniqueID().getFile());
  }

This fails consistently for me when running in an empty ext4 directory with:

  Expected: (f2->getSize()) != (f1->getSize()), actual: 8 vs 8
  Expected: (f2->getUniqueID().getFile()) != (f1->getUniqueID().getFile()), 
actual: 57855544 vs 57855544

I guess this wasn't considered a valid use case for the normal `#include` logic 
within Clang (which I believe is the primary beneficiary of this cache and 
doesn't really care about a changing file system). But with PCMs and Clang 
REPLs this is probably causing some strange bugs.

Anyway, I don't think I know the FileManager code well enough to come up with a 
real fix (beside just removing the inode cache).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97850

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

[PATCH] D97849: [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

We can (and do) run LIT tests under valgrind, so if you have a test that 
triggers this valgrind error then that seems like the way to go. I don't think 
the test has to deterministically fail outside valgrind to be a valid test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97849

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

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

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa54f160b3a98: Prefer /usr/bin/env xxx over /usr/bin/xxx 
where xxx = perl, python, awk (authored by haampie, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95119

Files:
  clang/test/make_test_dirs.pl
  clang/tools/scan-build/bin/set-xcode-analyzer
  clang/utils/TestUtils/pch-test.pl
  clang/utils/analyzer/reducer.pl
  clang/utils/analyzer/update_plist_test.pl
  clang/www/demo/index.cgi
  debuginfo-tests/llgdb-tests/test_debuginfo.pl
  lldb/docs/use/python-reference.rst
  lldb/scripts/disasm-gdb-remote.pl
  llvm/utils/GenLibDeps.pl
  llvm/utils/codegen-diff
  llvm/utils/findsym.pl
  llvm/utils/llvm-compilers-check
  llvm/utils/llvm-native-gxx
  openmp/runtime/tools/check-execstack.pl
  openmp/runtime/tools/check-instruction-set.pl
  openmp/runtime/tools/message-converter.pl
  polly/lib/External/isl/doc/mypod2latex

Index: polly/lib/External/isl/doc/mypod2latex
===
--- polly/lib/External/isl/doc/mypod2latex
+++ polly/lib/External/isl/doc/mypod2latex
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 use Pod::LaTeX;
Index: openmp/runtime/tools/message-converter.pl
===
--- openmp/runtime/tools/message-converter.pl
+++ openmp/runtime/tools/message-converter.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 #
 #//===--===//
Index: openmp/runtime/tools/check-instruction-set.pl
===
--- openmp/runtime/tools/check-instruction-set.pl
+++ openmp/runtime/tools/check-instruction-set.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 #
 #//===--===//
Index: openmp/runtime/tools/check-execstack.pl
===
--- openmp/runtime/tools/check-execstack.pl
+++ openmp/runtime/tools/check-execstack.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 #
 #//===--===//
Index: llvm/utils/llvm-native-gxx
===
--- llvm/utils/llvm-native-gxx
+++ llvm/utils/llvm-native-gxx
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 # Wrapper around LLVM tools to generate a native .o from llvm-gxx using an
 # LLVM back-end (CBE by default).
 
Index: llvm/utils/llvm-compilers-check
===
--- llvm/utils/llvm-compilers-check
+++ llvm/utils/llvm-compilers-check
@@ -1,4 +1,4 @@
-#!/usr/bin/python3
+#!/usr/bin/env python3
 ##===- utils/llvmbuild - Build the LLVM project *-python-*-===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
Index: llvm/utils/findsym.pl
===
--- llvm/utils/findsym.pl
+++ llvm/utils/findsym.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/env perl
 #
 # Program:  findsym.pl
 #
@@ -8,6 +8,8 @@
 # Syntax:   findsym.pl  
 #
 
+use warnings;
+
 # Give first option a name.
 my $Directory = $ARGV[0];
 my $Symbol = $ARGV[1];
Index: llvm/utils/codegen-diff
===
--- llvm/utils/codegen-diff
+++ llvm/utils/codegen-diff
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use Getopt::Std;
 $DEBUG = 0;
Index: llvm/utils/GenLibDeps.pl
===
--- llvm/utils/GenLibDeps.pl
+++ llvm/utils/GenLibDeps.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/env perl
 #
 # Program:  GenLibDeps.pl
 #
Index: lldb/scripts/disasm-gdb-remote.pl
===
--- lldb/scripts/disasm-gdb-remote.pl
+++ lldb/scripts/disasm-gdb-remote.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 
 use strict;
 
Index: lldb/docs/use/python-reference.rst
===
--- lldb/docs/use/python-reference.rst
+++ lldb/docs/use/python-reference.rst
@@ -607,7 +607,7 @@
 
 ::
 
-  #!/usr/bin/python
+  #!/usr/bin/env python
 
   import lldb
   import commands
@@ -715,7 +715,7 @@
 
 ::
 
-  #!/usr/bin/python
+  #!/usr/bin/env python
 
   import lldb
   import os
Index: debuginfo-tests/llgdb-tests/test_debuginfo.pl
===
--- debuginfo-tests/llgdb-tests/test_debuginfo.pl
+++ debuginfo-tests/llgdb-tests/test_debuginfo.pl
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/env perl
 #
 # This script tests debugging information generated by a compiler.
 # Input 

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184
+  SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID())
+  .getLocWithOffset(InputCount + 2);
+

The `+ 2` here is probably not what you want. This will just give you a pointer 
into Clang's source buffers and will eventually point to random source buffers 
(or worse) once InputCount is large enough.

I feel like the proper solution is to just use the StartOfFile Loc and don't 
add any offset to it. I think Clang should still be able to figure out a 
reasonable ordering for overload candidates etc. 

(I thought I already commented on this line before, but I can't see my comment 
or any replies on Phab so I'm just commenting again).



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:63
+if (CI.hasCodeCompletionConsumer())
+  CompletionConsumer = ();
+

v.g.vassilev wrote:
> teemperor wrote:
> > Can this completion code even be used? It doesn't look like it can (and I'm 
> > not sure if using the `CodeCompletionAt` flag is really useful in a REPL as 
> > you can only specify it once during startup). IMHO this can be left out 
> > until we actually can hook up this into the LineEditor (and we have a way 
> > to test this).
> Cling, on which this patch is based on, has a CodeCompleteConsumer and it 
> seems to work.
> 
> I can leave it out but then we will have to remember where to put it. I have 
> a slight preference to leave it as it is.
Alright, given that this is just passing along the CompletionConsumer from Ci 
to Sema, I think this can stay then.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:189
+  // NewLoc only used for diags.
+  PP.EnterSourceFile(FID, /*DirLookup*/ 0, NewLoc);
+

v.g.vassilev wrote:
> teemperor wrote:
> > I think we could assert that this succeeds.
> assert on the FID you mean?
I meant the `EnterSourceFile` call has a `bool` error it returns (along with a 
diagnostic). I think the only way it should fail is if the previous code got 
somehow messed up, hence the assert suggestion.



Comment at: clang/lib/Interpreter/Interpreter.cpp:82
+  PCHOps->registerWriter(std::make_unique());
+  PCHOps->registerReader(std::make_unique());
+

v.g.vassilev wrote:
> teemperor wrote:
> > Can you add a FIXME that this should be removed (as I think we keep adding 
> > these two lines to all kind of Clang tools)? I assume this is only needed 
> > because of Clang errors when you encounter -gmodules produced pcms ?
> I do not understand the problem entirely, could you propose wording for the 
> FIXME?
The PCM files generated with `-gmodules` are object-files that contain the 
Clang AST inside them. To deal with the object-file wrapping and get to the AST 
inside, we need the `ObjectFilePCHContainerReader`. But that class is part of 
CodeGen which isn't a dependency of the normal parsing logic, so these classes 
can't be hooked up automatically by Clang like the other 'ContainerReaders' 
classes (well, there is only one other classes which just opens the file 
normally).

So to work around the fact that to **parse** code we now need a **CodeGen** 
dependency, every clang tool (which usually depend on CodeGen + parsing code) 
has to manually register the `ObjectFilePCHContainer*` classes.

My point is just that it's not sustainable to have everyone copy around these 
three lines as otherwise Clang won't handle any PCM file that was generated 
with -gmodules.

I think the FIXME for this could be:
`FIXME: Clang should register these container operations automatically.`.



Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support

v.g.vassilev wrote:
> teemperor wrote:
> > v.g.vassilev wrote:
> > > teemperor wrote:
> > > > Commented out by mistake?
> > > Does not seem to be required. I will remove it for now...
> > I meant that shouldn't have been commented out (as I think we don't get the 
> > targets symbols from any other library so this fails to link)
> It compiles just fine for me. Should we add it back once we find the setup 
> that requires it?
On my Linux setup the dependencies were required (and it seems logical that 
they are required), so I would just add them.


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

https://reviews.llvm.org/D96033

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

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

LGTM.

Regarding the LLDB example: Given that the LLDB API is in theory not bound to 
the semantics a specific language, I think one can argue that `IsAnonymousType` 
could also return true for unnamed classes. The use case that triggered this 
whole discussion was someone who wanted to know whether the type has a name, so 
calling this function seems logical. Given that we don't really have a valid 
type name for unnamed classes, we might want to change the implementation of 
`IsAnonymousType` to also return true for unnamed classes. But all that is out 
of scope for this patch which is about fixing the type printer in Clang. Just 
bringing this up as it's the example in the patch description.


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

https://reviews.llvm.org/D96807

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

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D96427#2555251 , @JDevlieghere 
wrote:

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

Maybe it used the system dsymutil or something? (or maybe it worked by accident 
in the Xcode setup)

Anyway, I wonder if we could have some (low frequency) bot that could test this 
configuration? There is the LLDB+Xcode one and I have my own standalone build 
bot, but I guess we would need a Visual studio one too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96427

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

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

[Raphael Isemann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e3b9aba609f: [Timer] On macOS count number of executed 
instructions (authored by ahoppen, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96049

Files:
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-store-check-profile-one-tu.cpp
  llvm/CMakeLists.txt
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/Timer.h
  llvm/lib/Support/Timer.cpp

Index: llvm/lib/Support/Timer.cpp
===
--- llvm/lib/Support/Timer.cpp
+++ llvm/lib/Support/Timer.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Timer.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
@@ -24,6 +25,14 @@
 #include "llvm/Support/raw_ostream.h"
 #include 
 
+#if HAVE_UNISTD_H
+#include 
+#endif
+
+#ifdef HAVE_PROC_PID_RUSAGE
+#include 
+#endif
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -120,6 +129,17 @@
   return sys::Process::GetMallocUsage();
 }
 
+static uint64_t getCurInstructionsExecuted() {
+#if defined(HAVE_UNISTD_H) && defined(HAVE_PROC_PID_RUSAGE) && \
+defined(RUSAGE_INFO_V4)
+  struct rusage_info_v4 ru;
+  if (proc_pid_rusage(getpid(), RUSAGE_INFO_V4, (rusage_info_t *)) == 0) {
+return ru.ri_instructions;
+  }
+#endif
+  return 0;
+}
+
 TimeRecord TimeRecord::getCurrentTime(bool Start) {
   using Seconds = std::chrono::duration>;
   TimeRecord Result;
@@ -128,9 +148,11 @@
 
   if (Start) {
 Result.MemUsed = getMemUsage();
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 sys::Process::GetTimeUsage(now, user, sys);
   } else {
 sys::Process::GetTimeUsage(now, user, sys);
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 Result.MemUsed = getMemUsage();
   }
 
@@ -180,6 +202,8 @@
 
   if (Total.getMemUsed())
 OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
+  if (Total.getInstructionsExecuted())
+OS << format("%9" PRId64 "  ", (int64_t)getInstructionsExecuted());
 }
 
 
@@ -339,6 +363,8 @@
   OS << "   ---Wall Time---";
   if (Total.getMemUsed())
 OS << "  ---Mem---";
+  if (Total.getInstructionsExecuted())
+OS << "  ---Instr---";
   OS << "  --- Name ---\n";
 
   // Loop through all of the timing data, printing it out.
@@ -433,6 +459,10 @@
   OS << delim;
   printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
+if (T.getInstructionsExecuted()) {
+  OS << delim;
+  printJSONValue(OS, R, ".instr", T.getInstructionsExecuted());
+}
   }
   TimersToPrint.clear();
   return delim;
Index: llvm/include/llvm/Support/Timer.h
===
--- llvm/include/llvm/Support/Timer.h
+++ llvm/include/llvm/Support/Timer.h
@@ -24,12 +24,15 @@
 class raw_ostream;
 
 class TimeRecord {
-  double WallTime;   ///< Wall clock time elapsed in seconds.
-  double UserTime;   ///< User time elapsed.
-  double SystemTime; ///< System time elapsed.
-  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  double WallTime;   ///< Wall clock time elapsed in seconds.
+  double UserTime;   ///< User time elapsed.
+  double SystemTime; ///< System time elapsed.
+  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  uint64_t InstructionsExecuted; ///< Number of instructions executed
 public:
-  TimeRecord() : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0) {}
+  TimeRecord()
+  : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0),
+InstructionsExecuted(0) {}
 
   /// Get the current time and memory usage.  If Start is true we get the memory
   /// usage before the time, otherwise we get time before memory usage.  This
@@ -42,6 +45,7 @@
   double getSystemTime() const { return SystemTime; }
   double getWallTime() const { return WallTime; }
   ssize_t getMemUsed() const { return MemUsed; }
+  uint64_t getInstructionsExecuted() const { return InstructionsExecuted; }
 
   bool operator<(const TimeRecord ) const {
 // Sort by Wall Time elapsed, as it is the only thing really accurate
@@ -49,16 +53,18 @@
   }
 
   void operator+=(const TimeRecord ) {
-WallTime   += RHS.WallTime;
-UserTime   += RHS.UserTime;
+WallTime += RHS.WallTime;
+UserTime += RHS.UserTime;
 SystemTime += RHS.SystemTime;
-MemUsed+= RHS.MemUsed;
+MemUsed += RHS.MemUsed;
+InstructionsExecuted += RHS.InstructionsExecuted;
   }
   void operator-=(const TimeRecord ) {
-WallTime   -= RHS.WallTime;
-UserTime   -= RHS.UserTime;
+WallTime -= 

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a reviewer: sgraenitz.
teemperor added a comment.

Another round of comments.

Also a general note that applies to a bunch of places, if you specify the 
function names in comments when calling a function, you have to add a trailing 
`=` so that clang-tidy can actually check this for us 
::

  SomeFunction(/*ArgName*/ 0); // wrong
  SomeFunction(/*ArgName=*/ 0); // right

I think someone who knows more about the JIT should review the JIT-related code 
(I'm just gonna add Stefan too). And the changes related to the FrontendActions 
should also be signed off by someone else as I rarely touch that code. Not sure 
who though.

Beside that I think we're getting there, thanks!




Comment at: clang/include/clang/Frontend/FrontendAction.h:238
   /// objects, and run statistics and output file cleanup code.
-  void EndSourceFile();
+  virtual void EndSourceFile();
 

I think this and the change above requires updating `WrapperFrontendAction` to 
also forward these functions to the wrapped action. Otherwise wrapping actions 
would change their behaviour. See the other virtual functions there.



Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ 
-*-===//
+//

v.g.vassilev wrote:
> teemperor wrote:
> > Could this whole file just be part of `IncrementalParser.h` which is the 
> > only user ? `clang::Transaction` seems anyway a bit of a generic name, so 
> > maybe this could become `clang::IncrementalParser::Transaction` then.
> The intent is to expose the Transaction class and if I move it in the 
> `IncrementalParser` I will have to expose the `IncrementalParser.h`. Would it 
> make sense to move it as part of the Interpreter object? Eg. 
> `clang::Interpreter::Transaction`?
Makes sense. I guess moving this into the Interpreter would create some strange 
dependencies, so I would say we keep it in its own file. Maybe we could put it 
into a `interpreter` namespace or give it a more unique name 
`InterpreterTransaction`?

I also think this should have some documentation.



Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+  llvm::Error addModule(std::unique_ptr M);
+  llvm::Error runCtors() const;
+};

Should we maybe merge `runCtors` and `addModule`? Not sure if there is a use 
case for adding a Module but not running Ctors. Also documentation.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:63
+if (CI.hasCodeCompletionConsumer())
+  CompletionConsumer = ();
+

Can this completion code even be used? It doesn't look like it can (and I'm not 
sure if using the `CodeCompletionAt` flag is really useful in a REPL as you can 
only specify it once during startup). IMHO this can be left out until we 
actually can hook up this into the LineEditor (and we have a way to test this).



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:69
+Preprocessor  = CI.getPreprocessor();
+PP.enableIncrementalProcessing();
+PP.EnterMainSourceFile();

Could we do that before creating the Sema? Sema's constructor is doing quite a 
few things and some of them might depend on this setting in the future.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:73
+
+  void EndSourceFile() override {
+if (IsTerminating) {

I assume this function intercepts all EndSourceFile calls so that the 
preprocessor is 'stuck' in the main file even if it reaches the end of the user 
input so far? Might be worth a comment.



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:74
+  void EndSourceFile() override {
+if (IsTerminating) {
+  WrapperFrontendAction::EndSourceFile();

No `{ .. }` 



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:101
+  // later errors use the default handling behavior instead.
+  llvm::remove_fatal_error_handler();
+}

I think this should be in ClangRepl.cpp where we actually install the handler 
(I wouldn't expect that creating an `IncrementalParser` object would touch the 
global error handler).



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:136
+Consumer->HandleTopLevelDecl(DGR);
+  }
+

Could we test this?



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:165
+  std::ostringstream SourceName;
+  SourceName << "input_line_" << InputCount++;
+

I think this is OK for this patch, but if the file is called "input_line_" then 
Clang would still emit the actual line information (which is 1) and we get 
`input_line_1:1:9: error: ...` diagnostics. Since D83038 Clang can hide line 
numbers in diagnostics, so we could selectively turn this setting 

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

A more general comment: You have to `std::move` your `llvm::Error` variables 
when the result is `llvm::Expected` (otherwise this won't compile).




Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ 
-*-===//
+//

Could this whole file just be part of `IncrementalParser.h` which is the only 
user ? `clang::Transaction` seems anyway a bit of a generic name, so maybe this 
could become `clang::IncrementalParser::Transaction` then.



Comment at: clang/include/clang/Interpreter/Transaction.h:10
+// This file defines utilities tracking the incrementally processed pieces of
+// code
+//

missing `.`.



Comment at: clang/include/clang/Parse/Parser.h:2403
   }
-
+private:
   /// isForInitDeclaration - Disambiguates between a declaration or an

This doesn't seem to be needed for anything?



Comment at: clang/lib/Interpreter/IncrementalParser.cpp:175
+  memcpy(MBStart, input.data(), InputSize);
+  memcpy(MBStart + InputSize, "\n", 2);
+

I think overwriting the \0 in the buffer isn't necessary. So 
`MBStart[InputSize] = '\n';` should be enough.



Comment at: clang/lib/Interpreter/Interpreter.cpp:43
+#include "llvm/Support/Host.h"
+//
+

I think the comment here/above and some of the empty lines aren't really needed 
here.



Comment at: clang/lib/Interpreter/Interpreter.cpp:140
+
+  if (std::find(ClangArgv.begin(), ClangArgv.end(), " -x") == ClangArgv.end()) 
{
+// We do C++ by default; append right after argv[0] if no "-x" given

`llvm::find(ClangArgv, " -x")`



Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support

Commented out by mistake?



Comment at: clang/unittests/CodeGen/IncrementalProcessingTest.cpp:56-59
+  std::vector ClangArgs = {"-Xclang", "-emit-llvm-only"};
+  std::vector ClangArgv(ClangArgs.size());
+  std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
+ [](const std::string ) -> const char * { return s.data(); 
});





Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:69
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}

I think that's usually used for testing asserts but here it's an invalid memory 
access (which might even work out just if the stars align correctly).

What about either:
1. Shutting down clang-repl cleanly after we hit a diagnostic.
2. Making an assert that we can't codegen a TU that already had any error 
diagnostic (in which case you can surround it with the following):

```
#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D96033

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

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I believe rsmith is in GMT-8, so I assume this won't get a fix soon and I went 
ahead and reverted in 22ccdb787024e954318e35fcf904fd4fa36f5679 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91488

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

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Raphael Isemann via Phabricator via cfe-commits]

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

There are several LLDB tests failing with this change. The 
TestImportBaseClassWhenClassHasDerivedMember.py is probably the only relevant 
one. The others are probably failing for the same reason but with a more 
convoluted setup. I looked into the failure and it seems this patch is now 
skipping the workaround in `ImportContext` that was introduced in D78000 
 (as the `Importer.GetAlreadyImportedOrNull` 
will give us a DeclContext and then we just use that as-is). If you remove the 
`if (!DC || !LexicalDC)` before the `if ((Err = ImportDeclContext(D, DC, 
LexicalDC)))` this should keep the old behaviour.

Beside that issue this LGTM. Removing the 'if' seems straightforward, so I'll 
accept this. Thanks!




Comment at: clang/lib/AST/ASTImporter.cpp:2522
+  // Add to the lookupTable because we could not do that in MapImported.
+  Importer.AddToLookupTable(ToTypedef);
+

martong wrote:
> shafik wrote:
> > I am not super excited about this solution, I feel like several bugs we 
> > have had can be attributed to these exception control flow cases that we 
> > have in the ASTImporter. I don't have any immediate better solution.
> > 
> > 
> Before uploading this patch I had been thinking about several other solutions
> but all of them had some problems:
> 
> 1) 
> Detect the loop in the AST and return with UnsupportedConstruct. We could do
> the detection with the help of ImportPath. However, I realized this approach 
> is
> way too defensive and removes the support for an important AST node which is
> against our goals.
> 
> 2) 
> Try to eliminate the looping contruct from the AST. I could do that for some
> cases (D92101) but there are still cases when the Sema must create such a loop
> deliberately (the test case in this patch shows that).
> 
> It is essential to add a newly created Decl to the lookupTable ASAP because it
> makes it possible for subsequent imports to find the existing Decl and this 
> way
> avoiding the creation of duplicated Decls. This is why AddToLookupTable is
> called in MapImported. But the lookuptable operates on the DeclContext of the
> Decl, and in this patch we must not import the DeclContext before.
> Consequently, we have to add to the loopkuptable once the DeclContext is
> imported. Perhaps, we could provide an optional lambda function to
> GetImportedOrCreateDecl which would be called before MapImported (and that
> lambda would do the DC import), but this seems even more clumsy.
> 
> BTW, the lookuptable is not used in LLDB, there you use the traditional lookup
> implemented in DeclContext (addDeclInternal and noload_lookup). One problem
> with noload_lookup is that it does not find some Decls because it obeys to C++
> visibility rules, thus new duplicated Decls are created. The ASTImporter must
> be able to lookup e.g. template specialization Decls too, in order to avoid
> creating a redundant duplicated clone and this is the task of the lookupTable.
> I hope one day LLDB would be able to switch to ASTImporterLookupTable from
> noload_lookup. The other problem is with addDeclInternal: we call this later,
> not in MapImported. The only responsibility attached to the use of 
> addDeclInternal 
> should be to clone the visibility as it is in the source context (and not 
> managing 
> the do-not-create-duplicate-decls issue).
> 
> So yes, there are many exceptional control flow cases, but most of them stems
> from the complexity of trying to support two different needs: noload_lookup 
> and
> minimal import are needed exceptionally for LLDB. I was thinking about to
> create a separate ASTImporter implementation specifically for CTU and LLDB
> could have it's own implementation. For that we just need to create an
> interface class with virtual functions. Having two different implementations
> could save us from braking each others tests and this could be a big gain, 
> WDYT?
> (On the other hand, we'd loose updates and fixes from the other team.)
> 
> I hope one day LLDB would be able to switch to ASTImporterLookupTable from 
> noload_lookup.

Done here: https://reviews.llvm.org/D92848



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5932
+typedef T U;
+A(U, T);
+  };

Second parameter seems not relevant for the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92209

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

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D92209#2437416 , @martong wrote:

> Ping.
> @teemperor please take a look if you have some time. This is a really 
> important patch which may influence some future directions regarding the 
> ASTImporter.

Apologies, last week was very busy here. I'll review this tomorrow morning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92209

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

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Having the comparison code would be nice. The custom types in the associated 
types aren't compared by the generic code, so an overload like this in the 
`StmtComparer` class should be enough:

  bool IsStmtEquivalent(const GenericSelectionExpr *E1, const 
GenericSelectionExpr *E2) {
for (auto Pair : zip_longest(E1->getAssocTypeSourceInfos(),
 E2->getAssocTypeSourceInfos())) {
  Optional Child1 = std::get<0>(Pair);
  Optional Child2 = std::get<1>(Pair);
  // Different number of associated types.
  if (!Child1 || !Child2)
return false;
  
  if (!IsStructurallyEquivalent(Context, (*Child1)->getType(),
(*Child2)->getType()))
return false;
}
return true;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92600

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

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D92103#2428139 , @martong wrote:

> Hey Raphael, thanks for looking into the CTU crash!
>
> I also had a look and I could reproduce that on Linux Ubuntu 18.04 with GCC 
> 7. I think the discrepancy stems from GCC's libstdc++ and MacOS's libc++ 
> implementation differences of `basic_streambuf`. This is the CXXRecordDecl 
> which does not need an injected class name type (so there is the assert). 
> However, the situation is more complex because there is a 5 long redecl chain 
> (streambuf is forward declared in many places).
>
> Anyway, I don't think that this patch itself caused the crash, it just 
> revealed some other issues that are related to the possibly flawed import 
> logic of injected class (name) types. I am adding another college @balazske 
> as a subscriber because he started to investigate the crash deeper.

Quick note: I should have mention that I actually tried reproducing this issue 
with libstdc++/GCC 10.2.0 on Arch Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

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

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

@gamesh411 I recreated the setup you listed (thanks for that btw) but for me 
this works just fine. I assume the crash happens in a class template from one 
of the external libraries. It probably works for me because I don't have the 
same library version as you have, but just from the backtrace it's hard to know 
where the error could come from.

I'm not sure if the XTU analyzer has a way to create reproducers (maybe 
@martong knows), but if you could apply the patch below, recompile/run the 
analyzer and post the output that would help a lot with figuring out what code 
on your system is causing the crash:

  diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
  index 67ee8c0956d6..a471501aa71e 100644
  --- a/clang/lib/AST/ASTContext.cpp
  +++ b/clang/lib/AST/ASTContext.cpp
  @@ -4408,6 +4408,9 @@ static bool NeedsInjectedClassNameType(const RecordDecl 
*D) {
   /// injected class name type for the specified templated declaration.
   QualType ASTContext::getInjectedClassNameType(CXXRecordDecl *Decl,
 QualType TST) const {
  +  if (!NeedsInjectedClassNameType(Decl)) {
  +Decl->dumpColor();
  +  }
 assert(NeedsInjectedClassNameType(Decl));
 if (Decl->TypeForDecl) {
   assert(isa(Decl->TypeForDecl));

The output from this code is probably colorized in your terminal and looks a 
bit like this:

  |-ClassTemplateDecl 0x7ff94c04a0a0  line:1:36 A
  | |-TemplateTypeParmDecl 0x7ff94c049f50  col:26 referenced 
typename depth 0 index 0 T
  | |-CXXRecordDecl 0x7ff94c04a010  line:1:36 struct A 
definition
  | | |-DefinitionData empty standard_layout trivially_copyable 
has_user_declared_ctor can_const_default_init
  | | | |-DefaultConstructor defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  [...]

FWIW, I don't think the patch itself introduces this regression, but it just 
causes more of the AST to be imported (and the import of these AST nodes then 
runs into an unsupported use case). This would most likely already crash if the 
analysed source code referenced that class template in some other way.




Comment at: 
lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py:25
 
-deque_type = "std::deque >"
 size_type = deque_type + "::size_type"

shafik wrote:
> Why do the default arguments not show up in the results anymore?
Because we don't show args that match the default arg in the display type 
(which is identical to what Clang does). See also rdar://59292534


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

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

[PATCH] D92106: [ASTImporter] Import the default argument of NonTypeTemplateParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG89c1a7a67d69: [ASTImporter] Import the default argument of 
NonTypeTemplateParmDecl (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D92106?vs=307862=307999#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92106

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -953,6 +953,27 @@
   ASSERT_EQ(ToTemplate, ToExpectedDecl);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, NonTypeTemplateParmDeclNoDefaultArg) 
{
+  Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, nonTypeTemplateParmDecl(hasName("N")));
+  NonTypeTemplateParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, NonTypeTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, nonTypeTemplateParmDecl(hasName("S")));
+  NonTypeTemplateParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  Stmt *ToArg = To->getDefaultArgument();
+  ASSERT_TRUE(isa(ToArg));
+  ToArg = *ToArg->child_begin();
+  ASSERT_TRUE(isa(ToArg));
+  ASSERT_EQ(cast(ToArg)->getValue().getLimitedValue(), 1U);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX03);
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5227,15 +5227,22 @@
   if (Err)
 return std::move(Err);
 
-  // FIXME: Import default argument.
-
   NonTypeTemplateParmDecl *ToD = nullptr;
-  (void)GetImportedOrCreateDecl(
-  ToD, D, Importer.getToContext(),
-  Importer.getToContext().getTranslationUnitDecl(),
-  ToInnerLocStart, ToLocation, D->getDepth(),
-  D->getPosition(), ToDeclName.getAsIdentifierInfo(), ToType,
-  D->isParameterPack(), ToTypeSourceInfo);
+  if (GetImportedOrCreateDecl(ToD, D, Importer.getToContext(),
+  Importer.getToContext().getTranslationUnitDecl(),
+  ToInnerLocStart, ToLocation, D->getDepth(),
+  D->getPosition(),
+  ToDeclName.getAsIdentifierInfo(), ToType,
+  D->isParameterPack(), ToTypeSourceInfo))
+return ToD;
+
+  if (D->hasDefaultArgument()) {
+ExpectedExpr ToDefaultArgOrErr = import(D->getDefaultArgument());
+if (!ToDefaultArgOrErr)
+  return ToDefaultArgOrErr.takeError();
+ToD->setDefaultArgument(*ToDefaultArgOrErr);
+  }
+
   return ToD;
 }
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -953,6 +953,27 @@
   ASSERT_EQ(ToTemplate, ToExpectedDecl);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, NonTypeTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, nonTypeTemplateParmDecl(hasName("N")));
+  NonTypeTemplateParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, NonTypeTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03);
+  auto From = FirstDeclMatcher().match(
+  FromTU, nonTypeTemplateParmDecl(hasName("S")));
+  NonTypeTemplateParmDecl *To = Import(From, Lang_CXX03);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  Stmt *ToArg = To->getDefaultArgument();
+  ASSERT_TRUE(isa(ToArg));
+  ToArg = *ToArg->child_begin();
+  ASSERT_TRUE(isa(ToArg));
+  ASSERT_EQ(cast(ToArg)->getValue().getLimitedValue(), 1U);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX03);
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5227,15 +5227,22 @@
   if (Err)
 return std::move(Err);
 
-  // FIXME: Import default argument.
-
   NonTypeTemplateParmDecl *ToD = nullptr;
-  (void)GetImportedOrCreateDecl(
-  ToD, D, Importer.getToContext(),
-  Importer.getToContext().getTranslationUnitDecl(),
-  

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: clang/lib/AST/ASTImporter.cpp:5161
 
-  // FIXME: Import default argument  and constraint expression.
+  // FIXME: Import constraint expression.
 

martong wrote:
> I wonder whether this FIXME is already fixed, at line 5182?
I think so. I removed the whole comment instead. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

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

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f6c856bb5ae: [ASTImporter] Import the default argument of 
TemplateTypeParmDecl (authored by teemperor).
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D92103?vs=307614=307891#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92103

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  
lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/list/TestListFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/stack/TestStackFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  
lldb/test/API/commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  
lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py

Index: lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector/TestVectorFromStdModule.py
@@ -22,7 +22,7 @@
 
 self.runCmd("settings set target.import-std-module true")
 
-vector_type = "std::vector >"
+vector_type = "std::vector"
 size_type = vector_type + "::size_type"
 value_type = "std::__vector_base >::value_type"
 iterator = vector_type + "::iterator"
Index: lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
@@ -20,13 +20,9 @@
   "// Set break point at this line.",
   lldb.SBFileSpec("main.cpp"))
 
-vector_type = "std::vector >"
-vector_of_vector_type = "std::vector<" + vector_type + \
-", std::allocator > > >"
-size_type = (
-"std::vector >, " +
-"std::allocator > >" +
-" >::size_type")
+vector_type = "std::vector"
+vector_of_vector_type = "std::vector<" + vector_type + " >"
+size_type = vector_of_vector_type + "::size_type"
 value_type = "std::__vector_base >::value_type"
 
 self.runCmd("settings set target.import-std-module true")
@@ -35,13 +31,13 @@
 "a",
 result_type=vector_of_vector_type,
 result_children=[
-ValueCheck(type="std::vector >",
+ValueCheck(type="std::vector",
children=[
ValueCheck(value='1'),
ValueCheck(value='2'),
ValueCheck(value='3'),
]),
-ValueCheck(type="std::vector >",
+ValueCheck(type="std::vector",
children=[
ValueCheck(value='3'),
ValueCheck(value='2'),
Index: lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
@@ -23,7 +23,7 @@
 
 

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

[Raphael Isemann via Phabricator via cfe-commits]

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

I think the test could be made a bit stricter (see inline comment), but 
otherwise this LGTM. Thanks!




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5900
+  ASSERT_TRUE(ToD);
+  EXPECT_EQ(FromD->isCopyDeductionCandidate(), 
ToD->isCopyDeductionCandidate());
+  // Check that the deduced class template is also imported.

Not sure if that's actually testing what it should. `FromD` is not the copy 
deduction candidate, so this is just comparing that both Decls have the default 
value `false`?

If we pick the copy deduction candidate in this test then we could test that we 
actually copy the value over and not just have the default 'false' flag for 
`isCopyDeductionCandidate` (something like 
`cxxDeductionGuideDecl(hasParameter(0, hasType(asString("A"` should find 
the copy deduction candidate).

Then we could also simplify this to 
`EXPECT_TRUE(ToD->isCopyDeductionCandidate());`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92109

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

[PATCH] D92119: [ASTImporter] Import the default argument of TemplateTemplateParmDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39a5dd164ca8: [ASTImporter] Import the default argument of 
TemplateTemplateParmDecl (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92119

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -901,6 +901,39 @@
   .isValid());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   TemplateTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template typename TT> struct Y 
{};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template struct X {};
+   template typename TT = X> struct 
Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  const TemplateArgument  = 
To->getDefaultArgument().getArgument();
+  ASSERT_TRUE(To->isTemplateDecl());
+  TemplateDecl *ToTemplate = ToDefaultArg.getAsTemplate().getAsTemplateDecl();
+
+  // Find the default argument template 'X' in the AST and compare it against
+  // the default argument we got.
+  auto ToExpectedDecl = FirstDeclMatcher().match(
+  To->getTranslationUnitDecl(), classTemplateDecl(hasName("X")));
+  ASSERT_EQ(ToTemplate, ToExpectedDecl);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX03);
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5250,15 +5250,22 @@
   if (!TemplateParamsOrErr)
 return TemplateParamsOrErr.takeError();
 
-  // FIXME: Import default argument.
-
   TemplateTemplateParmDecl *ToD = nullptr;
-  (void)GetImportedOrCreateDecl(
-  ToD, D, Importer.getToContext(),
-  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
-  D->getDepth(), D->getPosition(), D->isParameterPack(),
-  (*NameOrErr).getAsIdentifierInfo(),
-  *TemplateParamsOrErr);
+  if (GetImportedOrCreateDecl(
+  ToD, D, Importer.getToContext(),
+  Importer.getToContext().getTranslationUnitDecl(), *LocationOrErr,
+  D->getDepth(), D->getPosition(), D->isParameterPack(),
+  (*NameOrErr).getAsIdentifierInfo(), *TemplateParamsOrErr))
+return ToD;
+
+  if (D->hasDefaultArgument()) {
+Expected ToDefaultArgOrErr =
+import(D->getDefaultArgument());
+if (!ToDefaultArgOrErr)
+  return ToDefaultArgOrErr.takeError();
+ToD->setDefaultArgument(Importer.getToContext(), *ToDefaultArgOrErr);
+  }
+
   return ToD;
 }
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -901,6 +901,39 @@
   .isValid());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   TemplateTemplateParmDeclNoDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template typename TT> struct Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_FALSE(To->hasDefaultArgument());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, TemplateTemplateParmDeclDefaultArg) {
+  Decl *FromTU = getTuDecl(R"(
+   template struct X {};
+   template typename TT = X> struct Y {};
+   )",
+   Lang_CXX17);
+  auto From = FirstDeclMatcher().match(
+  FromTU, templateTemplateParmDecl(hasName("TT")));
+  TemplateTemplateParmDecl *To = Import(From, Lang_CXX17);
+  ASSERT_TRUE(To->hasDefaultArgument());
+  const TemplateArgument  = To->getDefaultArgument().getArgument();
+  ASSERT_TRUE(To->isTemplateDecl());
+  TemplateDecl *ToTemplate = ToDefaultArg.getAsTemplate().getAsTemplateDecl();
+
+ 

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

[Raphael Isemann via Phabricator via cfe-commits]

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

You think it's worth it to have a test for a user-specified deduction guide? I 
think this is missing an 'explicit' deduction guide test, so maybe we could 
test both things in a single additional test case.

Also, IIUC we ignore the IsCopyDeductionCandidate bit when importing and that 
seems like a potential bug to me (the value is apparently used for some 
overload resolution logic)?

Beside those two things this looks good to me.




Comment at: clang/lib/AST/ASTImporter.cpp:3332
 
+  // Common code to import an explicit specifier of different kind of 
funcitons.
+  auto ImportExplicitExpr = [this, ](auto *Fun) -> ExpectedExpr {

typo "funcitons"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92109

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

[PATCH] D92016: [ASTImporter] Make the Import() return value consistent with the map of imported decls when merging ClassTemplateSpecializationDecls

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c926e6d245b: [ASTImporter] Make the Import() return value 
consistent with the map of… (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92016

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -3104,6 +3104,25 @@
   EXPECT_TRUE(ToFun->hasBody());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, MergeTemplateSpecWithForwardDecl) {
+  std::string ClassTemplate =
+  R"(
+  template
+  struct X { int m; };
+  template<>
+  struct X { int m; };
+  )";
+  // Append a forward decl for our template specialization.
+  getToTuDecl(ClassTemplate + "template<> struct X;", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate, Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X"), isDefinition()));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  // Check that our definition got merged with the existing definition.
+  EXPECT_TRUE(FromSpec->isThisDeclarationADefinition());
+  EXPECT_TRUE(ImportedSpec->isThisDeclarationADefinition());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
   std::string ClassTemplate =
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5423,8 +5423,9 @@
 
   if (PrevDecl) {
 if (IsStructuralMatch(D, PrevDecl)) {
-  if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
-Importer.MapImported(D, PrevDecl->getDefinition());
+  CXXRecordDecl *PrevDefinition = PrevDecl->getDefinition();
+  if (D->isThisDeclarationADefinition() && PrevDefinition) {
+Importer.MapImported(D, PrevDefinition);
 // Import those default field initializers which have been
 // instantiated in the "From" context, but not in the "To" context.
 for (auto *FromField : D->fields()) {
@@ -5446,7 +5447,7 @@
 //
 // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
 // what else could be fused during an AST merge.
-return PrevDecl;
+return PrevDefinition;
   }
 } else { // ODR violation.
   // FIXME HandleNameConflict


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -3104,6 +3104,25 @@
   EXPECT_TRUE(ToFun->hasBody());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, MergeTemplateSpecWithForwardDecl) {
+  std::string ClassTemplate =
+  R"(
+  template
+  struct X { int m; };
+  template<>
+  struct X { int m; };
+  )";
+  // Append a forward decl for our template specialization.
+  getToTuDecl(ClassTemplate + "template<> struct X;", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate, Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X"), isDefinition()));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  // Check that our definition got merged with the existing definition.
+  EXPECT_TRUE(FromSpec->isThisDeclarationADefinition());
+  EXPECT_TRUE(ImportedSpec->isThisDeclarationADefinition());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
   std::string ClassTemplate =
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5423,8 +5423,9 @@
 
   if (PrevDecl) {
 if (IsStructuralMatch(D, PrevDecl)) {
-  if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
-Importer.MapImported(D, PrevDecl->getDefinition());
+  CXXRecordDecl *PrevDefinition = PrevDecl->getDefinition();
+  if (D->isThisDeclarationADefinition() && PrevDefinition) {
+Importer.MapImported(D, PrevDefinition);
 // Import those default field initializers which have been
 // instantiated in the "From" context, but not in the "To" context.
 for (auto *FromField : D->fields()) {
@@ -5446,7 +5447,7 @@
 //
 // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
 // what else could be fused during an AST merge.
-return PrevDecl;
+return PrevDefinition;
   }
 } else { // ODR 

[PATCH] D83038: [clang] Add an option for hiding line numbers in diagnostics

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG659f4bd87efc: [clang] Add an option for hiding line numbers 
in diagnostics (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83038

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/unittests/Frontend/CMakeLists.txt
  clang/unittests/Frontend/TextDiagnosticTest.cpp

Index: clang/unittests/Frontend/TextDiagnosticTest.cpp
===
--- /dev/null
+++ clang/unittests/Frontend/TextDiagnosticTest.cpp
@@ -0,0 +1,100 @@
+//===- unittests/Frontend/TextDiagnosticTest.cpp - ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Frontend/TextDiagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+/// Prints a diagnostic with the given DiagnosticOptions and the given
+/// SourceLocation and returns the printed diagnostic text.
+static std::string PrintDiag(const DiagnosticOptions , FullSourceLoc Loc) {
+  std::string Out;
+  llvm::raw_string_ostream OS(Out);
+  clang::LangOptions LangOpts;
+  // Owned by TextDiagnostic.
+  DiagnosticOptions *DiagOpts = new DiagnosticOptions(Opts);
+  TextDiagnostic Diag(OS, LangOpts, DiagOpts);
+  // Emit a dummy diagnostic that is just 'message'.
+  Diag.emitDiagnostic(Loc, DiagnosticsEngine::Level::Warning, "message",
+  /*Ranges=*/{}, /*FixItHints=*/{});
+  OS.flush();
+  return Out;
+}
+
+TEST(TextDiagnostic, ShowLine) {
+  // Create dummy FileManager and SourceManager.
+  FileSystemOptions FSOpts;
+  FileManager FileMgr(FSOpts);
+  IntrusiveRefCntPtr DiagID(new DiagnosticIDs);
+  DiagnosticsEngine DiagEngine(DiagID, new DiagnosticOptions,
+   new IgnoringDiagConsumer());
+  SourceManager SrcMgr(DiagEngine, FileMgr);
+
+  // Create a dummy file with some contents to produce a test SourceLocation.
+  const llvm::StringRef file_path = "main.cpp";
+  const llvm::StringRef main_file_contents = "some\nsource\ncode\n";
+  const clang::FileEntry  = *FileMgr.getVirtualFile(
+  file_path,
+  /*Size=*/static_cast(main_file_contents.size()),
+  /*ModificationTime=*/0);
+
+  llvm::SmallVector buffer;
+  buffer.append(main_file_contents.begin(), main_file_contents.end());
+  auto file_contents = std::make_unique(
+  std::move(buffer), file_path);
+  SrcMgr.overrideFileContents(, std::move(file_contents));
+
+  // Create the actual file id and use it as the main file.
+  clang::FileID fid =
+  SrcMgr.createFileID(, SourceLocation(), clang::SrcMgr::C_User);
+  SrcMgr.setMainFileID(fid);
+
+  // Create the source location for the test diagnostic.
+  FullSourceLoc Loc(SrcMgr.translateLineCol(fid, /*Line=*/1, /*Col=*/2),
+SrcMgr);
+
+  DiagnosticOptions DiagOpts;
+  DiagOpts.ShowLine = true;
+  DiagOpts.ShowColumn = true;
+  // Hide printing the source line/caret to make the diagnostic shorter and it's
+  // not relevant for this test.
+  DiagOpts.ShowCarets = false;
+  EXPECT_EQ("main.cpp:1:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Check that ShowLine doesn't influence the Vi/MSVC diagnostic formats as its
+  // a Clang-specific diagnostic option.
+  DiagOpts.setFormat(TextDiagnosticFormat::Vi);
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp +1:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  DiagOpts.setFormat(TextDiagnosticFormat::MSVC);
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp(1,2): warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Reset back to the Clang format.
+  DiagOpts.setFormat(TextDiagnosticFormat::Clang);
+
+  // Hide line number but show column.
+  DiagOpts.ShowLine = false;
+  EXPECT_EQ("main.cpp:2: warning: message\n", PrintDiag(DiagOpts, Loc));
+
+  // Show line number but hide column.
+  DiagOpts.ShowLine = true;
+  DiagOpts.ShowColumn = false;
+  EXPECT_EQ("main.cpp:1: warning: message\n", PrintDiag(DiagOpts, Loc));
+}
+
+} // anonymous namespace
Index: clang/unittests/Frontend/CMakeLists.txt
===
--- clang/unittests/Frontend/CMakeLists.txt
+++ clang/unittests/Frontend/CMakeLists.txt
@@ -12,6 +12,7 @@
   ParsedSourceLocationTest.cpp
   PCHPreambleTest.cpp
   OutputStreamTest.cpp
+  TextDiagnosticTest.cpp
   )
 

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor updated this revision to Diff 301577.
teemperor added a comment.

- Respect forced deserialisation.


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

https://reviews.llvm.org/D80878

Files:
  clang/include/clang/AST/TextNodeDumper.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,182 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// An ExternalASTSource that records which DeclContexts were queried so far.
+class RecordingExternalASTSource : public ExternalASTSource {
+public:
+  llvm::DenseSet QueriedDCs;
+  ~RecordingExternalASTSource() override = default;
+
+  void FindExternalLexicalDecls(const DeclContext *DC,
+llvm::function_ref,
+SmallVectorImpl &) override {
+QueriedDCs.insert(DC);
+  }
+};
+
+
+class ASTDumpTestBase : public ::testing::Test {
+protected:
+  ASTDumpTestBase(clang::ExternalASTSource *Source)
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins) {
+Ctxt.setExternalSource(Source);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class NoDeserializeTest : public ASTDumpTestBase {
+protected:
+  NoDeserializeTest() : ASTDumpTestBase(new TrappingExternalASTSource()) {}
+};
+
+/// Tests which declarations Decl::dump deserializes;
+class DeserializeTest : public ASTDumpTestBase {
+protected:
+  DeserializeTest() : ASTDumpTestBase(Recorder = new RecordingExternalASTSource()) {}
+  RecordingExternalASTSource *Recorder;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  DC->setMustBuildLookupTable();
+}
+
+/// Dumps the given Decl.
+static void 

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

>> The tests are just loading a Clang module and then trying to import some 
>> Decls into another ASTContext, so I don't think the error here is specific 
>> to LLDB. We just don't have any ASTImporter tests that exercise modules.
>
> I'll add one.

I'm actually not even sure if we have a nice framework where we could test 
modules + ASTImporter, so don't feel obligated to solve this as part of this 
patch.

>> FWIW, if you quickly want to see if your SourceManager changes break LLDB, 
>> then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as 
>> LLDB's use of Clang modules are the only place where we have any valid 
>> SourceLocations within LLDB.
>
> This is helpful, thanks. I also need to deal with whatever entitlements 
> `check-lldb` needs (new machine since the last time and I remember it being 
> non-trivial) but I'll figure that out.
>
>> 1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a 
>> `LoadedSLocEntryTable` index is a bit cryptic. [...]
>
> Great comment; I think the way I'll do this is to create a simple wrapper 
> type that degrades to `Optional`.

Thanks!

>> 2. If we have this as an Optional, we might as well make the sentinel value 
>> `std::numeric_limits::max()` instead of 0 in `SLocEntryLoaded`. 
>> Then we can just all values as-is without translating them to an index.
>
> Could do, but there's a tradeoff since then a `resize` operation 
> can't/doesn't use zero-initialization. As a result I have a (weak) preference 
> for a 0-sentinel (probably my next patch will keep that semantic), but I'm 
> open to changing it.

True. Let's keep the 0 sentinel value for now then, I don't think the small 
advantage of using 2^32 is worth that trouble.

>> 3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and 
>> not the loaded status, maybe something like `SLocEntryIndices` would be a 
>> better name?
>
> Good idea.
>
>> 4. Do you still have your benchmarking values (or some estimates) for this? 
>> I'm just curious how much memory this actually saves.
>
> I'll find some numbers if Vassil hasn't shared his by the time I've fixed the 
> patch.




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

https://reviews.llvm.org/D89749

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

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a subscriber: v.g.vassilev.
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests to 
fail on my machine:

  lldb-api :: 
commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py

From the backtrace this seems like the ASTImporter needs to be updated:

  * frame #0: 0x725f1bb6 
_lldb.so`clang::ASTImporter::Import(clang::FileID, bool) + 566
frame #1: 0x725ecc56 
_lldb.so`clang::ASTImporter::Import(clang::SourceLocation) + 182
frame #2: 0x725bbaed _lldb.so`clang::SourceLocation 
clang::ASTNodeImporter::importChecked(llvm::Error&, 
clang::SourceLocation const&) + 61
frame #3: 0x725c34e8 
_lldb.so`clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 2968
frame #4: 0x725ebb31 
_lldb.so`clang::declvisitor::Base >::Visit(clang::Decl*) + 81
frame #5: 0x725ebac2 
_lldb.so`clang::ASTImporter::ImportImpl(clang::Decl*) + 34
frame #6: 0x70df9cb1 
_lldb.so`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*)
 + 1345
frame #7: 0x725ed3f4 
_lldb.so`clang::ASTImporter::Import(clang::Decl*) + 532
frame #8: 0x725aeb93 _lldb.so`llvm::Expected 
clang::ASTNodeImport

The tests are just loading a Clang module and then trying to import some Decls 
into another ASTContext, so I don't think the error here is specific to LLDB. 
We just don't have any ASTImporter tests that exercise modules.

FWIW, if you quickly want to see if your SourceManager changes break LLDB, then 
you can just set the LIT_FILTER to "Modules" and run `check-lldb` as LLDB's use 
of Clang modules are the only place where we have any valid SourceLocations 
within LLDB.

Regarding the patch itself:

1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a 
`LoadedSLocEntryTable` index is a bit cryptic. We already do similar magic in 
other places, but here it's really just about expressing an optional integer. 
Could we maybe have a wrapper function that turns this into a 
`llvm::Optional` (that is `llvm::None` when it's not loaded). 
Something like this maybe (which hopefully optimizes to something similar to 
the original code):

  llvm::Optional getLoadedSLocEntryTableIndex(size_t Index) const {
unsigned result = SLocEntryLoaded[Index];
if (result == /*sentinel value*/0)
  return llvm::None;
return result - 1U;
  }



2. If we have this as an Optional, we might as well make the sentinel value 
`std::numeric_limits::max()` instead of 0 in `SLocEntryLoaded`. Then 
we can just all values as-is without translating them to an index.

3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and not 
the loaded status, maybe something like `SLocEntryIndices` would be a better 
name?

4. Do you still have your benchmarking values (or some estimates) for this? I'm 
just curious how much memory this actually saves.

Otherwise I think this looks good.


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

https://reviews.llvm.org/D89749

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

[PATCH] D89748: SourceManager: Use the same fake SLocEntry whenever it fails to load

[Raphael Isemann via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D89748

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

[PATCH] D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr

[Raphael Isemann via Phabricator via cfe-commits]

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

LGTM, thanks!




Comment at: clang/test/ASTMerge/attr/Inputs/FormatAttr.cpp:2
+int foo(const char * fmt, ...)
+__attribute__ ((__format__ (__scanf__, 1, 2)));

(Not sure if we care about that in tests, but that's technically not in LLVM 
code style)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89319

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

[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

From the LLDB side we can just change our test setup to explicitly add an 
include the libc++ in our build tree (and have -nostdinc to avoid the libc++ in 
the SDK). So it wouldn't be a problem for us. I can make a review for that once 
this gets accepted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88984

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

[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I think we could at least have a unittest that just calls these functions with 
an invalid SourceLocation. `unittests/Basic/SourceManagerTest.cpp` already 
takes care of creating a valid SourceManager object, so the unit test would 
just be a single call to each method. Of course it wouldn't be the exact same 
setup as whatever is Cling is doing, but it's better than nothing.

Also, I'm still trying to puzzle together what exactly the situation is that 
triggers this bug in Cling. From what I understand:

1. We call the ASTImporter::Import(Decl) with a Decl.
2. That triggers the importing of some SourceLocation. Given the ASTImporter is 
early-exiting on an invalid SourceLocation, this means you were importing a 
valid SourceLocation when hitting this crash.
3. We enter with a valid SourceLocation `isWrittenInBuiltinFile` from 
`ASTImporter::Import(SourceLocation)`. Now the function is computing the 
PresumedLoc via `getPresumedLoc` and that returns an invalid PresumedLoc.
4. We hit the assert due to the invalid PresumedLoc.

Do you know where exactly is getPresumedLoc returning the invalid error value? 
The review title just mentions a "in-memory buffer with no source location 
info", but it doesn't really explain what's going on. Are there SourceLocations 
pointing to that memory buffer but it's not registered with the SourceManager?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88780

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

[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

On macOS, LLDB's tests pass `-isysroot /path/to/SDK` to Clang but expect that 
Clang always prefers the the libc++ headers that are in the Clang build 
directory. So if we change this behaviour then we should have a way to keep the 
old behaviour around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88984

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

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

> I am not sure how much do you guys value these diags in LLDB

I think we don't even display them most of the time (IIUC they go to the 
diagnostic engine which is not always hooked up for the ASTs in LLDB as only a 
few actually come from Clang).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88665

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

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88665

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

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

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

Shouldn't this compare the actual width expressions? In other words, this patch 
wouldn't pass the test below:

  TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDeclDifferentVal) {
const char *Code1 = "template  class foo { int a : 
sizeof(A); };";
const char *Code2 = "template  class foo { int a : 
sizeof(B); };";
auto t = makeNamedDecls(Code1, Code2, Lang_CXX03);
EXPECT_FALSE(testStructuralMatch(t));
  }

I don't want to derail the simple crash fix here, but I'm tempted to say that 
we might as well just simplify all this to `return 
IsStructurallyEquivalent(Context, Field1->getBitWidth(), 
Field2->getBitWidth());`. The nice diagnostic could live behind behind a check 
that ensures that both widths are not value-dependent.

If you want to keep this simple because you want to backport it I would also be 
fine with the current patch (not crashing is clearly an improvement)

In D88665#2307562 , @shafik wrote:

> So was the bug we were saying there were falsely equivalent or falsely not 
> equivalent?

I think the bug is the crash :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88665

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

[PATCH] D87619: [ASTImporter] Refactor IsStructurallyEquivalent's Decl overloads to be more consistent

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7c4575e15f06: [ASTImporter] Refactor 
IsStructurallyEquivalents Decl overloads to be more… (authored by 
teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87619

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -1586,6 +1586,22 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceStmtTest, MemberExpr) {
+  std::string ClassSrc = "struct C { int a; int b; };";
+  auto t = makeStmts(ClassSrc + "int wrapper() { C c; return c.a; }",
+ ClassSrc + "int wrapper() { C c; return c.a; }",
+ Lang_CXX03, memberExpr());
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceStmtTest, MemberExprDifferentMember) {
+  std::string ClassSrc = "struct C { int a; int b; };";
+  auto t = makeStmts(ClassSrc + "int wrapper() { C c; return c.a; }",
+ ClassSrc + "int wrapper() { C c; return c.b; }",
+ Lang_CXX03, memberExpr());
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceStmtTest, ObjCStringLiteral) {
   auto t =
   makeWrappedStmts("@\"a\"", "@\"a\"", Lang_OBJCXX, fallbackExprMatcher());
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -66,6 +66,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprConcepts.h"
@@ -242,6 +243,11 @@
 return E1->getValue() == E2->getValue();
   }
 
+  bool IsStmtEquivalent(const MemberExpr *E1, const MemberExpr *E2) {
+return IsStructurallyEquivalent(Context, E1->getFoundDecl(),
+E2->getFoundDecl());
+  }
+
   bool IsStmtEquivalent(const ObjCStringLiteral *E1,
 const ObjCStringLiteral *E2) {
 // Just wraps a StringLiteral child.
@@ -1364,6 +1370,17 @@
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  RecordDecl *D1, RecordDecl *D2) {
+
+  // Check for equivalent structure names.
+  IdentifierInfo *Name1 = D1->getIdentifier();
+  if (!Name1 && D1->getTypedefNameForAnonDecl())
+Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
+  IdentifierInfo *Name2 = D2->getIdentifier();
+  if (!Name2 && D2->getTypedefNameForAnonDecl())
+Name2 = D2->getTypedefNameForAnonDecl()->getIdentifier();
+  if (!IsStructurallyEquivalent(Name1, Name2))
+return false;
+
   if (D1->isUnion() != D2->isUnion()) {
 if (Context.Complain) {
   Context.Diag2(D2->getLocation(), Context.getApplicableDiagnostic(
@@ -1598,6 +1615,16 @@
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  EnumDecl *D1, EnumDecl *D2) {
 
+  // Check for equivalent enum names.
+  IdentifierInfo *Name1 = D1->getIdentifier();
+  if (!Name1 && D1->getTypedefNameForAnonDecl())
+Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
+  IdentifierInfo *Name2 = D2->getIdentifier();
+  if (!Name2 && D2->getTypedefNameForAnonDecl())
+Name2 = D2->getTypedefNameForAnonDecl()->getIdentifier();
+  if (!IsStructurallyEquivalent(Name1, Name2))
+return false;
+
   // Compare the definitions of these two enums. If either or both are
   // incomplete (i.e. forward declared), we assume that they are equivalent.
   D1 = D1->getDefinition();
@@ -1823,8 +1850,27 @@
   return false;
 }
 
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
+ TypedefNameDecl *D1, TypedefNameDecl *D2) {
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+return false;
+
+  return IsStructurallyEquivalent(Context, D1->getUnderlyingType(),
+  D2->getUnderlyingType());
+}
+
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  FunctionDecl *D1, FunctionDecl *D2) {
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+return false;
+
+  if (D1->isOverloadedOperator()) {
+if (!D2->isOverloadedOperator())
+  return false;
+if (D1->getOverloadedOperator() != D2->getOverloadedOperator())
+  

[PATCH] D87444: [ASTImporter] Add basic support for comparing Stmts and compare function bodies

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc0bcd11068fc: [ASTImporter] Add basic support for comparing 
Stmts and compare function bodies (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87444

Files:
  clang/include/clang/AST/ASTStructuralEquivalence.h
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -19,14 +19,10 @@
   std::unique_ptr AST0, AST1;
   std::string Code0, Code1; // Buffers for SourceManager
 
-  // Get a pair of node pointers into the synthesized AST from the given code
-  // snippets. To determine the returned node, a separate matcher is specified
-  // for both snippets. The first matching node is returned.
-  template 
-  std::tuple
-  makeDecls(const std::string , const std::string ,
-TestLanguage Lang, const MatcherType ,
-const MatcherType ) {
+  // Parses the source code in the specified language and sets the ASTs of
+  // the current test instance to the parse result.
+  void makeASTUnits(const std::string , const std::string ,
+TestLanguage Lang) {
 this->Code0 = SrcCode0;
 this->Code1 = SrcCode1;
 std::vector Args = getCommandLineArgsForTesting(Lang);
@@ -35,6 +31,17 @@
 
 AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
 AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+  }
+
+  // Get a pair of node pointers into the synthesized AST from the given code
+  // snippets. To determine the returned node, a separate matcher is specified
+  // for both snippets. The first matching node is returned.
+  template 
+  std::tuple
+  makeDecls(const std::string , const std::string ,
+TestLanguage Lang, const MatcherType ,
+const MatcherType ) {
+makeASTUnits(SrcCode0, SrcCode1, Lang);
 
 NodeType *D0 = FirstDeclMatcher().match(
 AST0->getASTContext().getTranslationUnitDecl(), Matcher0);
@@ -47,14 +54,7 @@
   std::tuple
   makeTuDecls(const std::string , const std::string ,
   TestLanguage Lang) {
-this->Code0 = SrcCode0;
-this->Code1 = SrcCode1;
-std::vector Args = getCommandLineArgsForTesting(Lang);
-
-const char *const InputFileName = "input.cc";
-
-AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
-AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+makeASTUnits(SrcCode0, SrcCode1, Lang);
 
 return std::make_tuple(AST0->getASTContext().getTranslationUnitDecl(),
AST1->getASTContext().getTranslationUnitDecl());
@@ -80,6 +80,56 @@
 return makeDecls(SrcCode0, SrcCode1, Lang, Matcher);
   }
 
+  // Wraps a Stmt and the ASTContext that contains it.
+  struct StmtWithASTContext {
+Stmt *S;
+ASTContext *Context;
+explicit StmtWithASTContext(Stmt , ASTContext )
+: S(), Context() {}
+explicit StmtWithASTContext(FunctionDecl *FD)
+: S(FD->getBody()), Context(>getASTContext()) {}
+  };
+
+  // Get a pair of node pointers into the synthesized AST from the given code
+  // snippets. To determine the returned node, a separate matcher is specified
+  // for both snippets. The first matching node is returned.
+  template 
+  std::tuple
+  makeStmts(const std::string , const std::string ,
+TestLanguage Lang, const MatcherType ,
+const MatcherType ) {
+makeASTUnits(SrcCode0, SrcCode1, Lang);
+
+Stmt *S0 = FirstDeclMatcher().match(
+AST0->getASTContext().getTranslationUnitDecl(), Matcher0);
+Stmt *S1 = FirstDeclMatcher().match(
+AST1->getASTContext().getTranslationUnitDecl(), Matcher1);
+
+return std::make_tuple(StmtWithASTContext(*S0, AST0->getASTContext()),
+   StmtWithASTContext(*S1, AST1->getASTContext()));
+  }
+
+  // Get a pair of node pointers into the synthesized AST from the given code
+  // snippets. The same matcher is used for both snippets.
+  template 
+  std::tuple
+  makeStmts(const std::string , const std::string ,
+TestLanguage Lang, const MatcherType ) {
+return makeStmts(SrcCode0, SrcCode1, Lang, AMatcher, AMatcher);
+  }
+
+  // Convenience function for makeStmts that wraps the code inside a function
+  // body.
+  template 
+  std::tuple
+  makeWrappedStmts(const std::string , const std::string ,
+   TestLanguage Lang, const MatcherType ) {
+auto Wrap = [](const std::string ) {
+  return "void wrapped() {" + Src + ";}";
+};
+return makeStmts(Wrap(SrcCode0), 

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

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

In D80878#2258942 , @riccibruno wrote:

> And what if deserialization is forced?

That's a good point. That should indeed continue to work with the ASTDumper and 
now (potentially) doesn't. I'll revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0478720157f6: [clang] Prevent that Decl::dump on a 
CXXRecordDecl deserialises further… (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80878

Files:
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,140 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class ExternalASTSourceDumpTest : public ::testing::Test {
+protected:
+  ExternalASTSourceDumpTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins) {
+Ctxt.setExternalSource(new TrappingExternalASTSource());
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  DC->setMustBuildLookupTable();
+}
+
+/// Dumps the given Decl.
+static void dumpDecl(Decl *D) {
+  // Try dumping the decl which shouldn't trigger any calls to the
+  // ExternalASTSource.
+
+  std::string Out;
+  llvm::raw_string_ostream OS(Out);
+  D->dump(OS);
+}
+
+TEST_F(ExternalASTSourceDumpTest, DumpObjCInterfaceDecl) {
+  // Define an Objective-C interface.
+  ObjCInterfaceDecl *I = ObjCInterfaceDecl::Create(
+  Ctxt, Ctxt.getTranslationUnitDecl(), SourceLocation(),
+  ("c"), nullptr, nullptr);
+  Ctxt.getTranslationUnitDecl()->addDecl(I);
+
+  setExternalStorageFlags(I);
+  dumpDecl(I);
+}
+
+TEST_F(ExternalASTSourceDumpTest, DumpRecordDecl) {
+  // Define a struct.
+  RecordDecl *R = RecordDecl::Create(
+  Ctxt, TagDecl::TagKind::TTK_Class, 

[PATCH] D86290: Move all fields of '-cc1' option related classes into def file databases

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Just to also jump in on the thread: This broke the modules builds. I fixed that 
in https://reviews.llvm.org/rGe0e7eb2e2648aee83caf2ecfe2972ce2f653d306 so 
please recommit that patch or merge it into this revision before relanding. 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86290

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

[PATCH] D86308: [CMake][compiler-rt][libunwind] Compile assembly files as ASM not C, unify workarounds

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D86308#2229936 , @tambre wrote:

> In D86308#2229901 , @teemperor wrote:
>
>> Sorry, just got around to check this out. With the new workaround this seems 
>> to work on macOS (the initial patch did produce the same error).
>
> Many thanks!
> I've submitted an upstream CMake MR to hopefully fix this 
> . I'd 
> appreciate if you could test that too. There's an example testcase in CMake 
> issue 20771 .

With that CMake patch your original patch compiles just fine on macOS. Feel 
free to add a FIXME that this workaround can be removed when the CMake version 
you merged this is because the new minimum required. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86308

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

[PATCH] D84013: Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG105151ca5669: Reland Correctly emit dwoIDs after 
ASTFileSignature refactoring (D81347) (authored by teemperor).

Changed prior to commit:
  https://reviews.llvm.org/D84013?vs=287318=287358#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84013

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/test/Modules/Inputs/DebugDwoId.h
  clang/test/Modules/Inputs/module.map
  clang/test/Modules/ModuleDebugInfoDwoId.cpp

Index: clang/test/Modules/ModuleDebugInfoDwoId.cpp
===
--- /dev/null
+++ clang/test/Modules/ModuleDebugInfoDwoId.cpp
@@ -0,0 +1,22 @@
+// Tests that dwoIds in modules match the dwoIDs in the main file.
+
+// REQUIRES: asserts
+
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -triple %itanium_abi_triple -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=%t.cache %s -I %S/Inputs -emit-llvm -o %t.ll -mllvm -debug-only=pchcontainer 2> %t.mod-out
+// RUN: cat %t.ll %t.mod-out | FileCheck %s
+// RUN: cat %t.ll | FileCheck --check-prefix=CHECK-REALIDS %s
+// RUN: cat %t.mod-out | FileCheck --check-prefix=CHECK-REALIDS %s
+
+@import DebugDwoId;
+
+Dummy d;
+
+// Find the emitted dwoID for DebugInfoId and compare it against the one in the PCM.
+// CHECK: DebugDwoId-{{[A-Z0-9]+}}.pcm
+// CHECK-SAME: dwoId: [[DWOID:[0-9]+]]
+// CHECK: dwoId: [[DWOID]]
+// CHECK-NEXT: !DIFile(filename: "DebugDwoId"
+
+// Make sure the dwo IDs are real IDs and not fallback values (~1ULL).
+// CHECK-REALIDS-NOT: dwoId: 18446744073709551615
Index: clang/test/Modules/Inputs/module.map
===
--- clang/test/Modules/Inputs/module.map
+++ clang/test/Modules/Inputs/module.map
@@ -357,6 +357,10 @@
   }
 }
 
+module DebugDwoId {
+  header "DebugDwoId.h"
+}
+
 module ImportNameInDir {
   header "ImportNameInDir.h"
   export *
Index: clang/test/Modules/Inputs/DebugDwoId.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/DebugDwoId.h
@@ -0,0 +1,4 @@
+#ifndef DEBUG_DWO_ID_H
+#define DEBUG_DWO_ID_H
+struct Dummy {};
+#endif
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -250,10 +250,10 @@
 // PCH files don't have a signature field in the control block,
 // but LLVM detects DWO CUs by looking for a non-zero DWO id.
 // We use the lower 64 bits for debug info.
+
 uint64_t Signature =
-Buffer->Signature
-? (uint64_t)Buffer->Signature[1] << 32 | Buffer->Signature[0]
-: ~1ULL;
+Buffer->Signature ? Buffer->Signature.truncatedValue() : ~1ULL;
+
 Builder->getModuleDebugInfo()->setDwoId(Signature);
 
 // Finalize the Builder.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2545,12 +2545,11 @@
 // We use the lower 64 bits for debug info.
 
 uint64_t Signature = 0;
-if (const auto  = Mod.getSignature()) {
-  for (unsigned I = 0; I != sizeof(Signature); ++I)
-Signature |= (uint64_t)ModSig[I] << (I * 8);
-} else {
+if (const auto  = Mod.getSignature())
+  Signature = ModSig.truncatedValue();
+else
   Signature = ~1ULL;
-}
+
 llvm::DIBuilder DIB(CGM.getModule());
 SmallString<0> PCM;
 if (!llvm::sys::path::is_absolute(Mod.getASTFile()))
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -62,6 +62,15 @@
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  /// Returns the value truncated to the size of an uint64_t.
+  uint64_t truncatedValue() const {
+uint64_t Value = 0;
+static_assert(sizeof(*this) >= sizeof(uint64_t), "No need to truncate.");
+for (unsigned I = 0; I < sizeof(uint64_t); ++I)
+  Value |= static_cast((*this)[I]) << (I * 8);
+return Value;
+  }
+
   static ASTFileSignature create(StringRef Bytes) {
 return create(Bytes.bytes_begin(), Bytes.bytes_end());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84013: Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D84013#2233242 , @davezarzycki 
wrote:

> I have an auto-bisecting cron job that has identified the "reland" as 
> breaking the test suite on Fedora 32 (x86). Is there a quick fix or can we 
> revert the reland?
>
>   FAIL: Clang :: Modules/ModuleDebugInfoDwoId.cpp (12657 of 68968)
>    TEST 'Clang :: Modules/ModuleDebugInfoDwoId.cpp' 
> FAILED 
>   Script:
>   --
>   : 'RUN: at line 3';   rm -rf 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache
>   : 'RUN: at line 4';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
> /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple 
> x86_64-unknown-linux-gnu -x objective-c++ -std=c++11 -debugger-tuning=lldb 
> -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs 
> -fimplicit-module-maps 
> -fmodules-cache-path=/tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache
>  /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp -I 
> /home/dave/ro_s/lp/clang/test/Modules/Inputs -emit-llvm -o 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll
>  -mllvm -debug-only=pchcontainer 2> 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out
>   : 'RUN: at line 5';   cat 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll
>  > 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
>   : 'RUN: at line 6';   cat 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out
>  >> 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
>   : 'RUN: at line 7';   cat 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
>  | /tmp/_update_lc/r/bin/FileCheck 
> /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
>   : 'RUN: at line 8';   cat 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll
>  | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS 
> /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
>   : 'RUN: at line 9';   cat 
> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out
>  | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS 
> /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
>   --
>   Exit Code: 1
>   
>   
>   
>   Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
>   
>   Failed Tests (1):
> Clang :: Modules/ModuleDebugInfoDwoId.cpp

I'm still not sure why the test is failing ("Exit Code: 1" isn't incredibly 
enlightening), so I reverted in 2b3074c0d14cadbd9595346fc795d4a49a479a20 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84013

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

[PATCH] D84013: Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGada2e8ea6739: Reland Correctly emit dwoIDs after 
ASTFileSignature refactoring (D81347) (authored by teemperor).

Changed prior to commit:
  https://reviews.llvm.org/D84013?vs=287017=287318#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84013

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/test/Modules/Inputs/DebugDwoId.h
  clang/test/Modules/Inputs/module.map
  clang/test/Modules/ModuleDebugInfoDwoId.cpp

Index: clang/test/Modules/ModuleDebugInfoDwoId.cpp
===
--- /dev/null
+++ clang/test/Modules/ModuleDebugInfoDwoId.cpp
@@ -0,0 +1,22 @@
+// Tests that dwoIds in modules match the dwoIDs in the main file.
+
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -triple %itanium_abi_triple -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=%t.cache %s -I %S/Inputs -emit-llvm -o %t.ll -mllvm -debug-only=pchcontainer 2> %t.mod-out
+// RUN: cat %t.ll > %t.combined_output
+// RUN: cat %t.mod-out >> %t.combined_output
+// RUN: cat %t.combined_output | FileCheck %s
+// RUN: cat %t.ll | FileCheck --check-prefix=CHECK-REALIDS %s
+// RUN: cat %t.mod-out | FileCheck --check-prefix=CHECK-REALIDS %s
+
+@import DebugDwoId;
+
+Dummy d;
+
+// Find the emitted dwoID for DebugInfoId and compare it against the one in the PCM.
+// CHECK: DebugDwoId-{{[A-Z0-9]+}}.pcm
+// CHECK-SAME: dwoId: [[DWOID:[0-9]+]]
+// CHECK: dwoId: [[DWOID]]
+// CHECK-NEXT: !DIFile(filename: "DebugDwoId"
+
+// Make sure the dwo IDs are real IDs and not fallback values (~1ULL).
+// CHECK-REALIDS-NOT: dwoId: 18446744073709551615
Index: clang/test/Modules/Inputs/module.map
===
--- clang/test/Modules/Inputs/module.map
+++ clang/test/Modules/Inputs/module.map
@@ -357,6 +357,10 @@
   }
 }
 
+module DebugDwoId {
+  header "DebugDwoId.h"
+}
+
 module ImportNameInDir {
   header "ImportNameInDir.h"
   export *
Index: clang/test/Modules/Inputs/DebugDwoId.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/DebugDwoId.h
@@ -0,0 +1,4 @@
+#ifndef DEBUG_DWO_ID_H
+#define DEBUG_DWO_ID_H
+struct Dummy {};
+#endif
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -250,10 +250,10 @@
 // PCH files don't have a signature field in the control block,
 // but LLVM detects DWO CUs by looking for a non-zero DWO id.
 // We use the lower 64 bits for debug info.
+
 uint64_t Signature =
-Buffer->Signature
-? (uint64_t)Buffer->Signature[1] << 32 | Buffer->Signature[0]
-: ~1ULL;
+Buffer->Signature ? Buffer->Signature.truncatedValue() : ~1ULL;
+
 Builder->getModuleDebugInfo()->setDwoId(Signature);
 
 // Finalize the Builder.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2545,12 +2545,11 @@
 // We use the lower 64 bits for debug info.
 
 uint64_t Signature = 0;
-if (const auto  = Mod.getSignature()) {
-  for (unsigned I = 0; I != sizeof(Signature); ++I)
-Signature |= (uint64_t)ModSig[I] << (I * 8);
-} else {
+if (const auto  = Mod.getSignature())
+  Signature = ModSig.truncatedValue();
+else
   Signature = ~1ULL;
-}
+
 llvm::DIBuilder DIB(CGM.getModule());
 SmallString<0> PCM;
 if (!llvm::sys::path::is_absolute(Mod.getASTFile()))
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -62,6 +62,15 @@
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  /// Returns the value truncated to the size of an uint64_t.
+  uint64_t truncatedValue() const {
+uint64_t Value = 0;
+static_assert(sizeof(*this) >= sizeof(uint64_t), "No need to truncate.");
+for (unsigned I = 0; I < sizeof(uint64_t); ++I)
+  Value |= static_cast((*this)[I]) << (I * 8);
+return Value;
+  }
+
   static ASTFileSignature create(StringRef Bytes) {
 return create(Bytes.bytes_begin(), Bytes.bytes_end());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84013: Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[Raphael Isemann via Phabricator via cfe-commits]

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

Somehow this ended up failing on Fuchsia with a plain "Exit code 1", but not 
sure what's the fault. I'll revert until I figured that out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84013

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

[PATCH] D84013: Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa4c3ed42ba56: Correctly emit dwoIDs after ASTFileSignature 
refactoring (D81347) (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84013

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/test/Modules/Inputs/DebugDwoId.h
  clang/test/Modules/Inputs/module.map
  clang/test/Modules/ModuleDebugInfoDwoId.cpp

Index: clang/test/Modules/ModuleDebugInfoDwoId.cpp
===
--- /dev/null
+++ clang/test/Modules/ModuleDebugInfoDwoId.cpp
@@ -0,0 +1,20 @@
+// Tests that dwoIds in modules match the dwoIDs in the main file.
+
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -triple %itanium_abi_triple -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=%t.cache %s -I %S/Inputs -emit-llvm -o %t.ll -mllvm -debug-only=pchcontainer &> %t.mod-out
+// RUN: cat %t.ll %t.mod-out | FileCheck %s
+// RUN: cat %t.ll | FileCheck --check-prefix=CHECK-REALIDS %s
+// RUN: cat %t.mod-out | FileCheck --check-prefix=CHECK-REALIDS %s
+
+@import DebugDwoId;
+
+Dummy d;
+
+// Find the emitted dwoID for DebugInfoId and compare it against the one in the PCM.
+// CHECK: DebugDwoId-{{[A-Z0-9]+}}.pcm
+// CHECK-SAME: dwoId: [[DWOID:[0-9]+]]
+// CHECK: dwoId: [[DWOID]]
+// CHECK-NEXT: !DIFile(filename: "DebugDwoId"
+
+// Make sure the dwo IDs are real IDs and not fallback values (~1ULL).
+// CHECK-REALIDS-NOT: dwoId: 18446744073709551615
Index: clang/test/Modules/Inputs/module.map
===
--- clang/test/Modules/Inputs/module.map
+++ clang/test/Modules/Inputs/module.map
@@ -357,6 +357,10 @@
   }
 }
 
+module DebugDwoId {
+  header "DebugDwoId.h"
+}
+
 module ImportNameInDir {
   header "ImportNameInDir.h"
   export *
Index: clang/test/Modules/Inputs/DebugDwoId.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/DebugDwoId.h
@@ -0,0 +1,4 @@
+#ifndef DEBUG_DWO_ID_H
+#define DEBUG_DWO_ID_H
+struct Dummy {};
+#endif
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -250,10 +250,10 @@
 // PCH files don't have a signature field in the control block,
 // but LLVM detects DWO CUs by looking for a non-zero DWO id.
 // We use the lower 64 bits for debug info.
+
 uint64_t Signature =
-Buffer->Signature
-? (uint64_t)Buffer->Signature[1] << 32 | Buffer->Signature[0]
-: ~1ULL;
+Buffer->Signature ? Buffer->Signature.truncatedValue() : ~1ULL;
+
 Builder->getModuleDebugInfo()->setDwoId(Signature);
 
 // Finalize the Builder.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2545,12 +2545,11 @@
 // We use the lower 64 bits for debug info.
 
 uint64_t Signature = 0;
-if (const auto  = Mod.getSignature()) {
-  for (unsigned I = 0; I != sizeof(Signature); ++I)
-Signature |= (uint64_t)ModSig[I] << (I * 8);
-} else {
+if (const auto  = Mod.getSignature())
+  Signature = ModSig.truncatedValue();
+else
   Signature = ~1ULL;
-}
+
 llvm::DIBuilder DIB(CGM.getModule());
 SmallString<0> PCM;
 if (!llvm::sys::path::is_absolute(Mod.getASTFile()))
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -62,6 +62,15 @@
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  /// Returns the value truncated to the size of an uint64_t.
+  uint64_t truncatedValue() const {
+uint64_t Value = 0;
+static_assert(sizeof(*this) >= sizeof(uint64_t), "No need to truncate.");
+for (unsigned I = 0; I < sizeof(uint64_t); ++I)
+  Value |= static_cast((*this)[I]) << (I * 8);
+return Value;
+  }
+
   static ASTFileSignature create(StringRef Bytes) {
 return create(Bytes.bytes_begin(), Bytes.bytes_end());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86308: [CMake][compiler-rt][libunwind] Compile assembly files as ASM not C, unify workarounds

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.

Sorry, just got around to check this out. With the new workaround this seems to 
work on macOS (the initial patch did produce the same error).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86308

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

[PATCH] D85706: [compiler-rt] Compile assembly files as ASM not C

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I reverted in adf0b8cc70325f027d202139e3ff984c41896b57 
 to get 
the bots green again . Feel free to ping if you need someone to test the patch 
on macOS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85706

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

[PATCH] D85706: [compiler-rt] Compile assembly files as ASM not C

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This broke Green Dragon's compiler-rt builds and also my local compiler-rt 
build. See for example here: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/23424/console


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85706

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

[PATCH] D84829: [clang] Don't make ObjCIvarDecl visible twice when adding them to an implicit ObjCInterfaceDecl

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG02899d7f1b9a: [clang] Dont make ObjCIvarDecl visible 
twice when adding them to an implicit… (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84829

Files:
  clang/lib/Sema/SemaDeclObjC.cpp


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -2122,7 +2122,12 @@
 // Add ivar's to class's DeclContext.
 for (unsigned i = 0, e = numIvars; i != e; ++i) {
   ivars[i]->setLexicalDeclContext(ImpDecl);
-  IDecl->makeDeclVisibleInContext(ivars[i]);
+  // In a 'fragile' runtime the ivar was added to the implicit
+  // ObjCInterfaceDecl while in a 'non-fragile' runtime the ivar is
+  // only in the ObjCImplementationDecl. In the non-fragile case the ivar
+  // therefore also needs to be propagated to the ObjCInterfaceDecl.
+  if (!LangOpts.ObjCRuntime.isFragile())
+IDecl->makeDeclVisibleInContext(ivars[i]);
   ImpDecl->addDecl(ivars[i]);
 }
 


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -2122,7 +2122,12 @@
 // Add ivar's to class's DeclContext.
 for (unsigned i = 0, e = numIvars; i != e; ++i) {
   ivars[i]->setLexicalDeclContext(ImpDecl);
-  IDecl->makeDeclVisibleInContext(ivars[i]);
+  // In a 'fragile' runtime the ivar was added to the implicit
+  // ObjCInterfaceDecl while in a 'non-fragile' runtime the ivar is
+  // only in the ObjCImplementationDecl. In the non-fragile case the ivar
+  // therefore also needs to be propagated to the ObjCInterfaceDecl.
+  if (!LangOpts.ObjCRuntime.isFragile())
+IDecl->makeDeclVisibleInContext(ivars[i]);
   ImpDecl->addDecl(ivars[i]);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84828: [clang] Don't make synthesized accessor stub functions visible twice

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG442a80292d50: [clang] Dont make synthesized accessor 
stub functions visible twice (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84828

Files:
  clang/lib/Sema/SemaDeclObjC.cpp


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3922,15 +3922,11 @@
   if (auto *OID = dyn_cast(CurContext)) {
 for (auto PropImpl : OID->property_impls()) {
   if (auto *Getter = PropImpl->getGetterMethodDecl())
-if (Getter->isSynthesizedAccessorStub()) {
-  OID->makeDeclVisibleInContext(Getter);
+if (Getter->isSynthesizedAccessorStub())
   OID->addDecl(Getter);
-}
   if (auto *Setter = PropImpl->getSetterMethodDecl())
-if (Setter->isSynthesizedAccessorStub()) {
-  OID->makeDeclVisibleInContext(Setter);
+if (Setter->isSynthesizedAccessorStub())
   OID->addDecl(Setter);
-}
 }
   }
 


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3922,15 +3922,11 @@
   if (auto *OID = dyn_cast(CurContext)) {
 for (auto PropImpl : OID->property_impls()) {
   if (auto *Getter = PropImpl->getGetterMethodDecl())
-if (Getter->isSynthesizedAccessorStub()) {
-  OID->makeDeclVisibleInContext(Getter);
+if (Getter->isSynthesizedAccessorStub())
   OID->addDecl(Getter);
-}
   if (auto *Setter = PropImpl->getSetterMethodDecl())
-if (Setter->isSynthesizedAccessorStub()) {
-  OID->makeDeclVisibleInContext(Setter);
+if (Setter->isSynthesizedAccessorStub())
   OID->addDecl(Setter);
-}
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Raphael Isemann via Phabricator via cfe-commits]

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

LGTM, thanks for the patch!


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

https://reviews.llvm.org/D83008



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

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I think we are talking about different things. My question was why is this a 
'Shell' test (like, a test in the `Shell` directory that uses FileCheck) and 
not a test in the `API` directory (using Python). An 'API' test could use the 
proper expression testing tools. And it could actually run when doing on-device 
testing (which is to my knowledge not supported for Shell tests) which seems 
important for a test concerning a bug that only triggers when doing on-device 
testing (beside that one ubuntu ARM bot).

Also when looking over the ARM-specific test, I think there might be two bugs 
that were involved in triggering it. One is the bug fixed here which triggers 
that Clang will produce its own layout for those classes. Now I also wonder why 
the layout the expression parser Clang generates doesn't match the one from the 
test (which appears to be a second bug). The ARM-specific test doesn't have any 
information in its AST that isn't also available in the expression AST, so why 
would they produce different layouts? Not sure what exactly is behind the 
"differences in how it deals with tail padding" description but I assume this 
is related to tail padding reuse? If yes, then maybe the second bug is that the 
records in our AST are (not) PODs in the expression AST (see the inline comment 
for where the tail padding is enabled/disabling based on whether the RD is POD).




Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2197
 // least as many of them as possible).
 return RD->isTrivial() && RD->isCXX11StandardLayout();
   }

See here for the POD check that we might get wrong.



Comment at: lldb/test/Shell/Expr/Inputs/layout.cpp:22
+
+class D2 : public B2, public Mixin {};
+

I think there should be some comment that explains why this test is structured 
like this (maybe point out where the tail padding change is happening).



Comment at: lldb/test/Shell/Expr/Inputs/layout.cpp:40
+  D2 d2;
+  D3 d3;
+

Do we actually need these locals in addition to the globals?


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

https://reviews.llvm.org/D83008



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Fixed here to get the bot running again: 
https://github.com/llvm/llvm-project/commit/7308e1432624f02d4e652ffa70e40d0eaa89fdb3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This broke the Green Dragon build for Clang: 
http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/

  FAILED: bin/diagtool 
  : && /Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++ 
 -Wdocumentation -fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -fmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache
 -fcxx-modules -Xclang -fmodules-local-submodule-visibility -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3  -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/diagtool_main.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/DiagTool.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/DiagnosticNames.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/FindDiagnosticID.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/ListWarnings.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/ShowEnabledWarnings.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/TreeView.cpp.o  -o 
bin/diagtool  -Wl,-rpath,@loader_path/../lib  lib/libLLVMSupport.a  
lib/libclangBasic.a  lib/libclangFrontend.a  lib/libclangDriver.a  
lib/libclangParse.a  lib/libclangSerialization.a  lib/libclangSema.a  
lib/libclangEdit.a  lib/libclangAnalysis.a  lib/libclangASTMatchers.a  
lib/libclangAST.a  lib/libclangLex.a  lib/libclangBasic.a  
lib/libLLVMFrontendOpenMP.a  lib/libLLVMTransformUtils.a  lib/libLLVMAnalysis.a 
 lib/libLLVMObject.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  
lib/libLLVMDebugInfoCodeView.a  lib/libLLVMDebugInfoMSF.a  lib/libLLVMTextAPI.a 
 lib/libLLVMBitReader.a  lib/libLLVMOption.a  lib/libLLVMProfileData.a  
lib/libLLVMCore.a  lib/libLLVMBinaryFormat.a  lib/libLLVMRemarks.a  
lib/libLLVMBitstreamReader.a  lib/libLLVMSupport.a  -lz  -lcurses  -lm  
lib/libLLVMDemangle.a && :
  Undefined symbols for architecture x86_64:
"clang::ento::PackageInfo::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::PackageInfo::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
"clang::ento::CmdLineOption::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::CmdLineOption::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
"clang::ento::CheckerInfo::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::CheckerInfo::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
  ld: symbol(s) not found for architecture x86_64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D83008#2131796 , @tschuett wrote:

> You could try:
>
>   clangxx_host -Xclang -fdump-record-layouts %p/Inputs/layout.cpp -emit-llvm 
> -o /dev/null
>
>
> You could commit the record layouts without your fix to show the differences 
> that you patch makes.


Yeah that's I think the proper way to check the results, but IIRC the problem 
with testing this in Clang is that `foverride-record-layout=` (the testing 
approach for this code) doesn't support specifying base class offset), so we 
can't even *trigger* the bug itself in Clang itself. That what I was referring 
to with "layout overwrite JSON file" in my comment above.


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

https://reviews.llvm.org/D83008



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

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

[Raphael Isemann via Phabricator via cfe-commits]

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

Thanks for tracking this down, this is a really nasty bug...

The fix itself is obviously fine, but I think I'm out of the loop regarding the 
testing strategy. We talked about adding a Clang test for this with the help of 
this layout overwrite JSON file. I assume that extending this to cover virtual 
bases turned out to be more complicated than expected? FWIW, I'm not 
necessarily the biggest fan of this Clang test option so I would be fine if we 
leave it as-is.

I think having an LLDB test is a good idea, but it's not clear why it's a Shell 
test. If I understand correctly this test requires running on arm64e (so, a 
remote test target), but from what I remember all the remote platform support 
is only in dotest.py? Also pretty much all other expression evaluation tests 
and the utilities for that are in the Python/API test infrastructure, so it's a 
bit out of place.

Also I think the test can be in general much simpler than an arm64-specific 
test. We get all base class offsets wrong in LLDB, so we can just write a 
simple test where you change the layout of some structs in a way that it 
doesn't fit the default layout. E.g., just putting `alignas` on a base class 
and then reading fields should be enough to trigger the same bug.


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

https://reviews.llvm.org/D83008



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

[PATCH] D80522: [Analyzer] [NFC] Parameter Regions

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This introduced a compiler warning:

  
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1102:23:
 warning: 'getDecl' overrides a member function but is not marked 'override' 
[-Winconsistent-missing-override]
const ObjCIvarDecl *getDecl() const;
^
  
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:898:28:
 note: overridden virtual function is here
virtual const ValueDecl *getDecl() const = 0;
 ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80522



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

[PATCH] D78086: [ASTImporter] Also import overwritten file buffers

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f1e81f1c0ac: [ASTImporter] Also import overwritten file 
buffers (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78086

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 
 #include "clang/AST/DeclContextInternals.h"
 #include "gtest/gtest.h"
@@ -5896,6 +5897,61 @@
   EXPECT_FALSE(ToSM.isBeforeInTranslationUnit(Location2, Location1));
 }
 
+TEST_P(ImportSourceLocations, NormalFileBuffer) {
+  // Test importing normal file buffers.
+
+  std::string Path = "input0.c";
+  std::string Source = "int X;";
+  TranslationUnitDecl *FromTU = getTuDecl(Source, Lang_C, Path);
+
+  SourceLocation ImportedLoc;
+  {
+// Import the VarDecl to trigger the importing of the FileID.
+auto Pattern = varDecl(hasName("X"));
+VarDecl *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ImportedLoc = Import(FromD, Lang_C)->getLocation();
+  }
+
+  // Make sure the imported buffer has the original contents.
+  SourceManager  = ToAST->getSourceManager();
+  FileID ImportedID = ToSM.getFileID(ImportedLoc);
+  EXPECT_EQ(Source, ToSM.getBuffer(ImportedID, SourceLocation())->getBuffer());
+}
+
+TEST_P(ImportSourceLocations, OverwrittenFileBuffer) {
+  // Test importing overwritten file buffers.
+
+  std::string Path = "input0.c";
+  TranslationUnitDecl *FromTU = getTuDecl("int X;", Lang_C, Path);
+
+  // Overwrite the file buffer for our input file with new content.
+  const std::string Contents = "overwritten contents";
+  SourceLocation ImportedLoc;
+  {
+SourceManager  = FromTU->getASTContext().getSourceManager();
+clang::FileManager  = FromSM.getFileManager();
+const clang::FileEntry  =
+*FM.getVirtualFile(Path, static_cast(Contents.size()), 0);
+
+llvm::SmallVector Buffer;
+Buffer.append(Contents.begin(), Contents.end());
+auto FileContents =
+std::make_unique(std::move(Buffer), 
Path);
+FromSM.overrideFileContents(, std::move(FileContents));
+
+// Import the VarDecl to trigger the importing of the FileID.
+auto Pattern = varDecl(hasName("X"));
+VarDecl *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ImportedLoc = Import(FromD, Lang_C)->getLocation();
+  }
+
+  // Make sure the imported buffer has the overwritten contents.
+  SourceManager  = ToAST->getSourceManager();
+  FileID ImportedID = ToSM.getFileID(ImportedLoc);
+  EXPECT_EQ(Contents,
+ToSM.getBuffer(ImportedID, SourceLocation())->getBuffer());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
   // Test if import of these packed and aligned attributes does not trigger an
   // error situation where source location from 'From' context is referenced in
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8560,7 +8560,7 @@
   } else {
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
 
-if (!IsBuiltin) {
+if (!IsBuiltin && !Cache->BufferOverridden) {
   // Include location of this file.
   ExpectedSLoc ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
   if (!ToIncludeLoc)


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 
 #include "clang/AST/DeclContextInternals.h"
 #include "gtest/gtest.h"
@@ -5896,6 +5897,61 @@
   EXPECT_FALSE(ToSM.isBeforeInTranslationUnit(Location2, Location1));
 }
 
+TEST_P(ImportSourceLocations, NormalFileBuffer) {
+  // Test importing normal file buffers.
+
+  std::string Path = "input0.c";
+  std::string Source = "int X;";
+  TranslationUnitDecl *FromTU = getTuDecl(Source, Lang_C, Path);
+
+  SourceLocation ImportedLoc;
+  {
+// Import the VarDecl to trigger the importing of the FileID.
+auto Pattern = varDecl(hasName("X"));
+VarDecl *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ImportedLoc = Import(FromD, Lang_C)->getLocation();
+  }
+
+  // Make sure the imported buffer has the original contents.
+  SourceManager  = ToAST->getSourceManager();
+  FileID ImportedID = 

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

Beside Gabors comment I think I'm happy with this. Thanks!




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5939
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {

"is completed" -> "it completed"



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5954
+   SmallVectorImpl ) override {
+DC->setHasExternalLexicalStorage(true);
+  }

You can remove this as you changed the check in the ASTImporter to only check 
for the existence of an ExternalASTSource. 


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

https://reviews.llvm.org/D78000



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