[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 23 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:1470
+    /// diagnostics should be emitted.
+    SmallVector<Decl *, 4> DeclsToCheckForDeferredDiags;
+
----------------
rjmccall wrote:
> This needs to be saved and restored in modules / PCH.
done


================
Comment at: clang/lib/Sema/Sema.cpp:1468
   }
-  S.DeviceDeferredDiags.erase(It);
 
----------------
yaxunl wrote:
> Fznamznon wrote:
> > This particular change causes duplication of deferred diagnostics.
> > Consider the following example (please correct me if I'm doing something wrong, I'm not an expert in OpenMP):
> > 
> > ```
> > int foobar1() { throw 1; } // error is expected here
> > 
> > // let's try to use foobar1 in the code where exceptions aren't allowed
> > #pragma omp declare target    
> > int (*B)() = &foobar1;        
> > #pragma omp end declare target
> > 
> > // and in some other place let's use foobar1 in device code again
> > #pragma omp declare target    
> > int a = foobar1();            
> > #pragma omp end declare target
> > 
> > ```
> > Then diagnostic for `foobar1`  will be duplicated for each use of `foobar1` under `target` directive.
> > I first experienced this behavior not with OpenMP, so I suppose reproducer can be done for each programming model which uses deferred diagnostics.
> The change is intentional so that each call chain causing the diagnostic can be identified. The drawback is that it is more verbose.
> 
> I can change this behavior so that the diagnostic will be emitted only for the first call chain that causes the diagnostic, if less verbose diagnostics is preferred.
the change is intentional to report all use chains which result in deferred diagnostics, otherwise user may fix one issue then see another issue, instead of see all of the issues in one compilation.


================
Comment at: clang/lib/Sema/Sema.cpp:1472
   if (HasWarningOrError && ShowCallStack)
-    emitCallStackNotes(S, FD);
+    emitCallStackNotes(*this, FD);
+}
----------------
rjmccall wrote:
> Hmm.  I know this is existing code, but I just realized something.  I think it's okay to not emit the notes on every diagnostic, but you might want to emit them on the first diagnostic from a function instead of after the last.  If the real bug is that the program is using something it's not supposed to use, and there are enough errors in that function to reach the error limit, then the diagnostics emitter will associate these notes with a diagnostic it's suppressing and so presumably suppress them as well, leaving the user with no way to find this information.
done


================
Comment at: clang/lib/Sema/Sema.cpp:1494
+      visitUsedDecl(E->getLocation(), FD);
+    }
+  }
----------------
rjmccall wrote:
> This needs to trigger if you use a variable with delayed diagnostics, too, right?
> 
> When you add these methods to `UsedDeclVisitor`, you'll be able to remove them here.
fixed


================
Comment at: clang/lib/Sema/Sema.cpp:1512
+    Inherited::VisitCapturedStmt(Node);
+  }
+
----------------
rjmccall wrote:
> Should this also go in the base `UsedDeclVisitor`?  I'm less sure about that because the captured statement is really always a part of the enclosing function, right?  Should the delay mechanism just be looking through local sub-contexts instead?
yes this one should also go to UsedDeclVisitor since this statement causes a RecordDecl generated which includes a FunctionDecl for a kernel, therefore this RecordDecl needs to be visited as used decl. I am not sure if other sub-context have the same effect. If so, I think they need to be handled case by case.


================
Comment at: clang/lib/Sema/UsedDeclVisitor.h:21
+template <class Derived>
+class UsedDeclVisitor : public EvaluatedExprVisitor<Derived> {
+protected:
----------------
rjmccall wrote:
> Could you add this in a separate patch?
extracted to https://reviews.llvm.org/D76262


================
Comment at: clang/lib/Sema/UsedDeclVisitor.h:30
+
+  Derived &asImpl() { return *static_cast<Derived *>(this); }
+
----------------
rjmccall wrote:
> There should definitely be cases in here for every expression that uses a declaration, including both `DeclRefExpr` and `MemberExpr`.  Those might be overridden in subclasses, but that's their business; the default behavior should be to visit every used decl.
done


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

https://reviews.llvm.org/D70172