Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Justin Lebar via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL278735: Add the notion of deferred diagnostics. (authored by 
jlebar).

Changed prior to commit:
  https://reviews.llvm.org/D23241?vs=68063=68073#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D23241

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/lib/AST/Decl.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.h

Index: cfe/trunk/lib/AST/Decl.cpp
===
--- cfe/trunk/lib/AST/Decl.cpp
+++ cfe/trunk/lib/AST/Decl.cpp
@@ -3446,6 +3446,20 @@
   return 0;
 }
 
+void FunctionDecl::addDeferredDiag(PartialDiagnosticAt PD) {
+  getASTContext().getDeferredDiags()[this].push_back(std::move(PD));
+}
+
+std::vector FunctionDecl::takeDeferredDiags() const {
+  auto  = getASTContext().getDeferredDiags();
+  auto It = DD.find(this);
+  if (It == DD.end())
+return {};
+  auto Ret = std::move(It->second);
+  DD.erase(It);
+  return Ret;
+}
+
 //===--===//
 // FieldDecl Implementation
 //===--===//
Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -497,6 +497,16 @@
   EmitVersionIdentMetadata();
 
   EmitTargetMetadata();
+
+  // Emit any deferred diagnostics gathered during codegen.  We didn't emit them
+  // when we first discovered them because that would have halted codegen,
+  // preventing us from gathering other deferred diags.
+  for (const PartialDiagnosticAt  : DeferredDiags) {
+SourceLocation Loc = DiagAt.first;
+const PartialDiagnostic  = DiagAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
 }
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
@@ -2872,6 +2882,33 @@
  llvm::GlobalValue *GV) {
   const auto *D = cast(GD.getDecl());
 
+  // Emit this function's deferred diagnostics, if none of them are errors.  If
+  // any of them are errors, don't codegen the function, but also don't emit any
+  // of the diagnostics just yet.  Emitting an error during codegen stops
+  // further codegen, and we want to display as many deferred diags as possible.
+  // We'll emit the now twice-deferred diags at the very end of codegen.
+  //
+  // (If a function has both error and non-error diags, we don't emit the
+  // non-error diags here, because order can be significant, e.g. with notes
+  // that follow errors.)
+  auto Diags = D->takeDeferredDiags();
+  bool HasError = llvm::any_of(Diags, [this](const PartialDiagnosticAt ) {
+return getDiags().getDiagnosticLevel(PDAt.second.getDiagID(), PDAt.first) >=
+   DiagnosticsEngine::Error;
+  });
+  if (HasError) {
+DeferredDiags.insert(DeferredDiags.end(),
+ std::make_move_iterator(Diags.begin()),
+ std::make_move_iterator(Diags.end()));
+return;
+  }
+  for (PartialDiagnosticAt  : Diags) {
+const SourceLocation  = PDAt.first;
+const PartialDiagnostic  = PDAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
+
   // Compute the function info and LLVM type.
   const CGFunctionInfo  = getTypes().arrangeGlobalDeclaration(GD);
   llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
Index: cfe/trunk/lib/CodeGen/CodeGenModule.h
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.h
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h
@@ -490,6 +490,10 @@
   /// MDNodes.
   llvm::DenseMap MetadataIdMap;
 
+  /// Diags gathered from FunctionDecl::takeDeferredDiags().  Emitted at the
+  /// very end of codegen.
+  std::vector> DeferredDiags;
+
 public:
   CodeGenModule(ASTContext , const HeaderSearchOptions ,
 const PreprocessorOptions ,
Index: cfe/trunk/include/clang/AST/Decl.h
===
--- cfe/trunk/include/clang/AST/Decl.h
+++ cfe/trunk/include/clang/AST/Decl.h
@@ -2271,6 +2271,14 @@
   /// returns 0.
   unsigned getMemoryFunctionKind() const;
 
+  /// Add a diagnostic to be emitted if and when this function is codegen'ed.
+  void addDeferredDiag(PartialDiagnosticAt PD);
+
+  /// Gets this object's list of deferred diagnostics, if there are any.
+  ///
+  /// Although this is logically const, it clears our list of deferred diags.
+  std::vector takeDeferredDiags() const;
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool 

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Reid Kleckner via cfe-commits]

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

lgtm


https://reviews.llvm.org/D23241



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

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Justin Lebar via cfe-commits]

jlebar updated this revision to Diff 68063.
jlebar added a comment.

Move deferred diags storage into ASTContext.


https://reviews.llvm.org/D23241

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -490,6 +490,10 @@
   /// MDNodes.
   llvm::DenseMap MetadataIdMap;
 
+  /// Diags gathered from FunctionDecl::takeDeferredDiags().  Emitted at the
+  /// very end of codegen.
+  std::vector> DeferredDiags;
+
 public:
   CodeGenModule(ASTContext , const HeaderSearchOptions ,
 const PreprocessorOptions ,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -497,6 +497,16 @@
   EmitVersionIdentMetadata();
 
   EmitTargetMetadata();
+
+  // Emit any deferred diagnostics gathered during codegen.  We didn't emit them
+  // when we first discovered them because that would have halted codegen,
+  // preventing us from gathering other deferred diags.
+  for (const PartialDiagnosticAt  : DeferredDiags) {
+SourceLocation Loc = DiagAt.first;
+const PartialDiagnostic  = DiagAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
 }
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
@@ -2872,6 +2882,33 @@
  llvm::GlobalValue *GV) {
   const auto *D = cast(GD.getDecl());
 
+  // Emit this function's deferred diagnostics, if none of them are errors.  If
+  // any of them are errors, don't codegen the function, but also don't emit any
+  // of the diagnostics just yet.  Emitting an error during codegen stops
+  // further codegen, and we want to display as many deferred diags as possible.
+  // We'll emit the now twice-deferred diags at the very end of codegen.
+  //
+  // (If a function has both error and non-error diags, we don't emit the
+  // non-error diags here, because order can be significant, e.g. with notes
+  // that follow errors.)
+  auto Diags = D->takeDeferredDiags();
+  bool HasError = llvm::any_of(Diags, [this](const PartialDiagnosticAt ) {
+return getDiags().getDiagnosticLevel(PDAt.second.getDiagID(), PDAt.first) >=
+   DiagnosticsEngine::Error;
+  });
+  if (HasError) {
+DeferredDiags.insert(DeferredDiags.end(),
+ std::make_move_iterator(Diags.begin()),
+ std::make_move_iterator(Diags.end()));
+return;
+  }
+  for (PartialDiagnosticAt  : Diags) {
+const SourceLocation  = PDAt.first;
+const PartialDiagnostic  = PDAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
+
   // Compute the function info and LLVM type.
   const CGFunctionInfo  = getTypes().arrangeGlobalDeclaration(GD);
   llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3446,6 +3446,20 @@
   return 0;
 }
 
+void FunctionDecl::addDeferredDiag(PartialDiagnosticAt PD) {
+  getASTContext().getDeferredDiags()[this].push_back(std::move(PD));
+}
+
+std::vector FunctionDecl::takeDeferredDiags() const {
+  auto  = getASTContext().getDeferredDiags();
+  auto It = DD.find(this);
+  if (It == DD.end())
+return {};
+  auto Ret = std::move(It->second);
+  DD.erase(It);
+  return Ret;
+}
+
 //===--===//
 // FieldDecl Implementation
 //===--===//
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -2271,6 +2271,14 @@
   /// returns 0.
   unsigned getMemoryFunctionKind() const;
 
+  /// Add a diagnostic to be emitted if and when this function is codegen'ed.
+  void addDeferredDiag(PartialDiagnosticAt PD);
+
+  /// Gets this object's list of deferred diagnostics, if there are any.
+  ///
+  /// Although this is logically const, it clears our list of deferred diags.
+  std::vector takeDeferredDiags() const;
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) {
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ 

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Reid Kleckner via cfe-commits]

rnk added a comment.

Sorry for the delay, was trying to stay focused on debugging



Comment at: clang/include/clang/AST/Decl.h:1647
@@ +1646,3 @@
+  /// clears this list) needs to be a logically-const operation.
+  mutable std::unique_ptr DeferredDiags;
+

Decl memory is pretty precious, I think you'll be better off with a map from 
Decl* to TinyPtrVector in the ASTContext. Just pass the ASTContext into 
takeDeferredDiags.


https://reviews.llvm.org/D23241



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

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Justin Lebar via cfe-commits]

