[clang] [llvm] [clang][Modules][Serialization] Avoid serializing duplicate source location entries for shared input files (PR #209795)

[Qiongsi Wu via cfe-commits]

qiongsiwu wrote:

Sorry about the delay! 

I think we still have pretty reasonable performance (~0.9% regression during scanning). There are a few more correctness issues I think we should look into. All of these have to do with stale/inconsistent data in the pcm that is reusing source location from other pcms. 

The repros share a setup: mod1 and mod2 both include `shared.h`, and `mod2.h` includes `mod1.h`
after it, so `mod2` is written with `mod1` loaded and reuses mod1's copy. `-DNO_MOD1` turns the
reuse off for comparison. A few data structures in mod2 are created assuming source locations not being shared. 

1.  The preprocessed-entity list is no longer "sorted" by preprocessing order. This has to do with `PPD_ENTITIES_OFFSETS` in mod2's pcm. mod2's entries for `shared.h` stay where they are in the list, in preprocessing order, but now hold locations in mod1's copy. `isBeforeInTranslationUnit` can't relate locations from two module files, so it compares FileIDs. Because mod1 is mod2's import and is always allocated first, these entries compare as after everything in mod2. The list that `ASTReader::findPreprocessedEntity` binary-searches is no longer sorted. Reproducer:
```
// RUN: rm -rf %t && split-file %s %t

// mod2.h includes mod1.h, so mod2 is written with mod1 loaded and reuses
// mod1's copy of shared.h.
// RUN: c-index-test -cursor-at=%t/mod2.h:5:9 -cursor-at=%t/mod2.h:6:23 \
// RUN:   %t/tu.c -fmodules -fimplicit-module-maps \
// RUN:   -fmodules-cache-path=%t/mc-reuse -I%t | FileCheck %s

// With -DNO_MOD1, mod2 doesn't load mod1 and reuses nothing.
// RUN: c-index-test -cursor-at=%t/mod2.h:5:9 -cursor-at=%t/mod2.h:6:23 \
// RUN:   %t/tu.c -fmodules -fimplicit-module-maps \
// RUN:   -fmodules-cache-path=%t/mc-control -I%t -DNO_MOD1 | FileCheck %s

// CHECK: 5:9 macro definition=M2_SIZE
// CHECK: 6:23 macro expansion=M2_SIZE:5:9

//--- module.modulemap
module mod1 { header "mod1.h" export * }
module mod2 { header "mod2.h" export * }

//--- shared.h
#ifndef SHARED_H
#define SHARED_H
#define SHARED_1 1
#define SHARED_2 2
#define SHARED_3 3
#define SHARED_4 4
#endif

//--- mod1.h
#include "shared.h"

//--- mod2.h
#include "shared.h"
#ifndef NO_MOD1
#include "mod1.h"
#endif
#define M2_SIZE 4
extern int mod2_array[M2_SIZE];

//--- tu.c
#include "mod2.h"
```
2. Include locations into a reused header are dropped. Specifically, the include location in `SM_SLOC_FILE_ENTRY`. A `getAffectingIncludeLoc` writes an empty include location when the including file isn't "affecting", and a reused header seems to be non "affecting". Reproducer:
```
// RUN: rm -rf %t && split-file %s %t

// mod2.h includes mod1.h, so mod2 is written with mod1 loaded and reuses
// mod1's copy of shared.h. x.h is reached only through mod2's shared.h.
// RUN: c-index-test -cursor-at=%t/x.h:1:5 %t/tu.c -fmodules \
// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t/mc-reuse -I%t \
// RUN:   | FileCheck %s --check-prefix=CURSOR
// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
// RUN:   -fmodules-cache-path=%t/mc-reuse-cc1 -I%t \
// RUN:   -fdiagnostics-show-note-include-stack %t/tu.c 2>&1 \
// RUN:   | FileCheck %s --check-prefix=NOTE

// With -DNO_MOD1, mod2 doesn't load mod1 and reuses nothing.
// RUN: c-index-test -cursor-at=%t/x.h:1:5 %t/tu.c -fmodules \
// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t/mc-control -I%t \
// RUN:   -DNO_MOD1 | FileCheck %s --check-prefix=CURSOR
// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps \
// RUN:   -fmodules-cache-path=%t/mc-control-cc1 -I%t -DNO_MOD1 \
// RUN:   -fdiagnostics-show-note-include-stack %t/tu.c 2>&1 \
// RUN:   | FileCheck %s --check-prefix=NOTE

// CURSOR: 1:5 FunctionDecl=x_fn:1:5
// NOTE: In module 'mod2' imported from {{.*}}tu.c:1:
// NOTE-NEXT: {{.*}}x.h:1:{{[0-9]+}}: note: 'x_fn' has been explicitly marked deprecated here

//--- module.modulemap
module mod1 { header "mod1.h" export * }
module mod2 { header "mod2.h" export * }

//--- shared.h
#ifndef SHARED_H
#define SHARED_H
int shared_fn(void);
#ifdef WANT_X
int extra_fn(void);
#include "x.h"
#endif
#endif

//--- x.h
int x_fn(void) __attribute__((deprecated));

//--- mod1.h
#include "shared.h"

//--- mod2.h
#define WANT_X 1
#include "shared.h"
#ifndef NO_MOD1
#include "mod1.h"
#endif

//--- tu.c
#include "mod2.h"
int use(void) { return x_fn() + extra_fn(); }
```
3. Declarations in a reused header are missing from the per-file declaration table. Specifically,  `FILE_SORTED_DECLS`, and each file's slice of it in `SM_SLOC_FILE_ENTRY` are not set quite right. `associateDeclWithFile` skips a reused file, on the assumption that mod1's table covers it. But mod1's table only has mod1's declarations. A declaration that only mod2 has in `shared.h` (`extra_fn` above, behind `WANT_X`) is in neither table.

It is a bit more complicated to reproduce this one. To reproduce it, use the files we used for 2 (or just reuse that directory), and we need to create a program that links with libclang. There does not seem to be a good way to lit test this. So a proper test should probably go to clang's unit tests. Build this program that links with libclang to trigger the issue: 
```c
// usage: decl-roundtrip <file> <line> <col> <clang args...>
// Finds the declaration referenced at <file>:<line>:<col>, then asks for the
// cursor at that declaration's own location.
#include <clang-c/Index.h>
#include <stdio.h>
#include <stdlib.h>

static void show(const char *What, CXCursor C) {
  CXString Kind = clang_getCursorKindSpelling(clang_getCursorKind(C));
  CXString Name = clang_getCursorSpelling(C);
  CXFile File;
  unsigned Line, Col;
  clang_getSpellingLocation(clang_getCursorLocation(C), &File, &Line, &Col,
                            NULL);
  CXString FileName = clang_getFileName(File);
  printf("%s: %s=%s at %s:%u:%u\n", What, clang_getCString(Kind),
         clang_getCString(Name), File ? clang_getCString(FileName) : "<none>",
         Line, Col);
  clang_disposeString(Kind);
  clang_disposeString(Name);
  clang_disposeString(FileName);
}

int main(int argc, const char **argv) {
  CXIndex Idx = clang_createIndex(0, 0);
  CXTranslationUnit TU;
  if (clang_parseTranslationUnit2(Idx, argv[1], argv + 4, argc - 4, NULL, 0,
                                  CXTranslationUnit_None, &TU))
    return 1;
  CXSourceLocation Loc = clang_getLocation(
      TU, clang_getFile(TU, argv[1]), atoi(argv[2]), atoi(argv[3]));
  CXCursor Use = clang_getCursor(TU, Loc);
  CXCursor Decl = clang_getCursorReferenced(Use);
  show("use", Use);
  show("decl", Decl);
  show("back", clang_getCursor(TU, clang_getCursorLocation(Decl)));
  return 0;
}
```
Build and run:

```sh
# please replace the llvm project path and the build path to the local working source tree/build directory.
clang decl-roundtrip.c -I<llvm-project>/clang/include -L<build>/lib -lclang \
  -Wl,-rpath,<build>/lib -o decl-roundtrip
./decl-roundtrip tu.c 2 33 -fmodules -fimplicit-module-maps -fmodules-cache-path=mc -I.
```
Results of running the program: 
Correct case (main, or the PR with -DNO_MOD1):
```
use: DeclRefExpr=extra_fn at tu.c:2:33
decl: FunctionDecl=extra_fn at ./shared.h:5:5
back: FunctionDecl=extra_fn at ./shared.h:5:5
```

Wrong case (the PR, with reuse):
```
use: DeclRefExpr=extra_fn at tu.c:2:33
decl: FunctionDecl=extra_fn at ./shared.h:5:5
back: NoDeclFound= at <none>:0:0
```

Could you take a look? I think this list contains all the correctness issues I am aware of. Claude assisted in the analysis and test case generation. 

https://github.com/llvm/llvm-project/pull/209795