jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith, arphaman, christof.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Some parts of the codebase use `FullSourceLoc` -- a wrapper around regular
`SourceLocation` and `SourceManager`.
>From the existing code, it looks like the wrapper can be in two states:
- valid `SourceLocation`, present `SourceManager`,
- invalid `SourceLocation`, missing `SourceManager`.
When importing an explicitly-built module, the `SourceLocation` of the `import`
directive is invalid, since the module is "imported" by the command-line
argument. This causes an assertion failure in `hasManager` when serializing
diagnostics containing this `SourceLocation` to file.
This patch makes sure the `FullSourceLoc` for invalid import `SourceLocations`
omits the `SourceManager`, upholding the class invariant.
I also tried more defensive approach: making sure the wrapped `SourceLocation`
is always valid in the `FullSourceLoc` constructor. However, some clients
apparently rely on the fact they can construct invalid source locations and
later modify their `ID` to make them valid.
Another approach (that works) is checking `Loc.isValid() && Loc.hasManager()`
in `SDiagsRenderer::emitNote` before working with the `FullSourceLoc`. I think
this is what all users of the wrapper should do, but I think that's something
people more closely familiar with this code to consider. (@christof)
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D106862
Files:
clang/lib/Basic/SourceLocation.cpp
clang/test/Modules/Inputs/explicit-build-diags/a.h
clang/test/Modules/Inputs/explicit-build-diags/module.modulemap
clang/test/Modules/explicit-build-diags.cpp
Index: clang/test/Modules/explicit-build-diags.cpp
===================================================================
--- /dev/null
+++ clang/test/Modules/explicit-build-diags.cpp
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: %clang_cc1 -fmodules -x c++
%S/Inputs/explicit-build-diags/module.modulemap -fmodule-name=a -emit-module -o
%t/a.pcm
+// RUN: %clang_cc1 -fmodules -Wdeprecated-declarations
-fdiagnostics-show-note-include-stack -serialize-diagnostic-file %t/tu.dia \
+// RUN: -I %S/Inputs/explicit-build-diags -fmodule-file=%t/a.pcm
-fsyntax-only %s
+
+#include "a.h"
+
+void foo() { a(); }
Index: clang/test/Modules/Inputs/explicit-build-diags/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/explicit-build-diags/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" }
Index: clang/test/Modules/Inputs/explicit-build-diags/a.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/explicit-build-diags/a.h
@@ -0,0 +1 @@
+void a() __attribute__((deprecated));
Index: clang/lib/Basic/SourceLocation.cpp
===================================================================
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -199,6 +199,10 @@
std::pair<SourceLocation, StringRef> ImportLoc =
SrcMgr->getModuleImportLoc(*this);
+
+ if (!ImportLoc.first.isValid())
+ return std::make_pair(FullSourceLoc(), ImportLoc.second);
+
return std::make_pair(FullSourceLoc(ImportLoc.first, *SrcMgr),
ImportLoc.second);
}
Index: clang/test/Modules/explicit-build-diags.cpp
===================================================================
--- /dev/null
+++ clang/test/Modules/explicit-build-diags.cpp
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: %clang_cc1 -fmodules -x c++ %S/Inputs/explicit-build-diags/module.modulemap -fmodule-name=a -emit-module -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules -Wdeprecated-declarations -fdiagnostics-show-note-include-stack -serialize-diagnostic-file %t/tu.dia \
+// RUN: -I %S/Inputs/explicit-build-diags -fmodule-file=%t/a.pcm -fsyntax-only %s
+
+#include "a.h"
+
+void foo() { a(); }
Index: clang/test/Modules/Inputs/explicit-build-diags/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/explicit-build-diags/module.modulemap
@@ -0,0 +1 @@
+module a { header "a.h" }
Index: clang/test/Modules/Inputs/explicit-build-diags/a.h
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/explicit-build-diags/a.h
@@ -0,0 +1 @@
+void a() __attribute__((deprecated));
Index: clang/lib/Basic/SourceLocation.cpp
===================================================================
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -199,6 +199,10 @@
std::pair<SourceLocation, StringRef> ImportLoc =
SrcMgr->getModuleImportLoc(*this);
+
+ if (!ImportLoc.first.isValid())
+ return std::make_pair(FullSourceLoc(), ImportLoc.second);
+
return std::make_pair(FullSourceLoc(ImportLoc.first, *SrcMgr),
ImportLoc.second);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