jlebar added a comment.

Reid, I'd still like you to have a look at this one if you don't mind, since 
it's outside my and Art's core competencies.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2886
@@ +2885,3 @@
+  // Check if this function has diagnostics that should be emitted when we
+  // codegen it.  If so, don't eit this function definition, but don't emit the
+  // diags just yet.  Emitting an error during codegen stops codegen, and we

tra wrote:
> eit->emit.  
> "don't do X, but don't do Y" construction sounds awkward to me.
> I'd reword the whole comment in terms of what the code does -- if there are 
> diagnostics, only collect them to emit at the end of codegen. Otherwise, 
> proceed to emit function definition.
> 
Thanks.  I tried to reword it, phal.

I also realized that this was skipping functions if they contained *any* 
deferred diags, but that's not right -- we only want to skip functions that 
contain deferred errors.  Fixed that too.

I wish I could use StoredDiagnostic instead of PartialDiagnosticAt, but I don't 
see a way to create a StoredDiagnostic for an error without setting the 
HasError bit in the DiagnosticEngine.  I need to carefully avoid setting that 
bit, otherwise we don't even make it to codegen.  (In fact it looks like the 
code for emitting diagnostics assumes that creating a StoredDiagnostic sets the 
bit, because I don't see it setting that bit when we emit the StoredDiagnostic.)


