tejohnson added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:594
+ // SourceLoc.
+ if (Context == nullptr) {
+ return FullSourceLoc();
----------------
xur wrote:
> 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.
>
>
Since Context==nullptr iff IR input, then for clarity can you add a comment by
these checks that this case is specifically corresponding to when we have IR
input?
(Also, I was a little confused when I first reviewed this since I didn't
realize this Context variable was an ASTContext and not the LLVMContext being
passed in to the new BackendConsumer constructor, now I see why it is always
null in that case.)
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:650
+ if (!Loc.isValid()) {
+ MsgStream << D.getLocationStr() << ": in function "
+ << D.getFunction().getName() << ' '
----------------
xur wrote:
> 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.
>
> Do you mean adding a test to test the path of branch starting line 650?
Yes, but:
> If so, there is already a test for this:
> clang/test/CodeGen/backend-unsupported-error.ll
I see, I didn't realize this was already being tested with IR input. Nevermind
then.
> The format is here:
> DiagnosticInfoUnsupported::print(...) in llvm/lib/IR/DiagnosticInfo.cpp
Ok thanks. Looking at that file, I'm wondering if we can use those facilities
directly rather than cloning the formatting code here. I.e. instead of
constructing a MsgStream manually here, duplicating the code in
DiagnosticInfoUnsupported::print, couldn't this handling be something like:
DiagnosticPrinterRawOStream DP(MsgStream);
D.print(DP);
These lines would subsume the manual setup of MsgStream in the null Context
case, as well as the MsgStream << D.getMessage() line below it. Then you would
still call Diags.Report with MsgStream.str() as usual.
And ditto for the other 2 types of DiagnosticInfos below. This mechanism is
used in quite a few diagnostic handlers setup in LLVM (e.g. the one in
gold-plugin.cpp).
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
