[SourceManager] Skip module maps when searching files for macro arguments

[jkorous]

Summary

I have a test-case for this issue but it's pretty convoluted - we basically found this because clang_annotateTokens in libclang wasn't
attaching RefDeclExpr to macro argument identifiers. This was happening for a source file that includes module that has external submodule defined in its modulemap.

[jkorous]

Ping

[arphaman]

Could you please add a comment stating why it's necessary to skip the module map files? Is the problem caused by the module map file that's included in the include hierarchy when it's an external module?

Is there any way to reduce the convoluted test-case and add it to the patch?

[jkorous]

I could be missing something but it seemed that skipping module maps when we're searching for source locations is obviously the right thing to do. Don't really know if repeating what the code does in the comment would bring any value.

The reproducer I have is at libclang API level and involves module and external submodule which seems pretty far (both conceptually and code-path-wise) from macro args cache initialization.

The reproducer dir looks like this:

> find .
.
./modulefoo
./modulefoo/module.map
./modulefoo/dyld.h
./modulefoo/dyld.modulemap
./modulefoo/loaderfoo.h
./code.c

> cat code.c
#include "modulefoo/loaderfoo.h"
#define FOO234878786(a) (a + a)
const int a = 1;
const int b = FOO234878786(a);

> cat modulefoo/module.map
module MachOFoo [system] [extern_c] {

  extern module dyld "dyld.modulemap"

  module loaderfoo {
    header "loaderfoo.h"
    export *
  }

}

And this is the way to see observable difference:

c-index-test -test-annotate-tokens=code.c:4:1:4:100 code.c -fmodules -fmodules-cache-path=/tmp/mcp -x c

[jkorous]

This comment was removed by jkorous.

[arphaman]

Okay, that's fair. Is there any way to trigger the computeMacroArgsCache outside of libclang, to create a clang-specific test case?

[jkorous]

Attempt to write a unit test - so far can't reproduce even with unpatched master :(

The only place where SourceManager::computeMacroArgsCache is used is SourceManager::getMacroArgExpandedLocation.

I literally just copied and slightly modified existing unit-test which doesn't work - any help would be appreciated.

[jkorous]

Friendly ping.
I'd like to land this sooner than later - I agree it'd be great to have a regression test but my (possibly biased) opinion is that the change is also simple and obvious enough that we could land it without a test.

[arphaman]

Hmm if this unit tests doesn't reproduce it then you don't need to add it. But no further objections from me on this change.

[dexonsmith]

clang/lib/Basic/SourceManager.cpp

1794–1799

It seems unintentinoal to copy out the FileInfo here. Should this be a reference?

auto &File = Entry.getFile();

clang/lib/Basic/SourceManager.cpp