https://reviews.llvm.org/D23241



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

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Justin Lebar via cfe-commits]

jlebar updated this revision to Diff 67367.
jlebar marked an inline comment as done.
jlebar added a comment.

Address review comments, and don't abort codegen'ing a function if it has only
deferred warnings -- check specifically for errors.


https://reviews.llvm.org/D23241

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -490,6 +490,10 @@
   /// MDNodes.
   llvm::DenseMap MetadataIdMap;
 
+  /// Diags gathered from FunctionDecl::takeDeferredDiags().  Emitted at the
+  /// very end of codegen.
+  std::vector> DeferredDiags;
+
 public:
   CodeGenModule(ASTContext , const HeaderSearchOptions ,
 const PreprocessorOptions ,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -497,6 +497,16 @@
   EmitVersionIdentMetadata();
 
   EmitTargetMetadata();
+
+  // Emit any deferred diagnostics gathered during codegen.  We didn't emit them
+  // when we first discovered them because that would have halted codegen,
+  // preventing us from gathering other deferred diags.
+  for (const PartialDiagnosticAt  : DeferredDiags) {
+SourceLocation Loc = DiagAt.first;
+const PartialDiagnostic  = DiagAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
 }
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
@@ -2872,6 +2882,33 @@
  llvm::GlobalValue *GV) {
   const auto *D = cast(GD.getDecl());
 
+  // Emit this function's deferred diagnostics, if none of them are errors.  If
+  // any of them are errors, don't codegen the function, but also don't emit any
+  // of the diagnostics just yet.  Emitting an error during codegen stops
+  // further codegen, and we want to display as many deferred diags as possible.
+  // We'll emit the now twice-deferred diags at the very end of codegen.
+  //
+  // (If a function has both error and non-error diags, we don't emit the
+  // non-error diags here, because order can be significant, e.g. with notes
+  // that follow errors.)
+  auto Diags = D->takeDeferredDiags();
+  bool HasError = llvm::any_of(Diags, [this](const PartialDiagnosticAt ) {
+return getDiags().getDiagnosticLevel(PDAt.second.getDiagID(), PDAt.first) >=
+   DiagnosticsEngine::Error;
+  });
+  if (HasError) {
+DeferredDiags.insert(DeferredDiags.end(),
+ std::make_move_iterator(Diags.begin()),
+ std::make_move_iterator(Diags.end()));
+return;
+  }
+  for (PartialDiagnosticAt  : Diags) {
+const SourceLocation  = PDAt.first;
+const PartialDiagnostic  = PDAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
+
   // Compute the function info and LLVM type.
   const CGFunctionInfo  = getTypes().arrangeGlobalDeclaration(GD);
   llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3436,6 +3436,22 @@
   return 0;
 }
 
+void FunctionDecl::addDeferredDiag(PartialDiagnosticAt PD) {
+  if (!DeferredDiags)
+DeferredDiags = llvm::make_unique();
+  DeferredDiags->emplace_back(PD);
+}
+
+std::vector FunctionDecl::takeDeferredDiags() const {
+  if (!DeferredDiags)
+return {};
+  assert(!DeferredDiags->empty() &&
+ "DeferredDiags should be non-null only if it's also non-empty.");
+  auto Ret = std::move(*DeferredDiags);
+  DeferredDiags.reset();
+  return Ret;
+}
+
 //===--===//
 // FieldDecl Implementation
 //===--===//
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -1638,6 +1638,14 @@
   /// declaration name embedded in the DeclaratorDecl base class.
   DeclarationNameLoc DNLoc;
 
+  /// Storage for diagnostics deferred until this function is codegen'ed (if it
+  /// ever is).
+  ///
+  /// These are rarely used, so we use a pointer-to-vector to save two words
+  /// inside FunctionDecl.  This is mutable because emitting diagnostics (which
+  /// clears this list) needs to be a logically-const operation.
+  mutable std::unique_ptr DeferredDiags;
+
   /// \brief Specify that this function declaration is actually a function
   

Re: [PATCH] D23241: Add the notion of deferred diagnostics.

[Artem Belevich via cfe-commits]

tra added inline comments.


Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2886
@@ +2885,3 @@
+  // Check if this function has diagnostics that should be emitted when we
+  // codegen it.  If so, don't eit this function definition, but don't emit the
+  // diags just yet.  Emitting an error during codegen stops codegen, and we

eit->emit.  
"don't do X, but don't do Y" construction sounds awkward to me.
I'd reword the whole comment in terms of what the code does -- if there are 
diagnostics, only collect them to emit at the end of codegen. Otherwise, 
proceed to emit function definition.



https://reviews.llvm.org/D23241



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

[PATCH] D23241: Add the notion of deferred diagnostics.

[Justin Lebar via cfe-commits]

jlebar created this revision.
jlebar added a reviewer: rnk.
jlebar added subscribers: tra, cfe-commits.

This patch lets you create diagnostics that are emitted if and only if a
particular FunctionDecl is codegen'ed.

This is necessary for CUDA, where some constructs -- e.g. calls from
host+device functions to host functions when compiling for device -- are
allowed to appear in semantically-correct programs, but only if they're
never codegen'ed.

https://reviews.llvm.org/D23241

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -490,6 +490,10 @@
   /// MDNodes.
   llvm::DenseMap MetadataIdMap;
 
+  /// Diags gathered from FunctionDecl::takeDeferredDiags().  Emitted at the
+  /// very end of codegen.
+  std::vector> DeferredDiags;
+
 public:
   CodeGenModule(ASTContext , const HeaderSearchOptions ,
 const PreprocessorOptions ,
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -497,6 +497,16 @@
   EmitVersionIdentMetadata();
 
   EmitTargetMetadata();
+
+  // Emit any deferred diagnostics gathered during codegen.  We didn't emit them
+  // when we first discovered them because that would have halted codegen,
+  // preventing us from gathering other deferred diags.
+  for (const PartialDiagnosticAt  : DeferredDiags) {
+SourceLocation Loc = DiagAt.first;
+const PartialDiagnostic  = DiagAt.second;
+DiagnosticBuilder Builder(getDiags().Report(Loc, PD.getDiagID()));
+PD.Emit(Builder);
+  }
 }
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
@@ -2872,6 +2882,19 @@
  llvm::GlobalValue *GV) {
   const auto *D = cast(GD.getDecl());
 
+  // Check if this function has diagnostics that should be emitted when we
+  // codegen it.  If so, don't eit this function definition, but don't emit the
+  // diags just yet.  Emitting an error during codegen stops codegen, and we
+  // want to display as many deferred diags as possible.  We'll emit the now
+  // twice-deferred diags at the very end of codegen.
+  auto Diags = D->takeDeferredDiags();
+  bool HasDiags = !Diags.empty();
+  DeferredDiags.insert(DeferredDiags.end(),
+   std::make_move_iterator(Diags.begin()),
+   std::make_move_iterator(Diags.end()));
+  if (HasDiags)
+return;
+
   // Compute the function info and LLVM type.
   const CGFunctionInfo  = getTypes().arrangeGlobalDeclaration(GD);
   llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3436,6 +3436,22 @@
   return 0;
 }
 
