[PATCH] D25704: [CUDA] When we emit an error that might have been deferred, also print a callstack.

[Justin Lebar via cfe-commits]

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

I'm going to submit this and send a patch to reuse FunctionDeclAndLoc.  But I'm happy to add a comment about the note as well.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6707
 
+def note_called_by : Note<"called by %0">;
 def err_kern_type_not_void_return : Error<
----------------
rnk wrote:
> Do you think it's worth trying to indicate why the root of the "called by" notes must be emitted? I'm not suggesting we do it in this patch, just wondering.
It seems just like e.g. note_template_class_instantiation_here, so I am not entirely sure what is the point of confusion.  But if you can clarify that, I am happy to add a comment in a separate patch -- this stuff is already quite complicated, so I'm in favor of whatever we can do to make it clearer.


================
Comment at: clang/include/clang/Sema/Sema.h:9259
   /// use this to avoid emitting the same deferred diag twice.
-  llvm::DenseSet<std::pair<FunctionDecl *, unsigned>> LocsWithCUDACallDiags;
+  llvm::DenseSet<std::pair<CanonicalDeclPtr<FunctionDecl>, unsigned>>
+      LocsWithCUDACallDiags;
----------------
rnk wrote:
> So, part of me wants to use FunctionDeclAndLoc here instead of std::pair, but then you'd have to bring back all the hashing machinery instead of getting it for free. Up to you, I guess.
Oh, and now make its hash function depend on both members instead of just the FD?

I actually like that change, but let me make it in a separate patch.


================
Comment at: clang/include/clang/Sema/Sema.h:9274
+  llvm::DenseMap</* Callee = */ CanonicalDeclPtr<FunctionDecl>,
+                 /* Caller = */ FunctionDeclAndLoc>
+      CUDAKnownEmittedFns;
----------------
rnk wrote:
> I guess there can be many callers, but we always record the first one that caused this function to be emitted.
Yes, exactly.

I was briefly worried about cycles here, but I think we're OK because ultimately we need to end up at an a priori known-emitted function, and that's a root in this tree.


https://reviews.llvm.org/D25704