if (ID > 0) {
       return;
     }
 
     bool Invalid = false;
     const SrcMgr::SLocEntry &Entry = getSLocEntryByID(ID, &Invalid);
     if (Invalid)
       return;
     if (Entry.isFile()) {
-      SourceLocation IncludeLoc = Entry.getFile().getIncludeLoc();
+      auto File = Entry.getFile();
+      if (File.getFileCharacteristic() == C_User_ModuleMap ||
+          File.getFileCharacteristic() == C_System_ModuleMap)
+        continue;
+
+      SourceLocation IncludeLoc = File.getIncludeLoc();
       bool IncludedInFID =
           (IncludeLoc.isValid() && isInFileID(IncludeLoc, FID)) ||
           // Predefined header doesn't have a valid include location in main
           // file, but any files created by it should still be skipped when
           // computing macro args expanded in the main file.
           (FID == MainFileID && Entry.getFile().Filename == "<built-in>");
       if (IncludedInFID) {
         // Skip the files/macros of the #include'd file, we only care about

clang/unittests/Basic/SourceManagerTest.cpp

TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
   EXPECT_TRUE(loc1.isFileID());
   EXPECT_TRUE(SourceMgr.isMacroArgExpansion(loc2));
   EXPECT_TRUE(SourceMgr.isMacroArgExpansion(loc3));
   EXPECT_EQ(loc2, toks[1].getLocation());
   EXPECT_EQ(loc3, toks[2].getLocation());
   EXPECT_TRUE(defLoc2.isFileID());
 }
 
+TEST_F(SourceManagerTest, getMacroArgExpandedLocationRegression) {
+  const char *main_c_content =
+    "#include \"/modulefoo/foo.h\"\n"
+    "#define FOO234878786(a) (a + a)\n"
+    "const int a = 1;\n"
+    "const int b = FOO234878786(a);\n";
+  std::unique_ptr<llvm::MemoryBuffer> main_c_buffer =
+      llvm::MemoryBuffer::getMemBuffer(main_c_content);
+  const FileEntry *main_c_file = FileMgr.getVirtualFile("/main.c", main_c_buffer->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(main_c_file, std::move(main_c_buffer));
+  FileID main_c_ID = SourceMgr.getOrCreateFileID(main_c_file, clang::SrcMgr::C_User);
+  SourceMgr.setMainFileID(main_c_ID);
+
+  const char *module_map_content =
+    "module foo [system] [extern_c] {\n"
+    "  extern module submodule \"submodule.modulemap\"\n"
+    "  module foo {\n"
+    "    header \"foo.h\"\n"
+    "    export *\n"
+    "  }\n"
+    "}\n";
+  std::unique_ptr<llvm::MemoryBuffer> module_map_buffer = llvm::MemoryBuffer::getMemBuffer(module_map_content);
+  const FileEntry *module_map_file = FileMgr.getVirtualFile("/modulefoo/module.map", module_map_buffer->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(module_map_file, std::move(module_map_buffer));
+
+  const char *foo_h_content = "\n";
+  std::unique_ptr<llvm::MemoryBuffer> foo_h_buffer = llvm::MemoryBuffer::getMemBuffer(foo_h_content);
+  const FileEntry *foo_h_file = FileMgr.getVirtualFile("/modulefoo/foo.h", foo_h_buffer->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(foo_h_file, std::move(foo_h_buffer));
+
+  const char *submodule_h_content = "\n";
+  std::unique_ptr<llvm::MemoryBuffer> submodule_h_buffer = llvm::MemoryBuffer::getMemBuffer(submodule_h_content);
+  const FileEntry *submodule_h_file = FileMgr.getVirtualFile("/modulefoo/submodule.h", submodule_h_buffer->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(submodule_h_file, std::move(submodule_h_buffer));
+
+  const char *submodule_map_content =
+    "module foo.submodule [system] [extern_c] {\n"
+    "  header \"submodule.h\"\n"
+    "  export *\n"
+    "}\n";
+  std::unique_ptr<llvm::MemoryBuffer> submodule_map_buffer = llvm::MemoryBuffer::getMemBuffer(submodule_map_content);
+  const FileEntry *submodule_map_file = FileMgr.getVirtualFile("/modulefoo/submodule.modulemap", submodule_map_buffer->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(submodule_map_file, std::move(submodule_map_buffer));
+
+  TrivialModuleLoader ModLoader;
+  HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
+                          Diags, LangOpts, &*Target);
+
+  Preprocessor PP(std::make_shared<PreprocessorOptions>(), Diags, LangOpts,
+                  SourceMgr, HeaderInfo, ModLoader,
+                  /*IILookup =*/nullptr,
+                  /*OwnsHeaderSearch =*/false);
+
+  // Ensure we can get expanded locations in presence of implicit includes.
+  // These are different than normal includes since predefines buffer doesn't
+  // have a valid insertion location.
+  PP.setPredefines("#include \"/implicit-header.h\"");
+  FileMgr.getVirtualFile("/implicit-header.h", 0, 0);
+  PP.Initialize(*Target);
+  PP.EnterMainSourceFile();
+
+
+  std::vector<Token> toks;
+  while (1) {
+    Token tok;
+    PP.Lex(tok);
+    if (tok.is(tok::eof))
+      break;
+    toks.push_back(tok);
+  }
+ 
+  SourceLocation macroArgLoc = SourceMgr.translateLineCol(main_c_ID, 4, 28);
+  SourceLocation macroArgExpandedLoc = SourceMgr.getMacroArgExpandedLocation(macroArgLoc);
+  
+  EXPECT_EQ(macroArgExpandedLoc, toks[13].getLocation());
+}
+
 namespace {
 
 struct MacroAction {
   enum Kind { kExpansion, kDefinition, kUnDefinition};
 
   SourceLocation Loc;
   std::string Name;
   unsigned MAKind : 3;