+void FunctionDecl::addDeferredDiag(PartialDiagnosticAt PD) {
+  if (!DeferredDiags)
+DeferredDiags = llvm::make_unique();
+  DeferredDiags->emplace_back(PD);
+}
+
+std::vector FunctionDecl::takeDeferredDiags() const {
+  if (!DeferredDiags)
+return {};
+  assert(!DeferredDiags->empty() &&
+ "DeferredDiags should be non-null only if it's also non-empty.");
+  auto Ret = std::move(*DeferredDiags);
+  DeferredDiags.reset();
+  return Ret;
+}
+
 //===--===//
 // FieldDecl Implementation
 //===--===//
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -1638,6 +1638,14 @@
   /// declaration name embedded in the DeclaratorDecl base class.
   DeclarationNameLoc DNLoc;
 
+  /// Storage for diagnostics deferred until this function is codegen'ed (if it
+  /// ever is).
+  ///
+  /// These are rarely used, so we use a pointer-to-vector to save two words
+  /// inside FunctionDecl.  This is mutable because emitting diagnostics (which
+  /// clears this list) needs to be a logically-const operation.
+  mutable std::unique_ptr DeferredDiags;
+
   /// \brief Specify that this function declaration is actually a function
   /// template specialization.
   ///
@@ -2271,6 +2279,14 @@
   /// returns 0.
   unsigned getMemoryFunctionKind() const;
 
+  /// Add a diagnostic to be emitted if and when this function is codegen'ed.
+  void addDeferredDiag(PartialDiagnosticAt PD);
+
+  /// Gets this object's list of deferred diagnostics, if there are any.
+  ///
+  /// Although this is logically const, it clears our list of deferred diags.
+