[PATCH] D106862: [clang][modules] Avoid creating partial FullSourceLoc for explicit modules imports

[Jan Svoboda via Phabricator via cfe-commits]

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);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106862.361981.patch
Type: text/x-patch
Size: 1643 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210727/b5dbdc85/attachment.bin>