[PATCH] D72523: [remark][diagnostics] Using clang diagnostic handler for IR input files

[Rong Xu via Phabricator via llvm-commits]

xur marked 3 inline comments as done.
xur added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:594
+  // SourceLoc.
+  if (Context == nullptr) {
+    return FullSourceLoc();
----------------
tejohnson wrote:
> Does this only happen with IR input? Does it always happen with IR input? If not, what happens when we have a non-null Context but IR input? It sounds like it should still always return an invalid Loc since there is no SourceManager? In that case can you set a flag on the BackendConsumer so we can bail out directly in the IR input case, instead of going through the motions only to get an invalid Loc back anyway? It would also be more clear.
Before this patch, IR input does not called to this function. IR input has a null Context (so it will seg-fault at the dereference at line 598). Context == nullptr should be only happening with the empty BackendConsume that installed in this patch. 
I tried to set Context but the whole point was to get SrouceManager which was not available for IR input.




================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:650
+  if (!Loc.isValid()) {
+    MsgStream << D.getLocationStr() << ": in function "
+              << D.getFunction().getName() << ' '
----------------
tejohnson wrote:
> Can you add a test for this one?
> 
> Also, what determines the format of the message when Loc is valid? I was trying to figure out where that got determined, but couldn't easily track it down. 
Do you mean adding a test to test the path of branch starting line 650?
(For the case of valid Loc, I don't think we need to test as it's not changed.)

If so, there is already a test for this:
clang/test/CodeGen/backend-unsupported-error.ll
I think this calls to llvm's diagnostic handler for IR input files (before this patch). The format is here:
DiagnosticInfoUnsupported::print(...) in  llvm/lib/IR/DiagnosticInfo.cpp

For the case of input other than IR files, the Diags should go to clang diagnostic handler.  This patch might change the behavior. 

I think I will check Context == nullptr directly (rather Loc.isValid). This way we don't need to call into getBestLocationFromDebugLoc(), and it will not affect non-IR input files.



================
Comment at: clang/test/CodeGen/thinlto-clang-diagnostic-handler-in-be.c:8
+// RUN: llvm-lto -thinlto -o %t %t1.bo
+// RUN: %clang -cc1 -O2 -fexperimental-new-pass-manager -x ir %t1.bo -fthinlto-index=%t.thinlto.bc -emit-obj -Rpass-analysis=info 2>&1 | FileCheck %s -check-prefix=CHECK-REMARK
+// RUN: llvm-profdata merge -o %t2.profdata %S/Inputs/thinlto_expect2.proftext
----------------
tejohnson wrote:
> In this case (since no -Wmisexpect), presumably we should not be getting the warning, whereas without your fix we were, correct? In that case, please add a NOT check to confirm that we don't get the message anymore.
Yes. You are right. 
I will add a NOT check to confirm this.


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

https://reviews.llvm.org/D72523