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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
